Backport #4492 to release/3.x: don't mutate the caller's schema in compress_schema - #4663
Conversation
(cherry picked from commit 6202008)
marvin-context-protocol
Bot
added
the
bug
![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: 7bcbb31298
ℹ️ 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".
| # Work on a copy so the caller's schema is never mutated (see docstring). The | ||
| # pruning phases below pop keys/$defs in place, which would otherwise corrupt a | ||
| # shared dict such as a live Tool.input_schema passed straight to compress_schema. | ||
| schema = copy.deepcopy(schema) |
There was a problem hiding this comment.
When compress_schema receives a valid schema with roughly 500 nested items objects under Python's default recursion limit, this deepcopy raises RecursionError before traverse_and_clean can reach its existing depth > 50 guard; the parent implementation returned normally by stopping at that guard. Deep schemas from remote or proxied MCP tools will therefore fail compression instead of returning a partially optimized schema, so the copy should be performed iteratively or otherwise honor the same bounded-recursion behavior.
Useful? React with 👍 / 👎.
2 participants
Backport of #4492 to
release/3.xfor the 3.4.5 patch release. Closes #4493 on the 3.x line.compress_schema()documented that it returns a new schema and leaves its input untouched, but its pruning phases poppedtitle,additionalProperties, and unused$defsoff the dict the caller passed in. Callers sharing a schema across components could see one component's compression silently alter another's.Codex review on this PR caught that the
deepcopyfixing that mutation introduces a recursion ceiling: a schema nested a few hundred levels deep now raisesRecursionErrorbefore the traversal's owndepth > 50guard can return a partially optimized schema. Chasing that surfaced a second, older problem — the reference scan stops at that same guard, but the pruning phase treats every definition it did not see referenced as unused, so a$refbelow the cutoff had its$defsentry deleted out from under it.This carries both fixes across alongside the original backport, so 3.4.5 ships the fix without the regression:
_copy_schemawalks the schema with an explicit stack instead of recursing, so the copy no longer sets a depth ceiling.$defspruning is skipped whenever the reference scan was truncated. An unpruned definition is harmless; a dangling$refis an invalid schema.The dangling-reference bug is not specific to this backport — it reproduces on
release/3.xtoday at 60 levels of nesting, well within whatdeepcopyhandled.Upstream on
mainas #4671.