Skip to content

fix(3468):Tool update resets visibility to public when field not provided#3503

Merged
crivetimihai merged 6 commits intoIBM:mainfrom
KKNithin:issue-3468-tool-update
Mar 7, 2026
Merged

fix(3468):Tool update resets visibility to public when field not provided#3503
crivetimihai merged 6 commits intoIBM:mainfrom
KKNithin:issue-3468-tool-update

Conversation

@KKNithin
Copy link
Copy Markdown
Collaborator

@KKNithin KKNithin commented Mar 5, 2026

🔗 Related Issue

Closes #3468

📝 Summary

Added logic to handle visibility when not provided

🏷️ Type of Change

  • Bug fix
  • Feature / Enhancement
  • Documentation
  • Refactor
  • Chore (deps, CI, tooling)
  • Other (describe below)

✅ Checklist

  • Code formatted (make black isort pre-commit)
  • Tests added/updated for changes
  • Documentation updated (if applicable)
  • No secrets or credentials committed

@KKNithin KKNithin force-pushed the issue-3468-tool-update branch from a87cf56 to cf8deea Compare March 5, 2026 14:57
@KKNithin KKNithin closed this Mar 5, 2026
@KKNithin KKNithin reopened this Mar 5, 2026
@KKNithin KKNithin marked this pull request as ready for review March 5, 2026 15:28
@KKNithin KKNithin changed the title Issue 3468 tool update fix(3468):Tool update resets visibility to public when field not provided Mar 5, 2026
@jonpspri
Copy link
Copy Markdown
Collaborator

jonpspri commented Mar 5, 2026

Note — additional fix bundled in this PR:

