Skip to content

Fix main: bridge test references removed meta key set - #6030

Closed
JAORMX wants to merge 1 commit into
mainfrom
fix-main-reserved-meta-test-build
Closed

Fix main: bridge test references removed meta key set#6030
JAORMX wants to merge 1 commit into
mainfrom
fix-main-reserved-meta-test-build

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

main does not currently build its tests. pkg/vmcp/server/bridge_regression_test.go:506 references mcpparser.ReservedModernMetaKeys, which no longer exists:

vet: pkg/vmcp/server/bridge_regression_test.go:506:38: undefined: mcpparser.ReservedModernMetaKeys

Both go vet ./... and task lint fail for everyone on main, and this is already failing other people's PRs#6022 is red on Linting / Lint Go Code and Tests / Test Go Code with exactly this error, having been green on its previous head before these landed.

Neither PR was wrong on its own. #6024 replaced the enumerated reserved-key slice with ReservedMetaPrefix + a passthrough exemption, and converted the three call sites that existed on its branch (modern_envelope_test.go, revision_realbackend_test.go, mcp_session_meta_propagation_test.go). #6006 added this file, which uses the old symbol. Each was green; the merge of the two is not. Same semantic-conflict class as an interface change landing beside a new call site — which bit #6013 earlier today for the same underlying reason.

The fix converts the assertion to the prefix scan #6024 established, and is strictly stronger than the original: it catches any reserved io.modelcontextprotocol/* key, including ones added after this test was written, rather than only the four the removed slice happened to list.

Type of change

  • Bug fix

Test plan

  • Unit tests (task test) — pkg/vmcp/server green, full package, 94s
  • Linting (task lint) — 0 issues; go vet ./... clean (this is the gate that was failing)
  • Manual testing (described below)

Confirmed the break is pre-existing and not introduced by me: git stash && go vet ./pkg/vmcp/server/ on unmodified main reproduces the identical error. Also confirmed #6022's lint annotation is byte-for-byte the same failure, so this unblocks that PR too.

One thing I did not manage to verify, stated plainly: I could not mutation-verify that the converted assertion still fires on a real leak. Neutering it leaves the test passing, because the strip works and no reserved key is present either way — it is a regression guard, not a currently-firing check. What I can attest to is that the build is restored, the property asserted is unchanged in intent, and the key set it covers is now a superset of before.

API Compatibility

  • This PR does not break the v1beta1 API.

Test-only change.

Does this introduce a user-facing change?

No.

Special notes for reviewers

  • legacyRequestPassthroughMetaKeys duplicates pkg/mcp's unexported passthroughMetaKeys. That duplication is deliberate but unfortunate — if the production set gains an entry and this one doesn't, the test starts failing on a legitimate passthrough. The comment says to prefer widening the production set. If you'd rather export the production set and drop the local copy, that's a cleaner end state and I'm happy to do it instead.
  • Worth considering whether go vet ./... belongs in a fast-failing CI job. task lint does run it, so CI did catch this — but only on the PRs that came after, not on the merge that caused it. A branch-protection check on main post-merge would have flagged it at the source.

Generated with Claude Code

main does not build its tests: pkg/vmcp/server/bridge_regression_test.go
references mcpparser.ReservedModernMetaKeys, which no longer exists. Both
`go vet ./...` and `task lint` fail for everyone on main.

Neither PR was wrong on its own. #6024 replaced the enumerated reserved-key
slice with ReservedMetaPrefix plus a passthrough exemption, converting the
three call sites that existed on its branch. #6006 added this file, which
uses the old symbol. Each was green; the merge of both is not -- the same
semantic-conflict class as an interface change landing beside a new call
site.

Convert the assertion to the prefix scan #6024 established in
modern_envelope_test.go and revision_realbackend_test.go. This is also
strictly stronger than the original: it catches any reserved key, including
ones added after this test was written, rather than only the four the old
slice happened to list.

The passthrough keys are mirrored locally because pkg/mcp's set is
unexported, with a comment to keep them in sync.
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Jul 27, 2026
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.18%. Comparing base (f0de82f) to head (4a273ce).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6030      +/-   ##
==========================================
+ Coverage   72.15%   72.18%   +0.02%     
==========================================
  Files         721      721              
  Lines       74991    75025      +34     
==========================================
+ Hits        54113    54156      +43     
+ Misses      17015    17007       -8     
+ Partials     3863     3862       -1     

☔ 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

JAORMX commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author

Closing — superseded by #6029, which landed the same fix first and did it better.

We converged on the same diagnosis independently (the #6024 prefix-API rename versus the bridge_regression_test.go #6006 added, each green alone), and both fixes convert the assertion to a prefix scan. The difference is the passthrough handling: I mirrored pkg/mcp's unexported passthroughMetaKeys into the test and exempted those keys, and flagged that duplication in this PR's own review notes as its weak point — if the production set gains an entry and the copy doesn't, the test fails on a legitimate passthrough.

#6029 avoids that entirely with a blanket prefix check, justified precisely: "No passthroughMetaKeys entry is in play on a tools/call, so a blanket prefix check is exact." That is the better call — it is strictly simpler and has no set to keep in sync. go vet ./... on main is clean now.

Two follow-on notes:

The one item here worth not losing: this class of failure — two independently-green PRs whose merge does not build — has now happened twice today (this, and #6013 meeting a new CallTool call site from #6010). A post-merge go vet ./... check on main would catch it at the source rather than in the next contributor's PR, where it reads as their failure. Raised in this PR's description; noting it here so it survives the close.

@JAORMX JAORMX closed this Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants