Repository navigation
Validate against the schema passed in, not the first registered under its $id - #2974
Merged
Merged
Conversation
…2935) getValidator reused engine.getSchema($id) unconditionally, so a changed outputSchema that kept its $id, or a second tool declaring the same $id, was validated against whatever was registered first. Reuse the lookup only when it is the very schema object passed in; otherwise remove the registration (by Ajv's normalized key) and compile the new schema. Each Ajv entry point keeps a compile budget of its own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
2 tasks done
There was a problem hiding this comment.
🟢 Approval recommended
There are no active review issues with the shared-ID schema validation change.
No open issues 🎉
What changed in this PR
Fixes #2935 in the shared MCP schema validator so schemas sharing a $id are validated against the supplied schema rather than the first registration.
Changes:
- Reuses registered validators only when their schema object matches; otherwise removes the normalized
$idregistration and compiles the replacement. - Tracks lookup and compilation budgets separately.
- Adds regression coverage for changed schemas, alternating schemas, trailing fragments, and IDs registered through
$defs.
| File | Description |
|---|---|
core/mcp/linearPatternValidator.ts |
Adds identity-aware validator reuse and normalized registration replacement while preserving compilation-budget tracking. |
clients/web/src/test/core/mcp/linearPatternValidator.test.ts |
Tests shared-ID replacements and continued correctness of previously compiled validators. |
Review effort: Balanced
Member
Author
|
Copilot review closed: round 1 was clean (no inline comments, no suppressed findings), so the loop stopped on the first clean round. |
2 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2935
Problem
LinearPatternJsonSchemaValidator.getValidatordidengine.getSchema($id) ?? engine.compile(schema)whenever the schema had a string$id, so whatever was first registered under that$idwon:outputSchemachanged aftertools/list_changedbut kept its$id→ results checked against the old schema (false failures and false passes);$id→ the second tool's results checked against the first tool's schema.Fix
core/mcp/linearPatternValidator.ts: a new privatevalidatorFor(engine, schema).$id→compile(schema), as before.$id→ look it up; reuse the result only when its.schemais the very object passed in (so a repeated call with the same schema, and a$defsentry Ajv compiles lazily on first lookup by$id, are reused exactly as before). OtherwiseremoveSchemathe registration and compile the new schema — removing first is what keeps Ajv's "schema with key or id already exists" from throwing.#/#/dropped, mirroring Ajv'snormalizeId), because Ajv registers normalized butremoveSchematakes the key verbatim — without that,"$id": "urn:x#"would throw on the second schema.tracked()budget, so a lookup that compiles a schema this call then discards does not charge the replacement's budget.trackedis now generic so it acceptsgetSchema's possibly-undefinedresult.A validator already compiled against an old registration (e.g. an outer schema whose
$defsregistered the$id) keeps working: Ajv resolved its references when it compiled.Tests
New "schemas sharing a
$id" block inclients/web/src/test/core/mcp/linearPatternValidator.test.ts:$id(both directions from the issue:"21C"accepted,21rejected withdata/temp must be string);$id, alternating, each validated against itself with no throw;$idwith a trailing empty fragment replaced without throwing;$idregistered by an earlier schema's$defsreplaced, while the earlier outer schema still validates against its own definition.The first three cases from the issue fail on
v2/mainand pass with the change; the existing nested-$idbudget test still passes.npm run local:gateis green.Approach follows the prototype prompt shared on the issue (object-identity reuse,
removeSchemathen compile).🤖 Generated with Claude Code