fix: disable_toolset unregisters the toolset's tools

[imbenrabi]

Requested by Ben · project thread

Before: disable_toolset only flipped the toolset's state. Its tools stayed listed and callable, and the docs said the MCP SDK could not remove tools.

After: disabling a toolset removes its tools from the MCP server, so clients no longer list or call them. The toolset can be enabled again. A partly failed enable also rolls back the tools it had registered, instead of leaving them behind.

The MCP SDK's registerTool returns a RegisteredTool with remove(), so the "SDK can't unregister" claim was outdated.

How

• DynamicToolManager keeps the RegisteredTool handle of every tool it registers, per toolset. disableToolset (and the rollback in enableToolset) calls remove() on them through a new private removeToolsetTools. This replaces handlePartialFailure and the registeredTools array, which only logged a warning.
• ToolRegistry.removeForToolset(key) frees the tool names, so a re-enable doesn't hit a name collision.
• The disable_toolset description, README, AGENTS.md and src/core/AGENTS.md no longer say disable is state-only. The disable message now ends with Removed N tools.
• CHANGELOG.md is untouched, because the release PR writes the entry.

Tests

npm run typecheck, npm run build and npx vitest run pass (337 tests). With only src/ reverted, the 7 new or extended tests fail. They cover:

• unit tests against a real McpServer over an in-memory transport: the tool is unlisted and uncallable after disable, re-enable works, only the disabled toolset's tools go, and a partly failed enable is rolled back
• ToolRegistry.removeForToolset
• the DYNAMIC E2E "disables toolsets" test, which now checks the tool is gone

Impact on the FMP server

I packed this branch and ran npm run verify:toolception -- --source <tarball> in the server: 25 of 26 checks pass. The one failure is the server's own smoke test should disable toolset (state only, tools remain) (__tests__/smoke/smoke-test-dynamic.test.ts), which asserts the old behaviour. A server PR that makes it accept both behaviours has to merge before this release, per the cross-repo order. The server's docs that say "state tracking only" should change together with the dependency bump.

Merge order and conflicts

This PR is off main and has no stack. Another thread is changing destructiveHint on the same meta-tools in src/meta/registerMetaTools.ts. The disable_toolset description line here sits right next to the annotations line, so a trivial textual conflict is possible, whichever merges second. Nothing is tagged or published.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GKeDY2JCSBC6vG56EEKziV

---

Generated by Claude Code

[imbenrabi]

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

Verdict: APPROVE (round 1, head 4ec564a). No CRITICAL, HIGH or MEDIUM findings; 8 LOW.

Reviewed: the full diff against main (source, docs, tests) plus the surrounding code, against typescript-standards, karpathy-guidelines and the repo's AGENTS.md style rules. Static analysis only. It also read the SDK 1.32.1 _createRegisteredTool to confirm remove() is idempotent and that a duplicate name throws before a handle is returned.

#	Sev	Location	Finding
1	LOW	src/core/DynamicToolManager.ts:311, tests/dynamicToolManager.test.ts	"Removed 1 tools" is ungrammatical for one tool, and a test pins it
2	LOW	src/core/DynamicToolManager.ts:270	Handle list is rebuilt with a spread and typed as a mutable array; use a readonly type
3	LOW	src/core/ToolRegistry.ts:58	removeForToolset returns names that no production caller uses
4	LOW	src/core/AGENTS.md invariant 6	The SDK's remove() and registerTool() each send tools/list_changed, so invariant 6 (one batched notification) needs a caveat
5	LOW	tests/dynamicToolManager.test.ts, tests/e2e/dynamicMode.e2e.test.ts	"Uncallable" is asserted as "rejected or isError", which passes on any failure
6	LOW	tests/dynamicToolManager.test.ts	The rollback test never re-enables after the failed enable
7	LOW	tests/dynamicToolManager.test.ts	The third new test duplicates the connectManager setup
8	LOW	tests/createMcpServer.test.ts:53	That fake registerTool returns undefined; safe today because its catalog has no tools (informational)