Besides the visibility-reset bug (#3468), commit 4bf8dcbf9 also fixes an auth_type wipe bug: previously, when tool_update.auth was None (i.e. the caller didn't provide auth fields), the else branch set tool.auth_type = None, silently erasing existing authentication credentials on the tool.

The fix removes that else branch so omitting auth in an update payload now correctly preserves the existing auth_type and auth_value. A dedicated test (test_update_tool_auth_not_reset_when_omitted) covers this.

@jonpspri jonpspri force-pushed the issue-3468-tool-update branch 2 times, most recently from e5bc346 to 2a64719 Compare March 5, 2026 21:52
@jonpspri jonpspri added the release-fix Critical bugfix required for the release label Mar 6, 2026
Nithin Katta and others added 5 commits March 7, 2026 00:22
@crivetimihai
fix(IBM#3468): handle visibility in tool update conflict checks
60aac51 
Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
@crivetimihai
fix(IBM#3468): remove default value from visibility field in tool upd…
63c3b9e 
…ate schema

Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
@crivetimihai
test(IBM#3468): update tool visibility conflict check tests to use mo…
5367e34 
…ck_tool fixture

Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
@jonpspri @crivetimihai
fix(IBM#3468): harden tool update to preserve omitted fields and chec…
12ba760 
…k visibility conflicts

- Change ToolUpdate.visibility default from "public" to None so omitting
  the field preserves the existing value instead of resetting to public.
- Remove client-settable team_id/owner_email from ToolUpdate; derive
  ownership fields from the DB record for conflict checks.
- Extract _check_tool_name_conflict() to handle public/team/private
  scoped uniqueness checks in a single reusable method.
- Add conflict checking when visibility changes without a name change.
- Fix custom_name_ref fallback to use existing custom_name when it
  diverges from name (instead of incorrectly using the new name).
- Remove the else branch that wiped auth_type when auth was not provided.

Closes IBM#3468

Signed-off-by: Jonathan Springer <jps@s390x.com>
@jonpspri @crivetimihai
fix(IBM#3468): add visibility value validation and warning log for in…
f62e6cb 
…complete conflict checks

- Change ToolUpdate.visibility from Optional[str] to
  Optional[Literal["private", "team", "public"]] so Pydantic rejects
  invalid values at the schema boundary.
- Add a warning log in _check_tool_name_conflict when a team/private
  visibility is missing the required team_id/owner_email, making
  skipped conflict checks observable.

Signed-off-by: Jonathan Springer <jps@s390x.com>
@crivetimihai crivetimihai force-pushed the issue-3468-tool-update branch from 2a64719 to f62e6cb Compare March 7, 2026 00:50
@crivetimihai
Copy link
Copy Markdown
Member

crivetimihai commented Mar 7, 2026

Review Summary

Rebased onto main — clean, no conflicts. Reviewed code, ran unit tests (all 307 pass), verified 100% differential test coverage, and performed live end-to-end testing against localhost:8080 (Docker Compose with PostgreSQL, nginx, 3 gateway replicas).

Findings

Correctness — All logic is sound:

  • Schema fix: ToolUpdate.visibility changed from default="public" to None with Literal validation.
  • Conflict check refactored into _check_tool_name_conflict() — handles public/team/private scopes correctly.
  • custom_name_ref resolution correctly handles diverged custom names.
  • Combined name+visibility changes use the target visibility for conflict checking.
  • Auth preservation: removed the else: tool.auth_type = None bug that wiped credentials.

Security — Improves security:

  • Derives team_id/owner_email from DB records, not client input (CLAUDE.md invariant).
  • Literal type constraint rejects invalid visibility values at schema boundary (HTTP 422).
  • The old code's tool_update.team_id reference was a latent crash bug (field doesn't exist on ToolUpdate).

Consistency — Aligns with resource_service.py and prompt_service.py patterns. The Literal constraint is stricter than those services (which use Optional[str]) — an improvement.

Test Coverage — 100% differential coverage. 14 new tests + helper covering all new code paths.

Performance — No additional DB queries introduced.

Live E2E Test Results

Test Result
Create private tool, update desc without visibility → stays private PASS
Update public tool desc without visibility → stays public PASS
Invalid visibility value "invalid_value" → HTTP 422 PASS
Auth (bearer) preserved after update without auth field PASS
Cross-scope rename (private → same name as public tool) → allowed PASS
Same-scope name conflict (public → existing public name) → rejected PASS
Admin UI: edit form pre-selects correct visibility radio PASS
Admin UI: save description change → visibility preserved PASS

LGTM.

crivetimihai
crivetimihai previously approved these changes Mar 7, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@crivetimihai crivetimihai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved — rebased, reviewed, unit-tested (307 pass), and E2E-verified against live deployment. All visibility preservation, conflict detection, auth preservation, and schema validation scenarios confirmed working.

@crivetimihai
fix(IBM#3468): label private conflicts correctly in ToolNameConflictE…
16f52d5 
…rror

ToolNameConflictError formatted private-scope conflicts as "Public"
because the else branch caught both public and private visibility.
This path is newly reachable since the PR adds private conflict
detection. Add an explicit "private" branch and a test.

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
@crivetimihai crivetimihai mentioned this pull request Mar 7, 2026
Open
@crivetimihai crivetimihai self-assigned this Mar 7, 2026
@crivetimihai crivetimihai added this to the Release 1.0.0-RC2 milestone Mar 7, 2026
@crivetimihai crivetimihai merged commit fd31814 into IBM:main Mar 7, 2026
39 checks passed
MohanLaksh pushed a commit that referenced this pull request Mar 12, 2026
@KKNithin @jonpspri
@crivetimihai @MohanLaksh
fix(3468):Tool update resets visibility to public when field not prov…
327e2fb 
…ided (#3503)

* fix(#3468): handle visibility in tool update conflict checks

Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

* fix(#3468): remove default value from visibility field in tool update schema

Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

* test(#3468): update tool visibility conflict check tests to use mock_tool fixture

Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

* fix(#3468): harden tool update to preserve omitted fields and check visibility conflicts

- Change ToolUpdate.visibility default from "public" to None so omitting
  the field preserves the existing value instead of resetting to public.
- Remove client-settable team_id/owner_email from ToolUpdate; derive
  ownership fields from the DB record for conflict checks.
- Extract _check_tool_name_conflict() to handle public/team/private
  scoped uniqueness checks in a single reusable method.
- Add conflict checking when visibility changes without a name change.
- Fix custom_name_ref fallback to use existing custom_name when it
  diverges from name (instead of incorrectly using the new name).
- Remove the else branch that wiped auth_type when auth was not provided.

Closes #3468

Signed-off-by: Jonathan Springer <jps@s390x.com>

* fix(#3468): add visibility value validation and warning log for incomplete conflict checks

- Change ToolUpdate.visibility from Optional[str] to
  Optional[Literal["private", "team", "public"]] so Pydantic rejects
  invalid values at the schema boundary.
- Add a warning log in _check_tool_name_conflict when a team/private
  visibility is missing the required team_id/owner_email, making
  skipped conflict checks observable.

Signed-off-by: Jonathan Springer <jps@s390x.com>

* fix(#3468): label private conflicts correctly in ToolNameConflictError

ToolNameConflictError formatted private-scope conflicts as "Public"
because the else branch caught both public and private visibility.
This path is newly reachable since the PR adds private conflict
detection. Add an explicit "private" branch and a test.

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

---------

Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
Signed-off-by: Jonathan Springer <jps@s390x.com>
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Co-authored-by: Nithin Katta <Nithin.Katta@ibm.com>
Co-authored-by: Jonathan Springer <jps@s390x.com>
Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
Yosiefeyob pushed a commit that referenced this pull request Mar 13, 2026
@KKNithin @jonpspri
@crivetimihai @Yosiefeyob
fix(3468):Tool update resets visibility to public when field not prov…
74bcf88 
…ided (#3503)

* fix(#3468): handle visibility in tool update conflict checks

Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

* fix(#3468): remove default value from visibility field in tool update schema

Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

* test(#3468): update tool visibility conflict check tests to use mock_tool fixture

Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

* fix(#3468): harden tool update to preserve omitted fields and check visibility conflicts

- Change ToolUpdate.visibility default from "public" to None so omitting
  the field preserves the existing value instead of resetting to public.
- Remove client-settable team_id/owner_email from ToolUpdate; derive
  ownership fields from the DB record for conflict checks.
- Extract _check_tool_name_conflict() to handle public/team/private
  scoped uniqueness checks in a single reusable method.
- Add conflict checking when visibility changes without a name change.
- Fix custom_name_ref fallback to use existing custom_name when it
  diverges from name (instead of incorrectly using the new name).
- Remove the else branch that wiped auth_type when auth was not provided.

Closes #3468

Signed-off-by: Jonathan Springer <jps@s390x.com>

* fix(#3468): add visibility value validation and warning log for incomplete conflict checks

- Change ToolUpdate.visibility from Optional[str] to
  Optional[Literal["private", "team", "public"]] so Pydantic rejects
  invalid values at the schema boundary.
- Add a warning log in _check_tool_name_conflict when a team/private
  visibility is missing the required team_id/owner_email, making
  skipped conflict checks observable.

Signed-off-by: Jonathan Springer <jps@s390x.com>

* fix(#3468): label private conflicts correctly in ToolNameConflictError

ToolNameConflictError formatted private-scope conflicts as "Public"
because the else branch caught both public and private visibility.
This path is newly reachable since the PR adds private conflict
detection. Add an explicit "private" branch and a test.

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

---------

Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
Signed-off-by: Jonathan Springer <jps@s390x.com>
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Co-authored-by: Nithin Katta <Nithin.Katta@ibm.com>
Co-authored-by: Jonathan Springer <jps@s390x.com>
Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
Signed-off-by: Yosief Eyob <yosiefogbazion@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crivetimihai crivetimihai crivetimihai left review comments

@kevalmahajan kevalmahajan Awaiting requested review from kevalmahajan kevalmahajan is a code owner

@madhav165 madhav165 Awaiting requested review from madhav165 madhav165 is a code owner

Assignees

@crivetimihai crivetimihai

Labels

release-fix Critical bugfix required for the release

Projects

None yet

Milestone

Release 1.0.0-RC2

Development

Successfully merging this pull request may close these issues.

[BUG][API]: Tool update resets visibility to public when field not provided

3 participants

@KKNithin @jonpspri @crivetimihai