Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 135384a The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8d3cdabaf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function isProvablyObjectShapedRoot(schema: Record<string, unknown>, root = schema, ancestors: ReadonlySet<object> = new Set()): boolean { | ||
| if (schema.type !== undefined) return schema.type === 'object'; | ||
| if (ancestors.has(schema) || (schema !== root && schema.$id !== undefined)) return false; | ||
| const seen = new Set(ancestors).add(schema); |
There was a problem hiding this comment.
When a valid definition DAG has multiple composition members referencing the same next definition, seen only records the current ancestor path, so every branch recursively reclassifies the entire shared subtree. The work doubles at each level (a 22-level schema with two identical refs per anyOf took about 17 seconds locally), allowing a small generated output schema to block synchronous conversion and thus tools/list; retain path-local cycle detection but memoize completed classifications by target.
Useful? React with 👍 / 👎.
| const seen = new Set(ancestors).add(schema); | ||
| if (schema.$ref !== undefined) { | ||
| const target = resolveLocalSchemaReference(root, schema.$ref); | ||
| return target !== undefined && isProvablyObjectShapedRoot(target, root, seen); |
There was a problem hiding this comment.
When a local $ref targets a typeless schema containing properties, required, or additionalProperties, this recursion treats that keyword as proof of an object even though JSON Schema ignores those keywords for non-object instances. For example, a target { additionalProperties: false } accepts a string, but conversion now stamps the root as type: 'object'; on a 2025 connection projectCallToolResult wraps that valid string by value while tools/list advertises the stamped schema, and the resulting { result: ... } violates additionalProperties: false. Following a reference should therefore require a true object type constraint rather than reuse the loose inline compatibility heuristic.
Useful? React with 👍 / 👎.
karaaslanz
left a comment
karaaslanz
left a comment
There was a problem hiding this comment.
Independent review of the new head 135384af8692a1ecd778a10fec30c3126d5a2212, specifically the two findings from the earlier Codex review of c8d3cda.
I fetched the exact packages/core-internal/src/util/standardSchema.ts blob (3434e81970c685f59dbd006ba25d79bdcb4b0c06), executed the actual isProvablyObjectShapedRoot and resolveLocalSchemaReference functions with Node 24's TypeScript-strip transform, and checked seven isolated cases:
- named object output through a local root
$ref-> recognized; - referenced typeless
requiredandadditionalProperties:falsetargets -> not misclassified as objects; - a chained reference -> recognized;
- direct cycle and nullable object -> conservatively left unclassified;
- 22-level shared-definition DAG with two references per level -> 45 reference reads, well below the 100-access bound (roughly 0.24 ms in this isolated run).
7/7 assertions passed, and the source blob was SHA-checked. The new completedReferences memoization appears to resolve the exponential traversal example, while requireObjectType addresses the typeless-reference counterexamples. No repo files were modified.
This is an isolated, exact-source helper verification, not a full SDK, cross-era HTTP, or published-package test. I am not asserting that all JSON Schema semantic corner cases are covered. The regression cases already added to this PR are appropriate for the two previous findings.
claude
Bot
added
the
v2
This branch has not been deployed
2 participants
Fixes #3032.
On a 2025-11-25 connection, an object output schema expressed through a local root
$refis classified as non-object, so the compatibility conversion returns{ result: { id, body } }. The equivalent inline schema returns{ id, body }.Follow same-document JSON Pointers when determining whether an output schema is constrained to objects, preserving the reference and definitions. Require object type constraints through references while retaining the existing inline object-keyword compatibility behavior. Cache completed reference classifications and keep path-local cycle detection so shared definition graphs do not cause exponential traversal. Leave unresolved, external, anchor, and nested-resource reference cases conservatively classified. No public API or dependency changes.
The 39 added cases cover reference chains, escaped names, recursive objects, shared graphs, cycles, missing targets, nested resources, and typeless targets, plus client/server HTTP exchanges on 2025-11-25 and 2026-07-28. The HTTP cases check inline and referenced objects, primitive wrapping, typeless schemas that accept strings, and rejection of invalid object results. The release note describes the corrected legacy output shape.
Validation
135384a, including the Node 20/22/24 main and end-to-end matrices, Bun/Deno, client/server conformance, build, and examples..meta({ id })reproduction: published SDK 2.3.1 exhibits the wrapper; the patched build returns the object directly for named and inline schemas on both protocols. The committed regression usesfromJsonSchemaso the repository's pinned Zod version can remain unchanged.Local check limitation:
check:alllocally stops on twoimport/no-extraneous-dependencieserrors forvitestintest/helpers/src/helpers/http.ts:5andoauth.ts:2. Both were reproduced on pristine base9bc40e903fde15534ec06ff4c1347d47000dbe16with the same dependency installation. The affected packages' lint and the documentation check pass separately; no unrelated changes are included to suppress these errors. GitHub runs the full aggregate checks on each revision (current build job).AI disclosure
The reproduction, implementation, tests, and PR/issue text were primarily written with Codex, which also ran the validation above. No independent human review is claimed.