fix(mcp): avoid top-level api_impact schema combinators#2489
Conversation
|
@koriyoshi2041 is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 14646 tests passed 62 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Downstream validation from the Electric maintenance fork is now complete: electricsheephq#123 The fork generalized the compatibility contract across every advertised MCP tool: no top-level This is provider-neutral implementation evidence; the tests use deterministic temporary data and contain no credentials or private repository policy. |
b07e263 to
470384b
Compare
|
CI readback after the rebase: all quality/typecheck/build/CodeQL jobs passed. The only non-Vercel failure is That test looks unrelated to this PR's |
magyargergo
left a comment
There was a problem hiding this comment.
Review: PR #2489 (rios/bedrock-api-impact-schema @ 470384b vs merge-base a05b501)
Findings
- [MEDIUM]
gitnexus/test/unit/tools.test.ts— the branch is CONFLICTING withmainand cannot be merged as-is (inline comment with resolution guidance). This is the only blocking item.
No defects found in the change itself.
Change and blast-radius summary
- The fix is real and verified end-to-end:
server.ts:157forwardsinputSchemaverbatim in ListTools, so the top-levelanyOfdid reach Bedrock's validator; removing it addressesTOOL_SCHEMA_INVALID(#2487). - The runtime guard the PR relies on exists and is structured:
local-backend.tsapiImpact()returns{ error: 'Either "route" or "file" parameter is required.' }when both are missing — no behavioral hole opens up from loosening the transport schema. - The tool description already documents the constraint (
tools.ts:744—Requires at least "route" or "file" parameter), and the new test pins it. - Removing the
anyOffield from theToolDefinitioninterface is safe: after merging with current main there are no remaining writers or readers ofinputSchema.anyOfanywhere ingitnexus/orgitnexus-web/(only negativetoHavePropertyassertions in tests, which don't depend on the TS type). - This matches the repo's established direction — main's 3f3494f ("fix(mcp): validate aliases without schema combinators") applied the same no-top-level-combinators pattern to the
impacttool. - GitNexus
detect_changes(compare vs merge-base): LOW risk, 0 affected execution flows. GraphimpactonGITNEXUS_TOOLSreturned 0 upstream hits, so consumers were verified by source instead:server.ts(ListTools) and the schema self-check loop attools.ts:896. - No competing fix landed on main; #2487 is still open and this PR is the only candidate.
Coverage and residual risk
- The regression test locks out
anyOf/oneOf/allOfat the top level of theapi_impactschema and asserts the description carries the requirement;calltool-dispatch.test.tsalready covers the runtime rejection path. - The failing
tests / windows-latest 1/3shard istest/integration/lbug-multiwriter-deadlock.test.ts— an LadybugDB multi-writer integration test unrelated to this 2-file MCP schema change (known Windows flake family). CI Gate is red only transitively; the rebase this review requires will trigger fresh CI anyway. - Out of scope but worth tracking from the #2487 discussion: the client-side swallowing of Bedrock's 400 into a generic streaming error is a separate diagnosability issue this PR doesn't (and needn't) address.
Verdict
REQUEST CHANGES — solely for mergeability. Rebase onto main and resolve the tools.test.ts conflict; the change itself is correct and I'll approve once the branch is mergeable with green (or flake-explained) CI.
Reviewed with the gitnexus-review skill (PR #2431) — GitNexus detect_changes/impact + source verification in a detached worktree at head.
| }); | ||
|
|
||
| it('api_impact tool expresses the route-or-file requirement via anyOf (#2308)', () => { | ||
| it('api_impact tool avoids top-level schema combinators for Bedrock compatibility (#2487)', () => { |
There was a problem hiding this comment.
[MEDIUM — the one blocking item] This file conflicts with current main: since your branch point (a05b501), main added adjacent tests in this exact region — context tool advertises file as a compatibility alias right above, and impact tool requires direction … without combinators right below (from 3f3494f/a75844b6) — while this PR rewrites the api_impact test in between, so the merge is positional-conflicted.
Resolution is mechanical: rebase onto main, keep both of main's neighboring tests unchanged, and take this PR's rewritten api_impact test in place of the old anyOf one. Your interface-field removal in tools.ts auto-merges cleanly (verified with git merge-tree), and post-merge nothing else reads or writes inputSchema.anyOf.
For the record, the red tests / windows-latest 1/3 shard is lbug-multiwriter-deadlock.test.ts — unrelated to this change; the rebase push will re-run CI.
470384b to
cb33f6b
Compare
|
Rebased onto current |
Summary
anyOffrom theapi_impactMCP input schemarouteandfileoptional at the transport layer so stricter provider validators can load the tool listanyOf/oneOf/allOfon this schema while relying on the existing runtime guard for missing lookup keysFixes #2487.
Verification
npx vitest run test/unit/tools.test.tsnpx vitest run test/unit/calltool-dispatch.test.tsnpx tsc --noEmitRisk
Low. The transport schema is looser, but
callTool('api_impact', {})still returns the existing structured error when neitherroutenorfileis provided.