{
  "recommendation": "APPROVE",
  "summary": "Removal via stored RegisteredTool handles is correctly ordered, rolls back partial enables, frees registry names so re-enable works, and docs and tests match; only minor LOW nits remain.",
  "findings": [
    { "severity": "LOW", "file": "src/core/DynamicToolManager.ts", "line": 311, "issue": "Message 'Removed N tools' is ungrammatical for N=1 and the test pins 'Removed 1 tools'." },
    { "severity": "LOW", "file": "src/core/DynamicToolManager.ts", "line": 270, "issue": "Handle list is rebuilt via spread on each registration and typed as mutable RegisteredTool[]; use readonly array type or set the list once." },
    { "severity": "LOW", "file": "src/core/ToolRegistry.ts", "line": 58, "issue": "removeForToolset returns removed names that no production caller uses (only the unit test)." },
    { "severity": "LOW", "file": "src/core/AGENTS.md", "line": 20, "issue": "SDK remove() and registerTool() each emit their own tools/list_changed; invariant 6 could carry a caveat." },
    { "severity": "LOW", "file": "tests/dynamicToolManager.test.ts", "line": 284, "issue": "'Uncallable' assertion is 'rejected OR isError', which passes on any failure; assert the specific not-found error (same in tests/e2e/dynamicMode.e2e.test.ts)." },
    { "severity": "LOW", "file": "tests/dynamicToolManager.test.ts", "line": 325, "issue": "Rollback test does not re-enable after the failed enable." },
    { "severity": "LOW", "file": "tests/dynamicToolManager.test.ts", "line": 305, "issue": "Third test duplicates connectManager setup; parametrize the helper with a catalog." },
    { "severity": "LOW", "file": "tests/createMcpServer.test.ts", "line": 53, "issue": "Fake registerTool returns undefined; would break remove() if a user tool were registered through it." }
  ]
}

---

Generated by Claude Code

[imbenrabi]

Fixes for the round 1 TS review

Fixes are in 0b4a411. Finding numbers follow the review's table.

#	Result	Detail
1	Left unchanged	"Removed N tools" matches the existing enable message ("Registered N tools."), and clients parse the JSON result, not the sentence. Not worth a plural helper.
2	Fixed	toolHandles is now Map<string, readonly RegisteredTool[]>; the copy-on-write is intentional.
3	Fixed	ToolRegistry.removeForToolset returns void. Its unit test now checks has, list and listByToolset instead of the return value.
4	Fixed	src/core/AGENTS.md invariant 6 now notes that the SDK also sends tools/list_changed per registerTool() and remove(), so a disable of N tools sends N+1.
5	Fixed	Both the unit and E2E tests now assert isError plus a "not found" message instead of "rejected or isError".
6	Fixed	The rollback test now removes the duplicate tool and re-enables the toolset, checking that the rolled-back names are free.
7	Fixed	connectManager takes a catalog, and the third test uses it.
8	Left unchanged	Informational. That fake's catalog registers no tools, so remove() is never reached.

npm run typecheck, npm run build and npx vitest run pass (337 tests). Re-running the reviewer on the new head next.

---

Generated by Claude Code

[imbenrabi]

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

Verdict: APPROVE (head 0b4a411). No CRITICAL, HIGH or MEDIUM findings; 3 LOW.

Reviewed: the round 2 delta and the full current change against main, plus ModuleResolver and the SDK 1.32.1 tools/call handler to check what the new tests assume. Static analysis only.

Fix check (my fix mapping): #2 to #7 accepted as fixed. #1 and #8 accepted as left unchanged. It confirmed that an unknown tool name gives isError: true with a "not found" message, so the specific assertions are accurate, and that the rollback test exercises a real partial failure.

Sev	Location	Finding	My disposition
LOW	tests/dynamicToolManager.test.ts (connectManager)	Record<string, unknown> plus as any is loose typing; a narrower catalog type would remove the cast	Left unchanged: optional, test-only, and it matches the file's existing as any catalogs
LOW	tests/dynamicToolManager.test.ts (rollback test)	tools.pop() relies on ModuleResolver keeping the catalog by reference; a comment would protect it	Left unchanged: optional. If a defensive copy is ever added, this test fails loudly, which is the signal
LOW	src/core/DynamicToolManager.ts (disable message)	"Removed 1 tools" reads oddly	Left unchanged: same as round 1 #1

{
  "recommendation": "APPROVE",
  "summary": "Round 2 fixes are correct and the deferred items are reasonable; the not-found and rollback test assertions check out against SDK 1.32.1 and the resolver, and the delta introduced no regressions.",
  "findings": [
    { "severity": "LOW", "file": "tests/dynamicToolManager.test.ts", "line": null, "issue": "connectManager takes Record<string, unknown> and casts the catalog with `as any`; a narrower catalog type would remove the cast (optional, matches existing file style)." },
    { "severity": "LOW", "file": "tests/dynamicToolManager.test.ts", "line": null, "issue": "Rollback test depends on ModuleResolver keeping the catalog tools array by reference (tools.pop() mutation); a short comment would protect against a future defensive copy." },
    { "severity": "LOW", "file": "src/core/DynamicToolManager.ts", "line": null, "issue": "Disable message 'Removed N tools' reads oddly for N=1; consistent with existing 'Registered N tools.' and accepted as disposition #1." }
  ]
}

The reviewer has approved with no blocking findings, so the cycle is complete.

---

Generated by Claude Code