feat(mcp): agent-principal OAuth via client_credentials grant (2LO) (#324, standalone half)

[initializ-mk]

Implements the standalone, buildable-now half of #324 — the agent-principal MCP OAuth resolver via the OAuth 2.0 client_credentials grant (2LO).

Why

A headless deployed agent can't run forge mcp login (laptop-only, interactive), so an oauth MCP server fails startup with mcp: no stored token — login required. #317 solves the delegated (per-user) path, but most deployed agents are agent-principal — they act as themselves, with no requesting human — so #317's consent flow can never fire for them. This is the more common and more urgent case.

What

grant: client_credentials on the existing type: oauth block. The agent authenticates as itself with client_id + a secret named by client_secret_env; the token is minted at runtime — no user, no browser, no login — and re-minted on expiry.

auth:
  type: oauth
  grant: client_credentials        # 2-legged; agent acts as itself
  client_id: forge-agent
  client_secret_env: MCP_INTERNAL_SECRET   # NAME of an env var; never in yaml
  token_url: https://mcp.internal.corp/token
  scopes: [read]

• oauth.ClientCredentialsTokenCtx (RFC 6749 §4.4, client_secret_post).
• BearerToken branches on the grant: client_credentials mints on demand (no ErrNoToken), caches via the store, re-mints on expiry through the same singleflight. Cache-write failure is non-fatal (the token is re-mintable), so a read-only store degrades to per-use mint rather than a broken request.
• buildAuthFn resolves the secret from env at call time (rotation without a Manager restart, mirroring bearer/static).
• Validation: requires explicit client_id + client_secret_env + token_url (2LO has no authorize endpoint, no DCR); guarded so the 3LO partial-pair check doesn't misfire; unknown grant rejected. Enforced at parse + NewServer.
• forge mcp login on such a server prints "no login needed" and exits 0.
• required: true is valid here (token resolves at startup); the eager connection falls out of the existing manager-startup connect.
• Egress: token_url host auto-merged via MCPDomains (unchanged).

Acceptance (from #324)

• A headless agent with a 2LO-capable server resolves a token at startup, no browser, no forge mcp login.
• required: true gates readiness correctly (server reachable ⇒ ready).
• Writes still route through DEFER regardless of token validity (governance unchanged — this only supplies the token, not authorization).
• Shares the existing token-store (no divergent second store).
• Managed mode (service refresh token vaulted broker-side) — deferred, needs the Epic: runtime per-user, ephemeral per-session MCP OAuth (interactive login via Slack / A2A UI) #317 platform seam.

Scope note

Expressed as grant: client_credentials (the mechanism — §18.3's client_credentials resolver, Forge layer, standalone-capable). The type: user | platform principal axis and the managed platform-token-endpoint half of #324 depend on the #317 seam and are deferred; grant: is forward-compatible with that axis (it's the mechanism the future type: platform standalone resolver uses).

Tests

mint+cache (one token-endpoint call), invalid_client → ErrTokenRevoked, buildAuthFn resolves secret from env; validate cases (full ok / missing secret_env / unknown grant). All forge-core + forge-cli/cmd suites pass; lint clean.

Docs

configuration.md agent-principal section, cli-reference.md login note, forge.md.

[initializ-mk]

Review: agent-principal OAuth via client_credentials (2LO)

Well-built, and the security lessons from the MCP-OAuth series are cleanly applied. I verified the parts that matter:

• The #320 lesson holds — no secret persistence. doClientCredentials persists only the minted token (SaveCredentials(newTok)), never the client_secret; the secret comes from client_secret_env (a var name in yaml, value resolved at call time), documented explicitly. OAuthServerConfig.ClientSecret is used transiently for the grant call only.
• Fail-closed correctly: unknown grant rejected, validation requires client_id + client_secret_env + token_url at both parse and NewServer (belt-and-braces), and the 3LO partial-pair guard is now scoped under case "", "authorization_code" so it can't misfire on 2LO.
• Careful concurrency: the singleflight collapses concurrent mints, and the mint ctx is derived from context.Background (not the caller's ctx) — the B2 lesson preserved. Non-fatal cache write degrades a read-only store to per-use mint rather than a broken request.
• Governance boundary right: writes still route through DEFER — this supplies the token, not authorization. And forge mcp login on such a server prints "no login needed" and exits 0 (naming the secret env var — nice touch).
• BearerToken restructure is logically equivalent for all four cases (cached-valid, expired-3LO, expired-2LO, absent-2LO), and tok.RefreshToken is only dereferenced on the non-2LO path where tok is guaranteed non-nil. No NPE.

No blockers. Three findings, all minor/operability (two inline):

1. Minor (operability): an empty resolved secret surfaces as a misleading ErrTokenRevoked

Validation ensures client_secret_env (the name) is set, but nothing checks the named var actually resolves to a value at runtime. When the platform hasn't injected the secret (the most common headless-deploy misconfiguration), buildAuthFn sends client_secret="", the AS rejects with invalid_client, and it surfaces as ErrTokenRevoked — a confusing "revoked" diagnostic for what is really "the secret env var is empty." It fails closed (good), just unhelpfully. Recommend an empty-secret guard (see inline).

2. Minor: invalid_scope is misclassified as ErrTokenRevoked

In doClientCredentials, invalid_scope (a bad requested scope — a config error) is grouped with invalid_client/unauthorized_client into ErrTokenRevoked (client revoked). It's correctly non-transient so functionally fine, but the "revoked" label misleads. Inline.

3. Nit: grant-switch leaves a stale token under the shared store key

Switching a server between authorization_code and client_credentials leaves the old grant's token cached under the same storeKey(name), usable until expiry. Very narrow operational edge; a clear-on-grant-mismatch would be belt-and-braces. Optional.

Verdict

Merge-ready. The secret contract is correct (persist the token, never the secret; resolve from env at call time), validation is thorough and fail-closed, and concurrency/context handling is careful. Findings are all minor — finding 1 is the one worth folding in, since "secret not injected" is the failure a headless operator will actually hit and the current message points them the wrong way. CI green.

[initializ-mk]

Minor (operability): guard the empty resolved secret here for a clear diagnostic.

getenv(secretEnv) returns "" when the named var is unset/empty — which then goes to the token endpoint as client_secret="", gets rejected invalid_client, and surfaces as ErrTokenRevoked. Validation checked that client_secret_env (the name) is configured, but not that it resolves to a value at runtime — so the most common headless misconfiguration ("the platform didn't inject the secret") produces a misleading "revoked" error.

Suggest: if c.ClientSecret == "", return a clear error before the mint — e.g. client_secret_env %q resolved to an empty value; is the secret injected into the agent's environment?. Fails closed either way; this just points the operator at the real cause instead of the AS's generic rejection.

[initializ-mk]

Fixed in the latest commit. buildAuthFn now guards c.ClientSecret == "" and fails closed before minting with a message that names the env var — client_secret_env %q resolved to an empty value — is the secret injected into the agent's environment? — so the headless "secret not injected" case points at the real cause instead of the AS's generic invalid_client. Test asserts it errors and never hits the token endpoint.

[initializ-mk]

Minor: invalid_scope grouped with invalid_client/unauthorized_client into ErrTokenRevoked is semantically off — invalid_scope is a bad requested scope (a config error in scopes:), not a revoked/invalid client. It's correctly treated as non-transient (no retry), so functionally fine, but the ErrTokenRevoked label will mislead an operator debugging a scope typo. Consider a distinct config-error classification, or at least don't call it "revoked."

[initializ-mk]

Fixed — split invalid_scope out of the ErrTokenRevoked group into ErrProtocolError with invalid scope requested (check auth.scopes) and a distinct invalid_scope audit reason. invalid_client/unauthorized_client stay ErrTokenRevoked.

[initializ-mk]

Folded in both actionable findings (c18ffa9):

• Finding 1 (empty secret → clear diagnostic) — buildAuthFn now fails closed before minting when client_secret_env resolves to "", naming the var and pointing at "is the secret injected into the agent's environment?" — instead of letting the AS's invalid_client surface as a misleading ErrTokenRevoked. This is the failure a headless operator will actually hit, now with an actionable message. Test asserts it errors and never touches the token endpoint.
• Finding 2 (invalid_scope misclassified) — split out of the ErrTokenRevoked group into ErrProtocolError ("invalid scope requested (check auth.scopes)") with its own invalid_scope audit reason. invalid_client/unauthorized_client stay ErrTokenRevoked.
• Finding 3 (grant-switch stale token) — left as-is: it needs tagging the cached token with its grant to detect a mismatch, which is more than the very-narrow "operator flips a server's grant type" edge warrants. Acknowledged; happy to file a follow-up if you'd rather track it.

CI re-running.

[initializ-mk]

Re-review of c18ffa9 — both findings resolved, CI green

Finding 1 (empty-secret guard) — fixed at the ideal layer. The guard sits in buildAuthFn right after resolving getenv(secretEnv) and returns ErrProtocolError before calling flow.BearerToken — so it never loads the store or hits the token endpoint. The message names the env var and points at the real cause. TestBuildAuthFn_ClientCredentials_EmptySecret asserts both halves — the error contains "resolved to an empty value" AND hits.Load() == 0, which is the load-bearing assertion (proves the short-circuit, not just that some error occurs). The most common headless misconfig ("platform didn't inject the secret") now yields an actionable error instead of a misleading ErrTokenRevoked.

One design note (not a concern): placing the guard in buildAuthFn means it fires on every call, so an empty secret errors even with a valid cached token. For the target scenario there's no cache anyway; for the rarer "secret set, cached, later emptied" case it fails immediately rather than limping to expiry — the safe-forward direction, since you can't re-mint without the secret. Reasonable choice.

Finding 2 (invalid_scope) — fixed cleanly. Split out of the invalid_client/unauthorized_client → ErrTokenRevoked group into ErrProtocolError with "invalid scope requested (check auth.scopes)" and a distinct invalid_scope audit reason. Points at the config, not credentials.

Finding 3 (grant-switch stale token) — left as-is, acknowledged on the thread as the narrow edge it is. That was an optional nit; a deliberate decline-with-acknowledgment is the right disposition.

Verdict

Merge-ready. Both diagnostic fixes are correct and minimally scoped, and the finding-1 test pins the property that matters (fails closed without reaching the token endpoint). All 10 checks green. Combined with the already-solid core (token persisted but never the secret, thorough fail-closed validation, careful singleflight/context handling), nothing outstanding — a clean addition to the MCP-OAuth series.