Consolidate the negotiate-down rationale

[JAORMX]

Stacked on #6013 — base is validate-xmcp-header-annotations, so review only the top commit
(Consolidate the negotiate-down rationale). GitHub will retarget this to main automatically
when #6013 merges. Stacked rather than branched off main because both PRs edit
pkg/vmcp/client/modern.go and client.go.

Summary

#6002 item 4. The rule that supportedVersions — not a clean server/discover response — is the authoritative Modern signal was explained in full at three separate sites: the errModernNegotiatedDown doc in modern.go, and both modernDiscover and probeRevision in client.go.

That matters more than ordinary comment duplication because it is precisely the surface someone must edit when the exact-match tripwire fires for a newer Modern revision. Three copies is three chances to update two of them and leave the third quietly contradicting the code.

The explanation now lives on errModernNegotiatedDown — the thing being explained — marked as the canonical site and naming its two dependents. The other two back-reference it. Both retain the concrete local behaviour they describe (what each function returns, and when); only the shared reasoning moved, so neither doc comment became less useful on its own.

Comment-only: no statement, expression, or signature changed.

Type of change

• Other (describe): comment-only de-duplication

Test plan

• Unit tests (task test) — unchanged; only pkg/api and pkg/secrets/keyring fail, both environmental and verified identical on main
• Linting (task lint) — 0 issues (this is the gate CI runs: golangci-lint plus go vet ./...)
• Manual testing (described below)

Verified the de-duplication actually happened rather than being asserted: grep -rn "answers both server/discover\|answers server/discover even" pkg/vmcp/client/*.go now returns exactly one hit, the canonical site in modern.go. Before the change it returned three.

API Compatibility

• This PR does not break the v1beta1 API.

No API surface of any kind is touched.

Does this introduce a user-facing change?

No.

Special notes for reviewers

The issue itself rates this item low-value, and I agree — it is worth having only because that comment block is load-bearing for the tripwire. Reasonable to reject in favour of leaving the duplication in place; nothing depends on it.

Refs #6002

Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.18%. Comparing base (0a9e32a) to head (a163320).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6017      +/-   ##
==========================================
- Coverage   72.21%   72.18%   -0.04%     
==========================================
  Files         721      721              
  Lines       74987    74987              
==========================================
- Hits        54152    54128      -24     
- Misses      16956    16999      +43     
+ Partials     3879     3860      -19     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JAORMX]

Rebased onto main now that #6013 has merged, and the diff is worth re-reading — it was briefly wrong.

#6013 was squash-merged as a single commit (83394ee5), so neither of this branch's two parent commits was an ancestor of main any more. GitHub retargeted this PR to main as expected, but the stack reappeared in the diff: it showed 3 commits, 36 files, +1644/-118 and picked up a size/XL label, for a change that is comment-only.

Rebuilt as main + the single comment commit. Now 2 files, +20/-17, which is the actual change. The size/XL label is stale and should drop on the next size calculation.

Re-verified after the rebase rather than assuming the cherry-pick was faithful:

• task lint (the CI gate: golangci-lint plus go vet ./...) — 0 issues
• The de-duplication still holds: exactly one remaining full statement of the rationale, the canonical one in modern.go; zero in client.go

Also note the earlier E2E Test Lifecycle (kind v1.34.3) failure on the pre-rebase head was a flake, not this change. It failed with a bare exit code 201 and no test-level annotation, while all three E2E Lifecycle jobs passed on ad85feb1 — the exact base this branch sat on, whose only delta from it was comment text. The rebase re-triggers CI, so that should resolve itself; if v1.34.3 fails again on a1633208 I'll treat it as real and dig in.

[jhrozek]

Reviewed the diff — comment-only, verified nothing but // lines changed. The consolidation is the right call: three copies of a rule that all have to change together when a newer Modern revision lands is a real hazard, and keeping it on errModernNegotiatedDown puts it on the thing being explained.

Two textual nits, neither blocking:

1. modern.go:110 says "the exact-match tripwire in modernDiscover", but the tripwire lives in discoverModernCapabilities (client.go:1068) — modernDiscover just builds the client and delegates. The other two references in this file get it right (modern.go:427 and :433 both name discoverModernCapabilities), so this one is the odd one out. Slightly unfortunate given the whole point is a single authoritative place naming the edit surface.

2. The removed modernDiscover text cited mcp/client.go:428-444, and the surviving canonical cites :360-369. Those are different spots in go-sdk — :360-369 is the -32022/data.supported path (source 2), :428-444 was the discover/supportedVersions path (source 1). :428-444 is now gone from the repo entirely, so the canonical block documents both sources but only carries source 2's citation. Worth listing both.

Happy either way — fold them in or land as-is.