refactor(acp): use credentialKey() for Claude OAuth token lookup#31901
Conversation
The daemon's ACP spawn was failing silently because claude-agent-acp requires CLAUDE_CODE_OAUTH_TOKEN to authenticate, but the token was never being passed in the spawned process's environment. Root cause: - The daemon process lacks CLAUDE_CODE_OAUTH_TOKEN in its env - The default ACP profile for claude had no env field to inject it - claude-agent-acp would exit immediately without registering Solution: - Fetch CLAUDE_CODE_OAUTH_TOKEN from the secure key store in spawnSession - Inject it into the agent config's env before spawning - Log a warning if the token is not found (but don't block spawn) This matches the pattern used by other provider auth flows in the codebase that use getSecureKeyAsync to fetch credentials at runtime.
- Replace raw 'acp/claude/oauth_token' string with credentialKey('acp', 'claude_oauth_token')
to align with the assistant credentials CLI pattern
- Update warning message to direct users to: assistant credentials set --service acp --field claude_oauth_token <token>
- Clarify in comments that this is the canonical provisioning method
- All ACP route tests pass
Adds a CLAUDE_CODE_OAUTH_TOKEN injection suite to acp-routes.test.ts that exercises the spawn handler's secure-key lookup path. The tests extend the existing acp/index.js mock with a spawn() that captures agentConfig, plus a secure-keys mock driven by a per-test store. Coverage: - happy path: token planted under credential/acp/claude_oauth_token ends up in agentConfig.env.CLAUDE_CODE_OAUTH_TOKEN for the claude agent - absent key: spawn proceeds without env injection (warn logged) - non-claude agent (codex): env injection is scoped to claude only - legacy key regression guard: token at the old 'acp/claude/oauth_token' path is NOT picked up, locking in the credentialKey() switch Also picks up an eslint --fix re-sort of the broadcastMessage import in acp-routes.ts (alphabetical order).
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5767c5468f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // pattern so `assistant credentials set --service acp --field claude_oauth_token <token>` | ||
| // is the canonical way to provision it. | ||
| const agentConfig = { ...resolved.agent }; | ||
| if (agent === "claude") { |
There was a problem hiding this comment.
The token lookup is gated by agent === "claude", but resolveAcpAgent() supports user-defined ACP agent IDs, so a custom ID that still points to claude-agent-acp (for example an alias in config.acp.agents) will skip injection and fail authentication. This should key off the resolved agent command/capability rather than a single ID string.
Useful? React with 👍 / 👎.
| agentConfig.env = { | ||
| ...agentConfig.env, | ||
| CLAUDE_CODE_OAUTH_TOKEN: claudeToken, |
There was a problem hiding this comment.
When a secure-store token exists, this assignment always overwrites any CLAUDE_CODE_OAUTH_TOKEN already set in acp.agents.<id>.env. That can break existing setups that intentionally provide a specific token via agent config (for example per-workspace or rotated credentials) by silently replacing it with a stale/different vault value. Inject the vault token only if the env var is currently unset, or explicitly enforce/validate precedence.
Useful? React with 👍 / 👎.
The original injection commit (2cf8e89) added `getSecureKeyAsync` to `runtime/routes/acp-routes.ts` without updating the Invariant 2 allowlist in `credential-security-invariants.test.ts`. The acp-routes route has a legitimate need: it injects CLAUDE_CODE_OAUTH_TOKEN into the claude-agent-acp subprocess env. Add it explicitly so the secure-keys import boundary continues to require per-importer review. Fixes red CI on PR #31901.
…dence
Three changes to claude-agent-acp env handling on the spawn route:
1. Fail fast on missing CLAUDE_CODE_OAUTH_TOKEN. Previously the route
would log a warning and let the spawn proceed; the agent would then
crash on auth, leaving callers debugging a defunct zombie subprocess.
Now we throw FailedDependencyError up front, symmetric with the
existing `binary_not_found` preflight, with the CLI hint in the
message so the fix is one shell line away.
2. Key env injection on the resolved command basename, not the
user-facing agent id. A custom alias like
`acp.agents.my-claude = { command: "claude-agent-acp", ... }`
needs the same env as the default "claude" id. Addresses Codex P2
feedback on PR #31901.
3. Treat config.json env as the override, vault as the fallback. The
previous order (`{ ...agentConfig.env, CLAUDE_CODE_OAUTH_TOKEN:
vaultValue }`) silently clobbered explicit per-workspace / rotated
tokens set under `acp.agents.<id>.env`. The vault lookup is now
gated on the config-env value being absent, which also avoids a
secure-store call (and log line) on every spawn for users who
provide the token via config. Addresses Codex P2 feedback on PR
#31901.
Tests:
- Existing happy-path and legacy-key regression tests pinned.
- NEW: config.json env override accepted without secure-store entry.
- NEW: config.json env override wins over vault token (precedence).
- NEW: command-based injection fires for user-aliased agent ids.
- NEW: throws FailedDependencyError when no token is available
from any source.
- 'non-claude agents unaffected' test updated to reflect command-
basename gating semantics.
|
Both Codex P2 findings addressed in #1 — command-based gating. Injection + preflight now key off #2 — env precedence. Inverted: vault token is only consulted when Also fixed in the same push: the original commit Verification: 20/20 acp-routes tests, 32/32 invariants, lint/prettier/typecheck clean. Full details + the codex-acp follow-up reasoning are in the updated PR description. @codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Summary
Follow-up to
2cf8e89a1(which shippedCLAUDE_CODE_OAUTH_TOKENinjection for ACPclaude-agent-acpspawns). The original commit had three gaps which this PR closes:"acp/claude/oauth_token"instead of routing throughcredentialKey(), soassistant credentials set --service acp --field claude_oauth_tokenhad no effect.secure-keysimport allowlist. Adding the import without updatingInvariant 2left this branch's CI red since open. The allowlist exists to force per-importer review of the secret-leak surface.This PR fixes all three plus the two Codex P2 findings on the first push.
Provisioning contract
CLAUDE_CODE_OAUTH_TOKENcan come from either of two routes. Config wins over vault so explicit per-workspace / rotated overrides are never silently clobbered:acp.agents.<id>.env.CLAUDE_CODE_OAUTH_TOKENinconfig.json(first priority — explicit user override).If neither produces a token, the spawn route throws
FailedDependencyErrorwith the CLI hint — symmetric with the existingbinary_not_foundpreflight.Changes
assistant/src/runtime/routes/acp-routes.tscredentialKey("acp", "claude_oauth_token")(resolves tocredential/acp/claude_oauth_token).basename(agentConfig.command) === "claude-agent-acp", not on the user-facing agent id, so user-aliased agent ids (acp.agents.my-claude = { command: "claude-agent-acp", ... }) still get the env they need. (Codex P2 feat: initialize Next.js app in /web directory #1)agentConfig.env.CLAUDE_CODE_OAUTH_TOKENis unset, so explicit config-json values are preserved. (Codex P2 feat: add platform terraform for GKE deployment #2)FailedDependencyErrorwhen no token is available from any source; error message includes the CLI hint plus the config-json route.assistant/src/runtime/routes/acp-routes.test.ts— 7 tests in the new env-injection suite (was 4 in the first push):agentConfig.env.claude-agent-acpstill triggers injection.FailedDependencyErrorwhen no token is available from any source.acp/claude/oauth_tokenpath is NOT picked up and the preflight throws.assistant/src/__tests__/credential-security-invariants.test.tsruntime/routes/acp-routes.tsto the secure-keys importer allowlist with a comment explaining the legitimate need (subprocess env injection).Verification
bun test src/runtime/routes/acp-routes.test.ts→ 20/20 passbun test src/__tests__/credential-security-invariants.test.ts→ 32/32 passbun test src/acp/→ 31/31 pass (no regression)bun run typecheck→ cleanbun run lint→ cleanbun x prettier --check→ cleanFollow-up: codex-acp parity (parked)
A parallel preflight for
codex-acpis the obvious next step, butcodex-acphas a more complex auth surface thanclaude-agent-acpand warrants its own scoped PR:CODEX_API_KEYenv var, file-based OAuth (browser login stored at~/.codex/...), ChatGPT subscription OAuth. A naive "fail if noCODEX_API_KEY" would break users on the OAuth paths.codex-acp; we'd need a newcredential/acp/codex_api_keyprovisioning story.credential/openai-codex/access_token(with refresh logic) for the OpenAI Codex inference provider. Whethercodex-acpaccepts bearer tokens or strictly needs ansk-...API key is unclear without reading the@zed-industries/codex-acpsource.The pattern this PR establishes (
credentialKey()lookup + command-based gating + fail-fast preflight) is whatcodex-acpshould follow once those questions are answered.Other follow-ups parked
nodeshim. Fresh containers fail to spawnclaude-agent-acp(shebang#!/usr/bin/env node, container only hasbun). Workaround in place this sandbox via/usr/local/bin/node → /usr/local/bin/bunsymlink; won't survive restart. Wants a Dockerfile/entrypoint hook.cc-sessionreads/workspace/config/.cc-oauth-token; ACP now reads the secure store. Two sources of truth.