From 2d7c6a39da17610a4cc235973e306590c371e5a2 Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 21 Oct 2026 17:37:32 -0400 Subject: [PATCH] fix(llm): keep claude_cli and codex_cli tasks independent of ambient instructions Claude tasks now pass --safe-mode, so the operator's CLAUDE.md, skills, plugins, hooks and MCP servers no longer reach review tasks; validateArgs rejects a Claude argv without it. Codex tasks pass -c project_doc_max_bytes=0, so the PR head's AGENTS.md no longer becomes instructions for its own review; validateArgs requires it on exec and exec resume. Codex has no flag for the user-level CODEX_HOME AGENTS.md, which the README now documents. Closes #681 Twapp-Session: 46c432b9-3bb2-438f-843f-10e4cd5050d7 --- README.md | 26 +++++-- internal/llmadapters/subprocess.go | 19 ++++++ internal/llmadapters/subprocess_test.go | 91 ++++++++++++++++++++++--- 3 files changed, 121 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 70e7d63..f71a422 100644 --- a/README.md +++ b/README.md @@ -807,18 +807,30 @@ cache workdir so Claude Code can resolve resumed sessions consistently; set `CR_CLAUDE_BG_WORK_DIR` to override that workdir. For checkout-native reviewer execution, Claude CLI receives a disposable reviewer workspace, run-owned scratch/temp/cache directories, and `Read,Write,Bash` tools through Claude Code -directory permissions. +directory permissions. Every Claude request passes `--safe-mode`, so the +operator's `CLAUDE.md` files, skills, plugins, hooks, and MCP servers do not +reach any review task; subscription auth and the requested tools still work. +The adapter rejects an argv without it. For OpenAI subscription profiles, `adapter: codex_cli` runs `codex exec` in an adapter-owned scratch directory with `--json`, `--ephemeral`, -`--skip-git-repo-check`, `--ignore-user-config`, `--ignore-rules`, a read-only -sandbox, and a scratch `--cd`. For checkout-native reviewer execution, Codex CLI -uses `workspace-write` with the disposable reviewer workspace as `--cd`, plus -run-owned scratch/temp/cache environment variables for verification commands. -The adapter rejects unsafe flags. Non-reviewer Codex requests remain -best-effort/beta because Codex CLI does not yet expose a first-class +`--skip-git-repo-check`, `--ignore-user-config`, `--ignore-rules`, +`-c project_doc_max_bytes=0`, a read-only sandbox, and a scratch `--cd`. For +checkout-native reviewer execution, Codex CLI uses `workspace-write` with the +disposable reviewer workspace as `--cd`, plus run-owned scratch/temp/cache +environment variables for verification commands. The project-doc limit keeps +Codex from loading the PR head's `AGENTS.md` files as instructions, so a PR +cannot change the instructions its own review runs under. Codex has no flag +that skips the user-level `$CODEX_HOME/AGENTS.md`, and `cr` does not move +`CODEX_HOME` because it also holds Codex auth, so that file still reaches +Codex tasks. The adapter rejects unsafe flags. Non-reviewer Codex requests +remain best-effort/beta because Codex CLI does not yet expose a first-class all-tools-disabled flag. +Repository guidance reaches reviewers through trusted agent prompts, which can +tell a reviewer to read `AGENTS.md`, `CLAUDE.md`, or `docs/` from the checkout +as files. See [docs/review-guidance.md](docs/review-guidance.md). + Credential key matrix: | Profile field | Purpose | Auth/provider | Required keys | Optional keys | v1 behavior | diff --git a/internal/llmadapters/subprocess.go b/internal/llmadapters/subprocess.go index 7330997..ae06a73 100644 --- a/internal/llmadapters/subprocess.go +++ b/internal/llmadapters/subprocess.go @@ -338,6 +338,7 @@ func buildClaudeForegroundArgs(req Request, scratch string, resumeSessionID stri } args := []string{ "-p", + "--safe-mode", "--output-format", "json", "--tools", tools, "--permission-mode", "acceptEdits", @@ -795,6 +796,7 @@ func (a *SubprocessAdapter) buildArgsForSession(req Request, scratch string, res } args := []string{ "--bg", + "--safe-mode", "--enable-auto-mode", "--tools", tools, "--permission-mode", "auto", @@ -872,6 +874,11 @@ func appendCodexNoNetworkArgs(args []string, req Request) []string { return args } +// codexNoProjectDocsConfig stops Codex from loading AGENTS.md files from the +// working directory. A reviewer's cwd is the PR head checkout, and a PR must not +// change the instructions its own review runs under. +const codexNoProjectDocsConfig = "project_doc_max_bytes=0" + func subprocessCodexBaseArgs(prefix []string, sandbox, cwd string, durable bool) []string { args := append([]string(nil), prefix...) args = append(args, "--json") @@ -882,6 +889,7 @@ func subprocessCodexBaseArgs(prefix []string, sandbox, cwd string, durable bool) "--skip-git-repo-check", "--ignore-user-config", "--ignore-rules", + "-c", codexNoProjectDocsConfig, "--sandbox", sandbox, "--cd", cwd, ) @@ -896,6 +904,7 @@ func subprocessCodexResumeArgs() []string { "--skip-git-repo-check", "--ignore-user-config", "--ignore-rules", + "-c", codexNoProjectDocsConfig, } } @@ -914,6 +923,7 @@ func (a *SubprocessAdapter) validateArgs(args []string, scratch string, req Requ } if err := validateAllowedFlags("claude_cli", checkedArgs, map[string]bool{ "--bg": false, + "--safe-mode": false, "--enable-auto-mode": false, "-p": false, "--output-format": true, @@ -931,6 +941,12 @@ func (a *SubprocessAdapter) validateArgs(args []string, scratch string, req Requ if err := validateClaudeNoNetworkArgs(checkedArgs, req); err != nil { return err } + // Safe mode keeps the operator's CLAUDE.md, skills, plugins, hooks and + // MCP servers out of every task, so a review depends on the repository + // and the agent catalog rather than on the machine that runs it. + if !containsFlag(checkedArgs, "--safe-mode") { + return fmt.Errorf("%w: claude_cli must pass --safe-mode", ErrUnsafeSubprocessConfig) + } // Exactly one transport: the detached job service (--bg) or headless // foreground print mode (-p, see claudeForegroundEnabled). if containsFlag(checkedArgs, "--bg") == containsFlag(checkedArgs, "-p") { @@ -1004,6 +1020,9 @@ func (a *SubprocessAdapter) validateArgs(args []string, scratch string, req Requ return fmt.Errorf("%w: missing %s", ErrUnsafeSubprocessConfig, flag) } } + if !slices.Contains(flagValues(checkedArgs, "-c"), codexNoProjectDocsConfig) { + return fmt.Errorf("%w: codex_cli must pass -c %s", ErrUnsafeSubprocessConfig, codexNoProjectDocsConfig) + } if workspace != nil && workspace.NoNetwork { configs := flagValues(checkedArgs, "-c") for _, want := range codexNoNetworkConfig { diff --git a/internal/llmadapters/subprocess_test.go b/internal/llmadapters/subprocess_test.go index 8d40d20..1d99b70 100644 --- a/internal/llmadapters/subprocess_test.go +++ b/internal/llmadapters/subprocess_test.go @@ -806,8 +806,8 @@ func TestSubprocessCodexSafetyModes(t *testing.T) { } } assertFlagValue(t, record.AdapterArgs, "--sandbox", "read-only") - if got := strings.Join(flagValues(record.AdapterArgs, "-c"), ","); got != "model_reasoning_effort=high" { - t.Fatalf("Codex config overrides = %q, want standard-speed reasoning only", got) + if got := strings.Join(flagValues(record.AdapterArgs, "-c"), ","); got != "project_doc_max_bytes=0,model_reasoning_effort=high" { + t.Fatalf("Codex config overrides = %q, want project docs off and standard-speed reasoning only", got) } if cd := flagValue(record.AdapterArgs, "--cd"); !samePath(t, cd, record.Cwd) { t.Fatalf("flagValue(--cd) = %q, helper cwd = %q, want same path", cd, record.Cwd) @@ -872,9 +872,8 @@ func TestSubprocessCodexSafetyModes(t *testing.T) { } } assertFlagValue(t, record.AdapterArgs, "--model", "gpt-5.5") - assertFlagValue(t, record.AdapterArgs, "-c", "model_reasoning_effort=high") - if got := strings.Join(flagValues(record.AdapterArgs, "-c"), ","); got != "model_reasoning_effort=high" { - t.Fatalf("Codex resume config overrides = %q, want standard-speed reasoning only", got) + if got := strings.Join(flagValues(record.AdapterArgs, "-c"), ","); got != "project_doc_max_bytes=0,model_reasoning_effort=high" { + t.Fatalf("Codex resume config overrides = %q, want project docs off and standard-speed reasoning only", got) } promptIndex := len(argsBeforePrompt(record.AdapterArgs)) if promptIndex+2 != len(record.AdapterArgs) || record.AdapterArgs[promptIndex] != "--" || record.AdapterArgs[promptIndex+1] != "resume prompt" { @@ -909,7 +908,7 @@ func TestSubprocessCodexSafetyModes(t *testing.T) { if _, err := stream.Wait(context.Background()); err != nil { t.Fatalf("Wait: %v", err) } - if got := strings.Join(flagValues(readHelperRecord(t, recordPath).AdapterArgs, "-c"), ","); got != `model_reasoning_effort=high,service_tier="fast"` { + if got := strings.Join(flagValues(readHelperRecord(t, recordPath).AdapterArgs, "-c"), ","); got != `project_doc_max_bytes=0,model_reasoning_effort=high,service_tier="fast"` { t.Fatalf("Codex config overrides = %q, want fast service tier", got) } }) @@ -2332,6 +2331,7 @@ func TestClaudeTransportXORValidation(t *testing.T) { scratch := t.TempDir() adapter := NewClaudeCLIAdapter(SubprocessOptions{}) base := []string{ + "--safe-mode", "--tools", "Read,Write", "--add-dir", scratch, "--", "positional prompt", @@ -2669,9 +2669,84 @@ func TestSubprocessCodexDisablesNetworkOnlyForOfflineReviewer(t *testing.T) { } } if noNetwork { - if err := adapter.validateArgs(removeFlagPair(started, "-c"), scratch, req); !errors.Is(err, ErrUnsafeSubprocessConfig) { - t.Fatalf("validateArgs without network config = %v, want unsafe config", err) + withoutNetwork := started + for _, value := range codexNoNetworkConfig { + withoutNetwork = removeFlagValuePair(withoutNetwork, "-c", value) } + if err := adapter.validateArgs(withoutNetwork, scratch, req); !errors.Is(err, ErrUnsafeSubprocessConfig) || !strings.Contains(err.Error(), "offline reviewer") { + t.Fatalf("validateArgs without network config = %v, want offline reviewer unsafe config", err) + } + } + } +} + +func TestSubprocessClaudeIgnoresAmbientInstructionFiles(t *testing.T) { + adapter := NewClaudeCLIAdapter(SubprocessOptions{}) + for _, workspace := range []bool{false, true} { + req, scratch := offlineTestRequest(t, true) + if !workspace { + req = Request{Prompt: "prompt"} + } + background, err := adapter.buildArgsForSession(req, scratch, "") + if err != nil { + t.Fatalf("buildArgsForSession: %v", err) + } + resumed, err := adapter.buildArgsForSession(req, scratch, "session-1") + if err != nil { + t.Fatalf("buildArgsForSession(resume): %v", err) + } + foreground := buildClaudeForegroundArgs(req, scratch, "") + for name, args := range map[string][]string{"background": background, "resumed": resumed, "foreground": foreground} { + if !containsFlag(argsBeforePrompt(args), "--safe-mode") { + t.Fatalf("%s args = %v, want --safe-mode", name, args) + } + if err := adapter.validateArgs(args, scratch, req); err != nil { + t.Fatalf("%s validateArgs(workspace=%v): %v", name, workspace, err) + } + if err := adapter.validateArgs(removeFlag(args, "--safe-mode"), scratch, req); !errors.Is(err, ErrUnsafeSubprocessConfig) || !strings.Contains(err.Error(), "--safe-mode") { + t.Fatalf("%s validateArgs without --safe-mode = %v, want unsafe config", name, err) + } + } + } +} + +func TestSubprocessCodexIgnoresProjectInstructionFiles(t *testing.T) { + adapter := NewCodexCLIAdapter(SubprocessOptions{AllowBestEffortNoTools: true}) + for _, workspace := range []bool{false, true} { + req, scratch := offlineTestRequest(t, false) + if !workspace { + req = Request{Prompt: "prompt"} } + started, err := adapter.buildArgsForSession(req, scratch, "") + if err != nil { + t.Fatalf("buildArgsForSession: %v", err) + } + resumed, err := adapter.buildArgsForSession(req, scratch, "session-1") + if err != nil { + t.Fatalf("buildArgsForSession(resume): %v", err) + } + for name, args := range map[string][]string{"started": started, "resumed": resumed} { + if !slices.Contains(flagValues(argsBeforePrompt(args), "-c"), codexNoProjectDocsConfig) { + t.Fatalf("%s args = %v, want -c %s", name, args, codexNoProjectDocsConfig) + } + if err := adapter.validateArgs(args, scratch, req); err != nil { + t.Fatalf("%s validateArgs(workspace=%v): %v", name, workspace, err) + } + if err := adapter.validateArgs(removeFlagValuePair(args, "-c", codexNoProjectDocsConfig), scratch, req); !errors.Is(err, ErrUnsafeSubprocessConfig) || !strings.Contains(err.Error(), codexNoProjectDocsConfig) { + t.Fatalf("%s validateArgs without project doc config = %v, want unsafe config", name, err) + } + } + } +} + +func removeFlagValuePair(args []string, flag string, value string) []string { + out := make([]string, 0, len(args)) + for i := 0; i < len(args); i++ { + if args[i] == flag && i+1 < len(args) && args[i+1] == value { + i++ + continue + } + out = append(out, args[i]) } + return out }