Skip to content

Replace opaque Step A/B labels in XAA CRD comments#5703

Open
jhrozek wants to merge 2 commits into
mainfrom
xaa-crd-rename-step-ab
Open

Replace opaque Step A/B labels in XAA CRD comments#5703
jhrozek wants to merge 2 commits into
mainfrom
xaa-crd-rename-step-ab

Conversation

@jhrozek

@jhrozek jhrozek commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

The XAASpec struct comments and the generated CRD YAML descriptions used
"Step A" and "Step B" to refer to the two phases of the ID-JAG protocol.
These labels are opaque — they tell the reader nothing about what each step
does or which endpoint it targets, and they're easy to confuse with each
other.

Replace them throughout with action-oriented names:

  • IdP exchange — RFC 8693 token exchange at the user's IdP; produces the ID-JAG JWT
  • target grant — RFC 7523 JWT bearer grant at the target AS; produces the backend access token

The rename covers:

  • MCPExternalAuthConfig's XAASpec (ExternalAuthTypeXAA const comment, the
    type comment, and every field description referencing the old labels).
  • pkg/vmcp/auth/types.XAAConfig — the runtime config struct that also feeds
    the VirtualMCPServer CRD schema (via Spec.Config.OutgoingAuth.Backends[].XAA).
    Left un-renamed initially, this would have made kubectl explain show two
    different names for the same protocol concept depending on which CRD you
    asked.
  • pkg/vmcp/auth/strategies/xaa.go — the XAAStrategy implementation:
    doc comments, slog debug/warn log strings, wrapped error messages, and the
    performStepA/performStepB function names (now performIDPExchange/
    performTargetGrant).
  • Two incidental mentions in the generic pkg/oauthproto/jwtbearer and
    pkg/oauthproto/tokenexchange packages that referenced XAA's steps as an
    example use case.

CRD YAML and the CRD API reference docs (docs/operator/crd-api.md)
regenerated with task operator-manifests and task crdref-gen.

Type of change

  • Documentation update (no production code change)

Test plan

  • task operator-manifests and task crdref-gen ran cleanly; grep -rc "Step A\|Step B" across the repo (.go, .yaml, .md) returns zero matches everywhere, including the VirtualMCPServer CRD (toolhive.stacklok.dev_virtualmcpservers.yaml) and docs/operator/crd-api.md
  • task lint-fix — 0 issues
  • task test — full unit suite passes

Generated with Claude Code

"Step A" and "Step B" in the XAASpec struct comments and the generated
CRD YAML descriptions don't tell the reader what each step does or which
endpoint it targets. Rename throughout to "IdP exchange" (RFC 8693 token
exchange at the user's IdP, produces the ID-JAG) and "target grant"
(RFC 7523 JWT bearer grant at the target AS, produces the backend access
token). Regenerate the CRD YAML with task operator-manifests.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Jul 1, 2026
@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 70.61%. Comparing base (2aca563) to head (091392a).

Files with missing lines Patch % Lines
pkg/vmcp/auth/strategies/xaa.go 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5703      +/-   ##
==========================================
- Coverage   70.63%   70.61%   -0.02%     
==========================================
  Files         667      667              
  Lines       67607    67607              
==========================================
- Hits        47752    47742      -10     
- Misses      16399    16411      +12     
+ Partials     3456     3454       -2     

☔ 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 2, 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.

Multi-agent panel review 🤖

Reviewed by a 4-axis panel (comment-vs-code accuracy · CRD generated-file consistency · OAuth/XAA protocol domain · doc clarity). Non-blocking COMMENT.

Consensus: approve-with-nits. Within the diff's scope everything checks out — the new labels are protocol-accurate (RFC 8693 "IdP exchange" / RFC 7523 "target grant", verified field-by-field against xaa.go), the Go godoc and both generated CRD YAMLs are byte-consistent, and the diff is reproducible by controller-gen with no hand-editing. Using distinct verbs — exchange (8693) vs grant (7523) — genuinely encodes the two OAuth mechanisms rather than just renaming; nice call.

The one substantive issue is that the rename is incomplete.

Main finding — incomplete rename (all four axes flagged this; doc-clarity rated it high)

The identical XAA "Step A/B" vocabulary still lives in two other places the PR didn't touch:

  • pkg/vmcp/auth/types/types.go:270-325 — a structurally-identical XAA config struct that is the controller-gen source for the VirtualMCPServer CRD. Its generated virtualmcpservers.yaml (both the files/crds and templates copies, ~24 occurrences each) still says "Step A (RFC 8693)" / "Step B (RFC 7523)".
  • pkg/vmcp/auth/strategies/xaa.go:32-33, 82, 125-141, 176 — the strategy godoc, performStepA/performStepB, and debug/error log strings.

After merge, kubectl explain mcpexternalauthconfig...xaa will show "IdP exchange / target grant" while kubectl explain virtualmcpserver...xaa — describing the same ID-JAG two-step exchange — still shows "Step A/B". Two names for one concept is exactly the confusion this PR set out to remove.

Suggested resolution: extend the identical rename to types.go (and xaa.go), regenerate, and include the virtualmcpservers.yaml deltas here — or, if this is intentionally a scoped first step, say so in the PR body and file the follow-up so it isn't silently left half-done.

Minor / nits

  • PR body test-plan claim — "generated YAML contains zero occurrences of 'Step A' or 'Step B'" is true only for mcpexternalauthconfigs.yaml; virtualmcpservers.yaml still contains them. Worth qualifying to the specific CRD.
  • mcpexternalauthconfig_types.go (~L1380) — lone "sent as the resource parameter in the IdP exchange"; every other new usage is article-less ("in IdP exchange"). Drop the article for consistency.
  • mcpexternalauthconfig_types.go (~L1338) — bullet "Target grant (RFC 7523)" uses capital-T while the canonical inline label is lowercase "target grant"; lowercase to match, or accept as sentence-case.
  • Label-basis asymmetry: "IdP exchange" (named by endpoint) vs "target grant" (named by grant type) — accurate, but "target grant" is coined shorthand rather than draft/RFC terminology.

Generated with Claude Code — multi-agent panel review.

The initial rename only touched mcpexternalauthconfig_types.go, leaving
pkg/vmcp/auth/types/types.go and pkg/vmcp/auth/strategies/xaa.go with the
old "Step A"/"Step B" labels for the identical ID-JAG exchange. Since
types.go feeds the VirtualMCPServer CRD schema, kubectl explain showed
two different names for the same protocol concept depending on which
CRD you asked.

Extend the IdP exchange / target grant rename to the runtime auth
types, the XAA strategy implementation (including the
performStepA/performStepB function names, log strings, and error
messages), and two incidental mentions in the generic OAuth protocol
packages. Regenerate the VirtualMCPServer and MCPExternalAuthConfig CRD
YAML and the CRD API reference docs to match, fixing the CI doc-drift
failure. Also drop a stray "the" before "IdP exchange" for consistency
with the rest of the file.
@jhrozek jhrozek requested a review from amirejaz as a code owner July 2, 2026 08:48
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/XS Extra small PR: < 100 lines changed size/M Medium PR: 300-599 lines changed labels Jul 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants