From f46b6281207f9f54d6ae1cc1af9335028951145e Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Wed, 24 Jun 2026 15:57:01 -0400 Subject: [PATCH] feat: add checkout-readonly adapter capability checks Closes #372 --- internal/cmd/reviewcmd/reviewcmd_test.go | 2 +- internal/llm/adapter.go | 49 +++++ internal/llm/adapter_test.go | 38 ++++ internal/llm/fake.go | 17 +- internal/llm/subprocess.go | 102 ++++++++- internal/llm/subprocess_test.go | 92 +++++++- internal/pipeline/pipeline.go | 165 +++++++++++++- internal/pipeline/pipeline_test.go | 262 ++++++++++++++++++++++- 8 files changed, 706 insertions(+), 21 deletions(-) diff --git a/internal/cmd/reviewcmd/reviewcmd_test.go b/internal/cmd/reviewcmd/reviewcmd_test.go index 63a1e717..b8fbb24e 100644 --- a/internal/cmd/reviewcmd/reviewcmd_test.go +++ b/internal/cmd/reviewcmd/reviewcmd_test.go @@ -2127,7 +2127,7 @@ func TestProgressPlannerWritesRunIDBreadcrumb(t *testing.T) { statepaths.NewLayout(t.TempDir(), t.TempDir()), &errOut, logger, - runtimeOptsWithWorkbench(t, RuntimeOptions{}), + runtimeOptsWithWorkbench(t, RuntimeOptions{PRRef: ref}), ) run, err := store.AllocateRun(context.Background(), ledger.AllocateRunParams{ diff --git a/internal/llm/adapter.go b/internal/llm/adapter.go index 9cff8cde..4e250539 100644 --- a/internal/llm/adapter.go +++ b/internal/llm/adapter.go @@ -31,12 +31,56 @@ type Adapter interface { Resume(context.Context, string, Request) (Stream, error) } +// CheckoutReadonlyCapable reports whether an adapter can safely inspect a +// caller-provided read-only checkout with writes limited to a caller-owned +// scratch root. +type CheckoutReadonlyCapable interface { + SupportsCheckoutReadonly() bool +} + +// CheckoutReadonlyRequest describes bounded checkout access for a single LLM +// invocation. RootDir is the reviewer-visible read root; ScratchDir is the +// writable scratch root owned by the harness; AllowedFiles preserves the +// orchestrator's optional narrowing intent for logs and smoke-path assertions. +type CheckoutReadonlyRequest struct { + RootDir string + ScratchDir string + AllowedFiles []string + MaxToolOutputBytes int +} + +// ErrCheckoutReadonlyUnsupported reports that an adapter cannot safely provide +// checkout-readonly review access. +var ErrCheckoutReadonlyUnsupported = errors.New("llm adapter: missing checkout-readonly capability") + +// SupportsCheckoutReadonly reports whether adapter exposes the supplemental +// checkout-readonly capability. +func SupportsCheckoutReadonly(adapter Adapter) bool { + capable, ok := adapter.(CheckoutReadonlyCapable) + return ok && capable.SupportsCheckoutReadonly() +} + +// RequireCheckoutReadonly returns a stable error when adapter does not expose +// the supplemental checkout-readonly capability. +func RequireCheckoutReadonly(adapter Adapter) error { + if SupportsCheckoutReadonly(adapter) { + return nil + } + name := "" + if adapter != nil { + name = adapter.Name() + } + return fmt.Errorf("%w: %s", ErrCheckoutReadonlyUnsupported, name) +} + // Request describes one LLM invocation. type Request struct { Model string Effort string Prompt string LogPath string + + CheckoutReadonly *CheckoutReadonlyRequest } // Stream is a started LLM request. @@ -216,6 +260,11 @@ func runOnceAttempt(ctx context.Context, adapter Adapter, resumeSessionID string stream Stream err error ) + if req.CheckoutReadonly != nil { + if err := RequireCheckoutReadonly(adapter); err != nil { + return "", Response{}, err + } + } if strings.TrimSpace(resumeSessionID) != "" && adapter.SupportsResume() { stream, err = adapter.Resume(ctx, resumeSessionID, req) } else { diff --git a/internal/llm/adapter_test.go b/internal/llm/adapter_test.go index 6548b352..10519e02 100644 --- a/internal/llm/adapter_test.go +++ b/internal/llm/adapter_test.go @@ -471,6 +471,44 @@ func TestFakeAdapterQuotaAndResume(t *testing.T) { } } +func TestCheckoutReadonlyCapabilityHelpers(t *testing.T) { + unsupported := &FakeAdapter{NameValue: "fake-unsupported"} + if SupportsCheckoutReadonly(unsupported) { + t.Fatal("SupportsCheckoutReadonly = true, want false") + } + if err := RequireCheckoutReadonly(unsupported); !errors.Is(err, ErrCheckoutReadonlyUnsupported) || !strings.Contains(err.Error(), "fake-unsupported") { + t.Fatalf("RequireCheckoutReadonly unsupported error = %v, want missing capability with adapter name", err) + } + + supported := &FakeAdapter{NameValue: "fake-supported", SupportsCheckoutReadonlyValue: true} + if !SupportsCheckoutReadonly(supported) { + t.Fatal("SupportsCheckoutReadonly = false, want true") + } + if err := RequireCheckoutReadonly(supported); err != nil { + t.Fatalf("RequireCheckoutReadonly supported: %v", err) + } + + got, _, err := RunStructured(context.Background(), unsupported, Request{ + Prompt: "prompt", + CheckoutReadonly: &CheckoutReadonlyRequest{ + RootDir: "/tmp/repo", + ScratchDir: "/tmp/scratch", + MaxToolOutputBytes: 1024, + }, + }, func(data []byte) (string, error) { + return string(data), nil + }) + if got != "" { + t.Fatalf("RunStructured result = %q, want empty on capability failure", got) + } + if !errors.Is(err, ErrCheckoutReadonlyUnsupported) || !strings.Contains(err.Error(), "fake-unsupported") { + t.Fatalf("RunStructured error = %v, want missing capability with adapter name", err) + } + if len(unsupported.Requests()) != 0 || len(unsupported.Resumes()) != 0 { + t.Fatalf("unsupported adapter should not be invoked: starts=%d resumes=%d", len(unsupported.Requests()), len(unsupported.Resumes())) + } +} + func TestRunStructuredRetriesTransientError(t *testing.T) { restore := activeRetryPolicy activeRetryPolicy.Base = 0 diff --git a/internal/llm/fake.go b/internal/llm/fake.go index 71785d64..aec85ec1 100644 --- a/internal/llm/fake.go +++ b/internal/llm/fake.go @@ -11,10 +11,11 @@ import ( type FakeAdapter struct { mu sync.Mutex - NameValue string - SupportsResumeValue bool - SupportsCacheAccountingValue bool - SupportsCostReportingValue bool + NameValue string + SupportsResumeValue bool + SupportsCheckoutReadonlyValue bool + SupportsCacheAccountingValue bool + SupportsCostReportingValue bool QuotaValue Quota QuotaSupported bool @@ -56,6 +57,14 @@ func (f *FakeAdapter) SupportsResume() bool { return f.SupportsResumeValue } +// SupportsCheckoutReadonly reports whether the fake supports checkout-readonly +// invocation mode. +func (f *FakeAdapter) SupportsCheckoutReadonly() bool { + f.mu.Lock() + defer f.mu.Unlock() + return f.SupportsCheckoutReadonlyValue +} + // SupportsCacheAccounting reports whether cache usage metrics are supported. func (f *FakeAdapter) SupportsCacheAccounting() bool { f.mu.Lock() diff --git a/internal/llm/subprocess.go b/internal/llm/subprocess.go index 131ff916..1138bc30 100644 --- a/internal/llm/subprocess.go +++ b/internal/llm/subprocess.go @@ -42,8 +42,9 @@ type SubprocessOptions struct { type subprocessKind string const ( - subprocessClaude subprocessKind = "claude_cli" - subprocessCodex subprocessKind = "codex_cli" + subprocessClaude subprocessKind = "claude_cli" + subprocessCodex subprocessKind = "codex_cli" + logBytesUnlimited = -1 claudeBGPromptFilename = "cr-prompt.txt" claudeBGResultFilename = "cr-result.json" @@ -117,6 +118,13 @@ func (a *SubprocessAdapter) Name() string { return string(a.kind) } // SupportsResume reports whether subprocess session resume is implemented. func (a *SubprocessAdapter) SupportsResume() bool { return a.kind == subprocessClaude } +// SupportsCheckoutReadonly reports whether subprocess adapter can safely mount +// a caller-owned read-only checkout and keep writes inside caller-owned scratch. +// Codex CLI can do this with a read-only sandbox plus caller-owned scratch; the +// current Claude background path cannot because it relies on a different launch +// model and writable result-file handoff. +func (a *SubprocessAdapter) SupportsCheckoutReadonly() bool { return a.kind == subprocessCodex } + // SupportsCacheAccounting reports whether cache usage metrics are guaranteed. func (a *SubprocessAdapter) SupportsCacheAccounting() bool { return false } @@ -140,7 +148,7 @@ func (a *SubprocessAdapter) Start(ctx context.Context, req Request) (Stream, err } func (a *SubprocessAdapter) startJSONLSubprocess(ctx context.Context, req Request) (Stream, error) { - scratch, cleanup, err := a.scratchDirFactory() + scratch, cleanup, err := a.invocationScratchDir(req) if err != nil { return nil, err } @@ -157,7 +165,7 @@ func (a *SubprocessAdapter) startJSONLSubprocess(ctx context.Context, req Reques _ = cleanup() return nil, err } - if err := a.validateArgs(args, scratch); err != nil { + if err := a.validateArgs(args, scratch, req); err != nil { _ = cleanup() return nil, err } @@ -227,6 +235,7 @@ func (a *SubprocessAdapter) startJSONLSubprocess(ctx context.Context, req Reques cancel: cancel, done: make(chan struct{}), logFile: logFile, + logBytesLeft: checkoutReadonlyLogBytes(req), cleanup: cleanup, processGroup: procGroup, } @@ -235,7 +244,7 @@ func (a *SubprocessAdapter) startJSONLSubprocess(ctx context.Context, req Reques } func (a *SubprocessAdapter) startClaudeBG(ctx context.Context, req Request, resumeSessionID string) (Stream, error) { - scratch, cleanup, err := a.scratchDirFactory() + scratch, cleanup, err := a.invocationScratchDir(req) if err != nil { return nil, err } @@ -256,7 +265,7 @@ func (a *SubprocessAdapter) startClaudeBG(ctx context.Context, req Request, resu _ = cleanup() return nil, err } - if err := a.validateArgs(args, scratch); err != nil { + if err := a.validateArgs(args, scratch, req); err != nil { _ = cleanup() return nil, err } @@ -330,6 +339,7 @@ func (a *SubprocessAdapter) startClaudeBG(ctx context.Context, req Request, resu cancel: cancel, done: make(chan struct{}), logFile: logFile, + logBytesLeft: checkoutReadonlyLogBytes(req), cleanup: cleanup, processGroup: procGroup, } @@ -386,6 +396,9 @@ func (a *SubprocessAdapter) buildArgsForSession(req Request, scratch string, res "--sandbox", "read-only", "--cd", scratch, } + if req.CheckoutReadonly != nil { + args = append(args, "--add-dir", req.CheckoutReadonly.RootDir) + } if req.Model != "" { args = append(args, "--model", req.Model) } @@ -398,7 +411,7 @@ func (a *SubprocessAdapter) buildArgsForSession(req Request, scratch string, res } } -func (a *SubprocessAdapter) validateArgs(args []string, scratch string) error { +func (a *SubprocessAdapter) validateArgs(args []string, scratch string, req Request) error { checkedArgs := argsBeforePrompt(args) if containsFlag(checkedArgs, "--search") { return fmt.Errorf("%w: unsafe tool-enabling flag present", ErrUnsafeSubprocessConfig) @@ -440,7 +453,7 @@ func (a *SubprocessAdapter) validateArgs(args []string, scratch string) error { return fmt.Errorf("%w: claude_cli must restrict add-dir to scratch dir", ErrUnsafeSubprocessConfig) } case subprocessCodex: - if containsFlag(checkedArgs, "--add-dir") { + if req.CheckoutReadonly == nil && containsFlag(checkedArgs, "--add-dir") { return fmt.Errorf("%w: unsafe tool-enabling flag present", ErrUnsafeSubprocessConfig) } if len(checkedArgs) == 0 || checkedArgs[0] != "exec" { @@ -452,6 +465,12 @@ func (a *SubprocessAdapter) validateArgs(args []string, scratch string) error { if flagValue(checkedArgs, "--cd") != scratch { return fmt.Errorf("%w: codex_cli must use scratch cwd", ErrUnsafeSubprocessConfig) } + if req.CheckoutReadonly != nil { + addDir, ok := flagValueOK(checkedArgs, "--add-dir") + if !ok || !sameCleanPath(addDir, req.CheckoutReadonly.RootDir) { + return fmt.Errorf("%w: codex_cli must add only the checkout-readonly root", ErrUnsafeSubprocessConfig) + } + } for _, flag := range []string{"--json", "--ephemeral", "--skip-git-repo-check", "--ignore-user-config", "--ignore-rules"} { if !containsFlag(checkedArgs, flag) { return fmt.Errorf("%w: missing %s", ErrUnsafeSubprocessConfig, flag) @@ -461,6 +480,30 @@ func (a *SubprocessAdapter) validateArgs(args []string, scratch string) error { return nil } +func (a *SubprocessAdapter) invocationScratchDir(req Request) (string, func() error, error) { + if req.CheckoutReadonly == nil { + return a.scratchDirFactory() + } + if a.kind != subprocessCodex { + return "", nil, RequireCheckoutReadonly(a) + } + root := strings.TrimSpace(req.CheckoutReadonly.ScratchDir) + if root == "" { + return "", nil, fmt.Errorf("%w: checkout-readonly scratch dir is required", ErrUnsafeSubprocessConfig) + } + if strings.TrimSpace(req.CheckoutReadonly.RootDir) == "" { + return "", nil, fmt.Errorf("%w: checkout-readonly root dir is required", ErrUnsafeSubprocessConfig) + } + if err := os.MkdirAll(root, 0o700); err != nil { + return "", nil, fmt.Errorf("llm subprocess: create checkout-readonly scratch root: %w", err) + } + scratch, err := os.MkdirTemp(root, "llm-subprocess-*") + if err != nil { + return "", nil, fmt.Errorf("llm subprocess: create checkout-readonly scratch dir: %w", err) + } + return scratch, func() error { return os.RemoveAll(scratch) }, nil +} + type subprocessStream struct { mu sync.Mutex sessionID string @@ -470,6 +513,8 @@ type subprocessStream struct { done chan struct{} logMu sync.Mutex logFile *os.File + logBytesLeft int + logCapped bool cleanup func() error processGroup *processGroup } @@ -663,7 +708,33 @@ func (w subprocessLogWriter) Write(p []byte) (int, error) { if w.stream.logFile == nil { return len(p), nil } - return w.stream.logFile.Write(p) + if w.stream.logBytesLeft == 0 { + if !w.stream.logCapped { + if _, err := w.stream.logFile.Write([]byte("warning: checkout-readonly tool output cap reached; further subprocess logs truncated\n")); err != nil { + return 0, err + } + w.stream.logCapped = true + } + return len(p), nil + } + writeBytes := p + if w.stream.logBytesLeft > 0 && len(writeBytes) > w.stream.logBytesLeft { + writeBytes = writeBytes[:w.stream.logBytesLeft] + } + n, err := w.stream.logFile.Write(writeBytes) + if w.stream.logBytesLeft > 0 { + w.stream.logBytesLeft -= n + } + if w.stream.logBytesLeft == 0 && !w.stream.logCapped { + if _, writeErr := w.stream.logFile.Write([]byte("warning: checkout-readonly tool output cap reached; further subprocess logs truncated\n")); writeErr != nil && err == nil { + err = writeErr + } + w.stream.logCapped = true + } + if err != nil { + return n, err + } + return len(p), nil } func (s *subprocessStream) writeLog(p []byte) { @@ -705,6 +776,19 @@ func (s *subprocessStream) setSessionID(id string) { } } +func checkoutReadonlyLogBytes(req Request) int { + if req.CheckoutReadonly == nil { + return logBytesUnlimited + } + if req.CheckoutReadonly.MaxToolOutputBytes == logBytesUnlimited { + return logBytesUnlimited + } + if req.CheckoutReadonly.MaxToolOutputBytes < 0 { + return 0 + } + return req.CheckoutReadonly.MaxToolOutputBytes +} + type subprocessEvent struct { sessionID string structuredOutput []byte diff --git a/internal/llm/subprocess_test.go b/internal/llm/subprocess_test.go index 555f087c..f262dd1a 100644 --- a/internal/llm/subprocess_test.go +++ b/internal/llm/subprocess_test.go @@ -568,6 +568,88 @@ func TestSubprocessCodexSafetyModes(t *testing.T) { } } }) + + t.Run("checkout readonly adds scoped read root and keeps scratch cwd", func(t *testing.T) { + tempDir := t.TempDir() + recordPath := filepath.Join(tempDir, "records.jsonl") + scratchRoot := filepath.Join(tempDir, "workbench-scratch") + if err := os.MkdirAll(scratchRoot, 0o700); err != nil { + t.Fatalf("MkdirAll(scratchRoot): %v", err) + } + repoRoot := filepath.Join(tempDir, "workbench-repo") + if err := os.MkdirAll(repoRoot, 0o700); err != nil { + t.Fatalf("MkdirAll(repoRoot): %v", err) + } + adapter := newCodexHelperAdapter("success", recordPath, 5*time.Second) + stream, err := adapter.Start(context.Background(), Request{ + Model: "gpt-5.5", + Effort: "high", + Prompt: "prompt", + CheckoutReadonly: &CheckoutReadonlyRequest{ + RootDir: repoRoot, + ScratchDir: scratchRoot, + MaxToolOutputBytes: 2048, + }, + }) + if err != nil { + t.Fatalf("Start: %v", err) + } + if _, err := stream.Wait(context.Background()); err != nil { + t.Fatalf("Wait: %v", err) + } + record := readHelperRecord(t, recordPath) + assertFlagValue(t, record.AdapterArgs, "--add-dir", repoRoot) + cwd := strings.TrimPrefix(filepath.Clean(record.Cwd), "/private") + wantRoot := strings.TrimPrefix(filepath.Clean(scratchRoot), "/private") + if !strings.HasPrefix(cwd, wantRoot+string(filepath.Separator)) { + t.Fatalf("cwd = %q, want invocation scratch under %q", record.Cwd, scratchRoot) + } + if samePath(t, record.Cwd, scratchRoot) { + t.Fatalf("cwd = %q, want child scratch dir rather than root", record.Cwd) + } + }) + + t.Run("checkout readonly caps subprocess logs", func(t *testing.T) { + tempDir := t.TempDir() + recordPath := filepath.Join(tempDir, "records.jsonl") + logPath := filepath.Join(tempDir, "events.log") + scratchRoot := filepath.Join(tempDir, "workbench-scratch") + if err := os.MkdirAll(scratchRoot, 0o700); err != nil { + t.Fatalf("MkdirAll(scratchRoot): %v", err) + } + repoRoot := filepath.Join(tempDir, "workbench-repo") + if err := os.MkdirAll(repoRoot, 0o700); err != nil { + t.Fatalf("MkdirAll(repoRoot): %v", err) + } + adapter := newCodexHelperAdapter("noisy-success", recordPath, 5*time.Second) + stream, err := adapter.Start(context.Background(), Request{ + Prompt: "prompt", + LogPath: logPath, + CheckoutReadonly: &CheckoutReadonlyRequest{ + RootDir: repoRoot, + ScratchDir: scratchRoot, + MaxToolOutputBytes: 64, + }, + }) + if err != nil { + t.Fatalf("Start: %v", err) + } + if _, err := stream.Wait(context.Background()); err != nil { + t.Fatalf("Wait: %v", err) + } + logged, err := os.ReadFile(logPath) // #nosec G304 -- test reads the log path it created with t.TempDir. + if err != nil { + t.Fatalf("ReadFile(log): %v", err) + } + logText := string(logged) + warningText := "warning: checkout-readonly tool output cap reached; further subprocess logs truncated\n" + if !strings.Contains(logText, warningText) { + t.Fatalf("log = %q, want truncation warning", logText) + } + if len(logged) > 64+len(warningText) { + t.Fatalf("log bytes = %d, want capped output at %d plus warning", len(logged), 64+len(warningText)) + } + }) } func TestSubprocessCodexToolUseAndProtocolFailures(t *testing.T) { @@ -650,7 +732,7 @@ func TestSubprocessRejectsUnsafeSpecs(t *testing.T) { {name: "prompt argv", args: append(append([]string(nil), claudeArgs...), "--", "prompt")}, } { t.Run("claude "+tt.name, func(t *testing.T) { - if err := claude.validateArgs(tt.args, scratch); !errors.Is(err, ErrUnsafeSubprocessConfig) { + if err := claude.validateArgs(tt.args, scratch, Request{Prompt: "prompt"}); !errors.Is(err, ErrUnsafeSubprocessConfig) { t.Fatalf("validateArgs error = %v, want ErrUnsafeSubprocessConfig", err) } }) @@ -658,7 +740,7 @@ func TestSubprocessRejectsUnsafeSpecs(t *testing.T) { inlineClaudeArgs := replaceSubprocessFlagPairWithEquals(claudeArgs, "--tools") inlineClaudeArgs = replaceSubprocessFlagPairWithEquals(inlineClaudeArgs, "--permission-mode") inlineClaudeArgs = replaceSubprocessFlagPairWithEquals(inlineClaudeArgs, "--add-dir") - if err := claude.validateArgs(inlineClaudeArgs, scratch); err != nil { + if err := claude.validateArgs(inlineClaudeArgs, scratch, Request{Prompt: "prompt"}); err != nil { t.Fatalf("validateArgs(claude inline flags): %v", err) } @@ -676,7 +758,7 @@ func TestSubprocessRejectsUnsafeSpecs(t *testing.T) { {name: "add dir", args: append([]string{"--add-dir", t.TempDir()}, codexArgs...)}, } { t.Run("codex "+tt.name, func(t *testing.T) { - if err := codex.validateArgs(tt.args, codexScratch); !errors.Is(err, ErrUnsafeSubprocessConfig) { + if err := codex.validateArgs(tt.args, codexScratch, Request{Prompt: "prompt"}); !errors.Is(err, ErrUnsafeSubprocessConfig) { t.Fatalf("validateArgs error = %v, want ErrUnsafeSubprocessConfig", err) } }) @@ -932,6 +1014,10 @@ func TestSubprocessHelperProcess(_ *testing.T) { fmt.Println(`{"type":"thread.started","thread_id":"session-1"}`) fmt.Println(`{"type":"unknown.event","ignored":true}`) fmt.Println(`{"type":"item.completed","item":{"id":"item_0","type":"agent_message","text":"{\"ok\":true}"}}`) + case "noisy-success": + fmt.Fprintln(os.Stderr, strings.Repeat("stderr-noise-", 32)) + fmt.Println(`{"type":"thread.started","thread_id":"session-1"}`) + fmt.Println(`{"type":"item.completed","item":{"id":"item_0","type":"agent_message","text":"{\"ok\":true}"}}`) case "tool": fmt.Println(`{"type":"thread.started","thread_id":"session-1"}`) fmt.Println(`{"type":"tool_use","name":"Read"}`) diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 7522a480..816a88c8 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -40,9 +40,10 @@ import ( ) const ( - defaultMaxAgents = 5 - defaultMaxConcurrency = 5 - defaultMaxPromptBytes = 512 * 1024 + defaultMaxAgents = 5 + defaultMaxConcurrency = 5 + defaultMaxPromptBytes = 512 * 1024 + defaultCheckoutReadonlyToolOutputBytes = 32 * 1024 ) // ErrStructuredOutputInvalidAfterRetry marks a selector or rollup response that @@ -3317,6 +3318,164 @@ func workbenchBranchArtifactFromRef(ref gitprovider.PRBranchRef) workbenchBranch } } +func buildCheckoutReadonlyRequest(opts Options, artifacts ArtifactPaths, agentID string, allowedFiles []string, model, effort, prompt, logPath string) (llm.Request, error) { + if err := llm.RequireCheckoutReadonly(opts.Adapter); err != nil { + return llm.Request{}, fmt.Errorf("pipeline: %w", err) + } + access, err := prepareCheckoutReadonlyAccess(artifacts, agentID, allowedFiles, defaultCheckoutReadonlyToolOutputBytes) + if err != nil { + return llm.Request{}, err + } + return llm.Request{ + Model: model, + Effort: effort, + Prompt: prompt, + LogPath: logPath, + CheckoutReadonly: &access, + }, nil +} + +func prepareCheckoutReadonlyAccess(artifacts ArtifactPaths, agentID string, allowedFiles []string, maxToolOutputBytes int) (llm.CheckoutReadonlyRequest, error) { + if strings.TrimSpace(artifacts.WorkbenchRepoDir) == "" { + return llm.CheckoutReadonlyRequest{}, fmt.Errorf("pipeline: workbench repo dir is required for checkout-readonly access") + } + if strings.TrimSpace(artifacts.WorkbenchScratch) == "" { + return llm.CheckoutReadonlyRequest{}, fmt.Errorf("pipeline: workbench scratch dir is required for checkout-readonly access") + } + rootDir := artifacts.WorkbenchRepoDir + if len(allowedFiles) > 0 { + scopeDir, err := materializeCheckoutReadonlyScope(artifacts, agentID, allowedFiles) + if err != nil { + return llm.CheckoutReadonlyRequest{}, err + } + rootDir = scopeDir + } + return llm.CheckoutReadonlyRequest{ + RootDir: rootDir, + ScratchDir: artifacts.WorkbenchScratch, + AllowedFiles: append([]string(nil), allowedFiles...), + MaxToolOutputBytes: maxToolOutputBytes, + }, nil +} + +func materializeCheckoutReadonlyScope(artifacts ArtifactPaths, agentID string, allowedFiles []string) (string, error) { + if strings.TrimSpace(agentID) == "" { + return "", fmt.Errorf("pipeline: agent ID is required for checkout-readonly scope") + } + scopeRoot := filepath.Join(artifacts.WorkbenchDir, "scopes", statepaths.Encode(agentID)) + if err := resetCheckoutReadonlyScope(scopeRoot); err != nil { + return "", err + } + if err := os.MkdirAll(scopeRoot, 0o700); err != nil { + return "", fmt.Errorf("pipeline: create checkout-readonly scope: %w", err) + } + for _, path := range allowedFiles { + clean := filepath.Clean(strings.TrimSpace(path)) + if isCheckoutScopeEscapePath(clean) { + return "", fmt.Errorf("pipeline: invalid checkout-readonly allowed file %q", path) + } + target := filepath.Join(artifacts.WorkbenchRepoDir, clean) + if err := validateCheckoutReadonlyScopeTarget(target, clean); err != nil { + return "", err + } + link := filepath.Join(scopeRoot, clean) + if err := os.MkdirAll(filepath.Dir(link), 0o700); err != nil { + return "", fmt.Errorf("pipeline: create checkout-readonly scope dir: %w", err) + } + if err := os.Symlink(target, link); err != nil { + return "", fmt.Errorf("pipeline: link checkout-readonly scope file %s: %w", clean, err) + } + } + if err := lockCheckoutReadonlyScope(scopeRoot); err != nil { + return "", err + } + return scopeRoot, nil +} + +func resetCheckoutReadonlyScope(scopeRoot string) error { + if _, err := os.Stat(scopeRoot); err == nil { + if unlockErr := unlockCheckoutReadonlyScope(scopeRoot); unlockErr != nil { + return unlockErr + } + } else if err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("pipeline: stat checkout-readonly scope: %w", err) + } + if err := os.RemoveAll(scopeRoot); err != nil { + return fmt.Errorf("pipeline: reset checkout-readonly scope: %w", err) + } + return nil +} + +func isCheckoutScopeEscapePath(clean string) bool { + return clean == "." || clean == "" || filepath.IsAbs(clean) || clean == ".." || strings.HasPrefix(clean, ".."+string(os.PathSeparator)) +} + +func validateCheckoutReadonlyScopeTarget(target string, displayPath string) error { + info, err := os.Lstat(target) // #nosec G304 -- target is derived from the pipeline-owned workbench checkout root plus validated relative paths. + if err != nil { + return fmt.Errorf("pipeline: stat checkout-readonly scope file %s: %w", displayPath, err) + } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("pipeline: checkout-readonly scope file %s must not be a symlink", displayPath) + } + if !info.Mode().IsRegular() { + return fmt.Errorf("pipeline: checkout-readonly scope file %s must be a regular file", displayPath) + } + return nil +} + +func lockCheckoutReadonlyScope(scopeRoot string) error { + if err := filepath.Walk(scopeRoot, func(path string, info fs.FileInfo, err error) error { + if err != nil { + return err + } + if info.Mode()&os.ModeSymlink != 0 { + return nil + } + var mode fs.FileMode + if info.IsDir() { + mode = 0o555 + } else { + mode = 0o444 + } + if chmodErr := os.Chmod(path, mode); chmodErr != nil { // #nosec G302,G122 -- scope files are pipeline-owned under the workbench artifact root; symlinks are skipped during the walk. + return chmodErr + } + return nil + }); err != nil { + return fmt.Errorf("pipeline: lock checkout-readonly scope: %w", err) + } + return nil +} + +func unlockCheckoutReadonlyScope(scopeRoot string) error { + scopeRoot = strings.TrimSpace(scopeRoot) + if scopeRoot == "" { + return nil + } + if err := filepath.Walk(scopeRoot, func(path string, info fs.FileInfo, err error) error { + if err != nil { + return err + } + if info.Mode()&os.ModeSymlink != 0 { + return nil + } + var mode fs.FileMode + if info.IsDir() { + mode = 0o700 + } else { + mode = 0o600 + } + if chmodErr := os.Chmod(path, mode); chmodErr != nil { // #nosec G302,G122 -- scope files are pipeline-owned under the workbench artifact root; symlinks are skipped during the walk. + return chmodErr + } + return nil + }); err != nil { + return fmt.Errorf("pipeline: unlock checkout-readonly scope: %w", err) + } + return nil +} + type workbenchRemoteStyle struct { scheme string user string diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 58f4f3db..53b8ead5 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -1772,6 +1772,213 @@ func TestDryRunPreparesWorkbenchInAllocatedRunArtifacts(t *testing.T) { } } +func TestPrepareCheckoutReadonlyAccessSmokeAllowsReadAndScratchWrite(t *testing.T) { + ctx := context.Background() + provider, req := dryRunHarness(t) + artifacts := ArtifactPathsFromDir(t.TempDir()) + adapter := &checkoutReadonlySmokeAdapter{} + opts := Options{Provider: provider, Adapter: adapter} + configureWorkbenchFixtureForTest(ctx, &opts, req.PRRef) + prepared, err := prepareSelectionContext(ctx, opts, selectionSetupRequest{ + PRRef: req.PRRef, + Profile: req.Profile, + AgentDirs: req.AgentDirs, + ReviewBaseSHA: req.ReviewBaseSHA, + ReviewHeadSHA: req.ReviewHeadSHA, + ResolveArtifacts: func(gitprovider.PR) (ArtifactPaths, error) { + return artifacts, nil + }, + }) + if err != nil { + t.Fatalf("prepareSelectionContext: %v", err) + } + if err := prepareWorkbenchArtifacts(ctx, opts, workbenchPreparationRequest{ + PRRef: req.PRRef, + ReviewPR: prepared.reviewPR, + ChangedFiles: prepared.changedFiles, + Artifacts: artifacts, + }); err != nil { + t.Fatalf("prepareWorkbenchArtifacts: %v", err) + } + defer func() { + if err := unlockWorkbenchRepo(artifacts.WorkbenchRepoDir); err != nil { + t.Fatalf("unlockWorkbenchRepo: %v", err) + } + }() + llmReq, err := buildCheckoutReadonlyRequest(Options{Adapter: adapter}, artifacts, "harness:smoke", nil, "gpt-5.5", "medium", "smoke", filepath.Join(t.TempDir(), "smoke.jsonl")) + if err != nil { + t.Fatalf("buildCheckoutReadonlyRequest: %v", err) + } + type smokeResult struct { + ReadOK bool `json:"read_ok"` + MainContainsChanged bool `json:"main_contains_changed"` + DeniedOutOfScope bool `json:"denied_out_of_scope"` + TrackedWriteDenied bool `json:"tracked_write_denied"` + UntrackedWriteDenied bool `json:"untracked_write_denied"` + ScratchWriteOK bool `json:"scratch_write_ok"` + MaxToolOutputBytes int `json:"max_tool_output_bytes"` + } + got, _, err := llm.RunStructured(context.Background(), adapter, llmReq, func(data []byte) (smokeResult, error) { + var out smokeResult + return out, json.Unmarshal(data, &out) + }) + if err != nil { + t.Fatalf("RunStructured: %v", err) + } + requests := adapter.Requests() + if len(requests) != 1 { + t.Fatalf("adapter requests = %d, want one smoke invocation", len(requests)) + } + if requests[0].CheckoutReadonly == nil || requests[0].CheckoutReadonly.RootDir != artifacts.WorkbenchRepoDir { + t.Fatalf("checkout readonly request = %#v, want workbench repo root", requests[0].CheckoutReadonly) + } + if requests[0].CheckoutReadonly.MaxToolOutputBytes != defaultCheckoutReadonlyToolOutputBytes { + t.Fatalf("max tool output bytes = %d, want default %d", requests[0].CheckoutReadonly.MaxToolOutputBytes, defaultCheckoutReadonlyToolOutputBytes) + } + if !got.ReadOK || !got.MainContainsChanged || !got.TrackedWriteDenied || !got.UntrackedWriteDenied || !got.ScratchWriteOK { + t.Fatalf("smoke result = %#v, want read success, repo write denial, and scratch write success", got) + } +} + +func TestPrepareCheckoutReadonlyAccessAllowedFilesNarrowsReadScope(t *testing.T) { + ctx := context.Background() + provider, req := dryRunHarness(t) + artifacts := ArtifactPathsFromDir(t.TempDir()) + opts := Options{Provider: provider, Adapter: &checkoutReadonlySmokeAdapter{}} + configureWorkbenchFixtureForTest(ctx, &opts, req.PRRef) + prepared, err := prepareSelectionContext(ctx, opts, selectionSetupRequest{ + PRRef: req.PRRef, + Profile: req.Profile, + AgentDirs: req.AgentDirs, + ReviewBaseSHA: req.ReviewBaseSHA, + ReviewHeadSHA: req.ReviewHeadSHA, + ResolveArtifacts: func(gitprovider.PR) (ArtifactPaths, error) { + return artifacts, nil + }, + }) + if err != nil { + t.Fatalf("prepareSelectionContext: %v", err) + } + if err := prepareWorkbenchArtifacts(ctx, opts, workbenchPreparationRequest{ + PRRef: req.PRRef, + ReviewPR: prepared.reviewPR, + ChangedFiles: prepared.changedFiles, + Artifacts: artifacts, + }); err != nil { + t.Fatalf("prepareWorkbenchArtifacts: %v", err) + } + defer func() { + if err := unlockWorkbenchRepo(artifacts.WorkbenchRepoDir); err != nil { + t.Fatalf("unlockWorkbenchRepo: %v", err) + } + }() + access, err := prepareCheckoutReadonlyAccess(artifacts, "harness:smoke", []string{"main.go"}, 1024) + if err != nil { + t.Fatalf("prepareCheckoutReadonlyAccess: %v", err) + } + defer func() { + if err := unlockCheckoutReadonlyScope(access.RootDir); err != nil { + t.Fatalf("unlockCheckoutReadonlyScope: %v", err) + } + }() + if access.RootDir == artifacts.WorkbenchRepoDir { + t.Fatalf("root dir = %q, want narrowed scope dir distinct from workbench repo", access.RootDir) + } + if _, err := os.ReadFile(filepath.Join(access.RootDir, "main.go")); err != nil { // #nosec G304 -- test reads only caller-produced scope path. + t.Fatalf("ReadFile(main.go): %v", err) + } + if _, err := os.ReadFile(filepath.Join(artifacts.WorkbenchRepoDir, "other.go")); err != nil { // #nosec G304 -- test reads only caller-produced workbench path. + t.Fatalf("ReadFile(workbench other.go): %v", err) + } + if _, err := os.ReadFile(filepath.Join(access.RootDir, "other.go")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("ReadFile(other.go) err = %v, want not exist outside allowed scope", err) + } +} + +func TestPrepareCheckoutReadonlyAccessAllowedFilesResetsExistingScope(t *testing.T) { + ctx := context.Background() + provider, req := dryRunHarness(t) + artifacts := ArtifactPathsFromDir(t.TempDir()) + opts := Options{Provider: provider, Adapter: &checkoutReadonlySmokeAdapter{}} + configureWorkbenchFixtureForTest(ctx, &opts, req.PRRef) + prepared, err := prepareSelectionContext(ctx, opts, selectionSetupRequest{ + PRRef: req.PRRef, + Profile: req.Profile, + AgentDirs: req.AgentDirs, + ReviewBaseSHA: req.ReviewBaseSHA, + ReviewHeadSHA: req.ReviewHeadSHA, + ResolveArtifacts: func(gitprovider.PR) (ArtifactPaths, error) { + return artifacts, nil + }, + }) + if err != nil { + t.Fatalf("prepareSelectionContext: %v", err) + } + if err := prepareWorkbenchArtifacts(ctx, opts, workbenchPreparationRequest{ + PRRef: req.PRRef, + ReviewPR: prepared.reviewPR, + ChangedFiles: prepared.changedFiles, + Artifacts: artifacts, + }); err != nil { + t.Fatalf("prepareWorkbenchArtifacts: %v", err) + } + defer func() { + if err := unlockWorkbenchRepo(artifacts.WorkbenchRepoDir); err != nil { + t.Fatalf("unlockWorkbenchRepo: %v", err) + } + }() + access, err := prepareCheckoutReadonlyAccess(artifacts, "harness:scope-reset", []string{"main.go"}, 1024) + if err != nil { + t.Fatalf("prepareCheckoutReadonlyAccess(first): %v", err) + } + defer func() { + if err := unlockCheckoutReadonlyScope(access.RootDir); err != nil { + t.Fatalf("unlockCheckoutReadonlyScope: %v", err) + } + }() + access, err = prepareCheckoutReadonlyAccess(artifacts, "harness:scope-reset", []string{"other.go"}, 1024) + if err != nil { + t.Fatalf("prepareCheckoutReadonlyAccess(second): %v", err) + } + if _, err := os.ReadFile(filepath.Join(access.RootDir, "other.go")); err != nil { // #nosec G304 -- test reads only caller-produced scope path. + t.Fatalf("ReadFile(other.go): %v", err) + } + if _, err := os.ReadFile(filepath.Join(access.RootDir, "main.go")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("ReadFile(main.go) err = %v, want not exist after scope reset", err) + } +} + +func TestPrepareCheckoutReadonlyAccessRejectsSymlinkTargets(t *testing.T) { + artifacts := ArtifactPathsFromDir(t.TempDir()) + if err := os.MkdirAll(artifacts.WorkbenchRepoDir, 0o700); err != nil { + t.Fatalf("MkdirAll(repo): %v", err) + } + if err := os.MkdirAll(artifacts.WorkbenchScratch, 0o700); err != nil { + t.Fatalf("MkdirAll(scratch): %v", err) + } + if err := os.WriteFile(filepath.Join(artifacts.WorkbenchRepoDir, "real.go"), []byte("package main\n"), 0o600); err != nil { + t.Fatalf("WriteFile(real.go): %v", err) + } + if err := os.Symlink(filepath.Join(artifacts.WorkbenchRepoDir, "real.go"), filepath.Join(artifacts.WorkbenchRepoDir, "link.go")); err != nil { + t.Fatalf("Symlink(link.go): %v", err) + } + _, err := prepareCheckoutReadonlyAccess(artifacts, "harness:symlink", []string{"link.go"}, 1024) + if err == nil || !strings.Contains(err.Error(), "must not be a symlink") { + t.Fatalf("prepareCheckoutReadonlyAccess error = %v, want symlink rejection", err) + } +} + +func TestBuildCheckoutReadonlyRequestUnsupportedAdapterFailsWithoutFallback(t *testing.T) { + artifacts := ArtifactPathsFromDir(t.TempDir()) + req, err := buildCheckoutReadonlyRequest(Options{Adapter: &promptAwareAdapter{}}, artifacts, "harness:smoke", nil, "gpt-5.5", "medium", "smoke", filepath.Join(t.TempDir(), "smoke.jsonl")) + if err == nil || !strings.Contains(err.Error(), "checkout-readonly capability") { + t.Fatalf("buildCheckoutReadonlyRequest error = %v, want missing checkout-readonly capability", err) + } + if (req != llm.Request{}) { + t.Fatalf("request = %#v, want zero request on unsupported adapter", req) + } +} + func TestDryRunNoDiffDoesNotResolveUnmappedModelTier(t *testing.T) { ctx := context.Background() store := openPipelineStore(t) @@ -4585,6 +4792,56 @@ func (a *reviewerIsolationAdapter) Start(_ context.Context, req llm.Request) (ll } } +type checkoutReadonlySmokeAdapter struct { + mu sync.Mutex + requests []llm.Request +} + +func (a *checkoutReadonlySmokeAdapter) Name() string { return "checkout-readonly-smoke" } +func (a *checkoutReadonlySmokeAdapter) SupportsResume() bool { return false } +func (a *checkoutReadonlySmokeAdapter) SupportsCheckoutReadonly() bool { return true } +func (a *checkoutReadonlySmokeAdapter) SupportsCacheAccounting() bool { return false } +func (a *checkoutReadonlySmokeAdapter) SupportsCostReporting() bool { return false } +func (a *checkoutReadonlySmokeAdapter) Quota(context.Context) (llm.Quota, bool, error) { + return llm.Quota{}, false, nil +} +func (a *checkoutReadonlySmokeAdapter) Resume(context.Context, string, llm.Request) (llm.Stream, error) { + return nil, errors.New("resume unsupported") +} +func (a *checkoutReadonlySmokeAdapter) Start(_ context.Context, req llm.Request) (llm.Stream, error) { + a.mu.Lock() + a.requests = append(a.requests, req) + a.mu.Unlock() + access := req.CheckoutReadonly + if access == nil { + return nil, errors.New("missing checkout-readonly request") + } + mainBytes, err := os.ReadFile(filepath.Join(access.RootDir, "main.go")) // #nosec G304 -- test adapter reads only caller-provided test workbench roots. + if err != nil { + return nil, err + } + _, deniedReadErr := os.ReadFile(filepath.Join(access.RootDir, "other.go")) // #nosec G304 -- test adapter probes only caller-provided test workbench roots. + trackedWriteErr := os.WriteFile(filepath.Join(access.RootDir, "main.go"), []byte("mutated"), 0o600) // #nosec G304,G306 -- test adapter intentionally probes read-only workbench writes. + untrackedWriteErr := os.WriteFile(filepath.Join(access.RootDir, "untracked.txt"), []byte("mutated"), 0o600) // #nosec G304,G306 -- test adapter intentionally probes read-only workbench writes. + scratchPath := filepath.Join(access.ScratchDir, "smoke-output.txt") + scratchWriteErr := os.WriteFile(scratchPath, []byte("scratch-ok"), 0o600) // #nosec G306 -- test adapter writes only to caller-owned scratch. + output := fmt.Sprintf(`{"read_ok":%t,"main_contains_changed":%t,"denied_out_of_scope":%t,"tracked_write_denied":%t,"untracked_write_denied":%t,"scratch_write_ok":%t,"max_tool_output_bytes":%d}`, + true, + strings.Contains(string(mainBytes), "var changed = true"), + deniedReadErr != nil, + trackedWriteErr != nil, + untrackedWriteErr != nil, + scratchWriteErr == nil, + access.MaxToolOutputBytes, + ) + return staticStream{sessionID: "checkout-smoke-session", output: output}, nil +} +func (a *checkoutReadonlySmokeAdapter) Requests() []llm.Request { + a.mu.Lock() + defer a.mu.Unlock() + return append([]llm.Request(nil), a.requests...) +} + func (a *reviewerIsolationAdapter) Requests() []llm.Request { a.mu.Lock() defer a.mu.Unlock() @@ -4767,7 +5024,10 @@ func newWorkbenchGitFixtureForRef(t *testing.T, ref gitprovider.PRRef) workbench if err := os.WriteFile(filepath.Join(repoDir, "main.go"), []byte("package main\n\nvar changed = false\n"), 0o600); err != nil { t.Fatalf("write main.go: %v", err) } - gitCommandMustSucceed(t, repoDir, "add", "main.go") + if err := os.WriteFile(filepath.Join(repoDir, "other.go"), []byte("package main\n\nvar helper = true\n"), 0o600); err != nil { + t.Fatalf("write other.go: %v", err) + } + gitCommandMustSucceed(t, repoDir, "add", "main.go", "other.go") gitCommandMustSucceed(t, repoDir, "commit", "-m", "base") baseSHA := strings.TrimSpace(gitCommandOutput(t, repoDir, "rev-parse", "HEAD")) gitCommandMustSucceed(t, repoDir, "checkout", "-b", "feature")