Repository navigation
mcpx(docs): disposition every revision conflict — Phase 1 of #2209 (#2210) - #2892
Conversation
…-ups (#2210 Phase 1, partial) Headline disposition index (13 items) from verified code sites; A/C5 and A/C23 rewritten to current behaviour (negotiate counter-offers; discover uses supportedVersions + _meta serverInfo); A/C17 stale protocolVersions claim fixed; protocol.md 1.1 closing sentence names the disposition index; requirements.json discover notes trued up (client parses supportedVersions; server partial only on missing ttlMs/cacheScope). Per-area (A-E) in-place dispositions remain; see queue task 3951 resume note.
…Phase 1) All ~105 per-area items in sections A-E of revision-conflicts.md now carry a disposition line (peer-settled / precedence-governed / recorded reading / undisposed with a named open question), classified from each item's own text plus code sites verified this shift (transport.go batchRefused/runBatch, server.go session disconnect block, client.go legacy 404 recovery and nextPage, request.go completions gate, conn.go server-sent cancel, tasks.go -32601 reading). Headline index truthed up from the same verification: headline 10 is peer-settled (client.go:1138-1145 era-keyed 404 recovery); headline 13 records the C10 recorded reading for the missing-required-task code while cancelled-task finality stays undisposed with its question. Docs only; no Go code or behaviour changes.
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. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 8 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 2db900ab08
ℹ️ 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".
… review (#2210) Codex P2s on PR #2892, both verified in code before fixing: - maybeTask() returns nil for a non-augmented legacy call before any task-support check (tasks.go:237-240); the -32601 branch covers an augmented call to a forbidden tool. C10 and headline 13 now record the actual behaviour and keep the -32601/-32600 question open. - The server never returns -32042: a URL question the client cannot answer stays with the broker (ask.go:192-207); conformance gap elicitation-server-must-return-32042 (#214). E spec-internal item 2 now separates the chosen MUST reading from the unimplemented behaviour; A/C11 and E cross item 12 corrected to match (client propagates -32042, client.go:630/1549-1586).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eff192df75
ℹ️ 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".
…positions per Codex round 2 (#2210) All three P2s verified in code before fixing: - Headline 12 cache gap was false: the modern envelope adds ttlMs and cacheScope to the discover result (server.go:762-763, TestModernEnvelope). Gap claims removed from headline 12, A/C17, C/C14, D/C11 and the requirements.json discover note truthed up to match. - D/C10: the default preference is PreferModern — probe() sends server/discover first (client.go:321-325, probe.go:296); initialize first only under a legacy preference or cached legacy era. - C/C9: Complete() implements the 2024-11-05 exception (ask anyway, -32601 read as absence, request.go:52-65) and the server serves from 2024-11-05 (Oldest, revisions.go:14) — not moot on either side.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 957fab253d
ℹ️ 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".
…d 3 (#2210) C3 recorded that mcpx as client POSTs notifications/cancelled on HTTP too. For a 2026-07-28 HTTP upstream that is the opposite of the code: cancelled() returns early for an HTTPTransport with a meta version set (internal/mcpclient/modern.go:582) and closing the response stream is the cancellation signal; TestModernHTTPCancellationClosesTheStream verifies no notification is POSTed. The disposition and the item text now record the era split (notification POST remains for stdio and legacy-era HTTP), and the repeated claim in the cancellation-stdio-client-must-send row of docs/spec/requirements.md is corrected the same way; requirements.json regenerated with genreq (which also converges one stale discover note toward the md already on main). Pre-existing on this branch, unrelated: internal/conformance TestAuthSplit/TestMatrix fail identically on the clean parent head (authsplit bindings; branch is behind main).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f423fd66b
ℹ️ 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".
| ## C. Cancellation, progress, pagination, logging, completion, tasks, caching, MRTR, subscriptions | ||
|
|
||
| **C1 — 2026-07-28 server-initiated subscription teardown: three texts, three answers (spec-internal).** | ||
| - **Disposition:** Recorded reading — headline 1: on stdio send `notifications/cancelled` (the MUST), then the listen result for a graceful end (the SHOULD); on HTTP send the listen result and close the stream. The reading is recorded in the comment block at internal/mcpserver/listen.go. |
There was a problem hiding this comment.
When mcpx itself ends a modern HTTP subscription, it sends notifications/cancelled before the closing result too: listen() stores the HTTP SSE sender in the listen stream (internal/mcpserver/listen.go:183), notifier teardown calls closeListen() (:211–216), and announceEnd() unconditionally sends both frames (:366–377). Consequently, the cited implementation does not follow this claimed transport-specific reading. Distinguish the chosen reading from the implementation gap here and in the repeated headline/B dispositions, rather than reporting the HTTP behavior as settled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b3c6d1f (pushed): verified against code — listen() stores the HTTP SSE sender in the listen stream (listen.go:183), closeListen (:354) calls announceEnd (:365-377), which sends notifications/cancelled and then the listen result unconditionally on every transport. All four places that stated the transport split as behaviour now distinguish the recorded reading from the implementation: headline 1, B-C12 disposition, C-C1 disposition, and C-C1 safest-reading sentence each record that mcpx sends both frames on modern HTTP too, i.e. an implementation gap against the reading (following the cancellation page unqualified MUST over the schema stdio scoping), not settled HTTP behaviour.
…2210) Codex round-4 P2 on PR 2892: announceEnd sends notifications/cancelled + listen result on every transport including modern HTTP, so the transport-split reading in headline 1 / B-C12 / C-C1 is a recorded reading with an implementation gap, not settled behaviour.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Phase 1 of #2209 (origin #1974), completing the work started in 934df38: every conflict in the mcpx revision-conflicts register now carries a disposition.
What changed (docs only)
pkgs/mcpx/docs/spec/revision-conflicts.md:internal/mcpclient/client.go:1138-1145; modern unknown method = 404 +-32601,internal/mcpserver/server.go:879); headline 13 records the section C item C10 recorded reading for the missing-required-task code (-32601,internal/mcpserver/tasks.go:244) while cancelled-task finality stays undisposed with its question named.protocol.md§1.1 closing sentence names the disposition index,requirements.jsondiscover notes truthed up.Code sites verified fresh this shift:
transport.go:106-116(batchRefused/runBatch), server.go session disconnect block,client.go:1138-1145(404 recovery),request.go:45-61(completions gate),conn.go:396(server-sent cancel),revisions.go:237(downgrade()),tasks.go:244,608.No Go code or behaviour changes in this phase.
Closes #2210
Need help on this PR? Tag
@codesmith-botwith what you need.