Route OIDCConfigHash through StatusManager in VirtualMCPServer

[ChrisJBurns]

Summary

handleOIDCConfig wrote OIDCConfigHash directly via r.Status().Update(ctx, vmcp) while every other VirtualMCPServer status field used the batched StatusManager. This mixed-write pattern could produce spurious 409 Conflict errors when the original vmcp object's resourceVersion became stale after earlier applyStatusUpdates calls in the same reconciliation.

Adds SetOIDCConfigHash to the StatusManager interface and routes both the set and clear paths through it, eliminating all direct VirtualMCPServer status writes from handleOIDCConfig.

Fixes #4504

Type of change

• Bug fix

Changes

File	Change
types.go	Add SetOIDCConfigHash(hash string) to StatusManager interface
collector.go	Add oidcConfigHash *string field, setter, UpdateStatus application logic, debug log
virtualmcpserver_controller.go	Replace 2 direct r.Status().Update() calls with statusManager.SetOIDCConfigHash(...)
collector_test.go	Add set + clear tests, update batched test
mocks/mock_collector.go	Regenerated via go generate

Test plan

• Unit tests (task test) — all pass, including 2 new collector tests
• Linting (task lint) — 0 issues
• Existing integration tests unaffected (no VirtualMCPServer unit test exercises handleOIDCConfig)

Special notes for reviewers

• The r.Status().Update(ctx, oidcConfig) in updateOIDCConfigReferencingWorkloads is intentionally unchanged — it writes to the MCPOIDCConfig resource (different resource), not VirtualMCPServer.
• Empty string clears the hash (matching SetURL/SetMessage semantics). The *string pointer in the collector distinguishes "never called" from "called with empty string".
• After this fix, handleOIDCConfig makes zero direct VirtualMCPServer status writes on any path.

Generated with Claude Code

[ChrisJBurns]

@claude Please review this PR with the agents. Only mention critical/high issues and non controversial medium issues. ignore issues around style and preference. give an approve or reject conclusion

[claude]

Claude finished @ChrisJBurns's task in 2m 44s —— View job

---

PR Review: Route OIDCConfigHash through StatusManager

• Read changed files
• Review with vmcp-review agent
• Post findings

---

Summary

The change is correct and safe. It eliminates the mixed-write pattern in handleOIDCConfig that caused spurious 409 conflicts by routing both the set and clear paths through StatusManager, consistent with all other status fields. applyStatusUpdates fetches a fresh resource version before each write, so the batched write is race-safe.

---

Issues Found

Medium — Pre-existing: UpdateStatus has no reset after successful application

StatusCollector never resets hasChanges = false (or clears collected fields) after UpdateStatus returns. This means if applyStatusUpdates is called twice on the same statusManager instance — which happens on some error paths — all previously collected fields are re-applied against a freshly fetched object. This PR adds oidcConfigHash to the set of fields subject to this behavior, but the issue exists identically for all other fields (phase, message, url, etc.). This is not a regression introduced here.

Medium — Pre-existing: Hash comparison reads stale vmcp.Status.OIDCConfigHash

handleOIDCConfig reads vmcp.Status.OIDCConfigHash from the object captured at reconcile-start (line 2774, 2835). If an earlier applyStatusUpdates call already persisted a new hash to etcd, the in-memory value is stale and the change guard vmcp.Status.OIDCConfigHash != oidcConfig.Status.ConfigHash can fire spuriously, triggering a redundant but harmless write. Again, this is a pre-existing condition applying equally to all status fields — not introduced or worsened by this PR.

Both issues are candidates for a follow-up cleanup (resetting hasChanges in UpdateStatus, or updating the in-memory status post-write), but they don't block this fix.

---

Conclusion: Approve

The PR correctly fixes the stated bug. The new SetOIDCConfigHash implementation is consistent with all existing setters, tests cover both set and clear paths, and the mock is properly regenerated. No issues introduced by this change.

[ChrisJBurns]

Agent Review of Findings

Three specialist agents (Kubernetes operator expert, Go code writer, code reviewer) independently traced the full reconcile flow to evaluate the two medium findings. All three reached the same conclusion: both are non-issues that require no follow-up.

---

Finding 1: UpdateStatus has no reset after successful application

Verdict: Non-issue.

• StatusCollector is created fresh per reconciliation (line 146) as a local variable — no cross-reconciliation reuse.
• Every call to applyStatusUpdates is immediately followed by a return. The agents exhaustively traced all call sites (lines 245, 317, 325, 333, 342, 352, 161, 170, 176, 218) and confirmed exactly one invocation executes per reconciliation — there is no fall-through from one call site to another.
• Even hypothetically, double-application is fully idempotent — applyStatusUpdates re-fetches a fresh latest from the API server before applying, so overwriting the same values is a no-op at the data level.
• Adding a reset would actually introduce risk: a future defer-based refactoring could silently lose updates collected after a partial apply. The current monotonic-accumulate pattern is the safer default for collect-then-flush.

Finding 2: Hash comparison reads stale vmcp.Status.OIDCConfigHash

Verdict: Non-issue. The premise is incorrect.

• The agents traced the exact control flow: runValidations (line 152) only calls applyStatusUpdates on error paths that return early from Reconcile before reaching handleOIDCConfig (line 160). The happy path (which is the only path that continues to handleOIDCConfig) makes zero calls to applyStatusUpdates.
• So when handleOIDCConfig reads vmcp.Status.OIDCConfigHash at line 2835, the value is exactly what was fetched from the API server at reconcile-start (line 135). No intervening write has occurred.
• Additionally, applyStatusUpdates writes to a freshly fetched latest object (line 268), never mutating the caller's vmcp — so even if someone added a new applyStatusUpdates call before handleOIDCConfig, the in-memory value would still be correct.
• Only handleOIDCConfig ever calls SetOIDCConfigHash, so no other step could have persisted a different hash value.

---

Both findings are artifacts of structural code reading (seeing a missing reset, seeing a potentially stale field) without tracing the actual control flow. The control flow makes both scenarios unreachable.

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.93%. Comparing base (ffef243) to head (8180e6d).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...perator/controllers/virtualmcpserver_controller.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4539      +/-   ##
==========================================
+ Coverage   68.86%   68.93%   +0.07%     
==========================================
  Files         505      505              
  Lines       52320    52380      +60     
==========================================
+ Hits        36028    36110      +82     
+ Misses      13504    13481      -23     
- Partials     2788     2789       +1     

☔ View full report in Codecov by Sentry.
📢 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.