diff --git a/.agents/skills/autoreview/AGENTS.md b/.agents/skills/autoreview/AGENTS.md new file mode 100644 index 000000000000..5a0173d73d39 --- /dev/null +++ b/.agents/skills/autoreview/AGENTS.md @@ -0,0 +1,6 @@ +# Autoreview Skill + +- Canonical source: `openclaw/agent-skills`, under `skills/autoreview`. +- Before editing any copy, fast-forward a checkout of `openclaw/agent-skills` from `origin/main`. +- Make and validate shared changes in canonical `skills/autoreview` first, then sync the complete directory into downstream repos. +- Never create repo-local behavior variants; downstream differences belong in repo-level validation, not the skill. diff --git a/.agents/skills/autoreview/CLAUDE.md b/.agents/skills/autoreview/CLAUDE.md new file mode 120000 index 000000000000..47dc3e3d863c --- /dev/null +++ b/.agents/skills/autoreview/CLAUDE.md @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file diff --git a/.agents/skills/autoreview/SKILL.md b/.agents/skills/autoreview/SKILL.md index 4eb891cf69ac..ff733bc86fc4 100644 --- a/.agents/skills/autoreview/SKILL.md +++ b/.agents/skills/autoreview/SKILL.md @@ -1,19 +1,19 @@ --- name: autoreview -description: "Pre-commit/ship code review: Codex default; optional Claude, Pi, Droid, Copilot, Cursor, or OpenCode." +description: "Pre-commit/ship code review: Codex default; optional Claude or Pi." --- # Auto Review Run the bundled structured review helper as a closeout check. This is code review, not Guardian `auto_review` approval routing. -Codex review is the default when no engine is set. It uses `gpt-5.6-sol` by default, usually delivers the best review results, and should remain the normal final closeout engine. Claude review is optional and uses `claude-fable-5` by default. +Codex review is the default when no engine is set. It uses `gpt-5.6-sol` with `high` reasoning by default, then retries once with `gpt-5.6-terra` only when the account cannot access Sol. Claude review is optional and uses `claude-fable-5` by default. For user-visible behavior, pair autoreview with `behavior-validator`. Autoreview is source-aware and judges the change bundle; behavior validation is source-blind and judges the running product or tool against a behavior contract. A clean autoreview is not proof that a UI, CLI, API, or generated artifact works from the user's perspective. Use when: -- user asks for Codex review / Claude review / Pi review / Droid review / Cursor review / OpenCode review / autoreview / second-model review +- user asks for Codex review / Claude review / Pi review / autoreview / second-model review - after non-trivial code edits, before final/commit/ship - reviewing a local branch or PR branch after fixes @@ -29,12 +29,14 @@ Use when: - Keep going until structured review returns no accepted/actionable findings only while the work remains inside the original task scope. - If a review-triggered fix changes code, rerun focused tests and rerun the structured review helper. - For security-audit suppression changes, verify accepted findings remain auditable: suppressed findings stay in structured output, active output keeps an unsuppressible suppression notice, and aggregate findings cannot hide unrelated active risk. -- Never switch or override the requested review engine/model. If the review hits model capacity, retry the same command a few times with the same engine/model. +- Never switch or override the requested review engine/model except for the documented Codex Sol-to-Terra account-access fallback. Capacity, rate-limit, and unrelated failures keep the same engine/model. - Be patient with large bundles. Structured review can take up to 30 minutes while the model call is active, especially with Codex tools or web search. -- Treat heartbeat lines like `review still running: ... elapsed=... pid=...` as healthy progress, not a hang. Let the helper continue while heartbeats are advancing. Pass `--stream-engine-output` when live engine text is useful; Codex, Claude, and Cursor filter tool/file chatter, other engines pass raw output through. +- Treat heartbeat lines like `review still running: ... elapsed=... pid=...` as healthy progress, not a hang. Let the helper continue while heartbeats are advancing. Pass `--stream-engine-output` when live engine text is useful; Codex and Claude filter tool/file chatter, other runnable engines pass raw output through. - Do not kill a review just because it has been quiet for 2-5 minutes, or because it is still running under the 30-minute window. Inspect the process only after missing multiple expected heartbeats, after 30 minutes, or after an obviously failed subprocess; prefer letting the same helper command finish. -- Tools are useful in review mode. The helper allows read-only inspection tools and web search by default so reviewers can check dependency contracts, upstream docs, and current behavior. +- Tools are useful in review mode. Codex receives the validated bundle in an empty workspace so ignored files and linked-worktree metadata remain unreadable; web search stays available for dependency contracts and upstream docs. - Security perspective is always included, but it should not cripple legitimate functionality. Report security findings only when the change creates a concrete, actionable risk or removes an important safety check. +- Reviewer subprocesses preserve engine authentication and non-credentialed proxy variables needed by headless or restricted-network environments while stripping process-injection, Git override, and credentialed proxy values. +- Review bundles fail closed before engine invocation when tracked or untracked paths look sensitive, patch text looks secret-like, or a Git diff exceeds the bundle limit. Redact/split the change; never accept a truncated patch as complete review proof. - For regression provenance, keep roles separate: blamed code author, blamed PR author, PR merger/committer, current PR author, and PR/date. If no blamed PR is traceable, use the blamed commit as the provenance: commit SHA, date, and author username. Do not guess a merger or frame missing PR metadata as a separate finding. - If the blamed PR was merged by `clawsweeper[bot]` or another automation, identify the human trigger when practical. Check timeline/comments first; if rate-limited, use gitcrawl/cache or public PR HTML. Look for maintainer commands such as `@clawsweeper automerge`, `/landpr`, or labels/status comments that armed automerge. Report `automerge triggered by @login`; if not found, say trigger unknown. - Do not invoke built-in `codex review`, nested reviewers, or reviewer panels from inside the review. The helper builds one bundle, calls one selected engine, validates one structured result, and stops. @@ -109,6 +111,27 @@ export AUTOREVIEW_HARNESS="$AGENTS_HOME/skills/autoreview/scripts/test-review-ha When using Claude Code, set `AGENTS_HOME="$HOME/.claude"` for global skills. Project-local skills live under `.claude/skills/` in the current repo. +On native Windows, choose the matching pair: + +```powershell +# Project-local skill in the current repo: +$AUTOREVIEW = ".agents\skills\autoreview\scripts\autoreview" +$AUTOREVIEW_HARNESS = ".agents\skills\autoreview\scripts\test-review-harness.ps1" +``` + +```powershell +# Source checkout of openclaw/agent-skills: +$AUTOREVIEW = "skills\autoreview\scripts\autoreview" +$AUTOREVIEW_HARNESS = "skills\autoreview\scripts\test-review-harness.ps1" +``` + +```powershell +# Global skill: +$AgentsHome = if ($env:AGENTS_HOME) { $env:AGENTS_HOME } else { Join-Path $HOME ".agents" } +$AUTOREVIEW = Join-Path $AgentsHome "skills\autoreview\scripts\autoreview" +$AUTOREVIEW_HARNESS = Join-Path $AgentsHome "skills\autoreview\scripts\test-review-harness.ps1" +``` + ## Pick Target Dirty local work: @@ -173,7 +196,7 @@ Tradeoff: tests may force code changes that stale the review. If tests or review Run multiple reviewers against one frozen bundle: ```bash -"$AUTOREVIEW" --reviewers codex,claude,pi,opencode +"$AUTOREVIEW" --reviewers codex,claude,pi ``` `--panel` is shorthand for Codex plus Claude unless `--engine` changes the first reviewer: @@ -198,14 +221,10 @@ For models with slashes or extra colons, prefer keyed form: ```bash "$AUTOREVIEW" --engine pi --model anthropic/claude-sonnet-4 --thinking high -"$AUTOREVIEW" --engine opencode --model opencode/north-mini-code-free --thinking high -"$AUTOREVIEW" --engine cursor --model auto --cursor-allow-workspace-instructions "$AUTOREVIEW" --reviewers codex,pi --model codex=gpt-5.6-sol --model pi=anthropic/claude-sonnet-4 -"$AUTOREVIEW" --reviewers codex,opencode --model codex=gpt-5.6-sol --model opencode=opencode/north-mini-code-free -"$AUTOREVIEW" --reviewers codex,cursor --model codex=gpt-5.6-sol --model cursor=auto --cursor-allow-workspace-instructions ``` -`--reviewers all` covers Codex, Claude, Copilot, Pi, and OpenCode. Cursor requires both explicit selection (`--engine cursor` or named in `--reviewers`) and `--cursor-allow-workspace-instructions` because the current Cursor CLI does not document a per-run flag that ignores project-local instructions/config. Droid selection currently fails closed because its CLI cannot disable both project instructions and all tools. +`--reviewers all` covers Codex, Claude, and Pi. Droid, Copilot, Cursor, and OpenCode selections fail closed because their current CLI contracts cannot confine project instructions, filesystem reads, or network fetches to the review boundary. ## Models and thinking @@ -213,25 +232,27 @@ The helper accepts `--model` globally or per engine (`engine=model`) and `--thin Recommended model defaults: -| Engine | Default model | Source note | -| ------------------- | ---------------- | ----------------------------------------------------- | -| **codex** (default) | `gpt-5.6-sol` | OpenAI's current high-capability GPT-5.6 tier | -| **claude** | `claude-fable-5` | Anthropic's most capable widely released Claude model | +| Engine | Default model | Source note | +| ------------------- | -------------------------------------------------- | ----------------------------------------------------- | +| **codex** (default) | `gpt-5.6-sol` -> `gpt-5.6-terra` on access failure | OpenClaw org review default | +| **claude** | `claude-fable-5` | Anthropic's most capable widely released Claude model | -CLI flags and environment variables override these defaults. Droid, Copilot, Pi, Cursor, and OpenCode do not get built-in model defaults here because their provider catalogs are external to the Codex/Claude closeout path and may vary by installation. +CLI flags and environment variables override these defaults. Pi does not get a built-in model default because its provider catalog may vary by installation. Droid, Copilot, Cursor, and OpenCode are currently refused. -| Engine | Model flag | Example model IDs | Thinking flag | Accepted levels | -| ------------------- | -------------------------- | ---------------------------------------------------------------------------- | ----------------------------- | ------------------------------------------------------ | -| **codex** (default) | `codex --model X exec ...` | `gpt-5.6-sol`, `gpt-5.6-luna` | `-c model_reasoning_effort=Y` | `none`, `minimal`, `low`, `medium`, `high`, `xhigh` | -| **claude** | `claude --model X` | `claude-fable-5`, `claude-opus-4-8`, `claude-sonnet-4-6`, `claude-haiku-4-5` | `--effort Y` | `low`, `medium`, `high`, `xhigh`, `max` | -| **droid** | currently refused | Factory model IDs | `-r, --reasoning-effort Y` | `off`, `none`, `low`, `medium`, `high`, `xhigh`, `max` | -| **copilot** | `copilot --model X` | `gpt-5.2`, Copilot model aliases | not supported | n/a | -| **pi** | `pi --model X` | `anthropic/claude-sonnet-4`, `openai/gpt-4o` | `--thinking Y` | `off`, `minimal`, `low`, `medium`, `high`, `xhigh` | -| **cursor** | `cursor-agent --model X` | `auto`, Cursor model aliases | not supported | n/a | -| **opencode** | `opencode run -m X` | `opencode/north-mini-code-free`, OpenCode provider/model IDs | `--variant Y` | `minimal`, `low`, `medium`, `high`, `max` | +| Engine | Model flag | Example model IDs | Thinking flag | Accepted levels | +| ------------------- | -------------------------- | ---------------------------------------------------------------------------- | ----------------------------- | ---------------------------------------------------------- | +| **codex** (default) | `codex --model X exec ...` | `gpt-5.6-sol`, then `gpt-5.6-terra` on Sol access failure | `-c model_reasoning_effort=Y` | `none`, `minimal`, `low`, `medium`, `high`, `xhigh`, `max` | +| **claude** | `claude --model X` | `claude-fable-5`, `claude-opus-4-8`, `claude-sonnet-4-6`, `claude-haiku-4-5` | `--effort Y` | `low`, `medium`, `high`, `xhigh`, `max` | +| **droid** | currently refused | Factory model IDs | `-r, --reasoning-effort Y` | `off`, `none`, `low`, `medium`, `high`, `xhigh`, `max` | +| **copilot** | currently refused | Copilot model aliases | not supported | n/a | +| **pi** | `pi --model X` | `anthropic/claude-sonnet-4`, `openai/gpt-4o` | `--thinking Y` | `off`, `minimal`, `low`, `medium`, `high`, `xhigh` | +| **cursor** | currently refused | Cursor model aliases | not supported | n/a | +| **opencode** | currently refused | OpenCode provider/model IDs | not supported | n/a | Claude also supports `--fallback-model a,b` for availability-based fallback chains ([model-config](https://code.claude.com/docs/en/model-config)). Current Claude docs note that auth, billing, rate-limit, request-size, and transport errors do not trigger fallback, and the changelog documents interactive-session support in `v2.1.166`. +[OpenAI's model guidance](https://developers.openai.com/api/docs/guides/latest-model) identifies Sol as the GPT-5.6 frontier-capability route and documents `max` support. Autoreview keeps `high` as its default; use `max` only for the hardest quality-first reviews after comparing its latency and cost with `xhigh` on representative changes. + Examples matching current `main` behavior: ```bash @@ -241,24 +262,16 @@ Examples matching current `main` behavior: # Codex fast mode (priority service tier); needs a model whose catalog lists the tier, silently standard otherwise "$AUTOREVIEW" --engine codex --codex-speed fast -# Arbitrary Codex config overrides (isolation flags still win; --codex-speed wins over a service_tier here) +# Safe Codex model/response tuning overrides (--codex-speed wins over a service_tier here) "$AUTOREVIEW" --engine codex --codex-config 'service_tier="fast"' # Claude Code aliases or full model names, with optional availability fallback "$AUTOREVIEW" --engine claude --model claude-fable-5 --thinking max "$AUTOREVIEW" --engine claude --model claude-fable-5 --fallback-model claude-opus-4-8,claude-sonnet-4-6 -# GitHub Copilot (model only; no thinking knob) -"$AUTOREVIEW" --engine copilot --model gpt-5.2 - # Pi with explicit model and thinking level "$AUTOREVIEW" --engine pi --model anthropic/claude-sonnet-4 --thinking high --pi-bin pi -# Cursor print-mode review (`cursor-agent` remains a compatibility alias) -"$AUTOREVIEW" --engine cursor --model auto --cursor-bin cursor-agent --cursor-allow-workspace-instructions - -# OpenCode with explicit provider/model and variant -"$AUTOREVIEW" --engine opencode --model opencode/north-mini-code-free --thinking high ``` `--cursor-agent-bin` and `CURSOR_AGENT_BIN` remain compatibility aliases for @@ -272,34 +285,36 @@ Store persistent personal defaults in your shell startup file or launcher environment. For repository-local defaults, use an existing local environment loader such as an untracked `.envrc`; the helper does not write a config file. -| Variable | Purpose | -| ------------------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------- | -| `AUTOREVIEW_MODEL` | Override the built-in default `--model` for all engines | -| `AUTOREVIEW_THINKING` | Default `--thinking` for all engines | -| `AUTOREVIEW_FALLBACK_MODEL` | Default Claude `--fallback-model` chain | -| `AUTOREVIEW__MODEL` | Per-engine model override, for example `AUTOREVIEW_CODEX_MODEL=gpt-5.6-sol` | -| `AUTOREVIEW__THINKING` | Per-engine thinking override | -| `AUTOREVIEW_CODEX_CONFIG` | Default Codex `-c key=value` overrides, semicolon-separated, e.g. `service_tier="fast"`; isolation flags still win | -| `AUTOREVIEW_CODEX_SPEED` | Default Codex service tier: `fast` (priority), `flex`, or `default`; silently standard when the model does not list the tier | -| `AUTOREVIEW_CLAUDE_FALLBACK_MODEL` | Claude-only fallback chain | -| `AUTOREVIEW_CURSOR_ALLOW_WORKSPACE_INSTRUCTIONS` | Required `1`/true opt-in for Cursor reviews of trusted repositories | +| Variable | Purpose | +| ---------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | +| `AUTOREVIEW_MODEL` | Override the built-in default `--model` for all engines | +| `AUTOREVIEW_THINKING` | Default `--thinking` for all engines | +| `AUTOREVIEW_FALLBACK_MODEL` | Default Claude `--fallback-model` chain | +| `AUTOREVIEW__MODEL` | Per-engine model override, for example `AUTOREVIEW_CODEX_MODEL=gpt-5.6-sol` | +| `AUTOREVIEW__THINKING` | Per-engine thinking override | +| `AUTOREVIEW_CODEX_CONFIG` | Safe Codex model/response tuning overrides, semicolon-separated, e.g. `service_tier="fast"`; capability-bearing keys fail closed | +| `AUTOREVIEW_CODEX_SPEED` | Codex service tier override: `fast` (priority), `flex`, or `default`; silently standard when the model does not list the tier | +| `AUTOREVIEW_CLAUDE_FALLBACK_MODEL` | Claude-only fallback chain | -Codex maps thinking to `model_reasoning_effort`. Claude maps thinking to `--effort`. Droid maps thinking to `-r, --reasoning-effort`. Pi maps thinking to `--thinking`. OpenCode maps thinking to `--variant`. Copilot and Cursor reject `--thinking`. Only Claude accepts `--fallback-model`; global CLI/env fallback requires at least one Claude reviewer, and engine-specific fallback overrides require that reviewer to be selected. Non-Claude fallback overrides, including `AUTOREVIEW__FALLBACK_MODEL`, fail closed instead of being silently ignored. +Codex maps thinking to `model_reasoning_effort`. Claude maps thinking to `--effort`. Pi maps thinking to `--thinking`. Only Claude accepts `--fallback-model`; global CLI/env fallback requires at least one Claude reviewer, and engine-specific fallback overrides require that reviewer to be selected. Non-Claude fallback overrides, including `AUTOREVIEW__FALLBACK_MODEL`, fail closed instead of being silently ignored. ## Review engine isolation When autoreview runs inside the repository under review, external reviewer CLIs must not load project-local trust or configuration that the branch controls. -| Engine | Isolation flags | Reference | -| ------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| **codex** | Auth-only config overrides, `-c project_doc_max_bytes=0`, repo `trust_level="untrusted"`, `exec --ignore-user-config --ignore-rules`, plus read-only sandbox | Codex CLI `exec --help` | -| **claude** | `--safe-mode --setting-sources user --strict-mcp-config --disallowedTools mcp__*` plus explicit `--allowedTools` (`--safe-mode` requires Claude Code `v2.1.169+`) | Claude Code [CLI reference](https://code.claude.com/docs/en/cli-reference) | -| **droid** | Fails closed: current CLI cannot disable both project instructions and all tools | Droid CLI `exec --help` and `--list-tools` | -| **pi** | `--no-approve --no-session --no-context-files --no-extensions --no-skills --no-prompt-templates --no-themes --no-tools` | Pi CLI `--help`; requires Pi `v0.79.0+` | -| **opencode** | `opencode run --dir --pure --format json`, prompt over stdin, neutral subprocess cwd, injected deny-by-default permissions, project config disabled | OpenCode CLI `--help` | -| **cursor** | `cursor-agent --print --mode ask --sandbox enabled --output-format json\|stream-json`, prompt over stdin, temporary read-only permission config, help-probed flags, and mandatory explicit trusted-workspace opt-in | Cursor CLI [headless mode](https://cursor.com/docs/cli/headless), [output format](https://cursor.com/docs/cli/reference/output-format), [permissions](https://cursor.com/docs/cli/reference/permissions), [configuration](https://cursor.com/docs/cli/reference/configuration) | +| Engine | Isolation flags | Reference | +| ------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | --------------------------------------------------------------------------- | +| **codex** | Auth-only config overrides, isolated workspace, `exec --ignore-user-config --ignore-rules --skip-git-repo-check`, plus read-only sandbox | Codex CLI `exec --help` | +| **claude** | `--safe-mode --setting-sources user --strict-mcp-config --disallowedTools mcp__*`; auto-memory and filesystem/shell tools disabled; empty external workspace; WebSearch by default (`v2.1.169+`) | Claude Code [CLI reference](https://code.claude.com/docs/en/cli-reference) | +| **droid** | Fails closed: current CLI cannot disable both project instructions and all tools | Droid CLI `exec --help` and `--list-tools` | +| **copilot** | Fails closed: repository read tools also expose ignored files outside the reviewed bundle | GitHub Copilot CLI command reference | +| **pi** | `--no-approve --no-session --no-context-files --no-extensions --no-skills --no-prompt-templates --no-themes --no-tools` | Pi CLI `--help`; requires Pi `v0.79.0+` | +| **opencode** | Fails closed: project/global config isolation and private-network fetch denial are not both proven | OpenCode CLI contract | +| **cursor** | Fails closed: documented read permissions can target absolute host paths and no proven repository-only filesystem sandbox is exposed | Cursor CLI [permissions](https://cursor.com/docs/cli/reference/permissions) | -Codex `--ignore-user-config` skips config loading for the exec run. Autoreview reconstructs only the documented `cli_auth_credentials_store`, `forced_login_method`, and `forced_chatgpt_workspace_id` settings from `CODEX_HOME/config.toml`, keeping authentication and workspace restrictions usable without forwarding unrelated user configuration. The explicit repo trust override and zero project-doc budget keep reviewed-repo `AGENTS.md` and `.codex/` trust surfaces out of the review prompt. `--ignore-rules` skips user/project execpolicy rules. Claude `--safe-mode` disables project hooks, skills, plugins, MCP servers, and CLAUDE.md while preserving normal authentication, model selection, built-in tools, and permissions; managed settings policy can still apply. `--setting-sources user` avoids project/local settings from the reviewed checkout, and current Claude Code docs note the project-skill blocking behavior was fixed in `v2.1.69`. `--strict-mcp-config` and `--disallowedTools mcp__*` keep MCP unavailable to the review run. `--bare` is not used here because Claude's headless docs say it skips OAuth and keychain reads. Droid fails closed because its CLI cannot disable reviewed-repository `AGENTS.md` loading and all tools in the same run. Pi `--no-approve` ignores project-local files for one run; the helper requires Pi `v0.79.0+` plus help output that advertises every required isolation flag because older legacy binaries can ignore unknown flags. The current package is `@earendil-works/pi-coding-agent`; deprecated `@mariozechner/pi-coding-agent` `0.73.x` is intentionally rejected. Pi version/help probes and the review command run from neutral temporary directories, not the reviewed repo. Pi `--no-context-files` removes `AGENTS.md`/`CLAUDE.md`, the resource-disable flags keep `.pi` extensions, skills, prompts, and themes out of the run, `--no-session` avoids writing review sessions, and `--no-tools` prevents built-in read tools from escaping the repository through absolute paths. OpenCode starts from a neutral temporary directory, points at the reviewed repo with `--dir`, disables project config through `OPENCODE_DISABLE_PROJECT_CONFIG=1`, and injects `OPENCODE_CONFIG_CONTENT`; permissions default to deny, allow read/grep/glob, preserve OpenCode's `.env` ask rules, and gate `websearch`/`webfetch` with `--no-web-search`. The injected config also clears command/instruction/plugin arrays and disables write/edit/bash/task/skill/todowrite tools without changing user auth storage. Cursor's documented headless path is print mode with JSON output and workspace-relative project-resource discovery. Because the CLI exposes no per-run flag that disables every current and future project instruction surface, autoreview requires `--cursor-allow-workspace-instructions` (or its environment equivalent) for every Cursor run. Project-local Cursor/Claude hook settings, project MCP config, and global Cursor MCP config remain hard refusals because hooks execute host commands and MCP tools cannot be constrained to read-only review access. Cursor capability probes run from neutral temporary directories with the sanitized engine environment. Review runs set documented `CURSOR_CONFIG_DIR` to an ephemeral configuration that allows workspace reads while denying shell commands and relative or absolute writes. The helper sends review prompts to OpenCode and Cursor over stdin rather than argv and extracts final structured JSON from terminal result/text events. OpenCode and Cursor reject `--no-tools`; Cursor also rejects `--no-web-search` because the CLI does not expose a documented per-run web-search disable flag. +Codex `--ignore-user-config` skips config loading for the exec run. Autoreview reconstructs only the documented `cli_auth_credentials_store`, `forced_login_method`, and `forced_chatgpt_workspace_id` settings from `CODEX_HOME/config.toml`, keeping authentication usable without forwarding unrelated user configuration. Codex runs in an empty temporary workspace: the validated bundle is its sole repository input, ignored files and linked-worktree metadata remain unreadable, and the zero project-doc budget keeps workspace instructions out of the prompt. `--ignore-rules` skips user/project execpolicy rules. Claude `--safe-mode` disables project hooks, skills, plugins, MCP servers, and CLAUDE.md; autoreview supplies WebSearch by default, permits only explicitly domain-constrained WebFetch rules, and exposes no filesystem or shell tools. Pi runs from a neutral temporary directory with project resources disabled and `--no-tools`. Droid, Copilot, Cursor, and OpenCode fail closed because their current CLI contracts cannot isolate untrusted review input from host, project, or private-network trust surfaces. + +Codex uses a named permission profile that grants read access only to an empty temporary workspace. This is narrower than repository-root access, which would expose ignored credentials, and narrower than the legacy `read-only` sandbox, which permits reads across the host filesystem. ## Context Efficiency @@ -322,13 +337,13 @@ The smoke harness has thin shell wrappers over a shared Python implementation: On native Windows, invoke the extensionless Python helper through Python: ```powershell -python skills\autoreview\scripts\autoreview --help +python $AUTOREVIEW --help ``` and the smoke harness: ```powershell -skills\autoreview\scripts\test-review-harness.ps1 -Fixture benign -Engine codex +& $AUTOREVIEW_HARNESS -Fixture benign -Engine codex ``` The helper: @@ -338,21 +353,19 @@ The helper: - otherwise uses current PR base if `gh pr view` works - otherwise uses `origin/main` for non-main branches - does not fetch automatically during branch review; the selected base ref must already resolve locally -- recognizes `--engine droid` only to fail closed with an isolation error; runnable engines are `codex`, `claude`, `copilot`, `pi`, `opencode`, and `cursor`; default is `AUTOREVIEW_ENGINE` or `codex` -- resolves bare `git`, `gh`, reviewer, and PowerShell shell commands from absolute `PATH` entries only, never from the reviewed checkout; explicit relative `--*-bin` paths are resolved from the reviewed repository root +- recognizes `--engine droid`, `copilot`, `cursor`, and `opencode` only to fail closed with isolation errors; runnable engines are `codex`, `claude`, and `pi`; default is `AUTOREVIEW_ENGINE` or `codex` +- resolves bare `git`, `gh`, reviewer, and PowerShell shell commands from absolute `PATH` entries only, never from the reviewed checkout; explicit `--*-bin` paths are interpreted from the reviewed repository root when relative and accepted only when both the supplied path and resolved target stay outside the reviewed repository - use `--mode commit --commit ` for already-committed work, especially clean `main` after landing - should be left in `--mode auto` or forced to `--mode branch` for PR/branch work; do not force `--mode local` after committing - writes only to stdout unless `--output`, `--json-output`, or live streamed engine stderr is set -- supports `--dry-run`, `--parallel-tests`, `--parallel-tests-shell`, `--prompt`, repo-relative `--prompt-file`, repo-relative `--dataset`, `--no-tools`, `--no-web-search`, repeatable Codex-only `--codex-config key=value`, Codex-only `--codex-speed fast|flex|default`, and commit refs -- supports `--stream-engine-output` or `AUTOREVIEW_STREAM_ENGINE_OUTPUT=1` for live engine text while preserving structured validation; Codex, Claude, and Cursor hide tool/file event details, emit compact activity summaries, and report usage at turn completion +- supports `--dry-run`, `--parallel-tests`, `--parallel-tests-shell`, `--prompt`, repo-relative `--prompt-file`, repo-relative `--dataset`, `--no-tools`, `--no-web-search`, repeatable Codex-only safe model/response tuning with `--codex-config key=value`, Codex-only `--codex-speed fast|flex|default`, and commit refs +- supports `--stream-engine-output` or `AUTOREVIEW_STREAM_ENGINE_OUTPUT=1` for live engine text while preserving structured validation; Codex and Claude hide tool/file event details, emit compact activity summaries, and report usage at turn completion - supports opt-in review panels with `--panel` / `--reviewers`, plus per-engine `--model`, `--thinking`, and Claude `--fallback-model` -- uses built-in model defaults `codex=gpt-5.6-sol` and `claude=claude-fable-5`; honors `AUTOREVIEW_MODEL`, `AUTOREVIEW_THINKING`, `AUTOREVIEW_FALLBACK_MODEL`, and per-engine `AUTOREVIEW__MODEL` / `AUTOREVIEW__THINKING` environment overrides when CLI flags are omitted -- allows read-only tools and web search by default where the selected CLI supports them; forbids nested review in the prompt; Codex is run through `codex exec` with auth-only user settings, read-only sandbox, reviewed-repo instruction/config/rule isolation flags, and structured output -- runs Claude with `--safe-mode` (`v2.1.169+`), `--setting-sources user`, MCP disabled, explicit allowed tools, and `--fallback-model` when set, so reviewed-repo hooks/skills/MCP do not affect the review run while normal auth still works; managed settings policy can still apply -- refuses Droid reviews until the CLI exposes a complete project-instruction and tool-isolation contract +- uses built-in defaults `codex=gpt-5.6-sol` with `high` reasoning and an access-only `gpt-5.6-terra` retry, plus `claude=claude-fable-5`; honors `AUTOREVIEW_MODEL`, `AUTOREVIEW_THINKING`, `AUTOREVIEW_FALLBACK_MODEL`, and per-engine `AUTOREVIEW__MODEL` / `AUTOREVIEW__THINKING` environment overrides when CLI flags are omitted +- gives Codex the bundle in an empty workspace with web search available; Claude receives the bundle plus WebSearch by default and optional domain-constrained WebFetch, and Pi receives the bundle with no tools +- runs Claude with `--safe-mode` (`v2.1.169+`), `--setting-sources user`, MCP and auto-memory disabled, no filesystem/shell tools, an empty external workspace, and `--fallback-model` when set +- refuses Droid, Copilot, Cursor, and OpenCode reviews until their CLIs expose the required project, filesystem, and network isolation - runs Pi `v0.79.0+` from neutral temporary directories with `--no-approve`, `--no-session`, disabled Pi context/resource loading, and `--no-tools` because its built-in read tools are not repository-confined -- runs OpenCode with `opencode run --dir --pure --format json` from a neutral temporary directory, forwards `--model` and `--variant`, injects deny-by-default permissions, disables project config loading, and passes the review prompt over stdin -- runs Cursor only with mandatory trusted-workspace opt-in, uses `cursor-agent --print --mode ask --sandbox enabled --output-format json`, forwards `--model`, passes the review prompt over stdin, and always refuses project-local hooks/MCP - prints `review still running: elapsed=s pid=` to stderr at long-running intervals while waiting for the selected review engine, unless streamed output or compact Codex activity has been visible recently - prints `autoreview clean: no accepted/actionable findings reported` when the selected review command exits 0 - exits nonzero when accepted/actionable findings are present diff --git a/.agents/skills/autoreview/scripts/autoreview b/.agents/skills/autoreview/scripts/autoreview index 7c0fe421f493..069d50ff2386 100755 --- a/.agents/skills/autoreview/scripts/autoreview +++ b/.agents/skills/autoreview/scripts/autoreview @@ -9,20 +9,22 @@ import json import os import queue import re +import stat import subprocess import sys import tempfile import textwrap import threading import time -from pathlib import Path +import urllib.parse +from pathlib import Path, PurePosixPath from typing import Any, Callable ENGINES = ("codex", "claude", "droid", "copilot", "pi", "opencode", "cursor") ENGINE_ALIASES = {"cursor-agent": "cursor"} ENGINE_CHOICES = (*ENGINES, *ENGINE_ALIASES) -ALL_REVIEWERS = ("codex", "claude", "copilot", "pi", "opencode") +ALL_REVIEWERS = ("codex", "claude", "pi") SAFE_GIT_CONFIG_ARGS = ( "-c", "core.fsmonitor=false", @@ -57,8 +59,17 @@ SENSITIVE_PATH_PARTS = { ".gnupg", ".ssh", "private", - "secrets", } +TRACKED_SENSITIVE_PATH_PARTS = SENSITIVE_PATH_PARTS - { + "private", + ".docker", +} +TRACKED_CREDENTIAL_DIR_PATTERN = re.compile( + r"^(?:.*[._-])?" + r"(secret|secrets|credential|credentials|service[-_]?account|private[-_]?key|api[-_]?key)" + r"(?:[._-].*)?$", + re.IGNORECASE, +) SENSITIVE_NAME_PATTERNS = [ re.compile(r"(^|/)\.env($|[._/-])", re.IGNORECASE), re.compile(r"(^|/)(id_rsa|id_dsa|id_ecdsa|id_ed25519)(\.pub)?$", re.IGNORECASE), @@ -68,10 +79,76 @@ SENSITIVE_NAME_PATTERNS = [ re.IGNORECASE, ), ] -SECRET_VALUE_PATTERNS = [ - re.compile(r"-----BEGIN (?:RSA |DSA |EC |OPENSSH |PGP )?PRIVATE KEY-----"), +TRACKED_SENSITIVE_NAME_PATTERNS = [ re.compile( - r"(?i)(api[_-]?key|token|secret|password)\s*[:=]\s*(?:[\"'][A-Za-z0-9_./+=-]{12,}[\"']|[A-Za-z0-9_+=/-]{20,})" + r"(^|/)\.env(?:$|/|[._-](?!(?:example|sample|template)$)[^/]*)", + re.IGNORECASE, + ), + re.compile(r"(^|/)(id_rsa|id_dsa|id_ecdsa|id_ed25519)(\.pub)?$", re.IGNORECASE), + re.compile(r"\.(pem|p12|pfx|key)$", re.IGNORECASE), + re.compile( + r"(^|/)(secret|secrets|credential|credentials|service[-_]?account|private[-_]?key|api[-_]?key|token|tokens)$", + re.IGNORECASE, + ), + re.compile( + r"(^|/)(?:[^/]*[._-])?" + r"(secret|secrets|credential|credentials|service[-_]?account|private[-_]?key|api[-_]?key|token|tokens)" + r"(?:[._-][^/]*)?\.(json|ya?ml|toml|ini|conf|config|txt|csv)$", + re.IGNORECASE, + ), +] +TRACKED_TOKEN_CREDENTIAL_STEMS = { + "access", + "account", + "auth", + "cache", + "credentials", + "credential", + "device", + "id", + "prod", + "production", + "refresh", + "secret", + "secrets", + "session", + "store", + "token", + "tokens", + "user", +} +TRACKED_TOKEN_CREDENTIAL_EXTENSIONS = { + "", + ".conf", + ".config", + ".csv", + ".dat", + ".db", + ".enc", + ".ini", + ".json", + ".jsonl", + ".jwt", + ".sqlite", + ".sqlite3", + ".txt", + ".toml", + ".yaml", + ".yml", +} +SECRET_ASSIGNMENT_PATTERN = re.compile( + r"(?i)(?:" + r"[\"'](?:api[_-]?key|aws[_-]?secret[_-]?access[_-]?key|client[_-]?secret|refresh[_-]?token|access[_-]?token|auth[_-]?token|id[_-]?token|token|secret|password)[\"']" + r"|(?:api[_-]?key|aws[_-]?secret[_-]?access[_-]?key|client[_-]?secret|refresh[_-]?token|access[_-]?token|auth[_-]?token|id[_-]?token|token|secret|password)" + r")\s*[:=]\s*" + r"(?:\"(?P[^\"\r\n]{12,})\"|" + r"'(?P[^'\r\n]{12,})'|" + r"(?P[A-Za-z0-9_./+=:@#$%&*!?-]{20,}))" +) +SECRET_VALUE_PATTERNS = [ + re.compile( + r"-----BEGIN (?:RSA |DSA |EC |OPENSSH |PGP |ENCRYPTED )?" + r"PRIVATE KEY(?: BLOCK)?-----" ), re.compile(r"(?i)bearer\s+[A-Za-z0-9._-]{20,}"), re.compile(r"\b(?:sk|rk|pk|org|proj)-[A-Za-z0-9_-]{20,}\b"), @@ -83,15 +160,114 @@ SECRET_VALUE_PATTERNS = [ re.compile(r"\b(?:A3T|AKIA|ASIA)[A-Z0-9]{16}\b"), re.compile(r"\bAIza[0-9A-Za-z_-]{35}\b"), re.compile(r"\bya29\.[0-9A-Za-z_-]{20,}\b"), + re.compile(r"\beyJ[A-Za-z0-9_-]{7,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\b"), ] MAX_BUNDLE_TEXT_BYTES = 180_000 +MAX_REVIEW_PROMPT_BYTES = 512_000 +SECRET_PLACEHOLDER_VALUES = { + "changeme", + "dummy", + "example", + "fake", + "gateway-token", + "not-a-real", + "placeholder", + "redacted", + "sample", + "secret-token", + "test-auth-token", + "test-token-placeholder", + "token-oversized", + "clawrouter-e2e-secret", + "very-long-browser-token-0123456789", +} +QUOTED_SECRET_REFERENCE_PATTERNS = ( + re.compile(r"^\$[A-Za-z_][A-Za-z0-9_]*$"), + re.compile(r"^\$env:[A-Za-z_][A-Za-z0-9_]*$", re.IGNORECASE), + re.compile(r"^\$\{[A-Za-z_][A-Za-z0-9_]*\}$"), + re.compile(r"^\$\{\{\s*[A-Za-z_][A-Za-z0-9_.-]*\s*\}\}$"), + re.compile(r"^\{\{\s*[A-Za-z_][A-Za-z0-9_.-]*\s*\}\}$"), + re.compile(r"^op://[^\r\n]+$"), +) +UNQUOTED_SECRET_REFERENCE_PATTERNS = ( + *QUOTED_SECRET_REFERENCE_PATTERNS, + re.compile( + r"^(?:process\.env|os\.environ|env|cfg|config|params|payload|provider|" + r"request|response|result|account|client|auth|auth_response|oauth_response|" + r"token_response|api_response|authentication|credentials|settings|self|this)" + r"(?:[.\[].*)$" + ), +) DEFAULT_ENGINE_PATHS = ("/usr/local/bin", "/usr/bin", "/bin") +MULTI_PROVIDER_CREDENTIAL_SUFFIXES = ( + "_ACCESS_TOKEN", + "_API_KEY", + "_API_TOKEN", + "_AUTH_TOKEN", + "_TOKEN", +) +MULTI_PROVIDER_ENV_KEYS = { + "AWS_CONTAINER_AUTHORIZATION_TOKEN", + "AWS_CONTAINER_CREDENTIALS_FULL_URI", + "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI", + "AWS_BEDROCK_FORCE_HTTP1", + "AWS_BEDROCK_SKIP_AUTH", + "AWS_ENDPOINT_URL_BEDROCK_RUNTIME", + "AWS_ROLE_ARN", + "AWS_ROLE_SESSION_NAME", + "AZURE_COGNITIVE_SERVICES_RESOURCE_NAME", + "AZURE_OPENAI_API_VERSION", + "AZURE_OPENAI_BASE_URL", + "AZURE_OPENAI_DEPLOYMENT_NAME_MAP", + "AZURE_OPENAI_RESOURCE_NAME", + "AZURE_RESOURCE_NAME", + "CLOUDFLARE_ACCOUNT_ID", + "CLOUDFLARE_GATEWAY_ID", + "GCLOUD_PROJECT", + "GOOGLE_CLOUD_LOCATION", + "GOOGLE_CLOUD_PROJECT", + "HF_TOKEN", + "SNOWFLAKE_ACCOUNT", + "VERTEXAI_LOCATION", + "VERTEXAI_PROJECT", +} +CLAUDE_CLOUD_CREDENTIAL_ENV_KEYS = { + "AWS_CONTAINER_AUTHORIZATION_TOKEN", + "AWS_CONTAINER_CREDENTIALS_FULL_URI", + "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI", + "AWS_ROLE_ARN", + "AWS_ROLE_SESSION_NAME", + "AZURE_CLIENT_ID", + "AZURE_CLIENT_SECRET", + "AZURE_TENANT_ID", + "GCLOUD_PROJECT", + "GOOGLE_CLOUD_PROJECT", +} +CODEX_TRUST_PATH_ENV_KEYS = { + "CODEX_CA_CERTIFICATE", + "SSL_CERT_DIR", + "SSL_CERT_FILE", +} +PROVIDER_CREDENTIAL_PATH_ENV_KEYS = { + "AWS_CONFIG_FILE", + "AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE", + "AWS_SHARED_CREDENTIALS_FILE", + "AWS_WEB_IDENTITY_TOKEN_FILE", + "GOOGLE_APPLICATION_CREDENTIALS", + "NODE_EXTRA_CA_CERTS", + "SSL_CERT_DIR", + "SSL_CERT_FILE", +} DEFAULT_MODEL_BY_ENGINE = { "codex": "gpt-5.6-sol", "claude": "claude-fable-5", } +DEFAULT_CODEX_ACCESS_FALLBACK_MODEL = "gpt-5.6-terra" +DEFAULT_THINKING_BY_ENGINE = { + "codex": "high", +} THINKING_LEVELS_BY_ENGINE = { - "codex": {"none", "minimal", "low", "medium", "high", "xhigh"}, + "codex": {"none", "minimal", "low", "medium", "high", "xhigh", "max"}, "claude": {"low", "medium", "high", "xhigh", "max"}, "droid": {"off", "none", "low", "medium", "high", "xhigh", "max"}, "copilot": set(), @@ -170,6 +346,7 @@ def run( input_text: str | None = None, check: bool = True, env: dict[str, str] | None = None, + text_errors: str = SUBPROCESS_TEXT_ERRORS, ) -> subprocess.CompletedProcess[str]: result = subprocess.run( args, @@ -177,7 +354,7 @@ def run( input=input_text, text=True, encoding=SUBPROCESS_TEXT_ENCODING, - errors=SUBPROCESS_TEXT_ERRORS, + errors=text_errors, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env, @@ -217,6 +394,44 @@ def safe_git_env(repo: Path) -> dict[str, str]: return env +def global_excludes_file(repo: Path) -> Path | None: + env = safe_git_env(repo) + env.pop("GIT_CONFIG_GLOBAL", None) + home = Path(env["HOME"]).expanduser() + if not external_env_path(repo, str(home)): + return None + result = run( + [ + resolve_command("git", repo), + "--no-optional-locks", + *SAFE_GIT_CONFIG_ARGS, + "config", + "--global", + "--path", + "--get", + "core.excludesFile", + ], + repo, + check=False, + env=env, + ) + if result.returncode != 0: + return None + raw_path = result.stdout.strip() + if not raw_path: + return None + candidate = Path(raw_path).expanduser() + if not candidate.is_absolute(): + candidate = home / candidate + try: + resolved = candidate.resolve(strict=True) + except OSError: + return None + if is_within(resolved, repo.resolve()) or not resolved.is_file(): + return None + return resolved + + def safe_engine_path(repo: Path, extra_paths: list[Path] | None = None) -> str: entries: list[str] = [] resolved_repo = repo.resolve() @@ -245,37 +460,294 @@ def safe_engine_path(repo: Path, extra_paths: list[Path] | None = None) -> str: return os.pathsep.join(entries) +def codex_tool_git_env() -> dict[str, str]: + env = {"GIT_CONFIG_COUNT": str(len(ENGINE_GIT_CONFIG_OVERRIDES))} + for index, (key, value) in enumerate(ENGINE_GIT_CONFIG_OVERRIDES): + env[f"GIT_CONFIG_KEY_{index}"] = key + env[f"GIT_CONFIG_VALUE_{index}"] = value + return env + + +def external_env_path(repo: Path, value: str) -> bool: + try: + resolved = Path(value).expanduser().resolve() + except OSError: + return False + return not is_within(resolved, repo.resolve()) + + +def external_env_path_value(repo: Path, key: str, value: str) -> bool: + return normalize_external_env_path_value(repo, key, value) is not None + + +def normalize_external_env_path_value( + repo: Path, + key: str, + value: str, +) -> str | None: + values = value.split(os.pathsep) if key == "SSL_CERT_DIR" else [value] + normalized: list[str] = [] + for item in values: + if not item: + return None + try: + resolved = Path(item).expanduser().resolve() + except OSError: + return None + if is_within(resolved, repo.resolve()): + return None + normalized.append(str(resolved)) + return os.pathsep.join(normalized) if normalized else None + + +def safe_dbus_session_address(repo: Path, value: str) -> bool: + match = re.fullmatch( + r"unix:path=(?P[^,;%]+)(?:,guid=[0-9a-fA-F]+)?", + value, + ) + if not match: + return False + path = match.group("path") + return Path(path).is_absolute() and external_env_path(repo, path) + + +def safe_temp_root(repo: Path) -> Path: + try: + root = Path(tempfile.gettempdir()).resolve(strict=True) + except OSError as exc: + raise SystemExit(f"unable to resolve temporary directory: {exc}") from exc + if is_within(root, repo.resolve()): + raise SystemExit( + "temporary directory must be outside the reviewed repository; " + "unset or relocate TMPDIR/TMP/TEMP" + ) + return root + + +def safe_proxy_url(value: str) -> bool: + try: + candidate = value if "://" in value else f"http://{value}" + parsed = urllib.parse.urlsplit(candidate) + _ = parsed.port + except ValueError: + return False + return ( + parsed.scheme.lower() + in {"http", "https", "socks", "socks4", "socks4a", "socks5", "socks5h"} + and bool(parsed.hostname) + and parsed.username is None + and parsed.password is None + and parsed.path in {"", "/"} + and not parsed.query + and not parsed.fragment + ) + + def safe_engine_env( repo: Path, extra_paths: list[Path] | None = None, extra: dict[str, str] | None = None, + *, + engine: str | None = None, ) -> dict[str, str]: - blocked_exact = { - "BASH_ENV", - "ENV", - "GIT_CONFIG", - "GIT_CONFIG_GLOBAL", - "GIT_CONFIG_NOSYSTEM", - "GIT_CONFIG_SYSTEM", - "GIT_OPTIONAL_LOCKS", - "GIT_TERMINAL_PROMPT", - "LD_PRELOAD", - "NODE_OPTIONS", - "PYTHONHOME", - "PYTHONPATH", + common_allowed_exact = { + "ALL_PROXY", + "COMSPEC", + "DISABLE_AUTOUPDATER", + "DISABLE_ERROR_REPORTING", + "DISABLE_TELEMETRY", + "DO_NOT_TRACK", + "HTTP_PROXY", + "HTTPS_PROXY", + "LANG", + "LC_ALL", + "LOGNAME", + "NO_PROXY", + "PATHEXT", + "SHELL", + "SYSTEMROOT", + "TEMP", + "TMP", + "TMPDIR", + "USER", + "WINDIR", + "all_proxy", + "http_proxy", + "https_proxy", + "no_proxy", } - blocked_prefixes = ("GIT_", "DYLD_") + codex_allowed_exact = { + "AZURE_OPENAI_API_KEY", + "AZURE_OPENAI_ENDPOINT", + "CODEX_API_KEY", + "OPENAI_API_KEY", + "OPENAI_BASE_URL", + "OPENAI_ORGANIZATION", + "OPENAI_PROJECT", + } + claude_allowed_exact = { + "ANTHROPIC_API_KEY", + "ANTHROPIC_AUTH_TOKEN", + "ANTHROPIC_AWS_API_KEY", + "ANTHROPIC_AWS_BASE_URL", + "ANTHROPIC_AWS_WORKSPACE_ID", + "ANTHROPIC_BASE_URL", + "ANTHROPIC_BEDROCK_BASE_URL", + "ANTHROPIC_BEDROCK_MANTLE_BASE_URL", + "ANTHROPIC_BEDROCK_SERVICE_TIER", + "ANTHROPIC_CUSTOM_HEADERS", + "ANTHROPIC_FOUNDRY_API_KEY", + "ANTHROPIC_FOUNDRY_AUTH_TOKEN", + "ANTHROPIC_FOUNDRY_BASE_URL", + "ANTHROPIC_FOUNDRY_RESOURCE", + "ANTHROPIC_SMALL_FAST_MODEL_AWS_REGION", + "ANTHROPIC_VERTEX_BASE_URL", + "ANTHROPIC_VERTEX_PROJECT_ID", + "ANTHROPIC_WORKSPACE_ID", + "AWS_ACCESS_KEY_ID", + "AWS_BEARER_TOKEN_BEDROCK", + "AWS_DEFAULT_REGION", + "AWS_PROFILE", + "AWS_REGION", + "AWS_SECRET_ACCESS_KEY", + "AWS_SESSION_TOKEN", + "CLAUDE_CODE_API_KEY_HELPER_TTL_MS", + "CLAUDE_CODE_CERT_STORE", + "CLAUDE_CODE_CLIENT_CERT", + "CLAUDE_CODE_CLIENT_KEY", + "CLAUDE_CODE_CLIENT_KEY_PASSPHRASE", + "CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC", + "CLAUDE_CODE_OAUTH_REFRESH_TOKEN", + "CLAUDE_CODE_OAUTH_SCOPES", + "CLAUDE_CODE_OAUTH_TOKEN", + "CLAUDE_CODE_PROVIDER_MANAGED_BY_HOST", + "CLAUDE_CODE_SKIP_ANTHROPIC_AWS_AUTH", + "CLAUDE_CODE_SKIP_BEDROCK_AUTH", + "CLAUDE_CODE_SKIP_FOUNDRY_AUTH", + "CLAUDE_CODE_SKIP_MANTLE_AUTH", + "CLAUDE_CODE_SKIP_VERTEX_AUTH", + "CLAUDE_CODE_USE_ANTHROPIC_AWS", + "CLAUDE_CODE_USE_BEDROCK", + "CLAUDE_CODE_USE_FOUNDRY", + "CLAUDE_CODE_USE_MANTLE", + "CLAUDE_CODE_USE_VERTEX", + "CLOUD_ML_REGION", + } | CLAUDE_CLOUD_CREDENTIAL_ENV_KEYS + multi_provider_allowed_exact = { + "ANTHROPIC_AWS_BASE_URL", + "ANTHROPIC_AWS_WORKSPACE_ID", + "ANTHROPIC_BASE_URL", + "ANTHROPIC_BEDROCK_BASE_URL", + "ANTHROPIC_BEDROCK_MANTLE_BASE_URL", + "ANTHROPIC_BEDROCK_SERVICE_TIER", + "ANTHROPIC_CUSTOM_HEADERS", + "ANTHROPIC_FOUNDRY_BASE_URL", + "ANTHROPIC_FOUNDRY_RESOURCE", + "ANTHROPIC_SMALL_FAST_MODEL_AWS_REGION", + "ANTHROPIC_VERTEX_BASE_URL", + "ANTHROPIC_VERTEX_PROJECT_ID", + "ANTHROPIC_WORKSPACE_ID", + "AWS_ACCESS_KEY_ID", + "AWS_BEARER_TOKEN_BEDROCK", + "AWS_DEFAULT_REGION", + "AWS_PROFILE", + "AWS_REGION", + "AWS_SECRET_ACCESS_KEY", + "AWS_SESSION_TOKEN", + "AZURE_OPENAI_ENDPOINT", + "CLOUD_ML_REGION", + "COPILOT_GITHUB_TOKEN", + "GITHUB_TOKEN", + "GH_TOKEN", + "OPENAI_BASE_URL", + "OPENAI_ORGANIZATION", + "OPENAI_PROJECT", + } | MULTI_PROVIDER_ENV_KEYS + engine_allowed_exact = { + "claude": claude_allowed_exact, + "codex": codex_allowed_exact, + "opencode": multi_provider_allowed_exact, + "pi": multi_provider_allowed_exact, + }.get(engine or "", set()) + allowed_prefixes = ("AUTOREVIEW_FAKE_",) + allow_multi_provider_credentials = engine in {"opencode", "pi"} env = { key: value for key, value in os.environ.items() - if key not in blocked_exact and not any(key.startswith(prefix) for prefix in blocked_prefixes) + if ( + key in common_allowed_exact + or key in engine_allowed_exact + or any(key.startswith(prefix) for prefix in allowed_prefixes) + or ( + allow_multi_provider_credentials + and ( + key in MULTI_PROVIDER_ENV_KEYS + or key.endswith(MULTI_PROVIDER_CREDENTIAL_SUFFIXES) + ) + ) + ) } + for key in ( + "ALL_PROXY", + "HTTP_PROXY", + "HTTPS_PROXY", + "all_proxy", + "http_proxy", + "https_proxy", + ): + value = env.get(key) + if value and not safe_proxy_url(value): + raise SystemExit( + f"unsafe credentialed or malformed proxy URL in {key}; " + "configure a credential-free proxy URL before running autoreview" + ) env["PATH"] = safe_engine_path(repo, extra_paths) - env["GIT_CONFIG_COUNT"] = str(len(ENGINE_GIT_CONFIG_OVERRIDES)) - for index, (key, value) in enumerate(ENGINE_GIT_CONFIG_OVERRIDES): - env[f"GIT_CONFIG_KEY_{index}"] = key - env[f"GIT_CONFIG_VALUE_{index}"] = value + for key in ("HOME", "USERPROFILE"): + value = os.environ.get(key) + if value and external_env_path(repo, value): + env[key] = value + engine_config_paths = { + "claude": ("CLAUDE_CONFIG_DIR",), + "codex": ("CODEX_HOME",), + "pi": ("PI_CODING_AGENT_DIR",), + } + for key in engine_config_paths.get(engine or "", ()): + value = os.environ.get(key) + if value and external_env_path(repo, value): + env[key] = value + if engine == "codex": + dbus_address = os.environ.get("DBUS_SESSION_BUS_ADDRESS") + if dbus_address and safe_dbus_session_address(repo, dbus_address): + env["DBUS_SESSION_BUS_ADDRESS"] = dbus_address + xdg_runtime_dir = os.environ.get("XDG_RUNTIME_DIR") + if xdg_runtime_dir and external_env_path(repo, xdg_runtime_dir): + env["XDG_RUNTIME_DIR"] = xdg_runtime_dir + for key in CODEX_TRUST_PATH_ENV_KEYS: + value = os.environ.get(key) + normalized = ( + normalize_external_env_path_value(repo, key, value) + if value + else None + ) + if normalized: + env[key] = normalized + if engine in {"claude", "opencode", "pi"}: + for key in PROVIDER_CREDENTIAL_PATH_ENV_KEYS: + value = os.environ.get(key) + normalized = ( + normalize_external_env_path_value(repo, key, value) + if value + else None + ) + if normalized: + env[key] = normalized + if engine == "opencode" and (xdg_data_home := os.environ.get("XDG_DATA_HOME")): + if external_env_path(repo, xdg_data_home): + env["XDG_DATA_HOME"] = xdg_data_home + env.update(codex_tool_git_env()) env.update(extra or {}) + if engine == "claude": + env["CLAUDE_CODE_DISABLE_AUTO_MEMORY"] = "1" return env @@ -399,21 +871,16 @@ def emit_heartbeat( def opencode_review_config(web_search: bool = True) -> dict[str, Any]: permission: dict[str, Any] = { "*": "deny", - "read": { - "*": "allow", - "*.env": "ask", - "*.env.*": "ask", - "*.env.example": "allow", - }, - "grep": "allow", - "glob": "allow", + "read": "deny", + "grep": "deny", + "glob": "deny", } if web_search: permission["websearch"] = "allow" - permission["webfetch"] = "allow" else: permission["websearch"] = "deny" - permission["webfetch"] = "deny" + # Generic fetches can reach loopback, private, link-local, or metadata endpoints. + permission["webfetch"] = "deny" return { "$schema": "https://opencode.ai/config.json", "autoupdate": False, @@ -434,13 +901,16 @@ def opencode_review_config(web_search: bool = True) -> dict[str, Any]: def opencode_review_env(web_search: bool = True) -> dict[str, str]: - return { + env = { "OPENCODE_DISABLE_PROJECT_CONFIG": "1", "OPENCODE_CONFIG_CONTENT": json.dumps(opencode_review_config(web_search), separators=(",", ":")), "OPENCODE_DISABLE_AUTOUPDATE": "1", "OPENCODE_DISABLE_AUTOCOMPACT": "1", "OPENCODE_DISABLE_MODELS_FETCH": "1", } + if web_search and (enable_exa := os.environ.get("OPENCODE_ENABLE_EXA")): + env["OPENCODE_ENABLE_EXA"] = enable_exa + return env def run_with_heartbeat( @@ -575,12 +1045,19 @@ def run_with_stream( def git(repo: Path, *args: str, check: bool = True) -> str: - return run( - [resolve_command("git", repo), "--no-optional-locks", *SAFE_GIT_CONFIG_ARGS, *args], - repo, - check=check, - env=safe_git_env(repo), - ).stdout + try: + return run( + [resolve_command("git", repo), "--no-optional-locks", *SAFE_GIT_CONFIG_ARGS, *args], + repo, + check=check, + env=safe_git_env(repo), + text_errors="strict", + ).stdout + except UnicodeDecodeError as exc: + raise SystemExit( + "refusing non-UTF-8 Git output because paths and diff content " + "cannot be validated without loss" + ) from exc def git_path_list(repo: Path, *args: str, check: bool = True) -> list[str]: @@ -593,15 +1070,18 @@ def repo_root() -> Path: git_bin = find_command("git", unsafe_root) if not git_bin: raise SystemExit("git executable not found. Install Git or add it to PATH.") - result = subprocess.run( - [git_bin, "--no-optional-locks", *SAFE_GIT_CONFIG_ARGS, "rev-parse", "--show-toplevel"], - text=True, - encoding=SUBPROCESS_TEXT_ENCODING, - errors=SUBPROCESS_TEXT_ERRORS, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - env=safe_git_env(unsafe_root), - ) + try: + result = subprocess.run( + [git_bin, "--no-optional-locks", *SAFE_GIT_CONFIG_ARGS, "rev-parse", "--show-toplevel"], + text=True, + encoding=SUBPROCESS_TEXT_ENCODING, + errors="strict", + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env=safe_git_env(unsafe_root), + ) + except UnicodeDecodeError as exc: + raise SystemExit("repository root is not valid UTF-8") from exc if result.returncode != 0: raise SystemExit("autoreview must run inside a git repository") return Path(result.stdout.strip()).resolve() @@ -657,7 +1137,12 @@ def find_command(name: str, repo: Path) -> str | None: command = Path(name) if has_directory_component(name, command): base = command if command.is_absolute() else repo / command - return first_executable_candidate(base) + if is_within( + Path(os.path.abspath(base)), + Path(os.path.abspath(repo)), + ): + return None + return first_executable_candidate(base, reject_root=repo.resolve()) for part in os.environ.get("PATH", "").split(os.pathsep): if not part or part == ".": continue @@ -696,13 +1181,17 @@ def first_executable_candidate(path: Path, *, reject_root: Path | None = None) - candidates = [path] for candidate in candidates: if candidate.is_file() and os.access(candidate, os.X_OK): - if reject_root is not None: - try: - if is_within(candidate.resolve(), reject_root): - continue - except OSError: - continue - return str(candidate) + try: + lexical_candidate = Path(os.path.abspath(candidate)) + resolved_candidate = candidate.resolve(strict=True) + except OSError: + continue + if reject_root is not None and ( + is_within(lexical_candidate, reject_root) + or is_within(resolved_candidate, reject_root) + ): + continue + return str(lexical_candidate) return None @@ -730,11 +1219,10 @@ def bounded(text: str, limit: int = 180_000) -> str: def ensure_reviewer_input_complete(reviewer: argparse.Namespace, input_truncated: bool) -> None: - can_recover_full_diff = reviewer.tools and reviewer.engine == "codex" - if input_truncated and not can_recover_full_diff: + if input_truncated: raise SystemExit( f"{reviewer.engine} engine refused truncated review input because it cannot recover omitted diff hunks; " - "reduce the change/input size or choose codex with tools enabled" + "reduce the change/input size" ) @@ -746,11 +1234,44 @@ def bounded_field(text: str, limit: int) -> str: def read_prefix(path: Path, limit: int) -> tuple[bytes, bool]: + descriptor: int | None = None try: - with path.open("rb") as handle: - data = handle.read(limit + 1) + before = path.stat(follow_symlinks=False) + if not stat.S_ISREG(before.st_mode): + raise OSError("not a regular file") + flags = ( + os.O_RDONLY + | getattr(os, "O_BINARY", 0) + | getattr(os, "O_CLOEXEC", 0) + | getattr(os, "O_NOFOLLOW", 0) + ) + descriptor = os.open(path, flags) + opened = os.fstat(descriptor) + if ( + not stat.S_ISREG(opened.st_mode) + or (before.st_dev, before.st_ino) != (opened.st_dev, opened.st_ino) + ): + raise OSError("file changed while opening") + chunks: list[bytes] = [] + remaining = limit + 1 + while remaining: + chunk = os.read(descriptor, remaining) + if not chunk: + break + chunks.append(chunk) + remaining -= len(chunk) + after = os.fstat(descriptor) + if ( + (opened.st_dev, opened.st_ino, opened.st_size, opened.st_mtime_ns) + != (after.st_dev, after.st_ino, after.st_size, after.st_mtime_ns) + ): + raise OSError("file changed while reading") + data = b"".join(chunks) except OSError as exc: raise SystemExit(f"unreadable file: {path}: {exc}") from exc + finally: + if descriptor is not None: + os.close(descriptor) return data[:limit], len(data) > limit @@ -758,10 +1279,13 @@ def read_text_with_status(path: Path, limit: int = MAX_BUNDLE_TEXT_BYTES) -> tup try: data, truncated = read_prefix(path, limit) except SystemExit as exc: - return f"[unreadable: {exc}]", False + return f"[unreadable: {exc}]", True if b"\0" in data: - return "[binary file omitted]", False - text = data.decode("utf-8", errors="replace") + return "[binary file omitted]", True + try: + text = data.decode("utf-8") + except UnicodeDecodeError: + return "[non-UTF-8 file omitted]", True if len(text) > limit: text = text[:limit] truncated = True @@ -793,7 +1317,31 @@ def raw_repo_path_has_symlink_component(repo: Path, rel_path: Path) -> bool: def secret_text_risk(text: str) -> bool: - return any(pattern.search(text) for pattern in SECRET_VALUE_PATTERNS) + if any(pattern.search(text) for pattern in SECRET_VALUE_PATTERNS): + return True + for match in SECRET_ASSIGNMENT_PATTERN.finditer(text): + quoted = match.group("bare_value") is None + value = ( + match.group("double_value") + or match.group("single_value") + or match.group("bare_value") + ) + if value is None: + continue + if value.lower() in SECRET_PLACEHOLDER_VALUES: + continue + reference_patterns = ( + QUOTED_SECRET_REFERENCE_PATTERNS + if quoted + else UNQUOTED_SECRET_REFERENCE_PATTERNS + ) + if any(pattern.fullmatch(value) for pattern in reference_patterns): + continue + call_target = re.fullmatch(r"[A-Za-z_][A-Za-z0-9_.]*", value) + if not quoted and call_target and text[match.end() :].startswith("("): + continue + return True + return False def require_no_secret_values(label: str, text: str) -> None: @@ -804,6 +1352,205 @@ def require_no_secret_values(label: str, text: str) -> None: ) +def unified_diff_contents(patch: str) -> tuple[str, str]: + old_content: list[str] = [] + new_content: list[str] = [] + in_hunk = False + prefix_columns = 1 + for line in patch.splitlines(): + hunk_header = re.match(r"^(@{2,})", line) + if hunk_header: + old_content.append(";") + new_content.append(";") + in_hunk = True + prefix_columns = len(hunk_header.group(1)) - 1 + continue + if line.startswith("diff --"): + old_content.append(";") + new_content.append(";") + in_hunk = False + continue + prefix = line[:prefix_columns] + if in_hunk and len(prefix) == prefix_columns and set(prefix) <= {"+", "-", " "}: + content = line[prefix_columns:] + if set(prefix) == {" "}: + old_content.append(content) + new_content.append(content) + elif "+" in prefix and "-" not in prefix: + new_content.append(content) + elif "-" in prefix and "+" not in prefix: + old_content.append(content) + else: + old_content.append(content) + new_content.append(content) + return "\n".join(old_content), "\n".join(new_content) + + +def sensitive_repo_path_risk(rel: str) -> str | None: + normalized = rel.replace(os.sep, "/") + path = Path(normalized) + credential_directory = any( + TRACKED_CREDENTIAL_DIR_PATTERN.fullmatch(part) + for part in path.parts[:-1] + ) + if ( + path_has_sensitive_part(normalized) + or credential_directory + or credential_store_path(normalized) + or token_credential_store_path(normalized) + ): + return "sensitive path" + if any(pattern.search(normalized) for pattern in SENSITIVE_NAME_PATTERNS): + return "sensitive filename" + return None + + +def token_credential_store_path(normalized: str) -> bool: + path = Path(normalized) + parts = {part.lower() for part in path.parts} + return ( + bool(parts & {"token", "tokens"}) + and path.stem.lower() in TRACKED_TOKEN_CREDENTIAL_STEMS + and path.suffix.lower() in TRACKED_TOKEN_CREDENTIAL_EXTENSIONS + ) + + +def credential_store_path(normalized: str) -> bool: + path = Path(normalized) + credential_directory = any( + TRACKED_CREDENTIAL_DIR_PATTERN.fullmatch(part) + for part in path.parts[:-1] + ) + credential_data_file = path.suffix.lower() not in { + ".c", + ".cc", + ".cpp", + ".cs", + ".go", + ".h", + ".hpp", + ".java", + ".js", + ".jsx", + ".kt", + ".mjs", + ".php", + ".py", + ".rb", + ".rs", + ".sh", + ".swift", + ".ts", + ".tsx", + ".vue", + } + return credential_directory and credential_data_file and not skill_instruction_path(path) + + +def skill_instruction_path(path: Path) -> bool: + parts = tuple(part.lower() for part in path.parts) + skill_root = parts[:1] == ("skills",) or any( + parts[index : index + 2] in {(".agents", "skills"), (".claude", "skills")} + for index in range(len(parts) - 1) + ) + return skill_root and path.name.lower() in {"agents.md", "claude.md", "skill.md"} + + +def tracked_sensitive_repo_path_risk(rel: str) -> str | None: + normalized = rel.replace(os.sep, "/") + parts = {part.lower() for part in Path(normalized).parts} + if ( + "/.config/gcloud/" in f"/{normalized.lower()}/" + or f"/{normalized.lower()}".endswith("/.docker/config.json") + or parts & TRACKED_SENSITIVE_PATH_PARTS + or credential_store_path(normalized) + or token_credential_store_path(normalized) + ): + return "sensitive path" + if any(pattern.search(normalized) for pattern in TRACKED_SENSITIVE_NAME_PATTERNS): + return "sensitive filename" + return None + + +def validate_review_patch( + label: str, + paths: list[str], + patch: str, + limit: int = MAX_BUNDLE_TEXT_BYTES, +) -> str: + blocked = [ + f"{rel} ({risk})" + for rel in paths + if (risk := tracked_sensitive_repo_path_risk(rel)) is not None + ] + if blocked: + details = "\n".join(f"- {item}" for item in blocked[:20]) + more = f"\n... {len(blocked) - 20} more" if len(blocked) > 20 else "" + raise SystemExit( + f"refusing to include tracked sensitive paths in {label}:\n" + f"{details}{more}" + ) + require_no_secret_values(label, patch) + for content in unified_diff_contents(patch): + require_no_secret_values(label, content) + patch_bytes = len(patch.encode("utf-8")) + if patch_bytes > limit: + raise SystemExit( + f"{label} is too large to review safely " + f"({patch_bytes} bytes; limit {limit}); split the change into smaller review targets" + ) + return patch + + +def require_no_binary_diff(label: str, numstat: str) -> None: + binary_paths: list[str] = [] + for record in numstat.split("\0"): + if not record: + continue + fields = record.split("\t", 2) + if len(fields) == 3 and fields[0] == "-" and fields[1] == "-": + binary_paths.append(fields[2]) + if binary_paths: + details = "\n".join(f"- {path}" for path in binary_paths[:20]) + more = f"\n... {len(binary_paths) - 20} more" if len(binary_paths) > 20 else "" + raise SystemExit( + f"refusing binary changes in {label} because their contents cannot be reviewed:\n" + f"{details}{more}" + ) + + +def require_no_gitlink_diff(label: str, raw_diff: str) -> None: + records = raw_diff.split("\0") + gitlink_paths: list[str] = [] + for index, record in enumerate(records): + if not record.startswith(":"): + continue + fields = record.split() + if len(fields) < 5: + continue + modes: list[str] = [] + for field_index, field in enumerate(fields): + candidate = field.lstrip(":") if field_index == 0 else field + if not re.fullmatch(r"[0-7]{6}", candidate): + break + modes.append(candidate) + if "160000" not in modes: + continue + path = records[index + 1] if index + 1 < len(records) else "" + gitlink_paths.append(path or "") + if gitlink_paths: + details = "\n".join(f"- {path}" for path in gitlink_paths[:20]) + more = ( + f"\n... {len(gitlink_paths) - 20} more" + if len(gitlink_paths) > 20 + else "" + ) + raise SystemExit( + f"refusing gitlink/submodule changes in {label} because the referenced " + f"dependency contents are not present in the review bundle:\n{details}{more}" + ) + + def file_bundle_risk( repo: Path, path: Path, @@ -811,43 +1558,79 @@ def file_bundle_risk( *, allow_binary_omission: bool = False, ) -> str | None: + return file_bundle_snapshot( + repo, + path, + rel, + allow_binary_omission=allow_binary_omission, + )[2] + + +def file_bundle_snapshot( + repo: Path, + path: Path, + rel: str, + *, + allow_binary_omission: bool = False, +) -> tuple[str, bool, str | None]: normalized = rel.replace(os.sep, "/") - if path_has_sensitive_part(normalized): - return "sensitive path" - for pattern in SENSITIVE_NAME_PATTERNS: - if pattern.search(normalized): - return "sensitive filename" + path_risk = sensitive_repo_path_risk(normalized) + if path_risk: + return "", True, path_risk if path.is_symlink(): - return "symlink" + return "", True, "symlink" try: resolved = path.resolve(strict=True) except OSError as exc: - return f"unreadable file: {exc}" + return "", True, f"unreadable file: {exc}" if not is_within(resolved, repo.resolve()): - return "path outside repository" + return "", True, "path outside repository" if not path.is_file(): - return "not a regular file" + return "", True, "not a regular file" try: - data, _ = read_prefix(path, MAX_BUNDLE_TEXT_BYTES) + data, truncated = read_prefix(path, MAX_BUNDLE_TEXT_BYTES) except SystemExit as exc: - return str(exc) + return "", True, str(exc) if b"\0" in data: - return None if allow_binary_omission else "binary file" - if secret_text_risk(data.decode("utf-8", errors="replace")): - return "secret-like content" - return None + if allow_binary_omission: + return "[binary file omitted]", True, None + return "", True, "binary file" + if truncated: + return "", True, "file too large to scan safely" + try: + text = data.decode("utf-8") + except UnicodeDecodeError: + return "", True, "non-UTF-8 file" + if secret_text_risk(text): + return "", True, "secret-like content" + return text, False, None -def safe_untracked_files(repo: Path) -> list[str]: - files = git_path_list(repo, "ls-files", "--others", "--exclude-standard", "-z") +def safe_untracked_file_snapshots(repo: Path) -> list[tuple[str, str, bool]]: + args: list[str] = [] + if excludes_file := global_excludes_file(repo): + args.extend(["-c", f"core.excludesFile={excludes_file}"]) + files = git_path_list( + repo, + *args, + "ls-files", + "--others", + "--exclude-standard", + "-z", + ) blocked: list[str] = [] - included: list[str] = [] + included: list[tuple[str, str, bool]] = [] for rel in files: - risk = file_bundle_risk(repo, repo / rel, rel, allow_binary_omission=True) + content, truncated, risk = file_bundle_snapshot( + repo, + repo / rel, + rel, + allow_binary_omission=True, + ) if risk: blocked.append(f"{rel} ({risk})") else: - included.append(rel) + included.append((rel, content, truncated)) if blocked: details = "\n".join(f"- {item}" for item in blocked[:20]) more = f"\n... {len(blocked) - 20} more" if len(blocked) > 20 else "" @@ -859,6 +1642,10 @@ def safe_untracked_files(repo: Path) -> list[str]: return included +def safe_untracked_files(repo: Path) -> list[str]: + return [rel for rel, _content, _truncated in safe_untracked_file_snapshots(repo)] + + def local_status(repo: Path, untracked: list[str]) -> str: status = git(repo, "status", "--short", "--untracked-files=no").rstrip() lines = [status] if status else [] @@ -869,25 +1656,57 @@ def local_status(repo: Path, untracked: list[str]) -> str: def local_bundle(repo: Path) -> tuple[str, bool]: staged_patch = git(repo, "diff", *SAFE_DIFF_FLAGS, "--cached", "--patch") unstaged_patch = git(repo, "diff", *SAFE_DIFF_FLAGS, "--patch") - untracked = safe_untracked_files(repo) + require_no_binary_diff( + "local staged diff", + git(repo, "diff", *SAFE_DIFF_FLAGS, "--cached", "--numstat", "-z"), + ) + require_no_binary_diff( + "local unstaged diff", + git(repo, "diff", *SAFE_DIFF_FLAGS, "--numstat", "-z"), + ) + require_no_gitlink_diff( + "local staged diff", + git(repo, "diff", *SAFE_DIFF_FLAGS, "--cached", "--raw", "-z"), + ) + require_no_gitlink_diff( + "local unstaged diff", + git(repo, "diff", *SAFE_DIFF_FLAGS, "--raw", "-z"), + ) + staged_paths = git_path_list( + repo, + "diff", + *SAFE_DIFF_FLAGS, + "--name-only", + "--cached", + "-z", + ) + unstaged_paths = git_path_list( + repo, + "diff", + *SAFE_DIFF_FLAGS, + "--name-only", + "-z", + ) + untracked_snapshots = safe_untracked_file_snapshots(repo) + untracked = [rel for rel, _content, _truncated in untracked_snapshots] if not staged_patch.strip() and not unstaged_patch.strip() and not untracked: raise SystemExit("no local changes to review") + staged_patch = validate_review_patch("local staged diff", staged_paths, staged_patch) + unstaged_patch = validate_review_patch("local unstaged diff", unstaged_paths, unstaged_patch) parts = [ "# Git Status", local_status(repo, untracked), "# Staged Diff", git(repo, "diff", *SAFE_DIFF_FLAGS, "--cached", "--stat"), - bounded(staged_patch), + staged_patch, "# Unstaged Diff", git(repo, "diff", *SAFE_DIFF_FLAGS, "--stat"), - bounded(unstaged_patch), + unstaged_patch, ] input_truncated = len(staged_patch) > 180_000 or len(unstaged_patch) > 180_000 if untracked: parts.append("# Untracked Files") - for rel in untracked: - path = repo / rel - content, truncated = read_text_with_status(path) + for rel, content, truncated in untracked_snapshots: input_truncated = input_truncated or truncated parts.append(f"## {rel}\n{content}") return "\n\n".join(parts), input_truncated @@ -895,14 +1714,49 @@ def local_bundle(repo: Path) -> tuple[str, bool]: def branch_bundle(repo: Path, base_ref: str) -> tuple[str, bool]: base_ref = validate_git_ref(repo, base_ref, "base") + diff_range = f"{base_ref}...HEAD" branch_patch = git( repo, "diff", *SAFE_DIFF_FLAGS, "--patch", "--end-of-options", - f"{base_ref}...HEAD", + diff_range, ) + branch_paths = git_path_list( + repo, + "diff", + *SAFE_DIFF_FLAGS, + "--name-only", + "-z", + "--end-of-options", + diff_range, + ) + require_no_binary_diff( + "branch diff", + git( + repo, + "diff", + *SAFE_DIFF_FLAGS, + "--numstat", + "-z", + "--end-of-options", + diff_range, + ), + ) + require_no_gitlink_diff( + "branch diff", + git( + repo, + "diff", + *SAFE_DIFF_FLAGS, + "--raw", + "-z", + "--end-of-options", + diff_range, + ), + ) + branch_patch = validate_review_patch("branch diff", branch_paths, branch_patch) return "\n\n".join( [ "# Branch Diff", @@ -913,15 +1767,21 @@ def branch_bundle(repo: Path, base_ref: str) -> tuple[str, bool]: *SAFE_DIFF_FLAGS, "--stat", "--end-of-options", - f"{base_ref}...HEAD", + diff_range, ), - bounded(branch_patch), + branch_patch, ] ), len(branch_patch) > 180_000 def commit_bundle(repo: Path, commit_ref: str) -> tuple[str, bool]: commit_ref = validate_git_ref(repo, commit_ref, "commit") + parents = git(repo, "rev-list", "--parents", "-n", "1", commit_ref).split() + if len(parents) > 2: + raise SystemExit( + "commit review does not accept merge commits; review the branch diff " + "or an individual parent-relative commit instead" + ) commit_patch = git( repo, "show", @@ -931,6 +1791,43 @@ def commit_bundle(repo: Path, commit_ref: str) -> tuple[str, bool]: "--end-of-options", commit_ref, ) + commit_paths = git_path_list( + repo, + "show", + *SAFE_DIFF_FLAGS, + "--name-only", + "--format=", + "-z", + "--end-of-options", + commit_ref, + ) + require_no_binary_diff( + "commit diff", + git( + repo, + "show", + *SAFE_DIFF_FLAGS, + "--numstat", + "--format=", + "-z", + "--end-of-options", + commit_ref, + ), + ) + require_no_gitlink_diff( + "commit diff", + git( + repo, + "show", + *SAFE_DIFF_FLAGS, + "--raw", + "--format=", + "-z", + "--end-of-options", + commit_ref, + ), + ) + commit_patch = validate_review_patch("commit diff", commit_paths, commit_patch) return "\n\n".join( [ "# Commit Diff", @@ -944,7 +1841,7 @@ def commit_bundle(repo: Path, commit_ref: str) -> tuple[str, bool]: "--end-of-options", commit_ref, ), - bounded(commit_patch), + commit_patch, ] ), len(commit_patch) > 180_000 @@ -999,10 +1896,9 @@ def validate_evidence_file(repo: Path, raw_path: str, label: str) -> tuple[Path, if not is_within(path, repo.resolve()): raise SystemExit(f"{label} must be inside the reviewed repository: {raw_path}") rel = str(path.relative_to(repo.resolve())) - risk = file_bundle_risk(repo, path, rel) + content, truncated, risk = file_bundle_snapshot(repo, path, rel) if risk: raise SystemExit(f"refusing to include unsafe {label}: {rel} ({risk})") - content, truncated = read_text_with_status(path) require_no_secret_values(f"{label} {rel}", content) return path, content, truncated @@ -1059,8 +1955,11 @@ def review_scope_policy() -> str: def build_prompt(repo: Path, target: str, target_ref: str | None, bundle: str, extra_prompt: str, datasets: str) -> str: target_line = f"{target} {target_ref}" if target_ref else target branch = current_branch(repo) + require_no_secret_values("current branch", branch) + if target_ref: + require_no_secret_values("review target ref", target_ref) scope_policy = review_scope_policy() - return textwrap.dedent( + prompt = textwrap.dedent( f""" You are a senior code reviewer. Review the provided git change bundle only. @@ -1082,7 +1981,7 @@ def build_prompt(repo: Path, target: str, target_ref: str | None, bundle: str, e Review target: {target_line} Current branch: {branch} - Repository: {repo} + Repository root: . {scope_policy} @@ -1094,10 +1993,22 @@ def build_prompt(repo: Path, target: str, target_ref: str | None, bundle: str, e {bundle} """ ).strip() + prompt_bytes = len(prompt.encode("utf-8")) + if prompt_bytes > MAX_REVIEW_PROMPT_BYTES: + raise SystemExit( + f"review input is {prompt_bytes} bytes, exceeding the {MAX_REVIEW_PROMPT_BYTES}-byte aggregate limit; " + "reduce the change, prompt files, or datasets" + ) + return prompt -def write_json_temp(data: dict[str, Any]) -> Path: - handle = tempfile.NamedTemporaryFile("w", suffix=".json", delete=False) +def write_json_temp(data: dict[str, Any], temp_root: Path) -> Path: + handle = tempfile.NamedTemporaryFile( + "w", + suffix=".json", + delete=False, + dir=temp_root, + ) with handle: json.dump(data, handle) return Path(handle.name) @@ -1107,12 +2018,32 @@ def toml_quoted_key_segment(value: str) -> str: return json.dumps(value) +def toml_inline_string_table(values: dict[str, str]) -> str: + entries = ", ".join(f"{key}={json.dumps(value)}" for key, value in sorted(values.items())) + return "{" + entries + "}" + + def codex_config_isolation_flags(repo: Path) -> list[str]: + tool_env = toml_inline_string_table(codex_tool_git_env()) return [ "-c", "project_doc_max_bytes=0", "-c", f"projects.{toml_quoted_key_segment(str(repo.resolve()))}.trust_level=\"untrusted\"", + "-c", + 'shell_environment_policy.inherit="core"', + "-c", + "shell_environment_policy.ignore_default_excludes=false", + "-c", + f"shell_environment_policy.set={tool_env}", + "-c", + "shell_environment_policy.experimental_use_profile=false", + "-c", + "allow_login_shell=false", + "-c", + 'default_permissions="autoreview"', + "-c", + 'permissions.autoreview.filesystem={":minimal"="read",":workspace_roots"="read"}', ] @@ -1173,8 +2104,10 @@ def load_codex_auth_config(path: Path) -> dict[str, Any]: return config if isinstance(config, dict) else {} -def codex_auth_config_flags() -> list[str]: +def codex_auth_config_flags(repo: Path) -> list[str]: codex_home = Path(os.environ.get("CODEX_HOME", Path.home() / ".codex")) + if not external_env_path(repo, str(codex_home)): + return [] config = load_codex_auth_config(codex_home / "config.toml") allowed_values = { @@ -1201,7 +2134,7 @@ def codex_auth_config_flags() -> list[str]: def codex_exec_isolation_flags() -> list[str]: - return ["--ignore-user-config", "--ignore-rules"] + return ["--ignore-user-config", "--ignore-rules", "--skip-git-repo-check"] def claude_review_isolation_flags() -> list[str]: @@ -1236,10 +2169,15 @@ def parse_cli_version(text: str) -> tuple[int, int, int] | None: def ensure_claude_isolation_supported(args: argparse.Namespace, repo: Path) -> None: claude_bin = resolve_command(args.claude_bin, repo) - engine_env = safe_engine_env(repo, [Path(claude_bin).parent]) - result = run([claude_bin, "--version"], repo, check=False, env=engine_env) + engine_env = safe_engine_env( + repo, + [Path(claude_bin).parent], + engine="claude", + ) + temp_root = safe_temp_root(repo) + result = run([claude_bin, "--version"], temp_root, check=False, env=engine_env) selected_models = [args.model, *(getattr(args, "fallback_model", "") or "").split(",")] - uses_fable = "claude-fable-5" in selected_models + uses_fable = any(model in {"claude-fable-5", "fable"} for model in selected_models) minimum_version = CLAUDE_FABLE_MIN_VERSION if uses_fable else CLAUDE_SAFE_MODE_MIN_VERSION version_reason = "for claude-fable-5" if uses_fable else "for --safe-mode" if result.returncode != 0: @@ -1252,9 +2190,9 @@ def ensure_claude_isolation_supported(args: argparse.Namespace, repo: Path) -> N f"claude engine requires Claude Code >= {format_version(minimum_version)} " f"{version_reason} (found {format_version(version)})" ) - help_result = run([claude_bin, "--help"], repo, check=False, env=engine_env) + help_result = run([claude_bin, "--help"], temp_root, check=False, env=engine_env) help_text = f"{help_result.stdout}\n{help_result.stderr}" - required_flags = ["--safe-mode", "--setting-sources", "--strict-mcp-config", "--disallowedTools"] + required_flags = ["--safe-mode", "--setting-sources", "--strict-mcp-config", "--disallowedTools", "--tools"] missing = [flag for flag in required_flags if flag not in help_text] if help_result.returncode != 0 or missing: detail = ", ".join(missing) if missing else "--help failed" @@ -1263,8 +2201,11 @@ def ensure_claude_isolation_supported(args: argparse.Namespace, repo: Path) -> N def ensure_pi_isolation_supported(args: argparse.Namespace, repo: Path) -> str: pi_bin = resolve_command(args.pi_bin, repo) - engine_env = safe_engine_env(repo, [Path(pi_bin).parent]) - with tempfile.TemporaryDirectory(prefix="autoreview-pi-probe.") as tempdir: + engine_env = safe_engine_env(repo, [Path(pi_bin).parent], engine="pi") + with tempfile.TemporaryDirectory( + prefix="autoreview-pi-probe.", + dir=safe_temp_root(repo), + ) as tempdir: probe_cwd = Path(tempdir) result = run([pi_bin, "--version"], probe_cwd, check=False, env=engine_env) help_result = run([pi_bin, "--help"], probe_cwd, check=False, env=engine_env) @@ -1296,6 +2237,22 @@ def format_version(version: tuple[int, int, int]) -> str: return ".".join(str(part) for part in version) +SAFE_CODEX_CONFIG_KEYS = { + "hide_agent_reasoning", + "model_auto_compact_token_limit", + "model_auto_compact_token_limit_scope", + "model_context_window", + "model_reasoning_effort", + "model_reasoning_summary", + "model_verbosity", + "personality", + "plan_mode_reasoning_effort", + "service_tier", + "show_raw_agent_reasoning", + "tool_output_token_limit", +} + + def codex_config_overrides(args: argparse.Namespace) -> list[str]: raw = list(getattr(args, "codex_config", None) or []) if not raw: @@ -1306,8 +2263,14 @@ def codex_config_overrides(args: argparse.Namespace) -> list[str]: if not item: continue key, sep, value = item.partition("=") - if not sep or not value.strip() or not re.fullmatch(r"[A-Za-z0-9_][A-Za-z0-9_.-]*", key.strip()): + key = key.strip() + if not sep or not value.strip() or not re.fullmatch(r"[A-Za-z0-9_][A-Za-z0-9_.-]*", key): raise SystemExit(f"invalid Codex config override (expected key=value): {item}") + if key not in SAFE_CODEX_CONFIG_KEYS: + raise SystemExit( + f"unsafe Codex config override refused: {key}; " + "only model and response tuning keys are allowed" + ) overrides.append(item) return overrides @@ -1326,16 +2289,72 @@ def codex_speed_override(args: argparse.Namespace) -> str | None: return f'service_tier="{speed}"' -def run_codex(args: argparse.Namespace, repo: Path, prompt: str) -> str: - if not args.tools: - raise SystemExit("--no-tools is not supported by the Codex engine; use --engine claude --no-tools for a no-tools run") - schema_path = write_json_temp(SCHEMA) - output_path = Path(tempfile.NamedTemporaryFile("w", suffix=".json", delete=False).name) - cmd = [resolve_command(args.codex_bin, repo), "--ask-for-approval", "never"] +def codex_error_messages(result: subprocess.CompletedProcess[str]) -> list[str]: + messages: list[str] = [] + for stream, accept_plain_text in ( + (result.stderr, True), + (result.stdout, False), + ): + for raw_line in stream.splitlines(): + line = raw_line.strip() + if not line: + continue + if not line.startswith("{"): + if accept_plain_text: + messages.append(line) + continue + try: + event = json.loads(line) + except json.JSONDecodeError: + continue + if not isinstance(event, dict) or event.get("type") not in { + "error", + "turn.failed", + }: + continue + message = event.get("message") + if isinstance(message, str): + messages.append(message) + error = event.get("error") + if isinstance(error, str): + messages.append(error) + elif isinstance(error, dict) and isinstance(error.get("message"), str): + messages.append(error["message"]) + return messages + + +def codex_model_access_failure(result: subprocess.CompletedProcess[str], model: str) -> bool: + for message in codex_error_messages(result): + lowered = message.lower() + if model.lower() not in lowered: + continue + if any( + marker in lowered + for marker in ( + "does not exist or you do not have access", + "do not have access to", + "don't have access to", + "does not appear in the list of models available to your account", + "not supported when using codex", + ) + ): + return True + return False + + +def codex_command( + args: argparse.Namespace, + source_repo: Path, + review_root: Path, + schema_path: Path, + output_path: Path, + model: str | None, +) -> list[str]: + cmd = [resolve_command(args.codex_bin, source_repo), "--ask-for-approval", "never"] if args.web_search: cmd.append("--search") - if args.model: - cmd.extend(["--model", args.model]) + if model: + cmd.extend(["--model", model]) # User overrides go before the isolation flags so isolation stays authoritative on conflicts. for override in codex_config_overrides(args): cmd.extend(["-c", override]) @@ -1346,8 +2365,8 @@ def run_codex(args: argparse.Namespace, repo: Path, prompt: str) -> str: speed_override = codex_speed_override(args) if speed_override is not None: cmd.extend(["-c", speed_override]) - cmd.extend(codex_config_isolation_flags(repo)) - cmd.extend(codex_auth_config_flags()) + cmd.extend(codex_config_isolation_flags(review_root)) + cmd.extend(codex_auth_config_flags(source_repo)) cmd.append("exec") if args.stream_engine_output: cmd.append("--json") @@ -1356,9 +2375,7 @@ def run_codex(args: argparse.Namespace, repo: Path, prompt: str) -> str: *codex_exec_isolation_flags(), "--ephemeral", "-C", - str(repo), - "-s", - "read-only", + str(review_root), "--output-schema", str(schema_path), "--output-last-message", @@ -1366,23 +2383,86 @@ def run_codex(args: argparse.Namespace, repo: Path, prompt: str) -> str: "-", ] ) - result = run_with_heartbeat( - cmd, - repo, - input_text=prompt, - label="codex", - stream_output=args.stream_engine_output, - stream_display=CodexStreamDisplay() if args.stream_engine_output else None, - env=safe_engine_env(repo, [Path(cmd[0]).parent]), - ) + return cmd + + +def run_codex(args: argparse.Namespace, repo: Path, prompt: str) -> str: + if not args.tools: + raise SystemExit("--no-tools is not supported by the Codex engine; use --engine claude --no-tools for a no-tools run") + temp_root = safe_temp_root(repo) + schema_path = write_json_temp(SCHEMA, temp_root) + with tempfile.NamedTemporaryFile( + "w", + suffix=".json", + delete=False, + dir=temp_root, + ) as output_file: + output_path = Path(output_file.name) + models = [args.model] + fallback_model = getattr(args, "fallback_model", None) + if fallback_model and fallback_model != args.model: + models.append(fallback_model) + primary_failure: subprocess.CompletedProcess[str] | None = None try: - output = output_path.read_text() + # The validated bundle is the sole repository input. The empty + # workspace keeps ignored credentials and linked-worktree metadata + # outside the model's readable filesystem boundary. + with tempfile.TemporaryDirectory( + prefix="autoreview-codex-workspace.", + dir=temp_root, + ) as tempdir: + review_root = Path(tempdir) + for index, model in enumerate(models): + output_path.write_text("") + cmd = codex_command( + args, + repo, + review_root, + schema_path, + output_path, + model, + ) + result = run_with_heartbeat( + cmd, + review_root, + input_text=prompt, + label="codex", + stream_output=args.stream_engine_output, + stream_display=CodexStreamDisplay() if args.stream_engine_output else None, + env=safe_engine_env( + repo, + [Path(cmd[0]).parent], + engine="codex", + ), + resolve_root=repo, + ) + output = output_path.read_text() + if result.returncode == 0: + return output or result.stdout + if ( + index == 0 + and len(models) > 1 + and model + and codex_model_access_failure(result, model) + ): + primary_failure = result + print( + f"codex model {model} is unavailable for this account; retrying with {models[1]}", + file=sys.stderr, + ) + continue + detail = result.stderr or result.stdout + if primary_failure is not None: + primary_detail = primary_failure.stderr or primary_failure.stdout + raise SystemExit( + f"codex engine failed with primary model ({primary_failure.returncode})\n{primary_detail}\n" + f"codex fallback model failed ({result.returncode})\n{detail}" + ) + raise SystemExit(f"codex engine failed ({result.returncode})\n{detail}") finally: schema_path.unlink(missing_ok=True) output_path.unlink(missing_ok=True) - if result.returncode != 0: - raise SystemExit(f"codex engine failed ({result.returncode})\n{result.stderr or result.stdout}") - return output or result.stdout + raise AssertionError("unreachable") def run_claude(args: argparse.Namespace, repo: Path, prompt: str) -> str: @@ -1398,7 +2478,8 @@ def run_claude(args: argparse.Namespace, repo: Path, prompt: str) -> str: json.dumps(SCHEMA), ] if args.tools: - cmd.extend(["--allowedTools", claude_allowed_tools(args)]) + allowed_tools = claude_allowed_tools(args) + cmd.extend(["--tools", claude_tool_inventory(args), "--allowedTools", allowed_tools]) else: cmd.extend(["--tools", ""]) if args.stream_engine_output: @@ -1409,15 +2490,24 @@ def run_claude(args: argparse.Namespace, repo: Path, prompt: str) -> str: cmd.extend(["--fallback-model", args.fallback_model]) if args.thinking: cmd.extend(["--effort", args.thinking]) - result = run_with_heartbeat( - cmd, - repo, - input_text=prompt, - label="claude", - stream_output=args.stream_engine_output, - stream_display=ClaudeStreamDisplay() if args.stream_engine_output else None, - env=safe_engine_env(repo, [Path(cmd[0]).parent]), - ) + with tempfile.TemporaryDirectory( + prefix="autoreview-claude-workspace.", + dir=safe_temp_root(repo), + ) as tempdir: + result = run_with_heartbeat( + cmd, + Path(tempdir), + input_text=prompt, + label="claude", + stream_output=args.stream_engine_output, + stream_display=ClaudeStreamDisplay() if args.stream_engine_output else None, + env=safe_engine_env( + repo, + [Path(cmd[0]).parent], + engine="claude", + ), + resolve_root=repo, + ) if result.returncode != 0: raise SystemExit(f"claude engine failed ({result.returncode})\n{result.stderr or result.stdout}") return result.stdout @@ -1426,56 +2516,15 @@ def run_claude(args: argparse.Namespace, repo: Path, prompt: str) -> str: def run_droid(args: argparse.Namespace, repo: Path, prompt: str) -> str: raise SystemExit( "droid engine is unavailable: the current Droid CLI cannot disable project instructions and all tools; " - "use codex, claude, copilot, pi, opencode, or cursor" + "use codex, claude, or pi" ) def run_copilot(args: argparse.Namespace, repo: Path, prompt: str) -> str: - if args.thinking: - raise SystemExit("--thinking is not supported by the copilot engine") - if not args.tools: - raise SystemExit("--no-tools is not supported by the copilot engine; copilot requires a read-only file view tool to load the review bundle without exposing it in argv") - # Copilot child processes can briefly retain this cwd on Windows after a - # successful review. Cleanup failure must not replace the review result. - with tempfile.TemporaryDirectory(prefix="autoreview-copilot.", ignore_cleanup_errors=True) as tempdir: - prompt_path = Path(tempdir) / "prompt.txt" - prompt_path.write_text(prompt) - os.chmod(prompt_path, 0o600) - cmd = [ - resolve_command(args.copilot_bin, repo), - "-C", - tempdir, - "-p", - "Read ./prompt.txt and follow it exactly. Return only the requested JSON object.", - "--output-format", - "json", - "--stream", - "on" if args.stream_engine_output else "off", - "--no-ask-user", - "--disable-builtin-mcps", - ] - if args.model: - cmd.extend(["--model", args.model]) - available_tools = ["read_agent", "rg", "view"] - allowed_tools = available_tools[:] - if args.web_search: - available_tools.append("web_fetch") - allowed_tools.append("web_fetch") - cmd.append("--allow-all-urls") - cmd.append(f"--available-tools={','.join(available_tools)}") - for tool in allowed_tools: - cmd.append(f"--allow-tool={tool}") - result = run_with_heartbeat( - cmd, - Path(tempdir), - label="copilot", - stream_output=args.stream_engine_output, - resolve_root=repo, - env=safe_engine_env(repo, [Path(cmd[0]).parent]), - ) - if result.returncode != 0: - raise SystemExit(f"copilot engine failed ({result.returncode})\n{result.stderr or result.stdout}") - return result.stdout + raise SystemExit( + "copilot engine is unavailable: its file tools cannot be confined to the reviewed bundle " + "without exposing ignored repository secrets; use codex, claude, or pi" + ) def build_opencode_cmd(args: argparse.Namespace, repo: Path) -> list[str]: @@ -1496,26 +2545,11 @@ def build_opencode_cmd(args: argparse.Namespace, repo: Path) -> list[str]: def run_opencode(args: argparse.Namespace, repo: Path, prompt: str) -> str: - if not args.tools: - raise SystemExit("--no-tools is not supported by the opencode engine") - cmd = build_opencode_cmd(args, repo) - with tempfile.TemporaryDirectory(prefix="autoreview-opencode-run.") as tempdir: - result = run_with_heartbeat( - cmd, - Path(tempdir), - input_text=prompt, - label="opencode", - stream_output=args.stream_engine_output, - env=safe_engine_env( - repo, - [Path(cmd[0]).parent], - opencode_review_env(args.web_search), - ), - resolve_root=repo, - ) - if result.returncode != 0: - raise SystemExit(f"opencode engine failed ({result.returncode})\n{result.stderr or result.stdout}") - return result.stdout + raise SystemExit( + "opencode engine is unavailable: the current CLI contract does not prove " + "project-config isolation and its generic fetch tool cannot be restricted " + "away from private or metadata endpoints; use codex, claude, or pi" + ) def cursor_local_mcp_paths(repo: Path) -> list[Path]: @@ -1527,7 +2561,7 @@ def cursor_local_mcp_paths(repo: Path) -> list[Path]: return [path for path in candidates if path.exists()] -def cursor_global_mcp_paths() -> list[Path]: +def cursor_home_candidates() -> set[Path]: home_candidates = [Path.home()] for name in ("HOME", "USERPROFILE"): if value := os.environ.get(name): @@ -1535,10 +2569,40 @@ def cursor_global_mcp_paths() -> list[Path]: if drive := os.environ.get("HOMEDRIVE"): if home_path := os.environ.get("HOMEPATH"): home_candidates.append(Path(f"{drive}{home_path}")) - paths = {home / ".cursor" / "mcp.json" for home in home_candidates} + return set(home_candidates) + + +def cursor_global_mcp_paths() -> list[Path]: + paths = {home / ".cursor" / "mcp.json" for home in cursor_home_candidates()} return sorted((path for path in paths if path.exists()), key=str) +def json_file_declares_hooks(path: Path) -> bool: + try: + parsed = json.loads(path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + return True + if not isinstance(parsed, dict): + return False + if parsed.get("hooks"): + return True + enabled_plugins = parsed.get("enabledPlugins") + return isinstance(enabled_plugins, dict) and any(bool(enabled) for enabled in enabled_plugins.values()) + + +def cursor_global_hook_paths() -> list[Path]: + paths: set[Path] = set() + for home in cursor_home_candidates(): + cursor_hooks = home / ".cursor" / "hooks.json" + if cursor_hooks.exists(): + paths.add(cursor_hooks) + for name in ("settings.json", "settings.local.json"): + claude_settings = home / ".claude" / name + if claude_settings.exists() and json_file_declares_hooks(claude_settings): + paths.add(claude_settings) + return sorted(paths, key=str) + + def cursor_local_hook_paths(repo: Path) -> list[Path]: candidates = [ repo / ".cursor" / "hooks.json", @@ -1651,83 +2715,10 @@ def build_cursor_cmd( def run_cursor(args: argparse.Namespace, repo: Path, prompt: str) -> str: - if args.thinking: - raise SystemExit("--thinking is not supported by the cursor engine") - if not args.tools: - raise SystemExit("--no-tools is not supported by the cursor engine") - if not args.web_search: - raise SystemExit("--no-web-search is not supported by the cursor engine; Cursor CLI does not expose a documented per-run web-search disable flag") - local_mcp_paths = cursor_local_mcp_paths(repo) - global_mcp_paths = cursor_global_mcp_paths() - local_hook_paths = cursor_local_hook_paths(repo) - local_permission_paths = cursor_local_permission_paths(repo) - if not args.cursor_allow_workspace_instructions: - raise SystemExit( - "cursor engine requires --cursor-allow-workspace-instructions for an explicitly trusted repo; " - "Cursor CLI does not expose a per-run project-resource disable flag." - ) - if local_hook_paths: - raise SystemExit( - "cursor engine refused project-local hooks: " - + format_repo_paths(repo, local_hook_paths) - + ". Cursor hooks execute host commands outside review tool permissions." - ) - if local_mcp_paths: - raise SystemExit( - "cursor engine refused project-local MCP config: " - + format_repo_paths(repo, local_mcp_paths) - + ". Cursor MCP tools cannot be restricted to read-only review access." - ) - if global_mcp_paths: - raise SystemExit( - "cursor engine refused global MCP config because Cursor can load it outside the temporary review config." - ) - if local_permission_paths: - raise SystemExit( - "cursor engine refused project-local permission config: " - + format_repo_paths(repo, local_permission_paths) - + ". Project permissions can override the read-only review policy." - ) - print( - "cursor isolation warning: trusted project-local resources are being allowed explicitly.", - file=sys.stderr, + raise SystemExit( + "cursor engine is unavailable: Cursor read permissions can target absolute host paths " + "and the CLI does not expose a proven repository-only filesystem sandbox" ) - cursor_bin = resolve_command(args.cursor_bin, repo) - with tempfile.TemporaryDirectory(prefix="autoreview-cursor-config.") as tempdir: - config_dir = Path(tempdir) - (config_dir / "cli-config.json").write_text( - json.dumps( - { - "version": 1, - "editor": {"vimMode": False}, - "permissions": { - "allow": ["Read(**)"], - "deny": ["Shell(*)", "Write(**)", "Write(/**)"], - }, - } - ) - + "\n" - ) - engine_env = safe_engine_env( - repo, - [Path(cursor_bin).parent], - {"CURSOR_CONFIG_DIR": str(config_dir)}, - ) - cmd = build_cursor_cmd(args, repo, cursor_bin, engine_env) - result = run_with_heartbeat( - cmd, - repo, - input_text=prompt, - label="cursor", - stream_output=args.stream_engine_output, - stream_display=CursorStreamDisplay() if args.stream_engine_output else None, - env=engine_env, - resolve_root=repo, - ) - if result.returncode != 0: - raise SystemExit(f"cursor engine failed ({result.returncode})\n{result.stderr or result.stdout}") - print_cursor_metadata(result.stdout) - return result.stdout def run_pi(args: argparse.Namespace, repo: Path, prompt: str) -> str: @@ -1744,7 +2735,10 @@ def run_pi(args: argparse.Namespace, repo: Path, prompt: str) -> str: # Pi's built-in read tools accept absolute paths and have no repository # confinement, so an untrusted review prompt must never receive them. cmd.append("--no-tools") - with tempfile.TemporaryDirectory(prefix="autoreview-pi-run.") as tempdir: + with tempfile.TemporaryDirectory( + prefix="autoreview-pi-run.", + dir=safe_temp_root(repo), + ) as tempdir: result = run_with_heartbeat( cmd, Path(tempdir), @@ -1752,7 +2746,11 @@ def run_pi(args: argparse.Namespace, repo: Path, prompt: str) -> str: label="pi", stream_output=args.stream_engine_output, resolve_root=repo, - env=safe_engine_env(repo, [Path(cmd[0]).parent]), + env=safe_engine_env( + repo, + [Path(cmd[0]).parent], + engine="pi", + ), ) if result.returncode != 0: raise SystemExit(f"pi engine failed ({result.returncode})\n{result.stderr or result.stdout}") @@ -1962,11 +2960,41 @@ def format_cursor_usage(usage: Any) -> str: return "cursor usage: " + " ".join(parts) if parts else "cursor usage: unavailable" -def claude_allowed_tools(args: argparse.Namespace) -> str: +def claude_tool_name(rule: str) -> str: + match = re.match(r"^([A-Za-z][A-Za-z0-9_-]*)(?:\(|$)", rule) + if not match: + raise SystemExit(f"invalid Claude tool rule: {rule}") + return match.group(1) + + +def claude_tool_rules(args: argparse.Namespace) -> list[str]: tools = [tool.strip() for tool in args.claude_allowed_tools.split(",") if tool.strip()] if not args.web_search: - tools = [tool for tool in tools if tool not in {"WebSearch", "WebFetch"}] - return ",".join(tools) + tools = [tool for tool in tools if claude_tool_name(tool) not in {"WebSearch", "WebFetch"}] + return tools + + +def claude_allowed_tools(args: argparse.Namespace) -> str: + return ",".join(claude_tool_rules(args)) + + +def claude_tool_inventory(args: argparse.Namespace) -> str: + safe_tools = {"WebFetch", "WebSearch"} + names: list[str] = [] + for rule in claude_tool_rules(args): + name = claude_tool_name(rule) + if name not in safe_tools: + raise SystemExit(f"Claude review tool is not read-only: {name}") + if name == "WebFetch" and not re.fullmatch( + r"WebFetch\(domain:[A-Za-z0-9.-]+\)", + rule, + ): + raise SystemExit( + "Claude WebFetch must be constrained to one explicit domain" + ) + if name not in names: + names.append(name) + return ",".join(names) def extract_json(text: str) -> dict[str, Any]: @@ -1979,7 +3007,7 @@ def extract_json(text: str) -> dict[str, Any]: jsonl_report = extract_json_from_jsonl(stripped) if jsonl_report: return jsonl_report - fenced_report = extract_findings_json_from_text(stripped) or parse_json_candidate(stripped) + fenced_report = parse_json_candidate(stripped) if isinstance(fenced_report, dict) and "findings" in fenced_report: return fenced_report raise SystemExit(f"review engine returned non-JSON output: {exc}\n{stripped[:2000]}") @@ -1992,7 +3020,7 @@ def extract_json(text: str) -> dict[str, Any]: if "findings" in result_object: return result_object if isinstance(parsed, dict) and isinstance(parsed.get("result"), str): - result_json = extract_findings_json_from_text(parsed["result"]) or parse_json_candidate(parsed["result"]) + result_json = parse_json_candidate(parsed["result"]) if isinstance(result_json, dict) and "findings" in result_json: return result_json raise SystemExit(f"review engine result was not structured JSON:\n{parsed['result'][:2000]}") @@ -2053,7 +3081,7 @@ def _report_from_events(events: list[Any]) -> dict[str, Any] | None: if "findings" in candidate: return candidate continue - parsed = extract_findings_json_from_text(candidate) or parse_json_candidate(candidate) + parsed = parse_json_candidate(candidate) if isinstance(parsed, dict) and "findings" in parsed: return parsed if terminal_candidates: @@ -2063,11 +3091,11 @@ def _report_from_events(events: list[Any]) -> dict[str, Any] | None: if "findings" in candidate: return candidate continue - parsed = extract_findings_json_from_text(candidate) or parse_json_candidate(candidate) + parsed = parse_json_candidate(candidate) if isinstance(parsed, dict) and "findings" in parsed: return parsed for candidate in reversed(assistant_candidates): - parsed = extract_findings_json_from_text(candidate) or parse_json_candidate(candidate) + parsed = parse_json_candidate(candidate) if isinstance(parsed, dict) and "findings" in parsed: return parsed return None @@ -2086,22 +3114,6 @@ def extract_json_from_jsonl(text: str) -> dict[str, Any] | None: return _report_from_events(events) -def extract_findings_json_from_text(text: str) -> dict[str, Any] | None: - stripped = text.strip() - decoder = json.JSONDecoder() - parsed: dict[str, Any] | None = None - for index, char in enumerate(stripped): - if char != "{": - continue - try: - candidate, _ = decoder.raw_decode(stripped[index:]) - except json.JSONDecodeError: - continue - if isinstance(candidate, dict) and "findings" in candidate: - parsed = candidate - return parsed - - def is_structured_output_failure(message: str) -> bool: return message.startswith("review engine returned non-JSON output") or message.startswith( "review engine result was not structured JSON" @@ -2239,10 +3251,15 @@ if "--version" in args or "-v" in args: print(os.environ.get("AUTOREVIEW_FAKE_CLAUDE_VERSION", "2.1.170 (Claude Code)")) raise SystemExit(0) if "--help" in args or "-h" in args: - print("--safe-mode\n--setting-sources\n--strict-mcp-config\n--disallowedTools\n--print\n--json-schema") + print("--safe-mode\n--setting-sources\n--strict-mcp-config\n--disallowedTools\n--tools\n--print\n--json-schema") raise SystemExit(0) record = os.environ["AUTOREVIEW_FAKE_RECORD"] -Path(record).write_text(json.dumps({"argv": args, "cwd": os.getcwd(), "stdin": sys.stdin.read()})) +Path(record).write_text(json.dumps({ + "argv": args, + "cwd": os.getcwd(), + "stdin": sys.stdin.read(), + "auto_memory_disabled": os.environ.get("CLAUDE_CODE_DISABLE_AUTO_MEMORY"), +})) report = { "findings": [], "overall_correctness": "patch is correct", @@ -2411,7 +3428,7 @@ def self_test_engine_isolation() -> int: model=None, thinking=None, stream_engine_output=False, - claude_allowed_tools="Read,Grep,Glob,WebSearch,WebFetch", + claude_allowed_tools="WebSearch,WebFetch(domain:docs.example.com)", cursor_allow_workspace_instructions=False, ) @@ -2446,26 +3463,32 @@ def self_test_engine_isolation() -> int: run_codex(args, repo, "review hostile patch") codex_record = json.loads(record_path.read_text()) codex_argv = codex_record["argv"] - expected_project_override = f"projects.{toml_quoted_key_segment(str(repo.resolve()))}.trust_level=\"untrusted\"" for required in [ "--ignore-user-config", "--ignore-rules", "project_doc_max_bytes=0", - expected_project_override, 'cli_auth_credentials_store="auto"', 'forced_login_method="chatgpt"', 'forced_chatgpt_workspace_id=["workspace-one", "workspace-two"]', + 'default_permissions="autoreview"', + 'permissions.autoreview.filesystem={":minimal"="read",":workspace_roots"="read"}', "--ephemeral", - str(repo), - "read-only", ]: if required not in codex_argv: raise SystemExit(f"codex isolation self-test failed: missing {required}") - for forbidden in ["hostile-user-model"]: + for forbidden in ["hostile-user-model", "read-only"]: if forbidden in codex_argv: raise SystemExit(f"codex isolation self-test failed: leaked {forbidden}") - if Path(codex_record["cwd"]).resolve() != repo.resolve(): - raise SystemExit("codex isolation self-test failed: wrong cwd") + codex_cwd = Path(codex_record["cwd"]).resolve() + if codex_cwd == repo.resolve() or is_within(codex_cwd, repo.resolve()): + raise SystemExit("codex isolation self-test failed: review ran inside hostile repo") + if str(repo) in codex_argv: + raise SystemExit("codex isolation self-test failed: hostile repo granted to tools") + expected_project_override = ( + f"projects.{toml_quoted_key_segment(str(codex_cwd))}.trust_level=\"untrusted\"" + ) + if expected_project_override not in codex_argv: + raise SystemExit("codex isolation self-test failed: isolated project override missing") run_claude(args, repo, "review hostile patch") claude_record = json.loads(record_path.read_text()) @@ -2473,8 +3496,21 @@ def self_test_engine_isolation() -> int: for required in claude_review_isolation_flags(): if required not in claude_argv: raise SystemExit(f"claude isolation self-test failed: missing {required}") - if Path(claude_record["cwd"]).resolve() != repo.resolve(): - raise SystemExit("claude isolation self-test failed: wrong cwd") + allowed_tools = claude_allowed_tools(args) + for required in ["--tools", allowed_tools, "--allowedTools"]: + if required not in claude_argv: + raise SystemExit(f"claude isolation self-test failed: missing {required}") + tools_index = claude_argv.index("--tools") + if claude_argv[tools_index + 1] != "WebSearch,WebFetch": + raise SystemExit("claude isolation self-test failed: wrong tool inventory") + allowed_index = claude_argv.index("--allowedTools") + if claude_argv[allowed_index + 1] != allowed_tools: + raise SystemExit("claude isolation self-test failed: scoped allowed tools lost") + claude_cwd = Path(claude_record["cwd"]).resolve() + if claude_cwd == repo.resolve() or is_within(claude_cwd, repo.resolve()): + raise SystemExit("claude isolation self-test failed: review ran inside hostile repo") + if claude_record["auto_memory_disabled"] != "1": + raise SystemExit("claude isolation self-test failed: auto-memory not disabled") run_pi(args, repo, f"review hostile patch\nRepository: {repo}") pi_record = json.loads(record_path.read_text()) @@ -2500,31 +3536,17 @@ def self_test_engine_isolation() -> int: if Path(entry["cwd"]).resolve() == repo.resolve(): raise SystemExit("pi isolation self-test failed: probe ran inside hostile repo") - run_opencode(args, repo, "review hostile patch") - opencode_record = json.loads(record_path.read_text()) - opencode_argv = opencode_record["argv"] - for required in ["run", "--dir", str(repo), "--pure", "--format", "json"]: - if required not in opencode_argv: - raise SystemExit(f"opencode isolation self-test failed: missing {required}") - if "--dangerously-skip-permissions" in opencode_argv: - raise SystemExit("opencode isolation self-test failed: skip-permissions present") - if Path(opencode_record["cwd"]).resolve() == repo.resolve(): - raise SystemExit("opencode isolation self-test failed: review ran inside hostile repo") - if opencode_record["stdin"] != "review hostile patch": - raise SystemExit("opencode isolation self-test failed: prompt not delivered over stdin") - if "review hostile patch" in opencode_argv: - raise SystemExit("opencode isolation self-test failed: prompt leaked into argv") - opencode_env = opencode_record["env"] - if opencode_env.get("OPENCODE_DISABLE_PROJECT_CONFIG") != "1": - raise SystemExit("opencode isolation self-test failed: project config env missing") - if opencode_env.get("OPENCODE_DISABLE_AUTOUPDATE") != "1": - raise SystemExit("opencode isolation self-test failed: autoupdate env missing") - config = json.loads(opencode_env["OPENCODE_CONFIG_CONTENT"]) - if config.get("instructions") != [] or config.get("plugin") != [] or config.get("command") != {}: - raise SystemExit("opencode isolation self-test failed: project-controlled extensions not cleared") - for disabled_tool in ("bash", "edit", "skill", "task", "todowrite", "write"): - if config.get("tools", {}).get(disabled_tool) is not False: - raise SystemExit(f"opencode isolation self-test failed: {disabled_tool} tool not disabled") + if record_path.exists(): + record_path.unlink() + try: + run_opencode(args, repo, "review hostile patch") + except SystemExit as exc: + if "opencode engine is unavailable" not in str(exc): + raise + else: + raise SystemExit("opencode isolation self-test failed: unsafe engine was allowed") + if record_path.exists(): + raise SystemExit("opencode isolation self-test failed: disabled engine was invoked") if hostile_ps_path.exists(): raise SystemExit("heartbeat metrics isolation self-test failed: repo-local ps executed") @@ -2533,73 +3555,12 @@ def self_test_engine_isolation() -> int: try: run_cursor(args, repo, "review hostile patch") except SystemExit as exc: - if "requires --cursor-allow-workspace-instructions" not in str(exc): + if "Cursor read permissions" not in str(exc): raise else: - raise SystemExit("cursor isolation self-test failed: hostile project surfaces should be refused") + raise SystemExit("cursor isolation self-test failed: unconfined reads were allowed") if record_path.exists(): - raise SystemExit("cursor isolation self-test failed: cursor invoked despite refused project surfaces") - - args.cursor_allow_workspace_instructions = True - try: - run_cursor(args, repo, "review hostile patch") - except SystemExit as exc: - if "project-local hooks" not in str(exc): - raise - else: - raise SystemExit("cursor isolation self-test failed: local hooks should be refused") - if record_path.exists(): - raise SystemExit("cursor isolation self-test failed: cursor invoked despite refused hooks") - - for relative_path in (Path(".claude/settings.json"), Path(".claude/settings.local.json")): - (repo / relative_path).unlink(missing_ok=True) - try: - run_cursor(args, repo, "review hostile patch") - except SystemExit as exc: - if "project-local MCP config" not in str(exc): - raise - else: - raise SystemExit("cursor isolation self-test failed: local MCP config should be refused") - if record_path.exists(): - raise SystemExit("cursor isolation self-test failed: cursor invoked despite refused MCP config") - - for relative_path in ( - Path(".cursor/mcp.json"), - Path(".mcp.json"), - Path("mcp.json"), - ): - (repo / relative_path).unlink(missing_ok=True) - try: - run_cursor(args, repo, "review hostile patch") - except SystemExit as exc: - if "project-local permission config" not in str(exc): - raise - else: - raise SystemExit("cursor isolation self-test failed: local permission config should be refused") - (repo / ".cursor" / "cli.json").unlink() - run_cursor(args, repo, "review hostile patch") - cursor_record = json.loads(record_path.read_text()) - cursor_argv = cursor_record["argv"] - for required in ["--print", "--output-format", "json"]: - if required not in cursor_argv: - raise SystemExit(f"cursor isolation self-test failed: missing {required}") - for forbidden in ["--workspace", "--trust"]: - if forbidden in cursor_argv: - raise SystemExit(f"cursor isolation self-test failed: unsupported flag present: {forbidden}") - for required in ["--mode", "ask", "--sandbox", "enabled"]: - if required not in cursor_argv: - raise SystemExit(f"cursor isolation self-test failed: missing {required}") - if Path(cursor_record["cwd"]).resolve() != repo.resolve(): - raise SystemExit("cursor isolation self-test failed: Cursor workspace cwd should be reviewed repo") - if cursor_record["stdin"] != "review hostile patch": - raise SystemExit("cursor isolation self-test failed: prompt not delivered over stdin") - if "review hostile patch" in cursor_argv: - raise SystemExit("cursor isolation self-test failed: prompt leaked into argv") - cursor_config = json.loads(cursor_record["cursor_config"]) - if cursor_config.get("permissions", {}).get("allow") != ["Read(**)"]: - raise SystemExit("cursor isolation self-test failed: read-only allowlist missing") - if cursor_config.get("permissions", {}).get("deny") != ["Shell(*)", "Write(**)", "Write(/**)"]: - raise SystemExit("cursor isolation self-test failed: shell/write denylist missing") + raise SystemExit("cursor isolation self-test failed: disabled cursor was invoked") os.environ["AUTOREVIEW_FAKE_CLAUDE_VERSION"] = "2.1.168 (Claude Code)" try: @@ -2753,7 +3714,7 @@ def self_test_cursor_jsonl_parser() -> int: result_text = { "type": "result", - "result": "prefix\n```json\n" + json.dumps(report) + "\n```", + "result": "```json\n" + json.dumps(report) + "\n```", "session_id": "session", "request_id": "request", } @@ -2776,8 +3737,12 @@ def self_test_cursor_jsonl_parser() -> int: else: raise SystemExit("cursor parser self-test failed: assistant draft masked bad result") - if extract_json("analysis before " + json.dumps(report) + " after") != report: - raise SystemExit("cursor parser self-test failed for embedded JSON") + try: + extract_json("analysis before " + json.dumps(report) + " after") + except SystemExit: + pass + else: + raise SystemExit("cursor parser self-test failed: embedded JSON was accepted") print("autoreview cursor jsonl parser self-test: ok") return 0 @@ -2814,23 +3779,16 @@ def _assert_opencode_permission(web_search: bool) -> None: raise SystemExit(f"opencode isolation self-test failed: {disabled_tool} tool not disabled") if permission.get("*") != "deny": raise SystemExit("opencode isolation self-test failed: default deny missing") - read_permission = permission.get("read") - if not isinstance(read_permission, dict): - raise SystemExit("opencode isolation self-test failed: read rules missing") - expected_read_rules = { - "*": "allow", - "*.env": "ask", - "*.env.*": "ask", - "*.env.example": "allow", - } - for pattern, action in expected_read_rules.items(): - if read_permission.get(pattern) != action: - raise SystemExit(f"opencode isolation self-test failed: read {pattern} must be {action}") + for filesystem_tool in ("read", "grep", "glob"): + if permission.get(filesystem_tool) != "deny": + raise SystemExit( + f"opencode isolation self-test failed: {filesystem_tool} must be denied" + ) expected_web = "allow" if web_search else "deny" if permission.get("websearch") != expected_web: raise SystemExit(f"opencode isolation self-test failed: websearch must be {expected_web} when web_search={web_search}") - if permission.get("webfetch") != expected_web: - raise SystemExit(f"opencode isolation self-test failed: webfetch must be {expected_web} when web_search={web_search}") + if permission.get("webfetch") != "deny": + raise SystemExit("opencode isolation self-test failed: webfetch must stay denied") def self_test_opencode_isolation() -> None: @@ -3027,12 +3985,20 @@ def validate_report(report: dict[str, Any], repo: Path, changed_paths: set[str], location = finding.get("code_location") if not isinstance(location, dict): raise SystemExit(f"finding {index} missing code_location") - rel = str(location.get("file_path", "")).strip() - line = location.get("line") - if not rel or not isinstance(line, int) or line < 1: + raw_rel = str(location.get("file_path", "")).strip() + if not raw_rel: raise SystemExit(f"finding {index} has invalid location: {location}") - if Path(rel).is_absolute() or ".." in Path(rel).parts: + normalized_rel = raw_rel if raw_rel in changed_paths else raw_rel.replace("\\", "/") + while normalized_rel.startswith("./"): + normalized_rel = normalized_rel[2:] + rel_path = PurePosixPath(normalized_rel) + rel = rel_path.as_posix() + line = location.get("line") + if not isinstance(line, int) or line < 1: + raise SystemExit(f"finding {index} has invalid location: {location}") + if rel_path.is_absolute() or ".." in rel_path.parts or re.match(r"^[A-Za-z]:/", rel): raise SystemExit(f"finding {index} uses invalid file path: {rel}") + location["file_path"] = rel if rel not in changed_paths: ignored_findings.append((index, finding, rel, line)) continue @@ -3123,14 +4089,14 @@ def parse_args() -> argparse.Namespace: parser.add_argument("--base") parser.add_argument("--commit", default="HEAD") parser.add_argument("--engine", choices=ENGINE_CHOICES, default=os.environ.get("AUTOREVIEW_ENGINE", "codex")) - parser.add_argument("--reviewers", help="Comma-separated review panel, e.g. codex,claude,cursor or codex:gpt-5.6-sol:high.") + parser.add_argument("--reviewers", help="Comma-separated review panel, e.g. codex,claude,pi or codex:gpt-5.6-sol:high.") parser.add_argument("--panel", action="store_true", help="Run a Codex/Claude review panel unless --engine changes the first reviewer.") parser.add_argument( "--model", action="append", - help="Model for all reviewers or engine=model. Repeatable. Defaults: codex=gpt-5.6-sol, claude=claude-fable-5.", + help="Model for all reviewers or engine=model. Repeatable. Defaults: codex=gpt-5.6-sol with an access-only gpt-5.6-terra retry, claude=claude-fable-5.", ) - parser.add_argument("--thinking", action="append", help="Thinking/effort for all reviewers or engine=level. Repeatable. Codex: none, minimal, low, medium, high, xhigh. Claude: low, medium, high, xhigh, max. Droid: off, none, low, medium, high. Pi: off, minimal, low, medium, high, xhigh. OpenCode: minimal, low, medium, high, max. Cursor: none.") + parser.add_argument("--thinking", action="append", help="Thinking/effort for all reviewers or engine=level. Repeatable. Codex: none, minimal, low, medium, high, xhigh, max. Claude: low, medium, high, xhigh, max. Droid: off, none, low, medium, high. Pi: off, minimal, low, medium, high, xhigh. OpenCode: minimal, low, medium, high, max. Cursor: none.") parser.add_argument( "--fallback-model", action="append", @@ -3141,7 +4107,7 @@ def parse_args() -> argparse.Namespace: parser.add_argument( "--codex-config", action="append", - help='Extra Codex "-c key=value" config override (TOML value), codex reviewer only. Repeatable. Env default: AUTOREVIEW_CODEX_CONFIG (semicolon-separated), e.g. service_tier="fast". Reviewed-repo isolation flags still take precedence.', + help='Safe Codex model/response tuning "-c key=value" override (TOML value), codex reviewer only. Repeatable. Capability-, command-, and path-bearing keys are refused. Env default: AUTOREVIEW_CODEX_CONFIG (semicolon-separated), e.g. service_tier="fast".', ) parser.add_argument( "--codex-speed", @@ -3160,7 +4126,7 @@ def parse_args() -> argparse.Namespace: ) parser.add_argument("--opencode-bin", default=os.environ.get("OPENCODE_BIN", "opencode")) parser.add_argument("--pi-bin", default=os.environ.get("PI_BIN", "pi")) - parser.add_argument("--no-tools", dest="tools", action="store_false", default=True, help="Disable tools for engines that support it. Codex, copilot, opencode, and cursor reject no-tools review.") + parser.add_argument("--no-tools", dest="tools", action="store_false", default=True, help="Disable tools for engines that support it. Codex, Droid, copilot, opencode, and cursor reject no-tools review.") parser.add_argument("--self-test", action="store_true", help="Run deterministic local autoreview self-tests.") parser.add_argument("--self-test-opencode-jsonl-parser", action="store_true", help=argparse.SUPPRESS) parser.add_argument("--self-test-opencode-isolation", action="store_true", help=argparse.SUPPRESS) @@ -3172,7 +4138,7 @@ def parse_args() -> argparse.Namespace: "--claude-allowed-tools", default=os.environ.get( "AUTOREVIEW_CLAUDE_TOOLS", - "Read,Grep,Glob,WebSearch,WebFetch", + "WebSearch", ), ) parser.add_argument("--prompt", action="append", help="Additional review instruction text.") @@ -3184,20 +4150,20 @@ def parse_args() -> argparse.Namespace: "--stream-engine-output", action="store_true", default=os.environ.get("AUTOREVIEW_STREAM_ENGINE_OUTPUT") == "1", - help="Stream review engine output while preserving buffered output for validation. Codex, Claude, and Cursor filter noisy tool/status chatter.", + help="Stream review engine output while preserving buffered output for validation. Codex and Claude filter noisy tool/status chatter.", ) parser.add_argument( "--cursor-allow-workspace-instructions", dest="cursor_allow_workspace_instructions", action="store_true", default=None, - help="Required opt-in for every Cursor review. Confirms the repository and its project-local resources are trusted.", + help="Legacy compatibility flag. Cursor review is unavailable because reads cannot be confined to the repository.", ) parser.add_argument( "--no-cursor-allow-workspace-instructions", dest="cursor_allow_workspace_instructions", action="store_false", - help="Refuse Cursor review because project-local resource loading cannot be disabled.", + help="Legacy compatibility flag. Cursor review remains unavailable.", ) parser.add_argument("--parallel-tests", help="Run a test command concurrently with review; failure fails the helper.") parser.add_argument( @@ -3361,6 +4327,7 @@ def reviewer_args(args: argparse.Namespace) -> list[argparse.Namespace]: or global_thinking or env_thinking_by_engine.get(engine) or env_global_thinking + or DEFAULT_THINKING_BY_ENGINE.get(engine) ) if engine == "claude": fallback_model = ( @@ -3369,6 +4336,8 @@ def reviewer_args(args: argparse.Namespace) -> list[argparse.Namespace]: or env_fallback_by_engine.get(engine) or env_global_fallback ) + elif engine == "codex" and model == DEFAULT_MODEL_BY_ENGINE["codex"]: + fallback_model = DEFAULT_CODEX_ACCESS_FALLBACK_MODEL else: fallback_model = None if thinking and thinking not in THINKING_LEVELS_BY_ENGINE[engine]: @@ -3523,18 +4492,36 @@ def preserve_env(keys: list[str]): def self_test_config_defaults() -> None: keys = [ "AUTOREVIEW_MODEL", - "AUTOREVIEW_CODEX_MODEL", - "AUTOREVIEW_CLAUDE_MODEL", "AUTOREVIEW_THINKING", - "AUTOREVIEW_CODEX_THINKING", - "AUTOREVIEW_CLAUDE_THINKING", + "AUTOREVIEW_FALLBACK_MODEL", "AUTOREVIEW_CODEX_CONFIG", "AUTOREVIEW_CODEX_SPEED", + *( + f"AUTOREVIEW_{engine.upper()}_{suffix}" + for engine in ENGINES + for suffix in ("MODEL", "THINKING", "FALLBACK_MODEL") + ), ] with preserve_env(keys): + for key in keys: + os.environ.pop(key, None) default_codex = reviewer_args(reviewer_test_args(engine="codex"))[0] if default_codex.model != "gpt-5.6-sol": raise SystemExit(f"self-test config defaults failed: default codex model={default_codex.model!r}") + if default_codex.fallback_model != "gpt-5.6-terra": + raise SystemExit( + f"self-test config defaults failed: default codex fallback={default_codex.fallback_model!r}" + ) + explicit_sol = reviewer_args(reviewer_test_args(engine="codex", model=["gpt-5.6-sol"]))[0] + if explicit_sol.fallback_model != "gpt-5.6-terra": + raise SystemExit( + f"self-test config defaults failed: explicit Sol access fallback={explicit_sol.fallback_model!r}" + ) + if default_codex.thinking != "high": + raise SystemExit(f"self-test config defaults failed: default codex thinking={default_codex.thinking!r}") + max_effort = reviewer_args(reviewer_test_args(engine="codex", thinking=["max"]))[0] + if max_effort.thinking != "max": + raise SystemExit("self-test config defaults failed: Codex max thinking should be accepted") default_claude = reviewer_args(reviewer_test_args(engine="claude"))[0] if default_claude.model != "claude-fable-5": raise SystemExit(f"self-test config defaults failed: default claude model={default_claude.model!r}") @@ -3583,6 +4570,18 @@ def self_test_config_defaults() -> None: rejected = "invalid Codex config override" in str(error) if not rejected: raise SystemExit("self-test config defaults failed: malformed codex config override accepted") + rejected = False + try: + codex_config_overrides( + reviewer_test_args( + engine="codex", + codex_config=['mcp_servers.review.command="touch /tmp/owned"'], + ) + ) + except SystemExit as error: + rejected = "unsafe Codex config override refused" in str(error) + if not rejected: + raise SystemExit("self-test config defaults failed: capability-bearing codex config override accepted") os.environ.pop("AUTOREVIEW_CODEX_CONFIG") try: reviewer_args(reviewer_test_args(engine="claude", codex_config=['service_tier="fast"'])) @@ -3617,19 +4616,25 @@ def self_test_config_defaults() -> None: def self_test_fallback_scope() -> None: keys = [ + "AUTOREVIEW_MODEL", "AUTOREVIEW_FALLBACK_MODEL", + "AUTOREVIEW_CODEX_MODEL", "AUTOREVIEW_CLAUDE_FALLBACK_MODEL", "AUTOREVIEW_CODEX_FALLBACK_MODEL", ] with preserve_env(keys): + for key in keys: + os.environ.pop(key, None) os.environ["AUTOREVIEW_FALLBACK_MODEL"] = "env-global-fallback" os.environ["AUTOREVIEW_CLAUDE_FALLBACK_MODEL"] = "env-claude-fallback" base = reviewer_test_args(reviewers="codex,claude") reviewers = reviewer_args(base) codex = next(r for r in reviewers if r.engine == "codex") claude = next(r for r in reviewers if r.engine == "claude") - if codex.fallback_model is not None: - raise SystemExit("self-test fallback scope failed: codex should ignore AUTOREVIEW_FALLBACK_MODEL") + if codex.fallback_model != "gpt-5.6-terra": + raise SystemExit( + f"self-test fallback scope failed: codex access fallback={codex.fallback_model!r}" + ) if claude.fallback_model != "env-claude-fallback": raise SystemExit(f"self-test fallback scope failed: claude fallback={claude.fallback_model!r}") os.environ.pop("AUTOREVIEW_CLAUDE_FALLBACK_MODEL") @@ -3653,7 +4658,7 @@ def self_test_fallback_scope() -> None: panel = reviewer_args(reviewer_test_args(reviewers="codex,claude", fallback_model=["cli-global"])) panel_codex = next(r for r in panel if r.engine == "codex") panel_claude = next(r for r in panel if r.engine == "claude") - if panel_codex.fallback_model is not None or panel_claude.fallback_model != "cli-global": + if panel_codex.fallback_model != "gpt-5.6-terra" or panel_claude.fallback_model != "cli-global": raise SystemExit("self-test fallback scope failed: CLI global fallback should apply only to Claude panel reviewers") try: reviewer_args(reviewer_test_args(engine="codex", fallback_model=["cli-global"])) diff --git a/.agents/skills/autoreview/scripts/autoreview_test.py b/.agents/skills/autoreview/scripts/autoreview_test.py index 2c4fa8593813..90f823ba1075 100644 --- a/.agents/skills/autoreview/scripts/autoreview_test.py +++ b/.agents/skills/autoreview/scripts/autoreview_test.py @@ -125,22 +125,11 @@ class AutoreviewCompatibilityTests(unittest.TestCase): os.environ[key] = value cls.home_dir.cleanup() - def test_harness_opts_both_cursor_aliases_into_trusted_fixture(self) -> None: + def test_harness_rejects_disabled_cursor_engine(self) -> None: harness_path = SCRIPT_PATH.with_name("test-review-harness.py") namespace = runpy.run_path(str(harness_path)) - commands: list[list[str]] = [] - run_reviews = namespace["run_reviews"] - with mock.patch.dict( - run_reviews.__globals__, - { - "run": lambda command, _cwd: commands.append(command), - "validate_prompt_policy": lambda _repo, _autoreview: None, - }, - ), tempfile.TemporaryDirectory(prefix="autoreview-harness-test.") as tmpdir: - run_reviews(Path(tmpdir), SCRIPT_PATH.parent, "benign", ["cursor", "cursor-agent"]) - self.assertEqual(len(commands), 2) - for command in commands: - self.assertIn("--cursor-allow-workspace-instructions", command) + with self.assertRaises(SystemExit): + namespace["parse_args"](["--engine", "cursor"]) def test_cursor_agent_bin_cli_alias(self) -> None: with mock.patch.object( @@ -175,8 +164,226 @@ class AutoreviewCompatibilityTests(unittest.TestCase): ) def test_codex_config_status_exposes_keys_only(self) -> None: - args = argparse.Namespace(codex_config=['model_provider="private-value"']) - self.assertEqual(AUTOREVIEW.codex_config_keys(args), ["model_provider"]) + args = argparse.Namespace(codex_config=['model_verbosity="low"']) + self.assertEqual(AUTOREVIEW.codex_config_keys(args), ["model_verbosity"]) + + def test_codex_retries_terra_after_sol_access_failure(self) -> None: + args = argparse.Namespace( + codex_bin="codex", + codex_config=None, + codex_speed=None, + fallback_model="gpt-5.6-terra", + model="gpt-5.6-sol", + stream_engine_output=False, + thinking="high", + tools=True, + web_search=False, + ) + models: list[str] = [] + + def fake_run(command: list[str], *_args: object, **_kwargs: object) -> subprocess.CompletedProcess[str]: + model = command[command.index("--model") + 1] + models.append(model) + if model == "gpt-5.6-sol": + return subprocess.CompletedProcess( + command, + 1, + "", + "The model `gpt-5.6-sol` does not exist or you do not have access to it.", + ) + output_path = Path(command[command.index("--output-last-message") + 1]) + output_path.write_text(json.dumps(FINAL_REPORT)) + return subprocess.CompletedProcess(command, 0, "", "") + + with tempfile.TemporaryDirectory(prefix="autoreview-codex-fallback.") as tmpdir, mock.patch.object( + AUTOREVIEW, + "resolve_command", + return_value="/usr/bin/codex", + ), mock.patch.object(AUTOREVIEW, "codex_auth_config_flags", return_value=[]), mock.patch.object( + AUTOREVIEW, + "run_with_heartbeat", + side_effect=fake_run, + ): + output = AUTOREVIEW.run_codex(args, Path(tmpdir), "review") + + self.assertEqual(json.loads(output), FINAL_REPORT) + self.assertEqual(models, ["gpt-5.6-sol", "gpt-5.6-terra"]) + + def test_codex_runs_outside_repo_with_bundle_only_workspace(self) -> None: + args = argparse.Namespace( + codex_bin="codex", + codex_config=None, + codex_speed=None, + fallback_model=None, + model="gpt-5.6-sol", + stream_engine_output=False, + thinking="high", + tools=True, + web_search=False, + ) + observed: dict[str, object] = {} + + def fake_run( + command: list[str], + cwd: Path, + *_args: object, + **_kwargs: object, + ) -> subprocess.CompletedProcess[str]: + observed["cwd"] = cwd + observed["command_cwd"] = Path(command[command.index("-C") + 1]) + observed["workspace_entries"] = list(cwd.iterdir()) + output_path = Path(command[command.index("--output-last-message") + 1]) + output_path.write_text(json.dumps(FINAL_REPORT)) + return subprocess.CompletedProcess(command, 0, "", "") + + with tempfile.TemporaryDirectory(prefix="autoreview-codex-workspace-test.") as tmpdir: + repo = Path(tmpdir) + (repo / ".env").write_text("OPENAI_API_KEY=ignored-secret\n") + with mock.patch.object( + AUTOREVIEW, + "resolve_command", + return_value="/usr/bin/codex", + ), mock.patch.object( + AUTOREVIEW, + "codex_auth_config_flags", + return_value=[], + ), mock.patch.object( + AUTOREVIEW, + "run_with_heartbeat", + side_effect=fake_run, + ): + output = AUTOREVIEW.run_codex(args, repo, "review") + + self.assertEqual(json.loads(output), FINAL_REPORT) + observed_cwd = observed["cwd"] + command_cwd = observed["command_cwd"] + self.assertIsInstance(observed_cwd, Path) + self.assertIsInstance(command_cwd, Path) + assert isinstance(observed_cwd, Path) + assert isinstance(command_cwd, Path) + self.assertNotEqual(observed_cwd.resolve(), repo.resolve()) + self.assertEqual(observed_cwd, command_cwd) + self.assertEqual(observed["workspace_entries"], []) + + def test_codex_does_not_fallback_after_unrelated_failure(self) -> None: + args = argparse.Namespace( + codex_bin="codex", + codex_config=None, + codex_speed=None, + fallback_model="gpt-5.6-terra", + model="gpt-5.6-sol", + stream_engine_output=False, + thinking="high", + tools=True, + web_search=False, + ) + models: list[str] = [] + + def fake_run(command: list[str], *_args: object, **_kwargs: object) -> subprocess.CompletedProcess[str]: + models.append(command[command.index("--model") + 1]) + return subprocess.CompletedProcess(command, 1, "", "network timeout") + + with tempfile.TemporaryDirectory(prefix="autoreview-codex-fallback.") as tmpdir, mock.patch.object( + AUTOREVIEW, + "resolve_command", + return_value="/usr/bin/codex", + ), mock.patch.object(AUTOREVIEW, "codex_auth_config_flags", return_value=[]), mock.patch.object( + AUTOREVIEW, + "run_with_heartbeat", + side_effect=fake_run, + ): + with self.assertRaisesRegex(SystemExit, "network timeout"): + AUTOREVIEW.run_codex(args, Path(tmpdir), "review") + + self.assertEqual(models, ["gpt-5.6-sol"]) + + def test_codex_does_not_fallback_after_model_capacity_failure(self) -> None: + args = argparse.Namespace( + codex_bin="codex", + codex_config=None, + codex_speed=None, + fallback_model="gpt-5.6-terra", + model="gpt-5.6-sol", + stream_engine_output=False, + thinking="high", + tools=True, + web_search=False, + ) + models: list[str] = [] + + def fake_run(command: list[str], *_args: object, **_kwargs: object) -> subprocess.CompletedProcess[str]: + models.append(command[command.index("--model") + 1]) + return subprocess.CompletedProcess( + command, + 1, + "", + "model_not_available: gpt-5.6-sol is temporarily unavailable due to capacity", + ) + + with tempfile.TemporaryDirectory(prefix="autoreview-codex-fallback.") as tmpdir, mock.patch.object( + AUTOREVIEW, + "resolve_command", + return_value="/usr/bin/codex", + ), mock.patch.object(AUTOREVIEW, "codex_auth_config_flags", return_value=[]), mock.patch.object( + AUTOREVIEW, + "run_with_heartbeat", + side_effect=fake_run, + ): + with self.assertRaisesRegex(SystemExit, "temporarily unavailable"): + AUTOREVIEW.run_codex(args, Path(tmpdir), "review") + + self.assertEqual(models, ["gpt-5.6-sol"]) + + def test_codex_access_fallback_ignores_structured_output_text(self) -> None: + result = subprocess.CompletedProcess( + ["codex"], + 1, + '{"type":"agent_message","text":"gpt-5.6-sol does not exist or you do not have access"}', + '{"type":"agent_message","message":"gpt-5.6-sol does not exist or you do not have access"}', + ) + + self.assertFalse( + AUTOREVIEW.codex_model_access_failure(result, "gpt-5.6-sol") + ) + + def test_codex_access_fallback_accepts_terminal_error_event(self) -> None: + result = subprocess.CompletedProcess( + ["codex"], + 1, + '{"type":"error","message":"gpt-5.6-sol does not exist or you do not have access"}', + "", + ) + + self.assertTrue( + AUTOREVIEW.codex_model_access_failure(result, "gpt-5.6-sol") + ) + + def test_codex_access_fallback_accepts_account_model_list_error(self) -> None: + result = subprocess.CompletedProcess( + ["codex"], + 1, + "", + ( + "The model gpt-5.6-sol does not appear in the list of models " + "available to your account" + ), + ) + + self.assertTrue( + AUTOREVIEW.codex_model_access_failure(result, "gpt-5.6-sol") + ) + + def test_codex_access_fallback_ignores_plain_stdout(self) -> None: + message = "gpt-5.6-sol does not exist or you do not have access" + stdout_result = subprocess.CompletedProcess(["codex"], 1, message, "") + stderr_result = subprocess.CompletedProcess(["codex"], 1, "", message) + + self.assertFalse( + AUTOREVIEW.codex_model_access_failure(stdout_result, "gpt-5.6-sol") + ) + self.assertTrue( + AUTOREVIEW.codex_model_access_failure(stderr_result, "gpt-5.6-sol") + ) def test_extract_json_accepts_dict_result_payload(self) -> None: payload = { @@ -188,32 +395,14 @@ class AutoreviewCompatibilityTests(unittest.TestCase): } self.assertEqual(AUTOREVIEW.extract_json(json.dumps(payload)), FINAL_REPORT) - def test_extract_json_accepts_result_string_with_preamble(self) -> None: + def test_extract_json_rejects_result_string_with_preamble(self) -> None: payload = { "type": "result", "subtype": "success", "result": "Inspecting the diff first.\n" + json.dumps(FINAL_REPORT), } - self.assertEqual(AUTOREVIEW.extract_json(json.dumps(payload)), FINAL_REPORT) - - def test_extract_findings_json_from_text_prefers_last_findings_object(self) -> None: - later_report = { - "findings": [ - { - "title": "Later finding", - "body": "later", - "priority": "P2", - "confidence": 0.8, - "category": "bug", - "code_location": {"file_path": "later.js", "line": 2}, - } - ], - "overall_correctness": "patch is incorrect", - "overall_explanation": "later", - "overall_confidence": 0.8, - } - text = f"{json.dumps(FINAL_REPORT)} separator {json.dumps(later_report)}" - self.assertEqual(AUTOREVIEW.extract_findings_json_from_text(text), later_report) + with self.assertRaisesRegex(SystemExit, "result was not structured JSON"): + AUTOREVIEW.extract_json(json.dumps(payload)) def test_retry_filter_only_matches_parse_failures(self) -> None: self.assertTrue(AUTOREVIEW.is_structured_output_failure("review engine returned non-JSON output: nope")) @@ -235,7 +424,7 @@ class AutoreviewCompatibilityTests(unittest.TestCase): ) with self.assertRaises(SystemExit) as exc_info: AUTOREVIEW.run_cursor(args, repo, "prompt") - self.assertIn("requires --cursor-allow-workspace-instructions", str(exc_info.exception)) + self.assertIn("cursor engine is unavailable", str(exc_info.exception)) def test_cursor_local_mcp_requires_explicit_approval(self) -> None: with tempfile.TemporaryDirectory(prefix="autoreview-cursor-test.") as tmpdir: @@ -253,7 +442,7 @@ class AutoreviewCompatibilityTests(unittest.TestCase): ) with self.assertRaises(SystemExit) as exc_info: AUTOREVIEW.run_cursor(args, repo, "prompt") - self.assertIn("cursor engine refused project-local MCP config", str(exc_info.exception)) + self.assertIn("cursor engine is unavailable", str(exc_info.exception)) def test_cursor_local_hooks_are_always_refused(self) -> None: with tempfile.TemporaryDirectory(prefix="autoreview-cursor-test.") as tmpdir: @@ -271,7 +460,7 @@ class AutoreviewCompatibilityTests(unittest.TestCase): ) with self.assertRaises(SystemExit) as exc_info: AUTOREVIEW.run_cursor(args, repo, "prompt") - self.assertIn("cursor engine refused project-local hooks", str(exc_info.exception)) + self.assertIn("cursor engine is unavailable", str(exc_info.exception)) def test_cursor_local_permissions_are_always_refused(self) -> None: with tempfile.TemporaryDirectory(prefix="autoreview-cursor-test.") as tmpdir: @@ -289,15 +478,14 @@ class AutoreviewCompatibilityTests(unittest.TestCase): ) with self.assertRaises(SystemExit) as exc_info: AUTOREVIEW.run_cursor(args, repo, "prompt") - self.assertIn("cursor engine refused project-local permission config", str(exc_info.exception)) + self.assertIn("cursor engine is unavailable", str(exc_info.exception)) - def test_cursor_command_uses_current_print_contract(self) -> None: + def test_cursor_is_disabled_without_repo_only_read_sandbox(self) -> None: with tempfile.TemporaryDirectory(prefix="autoreview-cursor-test.") as tmpdir: root = Path(tmpdir) repo = root / "repo" repo.mkdir() cursor_bin = root / "cursor-agent" - record_path = root / "record.json" AUTOREVIEW.write_executable(cursor_bin, AUTOREVIEW.fake_cursor_script()) args = argparse.Namespace( thinking=None, @@ -308,29 +496,11 @@ class AutoreviewCompatibilityTests(unittest.TestCase): model=None, stream_engine_output=False, ) - old_record = os.environ.get("AUTOREVIEW_FAKE_RECORD") - try: - os.environ["AUTOREVIEW_FAKE_RECORD"] = str(record_path) - AUTOREVIEW.run_cursor(args, repo, "prompt") - finally: - if old_record is None: - os.environ.pop("AUTOREVIEW_FAKE_RECORD", None) - else: - os.environ["AUTOREVIEW_FAKE_RECORD"] = old_record - record = json.loads(record_path.read_text()) - self.assertEqual(Path(record["cwd"]).resolve(), repo.resolve()) - self.assertEqual(record["stdin"], "prompt") - self.assertIn("--print", record["argv"]) - self.assertIn("--output-format", record["argv"]) - self.assertIn("json", record["argv"]) - self.assertIn("--mode", record["argv"]) - self.assertIn("ask", record["argv"]) - self.assertIn("--sandbox", record["argv"]) - self.assertIn("enabled", record["argv"]) - for unsupported in ("--workspace", "--trust"): - self.assertNotIn(unsupported, record["argv"]) + with mock.patch.object(AUTOREVIEW, "cursor_global_hook_paths", return_value=[]): + with self.assertRaisesRegex(SystemExit, "Cursor read permissions"): + AUTOREVIEW.run_cursor(args, repo, "prompt") - def test_cursor_engine_runs_end_to_end_with_sanitized_environment(self) -> None: + def test_cursor_engine_fails_closed_end_to_end(self) -> None: with tempfile.TemporaryDirectory(prefix="autoreview-cursor-e2e.") as tmpdir: root = Path(tmpdir) repo = root / "repo" @@ -356,6 +526,8 @@ class AutoreviewCompatibilityTests(unittest.TestCase): "NODE_OPTIONS": "--require=hostile.js", "PYTHONPATH": str(root / "hostile-python"), "PATH": f"{repo}{os.pathsep}{env.get('PATH', '')}", + "HOME": str(root), + "USERPROFILE": str(root), } ) result = subprocess.run( @@ -377,33 +549,9 @@ class AutoreviewCompatibilityTests(unittest.TestCase): check=False, ) - self.assertEqual(result.returncode, 0, result.stderr) - self.assertIn("autoreview clean: no accepted/actionable findings reported", result.stdout) - record = json.loads(record_path.read_text()) - self.assertEqual(Path(record["cwd"]).resolve(), repo.resolve()) - self.assertIn("diff --git a/example.txt b/example.txt", record["stdin"]) - self.assertIn("-before", record["stdin"]) - self.assertIn("+after", record["stdin"]) - self.assertEqual(record["environment"]["GIT_CONFIG_GLOBAL"], None) - self.assertEqual(record["environment"]["NODE_OPTIONS"], None) - self.assertEqual(record["environment"]["PYTHONPATH"], None) - self.assertNotIn(str(repo), record["environment"]["PATH"].split(os.pathsep)) - cursor_config_dir = Path(record["environment"]["CURSOR_CONFIG_DIR"]) - self.assertFalse(cursor_config_dir.exists()) - cursor_config = json.loads(record["cursor_config"]) - self.assertEqual(cursor_config["permissions"]["allow"], ["Read(**)"]) - self.assertEqual( - cursor_config["permissions"]["deny"], - ["Shell(*)", "Write(**)", "Write(/**)"], - ) - - invocations = [json.loads(line) for line in (root / "cursor-invocations.jsonl").read_text().splitlines()] - help_invocation = next(invocation for invocation in invocations if "--help" in invocation["argv"]) - self.assertNotEqual(Path(help_invocation["cwd"]).resolve(), repo.resolve()) - self.assertEqual(help_invocation["environment"]["GIT_CONFIG_GLOBAL"], None) - self.assertEqual(help_invocation["environment"]["NODE_OPTIONS"], None) - self.assertEqual(help_invocation["environment"]["PYTHONPATH"], None) - self.assertNotIn(str(repo), help_invocation["environment"]["PATH"].split(os.pathsep)) + self.assertNotEqual(result.returncode, 0) + self.assertIn("Cursor read permissions", result.stderr) + self.assertFalse(record_path.exists()) if __name__ == "__main__": diff --git a/.agents/skills/autoreview/scripts/test-review-harness.ps1 b/.agents/skills/autoreview/scripts/test-review-harness.ps1 index cb905b801fdb..4b859ca2dc83 100644 --- a/.agents/skills/autoreview/scripts/test-review-harness.ps1 +++ b/.agents/skills/autoreview/scripts/test-review-harness.ps1 @@ -3,7 +3,7 @@ param( [ValidateSet('malicious', 'benign')] [string] $Fixture, - [ValidateSet('codex', 'claude', 'droid', 'copilot', 'pi', 'opencode', 'cursor', 'cursor-agent')] + [ValidateSet('codex', 'claude', 'pi')] [string[]] $Engine, [Alias('h')] diff --git a/.agents/skills/autoreview/scripts/test-review-harness.py b/.agents/skills/autoreview/scripts/test-review-harness.py index 00ff8093538a..5077d40f731d 100644 --- a/.agents/skills/autoreview/scripts/test-review-harness.py +++ b/.agents/skills/autoreview/scripts/test-review-harness.py @@ -13,7 +13,7 @@ from collections.abc import Callable from pathlib import Path -ENGINES = ("codex", "claude", "droid", "copilot", "pi", "opencode", "cursor", "cursor-agent") +ENGINES = ("codex", "claude", "pi") DEFAULT_ENGINES = ("codex", "claude") MALICIOUS_INITIAL = """export function uploadPath(name) { @@ -175,10 +175,6 @@ def run_reviews(repo: Path, script_dir: Path, fixture: str, engines: list[str]) "--prompt", MALICIOUS_PROMPT if fixture == "malicious" else BENIGN_PROMPT, ] - if engine in {"cursor", "cursor-agent"}: - # The harness owns this temporary fixture, so it can make the - # trusted-workspace assertion required by Cursor reviews. - command.append("--cursor-allow-workspace-instructions") if fixture == "malicious": command.extend(["--require-finding", "command", "--expect-findings"]) run(command, repo) diff --git a/.agents/skills/autoreview/tests/test_autoreview_hardening.py b/.agents/skills/autoreview/tests/test_autoreview_hardening.py index 74faeef986a9..a30cd1444ada 100644 --- a/.agents/skills/autoreview/tests/test_autoreview_hardening.py +++ b/.agents/skills/autoreview/tests/test_autoreview_hardening.py @@ -2,6 +2,7 @@ from __future__ import annotations import argparse +import json import os import runpy import subprocess @@ -50,19 +51,33 @@ def init_repo(tempdir: Path) -> Path: return repo +def realistic_secret_value() -> str: + return "A7f9K2m4Q8v6" + "N3x5R1p0T9z8" + + class AutoreviewHardeningTests(unittest.TestCase): def setUp(self) -> None: self.helper = load_helper() + def test_powershell_harness_exposes_runnable_engines_only(self) -> None: + harness = SCRIPT.with_name("test-review-harness.ps1").read_text(encoding="utf-8") + + self.assertIn("[ValidateSet('codex', 'claude', 'pi')]", harness) + for disabled_engine in ("droid", "copilot", "opencode", "cursor"): + self.assertNotIn(f"'{disabled_engine}'", harness) + def test_local_bundle_blocks_sensitive_untracked_file(self) -> None: - with tempfile.TemporaryDirectory() as tempdir: - repo = init_repo(Path(tempdir)) - (repo / ".env").write_text("placeholder=true\n", encoding="utf-8") + for rel in (".env", "tokens/session.dat", "secrets/local.py"): + with self.subTest(rel=rel), tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + path = repo / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("placeholder=true\n", encoding="utf-8") - with self.assertRaisesRegex(SystemExit, "untracked sensitive files"): - self.helper["local_bundle"](repo) + with self.assertRaisesRegex(SystemExit, "untracked sensitive files"): + self.helper["local_bundle"](repo) - def test_local_bundle_omits_safe_untracked_binary_content(self) -> None: + def test_local_bundle_marks_untracked_binary_input_incomplete(self) -> None: with tempfile.TemporaryDirectory() as tempdir: repo = init_repo(Path(tempdir)) (repo / "image.bin").write_bytes(b"\x89PNG\r\n\0binary-content") @@ -70,7 +85,191 @@ class AutoreviewHardeningTests(unittest.TestCase): bundle, truncated = self.helper["local_bundle"](repo) self.assertIn("## image.bin\n[binary file omitted]", bundle) + self.assertTrue(truncated) + + def test_local_bundle_rejects_non_utf8_untracked_text(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + (repo / "latin.py").write_bytes(b"print('caf\xe9')\n") + + with self.assertRaisesRegex(SystemExit, "non-UTF-8 file"): + self.helper["local_bundle"](repo) + + def test_local_bundle_uses_validated_untracked_snapshot(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + (repo / "notes.txt").write_text("review me\n", encoding="utf-8") + original_read_prefix = self.helper["read_prefix"] + reads = 0 + + def read_once(path: Path, limit: int) -> tuple[bytes, bool]: + nonlocal reads + reads += 1 + if reads > 1: + raise AssertionError("untracked file was reopened after validation") + return original_read_prefix(path, limit) + + with mock.patch.dict( + self.helper["local_bundle"].__globals__, + {"read_prefix": read_once}, + ): + bundle, truncated = self.helper["local_bundle"](repo) + + self.assertIn("## notes.txt\nreview me", bundle) self.assertFalse(truncated) + self.assertEqual(reads, 1) + + def test_tracked_binary_changes_are_blocked_in_all_modes(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + binary = repo / "artifact.bin" + binary.write_bytes(b"\0base") + git(repo, "add", "artifact.bin") + git(repo, "commit", "-q", "-m", "base") + base = git(repo, "rev-parse", "HEAD").strip() + + binary.write_bytes(b"\0changed") + git(repo, "add", "artifact.bin") + with self.assertRaisesRegex(SystemExit, "refusing binary changes"): + self.helper["local_bundle"](repo) + + git(repo, "commit", "-q", "-m", "binary change") + with self.assertRaisesRegex(SystemExit, "refusing binary changes"): + self.helper["commit_bundle"](repo, "HEAD") + with self.assertRaisesRegex(SystemExit, "refusing binary changes"): + self.helper["branch_bundle"](repo, base) + + def test_gitlink_changes_are_blocked_in_all_modes(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + tracked = repo / "tracked.txt" + tracked.write_text("base\n", encoding="utf-8") + git(repo, "add", "tracked.txt") + git(repo, "commit", "-q", "-m", "base") + base = git(repo, "rev-parse", "HEAD").strip() + + git( + repo, + "update-index", + "--add", + "--cacheinfo", + f"160000,{base},vendor/dependency", + ) + with self.assertRaisesRegex(SystemExit, "gitlink/submodule changes"): + self.helper["local_bundle"](repo) + + git(repo, "commit", "-q", "-m", "add gitlink") + with self.assertRaisesRegex(SystemExit, "gitlink/submodule changes"): + self.helper["commit_bundle"](repo, "HEAD") + with self.assertRaisesRegex(SystemExit, "gitlink/submodule changes"): + self.helper["branch_bundle"](repo, base) + + def test_gitlink_guard_parses_combined_raw_modes(self) -> None: + raw_diff = ( + "::100644 100644 160000 " + + ("a" * 40) + + " " + + ("b" * 40) + + " " + + ("c" * 40) + + " MM\0vendor/dependency\0" + ) + + with self.assertRaisesRegex(SystemExit, "gitlink/submodule changes"): + self.helper["require_no_gitlink_diff"]("merge diff", raw_diff) + + def test_codex_config_rejects_capability_bearing_overrides(self) -> None: + for override in ( + 'mcp_servers.review.command="touch /tmp/owned"', + 'notify=["sh", "-c", "touch /tmp/owned"]', + 'model_instructions_file="/tmp/hostile.md"', + 'model_provider="credential-sink"', + 'hooks.PreToolUse.command="touch /tmp/owned"', + ): + with self.subTest(override=override), self.assertRaisesRegex( + SystemExit, + "unsafe Codex config override refused", + ): + self.helper["codex_config_overrides"]( + argparse.Namespace(codex_config=[override]) + ) + + def test_codex_config_accepts_safe_tuning_overrides(self) -> None: + args = argparse.Namespace( + codex_config=[ + 'service_tier="fast"', + 'model_verbosity="low"', + 'model_reasoning_summary="concise"', + ] + ) + + self.assertEqual( + self.helper["codex_config_overrides"](args), + args.codex_config, + ) + + def test_untracked_files_respect_trusted_global_excludes(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + root = Path(tempdir) + repo = init_repo(root) + home = root / "home" + home.mkdir() + excludes = root / "global-ignore" + excludes.write_text( + "ignored.local\n!settings.local\n", + encoding="utf-8", + ) + (home / ".gitconfig").write_text( + f"[core]\n\texcludesFile = {excludes.as_posix()}\n", + encoding="utf-8", + ) + (repo / "ignored.local").write_text("private notes\n", encoding="utf-8") + (repo / ".gitignore").write_text("settings.local\n", encoding="utf-8") + (repo / "settings.local").write_text("repo private\n", encoding="utf-8") + git(repo, "add", ".gitignore") + (repo / "visible.txt").write_text("review me\n", encoding="utf-8") + (repo / "hostile-gitconfig").write_text( + "[core]\n\texcludesFile = /does/not/exist\n", + encoding="utf-8", + ) + + with mock.patch.dict( + os.environ, + { + "HOME": str(home), + "USERPROFILE": str(home), + "GIT_CONFIG_GLOBAL": str(repo / "hostile-gitconfig"), + }, + ): + self.assertEqual( + self.helper["safe_untracked_files"](repo), + ["hostile-gitconfig", "visible.txt"], + ) + + def test_oversized_text_is_rejected_without_scanning_binary_tail(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + tail_secret = "\ntoken=" + "A" * 24 + "\n" + content = "x" * (64_000 * 3 - 4) + tail_secret + + untracked = repo / "untracked.txt" + untracked.write_text(content, encoding="utf-8") + with self.assertRaisesRegex(SystemExit, "file too large to scan safely"): + self.helper["safe_untracked_files"](repo) + + untracked.unlink() + binary = repo / "binary.bin" + binary.write_bytes(b"\0" + content.encode()) + self.assertEqual( + self.helper["safe_untracked_files"](repo), + ["binary.bin"], + ) + + binary.unlink() + evidence = repo / "evidence.txt" + evidence.write_text(content, encoding="utf-8") + with self.assertRaisesRegex(SystemExit, "file too large to scan safely"): + self.helper["validate_evidence_file"](repo, "evidence.txt", "--dataset") def test_branch_bundle_rejects_unsafe_or_unknown_base_before_diff(self) -> None: with tempfile.TemporaryDirectory() as tempdir: @@ -84,6 +283,26 @@ class AutoreviewHardeningTests(unittest.TestCase): with self.assertRaisesRegex(SystemExit, "unknown base ref"): self.helper["branch_bundle"](repo, "origin/main") + def test_commit_bundle_rejects_merge_commits(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + (repo / "base.txt").write_text("base\n", encoding="utf-8") + git(repo, "add", "base.txt") + git(repo, "commit", "-q", "-m", "base") + base_branch = git(repo, "branch", "--show-current").strip() + git(repo, "checkout", "-q", "-b", "side") + (repo / "side.txt").write_text("side\n", encoding="utf-8") + git(repo, "add", "side.txt") + git(repo, "commit", "-q", "-m", "side") + git(repo, "checkout", "-q", base_branch) + (repo / "main.txt").write_text("main\n", encoding="utf-8") + git(repo, "add", "main.txt") + git(repo, "commit", "-q", "-m", "main") + git(repo, "merge", "-q", "--no-ff", "side", "-m", "merge") + + with self.assertRaisesRegex(SystemExit, "does not accept merge commits"): + self.helper["commit_bundle"](repo, "HEAD") + def test_git_path_list_preserves_newline_filenames(self) -> None: if os.name == "nt": self.skipTest("Windows filesystems do not support newline path components") @@ -97,10 +316,401 @@ class AutoreviewHardeningTests(unittest.TestCase): self.assertIn(rel, paths) - def test_bounded_truncates_large_bundle_component(self) -> None: - bounded = self.helper["bounded"]("x" * 25, 10) + @unittest.skipUnless(sys.platform.startswith("linux"), "requires raw non-UTF-8 filename support") + def test_git_path_list_rejects_non_utf8_output(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + rel = os.fsdecode(b"invalid-\xff.txt") + (repo / rel).write_text("content\n", encoding="utf-8") + git(repo, "add", "--", rel) - self.assertEqual(bounded, "x" * 10 + "\n\n[truncated at 10 characters]\n") + with self.assertRaisesRegex(SystemExit, "non-UTF-8 Git output"): + self.helper["git_path_list"](repo, "ls-files", "-z") + + def test_review_patch_rejects_oversized_content(self) -> None: + with self.assertRaisesRegex(SystemExit, "too large to review safely"): + self.helper["validate_review_patch"]("local staged diff", ["safe.txt"], "x" * 25, 10) + + def test_review_patch_limit_counts_utf8_bytes(self) -> None: + with self.assertRaisesRegex(SystemExit, r"12 bytes; limit 10"): + self.helper["validate_review_patch"]("local staged diff", ["safe.txt"], "界" * 4, 10) + + def test_tracked_sensitive_paths_are_blocked_in_all_modes(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + (repo / "base.txt").write_text("base\n", encoding="utf-8") + git(repo, "add", "base.txt") + git(repo, "commit", "-q", "-m", "base") + base = git(repo, "rev-parse", "HEAD").strip() + + (repo / ".env").write_text("placeholder=true\n", encoding="utf-8") + git(repo, "add", ".env") + with self.assertRaisesRegex(SystemExit, "tracked sensitive paths"): + self.helper["local_bundle"](repo) + + git(repo, "commit", "-q", "-m", "sensitive path") + with self.assertRaisesRegex(SystemExit, "tracked sensitive paths"): + self.helper["branch_bundle"](repo, base) + with self.assertRaisesRegex(SystemExit, "tracked sensitive paths"): + self.helper["commit_bundle"](repo, "HEAD") + + def test_tracked_source_names_and_env_templates_remain_reviewable(self) -> None: + for rel in ( + "tokenizer.py", + "token_count.ts", + "src/token/parser.py", + "src/token/session.ts", + "internal/tokens/types.go", + "packages/token/package.json", + "scripts/tokens/session.sh", + "src/tokens/session.mjs", + "credentials/prod.py", + "secrets/runtime.ts", + "src/credentials/provider.py", + "src/secrets/scanner.ts", + "ui/tokens/session.vue", + "proto/token/session.proto", + "password_validator.go", + ".env.example", + "private/parser.py", + ".agents/skills/openclaw-secret-scanning-maintainer/SKILL.md", + "design-tokens/colors.json", + "tokens/default.json", + "token_count/generated.py", + ".docker/Dockerfile", + ".docker/scripts/build.sh", + ): + with self.subTest(rel=rel): + self.assertIsNone(self.helper["tracked_sensitive_repo_path_risk"](rel)) + + def test_untracked_token_source_paths_remain_reviewable(self) -> None: + for rel in ( + "src/token/parser.py", + "src/token/session.ts", + "scripts/tokens/session.sh", + "src/tokens/session.mjs", + "ui/tokens/session.vue", + "proto/token/session.proto", + ): + with self.subTest(rel=rel): + self.assertIsNone(self.helper["sensitive_repo_path_risk"](rel)) + + def test_sensitive_named_source_directories_are_blocked_untracked(self) -> None: + for rel in ( + "credentials/prod.py", + "secrets/runtime.ts", + "src/credentials/provider.py", + "src/secrets/scanner.ts", + ): + with self.subTest(rel=rel): + self.assertIsNotNone(self.helper["sensitive_repo_path_risk"](rel)) + + def test_tracked_env_variants_remain_sensitive(self) -> None: + for rel in ( + ".env-local", + ".env_prod", + ".env/production", + ".env/example/production", + ".env/template/prod", + ): + with self.subTest(rel=rel): + self.assertIsNotNone( + self.helper["tracked_sensitive_repo_path_risk"](rel) + ) + + def test_suffixed_credential_data_paths_remain_sensitive(self) -> None: + for rel in ( + "credentials-prod.json", + "service-account-dev.yaml", + "api-key.backup.json", + "token-prod.json", + "tokens.json", + "auth-token.yaml", + "prod-credentials.json", + "google-service-account.json", + "client-secret.yaml", + "credentials/prod.json", + "prod-credentials/client.conf", + "client-secrets/account.ini", + "token/production.json", + "tokens/production.json", + "tokens/session.dat", + "tokens/cache.json", + "token/user.json", + "tokens/device.sqlite", + "tokens/session.jwt", + "tokens/session", + "backup-secrets/prod.json", + "dev_credentials/runtime.yaml", + "client-secrets-old/account.ini", + "client-secrets/account.properties", + "credentials/prod.xml", + "secrets/prod.md", + "credentials.txt", + "client-secret.csv", + ".docker/config.json", + "deployment/.docker/config.json", + ): + with self.subTest(rel=rel): + self.assertIsNotNone( + self.helper["tracked_sensitive_repo_path_risk"](rel) + ) + + def test_secret_detector_handles_quoted_json_keys(self) -> None: + content = '{"' + 'api_key": "' + realistic_secret_value() + '"}' + + self.assertTrue(self.helper["secret_text_risk"](content)) + + def test_secret_detector_handles_raw_jwt(self) -> None: + content = ".".join( + ( + "eyJhbGciOiJIUzI1NiJ9", + "eyJzdWIiOiIxMjM0NTY3ODkwIn0", + "signatureplaceholder", + ) + ) + + self.assertTrue(self.helper["secret_text_risk"](content)) + + def test_secret_detector_handles_private_key_header_variants(self) -> None: + for content in ( + "-----BEGIN " + "ENCRYPTED PRIVATE KEY-----", + "-----BEGIN PGP " + "PRIVATE KEY BLOCK-----", + ): + with self.subTest(content=content): + self.assertTrue(self.helper["secret_text_risk"](content)) + + def test_secret_detector_allows_dotted_config_keys(self) -> None: + self.assertFalse( + self.helper["secret_text_risk"]( + 'permissions.autoreview.filesystem={":minimal"="read"}' + ) + ) + + def test_secret_detector_handles_punctuation_and_multiline_diff_values(self) -> None: + value = "Correct-Horse!" + "@Battery$Staple" + patch = ( + "@@ -1 +1,2 @@\n" + '+"api_key":\n' + '+ "' + value + '"\n' + ) + + self.assertTrue( + any( + self.helper["secret_text_risk"](content) + for content in self.helper["unified_diff_contents"](patch) + ) + ) + + def test_secret_detector_does_not_treat_code_expressions_as_values(self) -> None: + for content in ( + "token = secrets.token_urlsafe(32)", + "token = process.env.GITHUB_TOKEN", + 'token = os.environ["GITHUB_TOKEN"]', + 'password = payload.get("password")', + "token = auth_response.credentials.access_token", + "token = response.authentication.accessToken", + "token = request.headers.authorization", + "password = account.credentials.password", + "self.access_token = self.authentication.access_token", + "this.accessToken = this.authentication.accessToken", + "api_key = client.settings.apiKey", + 'token = "$GITHUB_TOKEN"', + 'token = "$env:GITHUB_TOKEN"', + 'token = "${{ secrets.GITHUB_TOKEN }}"', + 'token = "op://Vault/Item/token"', + 'token = "op://Development/AWS/Access Keys/access_key_id"', + 'token_endpoint = "https://accounts.example.com/oauth2/token"', + 'password_policy = "minimum-twelve-characters"', + ): + with self.subTest(content=content): + self.assertFalse(self.helper["secret_text_risk"](content)) + + def test_fallback_self_test_ignores_ambient_model_overrides(self) -> None: + with mock.patch.dict( + os.environ, + { + "AUTOREVIEW_MODEL": "ambient-global-model", + "AUTOREVIEW_CODEX_MODEL": "ambient-codex-model", + }, + clear=False, + ): + self.helper["self_test_fallback_scope"]() + + def test_secret_detector_handles_bare_call_keyword_values(self) -> None: + content = "client(api_" + "key=" + realistic_secret_value() + ")" + + self.assertTrue(self.helper["secret_text_risk"](content)) + + def test_secret_detector_handles_unquoted_underscore_tokens(self) -> None: + content = "token=prod_" + realistic_secret_value() + + self.assertTrue(self.helper["secret_text_risk"](content)) + + def test_secret_detector_allows_dotted_calls(self) -> None: + for content in ( + "token=secrets.token_urlsafe(32)", + "token = provider.issue_token()", + "token = generate_secure_token()", + ): + with self.subTest(content=content): + self.assertFalse(self.helper["secret_text_risk"](content)) + + def test_secret_detector_rejects_ambiguous_bare_values(self) -> None: + for content in ( + "pass" + "word=CORRECTHORSEBATTERYSTAPLE", + "to" + "ken=prod.opaquecredentialvalue", + "to" + "ken=TOKEN_FROM_ENVIRONMENT_SECRET", + "to" + "ken: prod.A7f9K2m4Q8v6N3x5R1p0T9z8 (production)", + "pass" + "word=correct.horse.battery.password", + "pass" + "word=Correct.horse.battery.staple", + "pass" + "word=\"${{ 'Correct.horse.battery.staple' }}\"", + "pass" + "word=\"{{ 'Correct.horse.battery.staple' }}\"", + ): + with self.subTest(content=content): + self.assertTrue(self.helper["secret_text_risk"](content)) + + def test_secret_detector_does_not_exempt_expression_text_in_literals(self) -> None: + for value in ( + "correct horse + battery staple", + "prefix-${credential}-suffix", + "secret.format(value)", + ): + with self.subTest(value=value): + content = "pass" + f'word="{value}"' + self.assertTrue(self.helper["secret_text_risk"](content)) + + def test_secret_detector_handles_lowercase_passphrases(self) -> None: + content = 'password="' + "correcthorsebatterystaple" + '"' + + self.assertTrue(self.helper["secret_text_risk"](content)) + + def test_secret_detector_handles_low_diversity_passwords(self) -> None: + content = 'password="' + "letmeinletmein" + '"' + + self.assertTrue(self.helper["secret_text_risk"](content)) + + def test_secret_detector_handles_aws_secret_access_keys(self) -> None: + content = ( + "AWS_SECRET_ACCESS_" + + "KEY=" + + "A7f9K2m4Q8v6N3x5R1p0T9z8B2c4D6e8F0h2" + ) + + self.assertTrue(self.helper["secret_text_risk"](content)) + + def test_secret_detector_allows_common_fixture_literals(self) -> None: + for content in ( + 'token: "token-oversized"', + 'API_KEY = "clawrouter-e2e-secret"', + 'token: "very-long-browser-token-0123456789"', + ): + with self.subTest(content=content): + self.assertFalse(self.helper["secret_text_risk"](content)) + + def test_secret_detector_does_not_trust_in_band_suppressions(self) -> None: + for marker in ("pragma: allowlist secret", "gitleaks:allow"): + with self.subTest(marker=marker): + content = ( + "pass" + + 'word="CorrectHorseBatteryStaple123!" # ' + + marker + ) + self.assertTrue(self.helper["secret_text_risk"](content)) + + def test_secret_detector_does_not_treat_quoted_code_text_as_a_reference(self) -> None: + for content in ( + "pass" + 'word="' + "CORRECT_HORSE_BATTERY_STAPLE" + '"', + "to" + 'ken="' + "process.env.PROD_TOKEN" + '"', + "api_" + 'key="' + "config.production_key" + '"', + ): + with self.subTest(content=content): + self.assertTrue(self.helper["secret_text_risk"](content)) + + self.assertFalse( + self.helper["secret_text_risk"]('api_key="${OPENAI_API_KEY}"') + ) + + def test_secret_detector_does_not_exempt_placeholder_substrings(self) -> None: + content = "pass" + 'word="prod-sample-' + realistic_secret_value() + '"' + + self.assertTrue(self.helper["secret_text_risk"](content)) + + def test_normalized_secret_scan_does_not_cross_hunks(self) -> None: + patch = ( + "@@ -1 +1 @@\n" + "+password:\n" + "@@ -20 +20 @@\n" + '+"ordinary long string"\n' + ) + + self.assertFalse( + any( + self.helper["secret_text_risk"](content) + for content in self.helper["unified_diff_contents"](patch) + ) + ) + + def test_normalized_secret_scan_handles_combined_diff_prefixes(self) -> None: + value = "Correct-Horse!" + "@Battery$Staple" + patch = ( + "diff --cc settings.json\n" + "@@@ -1,1 -1,1 +1,2 @@@\n" + '++"api_key":\n' + '++ "' + value + '"\n' + ) + + self.assertTrue( + any( + self.helper["secret_text_risk"](content) + for content in self.helper["unified_diff_contents"](patch) + ) + ) + + def test_normalized_secret_scan_separates_old_and_new_values(self) -> None: + value = "Correct-Horse!" + "@Battery$Staple" + patch = ( + "@@ -1,2 +1,2 @@\n" + " password:\n" + "- placeholder\n" + '+ "' + value + '"\n' + ) + + self.assertTrue( + any( + self.helper["secret_text_risk"](content) + for content in self.helper["unified_diff_contents"](patch) + ) + ) + + def test_secret_detector_handles_compound_json_keys(self) -> None: + for key in ("client_secret", "refresh_token"): + content = '{"' + key + '": "' + realistic_secret_value() + '"}' + with self.subTest(key=key): + self.assertTrue(self.helper["secret_text_risk"](content)) + + def test_secret_like_patch_content_is_blocked_in_all_modes(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + path = repo / "settings.txt" + path.write_text("base\n", encoding="utf-8") + git(repo, "add", "settings.txt") + git(repo, "commit", "-q", "-m", "base") + base = git(repo, "rev-parse", "HEAD").strip() + + path.write_text( + "api" + "_key=" + realistic_secret_value() + "\n", + encoding="utf-8", + ) + git(repo, "add", "settings.txt") + with self.assertRaisesRegex(SystemExit, "secret-like content"): + self.helper["local_bundle"](repo) + + git(repo, "commit", "-q", "-m", "secret content") + with self.assertRaisesRegex(SystemExit, "secret-like content"): + self.helper["branch_bundle"](repo, base) + with self.assertRaisesRegex(SystemExit, "secret-like content"): + self.helper["commit_bundle"](repo, "HEAD") def test_pi_refuses_truncated_review_input(self) -> None: reviewer = argparse.Namespace(engine="pi", tools=True) @@ -115,10 +725,11 @@ class AutoreviewHardeningTests(unittest.TestCase): reviewer, False, ) - self.helper["ensure_reviewer_input_complete"]( - argparse.Namespace(engine="codex", tools=True), - True, - ) + with self.assertRaisesRegex(SystemExit, "codex engine refused truncated review input"): + self.helper["ensure_reviewer_input_complete"]( + argparse.Namespace(engine="codex", tools=True), + True, + ) with self.assertRaisesRegex(SystemExit, "claude engine refused truncated review input"): self.helper["ensure_reviewer_input_complete"]( argparse.Namespace(engine="claude", tools=True), @@ -166,8 +777,12 @@ class AutoreviewHardeningTests(unittest.TestCase): repo = init_repo(Path(tempdir)) (repo / "AGENTS.md").write_text("hostile instructions\n", encoding="utf-8") - with self.assertRaisesRegex(SystemExit, "droid engine is unavailable"): + with self.assertRaisesRegex( + SystemExit, + r"droid engine is unavailable.*use codex, claude, or pi", + ) as error: self.helper["run_droid"](argparse.Namespace(), repo, "prompt") + self.assertNotIn("opencode", str(error.exception)) def test_prompt_file_keeps_recoverable_repo_path(self) -> None: with tempfile.TemporaryDirectory() as tempdir: @@ -180,6 +795,23 @@ class AutoreviewHardeningTests(unittest.TestCase): self.assertIn("# Prompt file: review.md", prompt) self.assertFalse(truncated) + def test_build_prompt_omits_absolute_repo_path_and_caps_aggregate_input(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + prompt = self.helper["build_prompt"](repo, "local", None, "diff", "", "") + + self.assertIn("Repository root: .", prompt) + self.assertNotIn(str(repo), prompt) + with self.assertRaisesRegex(SystemExit, "aggregate limit"): + self.helper["build_prompt"]( + repo, + "local", + None, + "x" * self.helper["MAX_REVIEW_PROMPT_BYTES"], + "", + "", + ) + def test_cursor_refuses_global_mcp_config(self) -> None: with tempfile.TemporaryDirectory() as tempdir: root = Path(tempdir) @@ -194,10 +826,48 @@ class AutoreviewHardeningTests(unittest.TestCase): cursor_allow_workspace_instructions=True, ) - with mock.patch.object(Path, "home", return_value=root): - with self.assertRaisesRegex(SystemExit, "cursor engine refused global MCP config"): + with mock.patch.object(Path, "home", return_value=root), mock.patch.dict( + os.environ, + {"HOME": str(root), "USERPROFILE": str(root)}, + ): + with self.assertRaisesRegex(SystemExit, "cursor engine is unavailable"): self.helper["run_cursor"](args, repo, "prompt") + def test_cursor_refuses_user_level_hooks(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + root = Path(tempdir) + repo = init_repo(root) + settings = root / ".claude" / "settings.json" + settings.parent.mkdir() + settings.write_text('{"hooks":{"PreToolUse":[{"command":"unsafe"}]}}\n', encoding="utf-8") + args = argparse.Namespace( + thinking=None, + tools=True, + web_search=True, + cursor_allow_workspace_instructions=True, + ) + + with mock.patch.object(Path, "home", return_value=root), mock.patch.dict( + os.environ, + {"HOME": str(root), "USERPROFILE": str(root)}, + ): + with self.assertRaisesRegex(SystemExit, "cursor engine is unavailable"): + self.helper["run_cursor"](args, repo, "prompt") + + settings.write_text('{"permissions":{"allow":["Read(**)"]}}\n', encoding="utf-8") + with mock.patch.object(Path, "home", return_value=root), mock.patch.dict( + os.environ, + {"HOME": str(root), "USERPROFILE": str(root)}, + ): + self.assertEqual(self.helper["cursor_global_hook_paths"](), []) + + settings.write_text('{"enabledPlugins":{"review-hooks@example":true}}\n', encoding="utf-8") + with mock.patch.object(Path, "home", return_value=root), mock.patch.dict( + os.environ, + {"HOME": str(root), "USERPROFILE": str(root)}, + ): + self.assertEqual(self.helper["cursor_global_hook_paths"](), [settings]) + def test_read_text_truncates_without_scanning_tail(self) -> None: with tempfile.TemporaryDirectory() as tempdir: path = Path(tempdir) / "large.txt" @@ -208,6 +878,16 @@ class AutoreviewHardeningTests(unittest.TestCase): self.assertIn("[truncated at 180000 characters]", text) self.assertNotEqual(text, "[binary file omitted]") + def test_read_text_marks_unreadable_input_incomplete(self) -> None: + with mock.patch.dict( + self.helper["read_text_with_status"].__globals__, + {"read_prefix": lambda *_args: (_ for _ in ()).throw(SystemExit("denied"))}, + ): + text, incomplete = self.helper["read_text_with_status"](Path("blocked")) + + self.assertIn("[unreadable:", text) + self.assertTrue(incomplete) + def test_evidence_file_must_be_repo_relative_and_not_symlinked(self) -> None: with tempfile.TemporaryDirectory() as tempdir: root = Path(tempdir) @@ -239,8 +919,49 @@ class AutoreviewHardeningTests(unittest.TestCase): os.environ["GIT_CONFIG_COUNT"] = "99" os.environ["DYLD_INSERT_LIBRARIES"] = "/tmp/unsafe.dylib" os.environ["NODE_OPTIONS"] = "--require=/tmp/unsafe.js" + os.environ["NODE_PATH"] = "/tmp/unsafe-node" + os.environ["LD_AUDIT"] = "/tmp/unsafe-audit.so" + os.environ["LD_LIBRARY_PATH"] = "/tmp/unsafe-lib" + os.environ["RUBYOPT"] = "-r/tmp/unsafe.rb" + os.environ["PERL5OPT"] = "-Munsafe" + os.environ["BUN_OPTIONS"] = "--preload=/tmp/unsafe.js" + os.environ["OPENCODE_CONFIG"] = "/tmp/unsafe-opencode.json" + os.environ["OPENCODE_PERMISSION"] = "allow" + os.environ["OPENCODE_AUTO_SHARE"] = "1" + os.environ["COPILOT_ALLOW_ALL"] = "1" + os.environ["CODEX_HOME"] = "/tmp/codex-auth" + os.environ["DBUS_SESSION_BUS_ADDRESS"] = "unix:path=/run/user/1000/bus" + os.environ["XDG_RUNTIME_DIR"] = "/run/user/1000" + os.environ["CLAUDE_CONFIG_DIR"] = "/tmp/claude-auth" + os.environ["PI_CODING_AGENT_DIR"] = "/tmp/pi-auth" + os.environ["CLAUDE_CODE_USE_FOUNDRY"] = "1" + os.environ["CLOUD_ML_REGION"] = "us-east5" + os.environ["ANTHROPIC_AUTH_TOKEN"] = "test-auth-token" + os.environ["AWS_BEARER_TOKEN_BEDROCK"] = "test-token-placeholder" + os.environ["ANTHROPIC_BEDROCK_BASE_URL"] = ( + "https://bedrock.example.invalid" + ) + os.environ["ANTHROPIC_VERTEX_BASE_URL"] = ( + "https://vertex.example.invalid" + ) + os.environ["AWS_PROFILE"] = "review-profile" + os.environ["AWS_CONFIG_FILE"] = "/tmp/unsafe-aws-config" + os.environ["GOOGLE_APPLICATION_CREDENTIALS"] = ( + "/tmp/unsafe-google-credentials" + ) + os.environ["GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES"] = "1" + os.environ["OPENROUTER_API_KEY"] = "test-provider-key" + os.environ["GITHUB_TOKEN"] = "test-token-placeholder" + os.environ["HTTPS_PROXY"] = "http://proxy.example.invalid:8080" + os.environ["HTTP_PROXY"] = "proxy.example.invalid:8080" + os.environ["ALL_PROXY"] = "socks5://proxy.example.invalid:1080" + os.environ["DO_NOT_TRACK"] = "1" + os.environ["DISABLE_TELEMETRY"] = "1" + os.environ["CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC"] = "1" - env = self.helper["safe_engine_env"](repo) + env = self.helper["safe_engine_env"](repo, engine="codex") + claude_env = self.helper["safe_engine_env"](repo, engine="claude") + pi_env = self.helper["safe_engine_env"](repo, engine="pi") self.assertNotEqual(env.get("GIT_DIR"), "/tmp/unsafe-git-dir") self.assertEqual( @@ -249,22 +970,563 @@ class AutoreviewHardeningTests(unittest.TestCase): ) self.assertNotIn("DYLD_INSERT_LIBRARIES", env) self.assertNotIn("NODE_OPTIONS", env) + for key in ( + "NODE_PATH", + "LD_AUDIT", + "LD_LIBRARY_PATH", + "RUBYOPT", + "PERL5OPT", + "BUN_OPTIONS", + "OPENCODE_CONFIG", + "OPENCODE_PERMISSION", + "OPENCODE_AUTO_SHARE", + ): + self.assertNotIn(key, env) + self.assertNotIn("COPILOT_ALLOW_ALL", env) + self.assertNotIn("GITHUB_TOKEN", env) + self.assertEqual(env["HTTPS_PROXY"], "http://proxy.example.invalid:8080") + self.assertEqual(env["HTTP_PROXY"], "proxy.example.invalid:8080") + self.assertEqual(env["ALL_PROXY"], "socks5://proxy.example.invalid:1080") + self.assertEqual(env["DO_NOT_TRACK"], "1") + self.assertEqual(env["DISABLE_TELEMETRY"], "1") + self.assertEqual(env["CODEX_HOME"], "/tmp/codex-auth") + if os.name == "nt": + self.assertNotIn("DBUS_SESSION_BUS_ADDRESS", env) + else: + self.assertEqual( + env["DBUS_SESSION_BUS_ADDRESS"], + "unix:path=/run/user/1000/bus", + ) + self.assertEqual(env["XDG_RUNTIME_DIR"], "/run/user/1000") + self.assertEqual( + claude_env["CLAUDE_CONFIG_DIR"], + "/tmp/claude-auth", + ) + self.assertEqual( + claude_env["CLAUDE_CODE_DISABLE_AUTO_MEMORY"], + "1", + ) + self.assertEqual(pi_env["PI_CODING_AGENT_DIR"], "/tmp/pi-auth") + self.assertEqual(claude_env["CLAUDE_CODE_USE_FOUNDRY"], "1") + self.assertEqual(claude_env["CLOUD_ML_REGION"], "us-east5") + self.assertEqual( + claude_env["ANTHROPIC_AUTH_TOKEN"], + "test-auth-token", + ) + self.assertEqual( + claude_env["AWS_BEARER_TOKEN_BEDROCK"], + "test-token-placeholder", + ) + self.assertEqual( + claude_env["ANTHROPIC_BEDROCK_BASE_URL"], + "https://bedrock.example.invalid", + ) + self.assertEqual( + claude_env["ANTHROPIC_VERTEX_BASE_URL"], + "https://vertex.example.invalid", + ) + self.assertEqual(claude_env["AWS_PROFILE"], "review-profile") + self.assertNotIn("AWS_CONFIG_FILE", env) + self.assertNotIn("GOOGLE_APPLICATION_CREDENTIALS", env) + self.assertNotIn( + "GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES", + env, + ) + self.assertNotIn("OPENROUTER_API_KEY", env) + self.assertEqual( + claude_env["CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC"], + "1", + ) finally: os.environ.clear() os.environ.update(old) + def test_safe_proxy_url_accepts_credential_free_formats(self) -> None: + for value in ( + "http://proxy.example.invalid:8080", + "proxy.example.invalid:8080", + "socks4://proxy.example.invalid", + "socks4a://proxy.example.invalid", + ): + with self.subTest(value=value): + self.assertTrue(self.helper["safe_proxy_url"](value)) + + for value in ( + "http://review-user:review-password@proxy.example.invalid:8080", + "socks5://review-user:review-password@proxy.example.invalid:1080", + ): + with self.subTest(value=value): + self.assertFalse(self.helper["safe_proxy_url"](value)) + + def test_safe_engine_env_rejects_credentialed_proxy(self) -> None: + with tempfile.TemporaryDirectory() as tempdir, mock.patch.dict( + os.environ, + { + "HTTPS_PROXY": ( + "http://review-user:review-password@proxy.example.invalid:8080" + ) + }, + clear=False, + ): + repo = init_repo(Path(tempdir)) + with self.assertRaisesRegex(SystemExit, "credentialed or malformed proxy"): + self.helper["safe_engine_env"](repo, engine="codex") + + def test_safe_temp_root_rejects_reviewed_repo_parent(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + hostile_temp = repo / "tmp" + hostile_temp.mkdir() + + with mock.patch.object( + tempfile, + "gettempdir", + return_value=str(hostile_temp), + ), self.assertRaisesRegex( + SystemExit, + "temporary directory must be outside", + ): + self.helper["safe_temp_root"](repo) + + def test_claude_fable_alias_requires_fable_safe_mode_version(self) -> None: + args = argparse.Namespace( + claude_bin="claude", + fallback_model=None, + model="fable", + ) + version_result = subprocess.CompletedProcess( + ["claude", "--version"], + 0, + "2.1.169 (Claude Code)", + "", + ) + + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + with mock.patch.dict( + self.helper["ensure_claude_isolation_supported"].__globals__, + { + "resolve_command": lambda *_args: "/usr/bin/claude", + "safe_engine_env": lambda *_args, **_kwargs: {}, + "safe_temp_root": lambda _repo: Path(tempdir), + "run": lambda *_args, **_kwargs: version_result, + }, + ), self.assertRaisesRegex( + SystemExit, + "2.1.170", + ): + self.helper["ensure_claude_isolation_supported"](args, repo) + + def test_claude_runs_outside_repo_with_auto_memory_disabled(self) -> None: + args = argparse.Namespace( + claude_allowed_tools=None, + claude_bin="claude", + fallback_model=None, + model=None, + stream_engine_output=False, + thinking=None, + tools=False, + web_search=False, + ) + observed: dict[str, object] = {} + + def fake_run( + _cmd: list[str], + cwd: Path, + **kwargs: object, + ) -> subprocess.CompletedProcess[str]: + observed["cwd"] = cwd + observed["env"] = kwargs["env"] + return subprocess.CompletedProcess([], 0, "{}", "") + + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + with mock.patch.dict( + self.helper["run_claude"].__globals__, + { + "ensure_claude_isolation_supported": lambda *_args: None, + "resolve_command": lambda *_args: "/usr/bin/claude", + "run_with_heartbeat": fake_run, + "safe_engine_env": lambda *_args, **_kwargs: { + "CLAUDE_CODE_DISABLE_AUTO_MEMORY": "1" + }, + }, + ): + self.helper["run_claude"](args, repo, "prompt") + + self.assertFalse( + self.helper["is_within"](observed["cwd"], repo.resolve()) + ) + self.assertEqual( + observed["env"]["CLAUDE_CODE_DISABLE_AUTO_MEMORY"], + "1", + ) + + def test_build_prompt_rejects_secret_like_git_metadata(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + secret = "ghp_" + "A" * 24 + git(repo, "checkout", "-q", "-b", f"feature/{secret}") + + with self.assertRaisesRegex(SystemExit, "secret-like content"): + self.helper["build_prompt"](repo, "local", None, "diff", "", "") + + git(repo, "checkout", "-q", "-B", "safe-branch") + with self.assertRaisesRegex(SystemExit, "secret-like content"): + self.helper["build_prompt"]( + repo, + "branch", + f"origin/{secret}", + "diff", + "", + "", + ) + + def test_codex_env_rejects_executable_dbus_transport(self) -> None: + old = os.environ.copy() + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + try: + os.environ["DBUS_SESSION_BUS_ADDRESS"] = ( + "unixexec:path=/tmp/hostile-helper" + ) + env = self.helper["safe_engine_env"](repo, engine="codex") + self.assertNotIn("DBUS_SESSION_BUS_ADDRESS", env) + finally: + os.environ.clear() + os.environ.update(old) + + def test_multi_provider_engines_preserve_provider_auth(self) -> None: + old = os.environ.copy() + with tempfile.TemporaryDirectory() as tempdir: + root = Path(tempdir).resolve() + repo = init_repo(root) + try: + os.environ["DEEPSEEK_API_KEY"] = "test-token-placeholder" + os.environ["CEREBRAS_API_KEY"] = "test-token-placeholder" + os.environ["CLOUDFLARE_ACCOUNT_ID"] = "test-account" + os.environ["CLOUDFLARE_API_TOKEN"] = "test-token-placeholder" + os.environ["GOOGLE_APPLICATION_CREDENTIALS"] = ( + str(root / "provider-credentials.json") + ) + os.environ["AWS_ROLE_ARN"] = ( + "arn:aws:iam::123456789012:role/autoreview" + ) + os.environ["AWS_WEB_IDENTITY_TOKEN_FILE"] = str( + root / "web-identity", + ) + os.environ["AWS_CONFIG_FILE"] = str(root / "aws-config") + os.environ["AWS_SHARED_CREDENTIALS_FILE"] = str( + root / "aws-credentials", + ) + os.environ["NODE_EXTRA_CA_CERTS"] = str(root / "corporate-ca.pem") + os.environ["SSL_CERT_FILE"] = str(root / "tls-ca.pem") + os.environ["SSL_CERT_DIR"] = str(root / "tls-ca") + os.environ["SNOWFLAKE_ACCOUNT"] = "test-account" + os.environ["SNOWFLAKE_CORTEX_TOKEN"] = "test-token-placeholder" + os.environ["AZURE_RESOURCE_NAME"] = "test-resource" + os.environ["ANTHROPIC_OAUTH_TOKEN"] = "test-token-placeholder" + os.environ["AWS_BEDROCK_FORCE_HTTP1"] = "1" + os.environ["AWS_BEDROCK_SKIP_AUTH"] = "1" + os.environ["AZURE_CLIENT_ID"] = "test-client" + os.environ["AZURE_CLIENT_SECRET"] = "test-token-placeholder" + os.environ["AZURE_TENANT_ID"] = "test-tenant" + os.environ["GCLOUD_PROJECT"] = "test-project" + os.environ["GOOGLE_CLOUD_PROJECT"] = "test-project" + os.environ["CODEX_API_KEY"] = "test-token-placeholder" + os.environ["CODEX_CA_CERTIFICATE"] = str(root / "codex-ca.pem") + os.environ["NODE_OPTIONS"] = "--require=/tmp/unsafe.js" + os.environ["GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES"] = "1" + os.environ["XDG_DATA_HOME"] = str(root / "opencode-auth") + + for engine in ("opencode", "pi"): + with self.subTest(engine=engine): + env = self.helper["safe_engine_env"](repo, engine=engine) + for key in ( + "AWS_ROLE_ARN", + "AWS_BEDROCK_FORCE_HTTP1", + "AWS_BEDROCK_SKIP_AUTH", + "AWS_CONFIG_FILE", + "AWS_SHARED_CREDENTIALS_FILE", + "AWS_WEB_IDENTITY_TOKEN_FILE", + "CEREBRAS_API_KEY", + "CLOUDFLARE_ACCOUNT_ID", + "CLOUDFLARE_API_TOKEN", + "DEEPSEEK_API_KEY", + "GOOGLE_APPLICATION_CREDENTIALS", + "NODE_EXTRA_CA_CERTS", + "SSL_CERT_DIR", + "SSL_CERT_FILE", + "SNOWFLAKE_ACCOUNT", + "SNOWFLAKE_CORTEX_TOKEN", + "AZURE_RESOURCE_NAME", + "ANTHROPIC_OAUTH_TOKEN", + ): + self.assertEqual(env[key], os.environ[key]) + self.assertNotIn("NODE_OPTIONS", env) + self.assertNotIn( + "GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES", + env, + ) + if engine == "opencode": + self.assertEqual( + env["XDG_DATA_HOME"], + str(root / "opencode-auth"), + ) + + claude_env = self.helper["safe_engine_env"](repo, engine="claude") + for key in ( + "AZURE_CLIENT_ID", + "AZURE_CLIENT_SECRET", + "AZURE_TENANT_ID", + "GCLOUD_PROJECT", + "GOOGLE_CLOUD_PROJECT", + "AWS_ROLE_ARN", + "AWS_CONFIG_FILE", + "AWS_SHARED_CREDENTIALS_FILE", + "AWS_WEB_IDENTITY_TOKEN_FILE", + "GOOGLE_APPLICATION_CREDENTIALS", + "NODE_EXTRA_CA_CERTS", + "SSL_CERT_DIR", + "SSL_CERT_FILE", + ): + self.assertEqual(claude_env[key], os.environ[key]) + self.assertNotIn("DEEPSEEK_API_KEY", claude_env) + self.assertNotIn("NODE_OPTIONS", claude_env) + codex_env = self.helper["safe_engine_env"](repo, engine="codex") + for key in ( + "CODEX_API_KEY", + "CODEX_CA_CERTIFICATE", + "SSL_CERT_DIR", + "SSL_CERT_FILE", + ): + self.assertEqual(codex_env[key], os.environ[key]) + finally: + os.environ.clear() + os.environ.update(old) + + def test_provider_credential_paths_are_forwarded_as_absolute(self) -> None: + old_env = os.environ.copy() + old_cwd = Path.cwd() + with tempfile.TemporaryDirectory() as tempdir: + root = Path(tempdir) + repo = init_repo(root) + try: + os.chdir(repo) + os.environ["AWS_CONFIG_FILE"] = "../shared/aws-config" + os.environ["SSL_CERT_DIR"] = os.pathsep.join( + ("../tls/one", "../tls/two"), + ) + + env = self.helper["safe_engine_env"](repo, engine="pi") + + self.assertEqual( + env["AWS_CONFIG_FILE"], + str((root / "shared" / "aws-config").resolve()), + ) + self.assertEqual( + env["SSL_CERT_DIR"], + os.pathsep.join( + ( + str((root / "tls" / "one").resolve()), + str((root / "tls" / "two").resolve()), + ) + ), + ) + finally: + os.chdir(old_cwd) + os.environ.clear() + os.environ.update(old_env) + + def test_opencode_rejects_repo_local_xdg_auth_store(self) -> None: + old = os.environ.copy() + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + try: + os.environ["XDG_DATA_HOME"] = str(repo / ".opencode-data") + os.environ["AWS_CONFIG_FILE"] = str(repo / ".aws-config") + os.environ["NODE_EXTRA_CA_CERTS"] = str(repo / "ca.pem") + os.environ["SSL_CERT_FILE"] = str(repo / "tls-ca.pem") + os.environ["SSL_CERT_DIR"] = os.pathsep.join( + (str(repo.parent / "tls-ca"), str(repo / "tls-ca")), + ) + env = self.helper["safe_engine_env"](repo, engine="opencode") + self.assertNotIn("XDG_DATA_HOME", env) + self.assertNotIn("AWS_CONFIG_FILE", env) + self.assertNotIn("NODE_EXTRA_CA_CERTS", env) + self.assertNotIn("SSL_CERT_FILE", env) + self.assertNotIn("SSL_CERT_DIR", env) + finally: + os.environ.clear() + os.environ.update(old) + + def test_engines_reject_repo_local_config_roots(self) -> None: + old = os.environ.copy() + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + try: + os.environ["CLAUDE_CONFIG_DIR"] = str(repo / ".claude") + os.environ["CODEX_HOME"] = str(repo / ".codex") + os.environ["PI_CODING_AGENT_DIR"] = str(repo / ".pi") + os.environ["CODEX_CA_CERTIFICATE"] = str(repo / "codex-ca.pem") + os.environ["SSL_CERT_FILE"] = str(repo / "tls-ca.pem") + os.environ["HOME"] = str(repo) + os.environ["USERPROFILE"] = str(repo) + claude_env = self.helper["safe_engine_env"](repo, engine="claude") + codex_env = self.helper["safe_engine_env"](repo, engine="codex") + pi_env = self.helper["safe_engine_env"](repo, engine="pi") + self.assertNotIn("CLAUDE_CONFIG_DIR", claude_env) + self.assertNotIn("CODEX_HOME", codex_env) + self.assertNotIn("CODEX_CA_CERTIFICATE", codex_env) + self.assertNotIn("SSL_CERT_FILE", codex_env) + self.assertNotIn("PI_CODING_AGENT_DIR", pi_env) + self.assertNotIn("HOME", claude_env) + self.assertNotIn("USERPROFILE", claude_env) + finally: + os.environ.clear() + os.environ.update(old) + + def test_codex_auth_config_ignores_repo_local_home(self) -> None: + old = os.environ.copy() + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + config_dir = repo / ".codex" + config_dir.mkdir() + (config_dir / "config.toml").write_text( + 'forced_login_method = "api"\n', + encoding="utf-8", + ) + try: + os.environ["CODEX_HOME"] = str(config_dir) + self.assertEqual(self.helper["codex_auth_config_flags"](repo), []) + finally: + os.environ.clear() + os.environ.update(old) + + def test_opencode_web_search_preserves_explicit_exa_opt_in(self) -> None: + old = os.environ.copy() + try: + os.environ["OPENCODE_ENABLE_EXA"] = "1" + enabled = self.helper["opencode_review_env"](True) + disabled = self.helper["opencode_review_env"](False) + self.assertEqual(enabled["OPENCODE_ENABLE_EXA"], "1") + self.assertNotIn("OPENCODE_ENABLE_EXA", disabled) + finally: + os.environ.clear() + os.environ.update(old) + + def test_codex_isolation_restricts_tool_environment(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + flags = self.helper["codex_config_isolation_flags"](repo) + + for required in ( + 'shell_environment_policy.inherit="core"', + "shell_environment_policy.ignore_default_excludes=false", + "shell_environment_policy.experimental_use_profile=false", + "allow_login_shell=false", + 'default_permissions="autoreview"', + 'permissions.autoreview.filesystem={":minimal"="read",":workspace_roots"="read"}', + ): + self.assertIn(required, flags) + set_flag = next( + flag for flag in flags if flag.startswith("shell_environment_policy.set=") + ) + for key, value in self.helper["codex_tool_git_env"]().items(): + self.assertIn(f"{key}={json.dumps(value)}", set_flag) + def test_safe_engine_env_excludes_repo_local_path_entries(self) -> None: old_path = os.environ.get("PATH", "") with tempfile.TemporaryDirectory() as tempdir: repo = init_repo(Path(tempdir)) os.environ["PATH"] = f"{repo}{os.pathsep}{old_path}" try: - env = self.helper["safe_engine_env"](repo) + env = self.helper["safe_engine_env"](repo, engine="codex") finally: os.environ["PATH"] = old_path self.assertNotIn(str(repo.resolve()), env["PATH"].split(os.pathsep)) + def test_find_command_rejects_explicit_repo_local_executables(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + root = Path(tempdir) + repo = init_repo(root) + (repo / "tools").mkdir() + (root / "trusted").mkdir() + repo_bin = self.helper["write_executable"]( + repo / "tools" / "codex", + "#!/bin/sh\nexit 0\n", + ) + external_bin = self.helper["write_executable"]( + root / "trusted" / "codex", + "#!/bin/sh\nexit 0\n", + ) + + self.assertIsNone( + self.helper["find_command"]("tools/codex", repo), + ) + self.assertIsNone( + self.helper["find_command"](str(repo_bin), repo), + ) + self.assertEqual( + self.helper["find_command"](str(external_bin), repo), + str(Path(os.path.abspath(external_bin))), + ) + self.assertEqual( + self.helper["find_command"]("../trusted/codex", repo), + str(Path(os.path.abspath(external_bin))), + ) + + external_link = root / "trusted" / "external-codex" + repo_link = repo / "tools" / "external-codex" + try: + external_link.symlink_to(repo_bin) + repo_link.symlink_to(external_bin) + except OSError as exc: + if os.name == "nt" and getattr(exc, "winerror", None) == 1314: + return + raise + self.assertIsNone( + self.helper["find_command"](str(external_link), repo), + ) + self.assertIsNone( + self.helper["find_command"](str(repo_link), repo), + ) + + def test_validate_report_normalizes_relative_finding_paths(self) -> None: + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + report = { + "findings": [ + { + "title": "Finding", + "body": "Body", + "priority": "P1", + "confidence": 0.9, + "category": "bug", + "code_location": {"file_path": r".\src\index.ts", "line": 1}, + } + ], + "overall_correctness": "patch is incorrect", + "overall_explanation": "Explanation", + "overall_confidence": 0.9, + } + + self.helper["validate_report"](report, repo, {"src/index.ts"}, []) + + self.assertEqual(report["findings"][0]["code_location"]["file_path"], "src/index.ts") + + report["findings"][0]["code_location"]["file_path"] = r"src\index.ts" + self.helper["validate_report"](report, repo, {r"src\index.ts"}, []) + self.assertEqual( + report["findings"][0]["code_location"]["file_path"], + r"src\index.ts", + ) + + report["findings"][0]["code_location"]["file_path"] = " " + with self.assertRaisesRegex(SystemExit, "invalid location"): + self.helper["validate_report"](report, repo, {"src/index.ts"}, []) + def test_safe_engine_env_ignores_inaccessible_path_entries(self) -> None: old_path = os.environ.get("PATH", "") with tempfile.TemporaryDirectory() as tempdir: @@ -281,7 +1543,7 @@ class AutoreviewHardeningTests(unittest.TestCase): try: with mock.patch.object(Path, "exists", fake_exists): - env = self.helper["safe_engine_env"](repo) + env = self.helper["safe_engine_env"](repo, engine="codex") finally: os.environ["PATH"] = old_path @@ -303,32 +1565,20 @@ class AutoreviewHardeningTests(unittest.TestCase): self.assertEqual(result.returncode, 0, result.stderr) self.assertIn("\ufffd", result.stdout) - def test_large_repo_relative_evidence_file_is_truncated(self) -> None: + def test_large_repo_relative_evidence_file_is_rejected(self) -> None: with tempfile.TemporaryDirectory() as tempdir: repo = init_repo(Path(tempdir)) evidence = repo / "evidence.txt" evidence.write_text("x" * 600_000, encoding="utf-8") - _, content, truncated = self.helper["validate_evidence_file"](repo, "evidence.txt", "--dataset") + with self.assertRaisesRegex(SystemExit, "file too large to scan safely"): + self.helper["validate_evidence_file"]( + repo, + "evidence.txt", + "--dataset", + ) - self.assertIn("[truncated at 180000 characters]", content) - self.assertTrue(truncated) - - def test_copilot_allows_web_fetch_only_when_web_search_is_enabled(self) -> None: - captured: list[list[str]] = [] - - def fake_run_with_heartbeat( - cmd: list[str], - cwd: Path, - **kwargs: object, - ) -> subprocess.CompletedProcess[str]: - captured.append(cmd) - return subprocess.CompletedProcess(cmd, 0, '{"findings":[]}', "") - - self.helper["run_copilot"].__globals__["run_with_heartbeat"] = fake_run_with_heartbeat - self.helper["run_copilot"].__globals__["resolve_command"] = ( - lambda command, repo: f"/resolved/{command}" - ) + def test_copilot_fails_closed_without_repo_only_read_sandbox(self) -> None: args = argparse.Namespace( copilot_bin="copilot", thinking=None, @@ -338,16 +1588,48 @@ class AutoreviewHardeningTests(unittest.TestCase): stream_engine_output=False, ) - self.helper["run_copilot"](args, Path("/repo"), "prompt") + with tempfile.TemporaryDirectory() as tempdir: + repo = init_repo(Path(tempdir)) + with self.assertRaisesRegex( + SystemExit, + r"ignored repository secrets; use codex, claude, or pi", + ) as error: + self.helper["run_copilot"]( + args, + repo, + "Repository root: .\n\nprompt", + ) + self.assertNotIn("opencode", str(error.exception)) - self.assertNotIn("--allow-tool=web_fetch", captured[-1]) - self.assertFalse(any(arg == "--allow-all-urls" for arg in captured[-1])) + def test_claude_inventory_is_bundle_and_web_only(self) -> None: + args = argparse.Namespace( + claude_allowed_tools="WebFetch(domain:docs.example.com),WebSearch", + web_search=True, + ) + + self.assertEqual( + self.helper["claude_allowed_tools"](args), + "WebFetch(domain:docs.example.com),WebSearch", + ) + self.assertEqual( + self.helper["claude_tool_inventory"](args), + "WebFetch,WebSearch", + ) + + args.web_search = False + self.assertEqual( + self.helper["claude_allowed_tools"](args), + "", + ) + + args.claude_allowed_tools = "Read" + with self.assertRaisesRegex(SystemExit, "not read-only"): + self.helper["claude_tool_inventory"](args) args.web_search = True - self.helper["run_copilot"](args, Path("/repo"), "prompt") - - self.assertIn("--allow-tool=web_fetch", captured[-1]) - self.assertIn("--allow-all-urls", captured[-1]) + args.claude_allowed_tools = "WebFetch" + with self.assertRaisesRegex(SystemExit, "one explicit domain"): + self.helper["claude_tool_inventory"](args) def test_self_test_shortcut_runs_deterministic_checks(self) -> None: command = [str(SCRIPT), "--self-test"]