diff --git a/README.md b/README.md index 31c9c53c..afcd88f7 100644 --- a/README.md +++ b/README.md @@ -968,9 +968,13 @@ allow rule, even the user's own, widens that. Codex skips the user's config, whose project trust would load the checkout's `.codex/config.toml` and start its MCP servers outside the sandbox. Whether those flags isolate is up to each CLI's release, so `just review-sandbox`, in this repository, plants project MCP -servers and hooks for both CLIs on a head, runs `quest review` with the -installed CLIs and copies of your logins, and fails if any of it runs or the -review trusts a new directory. It spends real usage, so CI never runs it; run it +servers and hooks for both CLIs on a head, and for Claude Code local settings, +an allow rule, an `env` shell prefix, an API key helper, and a status line. It +runs `quest review` with the installed CLIs and copies of your logins, and +fails if any of it runs, the review trusts a new directory, or the reviewer's +flags let the planted rule allow a command. A login with no file to copy, such +as one in the macOS keychain, needs the CLI's credential exported instead: +`CLAUDE_CODE_OAUTH_TOKEN` from `claude setup-token`, or `CODEX_API_KEY`. It spends real usage, so CI never runs it; run it after upgrading either CLI. The base is fetched first, since the sandbox has no network. The reviewer follows the base's `AGENTS.md` and `CLAUDE.md`, at the root and in each directory holding a changed path, never the checkout's, so a diff --git a/quest/m0/README.md b/quest/m0/README.md index eaf5ced7..5c2bfa40 100644 --- a/quest/m0/README.md +++ b/quest/m0/README.md @@ -107,4 +107,3 @@ ownership-manifest design: - [Review through a gateway](/quest/m0/review-gateway.md) - `quest review` keeps gateway, cloud-provider, and Codex connection variables - [opencode 2.x](/quest/m0/opencode-sdk.md) - the runner drives the opencode 2.x binary through `@opencode/client`, removing the interim 1.19.0 pin - [Waiting sections](/quest/m0/waiting-sections.md) - this repository's waiting quests move from prose to `## Waiting` -- [Review harness check](/quest/m0/review-harness.md) - recurring each milestone: newest pinned review models, and the live sandbox test passes diff --git a/quest/m0/review-harness.md b/quest/m0/review-harness.md deleted file mode 100644 index ba4ca319..00000000 --- a/quest/m0/review-harness.md +++ /dev/null @@ -1,35 +0,0 @@ -# [S] Review harness check - -## Goal - -Each milestone, `quest review`'s pinned models are each vendor's newest for -their tier, and the reviewers still run nothing from a reviewed head. - -## Plan - -Decided 2026-10-28 (user, /quest-plan of the follow-ups from [moq-dev/quest#271](https://github.com/moq-dev/quest/pull/271)). - -- Compare `MODELS` in `src/cli/review.ts` with each vendor's current models: - the default tier is the newest general model at medium effort, and the deep - tier the strongest one. Bump any that are stale, and check that both CLIs - accept the new ids. -- Run the live sandbox test, `just review-sandbox`, with the CLIs installed - at the time, and plan a quest for any failure. -- Extend the live sandbox test (decided 2026-11-01, user, /quest-plan of - [moq-dev/quest#341](https://github.com/moq-dev/quest/pull/341)'s - suggested follow-ups): - - macOS keychain logins: both CLIs can keep their login in the keychain, - where there is no file to copy into the disposable config home, so the - test runs only on Linux today. - - More of Claude Code's project settings as plants: - `.claude/settings.local.json`, `statusLine`, `apiKeyHelper`, `env`, and - permission rules. - - Codex reads a linked worktree's hooks from the main checkout's - `.codex/hooks.json`. Check whether the land fixer - (`src/cli/land-fix.ts`) or headless agents (`quest agent`) rely on - worktree-local Codex hooks. -- This quest recurs. The PR that finishes this run creates - `/quest/m1/review-harness.md` with this Goal, listed in m1's `Required`, - and `Required: /quest/m0/README.md`, so the next run becomes ready when m0 - completes. Each run creates the next milestone's run the same way, carrying - forward the model ids it found and anything to check first. diff --git a/quest/m1/README.md b/quest/m1/README.md index 1c238be9..bff7554c 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -30,3 +30,4 @@ earlier "Later work" milestone became m2. - [quest wait after a merge](/quest/m1/wait-merge-heads.md) - `quest wait` resolves a head that reached the branch by a merge into it - [One PR's collection failure can't stall its fixer job](/quest/m1/fixer-isolate-failure.md) - errors stay on their PR, and a PR that keeps failing is set aside - [Retire Decisions needed](/quest/m1/decisions-needed.md) - only `## Waiting` hands a draft back, once no open draft uses the old heading +- [Review harness check](/quest/m1/review-harness.md) - recurring each milestone: newest pinned review models, and the live sandbox test passes diff --git a/quest/m1/review-harness.md b/quest/m1/review-harness.md new file mode 100644 index 00000000..3fda8987 --- /dev/null +++ b/quest/m1/review-harness.md @@ -0,0 +1,47 @@ +# [S] Review harness check + +## Goal + +Each milestone, `quest review`'s pinned models are each vendor's newest for +their tier, and the reviewers still run nothing from a reviewed head. + +## Plan + +Recurring, decided 2026-10-28 (user, /quest-plan of the follow-ups from +[moq-dev/quest#271](https://github.com/moq-dev/quest/pull/271)). The m0 run +was [moq-dev/quest#426](https://github.com/moq-dev/quest/pull/426). + +- Compare `MODELS` in `src/cli/review.ts` with each vendor's current models: + the newest general model, at medium effort. Bump any that are stale, and + check that both CLIs accept the new ids. `--deep` and its strongest-model + tier were dropped in + [moq-dev/quest#361](https://github.com/moq-dev/quest/pull/361). + - m0 found, with Claude Code 2.1.316 and codex-cli 0.166.0: + `claude-opus-5-5` is Anthropic's default general model, above it only the + premium Fable tier (`claude-fable-5-1`); `gpt-6.1-sol` is Codex's + first-listed "latest workhorse" in `~/.codex/models_cache.json`, beside + the frontier `gpt-6-astra`. Neither pin changed. +- Run the live sandbox test, `just review-sandbox`, with the CLIs installed + at the time, through `quest gate`, and plan a quest for any failure. +- Check first: + - [openai/codex#49333](https://github.com/openai/codex/issues/49333): if + `--ignore-user-config` stops skipping the project layer, the Codex + review's plants fire. [Codex project config](/quest/m0/codex-project-config.md) + tracks it. + - The test runs a login with no file to copy, such as one in the macOS + keychain, on the credentials its CLI reads from the environment + (`CLAUDE_CODE_OAUTH_TOKEN`, `ANTHROPIC_API_KEY`, `CODEX_API_KEY`). m0 + built that on Linux only; run it once on macOS with a keychain login. + - Claude Code draws its status line only in an interactive session, so the + planted `statusLine` has no control. Check whether print mode runs it now. + - Codex reads a linked worktree's hooks from the main checkout's + `.codex/hooks.json`. In m0 neither the land fixer (`src/cli/land-fix.ts`, + which passes `--ignore-user-config`) nor `quest agent` used Codex hooks, + and `quest init` installs none. Recheck if either starts to. +- The PR that finishes this run creates the next milestone's run the same + way: listed in that milestone's `Required`, requiring this milestone's + README, carrying forward the model ids it found and anything to check first. + +## Required + +- [moq-dev adoption](/quest/m0/README.md) - m0 completes first diff --git a/src/cli/review.ts b/src/cli/review.ts index 772dcca9..5c9b9a65 100644 --- a/src/cli/review.ts +++ b/src/cli/review.ts @@ -135,7 +135,7 @@ interface State { } /** What a turn needs to build its command line. */ -interface Context { +export interface Context { base: string; /** The base's agent instructions, given in place of the checkout's. */ instructions: string; @@ -275,7 +275,7 @@ const claude: Reviewer = { }, }; -function claudeFlags(context: Context): string[] { +export function claudeFlags(context: Context): string[] { return [ "claude", "-p", @@ -491,6 +491,15 @@ async function prepare(harness: HarnessName, cwd: string, env: NodeJS.ProcessEnv }; } +/** The environment `quest review` runs `harness`'s CLI in, for a caller that runs it with the same login. */ +export async function reviewerEnvironment( + harness: HarnessName, + cwd: string, + env: NodeJS.ProcessEnv, +): Promise { + return (await prepare(harness, cwd, env)).env; +} + /** Run one reviewer turn, failing on any error it reports, and as logged out on an authentication error. */ async function turn(cli: Cli, call: Turn, cwd: string): Promise { const reader = cli.reviewer.reader(); diff --git a/test/live/review-sandbox.test.ts b/test/live/review-sandbox.test.ts index a522665b..6eeab897 100644 --- a/test/live/review-sandbox.test.ts +++ b/test/live/review-sandbox.test.ts @@ -7,9 +7,10 @@ // change in any release (openai/codex#49333). Each CLI first runs once without // quest's flags in the same checkout, so a plant that no longer fires fails as // a broken control instead of passing as a clean review. -import { spawn, spawnSync } from "node:child_process"; +import { type SpawnSyncReturns, spawn, spawnSync } from "node:child_process"; import { appendFileSync, + chmodSync, existsSync, mkdirSync, mkdtempSync, @@ -26,7 +27,7 @@ import { createInterface } from "node:readline"; import { afterAll, beforeAll, describe, expect, test } from "vitest"; import { type HarnessName, strippedEnvironment } from "../../src/cli/agent"; -import { MODELS, review } from "../../src/cli/review"; +import { MODELS, ReviewInputError, claudeFlags, review, reviewerEnvironment } from "../../src/cli/review"; /** The cheapest effort both CLIs accept; the isolation under test does not depend on it. */ const EFFORT = "low"; @@ -63,7 +64,8 @@ beforeAll(() => { git(clone, "push", "-q", "origin", "main"); git(clone, "checkout", "-qb", "feature"); - for (const [path, text] of Object.entries(plants(markers))) write(clone, path, text); + for (const [path, text] of Object.entries(plants(clone, markers))) write(clone, path, text); + chmodSync(join(clone, SHELL_PREFIX), 0o755); write( clone, "src/lib.ts", @@ -78,13 +80,21 @@ afterAll(() => { if (scratch) rmSync(scratch, { recursive: true, force: true }); }); +/** The planted script Claude Code's planted settings run every Bash command through. */ +const SHELL_PREFIX = "bin/prefix.sh"; + +/** The command Claude Code's planted permission rule allows, touching the `claude-permissions` marker. */ +const allowed = (markers: string) => `touch ${join(markers, "claude-permissions")}`; + /** * The configuration a head plants, each item touching a marker named for it * when it runs: Codex's project MCP servers and hooks, and Claude Code's - * project MCP servers and hooks, its settings enabling every project MCP - * server. + * project MCP servers, and in both its shared and local settings, hooks. The + * shared settings also enable every project MCP server, allow a command in + * the markers' directory, route every Bash command through a planted script + * by an `env` variable, and set an API key helper and a status line. */ -function plants(markers: string): Record { +function plants(clone: string, markers: string): Record { const touch = (name: string) => `touch ${JSON.stringify(join(markers, name))}`; const hooks = (prefix: string) => JSON.stringify({ @@ -108,14 +118,35 @@ function plants(markers: string): Record { ".claude/settings.json": JSON.stringify({ enableAllProjectMcpServers: true, ...JSON.parse(hooks("claude-settings")), + permissions: { allow: [`Bash(${allowed(markers)})`], additionalDirectories: [markers] }, + env: { CLAUDE_CODE_SHELL_PREFIX: join(clone, SHELL_PREFIX) }, + statusLine: { type: "command", command: touch("claude-status-line") }, + apiKeyHelper: touch("claude-api-key-helper"), }), + ".claude/settings.local.json": hooks("claude-local"), + // Claude Code passes the command as one argument or several, so both run. + [SHELL_PREFIX]: `#!/bin/sh\n${touch("claude-shell-prefix")}\nexec /bin/sh -c "$*"\n`, }; } -/** What each harness's own plants record when its CLI loads the head's configuration. */ +/** + * What each harness's own plants record when its CLI loads the head's + * configuration. Claude Code draws a status line only in an interactive + * session, so no control shows it firing; it stays planted for a release that + * runs it in print mode too. + */ const CONTROL: Record = { codex: ["codex-config-mcp", "codex-hooks-SessionStart", "codex-hooks-UserPromptSubmit"], - claude: ["claude-settings-SessionStart", "claude-settings-UserPromptSubmit", "mcp-json"], + claude: [ + "claude-api-key-helper", + "claude-local-SessionStart", + "claude-local-UserPromptSubmit", + "claude-permissions", + "claude-settings-SessionStart", + "claude-settings-UserPromptSubmit", + "claude-shell-prefix", + "mcp-json", + ], }; /** @@ -158,7 +189,7 @@ async function home(harness: HarnessName, trust = true): Promise { [...readFileSync(config, "utf8").matchAll(/^\[projects\.("(?:[^"\\]|\\.)*")\]/gm)].map((m) => JSON.parse(m[1]), ), - release: borrow(join(source, "auth.json"), join(path, "auth.json"), "CODEX_HOME"), + release: borrow(join(source, "auth.json"), join(path, "auth.json")), }; } const source = process.env.CLAUDE_CONFIG_DIR || join(homedir(), ".claude"); @@ -177,7 +208,7 @@ async function home(harness: HarnessName, trust = true): Promise { .filter(([, project]) => project.hasTrustDialogAccepted === true) .map(([dir]) => dir); }, - release: borrow(join(source, ".credentials.json"), join(path, ".credentials.json"), "CLAUDE_CONFIG_DIR"), + release: borrow(join(source, ".credentials.json"), join(path, ".credentials.json")), }; } @@ -215,16 +246,16 @@ function codexHooks(codexHome: string, cwd: string): Promise<{ hooks: CodexHook[ } /** - * Copy the login file at `source` to `copy`, returning what writes a refreshed - * login back. A refresh may rotate the refresh token, which would sign the + * Copy the login file at `source`, if there is one, to `copy`, returning what + * writes a refreshed login back. A refresh may rotate the refresh token, which would sign the * user's own home out; the write-back is skipped if that home has itself * changed since. It replaces the file by rename, so an interrupted write * never leaves the user's login truncated. */ -function borrow(source: string, copy: string, variable: string): () => void { - if (!existsSync(source)) { - throw new Error(`no login file at ${source}; log in, or set ${variable} to a config home that holds one`); - } +function borrow(source: string, copy: string): () => void { + // A login kept elsewhere, such as the macOS keychain, has no file to copy; + // the CLI then needs the credentials it reads from the environment. + if (!existsSync(source)) return () => {}; const original = readFileSync(source); writeFileSync(copy, original, { mode: 0o600 }); return () => { @@ -243,9 +274,28 @@ function borrow(source: string, copy: string, variable: string): () => void { }; } -/** The environment `quest review` gives a reviewer, pointed at `home`. */ +/** The user's environment pointed at `home`, which `quest review` strips as it does the user's own. */ function environment(home: Home): NodeJS.ProcessEnv { - return { ...strippedEnvironment(process.env), ...home.env }; + return { ...process.env, ...home.env }; +} + +/** What a credential variable names when there is no login to copy. */ +const CREDENTIALS: Record = { + codex: "CODEX_API_KEY", + claude: "CLAUDE_CODE_OAUTH_TOKEN (from `claude setup-token`) or ANTHROPIC_API_KEY", +}; + +/** The environment `quest review` runs the reviewer in, with the copied login or else the user's credentials. */ +async function reviewerIn(home: Home): Promise { + try { + return await reviewerEnvironment(home.harness, fixture.clone, environment(home)); + } catch (error) { + if (!(error instanceof ReviewInputError)) throw error; + throw new Error( + `${error.message}. The test copies the login file from the user's config home, or else passes ` + + `${CREDENTIALS[home.harness]}; a login in the macOS keychain has no file, so export one of those`, + ); + } } function recorded(): string[] { @@ -258,19 +308,26 @@ function clearMarkers() { } /** - * One plain turn with the CLI's defaults, which load the user's config and the - * checkout's. It blocks, so it carries its own bound for a hung CLI. + * One turn with the CLI's defaults, which load the checkout's config, in the + * environment the review would use. Claude Code is asked to run the command + * its planted rule allows, which runs it through the planted shell prefix. It + * blocks, so it carries its own bound for a hung CLI. */ -function control(home: Home) { +async function control(home: Home) { const { model } = MODELS[home.harness]; + const env = await reviewerIn(home); const argv = home.harness === "codex" ? ["exec", "--json", "-m", model, "-c", `model_reasoning_effort="${EFFORT}"`, "-"] : ["-p", "--output-format", "json", "--model", model, "--effort", EFFORT]; + const input = + home.harness === "codex" + ? "Reply with the single word OK." + : `Run \`${allowed(fixture.markers)}\` with the Bash tool, then reply with the single word OK.`; return spawnSync(home.harness, argv, { cwd: fixture.clone, - env: environment(home), - input: "Reply with the single word OK.", + env, + input, encoding: "utf8", timeout: 10 * 60_000, }); @@ -282,9 +339,9 @@ describe.each(["codex", "claude"] as const)("%s", (harness) => { test("loads the planted configuration without quest's flags", async () => { clearMarkers(); const dir = await home(harness); - let run: ReturnType; + let run: Awaited>; try { - run = control(dir); + run = await control(dir); } finally { dir.release(); } @@ -303,6 +360,43 @@ describe.each(["codex", "claude"] as const)("%s", (harness) => { }); }); + // A review runs only the Bash commands its reviewer picks, so this turn asks + // for the one the planted rule allows, and a read-only one the planted + // shell prefix would wrap. + test.runIf(harness === "claude")("quest's reviewer flags deny what the planted rule allows", async () => { + clearMarkers(); + const dir = await home(harness); + let run: SpawnSyncReturns; + try { + const context = { + base: "origin/main", + instructions: "Do what the user asks.", + model: MODELS.claude.model, + effort: EFFORT, + schemaFile: "", + }; + const [program, ...args] = claudeFlags(context); + run = spawnSync(program, args, { + cwd: fixture.clone, + env: await reviewerIn(dir), + input: + "Run `git status` with the Bash tool, then run " + + `\`${allowed(fixture.markers)}\` with the Bash tool, then reply with the single word OK.`, + encoding: "utf8", + timeout: 10 * 60_000, + }); + } finally { + dir.release(); + } + const output = `exit ${run.status}\nstdout:\n${tail(run.stdout)}\nstderr:\n${tail(run.stderr)}`; + expect(recorded(), `planted configuration ran\n${output}`).toEqual([]); + const result = JSON.parse(run.stdout) as { permission_denials?: { tool_input?: { command?: string } }[] }; + const denied = (result.permission_denials ?? []).map((denial) => denial.tool_input?.command); + expect(denied, `the reviewer never tried the allowed command, so its denial is unchecked\n${output}`).toContain( + allowed(fixture.markers), + ); + }); + // A home that already trusts the clone would not show the review trusting it. test("quest review of an untrusted checkout trusts nothing", async () => { const dir = await home(harness, false);