Repository navigation
Conversation
…ver admits inspect_analysis_view expressed the collection/selector pairing in a superRefine. A refinement is absent from the converted JSON Schema, so the advertised input schema accepted `modules` with `index` and `sections` with `node_id` while runtime validation rejected those same arguments. A client that validates against tools/list therefore sent arguments the server refused as invalid, before it could interpret whether the retained Evidence supported the requested collection. Express the existing alternatives in the owning contract instead: modules are selected by path or node_id, sections and symbols by index or name. Runtime validation, selection semantics, and Evidence identity are unchanged; the pairings that were rejected before are still rejected, now from the schema too. Adds tests/boundary/mcp/analysisViewSelectorAgreement.test.ts, which drives the real stdio MCP transport and asserts that every advertised pairing is admitted and every unadvertised one is rejected as invalid input. Closes morluto#1695
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: c21de4c6ce
ℹ️ 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".
| for (const entry of selectorCases) { | ||
| const rejected = await wasRejectedAsInvalidInput(client, entry.view); |
There was a problem hiding this comment.
When SDK conversion still advertises a forbidden pairing while the server's Zod parser rejects it—the exact regression this change addresses—this loop still passes because advertised.inputSchema is never compiled or applied and the helper only checks that the eventual tool response is an input error. In fact, retaining this test while reverting the production schema to the parent superRefine implementation would remain green. Compile the advertised schema with Ajv2020 and assert every case against it before separately checking runtime parity.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
…dation The previous test only inspected the final tool response, so it could not detect the defect it was added for: a client-side schema rejection and a server-side validation rejection both surface as an input error. Reverting the contract to the parent superRefine implementation left the test passing. Compile the input schema from tools/list with Ajv and compare its verdict with runtime validation for each collection/selector pairing, then assert the expected verdicts separately. The test now fails when the contract regresses: `modules + index: advertised schema and runtime validation disagree`.
|
Thanks, this is correct and I verified it. Reverting the contract to the parent The test now compiles the Pushed as 60954a7. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60954a7166
ℹ️ 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".
| expect( | ||
| advertisedAcceptsCase, | ||
| `${entry.label}: advertised schema and runtime validation disagree`, | ||
| ).toBe(runtimeAcceptsCase); |
There was a problem hiding this comment.
For the modules + index case, this expectation is deterministically false: fresh evidence after the earlier review is that this revision now compiles advertised.inputSchema, but the final tree leaves itemViewSchema in src/domain/analysisView/analysisView.ts unchanged as the independent collection/selector union plus superRefine, which SDK conversion omits. Ajv therefore accepts this case while safeParse rejects it, so every focused/CI run fails here and the advertised-contract bug remains; retain a JSON-Schema-expressible production contract (such as the discriminated collection alternatives) alongside this test.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
The previous commit staged the production file while it still held the reverted superRefine implementation used to check that the new test fails without the fix, so the committed tree dropped the contract change and the advertised schema regressed to the state this pull request exists to repair. Restore the discriminated collection alternatives. The strengthened test fails again when this contract regresses, which is the property the revert check established.
|
The P1 is correct and was my mistake. Commit Restored in
Lesson taken: a revert experiment must be undone before staging, and verification has to run against the committed tree rather than the working tree. |
|
Closing: superseded by #1705, which merged today ( I verified the two contracts are semantically identical over 56 collection/selector cases (0 verdict differences), so the production change here would conflict with main and add nothing. One thing this PR's test covers that #1705's does not: the |
1 participant
Closes #1695
Problem
inspect_analysis_viewexpressed its collection/selector pairing in asuperRefine. A refinement is absent from the converted JSON Schema, so theadvertised input schema and runtime validation disagreed:
collection: "modules"+selector: { index }collection: "sections"+selector: { node_id }A client that validates arguments against
tools/listtherefore sentarguments the server refused as invalid, before it could interpret whether the
retained Evidence supported the requested collection. Reproduced with the real
stdio MCP transport and Ajv 8.20.0 draft-2020-12 validation.
Change
Express the existing alternatives in the owning contract rather than in a
refinement: modules are selected by
pathornode_id; sections and symbolsby
indexorname. This is the direction the report suggested.rejected, now by the schema as well.
untouched.
against it cannot construct an argument set the server refuses.
Verification
tests/boundary/mcp/analysisViewSelectorAgreement.test.tsdrives the realstdio MCP transport over twelve collection/selector pairings and asserts that
every advertised pairing is admitted and every unadvertised one is rejected as
invalid input. Admission is keyed on the input-validation failure rather than on
call success, so a pairing that passes validation and then fails Evidence
resolution still counts as admitted.
npx tsc --noEmitpasses.tests/boundary/mcp/analysisViewMcp.test.ts(which exercises this schemaand its output contract) passes 8/8.
npm run mcp-catalog:checkpasses, so the generated catalog stays fresh.Note for review
This changes the shape of some validation errors for
view.kind: "item", forexample from
expected: ["index","name"]toexpected: "object", because thepairing is now a discriminated union. I believe that is the intended trade: the
previous error text described a constraint the schema did not advertise. If you
would rather preserve the old message shape, say so and I will adjust.
Not verified here
The five failures in
tests/boundary/cli/analysisViewCli.test.tsand two intests/boundary/mcp/analysisSummaryDetailMcp.test.tsare pre-existing on thischeckout (missing
stream-chain/stream-jsonand a stale expected-errorshape); I confirmed each by stashing this change and reproducing it on a clean
tree. No new failures are introduced.