fix: reject STATIC clients whose startup toolsets failed to load

[imbenrabi]

Requested by Ben · project thread

Before: when a startup toolset fails while registering its tools for a client (for example a tool name collision), the client still connects with whatever tools did register. ServerOrchestrator.isReady() reports true in that case, and FastifyTransport ignores the result anyway. This is follow-up 1 from the review on #27 (#27 (comment)).

After: the client's initialize gets a JSON-RPC -32603 error (HTTP 500, "Failed to load toolsets for this client.") and no session is created. The failed bundle is dropped from the cache, so the client's next initialize retries the load instead of hitting the cached failure for up to an hour. isReady() now returns false, and ensureReady() throws, when registering a startup toolset's tools fails.

Two findings that shape the fix:

• DynamicToolManager.enableToolsets() catches per-toolset errors and returns { success: false, results }. It never throws, so the catch in initializeToolsets() was effectively unreachable and isReady() was always true. The review comment assumed it already returned false. It only does now.
• ModuleResolver logs a throwing module loader and carries on (there's a test for that), so a loader that throws still yields a toolset with zero tools and is not treated as a failure. I left that alone because changing it also changes enable_toolset in DYNAMIC mode. If you want loader errors to fail the client too, that is a separate decision.

Breaking, and needs an FMP server fix first. The base orchestrator uses the same code, so createMcpServer in STATIC mode now fails at startup (the existing fail-fast ensureReady()) when registering a startup toolset's tools fails, where it used to start with the partial set. Toolsets refused before registration (exposure policy, duplicate names in startup.toolsets) don't count. npm run verify:toolception on this branch packed gives 19/21 for the FMP server: ALL_TOOLS, the default mode, fails to start. The server's earnings and calendar toolsets both include the earnings-transcript module, so getLatestEarningsTranscripts collides. Today that collision is swallowed and earnings is left inactive. The server's catalog has to be fixed and merged before it takes a toolception release with this change. Shipping it strict was confirmed by Ben on a decision card in the project thread (2026-10-10).

How: enable results now carry a code when registering tools fails; initializeToolsets() records initError only for failed results that have one. FastifyTransport checks the isReady() result before connect(). Intent nodes (core, http, server) and an Unreleased changelog entry (Breaking and Fixed) are updated.

Tests (the new ones fail on main, pass here):

• serverOrchestrator: not ready on a tool name collision; ready when the exposure policy refuses a toolset, when a startup toolset is listed twice, and when all enable.
• fastifyTransport: initialize returns the JSON-RPC error, never calls connect, and a second initialize builds a fresh bundle.
• STATIC e2e with a real SDK client: a client whose loader returns a duplicate tool name is rejected, a healthy client keeps working, and a client that failed once connects on retry.
• npm run typecheck, npm run build and npm run test:run pass (385 tests, with main merged in).

Coordination with #30 (permission-aware transport). #30 has merged. PermissionAwareFastifyTransport still ignores the isReady() result, and this PR deliberately doesn't touch that file. The isReady() check on that transport is owned by the 0.8.0 release thread. It is not part of this PR.

Base: main (merged at 9e7e8a0; the only conflict was the CHANGELOG.md [Unreleased] section, now holding both entries). Release as a minor (0.8.0) after the FMP catalog fix. No tag or publish from this thread.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UJfs8sWB5EK8dXXjDU7Mv3

[imbenrabi]

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

Verdict (round 1, head 20e7ba2): APPROVE / GO. No CRITICAL or HIGH findings. Two MEDIUM items and five LOW items. JSON recommendation: COMMENT.

Reviewed: the full diff against main, plus ServerOrchestrator.ts, DynamicToolManager.ts, FastifyTransport.ts, ClientResourceCache.ts, createMcpServer.ts, createPermissionBasedMcpServer.ts, ModuleResolver.ts and the core, http, server, session and mode intent nodes. Static analysis only.

#	Sev	Location	Finding
1	MEDIUM	src/core/ServerOrchestrator.ts:86-101	Duplicate names in startup.toolsets (for example ["core","core"]) are now fatal. The second enable returns "already enabled", which used to be ignored. Dedupe the names or ignore that case, and add a test.
2	MEDIUM	ServerOrchestrator.ts:94-101, createMcpServer.ts:34-36, CHANGELOG.md:7-13	toolsets: "ALL" with an exposure policy (allowlist, denylist, maxActiveToolsets) used to serve only the permitted toolsets. It now makes createMcpServer() reject at startup. The changelog files this under Fixed without saying so.
3	LOW	src/http/FastifyTransport.ts:243-259	No negative caching: each initialize with a failing config rebuilds the bundle and re-runs the module loaders. Intended retry design; document it.
4	LOW	tests/serverOrchestrator.test.ts:285-299	console.error spy is restored after the assertions, so it leaks if one fails. Use try/finally.
5	LOW	tests/fastifyTransport.test.ts:220-275	The identity guard (get(cacheKey) === bundle) is untested, and the retry's status code is not asserted.
6	LOW	tests/e2e/staticMode.e2e.test.ts:140	ctx: any. Prefer unknown with a guard.
7	LOW	ServerOrchestrator.ts:86-91	Nested ternary for names; a small helper would read better. Style only.

{
  "recommendation": "COMMENT",
  "summary": "The fix is correct and well scoped, the identity-guarded eviction is sound, and the tests cover the failure and retry paths. Two MEDIUM items remain: duplicate startup toolset names now fail fatally, and ALL combined with an exposure policy now fails server startup without an explicit CHANGELOG callout.",
  "findings": [
    { "severity": "MEDIUM", "file": "src/core/ServerOrchestrator.ts", "line": 86, "issue": "Duplicate startup toolset names are fatal now" },
    { "severity": "MEDIUM", "file": "CHANGELOG.md", "line": 7, "issue": "ALL plus exposure policy now rejects createMcpServer at startup; entry is under Fixed and does not say so" },
    { "severity": "LOW", "file": "src/http/FastifyTransport.ts", "line": 243, "issue": "No negative caching; document" },
    { "severity": "LOW", "file": "tests/serverOrchestrator.test.ts", "line": 285, "issue": "spy restored after assertions" },
    { "severity": "LOW", "file": "tests/fastifyTransport.test.ts", "line": 270, "issue": "identity guard untested; retry status not asserted" },
    { "severity": "LOW", "file": "tests/e2e/staticMode.e2e.test.ts", "line": 140, "issue": "ctx typed any" },
    { "severity": "LOW", "file": "src/core/ServerOrchestrator.ts", "line": 86, "issue": "nested ternary" }
  ]
}

---

Generated by Claude Code

[imbenrabi]

Fixes for the round 1 TS review

Pushed in eb3411b.

#	Finding	Disposition
1	Duplicate startup toolset names are fatal	Fixed. Instead of deduping, only failures that happen while registering a toolset's tools now count. DynamicToolManager.enableToolset returns a code on that path, and initializeToolsets ignores results without one ("already enabled", invalid name, policy). Test: stays ready when startup.toolsets lists a toolset twice.
2	ALL + exposure policy now rejects startup; changelog unclear	Fixed. Policy refusals carry no code, so they no longer fail readiness. Test: stays ready when the exposure policy refuses a startup toolset (ALL with a denylist, only core active). The behaviour that does change, a tool-registration failure (for example a name collision) now failing createMcpServer and the client, has its own Breaking changelog entry.
3	No negative caching	Documented in src/http/AGENTS.md. No rate limit added; that is a separate decision.
4	Spy leaks if an assertion fails	Fixed with try/finally in the rewritten registration-failure test.
5	Identity guard untested, retry status not asserted	Partly fixed. The unit test now asserts the retry also returns 500. The identity guard has no test: reproducing the interleaving needs two concurrent initializes with a controllable isReady, and the branch is one comparison. Left as is.
6	ctx: any	Left unchanged. It matches the any-typed loader contexts used throughout the existing e2e tests (sessionContextModuleLoaders).
7	Nested ternary	Left unchanged. Style only, and the lines are touched by this fix anyway; a helper would add a method for one call site.

Found while verifying against the FMP server (npm run verify:toolception with this branch packed): 19/21 checks pass. ALL_TOOLS fails to start because the server's earnings and calendar toolsets both include the earnings-transcript module, so getLatestEarningsTranscripts collides. Today that collision is swallowed and earnings is left inactive. With this change it is a registration failure. This is expected given the fix, but it means the server needs its catalog fixed before it takes a toolception release containing this PR.

---

Generated by Claude Code

[imbenrabi]

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

Verdict (head eb3411b): APPROVE / GO. No CRITICAL, HIGH or MEDIUM findings. The round 1 dispositions were accepted: #1 and #2 are fixed correctly, #3 and #4 are accepted, and #5, #6 and #7 are acceptable as left. Four LOW items were found.

#	Sev	Location	Finding	Disposition
1	LOW	CHANGELOG.md Breaking entry	"a tool name that another toolset already registered" is imprecise: with default namespacing, collisions come from duplicates inside one toolset, or from namespacing being off.	Fixed in the commit below: reworded to "a duplicate tool name, within a toolset or across toolsets when namespaceToolsWithSetKey is off".
2	LOW	DynamicToolManager.ts enableToolset	The return type gains an optional code; the @returns doc is stale and the changelog doesn't mention it (the enable_toolset meta-tool output now shows it on failure).	Fixed: @returns updated, and the Breaking entry notes the new code.
3	LOW	ServerOrchestrator.ts:97	"Has a code" is an implicit signal for "failed while registering".	Fixed with a comment at the catch-path code in DynamicToolManager. Invariant 8 and the tests already pin the behaviour.
4	LOW	ServerOrchestrator.ts:86-91	The nested ternary reduces to initial ?? []. Optional.	Left unchanged, as in round 1: the reviewer marked it optional.

The follow-up commit (docs, comment and TSDoc only, no logic change) is cbdd1a3. typecheck and 339/339 tests pass.

---

Generated by Claude Code

[imbenrabi]

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

Requested after merging main (#29 to #32) into the branch. Reviewed head 97ae7a4 against main 9e7e8a0.

Verdict: APPROVE / GO. No CRITICAL, HIGH or MEDIUM findings; three LOW nits. Rounds 1 and 2 dispositions were accepted.

Checked against merged main: the readiness 500 composes with the Origin guard (a disallowed Origin gets 403 first), sits after transport.onclose is set and before connect(), and the identity-guarded eviction holds under concurrent initializes. The permission-based server is unaffected (its orchestrators start with toolsets: []). The merged CHANGELOG [Unreleased] section reads correctly.

#	Sev	Location	Finding	Disposition
1	LOW	DynamicToolManager.ts enableToolsets catch, enableToolset TSDoc, core AGENTS.md invariant 8	The docs said code is set only when registering tools failed, but enableToolsets also sets E_INTERNAL if enableToolset throws, for example from a throwing exposurePolicy.onLimitExceeded. Treating that as a failure is arguably right.	Fixed (633f286): wording now says code is set when enabling threw, normally while registering tools. No logic change.
2	LOW	CHANGELOG.md [Unreleased]	Section order was Added, Fixed, Breaking; 0.7.0 leads with Breaking.	Fixed (633f286): Breaking now comes before Fixed.
3	LOW	CHANGELOG.md [Unreleased]	The merged main brings changes without an entry (meta-tool annotation hints, permission-aware transport readiness wait). Not this PR's code.	Left: belongs to the 0.8.0 release thread.

Informational (no action here): the reviewer notes that the later isReady() check in PermissionAwareFastifyTransport will currently never trigger, because those orchestrators have toolsets: [] and so never set initError. The release thread that owns that check should decide whether it is still worth adding.

typecheck and 385/385 tests pass on 633f286 (docs and comments only since the reviewed head).

---

Generated by Claude Code