Repository navigation
fix(mcp): keep Gemini-rejected dependency keywords out of input schemas - #2877
thefolahan wants to merge 4 commits into
Conversation
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. |
 |
![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: 67430914e7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Source fixes and publication evidence for the review findings below. Tests and executable checks were skipped at the maintainer's request. Findings that still require a decision or execution evidence remain open. The schema owner moves table-dependency guidance to root text retained by compact projection, and the public contract now states that compact clients cannot rely on omitted partner validation. The changes are live at 16b7a16. Source inspection and git diff --check only; tests were not run. |
a9f9bab to
aca7106
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aca7106961
ℹ️ 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".
| // Partner fields stay in the group description rather than in | ||
| // dependentRequired or dependencies: Gemini function declarations reject | ||
| // both, and one unknown keyword fails the client's whole tool catalog. | ||
| ...(fewestRequired > 0 ? { minProperties: fewestRequired } : {}), |
There was a problem hiding this comment.
npm run test:mcp now fails in tests/boundary/mcp/browserComparisonInput.test.ts: its invalid table still assigns advertisedValid: false to { ...passive, normalization } and the two scenario-plus-normalization cases (lines 113-121 and 144-147). After partner dependencies stop being emitted here, all three inputs contain only known, correctly typed properties and satisfy minProperties, so Ajv returns true while the assertion at line 155 expects false; move these cases into the advertised-valid/runtime-invalid coverage so the affected boundary test matches the new server-side-only enforcement.
AGENTS.md reference: AGENTS.md:L25-L25
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ea4cfc9. The three inputs that carry normalization without both scenario captures (with the passive pair, or with only one scenario side) are now in the group the advertised schema accepts and the canonical parser rejects, next to the mixed families case. A lone before or before_scenario is still rejected by the advertised schema, so those stay in the fully invalid list. The test passes locally on Node 24.21.0.
aca7106 to
5ffd3ef
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ffd3ef766
ℹ️ 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".
| enforces the exact union, including partner fields and exclusions that the | ||
| advertised projection does not express, so a client must not rely on the | ||
| advertised schema to reject an incomplete group. Tests validate the schemas |
There was a problem hiding this comment.
Although this general paragraph was corrected, the fresh evidence is the unchanged “Comparison input shapes” paragraph at lines 697–700, which still promises that the advertised compare_web_captures schema rejects incomplete pairs. This commit's test at src/contracts/toolInputReferences.test.ts:350-363 now establishes that { before_scenario: 1, normalization: true } passes advertised validation while the canonical parser rejects it, so users following the concrete tool documentation can incorrectly assume the client schema will catch such calls; update that section to match the new server-side-only partner enforcement.
AGENTS.md reference: AGENTS.md:L25-L25
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ea4cfc9. "Comparison input shapes" now says that mixed families, and normalization without both scenario captures, can pass advertised validation and are rejected by the SDK against the canonical schema. Incomplete pairs themselves are still rejected by the advertised schema, so that sentence stays.
Gemini function declarations reject dependentRequired and dependencies, and one unknown keyword fails the client's whole tool catalog. Flattened root unions no longer advertise the partner map, and the SQLite input states its table requirement in its descriptions instead. The canonical parsers still enforce both rules; a boundary test now checks every advertised schema in the full and compact profiles. Fixes morluto#2832
…ntract The compact profile strips field descriptions, so the SQLite table requirement moved to the root description, which it keeps; a boundary assertion checks it in both profiles. docs/mcp-contracts.md no longer promises that advertised root unions express group dependencies: partner fields are guidance there and enforced by the canonical parser.
5ffd3ef to
5447a5d
Compare
morluto
added
area: mcp
Use one named input-schema projection for clients and catalog identity, preserve dialect declarations, and bound compact schema depth. Keep canonical SDK output validation when output advertisements are disabled. Release request-owned state on cancellation and carry upstream limitations through composed workflows. Preserve doctor full-output envelopes and validate navigation facts at their owner. Reviewed fixes: #2088, #2108, #2513, #2684, #2693, #2695, #2836, #2864, #2868, #2877, #2933, #2998, #3002, #3015. Validation: focused schema, SDK output-validation, context, and workflow tests passed; no schema dialect or canonical output validation was relaxed.
Assert SAZ expansion-limit code and byte constraint through stdio MCP as well as CLI, without partial results or source mutation. Align schema presentation guidance with canonical runtime partner validation. The general MCP documentation paragraph preserves the upstream explanation from 5447a5d in PR morluto#2877. Co-authored-by: Omisakin Joshua <117924167+thefolahan@users.noreply.github.com> (cherry picked from commit 5d906ecbe1ef7e6ca9e2129dd9dbc25d6145d1e9)
Main (80de314) already strips dependentRequired/dependencies from every advertised input schema, states partner fields as root description guidance (for example the SQLite "When row_limit is provided, also provide table."), and tests both catalog profiles for those keywords. Keep main's SQLite schema, presentation, and tests instead of this branch's hand-written SQLite description and duplicate keyword walker. Keep this branch's docs update: main's contract text still said the projection retains group-specific field dependencies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
CI-fix track: I merged current main and resolved the conflicts (2807b99). The failing Static checks, Source tests and shard jobs reflected main's state at the time. Main already took the core of this fix in 80de314 (which lists #2877): advertised input schemas no longer carry |
|
Closing this, since the core of the fix landed on The only part left is the I am closing it partly to stay within the open pull request limit for contributors, so I can open a small fix for the real Ghidra Swift verifier, which currently fails on |
|
@thefolahan, thanks for identifying that a single unsupported dependency keyword could make an entire Gemini tool catalog unavailable. The catalog projection now explains field-group relationships in guidance text and leaves exact enforcement to the canonical runtime schema (main). |
Summary
Fixes #2832. Advertised tool input schemas no longer contain
dependentRequiredordependencies. Gemini function declarations reject both, and one unknown keyword in one tool fails the request for the whole catalog, so every REA tool became unavailable on Gemini-backed clients.Problem and expected behavior
Two tools carried the keyword in the advertised catalog on current
main:compare_web_captures, fromflattenRootObjectUnioninsrc/contracts/inputSchemaPresentation.ts, which turned "a field unique to a group needs that group's required partners" intodependentRequired(ordependenciesfor draft-07). This is the one reported in [Bug] Gemini-backed clients reject the whole tool catalog: advertised schemas emit dependentRequired / dependencies / $schema #2832.inspect_sqlite_database, from.meta({ dependentRequired: { row_limit: ["table"], columns: ["table"] } })insrc/domain/sqlite/sqliteDatabase.ts. It landed in feat(sqlite): inspect offline snapshots with selective evidence #2271 after the issue was filed, and the new test below found it.Change and scope
flattenRootObjectUnionno longer computes or emits the partner map. The group description it already writes ("Provide a complete input group: before + after; before_scenario + after_scenario + [normalization].") still tells callers which fields go together, andminPropertiesis kept.dependentRequiredfrom its metadata. The table requirement is now in the input schema's root description, which the compact profile keeps (field descriptions are stripped there), androw_limit's own description says it too. ItssuperRefinechecks are unchanged.{ before_scenario: 1, normalization: true }, and the server rejects it.toolInputReferences.test.tsnow states that expectation explicitly, next to the existing cross-group case.docs/mcp-contracts.mdnow describes partner fields as guidance enforced by the canonical parser, not by the advertised schema.I kept
$schema. MCP clients rely on it to tell draft-07 from 2020-12, and the reporter's capture shows gateways already strip it.compactInputSchema.tsis unchanged: it preserves whatever validation keywords it receives, and nothing upstream emits these two any more.Regression guard
tests/boundary/mcp/compactInputSchemaMcp.test.tslists the whole catalog over an in-memory MCP transport, with both the full and compact profiles, and walks every advertised input schema fordependentRequiredanddependenciesin keyword position. It skips property names and literalconst/enum/default/examplesdata, so a field that happens to be calleddependenciesis fine. Against the old code it fails with:Contract and boundary impact
inputSchemaPresentation.ts), plus the SQLite input contract's advertised metadata.Verification
Node 24.21.0 on macOS (Apple silicon):
vitest runfortests/boundary/mcp/compactInputSchemaMcp.test.ts,src/contracts/toolInputReferences.test.ts,src/contracts/compactInputSchema.test.ts,tests/conformance/contracts/mcpInputSchemaProfile.test.ts,src/application/sqliteandtests/boundary/sqlite: 11 files, 128 passed, 1 skipped.tests/boundary/mcp/toolSchemaValidity.test.tswith the compact test: 21 passed. Building the catalog artifact it needs requirescompile, which fails on currentmainatsrc/android/JadxAnalysis.ts:366(Main compile: optional Jadx schema output is incompatible with JsonValue #2852, fix in fix(android): require explicit invoke coverage state #2850). I ran it with a local, uncommitted workaround for that line only.tsc --noEmit: no errors in the changed files. The errors it does report are already onmain(JadxAnalysis.ts,analysisView.test.ts,IdaSessionClient.test.ts).verify:gemini-clientwas not run: no Gemini CLI on this host.Observation, not changed here: the catalog also advertises
if/then(inspect_sqlite_database) andnot(compare_web_captures), which are also outside Gemini's strictSchemamessage. The reporter's gateway strips those, so they did not break the request. I am happy to follow up if you would rather the advertised catalog avoid them too.