diff --git a/README.md b/README.md index bd12ecdb..ed72a5b1 100644 --- a/README.md +++ b/README.md @@ -668,12 +668,22 @@ command waits as long as the host stays busy. branch before it is pushed for review: Codex when run from Claude Code, and Claude Code when run from Codex. `--harness` names the reviewer instead; an unknown harness, or a missing or logged-out CLI, is an error rather than a -same-model review. The reviewer uses its CLI's own login, with a +same-model review. The reviewer keeps its CLI's saved login or environment +credentials and provider endpoint, with a pinned model and effort: a default tier for routine work and, with `--deep`, a stronger one for a high-stakes change such as security, a wire format or public API, or a risk of data loss. `quest review --help` lists them; `--model` and `--effort` override either. +For Claude Code, export `ANTHROPIC_BASE_URL` and `ANTHROPIC_AUTH_TOKEN` to use +an LLM gateway such as CLIProxyAPI. `ANTHROPIC_API_KEY` and +`CLAUDE_CODE_OAUTH_TOKEN` also work. Provider-specific connection variables +for Bedrock, Vertex, Foundry, and Claude Platform on AWS are kept too. For +Codex, `CODEX_API_KEY` works without a saved login. Only the selected reviewer's +connection variables are restored; parent-session state, other provider keys, +and plugin or permission overrides remain stripped. This does not change +`quest agent`'s separate-account isolation. + ```sh quest review # round 1 against origin's default branch quest review --responses .tmp/responses.md # round 2: the new head, with fixes and refutations diff --git a/quest/m0/review-login.md b/quest/m0/review-login.md index 9dc82e7e..547f04e5 100644 --- a/quest/m0/review-login.md +++ b/quest/m0/review-login.md @@ -1,11 +1,10 @@ -# [S] Review handles expired and environment logins +# [S] Review handles expired logins ## Goal When a reviewer CLI's login has expired or been revoked, `quest review` fails with the same "`` is not logged in; run ``" error it gives a -signed-out CLI, instead of a raw 401 from the reviewer's turn. A reviewer -whose only credentials come from the environment still reviews. +signed-out CLI, instead of a raw 401 from the reviewer's turn. ## Plan @@ -21,20 +20,3 @@ Decided 2026-10-29 (user, /quest-plan of the follow-ups from - Keep the `status` check for the signed-out case. - Cover it with the fake-CLI tests: one reviewer that exits with each CLI's auth error text. - -Decided 2026-10-30 (user, /quest-plan of the follow-ups from a moq-dev/moq -`/quest-complete` session): ✅ extend this quest / drop. - -- `quest review --harness claude` reported "claude is not logged in" in - sessions where Claude Code's credentials came only from `ANTHROPIC_*` or - `CLAUDE*` variables. `strippedEnvironment` (`src/cli/agent.ts`, used at - `src/cli/review.ts`'s status check) removes every one except - `CLAUDE_CONFIG_DIR`, so the status check fails. Codex-written PRs then got - no Claude review (moq-dev/moq#7084, moq-dev/moq#7251). -- Let a reviewer keep the credential its own CLI would read from the - environment, while still stripping variables that would redirect billing or - configuration. Weigh that against why `STRIPPED` exists before choosing - which names pass. If a credential is stripped on purpose, the error names the - variable instead of saying "not logged in". -- Test with a fake reviewer that authenticates only through an environment - variable. diff --git a/src/cli/main.ts b/src/cli/main.ts index c51b1ef8..80d4459a 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -777,7 +777,8 @@ export function main(argv: string[], out: Output): number | Promise { .description( "Review the checkout's committed branch against its base with the other harness: Codex when run from " + "Claude Code, Claude Code when run from Codex, detected from the variables each sets. --harness names " + - "the reviewer instead; there is no other fallback. The reviewer uses its CLI's own login, " + + "the reviewer instead; there is no other fallback. The reviewer keeps its CLI's own login or " + + "environment credentials and provider endpoint, " + `with ${models("default")}. --deep, for a high-stakes change, picks ${models("deep")}. ` + "--model and --effort override either.\n\n" + "The first round runs the reviewer's own review (`codex exec review --base`, or Claude Code's " + diff --git a/src/cli/review.ts b/src/cli/review.ts index 2b08d9fa..f9306585 100644 --- a/src/cli/review.ts +++ b/src/cli/review.ts @@ -108,6 +108,20 @@ export function runningHarness(env: NodeJS.ProcessEnv): HarnessName { const other = (harness: HarnessName): HarnessName => (harness === "claude" ? "codex" : "claude"); +// Reviewers use the user's chosen connection, rather than a key selected by +// `quest agent`. Keep the allowlist narrow: plugin, permission, model, effort, +// and parent-session variables must not change the isolated review. +const CONNECTION: Record = { + claude: [ + /^ANTHROPIC_(API_KEY|AUTH_TOKEN|BASE_URL|CUSTOM_HEADERS)$/, + /^ANTHROPIC_(AWS|BEDROCK|BEDROCK_MANTLE|FOUNDRY|VERTEX)_(API_KEY|BASE_URL|RESOURCE|WORKSPACE_ID)$/, + /^CLAUDE_CODE_OAUTH_TOKEN$/, + /^CLAUDE_CODE_USE_(BEDROCK|VERTEX|FOUNDRY|ANTHROPIC_AWS)$/, + /^CLAUDE_CODE_SKIP_(BEDROCK|VERTEX|FOUNDRY)_AUTH$/, + ], + codex: [/^CODEX_(API_KEY|ACCESS_TOKEN|CA_CERTIFICATE)$/], +}; + export interface ReviewOptions { /** The checkout to review. */ root: string; @@ -651,8 +665,15 @@ export async function review(options: ReviewOptions, deps: ReviewDependencies): } const env = strippedEnvironment(deps.env); - const status = await run(reviewer.status, root, env, ""); - if (status.code !== 0) throw new ReviewInputError(`${harness} is not logged in; run \`${reviewer.login}\``); + for (const [name, value] of Object.entries(deps.env)) { + if (CONNECTION[harness].some((pattern) => pattern.test(name))) env[name] = value; + } + // Codex accepts CODEX_API_KEY in exec/review, but login status checks only + // its stored login. The review itself validates an explicitly supplied key. + if (harness !== "codex" || !env.CODEX_API_KEY?.trim()) { + const status = await run(reviewer.status, root, env, ""); + if (status.code !== 0) throw new ReviewInputError(`${harness} is not logged in; run \`${reviewer.login}\``); + } // The reviewer's sandbox has no network, so the base is fetched first. const baseRef = options.base ?? `origin/${defaultBranch(root, "origin")}`; diff --git a/test/review.test.ts b/test/review.test.ts index 28ce7fda..fb9be895 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -26,8 +26,9 @@ const FAKES: Record = { const fs = require("node:fs"); const argv = process.argv.slice(2); const stdin = fs.readFileSync(0, "utf8"); -const env = Object.fromEntries(Object.entries(process.env).filter(([k]) => /^(CLAUDE|CODEX|FAKE_HOST)/.test(k))); +const env = Object.fromEntries(Object.entries(process.env).filter(([k]) => /^(CLAUDE|ANTHROPIC|OPENAI|CODEX|QUEST|OP_)/.test(k))); fs.appendFileSync(process.env.FAKE_LOG, JSON.stringify({ argv, stdin, env, cwd: process.cwd() }) + "\\n"); +if (process.env.FAKE_AUTH_VARIABLE && !process.env[process.env.FAKE_AUTH_VARIABLE]) process.exit(1); if (argv[0] === "auth") process.exit(process.env.FAKE_LOGGED_OUT ? 1 : 0); const at = (flag) => argv[argv.indexOf(flag) + 1]; const result = (fields) => console.log(JSON.stringify({ type: "result", is_error: false, ...fields })); @@ -57,8 +58,11 @@ if (argv.includes("--session-id")) { const fs = require("node:fs"); const argv = process.argv.slice(2); const stdin = fs.readFileSync(0, "utf8"); -const env = Object.fromEntries(Object.entries(process.env).filter(([k]) => /^(CLAUDE|CODEX|FAKE_HOST)/.test(k))); +const env = Object.fromEntries(Object.entries(process.env).filter(([k]) => /^(CLAUDE|ANTHROPIC|OPENAI|CODEX|QUEST|OP_)/.test(k))); fs.appendFileSync(process.env.FAKE_LOG, JSON.stringify({ argv, stdin, env, cwd: process.cwd() }) + "\\n"); +// CODEX_API_KEY is accepted by exec/review, not by login status. +if (argv[0] === "login" && process.env.FAKE_AUTH_VARIABLE === "CODEX_API_KEY") process.exit(1); +if (process.env.FAKE_AUTH_VARIABLE && !process.env[process.env.FAKE_AUTH_VARIABLE]) process.exit(1); if (argv[0] === "login") process.exit(process.env.FAKE_LOGGED_OUT ? 1 : 0); const event = (value) => console.log(JSON.stringify(value)); const message = (text) => event({ type: "item.completed", item: { type: "agent_message", text } }); @@ -312,6 +316,90 @@ fakes("claude reviews read-only with the base's instructions, then reports in th } }); +fakes.each(["ANTHROPIC_API_KEY", "ANTHROPIC_AUTH_TOKEN", "CLAUDE_CODE_OAUTH_TOKEN"])( + "Claude keeps %s and gateway routing through the status check and resumed rounds", + async (credential) => { + const s = setup("CODEX_THREAD_ID"); + vi.stubEnv(credential, "review-credential"); + vi.stubEnv("ANTHROPIC_BASE_URL", "http://127.0.0.1:8317"); + vi.stubEnv("ANTHROPIC_CUSTOM_HEADERS", "X-Gateway-Tenant: review"); + vi.stubEnv("CLAUDE_CODE_USE_BEDROCK", ""); + vi.stubEnv("CLAUDE_CODE_PLUGIN_DIRS", "/untrusted/plugins"); + vi.stubEnv("CLAUDE_CODE_EFFORT_LEVEL", "max"); + vi.stubEnv("CODEX_API_KEY", "other-provider-key"); + vi.stubEnv("OPENAI_API_KEY", "other-provider-key"); + vi.stubEnv("QUEST_API_KEYS", "other-agent-key"); + vi.stubEnv("OP_SERVICE_ACCOUNT_TOKEN", "secret-manager-token"); + vi.stubEnv("FAKE_AUTH_VARIABLE", credential); + vi.stubEnv("FAKE_REPLY", JSON.stringify({ findings: [] })); + + const first = await quest("--root", s.repo, "review", "--json"); + expect(first.code, first.stderr).toBe(0); + const resumed = await quest("--root", s.repo, "review", "--json"); + expect(resumed.code, resumed.stderr).toBe(0); + expect(JSON.parse(resumed.stdout)).toMatchObject({ round: 2, session: JSON.parse(first.stdout).session }); + for (const call of s.calls()) { + expect(call.env).toMatchObject({ + [credential]: "review-credential", + ANTHROPIC_BASE_URL: "http://127.0.0.1:8317", + ANTHROPIC_CUSTOM_HEADERS: "X-Gateway-Tenant: review", + CLAUDE_CODE_USE_BEDROCK: "", + }); + for (const name of [ + "CODEX_THREAD_ID", + "CODEX_API_KEY", + "OPENAI_API_KEY", + "QUEST_API_KEYS", + "OP_SERVICE_ACCOUNT_TOKEN", + "CLAUDE_CODE_PLUGIN_DIRS", + "CLAUDE_CODE_EFFORT_LEVEL", + ]) { + expect(call.env[name], name).toBeUndefined(); + } + } + }, +); + +fakes("Codex reviews with an exec-only environment key without requiring a stored login", async () => { + const s = setup(); + vi.stubEnv("CODEX_API_KEY", "review-key"); + vi.stubEnv("ANTHROPIC_AUTH_TOKEN", "other-provider-token"); + vi.stubEnv("FAKE_AUTH_VARIABLE", "CODEX_API_KEY"); + vi.stubEnv("FAKE_REPLY", JSON.stringify({ findings: [] })); + const first = await quest("--root", s.repo, "review", "--json"); + expect(first.code, first.stderr).toBe(0); + const resumed = await quest("--root", s.repo, "review", "--json"); + expect(resumed.code, resumed.stderr).toBe(0); + expect(JSON.parse(resumed.stdout).round).toBe(2); + for (const call of s.calls()) { + expect(call.argv[0]).toBe("exec"); + expect(call.env.CODEX_API_KEY).toBe("review-key"); + expect(call.env.CLAUDECODE).toBeUndefined(); + expect(call.env.ANTHROPIC_AUTH_TOKEN).toBeUndefined(); + } +}); + +fakes.each([ + ["BEDROCK", "ANTHROPIC_BEDROCK_BASE_URL"], + ["VERTEX", "ANTHROPIC_VERTEX_BASE_URL"], + ["FOUNDRY", "ANTHROPIC_FOUNDRY_BASE_URL"], + ["ANTHROPIC_AWS", "ANTHROPIC_AWS_BASE_URL"], +])("Claude keeps the %s provider selector with its endpoint", async (provider, endpoint) => { + const s = setup("CODEX_THREAD_ID"); + vi.stubEnv(`CLAUDE_CODE_USE_${provider}`, "1"); + vi.stubEnv(endpoint, "https://provider.example"); + vi.stubEnv("ANTHROPIC_MODEL", "parent-model"); + vi.stubEnv("ANTHROPIC_BEDROCK_MODEL", "parent-model"); + const result = await quest("--root", s.repo, "review"); + expect(result.code, result.stderr).toBe(0); + for (const call of s.calls()) { + expect(call.env[`CLAUDE_CODE_USE_${provider}`]).toBe("1"); + expect(call.env[endpoint]).toBe("https://provider.example"); + expect(call.env.ANTHROPIC_MODEL).toBeUndefined(); + expect(call.env.ANTHROPIC_BEDROCK_MODEL).toBeUndefined(); + } +}); + fakes("--deep picks each reviewer's stronger model, which --model cannot also name", async () => { const s = setup(); vi.stubEnv("FAKE_REPLY", JSON.stringify({ findings: [] }));