fix(mcp): keep every known MCP revision negotiable under rmcp 3 - #337
Merged
Merged
Conversation
This was referenced Aug 11, 2026
sgathrid
force-pushed
the
fix/mcp-protocol-negotiation
branch
from
August 11, 2026 15:15
f1bd1c8 to
91953b2
Compare
sgathrid
added a commit
that referenced
this pull request
Aug 11, 2026
`on.pull_request.branches` filters on a PR's *base*, and a stacked PR targets its parent topic branch. The allowlist `[main, worktree-rust-migration-v0.4.0, 'release/**']` therefore matched no stacked PR at all: #337 (base `chore/rmcp-3x-264`) drew zero Actions runs, so a stacked change was reviewed and merged with no verification behind it. `ci.yml` has no `workflow_dispatch`, so it could not even be triggered by hand. Widening the allowlist to topic-branch globs would admit every PR aimed at anyone's branch, which is not the intent. Instead the trigger is unfiltered and the policy moves into a `gate` job backed by `scripts/ci_gate.py`: * `push` — already filtered by the trigger. Run. * `pull_request` into a `TRUNK_PATTERNS` branch. Run, decided before any API call, so the path to `main` gains no new dependency. * `pull_request` belonging to a GitHub stack, whatever it targets. Run. * Any other `pull_request` — a one-off aimed at a topic branch. Skip. Stack membership is real data, not inferred from branch names: the `PullRequestStack` GraphQL API exposes `pullRequest.stack { number size }`, which returns a stack for #337 and #324 (stack #338, positions 2 and 1) and `null` for non-stacked PRs such as #320 and #318. `--selftest` covers all twelve decision-table cases and runs as the gate's first step, including the `push` path that could otherwise break `main` and a check that `release/*` does not match across a slash. Actions globs are not `fnmatch` — `*` stops at `/` while `**` crosses it — so the matcher is implemented rather than delegated. Unverifiable locally, and deliberately loud about it: whether an Actions installation token can read `PullRequestStack` is a different question from whether the logic is right. The local checks used a personal token. So the gate prints the raw GraphQL response including any `errors` array, never fails its own step, and fails *open* — an unreadable response runs CI with a `::warning::` saying the stacked-PR-only policy is not in effect, rather than silently skipping verification. This PR's own run is the test of that access. No CHANGELOG entry: CI plumbing, per `.agents/AGENTS.md:90`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeremy-wayland
pushed a commit
that referenced
this pull request
Aug 12, 2026
`on.pull_request.branches` filters on a PR's *base*, and a stacked PR targets its parent topic branch. The allowlist `[main, worktree-rust-migration-v0.4.0, 'release/**']` therefore matched no stacked PR at all: #337 (base `chore/rmcp-3x-264`) drew zero Actions runs, so a stacked change was reviewed and merged with no verification behind it. `ci.yml` has no `workflow_dispatch`, so it could not even be triggered by hand. Widening the allowlist to topic-branch globs would admit every PR aimed at anyone's branch, which is not the intent. Instead the trigger is unfiltered and the policy moves into a `gate` job backed by `scripts/ci_gate.py`: * `push` — already filtered by the trigger. Run. * `pull_request` into a `TRUNK_PATTERNS` branch. Run, decided before any API call, so the path to `main` gains no new dependency. * `pull_request` belonging to a GitHub stack, whatever it targets. Run. * Any other `pull_request` — a one-off aimed at a topic branch. Skip. Stack membership is real data, not inferred from branch names: the `PullRequestStack` GraphQL API exposes `pullRequest.stack { number size }`, which returns a stack for #337 and #324 (stack #338, positions 2 and 1) and `null` for non-stacked PRs such as #320 and #318. `--selftest` covers all twelve decision-table cases and runs as the gate's first step, including the `push` path that could otherwise break `main` and a check that `release/*` does not match across a slash. Actions globs are not `fnmatch` — `*` stops at `/` while `**` crosses it — so the matcher is implemented rather than delegated. Unverifiable locally, and deliberately loud about it: whether an Actions installation token can read `PullRequestStack` is a different question from whether the logic is right. The local checks used a personal token. So the gate prints the raw GraphQL response including any `errors` array, never fails its own step, and fails *open* — an unreadable response runs CI with a `::warning::` saying the stacked-PR-only policy is not in effect, rather than silently skipping verification. This PR's own run is the test of that access. No CHANGELOG entry: CI plumbing, per `.agents/AGENTS.md:90`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeremy-wayland
force-pushed
the
fix/mcp-protocol-negotiation
branch
from
August 12, 2026 15:15
91953b2 to
5649217
Compare
`31f0733` narrowed `SUPPORTED_PROTOCOL_VERSIONS` to `2025-11-25` and `2026-07-28` on the reasoning that the three older revisions had no test coverage. Under rmcp 3 that override is not advertisement-only: rmcp 3.1.0 (#1093) rewired `negotiate_protocol_version` to check the handler's list where rmcp 2.2 checked the SDK's `ProtocolVersion::KNOWN_VERSIONS`, and upstream documents the list as bounding `initialize` negotiation and validating per-request `_meta` versions. Narrowing it therefore stopped Topos speaking the revisions it dropped. The failure is quiet rather than loud: `initialize` with `2025-06-18` does not error, it answers `2025-11-25`, and per the MCP lifecycle spec a client that does not support the returned version should disconnect. A request declaring the older revision in `_meta` is a hard `-32022`. The stated rationale does not hold, because there is no per-revision code to cover. The only version-conditional behavior in rmcp sits at the `2026-07-28` boundary (SEP-2243 headers, the inline lifecycle, per-request `_meta`); every revision below it takes the same session-model path, and Topos returns byte-identical payloads across all of them — verified by diffing `tools/list` + `resources/list` output across negotiated `2024-11-05`, `2025-06-18`, and `2025-11-25` (43279 bytes each, differing only in response arrival order). Restoring the revisions costs no code path and no maintenance; omitting them only costs hosts. The list stays spelled out rather than deferring to rmcp's default so an SDK upgrade that adds a revision is an explicit decision here. For the same reason the `rmcp` bound moves from `"3"` to `"3.1"`: negotiation semantics changed in a *minor* release, so a looser bound lets protocol behavior move with no diff. Behavioral proof: `initialize_negotiates_every_supported_revision` walks all five revisions, asserting the handshake echoes each one and that the session then serves the same tool surface. Reintroducing the narrowing fails it (`left: "2025-11-25", right: "2024-11-05"`) alongside the `server/discover` assertion, so the guard is verified against the actual regression rather than assumed. Verification: `cargo test --workspace` 647 passed / 0 failed; `cargo clippy --workspace --all-targets` and `cargo fmt --all` clean; `topos_assess_worktree_change` on `server.rs` reports LATERAL_MOVE with verdict unchanged at SIMPLE_SECURE and all metric deltas 0 except `ast.entropy` +0.003 from the added comments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeremy-wayland
pushed a commit
that referenced
this pull request
Aug 12, 2026
`on.pull_request.branches` filters on a PR's *base*, and a stacked PR targets its parent topic branch. The allowlist `[main, worktree-rust-migration-v0.4.0, 'release/**']` therefore matched no stacked PR at all: #337 (base `chore/rmcp-3x-264`) drew zero Actions runs, so a stacked change was reviewed and merged with no verification behind it. `ci.yml` has no `workflow_dispatch`, so it could not even be triggered by hand. Widening the allowlist to topic-branch globs would admit every PR aimed at anyone's branch, which is not the intent. Instead the trigger is unfiltered and the policy moves into a `gate` job backed by `scripts/ci_gate.py`: * `push` — already filtered by the trigger. Run. * `pull_request` into a `TRUNK_PATTERNS` branch. Run, decided before any API call, so the path to `main` gains no new dependency. * `pull_request` belonging to a GitHub stack, whatever it targets. Run. * Any other `pull_request` — a one-off aimed at a topic branch. Skip. Stack membership is real data, not inferred from branch names: the `PullRequestStack` GraphQL API exposes `pullRequest.stack { number size }`, which returns a stack for #337 and #324 (stack #338, positions 2 and 1) and `null` for non-stacked PRs such as #320 and #318. `--selftest` covers all twelve decision-table cases and runs as the gate's first step, including the `push` path that could otherwise break `main` and a check that `release/*` does not match across a slash. Actions globs are not `fnmatch` — `*` stops at `/` while `**` crosses it — so the matcher is implemented rather than delegated. Unverifiable locally, and deliberately loud about it: whether an Actions installation token can read `PullRequestStack` is a different question from whether the logic is right. The local checks used a personal token. So the gate prints the raw GraphQL response including any `errors` array, never fails its own step, and fails *open* — an unreadable response runs CI with a `::warning::` saying the stacked-PR-only policy is not in effect, rather than silently skipping verification. This PR's own run is the test of that access. No CHANGELOG entry: CI plumbing, per `.agents/AGENTS.md:90`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeremy-wayland
force-pushed
the
fix/mcp-protocol-negotiation
branch
from
August 12, 2026 15:22
5649217 to
6f3d343
Compare
krv-labs-ai-developer
self-requested a review
August 14, 2026 21:52
krv-labs-ai-developer
approved these changes
Aug 14, 2026
jeremy-wayland
added a commit
that referenced
this pull request
Aug 14, 2026
* ci: run CI on stacked PRs, gated on real stack membership `on.pull_request.branches` filters on a PR's *base*, and a stacked PR targets its parent topic branch. The allowlist `[main, worktree-rust-migration-v0.4.0, 'release/**']` therefore matched no stacked PR at all: #337 (base `chore/rmcp-3x-264`) drew zero Actions runs, so a stacked change was reviewed and merged with no verification behind it. `ci.yml` has no `workflow_dispatch`, so it could not even be triggered by hand. Widening the allowlist to topic-branch globs would admit every PR aimed at anyone's branch, which is not the intent. Instead the trigger is unfiltered and the policy moves into a `gate` job backed by `scripts/ci_gate.py`: * `push` — already filtered by the trigger. Run. * `pull_request` into a `TRUNK_PATTERNS` branch. Run, decided before any API call, so the path to `main` gains no new dependency. * `pull_request` belonging to a GitHub stack, whatever it targets. Run. * Any other `pull_request` — a one-off aimed at a topic branch. Skip. Stack membership is real data, not inferred from branch names: the `PullRequestStack` GraphQL API exposes `pullRequest.stack { number size }`, which returns a stack for #337 and #324 (stack #338, positions 2 and 1) and `null` for non-stacked PRs such as #320 and #318. `--selftest` covers all twelve decision-table cases and runs as the gate's first step, including the `push` path that could otherwise break `main` and a check that `release/*` does not match across a slash. Actions globs are not `fnmatch` — `*` stops at `/` while `**` crosses it — so the matcher is implemented rather than delegated. Unverifiable locally, and deliberately loud about it: whether an Actions installation token can read `PullRequestStack` is a different question from whether the logic is right. The local checks used a personal token. So the gate prints the raw GraphQL response including any `errors` array, never fails its own step, and fails *open* — an unreadable response runs CI with a `::warning::` saying the stacked-PR-only policy is not in effect, rather than silently skipping verification. This PR's own run is the test of that access. No CHANGELOG entry: CI plumbing, per `.agents/AGENTS.md:90`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(ci): correct how TRUNK_PATTERNS relates to the trigger filters The comment described `TRUNK_PATTERNS` as mirroring the old `on.pull_request.branches` allowlist "plus `on.push.branches`", but the two differ: the push filter has no `release/**`. Reading it as a union would suggest a push to `release/*` consults the list, when the trigger never fires for one at all — pushes are filtered before the gate runs, and `decide()` admits any non-PR event outright. Also states plainly that nothing enforces the correspondence with the YAML, so the sync is by hand. Comments only; no behavior change. `--selftest` 12/12, `actionlint` clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * remove rust migration branch --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: jeremy-wayland <jeremy.don.wayland@gmail.com>
This was referenced Sep 16, 2026
Closed
Closed
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Do not merge #324 without this PR.
#324 narrowed
SUPPORTED_PROTOCOL_VERSIONSto two MCP revisions. Under rmcp 3.1, that list is no longer advertisement-only — it also boundsinitializenegotiation. Older clients quietly get2025-11-25instead of the version they asked for (or hard-fail on_meta).This PR restores all five known revisions, pins
rmcpto3.1, and adds a regression test.fix/mcp-protocol-negotiation(91953b2)chore/rmcp-3x-264(#324)2024-11-05/2025-03-26/2025-06-18stop negotiating correctlyProblem
server/discovernegotiate_protocol_versioninitializewith e.g.2025-06-182025-11-25(quiet fallback)_metawith dropped revision-32022There is no per-revision code path in Topos below the
2026-07-28boundary. Dropping revisions costs hosts and buys no safety.Our own CI MCP smoke test already speaks
2025-06-18and only grepstools/list— so the repo was already relying on that revision working.Fix
ProtocolVersionvalues inSUPPORTED_PROTOCOL_VERSIONS(2024-11-05…2026-07-28).rmcp = "3.1"— negotiation semantics changed in a minor release;"3"is too loose.initialize_negotiates_every_supported_revision— handshake echoes each revision; tool surface stays the same.Files
topos/mcp/src/server.rstopos/mcp/Cargo.tomlrmcp"3"→"3.1"topos/mcp/tests/lifecycle.rsCHANGELOG.mdHow to review
server.rs(supported_protocol_versions).No tool/resource/prompt surface changes.
Validate
Merge
Do not land #324 without #337.