Make DCR consume helpers value-in / value-out

Addresses #5198 review comments:
- INFO pkg/auth/dcr/resolver.go (3227004701): ConsumeResolution and
  ApplyResolutionToOAuth2Config mutated caller-supplied pointers and
  documented the "pass a copy" invariant only in prose. Converted
  both to value-in / value-out signatures so the no-caller-mutation
  contract is compile-time enforced rather than a prose discipline
  the second consumer (CLI flow in sub-issue 4b, #5219) would have
  to remember.

The change is structural but small: two signature changes
(ConsumeResolution / ApplyResolutionToOAuth2Config), two function
bodies returning the modified value, and call-site updates in
embeddedauthserver.go (1 production call site per function) and
resolver_test.go (4 test call sites for ConsumeResolution; zero for
ApplyResolutionToOAuth2Config).

Doc comments updated to describe the new value-in / value-out shape;
the "pass a copy of the run-config" prose-discipline paragraph is
removed because the type system now enforces it. Pointer-typed fields
inside the struct (DCRConfig) still share storage with the caller
via the shallow copy, but the only mutation here is nil-assignment to
the copy's DCRConfig field, which does not reach back through the
caller's pointer; this nuance is now called out in the doc comment.

ResolveCredentials is left as *authserver.OAuth2UpstreamRunConfig
deliberately: its rc input is read-only (no mutation), so the
pointer is just for avoiding a struct copy on the hot path.

Satisfies .claude/rules/go-style.md "Copy Before Mutating Caller
Input".

Author: tgrunnagle

pkg/auth/dcr/resolver.go

@@ -229,23 +229,26 @@ func NeedsDCR(rc *authserver.OAuth2UpstreamRunConfig) bool {
 	return rc.ClientID == "" && rc.DCRConfig != nil
 }
 
-// ConsumeResolution copies resolved credentials and endpoints from res into
-// rc and consumes rc.DCRConfig (sets it to nil), transitioning the run-
-// config copy from "DCR-pending" (ClientID == "" && DCRConfig != nil) to
-// "DCR-resolved" (ClientID populated && DCRConfig == nil). The "consume"
-// name is deliberate: a second call after the first is a no-op only
-// because the first cleared DCRConfig — this is a one-shot state
-// transition, not an idempotent default-fill.
+// ConsumeResolution returns a copy of rc with the resolved credentials and
+// endpoints from res copied in and DCRConfig consumed (set to nil),
+// transitioning the run-config from "DCR-pending" (ClientID == "" &&
+// DCRConfig != nil) to "DCR-resolved" (ClientID populated && DCRConfig
+// == nil). The "consume" name is deliberate: a second call on the
+// returned value is a no-op only because the first cleared DCRConfig —
+// this is a one-shot state transition, not an idempotent default-fill.
 //
-// Callers must pass a COPY of the upstream run-config so the caller's
-// original is unaffected; ConsumeResolution does not clone rc internally.
+// rc is taken by value and the modified copy is returned. The caller's
+// original is never observably mutated; the value-in / value-out shape
+// makes the no-mutation contract compile-time enforced rather than a
+// prose discipline the caller is required to remember. Pointer-typed
+// fields (DCRConfig) share storage with the caller's copy via the struct
+// shallow-copy, but the only mutation here is to assign nil to the
+// copy's DCRConfig — nil-assignment to the local field does not reach
+// back through the original pointer.
 //
 // Why DCRConfig is cleared: OAuth2UpstreamRunConfig.Validate enforces
 // ClientID xor DCRConfig — a resolved copy that left DCRConfig set would
-// fail the validator that runs downstream in buildPureOAuth2Config. The
-// caller's *original* OAuth2Config is unaffected because
-// buildUpstreamConfigs deep-copies before resolution; only the post-
-// resolution copy is mutated here.
+// fail the validator that runs downstream in buildPureOAuth2Config.
 //
 // ClientID, the endpoints, and RedirectURI are written only when rc leaves
 // them empty — explicit caller configuration always wins. The conditional
@@ -261,15 +264,15 @@ func NeedsDCR(rc *authserver.OAuth2UpstreamRunConfig) bool {
 // RedirectURI, which authserver.Config validation requires.
 //
 // Note on ClientSecret: ConsumeResolution does NOT write the resolved
-// secret to rc because OAuth2UpstreamRunConfig models secrets as file-or-
-// env references only. To propagate the DCR-resolved secret into the
-// final upstream.OAuth2Config, callers must pair this call with
+// secret because OAuth2UpstreamRunConfig models secrets as file-or-env
+// references only. To propagate the DCR-resolved secret into the final
+// upstream.OAuth2Config, callers must pair this call with
 // ApplyResolutionToOAuth2Config once the config has been built. Keeping
 // the two helpers side-by-side localises the DCR-specific application
 // logic.
-func ConsumeResolution(rc *authserver.OAuth2UpstreamRunConfig, res *Resolution) {
-	if rc == nil || res == nil {
-		return
+func ConsumeResolution(rc authserver.OAuth2UpstreamRunConfig, res *Resolution) authserver.OAuth2UpstreamRunConfig {
+	if res == nil {
+		return rc
 	}
 	if rc.ClientID == "" {
 		rc.ClientID = res.ClientID
@@ -284,28 +287,35 @@ func ConsumeResolution(rc *authserver.OAuth2UpstreamRunConfig, res *Resolution)
 	if rc.RedirectURI == "" {
 		rc.RedirectURI = res.RedirectURI
 	}
+	return rc
 }
 
-// ApplyResolutionToOAuth2Config overlays the DCR-resolved ClientSecret onto
-// a built *upstream.OAuth2Config. This is the companion to
+// ApplyResolutionToOAuth2Config returns a copy of cfg with the DCR-
+// resolved ClientSecret overlaid onto it. This is the companion to
 // ConsumeResolution: where that function writes fields representable in
 // the file-or-env run-config model, this one writes the inline-only
 // ClientSecret directly on the runtime config.
 //
-// The split exists because buildPureOAuth2Config intentionally retains a
-// narrow file-or-env contract (no DCR awareness) and because OAuth2's
-// ClientSecret on the run-config is modelled as a reference rather than
-// an inline string. Any future output path from OAuth2UpstreamRunConfig
-// to upstream.OAuth2Config must call BOTH ConsumeResolution (run-config
-// side) AND ApplyResolutionToOAuth2Config (built-config side) to get a
-// fully-resolved DCR client. Forgetting the second call leaves
-// ClientSecret empty and produces silent auth failures at request time —
-// the type system does not enforce the pair, so the invariant lives here.
-func ApplyResolutionToOAuth2Config(cfg *upstream.OAuth2Config, res *Resolution) {
-	if cfg == nil || res == nil {
-		return
+// cfg is taken by value and the modified copy is returned, mirroring
+// ConsumeResolution. The no-mutation contract is compile-time enforced
+// by the signature rather than a prose discipline.
+//
+// The split between these two helpers exists because buildPureOAuth2Config
+// intentionally retains a narrow file-or-env contract (no DCR awareness)
+// and because OAuth2's ClientSecret on the run-config is modelled as a
+// reference rather than an inline string. Any future output path from
+// OAuth2UpstreamRunConfig to upstream.OAuth2Config must call BOTH
+// ConsumeResolution (run-config side) AND ApplyResolutionToOAuth2Config
+// (built-config side) to get a fully-resolved DCR client. Forgetting the
+// second call leaves ClientSecret empty and produces silent auth failures
+// at request time — the type system does not enforce the pair, so the
+// invariant lives here.
+func ApplyResolutionToOAuth2Config(cfg upstream.OAuth2Config, res *Resolution) upstream.OAuth2Config {
+	if res == nil {
+		return cfg
 	}
 	cfg.ClientSecret = res.ClientSecret
+	return cfg
 }
 
 // Step identifiers for structured error logs emitted by the caller of

pkg/auth/dcr/resolver_test.go

@@ -635,7 +635,7 @@ func TestNeedsDCR(t *testing.T) {
 func TestConsumeResolution_RespectsExplicitEndpoints(t *testing.T) {
 	t.Parallel()
 
-	rc := &authserver.OAuth2UpstreamRunConfig{
+	rc := authserver.OAuth2UpstreamRunConfig{
 		AuthorizationEndpoint: "https://explicit/authorize",
 		TokenEndpoint:         "https://explicit/token",
 	}
@@ -644,7 +644,7 @@ func TestConsumeResolution_RespectsExplicitEndpoints(t *testing.T) {
 		AuthorizationEndpoint: "https://discovered/authorize",
 		TokenEndpoint:         "https://discovered/token",
 	}
-	ConsumeResolution(rc, res)
+	rc = ConsumeResolution(rc, res)
 	assert.Equal(t, "got-client", rc.ClientID)
 	assert.Equal(t, "https://explicit/authorize", rc.AuthorizationEndpoint)
 	assert.Equal(t, "https://explicit/token", rc.TokenEndpoint)
@@ -653,13 +653,13 @@ func TestConsumeResolution_RespectsExplicitEndpoints(t *testing.T) {
 func TestConsumeResolution_FillsMissingEndpoints(t *testing.T) {
 	t.Parallel()
 
-	rc := &authserver.OAuth2UpstreamRunConfig{}
+	rc := authserver.OAuth2UpstreamRunConfig{}
 	res := &Resolution{
 		ClientID:              "got-client",
 		AuthorizationEndpoint: "https://discovered/authorize",
 		TokenEndpoint:         "https://discovered/token",
 	}
-	ConsumeResolution(rc, res)
+	rc = ConsumeResolution(rc, res)
 	assert.Equal(t, "got-client", rc.ClientID)
 	assert.Equal(t, "https://discovered/authorize", rc.AuthorizationEndpoint)
 	assert.Equal(t, "https://discovered/token", rc.TokenEndpoint)
@@ -673,7 +673,7 @@ func TestConsumeResolution_FillsMissingEndpoints(t *testing.T) {
 func TestConsumeResolution_ClearsDCRConfig(t *testing.T) {
 	t.Parallel()
 
-	rc := &authserver.OAuth2UpstreamRunConfig{
+	rc := authserver.OAuth2UpstreamRunConfig{
 		DCRConfig: &authserver.DCRUpstreamConfig{
 			RegistrationEndpoint: "https://idp.example.com/register",
 		},
@@ -682,7 +682,7 @@ func TestConsumeResolution_ClearsDCRConfig(t *testing.T) {
 		ClientID: "dcr-issued-client",
 	}
 
-	ConsumeResolution(rc, res)
+	rc = ConsumeResolution(rc, res)
 
 	assert.Equal(t, "dcr-issued-client", rc.ClientID)
 	assert.Nil(t, rc.DCRConfig,
@@ -1205,13 +1205,13 @@ func TestResolveUpstreamRedirectURI_PreservesIssuerPath(t *testing.T) {
 func TestConsumeResolution_DoesNotOverwritePreProvisionedClientID(t *testing.T) {
 	t.Parallel()
 
-	rc := &authserver.OAuth2UpstreamRunConfig{
+	rc := authserver.OAuth2UpstreamRunConfig{
 		ClientID: "pre-provisioned",
 	}
 	res := &Resolution{
 		ClientID: "would-be-overwrite",
 	}
-	ConsumeResolution(rc, res)
+	rc = ConsumeResolution(rc, res)
 	assert.Equal(t, "pre-provisioned", rc.ClientID,
 		"ConsumeResolution must not overwrite a non-empty ClientID")
 }

pkg/authserver/runner/embeddedauthserver.go

@@ -385,20 +385,22 @@ func buildUpstreamConfigs(
 		// "does this upstream require DCR" avoids drift if the condition
 		// ever needs to be extended (e.g., to support OIDC DCR).
 		if dcr.NeedsDCR(rcCopy.OAuth2Config) {
-			// Deep-copy the OAuth2 sub-config so dcr.ConsumeResolution writes to the
-			// copy, not the caller's OAuth2UpstreamRunConfig pointer.
-			o2Copy := *rcCopy.OAuth2Config
-			rcCopy.OAuth2Config = &o2Copy
+			// Take a local copy of the OAuth2 sub-config. dcr.ResolveCredentials
+			// reads it but does not mutate; dcr.ConsumeResolution is value-in /
+			// value-out, so the caller's original OAuth2Config pointer target
+			// is never reached by either call.
+			o2 := *rcCopy.OAuth2Config
 
-			resolution, err := dcr.ResolveCredentials(ctx, &o2Copy, issuer, dcrStore)
+			resolution, err := dcr.ResolveCredentials(ctx, &o2, issuer, dcrStore)
 			if err != nil {
 				// Emit the single boundary Error record with enough context to
 				// correlate the failure back to this upstream; then return the
 				// wrapped error without further logging.
 				dcr.LogStepError(rc.Name, err)
 				return nil, fmt.Errorf("upstream %q: %w", rc.Name, err)
 			}
-			dcr.ConsumeResolution(&o2Copy, resolution)
+			o2 = dcr.ConsumeResolution(o2, resolution)
+			rcCopy.OAuth2Config = &o2
 			dcrResolution = resolution
 		}
 
@@ -413,7 +415,8 @@ func buildUpstreamConfigs(
 		// documented in pkg/auth/dcr/resolver.go — both calls must be
 		// paired to produce a fully-resolved DCR client.
 		if dcrResolution != nil && cfg.OAuth2Config != nil {
-			dcr.ApplyResolutionToOAuth2Config(cfg.OAuth2Config, dcrResolution)
+			applied := dcr.ApplyResolutionToOAuth2Config(*cfg.OAuth2Config, dcrResolution)
+			cfg.OAuth2Config = &applied
 		}
 
 		configs = append(configs, *cfg)