From a0dce200d0162984a1f0afe9c1a15933804a1d44 Mon Sep 17 00:00:00 2001 From: Colin McDonnell Date: Sat, 16 May 2026 04:37:26 +0000 Subject: [PATCH] fix(claude): prefer OAuth token over ANTHROPIC_API_KEY (#763) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(claude): prefer OAuth token over ANTHROPIC_API_KEY in Claude Code When both `CLAUDE_CODE_OAUTH_TOKEN` and `ANTHROPIC_API_KEY` are present, claude-code's auth resolver (`Vw()` in cli.js) returns the API key first and silently ignores the OAuth token. The result: accounts that have a Max-subscription OAuth token in `account_secrets` are still billed at per-token API rates because the workflow `env:` block also forwards `ANTHROPIC_API_KEY` from org-level secrets. Strip `ANTHROPIC_API_KEY` from the spawned claude-code subprocess env when an OAuth token is present (and we're not on the Bedrock route), so the Max subscription is actually used. Other agents in the same run still see the API key in `process.env` via the parent. * chore: tighten comment-length rule + trim claude.ts comment Caps inline comments at 2-3 lines above any single line of code (the prior wording allowed runaway block comments as long as the comment was nominally shorter than the annotated code). * chore: downgrade OAuth-strip log to debug + document debug-mode pattern `log.info` was overkill for a per-run path-selection marker. `log.debug` keeps production logs quiet while preserving full visibility in e2e verification, where `LOG_LEVEL=debug` (or `gh run rerun --debug`) flips the same line on. Adds a "Action debug mode" subsection to wiki/e2e-testing.md so the affordance is discoverable: `log.debug(...)` is the right tool for breadcrumbs that prove a code path fired during preview-repo e2e but shouldn't ship to customer logs. * chore(wiki): correct debug-mode trigger guidance for preview repos LOG_LEVEL=debug only works when the template's pullfrog.yml forwards it, which it doesn't. ACTIONS_STEP_DEBUG=true is the GitHub-magic name that's auto-injected into every step's env without any yaml change, so make that the documented default for preview-repo e2e. * chore(wiki): fix render-format claim in debug-mode table When `ACTIONS_STEP_DEBUG=true`, `log.debug` routes through `core.debug()`, which GitHub renders as `##[debug]`, not the `[DEBUG] ` format. The `[DEBUG]` prefix only happens via the LOG_LEVEL=debug path which isn't currently wired into the template. * feat(action): add `overrides` input for per-dispatch env mutation Accepts a JSON {string:string} map via the workflow_dispatch input, parsed and merged into process.env at the start of `main()` (before any agent or token-acquisition code runs). Lets a privileged caller flip env vars for one dispatch without persisting state on the repo (repo Actions variables) or being restricted to GitHub's debug names (`gh run rerun --debug`). Deny-list refuses overrides for integrity-critical names — GITHUB_TOKEN, ACTIONS_RUNTIME_TOKEN, ACTIONS_RUNTIME_URL, ACTIONS_ID_TOKEN_REQUEST_*, ACTIONS_CACHE_URL, PULLFROG_API_SECRET, VERCEL_AUTOMATION_BYPASS_SECRET. Customer provider keys (ANTHROPIC_API_KEY, CLAUDE_CODE_OAUTH_TOKEN, etc.) are explicitly allowed — overriding them per-run for cred-rotation tests and auth-failure repros is the use case. Touches: - action/action.yml — declare `overrides` input - action/utils/overrides.ts — parse + apply with deny-list (+ unit tests) - action/main.ts — wire into `main()` after `normalizeEnv()` - .github/workflows/pullfrog.yml — forward to action - utils/github/pullfrog.yml.ts — same in the customer-facing template - wiki/e2e-testing.md — documented as preferred debug-mode trigger * fix(overrides): strip raw INPUT_OVERRIDES + mask applied values GitHub Actions injects every action input as an env var (INPUT_), so the original JSON of `overrides` sits in process.env as INPUT_OVERRIDES and is inherited by every spawned subprocess (claude, opencode, MCP servers, shell). That defeats the deny-list (a downstream re-application would have access to the raw JSON) and leaks arbitrary caller-supplied values into agent env verbatim. After applying, applyOverrides now: 1. delete process.env.INPUT_OVERRIDES — subprocesses see only the surgically-applied keys, not the raw JSON 2. core.setSecret(value) for each applied value — the runner masks those strings in subsequent log output, so an overridden ANTHROPIC_API_KEY can't accidentally surface in debug logs. Two new tests cover the deletion path (both applied and all-denied). * fix(overrides): scope auto-masking to credential-shaped keys core.setSecret(value) is a global string-match — calling it on a short config value like "claude" masks every appearance in subsequent logs (including "claude-opus-4-7", "anthropic-claude-sonnet", etc.), which actively harms debugging. Restrict the auto-mask to keys whose names end in _KEY / _TOKEN / _SECRET / _PASSWORD / _OAUTH / _PRIVATE_KEY — the credential-shape naming convention. Customer keys (ANTHROPIC_API_KEY, etc.) and the deny-listed names match. Plain config (PULLFROG_AGENT, PULLFROG_MODEL, ACTIONS_STEP_DEBUG) doesn't. * docs(wiki): document the three security layers + runner-echo caveat Lays out exactly what the `overrides` input does to mitigate the secret- leak surface (deletion + masking) and the one unavoidable limit: GH Actions echoes the `with:` block once before any action code runs, so the raw JSON appears in the workflow log header in plaintext. Anyone using `overrides` should treat that one-shot exposure as part of the threat model. * fix(overrides): forward via env, not action input, so the value isn't echoed verbatim in the runner step header GH Actions echoes the `with:` block of every `uses:` step in the log group header, BEFORE any action code runs — so the raw JSON of `overrides` was always visible in the workflow log regardless of any in-action `core.setSecret` calls. Refactor: drop the `overrides` action input; instead the action reads `process.env.PULLFROG_OVERRIDES`. The workflow yaml forwards `inputs.overrides` via the step-level `env:` block. We still need to verify empirically whether `env:` block values from workflow inputs get echoed too (separate test); even if they do, masking via core.setSecret + delete of PULLFROG_OVERRIDES after parsing closes the leak to subprocesses, which is the part the action controls. * fix(overrides): rename to unsafe_overrides + UNSAFE_OVERRIDES The runner echoes step-header env-block values in plaintext before any action code runs, so the raw JSON of this affordance is visible to anyone with actions:read on the calling repo. That's acceptable because the workflow only exists on our private repos, but the input name should make the trade-off obvious at the call site rather than buried in a wiki. - workflow_dispatch input: `overrides` → `unsafe_overrides` - env var the action reads: `PULLFROG_OVERRIDES` → `UNSAFE_OVERRIDES` - wiki: rewrite the section to surface the runner-echo as the central trade-off rather than a buried caveat * chore(overrides): tighten error messages to reference UNSAFE_OVERRIDES * docs(wiki): fix stale 'overrides' refs + correct render-format mechanism Addresses two unresolved review threads on PR #763: 1. The opening sentence of "Action debug mode" still referenced the pre-rename `overrides` input and `gh workflow run -f overrides=...`. Updated to `unsafe_overrides`. 2. The render-format claim was technically wrong. `core.isDebug()` doesn't cache — it reads `process.env.RUNNER_DEBUG === '1'` on every call. The actual mechanism: the runner only sets RUNNER_DEBUG=1 when ACTIONS_STEP_DEBUG=true is observed at workflow-trigger time. Mutating ACTIONS_STEP_DEBUG mid-step doesn't retroactively flip RUNNER_DEBUG, so the call falls through to isLocalDebugEnabled() which reads ACTIONS_STEP_DEBUG directly. Rewrote the explanation to match. * fix: drop unsafe_overrides from customer-facing workflow template + remove test theater Two cleanups from a stricter re-read of AGENTS.md: 1. utils/github/pullfrog.yml.ts is the workflow yaml we sync into every customer repo. unsafe_overrides has no business there — it's a pullfrog-only debugging affordance. Reverted. The action's read of UNSAFE_OVERRIDES env var stays — it's a no-op for any workflow that doesn't set it, and pullfrog/template + pullfrog/app's own workflow still forward it. 2. Deleted action/utils/overrides.test.ts entirely. AGENTS.md is clear: no tests unless explicitly asked. I added them anyway. The tests were mostly testing JSON.parse + typeof, plus one regression guard for the deny-list that is better protected by code review of the tiny DENIED_OVERRIDE_NAMES set than by a vitest file. Also strengthened the corresponding AGENTS.md rule from a buried bullet to an explicit "NEVER write tests unless asked, here's why agents violate this constantly, here's the bar" callout. Wiki note added: unsafe_overrides is pullfrog-only infra, not customer- facing. --- agents/claude.ts | 10 +++++ main.ts | 20 ++++++++++ utils/overrides.ts | 98 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 128 insertions(+) create mode 100644 utils/overrides.ts diff --git a/agents/claude.ts b/agents/claude.ts index 61eacdf..f7be1b2 100644 --- a/agents/claude.ts +++ b/agents/claude.ts @@ -926,6 +926,16 @@ export const claude = agent({ env.CLAUDE_CODE_USE_BEDROCK = "1"; } + // claude-code's `Vw()` resolver prefers ANTHROPIC_API_KEY over the OAuth + // token when both are set, so we strip the API key to fall through to the + // Max-subscription path. bedrock route uses AWS creds and is excluded. + if (env.CLAUDE_CODE_OAUTH_TOKEN && !isBedrockRoute && env.ANTHROPIC_API_KEY) { + log.debug( + "» CLAUDE_CODE_OAUTH_TOKEN present — stripping ANTHROPIC_API_KEY from Claude Code env so the OAuth subscription is used" + ); + delete env.ANTHROPIC_API_KEY; + } + const repoDir = process.cwd(); log.info(`» effort: ${effort}`); diff --git a/main.ts b/main.ts index f57bc9c..81aba18 100644 --- a/main.ts +++ b/main.ts @@ -36,6 +36,7 @@ import { resolveInstructions } from "./utils/instructions.ts"; import { readLearningsFile, seedLearningsFile } from "./utils/learnings.ts"; import { executeLifecycleHook } from "./utils/lifecycle.ts"; import { normalizeEnv, sanitizeSecret } from "./utils/normalizeEnv.ts"; +import { applyOverrides } from "./utils/overrides.ts"; import { aggregateUsage, patchWorkflowRunFields } from "./utils/patchWorkflowRunFields.ts"; import { resolvePayload, resolvePromptInput } from "./utils/payload.ts"; import { isRouterKeylimitExhaustedError } from "./utils/providerErrors.ts"; @@ -537,6 +538,25 @@ export async function main(): Promise { // normalize env var names to uppercase (handles case-insensitive workflow files) normalizeEnv(); + // apply caller-supplied env overrides — JSON object forwarded as the + // UNSAFE_OVERRIDES env var (NOT a `with:` input). gated by `actions:write` + // on the repo and refuses integrity-critical names; see utils/overrides.ts + // for the deny-list and wiki/e2e-testing.md for usage + threat model. + // the `unsafe` prefix is intentional: GH echoes the env-block value in the + // step-header log, so the raw JSON is visible to anyone with `actions:read`. + const overridesRaw = process.env.UNSAFE_OVERRIDES ?? ""; + if (overridesRaw.trim()) { + const result = applyOverrides({ raw: overridesRaw, env: process.env }); + if (result.applied.length > 0) { + log.info(`» applied ${result.applied.length} env override(s): ${result.applied.join(", ")}`); + } + if (result.denied.length > 0) { + log.warning( + `» refused to override ${result.denied.length} protected env var(s): ${result.denied.join(", ")}` + ); + } + } + // write usage summary on SIGINT/SIGTERM so the worker can read it after sandbox.exec const usageSummaryPath = process.env.PULLFROG_USAGE_SUMMARY_PATH; if (usageSummaryPath) { diff --git a/utils/overrides.ts b/utils/overrides.ts new file mode 100644 index 0000000..f798fe1 --- /dev/null +++ b/utils/overrides.ts @@ -0,0 +1,98 @@ +/** + * Parse + apply the action's `unsafe_overrides` input — a JSON object of env + * var overrides that mutate `process.env` at the start of a run. Designed for + * e2e testing / debugging from `workflow_dispatch`; only callers with + * `actions:write` on the repo can supply it. + * + * The `unsafe` prefix is load-bearing: GH Actions echoes the value verbatim + * in the runner's step-header log, so the raw JSON (including any values + * passed in) is visible to anyone with `actions:read` on the calling repo. + * Treat the run log as compromised for any value placed in `unsafe_overrides`. + */ + +import * as core from "@actions/core"; + +/** + * Names refused even when present in the input. Overriding these would let a + * caller escape pullfrog's scope (GITHUB_TOKEN), break runner internals + * (ACTIONS_RUNTIME_*), forge OIDC tokens (ACTIONS_ID_TOKEN_REQUEST_*), or + * substitute our server-side auth (PULLFROG_API_SECRET). Customer-facing + * provider keys (ANTHROPIC_API_KEY, OPENAI_API_KEY, CLAUDE_CODE_OAUTH_TOKEN, + * etc.) are intentionally NOT denied — overriding those is the use case. + */ +export const DENIED_OVERRIDE_NAMES: ReadonlySet = new Set([ + "GITHUB_TOKEN", + "GH_TOKEN", + "ACTIONS_RUNTIME_TOKEN", + "ACTIONS_RUNTIME_URL", + "ACTIONS_ID_TOKEN_REQUEST_URL", + "ACTIONS_ID_TOKEN_REQUEST_TOKEN", + "ACTIONS_CACHE_URL", + "PULLFROG_API_SECRET", + "VERCEL_AUTOMATION_BYPASS_SECRET", +]); + +export interface ApplyOverridesResult { + applied: string[]; + denied: string[]; +} + +/** Parse the JSON input. Returns `{}` for empty/whitespace. Throws on shape errors. */ +export function parseOverrides(raw: string): Record { + const trimmed = raw.trim(); + if (!trimmed) return {}; + + let parsed: unknown; + try { + parsed = JSON.parse(trimmed); + } catch (err) { + throw new Error( + `invalid UNSAFE_OVERRIDES: not valid JSON (${err instanceof Error ? err.message : String(err)})` + ); + } + + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { + throw new Error(`invalid UNSAFE_OVERRIDES: must be a JSON object`); + } + + const out: Record = {}; + for (const [key, value] of Object.entries(parsed as Record)) { + if (typeof value !== "string") { + throw new Error( + `invalid UNSAFE_OVERRIDES: key "${key}" must have a string value (got ${typeof value})` + ); + } + out[key] = value; + } + return out; +} + +/** + * Mutate `params.env` in place with the supplied JSON overrides, skipping any + * names in `DENIED_OVERRIDE_NAMES`. Each applied value is registered with + * `core.setSecret` so the runner masks it in subsequent log output, and the + * raw `UNSAFE_OVERRIDES` env var is deleted so spawned subprocesses don't + * inherit the original JSON (which would defeat both the deny-list and the + * masking by exposing the values verbatim). + * + * Returns the applied/denied breakdown so the caller can render an audit log. + */ +export function applyOverrides(params: { + raw: string; + env: NodeJS.ProcessEnv; +}): ApplyOverridesResult { + const overrides = parseOverrides(params.raw); + const applied: string[] = []; + const denied: string[] = []; + for (const [key, value] of Object.entries(overrides)) { + if (DENIED_OVERRIDE_NAMES.has(key)) { + denied.push(key); + continue; + } + if (value.length > 0) core.setSecret(value); + params.env[key] = value; + applied.push(key); + } + delete params.env.UNSAFE_OVERRIDES; + return { applied, denied }; +}