Repository navigation
feat(mcp): report ignored input keys and cap tool schema growth - #111786
Conversation
Tool input schemas are Zod objects, which strip unknown keys without an error. A misnamed or misplaced field produced a success response while nothing was saved (insight-update after an empty PATCH). Making schemas strict would break clients that send harmless extras, so the result now names the keys that were dropped. Declared aliases are excluded because the schema folds them on purpose. The same helper runs in tools mode and exec mode.
|
😎 Merged successfully - details. |
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review 🦔 PostHog Review reviewed this pull requestNothing worth raising this time. Enjoy the moment:
|
📝 WalkthroughWalkthroughThe MCP tool execution paths compare caller-supplied arguments with validated arguments to identify keys that were not retained. Supported results include an ignored-key notice and, for object results, the ignored-key list. Schema reshaping beyond registered aliases suppresses key detection. Tests cover both execution paths and several input and output cases. Schema snapshot tests also check serialized input-schema size and nested-property description coverage against defined thresholds. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Ignored-key notices can still contain Unicode line separators, and the schema-description check can miss properties inside map values. Both are bounded concerns to address or explicitly accept before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change retains input validation and existing access controls. No privilege expansion or successful attack was established, but incomplete security verification and uncertainty around diagnostic rendering prevent a minimal-risk assessment. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/mcp/src/lib/ignored-input-keys.ts (1)
80-80: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTruncation hides the true count of ignored keys.
slice(0, MAX_IGNORED_KEYS)drops keys beyond 20 with no indicator. The notice then reads as a complete list. Add the total count when truncation happens so the agent does not assume the list is complete.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
4624b219-dc1e-4809-b3d4-dacbcfacbe0f
📒 Files selected for processing (6)
services/mcp/src/hono/tool-executor.tsservices/mcp/src/lib/ignored-input-keys.tsservices/mcp/src/tools/cast-helpers.tsservices/mcp/src/tools/exec.tsservices/mcp/tests/hono/tool-executor.test.tsservices/mcp/tests/unit/exec.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 5 remain after this review.
HostHog preview —
|
|
Risk: No findings Since the last review this pull request adds one test-file line that enables the Sentinel reviewed |
|
@codex review |
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. |
🤖 CI report✅ Trunk lane — non-backend lane (
|
| File | Comment lines | Added lines |
|---|---|---|
services/mcp/src/lib/ignored-input-keys.ts |
12 | 109 |
services/mcp/tests/unit/tool-schema-snapshots.test.ts |
7 | 113 |
services/mcp/src/tools/cast-helpers.ts |
2 | 19 |
services/mcp/src/hono/tool-executor.ts |
1 | 6 |
services/mcp/src/lib/constants.ts |
1 | 2 |
This check does not block merging. It updates on every push and clears when the share drops.
…rage Claude web and desktop silently drop a tool whose serialized inputSchema reaches 16,384 chars. Only the exec tool had a budget test, but tools mode registers every tool on its own and 44 schemas are already over. The count of oversized schemas can now only go down, and nested property description coverage cannot fall below its current level. Both checks live in the existing schema snapshot test and use counts and percentages only, so no tool names are copied into the test.
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Key names come from the caller and ended up verbatim in a message the agent reads as server output, newlines included. Control characters are now masked. The notice also states how many keys were cut off at the cap, so a truncated list is not read as complete.
![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: 4c8d16a37d
ℹ️ 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".
webjunkie
changed the title
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
services/mcp/tests/unit/tool-schema-snapshots.test.ts-162-164 (1)
162-164: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCount properties inside map-value schemas. When a nested object uses an object-valued
additionalPropertiesschema,schemaChildrendoes not visit that value schema. Its properties therefore do not enter the coverage calculation. Traverse object-valuedadditionalPropertiesso missing descriptions in map values cannot inflate the reported percentage.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
402c3851-d796-45ac-a5d3-82d093bb61d1
📒 Files selected for processing (4)
services/mcp/src/lib/constants.tsservices/mcp/src/lib/ignored-input-keys.tsservices/mcp/tests/unit/exec.test.tsservices/mcp/tests/unit/tool-schema-snapshots.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 4 remain after this review.
Review follow-ups on the ignored keys notice: - Key names are JSON-encoded in the notice so a caller-chosen name stays inside quotes. - The walk has a depth cap and stops collecting at 100 keys, and in tools mode it runs before the handler so it cannot fail after a write. - Own-property check instead of `in`, so keys like `constructor` that the schema strips are reported. - Array results keep their formatted override and informational marker when wrapped, which were read from the wrapper and lost before.
| /** | ||
| * claude.ai's connector registry silently drops a tool whose serialized `inputSchema` | ||
| * reaches this many characters (2^14). See the exec budget test in | ||
| * `tests/unit/instructions-formatter-snapshot.test.ts` for how the limit was measured. | ||
| */ |
There was a problem hiding this comment.
nitpick: I'm spotting some potentially drift-prone comments; did this PR follow our code comments skill? It says it read /writing-code-comments but if so why did it post these?
rnegron
left a comment
rnegron
left a comment
There was a problem hiding this comment.
didn't check but worth confirming: this is backwards compatible right? won't break existing MCP usage?
Comments now explain only why. File pointers, tool examples and behavior that the code already shows are removed because they go stale.
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
services/mcp/src/lib/ignored-input-keys.ts-12-12 (1)
12-12: 🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟡 Minor | ⚡ Quick winLLM Security
Reachability: External
Exploitability: Moderate
CWE: CWE-93Mask Unicode line separators in ignored key names.
If a caller puts U+2028 or U+2029 in an ignored key, the key reaches the agent-facing notice through
collectIgnoredKeys. The current mask leaves both characters intact, andJSON.stringifydoes not escape them. A crafted key can therefore create a visible line break followed by instruction-like text despite the existing newline fix. Mask both separators before building_ignoredKeysand the notice; verify their display in the agent-facing output. (tc39.es)Based on learnings, “do not rely on JSON.stringify or similar escaping alone” for U+2028 and U+2029 in model-facing text.
Source: Learnings
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
34c6b675-9002-4097-a19c-af225018cc68
📒 Files selected for processing (4)
services/mcp/src/lib/constants.tsservices/mcp/src/lib/ignored-input-keys.tsservices/mcp/src/tools/cast-helpers.tsservices/mcp/tests/unit/tool-schema-snapshots.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
Problem
MCP tool inputs have two silent failure modes that agents hit and cannot see.
Dropped keys. Tool input schemas are Zod objects, and Zod strips unknown keys without an error.
An agent that sends a misnamed or misplaced field gets a success response while nothing was saved.
Example:
insight-updatereports success after an empty PATCH (PR #108314, issue #108309)..strict()would reject these calls, but it also breaks clients that send harmless extra keys.Oversized schemas. Claude web and desktop silently drop a tool whose serialized
inputSchemareaches 16,384 characters.In tools mode every tool is registered on its own, and 44 tool schemas are already at or over that size.
Only the
exectool has a budget test.About 56% of properties below the top level have a description, and nothing stops that share from dropping.
Changes
Report ignored input keys
.transform(), a root union, or a custom root preprocess such asread-data-schema) get no notice. A missing notice beats a wrong one, because the diff cannot tell a consumed key from a dropped one there.services/mcp/src/lib/ignored-input-keys.ts, used by bothtool-executor.tsandexec.ts.Response shape for an object result (paths are dotted, array items use the index):
{ "ok": true, "_ignoredKeys": ["title", "query.source.serie"], "_ignoredKeysNote": "Ignored input keys: title, query.source.serie. These keys are not part of the tool's input schema, so the tool did not use them." }{ results, _ignoredKeys, _ignoredKeysNote }._agentNotewas not reused. It is static per tool, and a tool can already define one.Cap tool schema growth
exclude_params,include_paramsorparam_overrides, or addhelp_texton the serializer and regenerate.prepareToolList, the same path registration uses, so the injectedcontextproperty counts.CLAUDE_REGISTRY_INPUT_SCHEMA_CHAR_LIMITinservices/mcp/src/lib/constants.ts. The checks use counts and percentages only, so no tool names are copied into the test.Limits on the notice:
posthog-connection-call) validate inside their own handler and get no notice yet.How did you test this code?
Automated only: the MCP unit tests, the schema snapshot test and
test:honopass locally, and typecheck is clean. No manual run against a live server.Test rationale:
tests/unit/exec.test.tsextend the existing execcalltests. They cover a top-level key, a nested key, an array item key, a key with a newline, a folded alias, a clean call, and the three schema shapes that must report nothing. Those three fail if the reshaping guard is removed. One case intests/hono/tool-executor.test.tscovers tools mode, which does not share the exec code path.tests/unit/tool-schema-snapshots.test.ts, which already iterates every tool schema. They catch a new oversized schema or a drop in nested descriptions, which no existing test covers outsideexec.Release status
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code. Code written by a Claude Sonnet 5.5 (claude-sonnet-5-5) subagent, orchestrated by Claude Opus 5.5 (claude-opus-5-5).
/writing-tests,/writing-code-comments,/reviewing-with-coderabbit,/writing-pr-descriptions.context. A ReviewHog finding caught it. The count moved from 42 to 44 after the fix.--deep) on the ignored keys change, two findings, both rejected:$mcp_*fields were touched.