Skip to content

fix: mark enable_toolset and disable_toolset as non-destructive - #29

Open
imbenrabi wants to merge 2 commits into
mainfrom
claude/project-thread-gpt0wt
Open

imbenrabi wants to merge 2 commits into
mainfrom
claude/project-thread-gpt0wt

Conversation

@imbenrabi

@imbenrabi imbenrabi commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Requested by Ben · project thread

Before: enable_toolset and disable_toolset were advertised to MCP clients with destructiveHint: true, so clients could treat turning a toolset on or off as a data-destroying action and prompt for confirmation.

After: both advertise destructiveHint: false, idempotentHint: true. In the MCP spec destructiveHint means the tool may delete or overwrite data. These tools only change which tools are exposed, one reverses the other, and repeating either has no further effect on state.

How: two annotation values in src/meta/registerMetaTools.ts, a new annotations test block in tests/metaTools.test.ts (fails on the old values), and the matching invariant line in src/meta/AGENTS.md. Nothing else changes, so it merges cleanly with the sibling PR that changes disable_toolset's removal behaviour. No CHANGELOG entry: 0.7.0 is already released and the release PR adds the next entry. Unchanged by design: openWorldHint still defaults to true per spec for these tools; that is outside this bug.

Base: main. No stacking, any merge order works.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JZj3jC148r1VwBYogZpWKL

Both meta-tools were annotated destructiveHint: true, but neither deletes
anything: they change which tools are exposed and can be reversed by the
other. Set destructiveHint: false, keep idempotentHint: true, assert the
annotations in tests and update the meta intent node.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JZj3jC148r1VwBYogZpWKL
@imbenrabi imbenrabi self-assigned this Oct 10, 2026
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JZj3jC148r1VwBYogZpWKL

Copy link
Copy Markdown
Contributor Author

TS review: pipeline-ts-reviewer (feature-pipeline)

Verdict: APPROVE. No CRITICAL or HIGH findings. Reviewed at head 2381fd7+PR commit (before the docs wording fix below).

Reviewed: the 3-file diff against main (src/meta/registerMetaTools.ts, src/meta/AGENTS.md, tests/metaTools.test.ts) with surrounding context (DynamicToolManager, tests/helpers/fakes.ts). Static analysis only, against typescript-standards and karpathy-guidelines.

# Sev Location Finding
1 LOW tests/metaTools.test.ts (new "discovery meta-tools as read-only" test) Pins annotations this PR doesn't change; a cheap regression guard, marginally outside "only the two flipped annotations".
2 LOW src/meta/AGENTS.md:23 "calling either again is a no-op" is true for state but not for the response (a repeat disable_toolset returns success: false). Suggested "no further effect on state".
{
  "recommendation": "APPROVE",
  "summary": "Surgical, correct annotation fix for enable_toolset/disable_toolset with matching doc update and strict regression tests; only minor wording and scope notes.",
  "findings": [
    { "severity": "LOW", "file": "tests/metaTools.test.ts", "issue": "Discovery read-only annotation test covers behaviour this PR does not change; acceptable as a regression guard." },
    { "severity": "LOW", "file": "src/meta/AGENTS.md", "line": 23, "issue": "'no-op' holds for state only; consider 'no further effect on state'." }
  ]
}

Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Fixes for the TS review, and re-review

Mapping to the review comment above:

# Finding Outcome
1 LOW: discovery read-only test is slightly outside the two flipped annotations Left unchanged. It guards the invariant the updated AGENTS.md line states (discovery tools stay readOnlyHint: true), costs nine lines, and sits in the same new annotations block.
2 LOW: "calling either again is a no-op" is true for state, not for the response Fixed in f1becfd. src/meta/AGENTS.md now says "repeating either call has no further effect on state".

Re-review of the head (f1becfd): APPROVE, no findings. The reviewer checked the new wording against DynamicToolManager (a repeat enable leaves activeToolsets unchanged; a repeat disable returns success: false and changes nothing) and accepted the kept test as a deliberate regression guard.

{
  "recommendation": "APPROVE",
  "summary": "The annotation change is surgical and correct, the AGENTS.md wording now accurately describes state-level idempotence, and the tests cover the stated invariant.",
  "findings": []
}

Generated by Claude Code

@imbenrabi imbenrabi mentioned this pull request Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants