[imbenrabi]

Requested by Ben · project thread

Stack and merge order

T3, stacked on the base PR B1 (#35). Base branch of this PR: claude/project-thread-1w8fky (B1's head).

1. Merge feat!: move to MCP SDK v2 and Zod 4 (base) #35 (B1) into main first. GitHub then retargets this PR to main.
2. Merge this PR (T1 and T2 are independent of it and can merge in any order).
3. R1 release PR 0.9.0, then Ben tags v0.9.0.

B1's branch is merged into this one, never rebased. Nothing here is tagged or published.

What changed

Todo item b6. A tool whose inputSchema is a plain JSON Schema object used to register as z.looseObject({}): clients saw no parameters and any arguments reached the handler. It now goes through the SDK v2 fromJsonSchema().

• toRegistrableInputSchema() in src/core/DynamicToolManager.ts: Zod 4 schemas and Zod 4 raw shapes are unchanged. A JSON Schema with root type: "object" is passed to fromJsonSchema(), so tools/list shows the real schema and arguments are validated before the handler runs (invalid arguments give an isError result, the handler is not called). A missing or non-object schema still registers as an open object.
• Fail at registration: a JSON Schema that does not compile (unknown type name, unresolved $ref, unsupported $schema dialect), and any inputSchema that is neither a Zod 4 shape nor a JSON Schema object (no root type: "object", or a shape mixing Zod and non-Zod values), fails the toolset's enable with E_VALIDATION naming the tool. Nothing is registered. The last case used to silently register as an open object.
• Root $id is dropped before compiling: the SDK's Ajv validator caches compiled schemas by root $id, so two tools sharing a $id would get the first tool's validator while tools/list advertises the second schema. Stripping it avoids the cache hit; it is therefore not advertised either. Reference the root with #.
• Per-client validator (multi-client must-have): each DynamicToolManager, which is one per client, owns its own AjvJsonSchemaValidator (public @modelcontextprotocol/server/validators/ajv export, externalised by vite.config.ts). Ajv keeps every compiled schema for the validator's lifetime, so the SDK's process-wide default would hold all clients' schemas until exit. A per-client one shares nothing and is freed with the client. A client that toggles a loader-backed toolset repeatedly grows only its own cache, until the session ends.
• Docs: README, src/core/AGENTS.md (invariants 7 and 9), src/types/AGENTS.md, the McpToolDefinition JSDoc, and CHANGELOG [Unreleased] (breaking: arguments are now validated, plus migration note).

Behaviour change to know about

Handlers that relied on receiving arguments their JSON Schema forbids (missing required, wrong type, extra properties with additionalProperties: false) now get a validation error instead. This reverses the 2026-10-10 "keep passthrough" decision, as approved by Ben on 2026-11-04 (validation on).

Multi-client acceptance gate (plan section 3a)

tests/e2e/multiClient.e2e.test.ts (B1) passes unchanged on this branch. New tests:

• e2e over HTTP, two concurrent mcp-client-id clients on one DYNAMIC server, each with its own JSON Schema toolset: independent tools/list, independent validation, A's disable leaves B working
• e2e: invalid arguments rejected, the next valid call on the same client succeeds; tools/list returns the declared schema
• unit: identical nested $ids with different schemas across two managers; two tools of one manager sharing a root $id; disable then re-enable; rollback of an earlier good tool when a later JSON Schema tool is bad

Checks

• npm run typecheck ✔, npm run build ✔, npm run test:run ✔ 430 tests (Node 22 locally; CI is the authority)
• npm audit --audit-level=critical ✔ (exit 0)
• Mutation check: stripping the root $id is what the same-root-$id test guards; with it removed that test fails.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VmUXGTprrczjscL9XyWxYz

[imbenrabi]

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

Verdict: COMMENT (round 1). No CRITICAL or HIGH findings; 2 MEDIUM, 6 LOW. Fixes follow in a separate comment.

Reviewed: the PR diff against its base (B1, claude/project-thread-1w8fky) at head 943bbed, against typescript-standards and karpathy-guidelines, checked statically against the SDK v2.4.0 sources. The owner's multi-client requirement holds: one manager per client, the validator is a per-manager field, and no module-level validator state exists in toolception.

#	Sev	Location	Finding
1	MEDIUM	src/core/DynamicToolManager.ts:268	The SDK's getValidator reuses engine.getSchema($id) before compiling. Two tools in one manager sharing a $id with different schemas get the first tool's validator while tools/list advertises the second schema. A loader whose schema changes on re-enable with a stable $id keeps a stale validator.
2	MEDIUM	src/core/AGENTS.md:23, DynamicToolManager.ts:39	"No cache growth" is overstated: Ajv's _cache is never evicted on disable and loaders yield fresh schema objects per enable. Growth is bounded by the client's lifetime, not removed.
3	LOW	DynamicToolManager.ts:39, 265-269, 465	Lazy optional field plus getValidator thunk is unnecessary: the AjvJsonSchemaValidator constructor is cheap (engines are lazy).
4	LOW	DynamicToolManager.ts:39	Field jsonSchemaValidator has the same name as the imported type.
5	LOW	DynamicToolManager.ts:452	TSDoc says "missing schema"; code opens any non-object value.
6	LOW	CHANGELOG.md:11, src/core/AGENTS.md	"instead of failing at tools/list" is inaccurate (such schemas never failed before) and garbled in AGENTS.md.
7	LOW	tests/e2e/jsonSchemaTools.e2e.test.ts	No end-to-end test of two concurrent mcp-client-id clients with independent enable/disable state and a JSON Schema tool.
8	LOW	tests/dynamicToolManager.test.ts	Loose `/invalid

{
  "recommendation": "COMMENT",
  "summary": "Correct and well-scoped; the per-client validator isolation holds, but same-$id collisions and cache growth within one client remain, the lazy-validator thunk is unnecessary, and the concurrent-client requirement lacks an e2e test.",
  "findings": [
    { "severity": "MEDIUM", "file": "src/core/DynamicToolManager.ts", "line": 268, "issue": "Same-$id tools in one manager (or a re-enabled loader tool with changed schema) reuse the first compiled validator while tools/list advertises the new schema." },
    { "severity": "MEDIUM", "file": "src/core/AGENTS.md", "line": 23, "issue": "Claim that per-manager validators avoid cache growth is only partly true; growth is bounded by client lifetime." },
    { "severity": "LOW", "file": "src/core/DynamicToolManager.ts", "line": 39, "issue": "Lazy optional field plus getValidator thunk is unnecessary." },
    { "severity": "LOW", "file": "src/core/DynamicToolManager.ts", "line": 39, "issue": "Field name shadows the imported type name." },
    { "severity": "LOW", "file": "src/core/DynamicToolManager.ts", "line": 452, "issue": "TSDoc 'missing schema' should be 'missing or non-object'." },
    { "severity": "LOW", "file": "CHANGELOG.md", "line": 11, "issue": "'instead of failing at tools/list' is inaccurate; garbled in AGENTS.md." },
    { "severity": "LOW", "file": "tests/e2e/jsonSchemaTools.e2e.test.ts", "line": null, "issue": "No e2e test for two concurrent clients with a JSON Schema tool." },
    { "severity": "LOW", "file": "tests/dynamicToolManager.test.ts", "line": null, "issue": "Loose assertion, single-tool rollback test, stray trailing comma." }
  ]
}

---

Generated by Claude Code

[imbenrabi]

Fix mapping for TS review round 1

Fixes are in ba60dfe. Numbers follow the review's table.

#	Status	What changed
1	Fixed	The root $id is now dropped before compiling (toRegistrableInputSchema), so the SDK's getSchema($id) cache hit can no longer give a tool another tool's validator or a stale one. It is also no longer advertised in tools/list; reference the root with #. New tests: two tools in one manager with the same root $id and different types each validate against their own schema (and no $id is advertised); the earlier re-enable test still passes. Checked with a probe that nested $ids with the same URI do not collide in Ajv (only a root $id registers globally), so nothing more is needed there.
2	Fixed (wording)	Agreed the "no growth" claim was overstated. After #1 the per-manager validator is justified by lifetime, not correctness: Ajv keeps every compiled schema, so the SDK's process-wide default would hold all clients' schemas until exit, while a per-manager one is freed with the client. src/core/AGENTS.md (invariant 9), the field TSDoc and the CHANGELOG now say this, including that a client toggling a loader-backed toolset grows only its own cache for the session. I also removed my earlier PR-description claim that the old $id test failed against the shared default for a nested $id; it only failed for the root $id, which is now stripped.
3	Fixed	schemaValidator is an eager private readonly field; the thunk parameter and ??= are gone, so toRegistrableInputSchema is pure again and takes the validator as a plain argument.
4	Fixed	Field renamed to schemaValidator.
5	Fixed	TSDoc now says "A missing or non-object schema".
6	Fixed	CHANGELOG and src/core/AGENTS.md no longer say "instead of failing at tools/list".
7	Fixed	New e2e test in tests/e2e/jsonSchemaTools.e2e.test.ts: two concurrent mcp-client-id clients on one DYNAMIC createMcpServer, each enabling a different toolset whose JSON Schema tool shares a root and nested $id but differs in type. Asserts independent tools/list, independent validation, and that A disabling its toolset leaves B's tool working.
8	Fixed	Rejection cases now pin the Ajv detail (required property 'symbol', symbol must be string, limit must be >= 1, additional properties). New rollback test with a good tool before a bad JSON Schema tool. Trailing comma removed.

Checks: typecheck ✔, build ✔, test:run ✔ 430 tests. multiClient.e2e.test.ts unchanged and green. Re-running the reviewer next.

---

Generated by Claude Code

[imbenrabi]

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

Verdict: COMMENT. No CRITICAL or HIGH findings; 1 MEDIUM, 5 LOW. Reviewed at ba60dfe against base B1. All round-1 fixes were confirmed real against the SDK sources (root $id collision, per-manager validator, compile failures wrapped as E_VALIDATION, rollback, build externals).

#	Sev	Location	Finding
1	MEDIUM	DynamicToolManager.ts:495, src/core/AGENTS.md:23	The shallow copy { ...schema } is made for every JSON Schema tool, so Ajv's identity cache never hits on re-enable, and invariant 9 understates which toolsets grow the cache. Copy only when a root $id exists.
2	LOW	tests/dynamicToolManager.test.ts:536, tests/e2e/jsonSchemaTools.e2e.test.ts:148	The cross-manager and cross-client tests are valid behaviour tests but do not pin the per-manager validator they are named after. Rename, or add a constructor spy.
3	LOW	DynamicToolManager.ts:489, CHANGELOG.md:20, AGENTS.md	Docs say a Zod/non-Zod mixed shape always fails; one with type: "object" goes to Ajv. type: ["object"] arrays are now refused and used to work.
4	LOW	DynamicToolManager.ts:452, CHANGELOG.md:22	Docs warn only about refs to the root $id URL; nested relative or absolute $id refs may also be affected (unverified).
5	LOW	tests/e2e/jsonSchemaTools.e2e.test.ts:83	/invalid|validation/i is loose; the SDK message starts with Input validation error.
6	LOW	DynamicToolManager.ts:452	Nit: TSDoc paragraph indentation.

{
  "recommendation": "COMMENT",
  "summary": "All round-1 fixes are real and the PR is functionally sound; the main new issue is that the $id-strip copy runs for every JSON Schema tool, so it defeats Ajv's identity cache and AGENTS.md's loader-backed-only growth claim is now inaccurate, plus a few tests/docs that overstate what they prove.",
  "findings": [
    { "severity": "MEDIUM", "file": "src/core/DynamicToolManager.ts", "line": 495, "issue": "Shallow copy made for every JSON Schema tool defeats Ajv's identity cache on re-enable; copy only when a root $id exists or reword invariant 9." },
    { "severity": "LOW", "file": "tests/e2e/jsonSchemaTools.e2e.test.ts", "line": 148, "issue": "Cross-client tests do not pin the per-manager validator property; rename or add a constructor spy." },
    { "severity": "LOW", "file": "src/core/DynamicToolManager.ts", "line": 489, "issue": "Docs broader than code for mixed shapes; type arrays now refused; add a note." },
    { "severity": "LOW", "file": "src/core/DynamicToolManager.ts", "line": 452, "issue": "Root $id strip may affect nested/absolute refs; widen note or pin with a test." },
    { "severity": "LOW", "file": "tests/e2e/jsonSchemaTools.e2e.test.ts", "line": 83, "issue": "Loose error-message regex." },
    { "severity": "LOW", "file": "src/core/DynamicToolManager.ts", "line": 452, "issue": "Nit: TSDoc indentation." }
  ]
}

---

Generated by Claude Code

[imbenrabi]

Fix mapping for TS review round 2

Fixes are in c206ae1. Numbers follow the review's table.

#	Status	What changed
1	Fixed	The schema is copied only when it has a string root $id (toRegistrableInputSchema), so a catalog schema without $id keeps its object identity and re-enables hit Ajv's cache. Invariant 9 was already worded as "a loader-backed toolset" for growth, which is now accurate again; a schema that does have a root $id is still copied per enable.
2	Fixed	New tests/schemaValidatorScope.test.ts spies on the AjvJsonSchemaValidator constructor: two managers construct exactly two validators. The cross-manager unit test and the e2e test were renamed to claim only what they show (each client/manager is validated against its own schema).
3	Fixed	CHANGELOG and src/core/AGENTS.md now say the root type must be the string "object", so type: ["object", "null"] is refused, and that this covers a mixed shape (a mixed object that itself has type: "object" is read as JSON Schema, as the reviewer noted). New test row for a root type array.
4	Fixed (verified, no code change)	I probed Ajv: absolute and relative $refs to a nested $id resolve the same with and without a root $id, so only references to the root $id URL itself are affected, which the docs already say. Pinned with a new test (root $id dropped, absolute $ref to a nested $id still validates).
5	Fixed	The e2e assertion is now /Input validation error/. The older Zod-path assertion at dynamicToolManager.test.ts:391 comes from B1 and is left alone.
6	Fixed	The $id paragraph in the TSDoc was rewritten and shortened.

Checks: typecheck ✔, build ✔, test:run ✔ 433 tests. multiClient.e2e.test.ts unchanged and green. Re-running the reviewer for round 3.

---

Generated by Claude Code

[imbenrabi]

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

Verdict: APPROVE. No CRITICAL, HIGH or MEDIUM findings at c206ae1. All six round-2 fixes were confirmed against the SDK's Ajv and fromJsonSchema sources. One LOW doc nit remains.

#	Sev	Location	Finding	Status
1	LOW	src/core/AGENTS.md:23, DynamicToolManager.ts:~168	Invariant 9 and the code comment say only loader-backed toolsets grow the Ajv cache on toggle; a schema with a root $id is copied on each enable (new cache key), so those toolsets grow it too.	Fixed in 86bff9e (docs and comment only): both now name the root-$id case and say the growth is bounded by the client's lifetime. I kept the copy per enable instead of adding a WeakMap memo, since it is the same acknowledged trade-off and not worth more code.

The reviewer's optional test nit (a nested $id on re-enable has no test of its own) needs no action; the reviewer traced it as correct and the nested-$id pin test covers compile.

{
  "recommendation": "APPROVE",
  "summary": "All six round-2 fixes are real and correct; Ajv semantics for $id stripping, nested $id refs and per-manager validator scope check out; only a LOW doc nit remains.",
  "findings": [
    { "severity": "LOW", "file": "src/core/AGENTS.md", "line": 23, "issue": "Invariant 9 and the DynamicToolManager.ts comment say only loader-backed toolsets grow the Ajv cache on toggle; a schema with a root $id is copied on every enable, so those grow it too. Reword the docs or memoize the copy." }
  ]
}

Final state at 86bff9e: typecheck ✔, build ✔, test:run ✔ 433 tests; multiClient.e2e.test.ts green.

---

Generated by Claude Code