Skip to content

Unblock main CI: fix vMCP nil-registry panic and CRD docs drift - #5999

Merged
jhrozek merged 2 commits into
mainfrom
fix-vmcp-probe-nil-registry
Jul 26, 2026
Merged

Unblock main CI: fix vMCP nil-registry panic and CRD docs drift#5999
jhrozek merged 2 commits into
mainfrom
fix-vmcp-probe-nil-registry

Conversation

@jhrozek

@jhrozek jhrozek commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Main is currently red on two independent checks, both fallout from the "Handle per-backend MCP revision in vMCP" merge. Since every PR branches off main, a fix for just one still inherits the other's red and never gets a green run — so this PR clears both.

1. Tests / Test Go Code — nil-registry panic

TestRegression_ToolSchemaFidelity_PreservesCompositors panics with a nil-pointer dereference, and because a panic tears down the whole pkg/vmcp/client test binary, every test in the package reports as failed.

Why: the test builds httpBackendClient as a raw struct literal that never sets registry. The per-backend revision handling made ListCapabilities run probeRevision -> buildModernHTTPClient -> resolveAuthStrategy, which calls h.registry.GetStrategy(...) on the nil registry (client.go:454). The real constructor NewHTTPBackendClient rejects a nil registry, so this is a test-setup gap, not a missing production guard.

What: build the probe client via the existing newProbeClient(t) helper (wires a real registry with UnauthenticatedStrategy). Test-only.

2. Operator CI / Generate CRD Docs — stale CRD reference

The same merge added an MCPRevision field to the backend status but did not regenerate docs/operator/crd-api.md, so crd-ref-docs + git diff --exit-code fails. Regenerated to add the missing mcpRevision entry. Generated-docs only.

Both are carved from the fixes bundled in #5993 (go-sdk v1.7 migration) so main and dependent PRs can go green now instead of waiting on that larger PR. cc @JAORMX#5993 can rebase cleanly over this.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Test plan

  • Unit tests (task test): go test ./pkg/vmcp/client/ -run TestRegression_ToolSchemaFidelity_PreservesCompositors -count=2 — panics before, passes after.
  • task crdref-gen produces no further diff after this change.

Does this introduce a user-facing change?

No.

Generated with Claude Code

main is red: TestRegression_ToolSchemaFidelity_PreservesCompositors
panics with a nil-pointer dereference, which crashes the whole
pkg/vmcp/client test binary and fails every test in it. The test builds
httpBackendClient as a raw struct literal that never sets registry;
since the per-backend revision handling landed, ListCapabilities now
runs probeRevision -> buildModernHTTPClient -> resolveAuthStrategy, which
calls h.registry.GetStrategy on the nil registry. The real constructor
NewHTTPBackendClient rejects a nil registry, so this is purely a
test-setup gap, not a missing production guard.

Build the probe client through the existing newProbeClient helper, which
wires a real registry with an UnauthenticatedStrategy. Test-only change.

Carved out of #5993 (go-sdk v1.7 migration), which contains the same fix
bundled with unrelated changes, so main and dependent PRs can go green
without waiting on that larger PR.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Jul 26, 2026
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.11%. Comparing base (885e8b1) to head (9d794c2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5999      +/-   ##
==========================================
+ Coverage   72.06%   72.11%   +0.04%     
==========================================
  Files         718      718              
  Lines       74462    74462              
==========================================
+ Hits        53662    53698      +36     
+ Misses      16965    16912      -53     
- Partials     3835     3852      +17     

☔ 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 previously approved these changes Jul 26, 2026

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — this is the right fix for the main breakage, and the diagnosis matches exactly what I traced when #5980 + #5990 landed together: the regression test built httpBackendClient as a bare struct literal with a nil registry, and the post-#5980 ListCapabilities → probeRevision → buildModernHTTPClient → resolveAuthStrategy path derefs h.registry.GetStrategy(...) (client.go:454) → nil panic that tears down the whole pkg/vmcp/client test binary (so every test in the package, and the Test Go Code gate, goes red on main and every open PR).

Routing through newProbeClient(t) (real registry + UnauthenticatedStrategy) is the correct, minimal, test-only fix — and correctly framed as a test-setup gap, not a missing production guard, since NewHTTPBackendClient already rejects a nil registry. Overriding h.clientFactory afterward preserves the test's streamable-HTTP backend. Good call carving this out of #5993 so main can go green now.

Approving. Worth merging promptly since it unblocks CI across all open PRs.

The per-backend MCP revision change added an MCPRevision field to the
backend target status but did not regenerate docs/operator/crd-api.md,
so the "Generate CRD Docs" job (crd-ref-docs + git diff --exit-code)
fails on main and on every PR. Regenerate the reference doc to add the
missing mcpRevision entry. Generated-docs only.

Same root merge as the nil-registry panic fixed in this PR: pulling both
into one change so a single run clears main's CI reds instead of each
fix inheriting the other's red.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jhrozek jhrozek changed the title Fix nil-registry panic in ToolSchemaFidelity regression test Unblock main CI: fix vMCP nil-registry panic and CRD docs drift Jul 26, 2026
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Jul 26, 2026

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving after the scope expansion (head 9d794c2). Good call folding the second main-red check in — both failures are #5980 fallout and, since every branch forks off main, fixing only one would still inherit the other's red, so clearing both together is the right move.

  • Nil-registry panic (Test Go Code): unchanged from my earlier approval — routing through newProbeClient(t) is correct.
  • CRD docs drift (Generate CRD Docs): the one-line mcpRevision entry in docs/operator/crd-api.md matches the MCPRevision field #5980 added to the discovered-backend status, and you confirmed task crdref-gen yields no further diff. Generated-docs only.

Still test/docs-only, no production change. LGTM — worth merging promptly to unblock CI across all open PRs.

@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Jul 26, 2026
@jhrozek
jhrozek merged commit b928cc3 into main Jul 26, 2026
93 checks passed
@jhrozek
jhrozek deleted the fix-vmcp-probe-nil-registry branch July 26, 2026 11:37
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