From f6b5c0ffa237da5f895ce4b35cf6f741bf633764 Mon Sep 17 00:00:00 2001 From: Aaron Wong <6979793+zzwong@users.noreply.github.com> Date: Mon, 3 Aug 2026 23:49:01 -0400 Subject: [PATCH 01/20] fix: restore bounded Pi RPC reviewer workspaces --- README.md | 9 +- cmd/cr/main.go | 2 + cmd/cr/main_test.go | 35 ++ docs/checkout-native-review-contract.md | 13 + .../cmd/pireviewtoolcmd/pireviewtoolcmd.go | 30 + internal/llm/adapter.go | 1 + internal/llmadapters/pi_rpc.go | 375 +++++++++++- .../llmadapters/pi_rpc_extension_unix_test.go | 68 +++ internal/llmadapters/pi_rpc_test.go | 374 +++++++++++- internal/pipeline/pipeline_test.go | 13 +- internal/pireviewtool/path_other.go | 9 + internal/pireviewtool/path_unix.go | 18 + internal/pireviewtool/path_windows.go | 20 + internal/pireviewtool/tool.go | 561 ++++++++++++++++++ internal/pireviewtool/tool_test.go | 383 ++++++++++++ internal/workbench/workbench.go | 1 + internal/workbench/workbench_test.go | 38 ++ 17 files changed, 1910 insertions(+), 40 deletions(-) create mode 100644 internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go create mode 100644 internal/llmadapters/pi_rpc_extension_unix_test.go create mode 100644 internal/pireviewtool/path_other.go create mode 100644 internal/pireviewtool/path_unix.go create mode 100644 internal/pireviewtool/path_windows.go create mode 100644 internal/pireviewtool/tool.go create mode 100644 internal/pireviewtool/tool_test.go diff --git a/README.md b/README.md index bd90a0c2..45e4c4fb 100644 --- a/README.md +++ b/README.md @@ -117,8 +117,13 @@ cr init --non-interactive \ Setup with Pi's local RPC runtime. Install Pi's coding agent and make sure the `pi` binary is available on `PATH` before running `cr review`. New installs should use the current npm package (`@earendil-works/pi-coding-agent`); existing -installs from the previous npm scope can also work if their `pi` binary supports -the required `--mode rpc` and `--system-prompt` flags. +installs from the previous npm scope can also work if their `pi --help` reports +the reviewer controls CR requires: RPC/system-prompt mode; `--no-builtin-tools` +with an exact `--tools` allowlist; explicit `--extension` loading while +`--no-extensions` disables discovery; and `--no-context-files`, `--no-approve`, +`--no-skills`, `--no-prompt-templates`, `--no-themes`, and `--no-session`. +CR preflights these capabilities before starting a Pi reviewer and returns an +incompatible-runtime error when any control is unavailable. ```bash cr init --non-interactive \ diff --git a/cmd/cr/main.go b/cmd/cr/main.go index 307d3cf7..5ab2cb4e 100644 --- a/cmd/cr/main.go +++ b/cmd/cr/main.go @@ -19,6 +19,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/cmd/exitcode" "github.com/open-cli-collective/codereview-cli/internal/cmd/initcmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/mecmd" + "github.com/open-cli-collective/codereview-cli/internal/cmd/pireviewtoolcmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/respondcmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/reviewcmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/root" @@ -54,6 +55,7 @@ func buildRootCommand(stdin io.Reader, stdout, stderr io.Writer) (*cobra.Command credentialcmd.Register, initcmd.Register, mecmd.Register, + pireviewtoolcmd.Register, agentscmd.Register, reviewcmd.Register, respondcmd.Register, diff --git a/cmd/cr/main_test.go b/cmd/cr/main_test.go index a179c188..b53ed129 100644 --- a/cmd/cr/main_test.go +++ b/cmd/cr/main_test.go @@ -66,6 +66,41 @@ func TestRun(t *testing.T) { } } +func TestRunPiReviewerToolHiddenCommand(t *testing.T) { + tempDir := t.TempDir() + repoDir := filepath.Join(tempDir, "repo") + if err := os.MkdirAll(repoDir, 0o700); err != nil { + t.Fatalf("MkdirAll(repo): %v", err) + } + if err := os.WriteFile(filepath.Join(repoDir, "main.go"), []byte("package main\n"), 0o600); err != nil { + t.Fatalf("WriteFile(main.go): %v", err) + } + diffPath := filepath.Join(tempDir, "diff.patch") + if err := os.WriteFile(diffPath, []byte("fixed diff\n"), 0o600); err != nil { + t.Fatalf("WriteFile(diff): %v", err) + } + configPath := filepath.Join(tempDir, "config.json") + configBytes, err := json.Marshal(map[string]any{ + "repo_dir": repoDir, "diff_path": diffPath, "max_output_bytes": 2048, "timeout_ms": 1000, + }) + if err != nil { + t.Fatalf("Marshal(config): %v", err) + } + if err := os.WriteFile(configPath, configBytes, 0o600); err != nil { + t.Fatalf("WriteFile(config): %v", err) + } + var stdout, stderr bytes.Buffer + code := run([]string{"__pi-review-tool", "--config", configPath}, strings.NewReader(`{"tool":"cr_read","path":"main.go"}`), &stdout, &stderr) + if code != 0 || stdout.String() != "package main\n" || stderr.Len() != 0 { + t.Fatalf("run helper = %d, stdout %q, stderr %q", code, stdout.String(), stderr.String()) + } + stdout.Reset() + stderr.Reset() + if code := run([]string{"--help"}, strings.NewReader(""), &stdout, &stderr); code != 0 || strings.Contains(stdout.String(), "__pi-review-tool") { + t.Fatalf("root help code = %d, stdout %q, hidden helper must stay hidden", code, stdout.String()) + } +} + func TestRunConfigShowJSON(t *testing.T) { statedirtest.Hermetic(t) path, err := config.Path() diff --git a/docs/checkout-native-review-contract.md b/docs/checkout-native-review-contract.md index 746a4908..5c8e0bb5 100644 --- a/docs/checkout-native-review-contract.md +++ b/docs/checkout-native-review-contract.md @@ -292,6 +292,19 @@ trusted review workbench rather than an OS-enforced write boundary. Codex CLI reviewers run with `workspace-write` and the reviewer checkout as their working directory. +Pi RPC reviewers use `permission_bounded` mode. They run from the disposable +reviewer checkout with Pi's built-in tools disabled and one invocation-owned +extension that exposes only `cr_read`, `cr_search`, `cr_list`, and `cr_diff`. +Those tools delegate to CR's bounded read-only helper: repository paths reject +absolute paths, traversal, links/reparse points, and filesystem-boundary +crossings, while `cr_diff` reads the run's precomputed pinned diff artifact +instead of invoking Git or honoring repository/user Git configuration. +Read/diff responses expose bounded byte ranges with deterministic continuation +offsets, and list/search omit VCS metadata such as `.git`. Per-tool output, +tool duration, and aggregate reviewer RPC/stderr logs are bounded without +limiting protocol parsing. Non-reviewer Pi tasks retain their tool-free scratch +working directory. + Unsupported adapters must fail clearly. They must not silently fall back to stuffed diffs or full file bodies. diff --git a/internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go b/internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go new file mode 100644 index 00000000..b0023bf9 --- /dev/null +++ b/internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go @@ -0,0 +1,30 @@ +// Package pireviewtoolcmd wires the hidden Pi reviewer tool subprocess. +package pireviewtoolcmd + +import ( + "errors" + + "github.com/spf13/cobra" + + "github.com/open-cli-collective/codereview-cli/internal/cmd/root" + "github.com/open-cli-collective/codereview-cli/internal/pireviewtool" +) + +// Register adds the internal helper command used only by the generated Pi +// reviewer extension. +func Register(rootCmd *cobra.Command, opts *root.Options) { + var configPath string + cmd := &cobra.Command{ + Use: "__pi-review-tool", + Hidden: true, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + if code := pireviewtool.Run(cmd.Context(), []string{"--config", configPath}, opts.Stdin, opts.Stdout, opts.Stderr); code != 0 { + return errors.New("pi reviewer tool failed") + } + return nil + }, + } + cmd.Flags().StringVar(&configPath, "config", "", "Internal reviewer tool configuration") + rootCmd.AddCommand(cmd) +} diff --git a/internal/llm/adapter.go b/internal/llm/adapter.go index c12c357c..fdad7add 100644 --- a/internal/llm/adapter.go +++ b/internal/llm/adapter.go @@ -59,6 +59,7 @@ type ReviewerWorkspaceCapable interface { type ReviewerWorkspaceRequest struct { RepoDir string ScratchDir string + DiffPath string Env []string AllowedFiles []string MaxToolOutputBytes int diff --git a/internal/llmadapters/pi_rpc.go b/internal/llmadapters/pi_rpc.go index b319938f..b3811883 100644 --- a/internal/llmadapters/pi_rpc.go +++ b/internal/llmadapters/pi_rpc.go @@ -9,17 +9,29 @@ import ( "io" "os" "os/exec" + "path/filepath" + "strconv" "strings" + "sync" "time" "github.com/open-cli-collective/codereview-cli/internal/llm" ) const ( - piRPCPromptID = "prompt-1" - piRPCSystemPrompt = "You are a strict JSON API for code review structured output. Return exactly one JSON object that matches the requested schema. Do not include markdown fences, prose, explanations, or leading/trailing text. The first byte of your final answer must be { and the last byte must be }." + piRPCPromptID = "prompt-1" + piRPCSystemPrompt = "You are a strict JSON API for code review structured output. Return exactly one JSON object that matches the requested schema. Do not include markdown fences, prose, explanations, or leading/trailing text. The first byte of your final answer must be { and the last byte must be }." + piRPCReviewerSystemPrompt = piRPCSystemPrompt + " Inspect the disposable repository only through the CR-owned cr_read, cr_search, cr_list, and cr_diff tools. These tools are read-only; do not request shell, write, edit, or any other tool." + piRPCReviewerToolTimeout = 15 * time.Second + piRPCReviewerToolNames = "cr_read,cr_search,cr_list,cr_diff" + piRPCPreflightTimeout = 5 * time.Second + piRPCPreflightOutputBytes = 64 * 1024 ) +// ErrPiRPCIncompatible reports that the installed Pi runtime cannot enforce +// the bounded reviewer tool contract. +var ErrPiRPCIncompatible = errors.New("llm pi rpc: incompatible Pi runtime") + // PiRPCOptions configures the Pi RPC subprocess adapter. type PiRPCOptions struct { Command string @@ -38,9 +50,12 @@ type PiRPCAdapter struct { timeout time.Duration scratchDirFactory ScratchDirFactory fastModeModels []string + preflightOnce sync.Once + preflightErr error } var _ llm.Adapter = (*PiRPCAdapter)(nil) +var _ llm.ReviewerWorkspaceCapable = (*PiRPCAdapter)(nil) // NewPiRPCAdapter returns a Pi RPC subprocess adapter. func NewPiRPCAdapter(opts PiRPCOptions) *PiRPCAdapter { @@ -71,6 +86,11 @@ func NewPiRPCAdapter(opts PiRPCOptions) *PiRPCAdapter { // Name returns the adapter name. func (a *PiRPCAdapter) Name() string { return "pi_rpc" } +// ReviewerWorkspaceMode reports Pi's CR-owned, read-only inspection boundary. +func (a *PiRPCAdapter) ReviewerWorkspaceMode() ReviewerWorkspaceMode { + return ReviewerWorkspacePermissionBounded +} + // SupportsResume reports whether Pi RPC session resume is implemented. func (a *PiRPCAdapter) SupportsResume() bool { return false } @@ -95,34 +115,40 @@ func (a *PiRPCAdapter) Start(ctx context.Context, req Request) (Stream, error) { if err := validateFastMode(a.Name(), a.fastModeModels, req); err != nil { return nil, err } - scratch, cleanup, err := a.scratchDirFactory() + if req.ReviewerWorkspace != nil { + a.preflightOnce.Do(func() { a.preflightErr = a.preflightReviewerRuntime(ctx) }) + if a.preflightErr != nil { + return nil, a.preflightErr + } + } + scratch, cleanup, workDir, extensionPath, err := a.prepareInvocation(req) if err != nil { return nil, err } if cleanup == nil { cleanup = func() error { return nil } } - scratch, err = validateScratchDir(scratch) - if err != nil { - _ = cleanup() - return nil, err - } - args, err := a.buildArgs(req, scratch) + args, err := a.buildArgs(req, extensionPath) if err != nil { _ = cleanup() return nil, err } - if err := a.validateArgs(args); err != nil { + if err := a.validateArgs(args, req, extensionPath); err != nil { _ = cleanup() return nil, err } execArgs := append(append([]string(nil), a.commandArgsPrefix...), args...) - var env []string - if len(a.env) > 0 { - env = append(os.Environ(), a.env...) + env := append(os.Environ(), a.env...) + if req.ReviewerWorkspace != nil { + env = append(env, req.ReviewerWorkspace.Env...) + env, err = reviewerInvocationEnv(env, scratch) + if err != nil { + _ = cleanup() + return nil, err + } } - process, err := launchProcess(ctx, a.command, execArgs, scratch, env, a.timeout, req.LogPath, cleanup, true) + process, err := launchProcess(ctx, a.command, execArgs, workDir, env, a.timeout, req.LogPath, cleanup, true) if err != nil { return nil, err } @@ -132,24 +158,169 @@ func (a *PiRPCAdapter) Start(ctx context.Context, req Request) (Stream, error) { } stream := &piRPCStream{ - baseStream: llm.NewProcessStream(process, cleanup), - stdin: process.Stdin(), + baseStream: llm.NewProcessStream(process, cleanup), + stdin: process.Stdin(), + allowReviewerTools: req.ReviewerWorkspace != nil, + logBytesLeft: -1, + } + if req.ReviewerWorkspace != nil { + stream.logBytesLeft = req.ReviewerWorkspace.MaxToolOutputBytes } go stream.run(process.Context(), process.Command(), process.Stdout(), process.Stderr()) return stream, nil } -func (a *PiRPCAdapter) buildArgs(req Request, _ string) ([]string, error) { +func (a *PiRPCAdapter) preflightReviewerRuntime(parent context.Context) error { + ctx, cancel := context.WithTimeout(parent, piRPCPreflightTimeout) + defer cancel() + preflightDir, err := os.MkdirTemp("", "codereview-pi-preflight-*") + if err != nil { + return fmt.Errorf("%w: create empty preflight directory: %w", ErrPiRPCIncompatible, err) + } + defer func() { _ = os.RemoveAll(preflightDir) }() + args := append(append([]string(nil), a.commandArgsPrefix...), + "--no-tools", + "--no-extensions", + "--no-skills", + "--no-prompt-templates", + "--no-themes", + "--no-context-files", + "--no-approve", + "--no-session", + "--help", + ) + cmd := exec.CommandContext(ctx, a.command, args...) // #nosec G204 -- adapter command and fixed help argument come from trusted runtime configuration. + cmd.Dir = preflightDir + cmd.Env = append(os.Environ(), a.env...) + capture := &boundedPiRPCPreflightCapture{remaining: piRPCPreflightOutputBytes} + cmd.Stdout = capture + cmd.Stderr = capture + if err := cmd.Run(); err != nil { + if ctx.Err() != nil { + return fmt.Errorf("%w: help preflight timed out: %w", ErrPiRPCIncompatible, ctx.Err()) + } + return fmt.Errorf("%w: help preflight failed: %w", ErrPiRPCIncompatible, err) + } + help := capture.String() + required := []string{ + "--mode", "rpc", "--system-prompt", "--no-builtin-tools", "--tools", + "--extension", "--no-extensions", "--no-skills", "--no-prompt-templates", + "--no-themes", "--no-session", "--no-context-files", "--no-approve", + "explicit -e paths still work", + } + for _, capability := range required { + if !strings.Contains(help, capability) { + return fmt.Errorf("%w: required capability %q is missing", ErrPiRPCIncompatible, capability) + } + } + return nil +} + +type boundedPiRPCPreflightCapture struct { + mu sync.Mutex + remaining int + data strings.Builder +} + +func (w *boundedPiRPCPreflightCapture) Write(p []byte) (int, error) { + w.mu.Lock() + defer w.mu.Unlock() + writeBytes := len(p) + if writeBytes > w.remaining { + writeBytes = w.remaining + } + if writeBytes > 0 { + _, _ = w.data.Write(p[:writeBytes]) + w.remaining -= writeBytes + } + return len(p), nil +} + +func (w *boundedPiRPCPreflightCapture) String() string { + w.mu.Lock() + defer w.mu.Unlock() + return w.data.String() +} + +func (a *PiRPCAdapter) prepareInvocation(req Request) (scratch string, cleanup func() error, workDir, extensionPath string, err error) { + if req.ReviewerWorkspace == nil { + scratch, cleanup, err = a.scratchDirFactory() + if err != nil { + return "", nil, "", "", err + } + scratch, err = validateScratchDir(scratch) + if err != nil { + _ = cleanup() + return "", nil, "", "", err + } + return scratch, cleanup, scratch, "", nil + } + workspace := req.ReviewerWorkspace + for label, dir := range map[string]string{"repo": workspace.RepoDir, "scratch": workspace.ScratchDir} { + if strings.TrimSpace(dir) == "" || !filepath.IsAbs(dir) { + return "", nil, "", "", fmt.Errorf("%w: reviewer %s dir must be absolute", ErrUnsafeSubprocessConfig, label) + } + info, statErr := os.Lstat(filepath.Clean(dir)) + if statErr != nil || info.Mode()&os.ModeSymlink != 0 || !info.IsDir() { + return "", nil, "", "", fmt.Errorf("%w: reviewer %s dir is not a real directory", ErrUnsafeSubprocessConfig, label) + } + } + if strings.TrimSpace(workspace.DiffPath) == "" || !filepath.IsAbs(workspace.DiffPath) || workspace.MaxToolOutputBytes <= 0 { + return "", nil, "", "", fmt.Errorf("%w: reviewer fixed diff and positive output limit are required", ErrUnsafeSubprocessConfig) + } + scratch, err = os.MkdirTemp(workspace.ScratchDir, "pi-rpc-") + if err != nil { + return "", nil, "", "", fmt.Errorf("llm pi rpc: create reviewer invocation scratch: %w", err) + } + cleanup = func() error { return os.RemoveAll(scratch) } + configPath := filepath.Join(scratch, "review-tools.json") + config := map[string]any{ + "repo_dir": workspace.RepoDir, + "diff_path": workspace.DiffPath, + "allowed_files": append([]string(nil), workspace.AllowedFiles...), + "max_output_bytes": workspace.MaxToolOutputBytes, + "timeout_ms": piRPCReviewerToolTimeout.Milliseconds(), + } + data, marshalErr := json.Marshal(config) + if marshalErr != nil { + _ = cleanup() + return "", nil, "", "", marshalErr + } + if writeErr := os.WriteFile(configPath, append(data, '\n'), 0o600); writeErr != nil { + _ = cleanup() + return "", nil, "", "", fmt.Errorf("llm pi rpc: write reviewer tool config: %w", writeErr) + } + executable, executableErr := os.Executable() + if executableErr != nil { + _ = cleanup() + return "", nil, "", "", fmt.Errorf("llm pi rpc: locate CR executable: %w", executableErr) + } + extensionPath = filepath.Join(scratch, "cr-review-tools.mjs") + extension := piRPCReviewerExtension(executable, configPath, workspace.RepoDir, workspace.MaxToolOutputBytes, piRPCReviewerToolTimeout) + if writeErr := os.WriteFile(extensionPath, []byte(extension), 0o600); writeErr != nil { + _ = cleanup() + return "", nil, "", "", fmt.Errorf("llm pi rpc: write reviewer extension: %w", writeErr) + } + return scratch, cleanup, workspace.RepoDir, extensionPath, nil +} + +func (a *PiRPCAdapter) buildArgs(req Request, extensionPath string) ([]string, error) { + systemPrompt := piRPCSystemPrompt args := []string{ "--mode", "rpc", - "--system-prompt", piRPCSystemPrompt, - "--no-tools", + "--system-prompt", systemPrompt, "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-session", } + if req.ReviewerWorkspace == nil { + args = append(args, "--no-tools") + } else { + args[3] = piRPCReviewerSystemPrompt + args = append(args, "--no-builtin-tools", "--no-context-files", "--no-approve", "--tools", piRPCReviewerToolNames, "--extension", extensionPath) + } if req.Model != "" { args = append(args, "--model", req.Model) } @@ -159,38 +330,67 @@ func (a *PiRPCAdapter) buildArgs(req Request, _ string) ([]string, error) { return args, nil } -func (a *PiRPCAdapter) validateArgs(args []string) error { - if err := validateAllowedFlags("pi_rpc", args, map[string]bool{ +func (a *PiRPCAdapter) validateArgs(args []string, req Request, extensionPath string) error { + allowedFlags := map[string]bool{ "--mode": true, "--system-prompt": true, "--no-tools": false, + "--no-builtin-tools": false, + "--no-context-files": false, + "--no-approve": false, + "--tools": true, "--no-extensions": false, + "--extension": true, "--no-skills": false, "--no-prompt-templates": false, "--no-themes": false, "--no-session": false, "--model": true, "--thinking": true, - }); err != nil { + } + if err := validateAllowedFlags("pi_rpc", args, allowedFlags); err != nil { return err } + for flag := range allowedFlags { + if countPiRPCFlag(args, flag) > 1 { + return fmt.Errorf("%w: duplicate %s", ErrUnsafeSubprocessConfig, flag) + } + } if flagValue(args, "--mode") != "rpc" { return fmt.Errorf("%w: pi_rpc must use rpc mode", ErrUnsafeSubprocessConfig) } - if containsFlag(args, "--tools") || containsFlag(args, "-t") { - return fmt.Errorf("%w: pi_rpc must disable tools", ErrUnsafeSubprocessConfig) - } - for _, flag := range []string{"--system-prompt", "--no-tools", "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-session"} { + for _, flag := range []string{"--system-prompt", "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-session"} { if !containsFlag(args, flag) { return fmt.Errorf("%w: missing %s", ErrUnsafeSubprocessConfig, flag) } } - if containsFlag(args, "--system-prompt") && flagValue(args, "--system-prompt") != piRPCSystemPrompt { + wantSystemPrompt := piRPCSystemPrompt + if req.ReviewerWorkspace == nil { + if !containsFlag(args, "--no-tools") || containsFlag(args, "--no-builtin-tools") || containsFlag(args, "--tools") || containsFlag(args, "-t") || containsFlag(args, "--extension") { + return fmt.Errorf("%w: non-reviewer pi_rpc must disable all tools", ErrUnsafeSubprocessConfig) + } + } else { + wantSystemPrompt = piRPCReviewerSystemPrompt + if containsFlag(args, "--no-tools") || !containsFlag(args, "--no-builtin-tools") || !containsFlag(args, "--no-context-files") || !containsFlag(args, "--no-approve") || flagValue(args, "--tools") != piRPCReviewerToolNames || flagValue(args, "--extension") != extensionPath { + return fmt.Errorf("%w: reviewer pi_rpc must load only the CR-owned extension", ErrUnsafeSubprocessConfig) + } + } + if containsFlag(args, "--system-prompt") && flagValue(args, "--system-prompt") != wantSystemPrompt { return fmt.Errorf("%w: pi_rpc system prompt mismatch", ErrUnsafeSubprocessConfig) } return nil } +func countPiRPCFlag(args []string, flag string) int { + count := 0 + for _, arg := range args { + if arg == flag || strings.HasPrefix(arg, flag+"=") { + count++ + } + } + return count +} + func writePiRPCPrompt(stdin io.Writer, prompt string) error { command := map[string]string{ "id": piRPCPromptID, @@ -210,7 +410,11 @@ func writePiRPCPrompt(stdin io.Writer, prompt string) error { type piRPCStream struct { baseStream - stdin io.Closer + stdin io.Closer + allowReviewerTools bool + logLimitMu sync.Mutex + logBytesLeft int + logCapped bool } func (s *piRPCStream) run(ctx context.Context, cmd *exec.Cmd, stdout io.Reader, stderr io.Reader) { @@ -218,7 +422,7 @@ func (s *piRPCStream) run(ctx context.Context, cmd *exec.Cmd, stdout io.Reader, go func() { defer close(stderrDone) if s.HasLog() { - _, _ = io.Copy(&s.baseStream, stderr) + _, _ = io.Copy(piRPCLogWriter{stream: s}, stderr) return } _, _ = io.Copy(io.Discard, stderr) @@ -269,7 +473,7 @@ func (s *piRPCStream) scanStdout(stdout io.Reader) piRPCScanResult { var result piRPCScanResult for scanner.Scan() { line := append([]byte(nil), scanner.Bytes()...) - s.WriteLog(normalizePiRPCLogLine(line)) + s.writeLog(normalizePiRPCLogLine(line)) event, err := parsePiRPCEvent(line) if err != nil { s.Cancel() @@ -279,7 +483,7 @@ func (s *piRPCStream) scanStdout(stdout io.Reader) piRPCScanResult { if event.sessionID != "" { s.SetSessionID(event.sessionID) } - if event.toolUse { + if event.toolUse && (!s.allowReviewerTools || !isAllowedPiRPCReviewerTool(event.toolName)) { s.Cancel() result.err = ErrToolUse return result @@ -304,6 +508,47 @@ func (s *piRPCStream) scanStdout(stdout io.Reader) piRPCScanResult { return result } +type piRPCLogWriter struct{ stream *piRPCStream } + +func (w piRPCLogWriter) Write(p []byte) (int, error) { + w.stream.writeLog(p) + return len(p), nil +} + +func (s *piRPCStream) writeLog(p []byte) { + s.logLimitMu.Lock() + defer s.logLimitMu.Unlock() + if s.logBytesLeft < 0 { + s.WriteLog(p) + return + } + if s.logCapped || s.logBytesLeft == 0 { + return + } + const marker = "warning: reviewer RPC/stderr log cap reached; further logs truncated\n" + if len(p) < s.logBytesLeft { + s.WriteLog(p) + s.logBytesLeft -= len(p) + return + } + bodyBytes := s.logBytesLeft - len(marker) + if bodyBytes > len(p) { + bodyBytes = len(p) + } + if bodyBytes > 0 { + s.WriteLog(p[:bodyBytes]) + } + markerBytes := s.logBytesLeft - max(bodyBytes, 0) + if markerBytes > len(marker) { + markerBytes = len(marker) + } + if markerBytes > 0 { + s.WriteLog([]byte(marker[:markerBytes])) + } + s.logBytesLeft = 0 + s.logCapped = true +} + func normalizePiRPCLogLine(line []byte) []byte { logLine := append([]byte(nil), line...) if len(logLine) == 0 { @@ -371,6 +616,7 @@ type piRPCEvent struct { structuredOutput []byte usage Usage toolUse bool + toolName string responseFailure string agentEnd bool } @@ -389,6 +635,9 @@ func parsePiRPCEvent(line []byte) (piRPCEvent, error) { toolUse: piRPCEventIndicatesToolUse(eventType) || valueIndicatesToolUse(decoded), usage: parsePiRPCUsage(raw), } + if event.toolUse { + event.toolName = firstRawString(raw, "toolName", "tool_name", "name") + } if id := firstRawString(raw, "sessionId", "session_id"); id != "" { event.sessionID = id } @@ -416,6 +665,70 @@ func parsePiRPCEvent(line []byte) (piRPCEvent, error) { return event, nil } +func isAllowedPiRPCReviewerTool(name string) bool { + switch name { + case "cr_read", "cr_search", "cr_list", "cr_diff": + return true + default: + return false + } +} + +func piRPCReviewerExtension(executable, configPath, repoDir string, maxOutputBytes int, timeout time.Duration) string { + quoted := func(value string) string { + data, _ := json.Marshal(value) + return string(data) + } + return `import { spawn } from "node:child_process"; + +const executable = ` + quoted(executable) + `; +const configPath = ` + quoted(configPath) + `; +const repoDir = ` + quoted(repoDir) + `; +const maxOutputBytes = ` + strconv.Itoa(maxOutputBytes) + `; +const timeoutMs = ` + strconv.FormatInt(timeout.Milliseconds(), 10) + `; + +function runTool(tool, params, signal) { + return new Promise((resolve) => { + const child = spawn(executable, ["__pi-review-tool", "--config", configPath], { + cwd: repoDir, + env: process.env, + stdio: ["pipe", "pipe", "pipe"], + }); + let stdout = Buffer.alloc(0); + let stderr = Buffer.alloc(0); + const append = (current, chunk) => Buffer.concat([current, chunk]).subarray(0, maxOutputBytes); + child.stdout.on("data", (chunk) => { stdout = append(stdout, chunk); }); + child.stderr.on("data", (chunk) => { stderr = append(stderr, chunk); }); + const kill = () => { + try { + child.kill("SIGKILL"); + } catch {} + }; + const timer = setTimeout(kill, timeoutMs); + signal?.addEventListener("abort", kill, { once: true }); + child.on("error", (error) => { + clearTimeout(timer); + resolve({ content: [{ type: "text", text: String(error) }], details: {}, isError: true }); + }); + child.on("close", (code) => { + clearTimeout(timer); + signal?.removeEventListener("abort", kill); + const text = code === 0 ? stdout.toString("utf8") : (stderr.toString("utf8") || ("tool exited " + code)); + resolve({ content: [{ type: "text", text }], details: {}, isError: code !== 0 }); + }); + child.stdin.end(JSON.stringify({ ...params, tool })); + }); +} + +export default function (pi) { + pi.registerTool({ name: "cr_read", label: "CR Read", description: "Read one repository file. Use offset and limit with next_offset from ranged responses to continue.", parameters: { type: "object", properties: { path: { type: "string" }, offset: { type: "integer", minimum: 0 }, limit: { type: "integer", minimum: 0 } }, required: ["path"], additionalProperties: false }, execute: (_id, params, signal) => runTool("cr_read", params, signal) }); + pi.registerTool({ name: "cr_search", label: "CR Search", description: "Search repository text literally.", parameters: { type: "object", properties: { query: { type: "string" }, path: { type: "string" } }, required: ["query"], additionalProperties: false }, execute: (_id, params, signal) => runTool("cr_search", params, signal) }); + pi.registerTool({ name: "cr_list", label: "CR List", description: "List repository files.", parameters: { type: "object", properties: { path: { type: "string" } }, additionalProperties: false }, execute: (_id, params, signal) => runTool("cr_list", params, signal) }); + pi.registerTool({ name: "cr_diff", label: "CR Diff", description: "Read the fixed pinned review diff. Use offset and limit with next_offset from ranged responses to continue.", parameters: { type: "object", properties: { offset: { type: "integer", minimum: 0 }, limit: { type: "integer", minimum: 0 } }, additionalProperties: false }, execute: (_id, params, signal) => runTool("cr_diff", params, signal) }); +} +` +} + func piRPCEventIndicatesToolUse(value string) bool { normalized := strings.ToLower(strings.TrimSpace(value)) return eventIndicatesToolUse(value) || diff --git a/internal/llmadapters/pi_rpc_extension_unix_test.go b/internal/llmadapters/pi_rpc_extension_unix_test.go new file mode 100644 index 00000000..427829a0 --- /dev/null +++ b/internal/llmadapters/pi_rpc_extension_unix_test.go @@ -0,0 +1,68 @@ +//go:build unix + +package llmadapters + +import ( + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +func TestPiRPCReviewerHelperStaysInParentProcessGroup(t *testing.T) { + nodePath, err := exec.LookPath("node") + if err != nil { + t.Skip("Node is not installed") + } + tempDir := t.TempDir() + pidPath := filepath.Join(tempDir, "helper.pid") + helperPath := filepath.Join(tempDir, "helper.mjs") + helper := "#!/usr/bin/env node\nimport fs from 'node:fs';\nfs.writeFileSync(process.env.PI_RPC_TEST_PID, String(process.pid));\nsetInterval(() => {}, 1000);\n" + if err := os.WriteFile(helperPath, []byte(helper), 0o700); err != nil { // #nosec G306,G703 -- executable test helper is rooted in t.TempDir. + t.Fatalf("WriteFile(helper): %v", err) + } + extensionPath := filepath.Join(tempDir, "extension.mjs") + extension := piRPCReviewerExtension(helperPath, filepath.Join(tempDir, "config.json"), tempDir, 2048, 5*time.Second) + if err := os.WriteFile(extensionPath, []byte(extension), 0o600); err != nil { // #nosec G703 -- extensionPath is rooted in t.TempDir. + t.Fatalf("WriteFile(extension): %v", err) + } + runnerPath := filepath.Join(tempDir, "runner.mjs") + runner := "import extension from " + strconv.Quote(extensionPath) + ";\nconst tools = {};\nextension({ registerTool(tool) { tools[tool.name] = tool; } });\nawait tools.cr_read.execute('call-1', { path: 'main.go' }, new AbortController().signal);\n" + if err := os.WriteFile(runnerPath, []byte(runner), 0o600); err != nil { + t.Fatalf("WriteFile(runner): %v", err) + } + cmd := exec.Command(nodePath, runnerPath) // #nosec G204 -- test launches the discovered Node executable with a test-owned script. + cmd.Dir = tempDir + cmd.Env = append(os.Environ(), "PI_RPC_TEST_PID="+pidPath) + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + if err := cmd.Start(); err != nil { + t.Fatalf("Start(runner): %v", err) + } + defer func() { + _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + _ = cmd.Wait() + }() + eventually(t, 3*time.Second, func() bool { + _, err := os.Stat(pidPath) + return err == nil + }) + pidData, err := os.ReadFile(pidPath) // #nosec G304 -- pidPath is rooted in t.TempDir. + if err != nil { + t.Fatalf("ReadFile(pid): %v", err) + } + helperPID, err := strconv.Atoi(strings.TrimSpace(string(pidData))) + if err != nil { + t.Fatalf("Atoi(pid): %v", err) + } + t.Cleanup(func() { _ = syscall.Kill(helperPID, syscall.SIGKILL) }) + + if err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL); err != nil { + t.Fatalf("kill runner process group: %v", err) + } + _ = cmd.Wait() + eventually(t, time.Second, func() bool { return !processExists(helperPID) }) +} diff --git a/internal/llmadapters/pi_rpc_test.go b/internal/llmadapters/pi_rpc_test.go index 9a95c7b7..74af9349 100644 --- a/internal/llmadapters/pi_rpc_test.go +++ b/internal/llmadapters/pi_rpc_test.go @@ -95,6 +95,275 @@ func TestPiRPCLaunchSafetyAndSuccess(t *testing.T) { } } +func TestPiRPCReviewerWorkspaceModeIsPermissionBounded(t *testing.T) { + adapter := NewPiRPCAdapter(PiRPCOptions{}) + if got := AdapterReviewerWorkspaceMode(adapter); got != ReviewerWorkspacePermissionBounded { + t.Fatalf("ReviewerWorkspaceMode = %q, want %q", got, ReviewerWorkspacePermissionBounded) + } + if got := AdapterReviewerWorkspaceMode(adapter); got == ReviewerWorkspaceWrite { + t.Fatalf("ReviewerWorkspaceMode = %q, must not grant workspace_write", got) + } +} + +func TestPiRPCReviewerWorkspaceLaunchUsesOnlyCROwnedTools(t *testing.T) { + tempDir := t.TempDir() + repoDir := filepath.Join(tempDir, "repo") + scratchDir := filepath.Join(tempDir, "scratch") + for _, dir := range []string{repoDir, scratchDir} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("MkdirAll(%s): %v", dir, err) + } + } + diffPath := filepath.Join(tempDir, "diff.patch") + if err := os.WriteFile(diffPath, []byte("fixed diff\n"), 0o600); err != nil { + t.Fatalf("WriteFile(diff): %v", err) + } + recordPath := filepath.Join(tempDir, "record.json") + adapter := NewPiRPCAdapter(PiRPCOptions{ + Command: os.Args[0], + commandArgsPrefix: piRPCHelperPrefix(), + Env: piRPCHelperEnv("reviewer-tools", recordPath), + Timeout: 5 * time.Second, + }) + + stream, err := adapter.Start(context.Background(), Request{ + Prompt: "review assigned files", + ReviewerWorkspace: &ReviewerWorkspaceRequest{ + RepoDir: repoDir, + ScratchDir: scratchDir, + DiffPath: diffPath, + AllowedFiles: []string{"assigned.go"}, + MaxToolOutputBytes: 2048, + }, + }) + if err != nil { + t.Fatalf("Start: %v", err) + } + if _, err := stream.Wait(context.Background()); err != nil { + t.Fatalf("Wait: %v", err) + } + + record := readPiRPCRecord(t, recordPath) + if !samePath(t, record.Cwd, repoDir) { + t.Fatalf("cwd = %q, want reviewer repo %q", record.Cwd, repoDir) + } + if containsFlag(record.AdapterArgs, "--no-tools") { + t.Fatalf("args = %#v, reviewer extension tools must remain enabled", record.AdapterArgs) + } + assertFlagValue(t, record.AdapterArgs, "--tools", piRPCReviewerToolNames) + for _, flag := range []string{"--no-builtin-tools", "--no-context-files", "--no-approve", "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-session"} { + if !containsFlag(record.AdapterArgs, flag) { + t.Fatalf("args = %#v, want %s", record.AdapterArgs, flag) + } + } + extensionPath := flagValue(record.AdapterArgs, "--extension") + if extensionPath == "" || !pathWithin(t, scratchDir, extensionPath) { + t.Fatalf("--extension = %q, want generated extension under %q", extensionPath, scratchDir) + } + extension := []byte(record.Extension) + for _, tool := range []string{"cr_read", "cr_search", "cr_list", "cr_diff"} { + if !strings.Contains(string(extension), `name: "`+tool+`"`) { + t.Fatalf("extension does not register %s:\n%s", tool, extension) + } + } + for _, forbidden := range []string{"workspace_write", `name: "bash"`, `name: "edit"`, `name: "write"`} { + if strings.Contains(strings.ToLower(string(extension)), forbidden) { + t.Fatalf("extension contains forbidden capability %q:\n%s", forbidden, extension) + } + } + for _, key := range []string{"TMPDIR", "GOTMPDIR", "GOCACHE", "XDG_CACHE_HOME"} { + value := record.Env[key] + if value == "" || !pathWithin(t, scratchDir, value) { + t.Fatalf("%s = %q, want scratch-rooted path under %q", key, value, scratchDir) + } + } +} + +func TestPiRPCReviewerWorkspaceRejectsUnknownToolEvents(t *testing.T) { + tempDir := t.TempDir() + repoDir := filepath.Join(tempDir, "repo") + scratchDir := filepath.Join(tempDir, "scratch") + for _, dir := range []string{repoDir, scratchDir} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("MkdirAll(%s): %v", dir, err) + } + } + diffPath := filepath.Join(tempDir, "diff.patch") + if err := os.WriteFile(diffPath, []byte("fixed diff\n"), 0o600); err != nil { + t.Fatalf("WriteFile(diff): %v", err) + } + adapter := NewPiRPCAdapter(PiRPCOptions{ + Command: os.Args[0], + commandArgsPrefix: piRPCHelperPrefix(), + Env: piRPCHelperEnv("tool", filepath.Join(tempDir, "record.json")), + Timeout: 5 * time.Second, + }) + stream, err := adapter.Start(context.Background(), Request{ + Prompt: "review", + ReviewerWorkspace: &ReviewerWorkspaceRequest{ + RepoDir: repoDir, ScratchDir: scratchDir, DiffPath: diffPath, MaxToolOutputBytes: 2048, + }, + }) + if err != nil { + t.Fatalf("Start: %v", err) + } + if _, err := stream.Wait(context.Background()); !errors.Is(err, ErrToolUse) { + t.Fatalf("Wait error = %v, want ErrToolUse for native Read event", err) + } +} + +func TestPiRPCReviewerLogCapDoesNotBreakProtocolCompletion(t *testing.T) { + tempDir := t.TempDir() + repoDir := filepath.Join(tempDir, "repo") + scratchDir := filepath.Join(tempDir, "scratch") + for _, dir := range []string{repoDir, scratchDir} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("MkdirAll(%s): %v", dir, err) + } + } + diffPath := filepath.Join(tempDir, "diff.patch") + if err := os.WriteFile(diffPath, []byte("fixed diff\n"), 0o600); err != nil { + t.Fatalf("WriteFile(diff): %v", err) + } + logPath := filepath.Join(tempDir, "reviewer.jsonl") + adapter := NewPiRPCAdapter(PiRPCOptions{ + Command: os.Args[0], + commandArgsPrefix: piRPCHelperPrefix(), + Env: piRPCHelperEnv("reviewer-log-flood", filepath.Join(tempDir, "record.json")), + Timeout: 5 * time.Second, + }) + stream, err := adapter.Start(context.Background(), Request{ + Prompt: "review", + LogPath: logPath, + ReviewerWorkspace: &ReviewerWorkspaceRequest{ + RepoDir: repoDir, ScratchDir: scratchDir, DiffPath: diffPath, MaxToolOutputBytes: 2048, + }, + }) + if err != nil { + t.Fatalf("Start: %v", err) + } + response, err := stream.Wait(context.Background()) + if err != nil { + t.Fatalf("Wait: %v", err) + } + if string(response.StructuredOutput) != `{"ok":true}` { + t.Fatalf("StructuredOutput = %s, want completed final response", response.StructuredOutput) + } + logged, err := os.ReadFile(logPath) // #nosec G304 -- logPath is rooted in t.TempDir. + if err != nil { + t.Fatalf("ReadFile(log): %v", err) + } + if len(logged) > 2048 { + t.Fatalf("reviewer log = %d bytes, want aggregate cap 2048", len(logged)) + } + if !strings.Contains(string(logged), "reviewer RPC/stderr log cap reached") { + t.Fatalf("reviewer log = %q, want cap marker", logged) + } +} + +func TestPiRPCReviewerExtensionLoadsInInstalledPi(t *testing.T) { + piPath, err := exec.LookPath("pi") + if err != nil { + t.Skip("Pi is not installed") + } + if err := NewPiRPCAdapter(PiRPCOptions{Command: piPath}).preflightReviewerRuntime(context.Background()); err != nil { + t.Fatalf("installed Pi reviewer preflight: %v", err) + } + tempDir := t.TempDir() + extensionPath := filepath.Join(tempDir, "cr-review-tools.mjs") + extension := piRPCReviewerExtension(os.Args[0], filepath.Join(tempDir, "config.json"), tempDir, 2048, time.Second) + if err := os.WriteFile(extensionPath, []byte(extension), 0o600); err != nil { // #nosec G703 -- extensionPath is rooted in t.TempDir. + t.Fatalf("WriteFile(extension): %v", err) + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, piPath, + "--mode", "rpc", + "--system-prompt", piRPCReviewerSystemPrompt, + "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-session", + "--no-builtin-tools", "--no-context-files", "--no-approve", + "--tools", piRPCReviewerToolNames, + "--extension", extensionPath, + ) // #nosec G204 -- test launches the discovered Pi executable with fixed arguments. + cmd.Dir = tempDir + cmd.Stdin = strings.NewReader("{\"id\":\"state-1\",\"type\":\"get_state\"}\n") + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("Pi extension load: %v\n%s", err, output) + } + if !strings.Contains(string(output), `"id":"state-1"`) || !strings.Contains(string(output), `"success":true`) { + t.Fatalf("Pi get_state output = %s, want successful response", output) + } +} + +func TestPiRPCReviewerPreflightRejectsUnsupportedPi(t *testing.T) { + tempDir := t.TempDir() + repoDir := filepath.Join(tempDir, "repo") + scratchDir := filepath.Join(tempDir, "scratch") + for _, dir := range []string{repoDir, scratchDir} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("MkdirAll(%s): %v", dir, err) + } + } + diffPath := filepath.Join(tempDir, "diff.patch") + if err := os.WriteFile(diffPath, []byte("diff\n"), 0o600); err != nil { + t.Fatalf("WriteFile(diff): %v", err) + } + adapter := NewPiRPCAdapter(PiRPCOptions{ + Command: os.Args[0], + commandArgsPrefix: piRPCHelperPrefix(), + Env: append(piRPCHelperEnv("success", filepath.Join(tempDir, "record.json")), + "LLM_PI_RPC_HELP_UNSUPPORTED=1", + ), + Timeout: 5 * time.Second, + }) + stream, err := adapter.Start(context.Background(), Request{ + Prompt: "review", + ReviewerWorkspace: &ReviewerWorkspaceRequest{ + RepoDir: repoDir, ScratchDir: scratchDir, DiffPath: diffPath, MaxToolOutputBytes: 2048, + }, + }) + if !errors.Is(err, ErrPiRPCIncompatible) || !strings.Contains(err.Error(), "--no-builtin-tools") { + t.Fatalf("Start error = %v, want classified Pi compatibility error naming missing flag", err) + } + if stream != nil { + t.Fatalf("stream = %#v, want nil before reviewer launch", stream) + } +} + +func TestPiRPCReviewerPreflightUsesEmptyDiscoveryDisabledDirectory(t *testing.T) { + tempDir := t.TempDir() + recordPath := filepath.Join(tempDir, "preflight.json") + mutationPath := filepath.Join(tempDir, "hostile-resource-loaded") + adapter := NewPiRPCAdapter(PiRPCOptions{ + Command: os.Args[0], + commandArgsPrefix: piRPCHelperPrefix(), + Env: []string{ + "LLM_PI_RPC_HELPER=1", + "LLM_HELPER_RECORD=" + recordPath, + "LLM_PI_RPC_HOSTILE_MUTATION=" + mutationPath, + }, + }) + if err := adapter.preflightReviewerRuntime(context.Background()); err != nil { + t.Fatalf("preflightReviewerRuntime: %v", err) + } + record := readPiRPCRecord(t, recordPath) + if record.CwdEntries != 0 { + t.Fatalf("preflight cwd = %q with %d entries, want invocation-owned empty directory", record.Cwd, record.CwdEntries) + } + if samePath(t, record.Cwd, repoRootForTest(t)) { + t.Fatalf("preflight cwd = repository root %q", record.Cwd) + } + for _, flag := range []string{"--no-tools", "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-context-files", "--no-approve", "--no-session"} { + if !containsFlag(record.AdapterArgs, flag) { + t.Fatalf("preflight args = %#v, want %s", record.AdapterArgs, flag) + } + } + if _, err := os.Stat(mutationPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("hostile resource mutation stat = %v, want resource undiscovered", err) + } +} + func TestPiRPCLogStripsCumulativeStreamingPartials(t *testing.T) { recordPath := filepath.Join(t.TempDir(), "record.json") logPath := filepath.Join(t.TempDir(), "pi-rpc.jsonl") @@ -330,7 +599,8 @@ func TestPiRPCProtocolFailures(t *testing.T) { func TestPiRPCRejectsUnsafeSpecs(t *testing.T) { adapter := NewPiRPCAdapter(PiRPCOptions{}) - args, err := adapter.buildArgs(Request{Model: "opencode-go/kimi-k2.6", Prompt: "prompt"}, t.TempDir()) + req := Request{Model: "opencode-go/kimi-k2.6", Prompt: "prompt"} + args, err := adapter.buildArgs(req, "") if err != nil { t.Fatalf("buildArgs: %v", err) } @@ -346,7 +616,35 @@ func TestPiRPCRejectsUnsafeSpecs(t *testing.T) { {name: "wrong system prompt", args: replaceFlagValue(args, "--system-prompt", "be loose")}, } { t.Run(tt.name, func(t *testing.T) { - if err := adapter.validateArgs(tt.args); !errors.Is(err, ErrUnsafeSubprocessConfig) { + if err := adapter.validateArgs(tt.args, req, ""); !errors.Is(err, ErrUnsafeSubprocessConfig) { + t.Fatalf("validateArgs error = %v, want ErrUnsafeSubprocessConfig", err) + } + }) + } +} + +func TestPiRPCRejectsUnsafeReviewerSpecs(t *testing.T) { + adapter := NewPiRPCAdapter(PiRPCOptions{}) + extensionPath := filepath.Join(t.TempDir(), "extension.mjs") + req := Request{Prompt: "prompt", ReviewerWorkspace: &ReviewerWorkspaceRequest{}} + args, err := adapter.buildArgs(req, extensionPath) + if err != nil { + t.Fatalf("buildArgs: %v", err) + } + for _, tt := range []struct { + name string + args []string + }{ + {name: "missing builtin disable", args: removeFlag(args, "--no-builtin-tools")}, + {name: "missing context disable", args: removeFlag(args, "--no-context-files")}, + {name: "missing project approval disable", args: removeFlag(args, "--no-approve")}, + {name: "all tools disabled", args: append(removeFlagWithValue(args, "--tools"), "--no-tools")}, + {name: "native bash added", args: replaceFlagValue(args, "--tools", piRPCReviewerToolNames+",bash")}, + {name: "wrong extension", args: replaceFlagValue(args, "--extension", filepath.Join(t.TempDir(), "other.mjs"))}, + {name: "extra extension", args: append(args, "--extension", filepath.Join(t.TempDir(), "extra.mjs"))}, + } { + t.Run(tt.name, func(t *testing.T) { + if err := adapter.validateArgs(tt.args, req, extensionPath); !errors.Is(err, ErrUnsafeSubprocessConfig) { t.Fatalf("validateArgs error = %v, want ErrUnsafeSubprocessConfig", err) } }) @@ -364,6 +662,28 @@ func TestPiRPCHelperProcess(_ *testing.T) { if os.Getenv("LLM_PI_RPC_HELPER") != "1" { return } + if containsFlag(adapterArgsFromHelper(), "--help") { + cwd, _ := os.Getwd() + entries, _ := os.ReadDir(cwd) + record := piRPCRecord{AdapterArgs: adapterArgsFromHelper(), Cwd: cwd, CwdEntries: len(entries)} + if recordPath := os.Getenv("LLM_HELPER_RECORD"); recordPath != "" { + data, _ := json.Marshal(record) + _ = os.WriteFile(recordPath, data, 0o600) // #nosec G703 -- helper writes only to a test-owned path. + } + safe := len(entries) == 0 + for _, flag := range []string{"--no-tools", "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-context-files", "--no-approve", "--no-session"} { + safe = safe && containsFlag(record.AdapterArgs, flag) + } + if !safe && os.Getenv("LLM_PI_RPC_HOSTILE_MUTATION") != "" { + _ = os.WriteFile(os.Getenv("LLM_PI_RPC_HOSTILE_MUTATION"), []byte("loaded"), 0o600) // #nosec G703 -- test helper writes to a test-owned marker. + } + if os.Getenv("LLM_PI_RPC_HELP_UNSUPPORTED") == "1" { + fmt.Println("--mode rpc --system-prompt --no-tools") + } else { + fmt.Println("--mode rpc --system-prompt --no-tools --no-builtin-tools --tools --extension --no-extensions --no-skills --no-prompt-templates --no-themes --no-session --no-context-files --no-approve explicit -e paths still work") + } + os.Exit(0) + } recordPath := os.Getenv("LLM_HELPER_RECORD") cwd, _ := os.Getwd() entries, _ := os.ReadDir(cwd) @@ -380,6 +700,15 @@ func TestPiRPCHelperProcess(_ *testing.T) { Cwd: cwd, CwdEntries: len(entries), Commands: []map[string]string{command}, + Env: map[string]string{ + "TMPDIR": os.Getenv("TMPDIR"), + "GOTMPDIR": os.Getenv("GOTMPDIR"), + "GOCACHE": os.Getenv("GOCACHE"), + "XDG_CACHE_HOME": os.Getenv("XDG_CACHE_HOME"), + }, + } + if extensionPath := flagValue(record.AdapterArgs, "--extension"); extensionPath != "" { + record.Extension = string(mustReadHelperFile(extensionPath)) } if recordPath != "" { data, _ := json.Marshal(record) @@ -427,6 +756,22 @@ func TestPiRPCHelperProcess(_ *testing.T) { fmt.Println(`{"id":"prompt-1","type":"response","command":"prompt","success":true}`) fmt.Println(`{"type":"tool_execution_start","toolCallId":"tool-1","toolName":"Read","args":{"path":"x"}}`) time.Sleep(10 * time.Second) + case "reviewer-tools": + fmt.Println(`{"id":"prompt-1","type":"response","command":"prompt","success":true}`) + for _, tool := range []string{"cr_read", "cr_search", "cr_list", "cr_diff"} { + fmt.Printf("{\"type\":\"tool_execution_start\",\"toolCallId\":\"%s\",\"toolName\":%q,\"args\":{}}\n", tool, tool) + fmt.Printf("{\"type\":\"tool_execution_end\",\"toolCallId\":\"%s\",\"toolName\":%q,\"result\":{\"content\":[{\"type\":\"text\",\"text\":\"ok\"}]}}\n", tool, tool) + } + fmt.Println(`{"type":"message_end","message":{"role":"assistant","content":[{"type":"text","text":"{\"ok\":true}"}]}}`) + fmt.Println(`{"type":"agent_end","messages":[{"role":"assistant","content":[{"type":"text","text":"{\"ok\":true}"}]}]}`) + case "reviewer-log-flood": + fmt.Fprintln(os.Stderr, strings.Repeat("stderr flood\n", 1000)) + fmt.Println(`{"id":"prompt-1","type":"response","command":"prompt","success":true}`) + for i := 0; i < 20; i++ { + fmt.Printf("{\"type\":\"tool_execution_end\",\"toolCallId\":\"tool-%d\",\"toolName\":\"cr_read\",\"result\":{\"content\":[{\"type\":\"text\",\"text\":%q}]}}\n", i, strings.Repeat("tool output ", 500)) + } + fmt.Println(`{"type":"message_end","message":{"role":"assistant","content":[{"type":"text","text":"{\"ok\":true}"}]}}`) + fmt.Println(`{"type":"agent_end","messages":[{"role":"assistant","content":[{"type":"text","text":"{\"ok\":true}"}]}]}`) case "sleep": time.Sleep(10 * time.Second) case "malformed": @@ -464,6 +809,19 @@ type piRPCRecord struct { Cwd string `json:"cwd"` CwdEntries int `json:"cwd_entries"` Commands []map[string]string `json:"commands"` + Env map[string]string `json:"env"` + Extension string `json:"extension"` +} + +func mustReadHelperFile(path string) []byte { + data, _ := os.ReadFile(filepath.Clean(path)) // #nosec G304 -- helper reads the CR-generated extension path from its own argv. + return data +} + +func pathWithin(t *testing.T, root, candidate string) bool { + t.Helper() + rel, err := filepath.Rel(root, candidate) + return err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) && !filepath.IsAbs(rel) } func piRPCHelperPrefix() []string { @@ -503,6 +861,18 @@ func removeFlag(args []string, flag string) []string { return out } +func removeFlagWithValue(args []string, flag string) []string { + out := make([]string, 0, len(args)) + for i := 0; i < len(args); i++ { + if args[i] == flag { + i++ + continue + } + out = append(out, args[i]) + } + return out +} + func replaceFlagValue(args []string, flag string, value string) []string { out := append([]string(nil), args...) for i := 0; i+1 < len(out); i++ { diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index b18582b9..8cca61e5 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -218,7 +218,7 @@ func TestBuildPlanClassifiesActionIDFailureTerminalAcrossPaths(t *testing.T) { } } -func TestReviewPipelineAcceptanceHarnessDryRunWithFakes(t *testing.T) { +func TestReviewPipelineAcceptanceHarnessPiRPCPermissionBoundedDryRunCompletesWithFakes(t *testing.T) { ctx := context.Background() store := openPipelineStore(t) defer closeStore(t, store) @@ -254,9 +254,11 @@ func TestReviewPipelineAcceptanceHarnessDryRunWithFakes(t *testing.T) { Event: review.ReviewEventApprove, }} baseAdapter := &llm.FakeAdapter{ - NameValue: "fake-llm", - QuotaValue: llm.Quota{BlockRemainingPct: 87, WeeklyRemainingPct: 64}, - QuotaSupported: true, + NameValue: "pi_rpc", + ReviewerWorkspaceModeSet: true, + ReviewerWorkspaceModeValue: llm.ReviewerWorkspacePermissionBounded, + QuotaValue: llm.Quota{BlockRemainingPct: 87, WeeklyRemainingPct: 64}, + QuotaSupported: true, } baseAdapter.Queue(fakeLLMResult("dossier-summary-session", discussionSummaryJSON([]string{"Top-level concern", "Review body"}, []threadSummary{{path: "main.go", line: 2, status: "unresolved", summary: "Inline concern"}}), 8, 2)) baseAdapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "main.go"), 10, 2)) @@ -1113,8 +1115,9 @@ func TestDryRunWithPinnedReviewSHAsUsesCompareDiffAndPinnedFileRefs(t *testing.T workspace := requests[1].ReviewerWorkspace if !strings.Contains(workspace.RepoDir, filepath.Join("workbench", "reviewers")) || !strings.HasPrefix(workspace.ScratchDir, result.Artifacts.WorkbenchScratch+string(filepath.Separator)) || + workspace.DiffPath != result.Artifacts.DiffPatch || workspace.MaxToolOutputBytes != 32*1024 { - t.Fatalf("reviewer workspace request = %#v, want disposable repo, scratch, and default cap", workspace) + t.Fatalf("reviewer workspace request = %#v, want disposable repo, scratch, fixed diff, and default cap", workspace) } if provider.threadCalls != 0 { t.Fatalf("thread calls = %d, want no live thread reads for pinned review", provider.threadCalls) diff --git a/internal/pireviewtool/path_other.go b/internal/pireviewtool/path_other.go new file mode 100644 index 00000000..eb305bb5 --- /dev/null +++ b/internal/pireviewtool/path_other.go @@ -0,0 +1,9 @@ +//go:build !unix && !windows + +package pireviewtool + +import "os" + +func isLinkLike(info os.FileInfo) bool { return info.Mode()&os.ModeSymlink != 0 } + +func sameFileSystem(_, _ os.FileInfo) bool { return true } diff --git a/internal/pireviewtool/path_unix.go b/internal/pireviewtool/path_unix.go new file mode 100644 index 00000000..8ad15898 --- /dev/null +++ b/internal/pireviewtool/path_unix.go @@ -0,0 +1,18 @@ +//go:build unix + +package pireviewtool + +import ( + "os" + "syscall" +) + +func isLinkLike(info os.FileInfo) bool { + return info.Mode()&os.ModeSymlink != 0 +} + +func sameFileSystem(root, candidate os.FileInfo) bool { + rootStat, rootOK := root.Sys().(*syscall.Stat_t) + candidateStat, candidateOK := candidate.Sys().(*syscall.Stat_t) + return rootOK && candidateOK && rootStat.Dev == candidateStat.Dev +} diff --git a/internal/pireviewtool/path_windows.go b/internal/pireviewtool/path_windows.go new file mode 100644 index 00000000..43a8be5e --- /dev/null +++ b/internal/pireviewtool/path_windows.go @@ -0,0 +1,20 @@ +//go:build windows + +package pireviewtool + +import ( + "os" + "syscall" +) + +func isLinkLike(info os.FileInfo) bool { + if info.Mode()&os.ModeSymlink != 0 { + return true + } + data, ok := info.Sys().(*syscall.Win32FileAttributeData) + return ok && data.FileAttributes&syscall.FILE_ATTRIBUTE_REPARSE_POINT != 0 +} + +// A path below the root can change Windows volumes only through a reparse +// point, which is rejected by isLinkLike at every component. +func sameFileSystem(_, _ os.FileInfo) bool { return true } diff --git a/internal/pireviewtool/tool.go b/internal/pireviewtool/tool.go new file mode 100644 index 00000000..de4fb2ce --- /dev/null +++ b/internal/pireviewtool/tool.go @@ -0,0 +1,561 @@ +// Package pireviewtool implements the bounded inspection surface used by Pi +// RPC reviewers. It intentionally has no shell or repository mutation API. +package pireviewtool + +import ( + "bufio" + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" + "sort" + "strings" + "time" + "unicode/utf8" +) + +const ( + // ToolRead reads one confined repository file. + ToolRead = "cr_read" + // ToolSearch searches repository files for a literal string. + ToolSearch = "cr_search" + // ToolList lists regular files below a confined repository path. + ToolList = "cr_list" + // ToolDiff reads the fixed pinned diff artifact. + ToolDiff = "cr_diff" +) + +const ( + maxConfiguredOutputBytes = 1024 * 1024 + maxConfiguredTimeoutMS = 60_000 +) + +// ErrDenied marks tool requests outside the fixed read-only contract. +var ErrDenied = errors.New("pi reviewer tool request denied") + +// Config fixes the roots and output limit for one reviewer invocation. +type Config struct { + RepoDir string `json:"repo_dir"` + DiffPath string `json:"diff_path"` + AllowedFiles []string `json:"allowed_files,omitempty"` + MaxOutputBytes int `json:"max_output_bytes"` + TimeoutMS int `json:"timeout_ms"` +} + +// Run implements the strict stdin/stdout protocol used by the generated Pi +// extension. It returns a process exit code and never accepts tool arguments on +// the command line. +func Run(parent context.Context, args []string, stdin io.Reader, stdout, stderr io.Writer) int { + if len(args) != 2 || args[0] != "--config" || strings.TrimSpace(args[1]) == "" { + _, _ = fmt.Fprintln(stderr, "pi reviewer tool: expected --config ") + return 2 + } + configFile, err := os.Open(filepath.Clean(args[1])) // #nosec G304 -- path is supplied by the CR-owned generated extension. + if err != nil { + _, _ = fmt.Fprintf(stderr, "pi reviewer tool: open config: %v\n", err) + return 1 + } + defer configFile.Close() + var config Config + if err := decodeStrictJSON(io.LimitReader(configFile, 1024*1024), &config); err != nil { + _, _ = fmt.Fprintf(stderr, "pi reviewer tool: decode config: %v\n", err) + return 1 + } + if config.MaxOutputBytes <= 0 || config.MaxOutputBytes > maxConfiguredOutputBytes || config.TimeoutMS <= 0 || config.TimeoutMS > maxConfiguredTimeoutMS { + _, _ = fmt.Fprintln(stderr, "pi reviewer tool: invalid output or timeout bound") + return 1 + } + var request Request + if err := decodeStrictJSON(io.LimitReader(stdin, 1024*1024), &request); err != nil { + _, _ = fmt.Fprintf(stderr, "pi reviewer tool: decode request: %v\n", err) + return 1 + } + ctx := parent + cancel := func() {} + if config.TimeoutMS > 0 { + ctx, cancel = context.WithTimeout(parent, time.Duration(config.TimeoutMS)*time.Millisecond) + } + defer cancel() + output, err := Execute(ctx, config, request) + if err != nil { + _, _ = fmt.Fprintf(stderr, "pi reviewer tool: %v\n", err) + return 1 + } + _, _ = stdout.Write([]byte(output)) + return 0 +} + +func decodeStrictJSON(reader io.Reader, target any) error { + decoder := json.NewDecoder(reader) + decoder.DisallowUnknownFields() + if err := decoder.Decode(target); err != nil { + return err + } + var trailing any + if err := decoder.Decode(&trailing); !errors.Is(err, io.EOF) { + if err == nil { + return errors.New("multiple JSON values") + } + return err + } + return nil +} + +// Request is one CR-owned reviewer tool invocation. +type Request struct { + Tool string `json:"tool"` + Path string `json:"path,omitempty"` + Query string `json:"query,omitempty"` + Offset int64 `json:"offset,omitempty"` + Limit int `json:"limit,omitempty"` +} + +// Execute performs one bounded, read-only inspection operation. +func Execute(ctx context.Context, config Config, request Request) (string, error) { + if err := ctx.Err(); err != nil { + return "", err + } + if config.MaxOutputBytes <= 0 { + return "", fmt.Errorf("%w: invalid output limit", ErrDenied) + } + root, err := validateRepoRoot(config.RepoDir) + if err != nil { + return "", err + } + var output []byte + rootHandle, err := os.OpenRoot(root) + if err != nil { + return "", fmt.Errorf("%w: open repository root: %w", ErrDenied, err) + } + defer rootHandle.Close() + switch request.Tool { + case ToolRead: + path, err := confinedPath(root, request.Path, false) + if err != nil { + return "", err + } + rel, _ := filepath.Rel(root, path) + file, openErr := rootHandle.Open(rel) + if openErr != nil { + return "", fmt.Errorf("read %q: %w", request.Path, openErr) + } + output, err = readRange(file, request.Offset, request.Limit, config.MaxOutputBytes) + _ = file.Close() + if err != nil { + return "", fmt.Errorf("read %q: %w", request.Path, err) + } + case ToolSearch: + if request.Query == "" { + return "", fmt.Errorf("%w: search query is required", ErrDenied) + } + path, err := confinedPath(root, request.Path, true) + if err != nil { + return "", err + } + rel, _ := filepath.Rel(root, path) + output, err = search(ctx, rootHandle.FS(), rel, request.Query, config.MaxOutputBytes) + if err != nil { + return "", err + } + case ToolList: + path, err := confinedPath(root, request.Path, true) + if err != nil { + return "", err + } + rel, _ := filepath.Rel(root, path) + output, err = list(ctx, rootHandle.FS(), rel, config.MaxOutputBytes) + if err != nil { + return "", err + } + case ToolDiff: + output, err = readFixedDiff(config.DiffPath, request.Offset, request.Limit, config.MaxOutputBytes) + if err != nil { + return "", err + } + default: + return "", fmt.Errorf("%w: unknown tool %q", ErrDenied, request.Tool) + } + return boundOutput(output, config.MaxOutputBytes), nil +} + +func validateRepoRoot(root string) (string, error) { + if strings.TrimSpace(root) == "" || !filepath.IsAbs(root) { + return "", fmt.Errorf("%w: repository root must be absolute", ErrDenied) + } + root = filepath.Clean(root) + info, err := os.Lstat(root) + if err != nil { + return "", fmt.Errorf("%w: repository root: %w", ErrDenied, err) + } + if isLinkLike(info) || !info.IsDir() { + return "", fmt.Errorf("%w: repository root is not a real directory", ErrDenied) + } + return root, nil +} + +func resolveRepoPath(root, requested string) (string, error) { + requested = strings.TrimSpace(requested) + if requested == "" { + requested = "." + } + if filepath.IsAbs(requested) || filepath.VolumeName(requested) != "" { + return "", fmt.Errorf("%w: absolute or volume-qualified path", ErrDenied) + } + clean := filepath.Clean(requested) + if clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("%w: path traversal", ErrDenied) + } + for _, component := range strings.Split(clean, string(filepath.Separator)) { + if isVCSMetadataDir(component) { + return "", fmt.Errorf("%w: VCS metadata is not reviewer-visible", ErrDenied) + } + } + target := filepath.Join(root, clean) + if filepath.VolumeName(root) != filepath.VolumeName(target) { + return "", fmt.Errorf("%w: cross-volume path", ErrDenied) + } + rel, err := filepath.Rel(root, target) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { + return "", fmt.Errorf("%w: path escape", ErrDenied) + } + return target, nil +} + +func confinedPath(root, requested string, allowDir bool) (string, error) { + target, err := resolveRepoPath(root, requested) + if err != nil { + return "", err + } + rel, _ := filepath.Rel(root, target) + rootInfo, err := os.Lstat(root) + if err != nil { + return "", fmt.Errorf("%w: repository root disappeared", ErrDenied) + } + current := root + if rel != "." { + for _, component := range strings.Split(rel, string(filepath.Separator)) { + current = filepath.Join(current, component) + info, statErr := os.Lstat(current) + if statErr != nil { + return "", fmt.Errorf("inspect %q: %w", requested, statErr) + } + if isLinkLike(info) || !sameFileSystem(rootInfo, info) { + return "", fmt.Errorf("%w: linked or cross-volume path component", ErrDenied) + } + } + } + info, err := os.Lstat(target) + if err != nil { + return "", fmt.Errorf("inspect %q: %w", requested, err) + } + if isLinkLike(info) || !sameFileSystem(rootInfo, info) || (!allowDir && !info.Mode().IsRegular()) { + return "", fmt.Errorf("%w: unsupported path type", ErrDenied) + } + return target, nil +} + +func readFixedDiff(path string, offset int64, limit, maxBytes int) ([]byte, error) { + if strings.TrimSpace(path) == "" || !filepath.IsAbs(path) { + return nil, fmt.Errorf("%w: fixed diff path is invalid", ErrDenied) + } + info, err := os.Lstat(filepath.Clean(path)) + if err != nil { + return nil, fmt.Errorf("fixed diff: %w", err) + } + if isLinkLike(info) || !info.Mode().IsRegular() { + return nil, fmt.Errorf("%w: fixed diff is not a regular file", ErrDenied) + } + file, err := os.Open(filepath.Clean(path)) // #nosec G304 -- the fixed path is CR-owned configuration and checked above. + if err != nil { + return nil, fmt.Errorf("fixed diff: %w", err) + } + defer file.Close() + data, err := readRange(file, offset, limit, maxBytes) + if err != nil { + return nil, fmt.Errorf("fixed diff: %w", err) + } + return data, nil +} + +func readRange(file *os.File, offset int64, limit, maxBytes int) ([]byte, error) { + if offset < 0 || limit < 0 { + return nil, fmt.Errorf("%w: range offset and limit must be non-negative", ErrDenied) + } + info, err := file.Stat() + if err != nil { + return nil, err + } + total := info.Size() + if offset > total { + return nil, fmt.Errorf("%w: range offset %d exceeds file size %d", ErrDenied, offset, total) + } + if offset == 0 && limit == 0 && total <= int64(maxBytes) { + return io.ReadAll(io.LimitReader(file, int64(maxBytes)+1)) + } + want := limit + if want == 0 || want > maxBytes { + want = maxBytes + } + remaining := total - offset + if int64(want) > remaining { + want = int(remaining) + } +rangeLoop: + for { + end := offset + int64(want) + next := end + if end >= total { + next = -1 + } + header := []byte(fmt.Sprintf("[cr-range offset=%d end=%d total=%d next_offset=%d]\n", offset, end, total, next)) + available := maxBytes - len(header) + if available < 0 { + return nil, fmt.Errorf("%w: output cap is too small for range metadata", ErrDenied) + } + if want > available { + want = available + continue + } + data := make([]byte, want) + if want > 0 { + n, readErr := file.ReadAt(data, offset) + data = data[:n] + if readErr != nil && !errors.Is(readErr, io.EOF) { + return nil, readErr + } + } + if end < total && !utf8.Valid(data) { + for trim := 1; trim < utf8.UTFMax && trim < len(data); trim++ { + if utf8.Valid(data[:len(data)-trim]) { + want = len(data) - trim + continue rangeLoop + } + } + } + return append(header, data...), nil + } +} + +func search(ctx context.Context, rootFS fs.FS, start, query string, maxBytes int) ([]byte, error) { + var out bytes.Buffer + err := walkRegularFiles(ctx, rootFS, start, func(path string) error { + file, err := rootFS.Open(path) + if err != nil { + return err + } + collector := newSearchMatchCollector(maxBytes) + binary, scanErr := searchFile(ctx, file, query, collector) + _ = file.Close() + if scanErr != nil { + return scanErr + } + if binary { + return nil + } + for _, match := range collector.matches { + fmt.Fprintf(&out, "%s:%d:%s\n", filepath.ToSlash(path), match.line, match.text) + if out.Len() >= maxBytes { + return fs.SkipAll + } + } + return nil + }) + if err != nil { + return nil, err + } + return out.Bytes(), nil +} + +type searchMatch struct { + line int + text string +} + +// searchMatchStorageOverhead conservatively charges retained slice and string +// metadata against the same deterministic budget as match text. +const searchMatchStorageOverhead = 32 + +type searchMatchCollector struct { + matches []searchMatch + retainedBytes int + maxBytes int +} + +func newSearchMatchCollector(maxBytes int) *searchMatchCollector { + return &searchMatchCollector{maxBytes: maxBytes} +} + +func (c *searchMatchCollector) add(match searchMatch) { + storageBytes := len(match.text) + searchMatchStorageOverhead + if c.retainedBytes+storageBytes > c.maxBytes { + return + } + c.matches = append(c.matches, match) + c.retainedBytes += storageBytes +} + +func searchFile(ctx context.Context, file io.Reader, query string, collector *searchMatchCollector) (bool, error) { + if len(query) > maxConfiguredOutputBytes { + return false, fmt.Errorf("%w: search query is too large", ErrDenied) + } + reader := bufio.NewReaderSize(file, 64*1024) + queryBytes := []byte(query) + lineNumber := 1 + lineMatched := false + lineBinary := false + fileBinary := false + lineBytes := 0 + preview := make([]byte, 0, min(collector.maxBytes, 4096)) + tail := make([]byte, 0, max(0, len(queryBytes)-1)) + finishLine := func() { + if lineMatched && !lineBinary && collector.retainedBytes < collector.maxBytes { + text := string(bytes.ToValidUTF8(preview, []byte("�"))) + if lineBytes > len(preview) { + text += "…" + } + collector.add(searchMatch{line: lineNumber, text: text}) + } + lineNumber++ + lineMatched = false + lineBinary = false + lineBytes = 0 + preview = preview[:0] + tail = tail[:0] + } + for { + if err := ctx.Err(); err != nil { + return false, err + } + fragment, readErr := reader.ReadSlice('\n') + hasNewline := len(fragment) > 0 && fragment[len(fragment)-1] == '\n' + if hasNewline { + fragment = fragment[:len(fragment)-1] + } + if bytes.IndexByte(fragment, 0) >= 0 { + lineBinary = true + fileBinary = true + } + lineBytes += len(fragment) + if len(preview) < cap(preview) { + remaining := cap(preview) - len(preview) + if remaining > len(fragment) { + remaining = len(fragment) + } + preview = append(preview, fragment[:remaining]...) + } + candidate := make([]byte, 0, len(tail)+len(fragment)) + candidate = append(candidate, tail...) + candidate = append(candidate, fragment...) + if bytes.Contains(candidate, queryBytes) { + lineMatched = true + } + keep := min(max(0, len(queryBytes)-1), len(candidate)) + tail = append(tail[:0], candidate[len(candidate)-keep:]...) + if hasNewline { + finishLine() + } + switch { + case readErr == nil: + continue + case errors.Is(readErr, bufio.ErrBufferFull): + continue + case errors.Is(readErr, io.EOF): + if lineBytes > 0 || len(preview) > 0 { + finishLine() + } + return fileBinary, nil + default: + return false, readErr + } + } +} + +func list(ctx context.Context, rootFS fs.FS, start string, maxBytes int) ([]byte, error) { + paths := make([]string, 0) + collectedBytes := 0 + err := walkRegularFiles(ctx, rootFS, start, func(path string) error { + path = filepath.ToSlash(path) + paths = append(paths, path) + collectedBytes += len(path) + 1 + if collectedBytes >= maxBytes { + return fs.SkipAll + } + return nil + }) + if err != nil { + return nil, err + } + sort.Strings(paths) + var out bytes.Buffer + for _, path := range paths { + out.WriteString(path) + out.WriteByte('\n') + if out.Len() >= maxBytes { + break + } + } + return out.Bytes(), nil +} + +func walkRegularFiles(ctx context.Context, rootFS fs.FS, root string, visit func(string) error) error { + rootInfo, err := fs.Stat(rootFS, ".") + if err != nil { + return err + } + return fs.WalkDir(rootFS, filepath.ToSlash(root), func(path string, entry fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if err := ctx.Err(); err != nil { + return err + } + if path != root && isVCSMetadataDir(entry.Name()) { + if entry.IsDir() { + return filepath.SkipDir + } + return nil + } + info, err := entry.Info() + if err != nil { + return err + } + if path != root && (isLinkLike(info) || !sameFileSystem(rootInfo, info)) { + if entry.IsDir() { + return filepath.SkipDir + } + return nil + } + if entry.IsDir() { + return nil + } + if !info.Mode().IsRegular() { + return nil + } + return visit(path) + }) +} + +func isVCSMetadataDir(name string) bool { + switch { + case strings.EqualFold(name, ".git"), strings.EqualFold(name, ".hg"), strings.EqualFold(name, ".svn"): + return true + default: + return false + } +} + +func boundOutput(data []byte, maxBytes int) string { + if len(data) <= maxBytes { + return string(data) + } + const marker = "[truncated]\n" + if maxBytes <= len(marker) { + return marker[:maxBytes] + } + return string(data[:maxBytes-len(marker)]) + marker +} diff --git a/internal/pireviewtool/tool_test.go b/internal/pireviewtool/tool_test.go new file mode 100644 index 00000000..a59ffa6b --- /dev/null +++ b/internal/pireviewtool/tool_test.go @@ -0,0 +1,383 @@ +package pireviewtool + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + "testing/iotest" + "unicode/utf8" +) + +func TestRunDecodesOneStrictRequest(t *testing.T) { + repo, diff := reviewerToolFixture(t) + configPath := filepath.Join(t.TempDir(), "config.json") + configJSON := `{"repo_dir":` + quoteJSON(t, repo) + `,"diff_path":` + quoteJSON(t, diff) + `,"max_output_bytes":4096,"timeout_ms":1000}` + if err := os.WriteFile(configPath, []byte(configJSON), 0o600); err != nil { + t.Fatalf("WriteFile(config): %v", err) + } + var stdout, stderr bytes.Buffer + code := Run(context.Background(), []string{"--config", configPath}, strings.NewReader(`{"tool":"cr_read","path":"assigned.go"}`), &stdout, &stderr) + if code != 0 || stdout.String() != "package root\n" || stderr.Len() != 0 { + t.Fatalf("Run = %d, stdout %q, stderr %q", code, stdout.String(), stderr.String()) + } + stdout.Reset() + stderr.Reset() + code = Run(context.Background(), []string{"--config", configPath}, strings.NewReader(`{"tool":"cr_read","path":"assigned.go","command":"sh"}`), &stdout, &stderr) + if code == 0 || !strings.Contains(stderr.String(), "unknown field") { + t.Fatalf("strict Run = %d, stderr %q, want unknown-field failure", code, stderr.String()) + } +} + +func TestRunReadAndDiffRangesReachContentBeyondOutputCap(t *testing.T) { + repo, diff := reviewerToolFixture(t) + largeFile := strings.Repeat("a", 40*1024) + "READ_TARGET" + strings.Repeat("b", 40*1024) + if err := os.WriteFile(filepath.Join(repo, "large.txt"), []byte(largeFile), 0o600); err != nil { + t.Fatalf("WriteFile(large): %v", err) + } + largeDiff := strings.Repeat("x", 40*1024) + "DIFF_TARGET" + strings.Repeat("y", 40*1024) + if err := os.WriteFile(diff, []byte(largeDiff), 0o600); err != nil { + t.Fatalf("WriteFile(diff): %v", err) + } + configPath := writeToolConfig(t, Config{RepoDir: repo, DiffPath: diff, MaxOutputBytes: 128, TimeoutMS: 1000}) + + for _, tt := range []struct { + name string + request string + want string + }{ + {name: "read", request: `{"tool":"cr_read","path":"large.txt","offset":40950,"limit":40}`, want: "READ_TARGET"}, + {name: "diff", request: `{"tool":"cr_diff","offset":40950,"limit":40}`, want: "DIFF_TARGET"}, + } { + t.Run(tt.name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + code := Run(context.Background(), []string{"--config", configPath}, strings.NewReader(tt.request), &stdout, &stderr) + if code != 0 { + t.Fatalf("Run = %d, stderr %q", code, stderr.String()) + } + if !strings.Contains(stdout.String(), tt.want) || !strings.Contains(stdout.String(), "offset=40950") || !strings.Contains(stdout.String(), "next_offset=40990") { + t.Fatalf("range output = %q, want target and deterministic next offset", stdout.String()) + } + if len(stdout.String()) > 128 { + t.Fatalf("range output = %d bytes, want aggregate tool cap 128", len(stdout.String())) + } + }) + } +} + +func TestExecuteReadRangesPreserveUTF8AcrossContinuationBoundaries(t *testing.T) { + repo, diff := reviewerToolFixture(t) + want := strings.Repeat("🙂", 100) + if err := os.WriteFile(filepath.Join(repo, "unicode.txt"), []byte(want), 0o600); err != nil { + t.Fatalf("WriteFile(unicode): %v", err) + } + config := Config{RepoDir: repo, DiffPath: diff, MaxOutputBytes: 128} + var reconstructed []byte + offset := int64(0) + for page := 0; page < 20; page++ { + got, err := Execute(context.Background(), config, Request{Tool: ToolRead, Path: "unicode.txt", Offset: offset}) + if err != nil { + t.Fatalf("Execute(page %d): %v", page, err) + } + headerEnd := strings.IndexByte(got, '\n') + if headerEnd < 0 { + t.Fatalf("page %d = %q, want range header", page, got) + } + var start, end, total, next int64 + if _, err := fmt.Sscanf(got[:headerEnd], "[cr-range offset=%d end=%d total=%d next_offset=%d]", &start, &end, &total, &next); err != nil { + t.Fatalf("parse page %d header %q: %v", page, got[:headerEnd], err) + } + payload := []byte(got[headerEnd+1:]) + if !utf8.Valid(payload) { + t.Fatalf("page %d payload splits a UTF-8 sequence: %x", page, payload) + } + if start != offset || end != start+int64(len(payload)) || total != int64(len([]byte(want))) { + t.Fatalf("page %d metadata = %d/%d/%d for %d payload bytes", page, start, end, total, len(payload)) + } + reconstructed = append(reconstructed, payload...) + if next < 0 { + break + } + offset = next + } + if string(reconstructed) != want { + t.Fatalf("reconstructed %d bytes, want all %d UTF-8 bytes", len(reconstructed), len([]byte(want))) + } +} + +func TestExecuteReadSearchListAndFixedDiff(t *testing.T) { + repo, diff := reviewerToolFixture(t) + config := Config{RepoDir: repo, DiffPath: diff, AllowedFiles: []string{"assigned.go"}, MaxOutputBytes: 4096} + + read, err := Execute(context.Background(), config, Request{Tool: ToolRead, Path: "nested/context.go"}) + if err != nil || !strings.Contains(read, "package nested") { + t.Fatalf("read = %q, err = %v", read, err) + } + // AllowedFiles is assignment metadata, not a filesystem sensitivity boundary. + if !strings.Contains(read, "context") { + t.Fatalf("read outside allowed_files = %q, want repository context", read) + } + search, err := Execute(context.Background(), config, Request{Tool: ToolSearch, Query: "needle"}) + if err != nil || !strings.Contains(search, "nested/context.go:3") { + t.Fatalf("search = %q, err = %v", search, err) + } + list, err := Execute(context.Background(), config, Request{Tool: ToolList, Path: "nested"}) + if err != nil || !strings.Contains(list, "nested/context.go") { + t.Fatalf("list = %q, err = %v", list, err) + } + fixedDiff, err := Execute(context.Background(), config, Request{Tool: ToolDiff, Path: "../../etc/passwd"}) + if err != nil || fixedDiff != "fixed pinned diff\n" { + t.Fatalf("diff = %q, err = %v", fixedDiff, err) + } +} + +func TestExecuteRejectsPathEscapesLinksAndUnknownTools(t *testing.T) { + repo, diff := reviewerToolFixture(t) + outside := filepath.Join(t.TempDir(), "outside.txt") + if err := os.WriteFile(outside, []byte("secret"), 0o600); err != nil { + t.Fatalf("WriteFile(outside): %v", err) + } + link := filepath.Join(repo, "outside-link") + if err := os.Symlink(outside, link); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + config := Config{RepoDir: repo, DiffPath: diff, MaxOutputBytes: 4096} + + for _, request := range []Request{ + {Tool: ToolRead, Path: outside}, + {Tool: ToolRead, Path: "../outside.txt"}, + {Tool: ToolRead, Path: "nested/../../outside.txt"}, + {Tool: ToolRead, Path: "outside-link"}, + {Tool: ToolList, Path: "outside-link"}, + {Tool: "bash", Path: "nested/context.go"}, + } { + if _, err := Execute(context.Background(), config, request); !errors.Is(err, ErrDenied) { + t.Errorf("Execute(%+v) error = %v, want ErrDenied", request, err) + } + } +} + +func TestExecuteRejectsSymlinkComponentsAndDiffLinks(t *testing.T) { + repo, diff := reviewerToolFixture(t) + outsideDir := t.TempDir() + if err := os.WriteFile(filepath.Join(outsideDir, "secret.txt"), []byte("secret"), 0o600); err != nil { + t.Fatalf("WriteFile(secret): %v", err) + } + if err := os.Symlink(outsideDir, filepath.Join(repo, "linked-dir")); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + config := Config{RepoDir: repo, DiffPath: diff, MaxOutputBytes: 4096} + if _, err := Execute(context.Background(), config, Request{Tool: ToolRead, Path: "linked-dir/secret.txt"}); !errors.Is(err, ErrDenied) { + t.Fatalf("symlink component error = %v, want ErrDenied", err) + } + diffLink := filepath.Join(t.TempDir(), "diff-link") + if err := os.Symlink(diff, diffLink); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + config.DiffPath = diffLink + if _, err := Execute(context.Background(), config, Request{Tool: ToolDiff}); !errors.Is(err, ErrDenied) { + t.Fatalf("symlink diff error = %v, want ErrDenied", err) + } +} + +func TestExecuteListAndSearchExcludeCloneMetadata(t *testing.T) { + gitPath, err := exec.LookPath("git") + if err != nil { + t.Skip("Git is not installed") + } + source := filepath.Join(t.TempDir(), "source") + clone := filepath.Join(t.TempDir(), "clone") + if err := os.MkdirAll(source, 0o700); err != nil { + t.Fatalf("MkdirAll(source): %v", err) + } + runGitForToolTest(t, gitPath, source, "init", "-b", "main") + runGitForToolTest(t, gitPath, source, "config", "user.name", "Tool Test") + runGitForToolTest(t, gitPath, source, "config", "user.email", "tool@example.com") + if err := os.WriteFile(filepath.Join(source, "visible.txt"), []byte("visible needle\n"), 0o600); err != nil { + t.Fatalf("WriteFile(visible): %v", err) + } + runGitForToolTest(t, gitPath, source, "add", "visible.txt") + runGitForToolTest(t, gitPath, source, "commit", "-m", "fixture") + runGitForToolTest(t, gitPath, "", "clone", source, clone) + if err := os.WriteFile(filepath.Join(clone, ".git", "metadata-secret"), []byte("metadata needle\n"), 0o600); err != nil { + t.Fatalf("WriteFile(metadata): %v", err) + } + if err := os.MkdirAll(filepath.Join(clone, "submodule"), 0o700); err != nil { + t.Fatalf("MkdirAll(submodule): %v", err) + } + if err := os.WriteFile(filepath.Join(clone, "submodule", ".git"), []byte("gitdir: ../.git/modules/submodule\nmetadata needle\n"), 0o600); err != nil { + t.Fatalf("WriteFile(submodule metadata): %v", err) + } + diff := filepath.Join(t.TempDir(), "diff.patch") + if err := os.WriteFile(diff, []byte("diff\n"), 0o600); err != nil { + t.Fatalf("WriteFile(diff): %v", err) + } + config := Config{RepoDir: clone, DiffPath: diff, MaxOutputBytes: 4096} + + listed, err := Execute(context.Background(), config, Request{Tool: ToolList}) + if err != nil { + t.Fatalf("Execute(list): %v", err) + } + if strings.Contains(listed, ".git") || !strings.Contains(listed, "visible.txt") { + t.Fatalf("list = %q, want worktree files without VCS metadata", listed) + } + searched, err := Execute(context.Background(), config, Request{Tool: ToolSearch, Query: "needle"}) + if err != nil { + t.Fatalf("Execute(search): %v", err) + } + if strings.Contains(searched, ".git") || strings.Contains(searched, "metadata needle") || !strings.Contains(searched, "visible needle") { + t.Fatalf("search = %q, want worktree match without VCS metadata", searched) + } +} + +func TestExecuteSearchSkipsOversizedAndBinaryLinesWithoutAbortingRepository(t *testing.T) { + repo, diff := reviewerToolFixture(t) + if err := os.WriteFile(filepath.Join(repo, "a-oversized.txt"), []byte(strings.Repeat("x", 2*1024*1024)), 0o600); err != nil { + t.Fatalf("WriteFile(oversized): %v", err) + } + binary := append([]byte{0, 1, 2, 3}, []byte("needle in binary")...) + if err := os.WriteFile(filepath.Join(repo, "b-binary.bin"), binary, 0o600); err != nil { + t.Fatalf("WriteFile(binary): %v", err) + } + if err := os.WriteFile(filepath.Join(repo, "z-match.txt"), []byte("final needle\n"), 0o600); err != nil { + t.Fatalf("WriteFile(match): %v", err) + } + + got, err := Execute(context.Background(), Config{RepoDir: repo, DiffPath: diff, MaxOutputBytes: 4096}, Request{Tool: ToolSearch, Query: "needle"}) + if err != nil { + t.Fatalf("Execute(search): %v", err) + } + if !strings.Contains(got, "z-match.txt:1:final needle") { + t.Fatalf("search = %q, want later text match after oversized file", got) + } + if strings.Contains(got, "b-binary.bin") { + t.Fatalf("search = %q, want binary file omitted", got) + } +} + +func TestSearchFileRetainsOnlyBoundedMatchesBeforeEOF(t *testing.T) { + const ( + maxBytes = 256 + matchCount = 10_000 + ) + beforeEOF := errors.New("reader stopped before EOF") + reader := io.MultiReader( + strings.NewReader(strings.Repeat("needle repeated content\n", matchCount)), + iotest.ErrReader(beforeEOF), + ) + collector := newSearchMatchCollector(maxBytes) + binary, err := searchFile(context.Background(), reader, "needle", collector) + if !errors.Is(err, beforeEOF) { + t.Fatalf("searchFile error = %v, want pre-EOF sentinel", err) + } + if binary { + t.Fatal("searchFile reported text fixture as binary") + } + if collector.retainedBytes > maxBytes { + t.Fatalf("retained match storage = %d bytes across %d matches, want <= %d", collector.retainedBytes, len(collector.matches), maxBytes) + } + if len(collector.matches) >= matchCount { + t.Fatalf("retained %d matches, want collector to stop retaining before EOF", len(collector.matches)) + } +} + +func TestExecuteBoundsOutputAndHonorsCancellation(t *testing.T) { + repo, diff := reviewerToolFixture(t) + if err := os.WriteFile(filepath.Join(repo, "nested", "context.go"), []byte(strings.Repeat("context ", 80)), 0o600); err != nil { + t.Fatalf("WriteFile(context): %v", err) + } + config := Config{RepoDir: repo, DiffPath: diff, MaxOutputBytes: 128} + got, err := Execute(context.Background(), config, Request{Tool: ToolRead, Path: "nested/context.go"}) + if err != nil { + t.Fatalf("Execute(read): %v", err) + } + if len(got) > config.MaxOutputBytes || !strings.Contains(got, "next_offset=") { + t.Fatalf("bounded output = %q (%d bytes), want <= %d with range metadata", got, len(got), config.MaxOutputBytes) + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() + if _, err := Execute(ctx, Config{RepoDir: repo, DiffPath: diff, MaxOutputBytes: 4096}, Request{Tool: ToolList}); !errors.Is(err, context.Canceled) { + t.Fatalf("canceled Execute error = %v, want context.Canceled", err) + } +} + +func TestExecuteFixedDiffDoesNotInvokeGitHelpersOrConfig(t *testing.T) { + repo, diff := reviewerToolFixture(t) + marker := filepath.Join(t.TempDir(), "external-diff-ran") + t.Setenv("GIT_EXTERNAL_DIFF", marker) + t.Setenv("GIT_CONFIG_GLOBAL", marker) + got, err := Execute(context.Background(), Config{RepoDir: repo, DiffPath: diff, MaxOutputBytes: 4096}, Request{Tool: ToolDiff}) + if err != nil || got != "fixed pinned diff\n" { + t.Fatalf("Execute(diff) = %q, %v", got, err) + } + if _, err := os.Stat(marker); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("git helper marker exists or stat failed: %v", err) + } +} + +func TestResolveRepoPathRejectsCrossVolumeWhenRepresentable(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("cross-volume paths are represented only on Windows") + } + if _, err := resolveRepoPath(`C:\repo`, `D:\escape`); !errors.Is(err, ErrDenied) { + t.Fatalf("cross-volume error = %v, want ErrDenied", err) + } +} + +func reviewerToolFixture(t *testing.T) (string, string) { + t.Helper() + repo := filepath.Join(t.TempDir(), "repo") + if err := os.MkdirAll(filepath.Join(repo, "nested"), 0o700); err != nil { + t.Fatalf("MkdirAll(repo): %v", err) + } + if err := os.WriteFile(filepath.Join(repo, "assigned.go"), []byte("package root\n"), 0o600); err != nil { + t.Fatalf("WriteFile(assigned): %v", err) + } + if err := os.WriteFile(filepath.Join(repo, "nested", "context.go"), []byte("package nested\n\n// needle context\n"), 0o600); err != nil { + t.Fatalf("WriteFile(context): %v", err) + } + diff := filepath.Join(t.TempDir(), "diff.patch") + if err := os.WriteFile(diff, []byte("fixed pinned diff\n"), 0o600); err != nil { + t.Fatalf("WriteFile(diff): %v", err) + } + return repo, diff +} + +func quoteJSON(t *testing.T, value string) string { + t.Helper() + data, err := json.Marshal(value) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + return string(data) +} + +func writeToolConfig(t *testing.T, config Config) string { + t.Helper() + path := filepath.Join(t.TempDir(), "config.json") + data, err := json.Marshal(config) + if err != nil { + t.Fatalf("Marshal(config): %v", err) + } + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatalf("WriteFile(config): %v", err) + } + return path +} + +func runGitForToolTest(t *testing.T, gitPath, dir string, args ...string) { + t.Helper() + cmd := exec.Command(gitPath, args...) // #nosec G204 -- test launches discovered Git with fixed/test-owned arguments. + cmd.Dir = dir + if output, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, output) + } +} diff --git a/internal/workbench/workbench.go b/internal/workbench/workbench.go index 96112053..707a54ca 100644 --- a/internal/workbench/workbench.go +++ b/internal/workbench/workbench.go @@ -425,6 +425,7 @@ func prepareReviewerWorkspace(ctx context.Context, deps Deps, artifacts runartif return llm.ReviewerWorkspaceRequest{ RepoDir: workspaceRepo, ScratchDir: workspaceScratch, + DiffPath: artifacts.DiffPatch, AllowedFiles: append([]string(nil), allowedFiles...), MaxToolOutputBytes: maxToolOutputBytes, }, cleanup, nil diff --git a/internal/workbench/workbench_test.go b/internal/workbench/workbench_test.go index 63e939ea..e951fc75 100644 --- a/internal/workbench/workbench_test.go +++ b/internal/workbench/workbench_test.go @@ -276,6 +276,9 @@ func TestReviewerWorkspaceSmokeAllowsReadAndWorkspaceWrites(t *testing.T) { if requests[0].ReviewerWorkspace.MaxToolOutputBytes != defaultReviewerWorkspaceToolOutputBytes { t.Fatalf("max tool output bytes = %d, want default %d", requests[0].ReviewerWorkspace.MaxToolOutputBytes, defaultReviewerWorkspaceToolOutputBytes) } + if requests[0].ReviewerWorkspace.DiffPath != artifacts.DiffPatch { + t.Fatalf("fixed diff path = %q, want %q", requests[0].ReviewerWorkspace.DiffPath, artifacts.DiffPatch) + } if !got.ReadOK || !got.MainContainsChanged || !got.OutOfScopeReadable || !got.TrackedWriteOK || !got.UntrackedWriteOK || !got.ScratchWriteOK { t.Fatalf("smoke result = %#v, want checkout read success plus workspace and scratch writes", got) } @@ -477,6 +480,41 @@ func TestPrepareReviewerRequestAcceptsPermissionBoundedAdapter(t *testing.T) { if req.ReviewerWorkspace.RepoDir == artifacts.WorkbenchRepoDir || !strings.HasPrefix(req.ReviewerWorkspace.ScratchDir, artifacts.WorkbenchScratch+string(filepath.Separator)) { t.Fatalf("ReviewerWorkspace = %#v, want disposable repo and scratch", req.ReviewerWorkspace) } + if req.ReviewerWorkspace.DiffPath != artifacts.DiffPatch { + t.Fatalf("fixed diff path = %q, want %q", req.ReviewerWorkspace.DiffPath, artifacts.DiffPatch) + } +} + +func TestPrepareReviewerRequestValidationRetryGetsFreshWorkspaceWithSameFixedDiff(t *testing.T) { + fixture, artifacts, deps := prepareReviewerFixture(t) + adapter := &llm.FakeAdapter{ + ReviewerWorkspaceModeSet: true, + ReviewerWorkspaceModeValue: llm.ReviewerWorkspacePermissionBounded, + } + req, cleanup, err := PrepareReviewerRequest(context.Background(), deps, adapter, artifacts, fixture.headSHA, "harness:retry", []string{"main.go"}, "model", "medium", "prompt", filepath.Join(t.TempDir(), "review.jsonl")) + if err != nil { + t.Fatalf("PrepareReviewerRequest: %v", err) + } + defer cleanupForTest(t, cleanup) + firstRepo := req.ReviewerWorkspace.RepoDir + if err := os.WriteFile(filepath.Join(firstRepo, "untracked"), []byte("dirty"), 0o600); err != nil { + t.Fatalf("WriteFile(untracked): %v", err) + } + if err := req.OnValidationRetry(&req); err != nil { + t.Fatalf("OnValidationRetry: %v", err) + } + if !req.FreshValidationRetrySession || req.ReviewerWorkspace == nil { + t.Fatalf("retry request = %#v, want fresh reviewer session", req) + } + if _, err := os.Stat(filepath.Join(firstRepo, "untracked")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("first workspace stat error = %v, want cleaned", err) + } + if _, err := os.Stat(filepath.Join(req.ReviewerWorkspace.RepoDir, "untracked")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("retry workspace stat error = %v, want clean", err) + } + if req.ReviewerWorkspace.DiffPath != artifacts.DiffPatch { + t.Fatalf("retry fixed diff = %q, want %q", req.ReviewerWorkspace.DiffPath, artifacts.DiffPatch) + } } type workbenchGitFixture struct { From 27d59c833e6493b6d384f9e781f8b5633117c90c Mon Sep 17 00:00:00 2001 From: Aaron Wong <6979793+zzwong@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:16:00 -0400 Subject: [PATCH 02/20] fix: preserve Pi diff invocation evidence --- docs/checkout-native-review-contract.md | 3 + internal/llmadapters/pi_rpc.go | 74 ++++++++++++++++++++++--- internal/llmadapters/pi_rpc_test.go | 68 +++++++++++++++++++++++ 3 files changed, 138 insertions(+), 7 deletions(-) diff --git a/docs/checkout-native-review-contract.md b/docs/checkout-native-review-contract.md index 5c8e0bb5..8153922f 100644 --- a/docs/checkout-native-review-contract.md +++ b/docs/checkout-native-review-contract.md @@ -299,6 +299,9 @@ Those tools delegate to CR's bounded read-only helper: repository paths reject absolute paths, traversal, links/reparse points, and filesystem-boundary crossings, while `cr_diff` reads the run's precomputed pinned diff artifact instead of invoking Git or honoring repository/user Git configuration. +The reviewer prompt requires `cr_diff` before head-file inspection. CR reserves +space within the existing aggregate log cap for a compact `cr_diff` event +summary so operators can distinguish no invocation, failure, and completion. Read/diff responses expose bounded byte ranges with deterministic continuation offsets, and list/search omit VCS metadata such as `.git`. Per-tool output, tool duration, and aggregate reviewer RPC/stderr logs are bounded without diff --git a/internal/llmadapters/pi_rpc.go b/internal/llmadapters/pi_rpc.go index b3811883..a0df00a7 100644 --- a/internal/llmadapters/pi_rpc.go +++ b/internal/llmadapters/pi_rpc.go @@ -21,9 +21,10 @@ import ( const ( piRPCPromptID = "prompt-1" piRPCSystemPrompt = "You are a strict JSON API for code review structured output. Return exactly one JSON object that matches the requested schema. Do not include markdown fences, prose, explanations, or leading/trailing text. The first byte of your final answer must be { and the last byte must be }." - piRPCReviewerSystemPrompt = piRPCSystemPrompt + " Inspect the disposable repository only through the CR-owned cr_read, cr_search, cr_list, and cr_diff tools. These tools are read-only; do not request shell, write, edit, or any other tool." + piRPCReviewerSystemPrompt = piRPCSystemPrompt + " Inspect the disposable repository only through the CR-owned cr_read, cr_search, cr_list, and cr_diff tools. Invoke cr_diff before cr_read, cr_search, or cr_list so the review starts from the pinned change. If cr_diff fails, record that exact tool failure as a constraint before inspecting allowed head files. These tools are read-only; do not request shell, write, edit, or any other tool." piRPCReviewerToolTimeout = 15 * time.Second piRPCReviewerToolNames = "cr_read,cr_search,cr_list,cr_diff" + piRPCToolEvidenceReserve = 256 piRPCPreflightTimeout = 5 * time.Second piRPCPreflightOutputBytes = 64 * 1024 ) @@ -164,7 +165,8 @@ func (a *PiRPCAdapter) Start(ctx context.Context, req Request) (Stream, error) { logBytesLeft: -1, } if req.ReviewerWorkspace != nil { - stream.logBytesLeft = req.ReviewerWorkspace.MaxToolOutputBytes + stream.toolEvidenceBytesLeft = min(piRPCToolEvidenceReserve, req.ReviewerWorkspace.MaxToolOutputBytes) + stream.logBytesLeft = req.ReviewerWorkspace.MaxToolOutputBytes - stream.toolEvidenceBytesLeft } go stream.run(process.Context(), process.Command(), process.Stdout(), process.Stderr()) return stream, nil @@ -410,11 +412,15 @@ func writePiRPCPrompt(stdin io.Writer, prompt string) error { type piRPCStream struct { baseStream - stdin io.Closer - allowReviewerTools bool - logLimitMu sync.Mutex - logBytesLeft int - logCapped bool + stdin io.Closer + allowReviewerTools bool + logLimitMu sync.Mutex + logBytesLeft int + logCapped bool + toolEvidenceBytesLeft int + diffToolStarted int + diffToolCompleted int + diffToolFailed int } func (s *piRPCStream) run(ctx context.Context, cmd *exec.Cmd, stdout io.Reader, stderr io.Reader) { @@ -434,6 +440,7 @@ func (s *piRPCStream) run(ctx context.Context, cmd *exec.Cmd, stdout io.Reader, } waitErr := cmd.Wait() <-stderrDone + s.writeReviewerToolEvidence() result := subprocessResult{response: scanResult.response} switch { @@ -488,6 +495,7 @@ func (s *piRPCStream) scanStdout(stdout io.Reader) piRPCScanResult { result.err = ErrToolUse return result } + s.observeReviewerToolEvent(event) if event.responseFailure != "" { s.Cancel() result.err = fmt.Errorf("llm pi rpc: prompt failed: %s", event.responseFailure) @@ -549,6 +557,44 @@ func (s *piRPCStream) writeLog(p []byte) { s.logCapped = true } +func (s *piRPCStream) observeReviewerToolEvent(event piRPCEvent) { + if !s.allowReviewerTools || event.toolName != "cr_diff" { + return + } + if event.toolStarted { + s.diffToolStarted++ + } + if event.toolCompleted { + s.diffToolCompleted++ + } + if event.toolFailed { + s.diffToolFailed++ + } +} + +func (s *piRPCStream) writeReviewerToolEvidence() { + if !s.allowReviewerTools || s.toolEvidenceBytesLeft <= 0 { + return + } + status := "not_invoked" + switch { + case s.diffToolFailed > 0: + status = "failed" + case s.diffToolCompleted > 0: + status = "succeeded" + case s.diffToolStarted > 0: + status = "incomplete" + } + evidence := []byte(fmt.Sprintf("codereview-pi-tool-evidence tool=cr_diff status=%s started=%d completed=%d failed=%d\n", status, s.diffToolStarted, s.diffToolCompleted, s.diffToolFailed)) + s.logLimitMu.Lock() + defer s.logLimitMu.Unlock() + writeBytes := min(len(evidence), s.toolEvidenceBytesLeft) + if writeBytes > 0 { + s.WriteLog(evidence[:writeBytes]) + s.toolEvidenceBytesLeft -= writeBytes + } +} + func normalizePiRPCLogLine(line []byte) []byte { logLine := append([]byte(nil), line...) if len(logLine) == 0 { @@ -617,6 +663,9 @@ type piRPCEvent struct { usage Usage toolUse bool toolName string + toolStarted bool + toolCompleted bool + toolFailed bool responseFailure string agentEnd bool } @@ -637,6 +686,17 @@ func parsePiRPCEvent(line []byte) (piRPCEvent, error) { } if event.toolUse { event.toolName = firstRawString(raw, "toolName", "tool_name", "name") + switch eventType { + case "tool_execution_start": + event.toolStarted = true + case "tool_execution_end": + event.toolCompleted = true + event.toolFailed = rawBool(raw, "isError") + var resultRaw map[string]json.RawMessage + if err := json.Unmarshal(raw["result"], &resultRaw); err == nil && rawBool(resultRaw, "isError") { + event.toolFailed = true + } + } } if id := firstRawString(raw, "sessionId", "session_id"); id != "" { event.sessionID = id diff --git a/internal/llmadapters/pi_rpc_test.go b/internal/llmadapters/pi_rpc_test.go index 74af9349..b55467e4 100644 --- a/internal/llmadapters/pi_rpc_test.go +++ b/internal/llmadapters/pi_rpc_test.go @@ -151,6 +151,12 @@ func TestPiRPCReviewerWorkspaceLaunchUsesOnlyCROwnedTools(t *testing.T) { t.Fatalf("args = %#v, reviewer extension tools must remain enabled", record.AdapterArgs) } assertFlagValue(t, record.AdapterArgs, "--tools", piRPCReviewerToolNames) + reviewerPrompt := flagValue(record.AdapterArgs, "--system-prompt") + for _, instruction := range []string{"Invoke cr_diff before cr_read, cr_search, or cr_list", "If cr_diff fails"} { + if !strings.Contains(reviewerPrompt, instruction) { + t.Fatalf("reviewer system prompt = %q, want instruction %q", reviewerPrompt, instruction) + } + } for _, flag := range []string{"--no-builtin-tools", "--no-context-files", "--no-approve", "--no-extensions", "--no-skills", "--no-prompt-templates", "--no-themes", "--no-session"} { if !containsFlag(record.AdapterArgs, flag) { t.Fatalf("args = %#v, want %s", record.AdapterArgs, flag) @@ -259,6 +265,61 @@ func TestPiRPCReviewerLogCapDoesNotBreakProtocolCompletion(t *testing.T) { if !strings.Contains(string(logged), "reviewer RPC/stderr log cap reached") { t.Fatalf("reviewer log = %q, want cap marker", logged) } + if !strings.Contains(string(logged), "codereview-pi-tool-evidence tool=cr_diff status=not_invoked started=0 completed=0 failed=0") { + t.Fatalf("reviewer log = %q, want bounded no-invocation evidence", logged) + } +} + +func TestPiRPCReviewerLogCapPreservesDiffFailureEvidence(t *testing.T) { + tempDir := t.TempDir() + repoDir := filepath.Join(tempDir, "repo") + scratchDir := filepath.Join(tempDir, "scratch") + for _, dir := range []string{repoDir, scratchDir} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("MkdirAll(%s): %v", dir, err) + } + } + diffPath := filepath.Join(tempDir, "diff.patch") + if err := os.WriteFile(diffPath, []byte("fixed diff\n"), 0o600); err != nil { + t.Fatalf("WriteFile(diff): %v", err) + } + logPath := filepath.Join(tempDir, "reviewer.jsonl") + adapter := NewPiRPCAdapter(PiRPCOptions{ + Command: os.Args[0], + commandArgsPrefix: piRPCHelperPrefix(), + Env: piRPCHelperEnv("reviewer-diff-failure-log-flood", filepath.Join(tempDir, "record.json")), + Timeout: 5 * time.Second, + }) + stream, err := adapter.Start(context.Background(), Request{ + Prompt: "review", + LogPath: logPath, + ReviewerWorkspace: &ReviewerWorkspaceRequest{ + RepoDir: repoDir, ScratchDir: scratchDir, DiffPath: diffPath, MaxToolOutputBytes: 2048, + }, + }) + if err != nil { + t.Fatalf("Start: %v", err) + } + response, err := stream.Wait(context.Background()) + if err != nil { + t.Fatalf("Wait: %v", err) + } + if string(response.StructuredOutput) != `{"ok":true}` { + t.Fatalf("StructuredOutput = %s, want completed final response", response.StructuredOutput) + } + logged, err := os.ReadFile(logPath) // #nosec G304 -- logPath is rooted in t.TempDir. + if err != nil { + t.Fatalf("ReadFile(log): %v", err) + } + if len(logged) > 2048 { + t.Fatalf("reviewer log = %d bytes, want aggregate cap 2048", len(logged)) + } + if !strings.Contains(string(logged), "reviewer RPC/stderr log cap reached") { + t.Fatalf("reviewer log = %q, want cap marker", logged) + } + if !strings.Contains(string(logged), "codereview-pi-tool-evidence tool=cr_diff status=failed started=1 completed=1 failed=1") { + t.Fatalf("reviewer log = %q, want bounded failed-invocation evidence", logged) + } } func TestPiRPCReviewerExtensionLoadsInInstalledPi(t *testing.T) { @@ -772,6 +833,13 @@ func TestPiRPCHelperProcess(_ *testing.T) { } fmt.Println(`{"type":"message_end","message":{"role":"assistant","content":[{"type":"text","text":"{\"ok\":true}"}]}}`) fmt.Println(`{"type":"agent_end","messages":[{"role":"assistant","content":[{"type":"text","text":"{\"ok\":true}"}]}]}`) + case "reviewer-diff-failure-log-flood": + fmt.Fprintln(os.Stderr, strings.Repeat("stderr flood\n", 1000)) + fmt.Println(`{"id":"prompt-1","type":"response","command":"prompt","success":true}`) + fmt.Println(`{"type":"tool_execution_start","toolCallId":"diff-1","toolName":"cr_diff","args":{}}`) + fmt.Println(`{"type":"tool_execution_end","toolCallId":"diff-1","toolName":"cr_diff","result":{"content":[{"type":"text","text":"fixed diff unavailable"}],"isError":true}}`) + fmt.Println(`{"type":"message_end","message":{"role":"assistant","content":[{"type":"text","text":"{\"ok\":true}"}]}}`) + fmt.Println(`{"type":"agent_end","messages":[{"role":"assistant","content":[{"type":"text","text":"{\"ok\":true}"}]}]}`) case "sleep": time.Sleep(10 * time.Second) case "malformed": From 85551fa326b68e9969d2223ec02e21fb85c5f4f4 Mon Sep 17 00:00:00 2001 From: Aaron Wong <6979793+zzwong@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:45:46 -0400 Subject: [PATCH 03/20] fix: correct reused-cohort summary accounting --- internal/pipeline/pipeline.go | 39 +++++++++++++++++++++++++++--- internal/pipeline/pipeline_test.go | 30 +++++++++++++++++++++-- 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 6f73af40..2617236d 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -2297,20 +2297,25 @@ func (opts Options) buildRunSummary(req Request, inputs planRunInputs) (reviewpl agentByRow[draft.RowID] = *draft.AgentID } - workstreams := []reviewplan.WorkstreamUsage{workstreamUsage(orchestratorSelectionStage, inputs.selection)} + workstreams := make([]reviewplan.WorkstreamUsage, 0, len(inputs.reviewers)+2) + if sessionDraftExecuted(inputs.selection) { + workstreams = append(workstreams, workstreamUsage(orchestratorSelectionStage, inputs.selection)) + } selectedIDs := make([]string, 0, len(inputs.selectedAgents)) for _, selected := range inputs.selectedAgents { selectedIDs = append(selectedIDs, selected.AgentID) - if draft, ok := reviewerByAgent[selected.AgentID]; ok { + if draft, ok := reviewerByAgent[selected.AgentID]; ok && sessionDraftExecuted(draft) { workstreams = append(workstreams, workstreamUsage(selected.AgentID, draft)) } } - workstreams = append(workstreams, workstreamUsage(orchestratorRollupStage, inputs.rollup)) + if sessionDraftExecuted(inputs.rollup) { + workstreams = append(workstreams, workstreamUsage(orchestratorRollupStage, inputs.rollup)) + } wallMS := opts.now().Sub(inputs.startedAt).Milliseconds() summary := reviewplan.RunSummary{ ToolVersion: req.ToolVersion, - Adapter: inputs.selection.Adapter, + Adapter: runAdapter(inputs), Model: sharedWorkstreamModel(workstreams), PostingIdentity: runlifecycle.PostingKey(req.PostingIdentity), SelectedReviewers: selectedIDs, @@ -2329,6 +2334,32 @@ func (opts Options) buildRunSummary(req Request, inputs planRunInputs) (reviewpl return summary, findingReviewers } +// sessionDraftExecuted distinguishes a phase that ran (or loaded durable +// telemetry) from the zero draft left by a reused cohort that skipped it. +func sessionDraftExecuted(draft sessionDraft) bool { + return strings.TrimSpace(draft.Adapter) != "" || + strings.TrimSpace(draft.ProviderSessionID) != "" || + strings.TrimSpace(draft.ProviderReportedSessionID) != "" || + !draft.StartedAt.IsZero() || !draft.CompletedAt.IsZero() || + draft.Response.DurationMS != 0 || draft.Response.Usage.TokensIn != nil || + draft.Response.Usage.TokensOut != nil || draft.Response.Usage.CacheRead != nil || + draft.Response.Usage.CacheCreate != nil || draft.Response.Usage.CostUSD != nil +} + +// runAdapter recovers the adapter from an executed phase when selection was +// skipped during cohort reuse. +func runAdapter(inputs planRunInputs) string { + if adapter := strings.TrimSpace(inputs.selection.Adapter); adapter != "" { + return adapter + } + for _, draft := range inputs.reviewers { + if adapter := strings.TrimSpace(draft.Adapter); adapter != "" { + return adapter + } + } + return strings.TrimSpace(inputs.rollup.Adapter) +} + func reviewerFailureSummaries(failures []ReviewerFailure) []reviewplan.ReviewerFailureSummary { out := make([]reviewplan.ReviewerFailureSummary, 0, len(failures)) for _, failure := range failures { diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 8cca61e5..651d36a0 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -3367,8 +3367,14 @@ func TestDefaultSessionPersistsCohortAndResumesReviewerOnLiveRerun(t *testing.T) FakeAdapter: &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true}, reviewerSessionID: "reviewer-dry", } - liveAdapter.Queue(fakeLLMResult("reviewer-live", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) - liveAdapter.Queue(fakeLLMResult("rollup-live", rollupJSON("comment", []string{"live-finding-1"}), 30, 6)) + reviewerLive := fakeLLMResult("reviewer-live", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4) + reviewerCost := 1.25 + reviewerLive.Response.Usage.CostUSD = &reviewerCost + liveAdapter.Queue(reviewerLive) + rollupLive := fakeLLMResult("rollup-live", rollupJSON("comment", []string{"live-finding-1"}), 30, 6) + rollupCost := 2.0 + rollupLive.Response.Usage.CostUSD = &rollupCost + liveAdapter.Queue(rollupLive) liveResult, err := liveForTest(ctx, Options{ Provider: provider, @@ -3398,6 +3404,26 @@ func TestDefaultSessionPersistsCohortAndResumesReviewerOnLiveRerun(t *testing.T) if liveResult.NamedSessionCandidate == nil || liveResult.NamedSessionCandidate.Name != stored.Name { t.Fatalf("live candidate = %#v, want shared default key %q", liveResult.NamedSessionCandidate, stored.Name) } + if liveResult.Plan.Summary.Run.Adapter != "fake-llm" { + t.Fatalf("live summary adapter = %q, want resumed run adapter", liveResult.Plan.Summary.Run.Adapter) + } + var workstreamNames []string + for _, workstream := range liveResult.Plan.Summary.Run.Workstreams { + workstreamNames = append(workstreamNames, workstream.Name) + } + if want := []string{"harness:reviewer", "orchestrator-rollup"}; !reflect.DeepEqual(workstreamNames, want) { + t.Fatalf("live summary workstreams = %#v, want only executed workstreams %#v", workstreamNames, want) + } + totals := liveResult.Plan.Summary.Totals + if totals.TokensIn == nil || *totals.TokensIn != 50 || totals.TokensOut == nil || *totals.TokensOut != 10 { + t.Fatalf("live summary token totals = %#v, want 50 in / 10 out", totals) + } + if totals.CostUSD == nil || *totals.CostUSD != 3.25 { + t.Fatalf("live summary cost total = %#v, want 3.25", totals) + } + if totals.ComputeDurationMS == nil || *totals.ComputeDurationMS != 246 { + t.Fatalf("live summary compute duration = %#v, want 246ms", totals) + } } func TestFreshSessionSkipsStoredDefaultWithoutChangingItsKey(t *testing.T) { From a6a44d1d566e54556136827af8b61fdf1a1286da Mon Sep 17 00:00:00 2001 From: Aaron Wong <6979793+zzwong@users.noreply.github.com> Date: Tue, 4 Aug 2026 01:54:50 -0400 Subject: [PATCH 04/20] fix: publish reviewer diff before execution --- internal/pipeline/artifacts.go | 15 ++-- internal/pipeline/pipeline.go | 5 +- .../pipeline/workbench_integration_test.go | 70 +++++++++++++++++++ 3 files changed, 85 insertions(+), 5 deletions(-) diff --git a/internal/pipeline/artifacts.go b/internal/pipeline/artifacts.go index 31df9598..901c37a4 100644 --- a/internal/pipeline/artifacts.go +++ b/internal/pipeline/artifacts.go @@ -13,16 +13,23 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/review" ) -func writeArtifacts(paths ArtifactPaths, rawDiff string, patches []FilePatch, catalog agents.Catalog, selection llm.Selection, findings []review.Finding, rollup string, reviewerRuntime map[string]reviewerRuntimeResolution) error { +func writeReviewerInputArtifacts(paths ArtifactPaths, rawDiff string) error { if err := os.MkdirAll(paths.Dir, 0o700); err != nil { return fmt.Errorf("pipeline: create artifact dir: %w", err) } - if err := os.MkdirAll(paths.SlicesDir, 0o700); err != nil { - return fmt.Errorf("pipeline: create slices dir: %w", err) - } if err := fsatomic.WriteFileAtomic(paths.DiffPatch, []byte(rawDiff), 0o600); err != nil { return fmt.Errorf("pipeline: write diff: %w", err) } + return nil +} + +func writeArtifacts(paths ArtifactPaths, patches []FilePatch, catalog agents.Catalog, selection llm.Selection, findings []review.Finding, rollup string, reviewerRuntime map[string]reviewerRuntimeResolution) error { + if err := os.MkdirAll(paths.Dir, 0o700); err != nil { + return fmt.Errorf("pipeline: create artifact dir: %w", err) + } + if err := os.MkdirAll(paths.SlicesDir, 0o700); err != nil { + return fmt.Errorf("pipeline: create slices dir: %w", err) + } sourceJSON, err := json.MarshalIndent(agentSourcesArtifactFromCatalog(catalog, reviewerRuntime), "", " ") if err != nil { return err diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 2617236d..ffdd8cb5 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -693,6 +693,9 @@ func execute(ctx context.Context, opts Options, req Request, mode executionMode) }); err != nil { return Result{}, pipelineTaskError(err) } + if err := writeReviewerInputArtifacts(prepared.artifacts, prepared.rawDiff); err != nil { + return Result{}, err + } findingSessions, blockingFailure, err := executePlanPhases(ctx, opts, req, mode, run, prepared, now, maxAgents, maxConcurrency, &result) if err != nil { @@ -952,7 +955,7 @@ func persistExecutionResult(ctx context.Context, opts Options, req Request, run return err } result.PlannedActions = plannedActions - return writeArtifacts(prepared.artifacts, prepared.rawDiff, prepared.parsed.Patches, result.Catalog, result.Selection, result.Findings, result.Plan.RollupMarkdown, reviewerRuntimeArtifact(req, prepared.catalog, result.Selection, result.reviewerFastDelivered, prepared.fastRequested, prepared.fastIgnored)) + return writeArtifacts(prepared.artifacts, prepared.parsed.Patches, result.Catalog, result.Selection, result.Findings, result.Plan.RollupMarkdown, reviewerRuntimeArtifact(req, prepared.catalog, result.Selection, result.reviewerFastDelivered, prepared.fastRequested, prepared.fastIgnored)) } func findIncompleteDryRun(ctx context.Context, store Store, req Request, pr gitprovider.PR) (ledger.Run, bool, error) { diff --git a/internal/pipeline/workbench_integration_test.go b/internal/pipeline/workbench_integration_test.go index a788a706..4ed95144 100644 --- a/internal/pipeline/workbench_integration_test.go +++ b/internal/pipeline/workbench_integration_test.go @@ -2,6 +2,7 @@ package pipeline import ( "context" + "fmt" "os" "path/filepath" "reflect" @@ -110,6 +111,57 @@ func TestDryRunPreparesWorkbenchInAllocatedRunArtifacts(t *testing.T) { } } +func TestDryRunPublishesPinnedDiffBeforeReviewerStart(t *testing.T) { + ctx := context.Background() + invocationDir := t.TempDir() + gitCommandMustSucceed(t, invocationDir, "init") + t.Chdir(invocationDir) + fixture := newWorkbenchGitFixture(t) + provider, req := dryRunHarness(t) + provider.pr = fixture.pr + addRepoAgentFixture(provider) + pinnedDiff := "diff --git a/main.go b/main.go\nindex 1111111..2222222 100644\n--- a/main.go\n+++ b/main.go\n@@ -1,2 +1,2 @@\n package main\n-var changed = false\n+var changed = true\n" + provider.diff = gitprovider.UnifiedDiff{Raw: pinnedDiff} + req.PRRef = fixture.pr.Ref + req.PRURL = fixture.pr.URL + store := openPipelineStore(t) + defer closeStore(t, store) + base := &llm.FakeAdapter{NameValue: "fake-llm"} + adapter := &diffReadingAdapter{FakeAdapter: base} + adapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "main.go"), 10, 2)) + adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) + adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) + + result, err := dryRunForTest(ctx, Options{ + Provider: provider, + Adapter: adapter, + Store: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + GitCommand: workbenchGitCommandForTest(req.PRRef, fixture.repoDir), + ResolveRepoRoot: func(context.Context) (string, error) { + return invocationDir, nil + }, + NewRunID: func() string { return "run-pinned-diff" }, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, + }, req) + if err != nil { + t.Fatalf("DryRun: %v", err) + } + if adapter.diffReadErr != nil { + t.Fatalf("reviewer read diff: %v", adapter.diffReadErr) + } + if len(adapter.diffContents) != 1 || adapter.diffContents[0] != pinnedDiff { + t.Fatalf("reviewer diff contents = %#v, want pinned diff", adapter.diffContents) + } + if got, err := os.ReadFile(result.Artifacts.DiffPatch); err != nil || string(got) != pinnedDiff { + t.Fatalf("retained diff = %q, %v; want pinned diff", got, err) + } +} + func TestRunReviewerRejectsStaleWorkbenchMetadata(t *testing.T) { ctx := context.Background() store := openPipelineStore(t) @@ -200,6 +252,24 @@ func TestRunReviewerRejectsStaleWorkbenchMetadata(t *testing.T) { } } +type diffReadingAdapter struct { + *llm.FakeAdapter + diffContents []string + diffReadErr error +} + +func (a *diffReadingAdapter) Start(ctx context.Context, req llm.Request) (llm.Stream, error) { + if req.ReviewerWorkspace != nil { + data, err := os.ReadFile(req.ReviewerWorkspace.DiffPath) + if err != nil { + a.diffReadErr = fmt.Errorf("read reviewer diff: %w", err) + } else { + a.diffContents = append(a.diffContents, string(data)) + } + } + return a.FakeAdapter.Start(ctx, req) +} + func TestDryRunReviewerWorkspaceAvoidsContextStuffingBudgetFailures(t *testing.T) { tests := []struct { name string From e10b30b17f8352badc91057b8491b44b8ee04488 Mon Sep 17 00:00:00 2001 From: Aaron Wong <6979793+zzwong@users.noreply.github.com> Date: Tue, 4 Aug 2026 01:56:42 -0400 Subject: [PATCH 05/20] fix: render reviewer coverage empties accurately --- internal/reviewplan/summary.go | 26 +++++++++++++++-- internal/reviewplan/summary_test.go | 45 +++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/internal/reviewplan/summary.go b/internal/reviewplan/summary.go index 84a31332..2f95427f 100644 --- a/internal/reviewplan/summary.go +++ b/internal/reviewplan/summary.go @@ -272,19 +272,41 @@ func writeReviewerCoverageDiagnostics(out *strings.Builder, coverage []ReviewerC fmt.Fprintf(out, "| %s | %s | %s | %s | %s |\n", escapeCell(entry.AgentID), escapeCell(entry.Status), - escapeCell(orUnavailable(strings.Join(entry.InspectedFiles, ", "))), - escapeCell(orUnavailable(strings.Join(entry.SkippedFiles, ", "))), + escapeCell(coverageCollectionCell(entry, entry.InspectedFiles)), + escapeCell(coverageCollectionCell(entry, entry.SkippedFiles)), escapeCell(coverageAnnotationCell(entry)), ) } out.WriteString("\n") } +func coverageCollectionCell(entry ReviewerCoverageSummary, values []string) string { + if len(values) > 0 { + return strings.Join(values, ", ") + } + if coverageResultProduced(entry.Status) { + return "none" + } + return unavailableValue +} + +func coverageResultProduced(status string) bool { + switch strings.TrimSpace(status) { + case "complete_broad", "complete_constrained", "incomplete_skipped": + return true + default: + return false + } +} + func coverageAnnotationCell(entry ReviewerCoverageSummary) string { parts := append([]string(nil), entry.Constraints...) if strings.TrimSpace(entry.Diagnostic) != "" { parts = append(parts, entry.Diagnostic) } + if len(parts) == 0 && coverageResultProduced(entry.Status) { + return "none" + } return orUnavailable(strings.Join(parts, "; ")) } diff --git a/internal/reviewplan/summary_test.go b/internal/reviewplan/summary_test.go index 634e4df7..81c940e3 100644 --- a/internal/reviewplan/summary_test.go +++ b/internal/reviewplan/summary_test.go @@ -364,6 +364,51 @@ func TestRollupSummaryRendering(t *testing.T) { } }) + t.Run("coverage cells distinguish known empty collections from missing results", func(t *testing.T) { + req := baseRequest() + req.Findings = nil + req.Rollup = review.Rollup{ + ReviewEvent: review.ReviewEventApprove, + ReviewEventRationale: "no findings", + OrderedFindings: nil, + } + req.RunSummary = RunSummary{ + SelectedReviewers: []string{"complete-broad", "complete-constrained", "failed"}, + ReviewerCoverage: []ReviewerCoverageSummary{ + { + AgentID: "complete-broad", + Status: "complete_broad", + InspectedFiles: []string{"main.go"}, + }, + { + AgentID: "complete-constrained", + Status: "complete_constrained", + SkippedFiles: []string{"main.go"}, + Constraints: []string{"read-only tools"}, + }, + { + AgentID: "failed", + Status: "incomplete_failed", + Scope: []string{"main.go"}, + }, + }, + } + plan, err := Build(req) + if err != nil { + t.Fatalf("Build: %v", err) + } + md := plan.RollupMarkdown + for _, want := range []string{ + "| complete-broad | complete_broad | main.go | none | none |", + "| complete-constrained | complete_constrained | none | main.go | read-only tools |", + "| failed | incomplete_failed | unavailable | unavailable | unavailable |", + } { + if !strings.Contains(md, want) { + t.Fatalf("coverage row missing %q:\n%s", want, md) + } + } + }) + t.Run("unknown reviewer coverage status force comment", func(t *testing.T) { req := baseRequest() req.Findings = nil From bc96a2b2436e3597435ea33da7e051af29fa044b Mon Sep 17 00:00:00 2001 From: Aaron Wong <6979793+zzwong@users.noreply.github.com> Date: Tue, 4 Aug 2026 02:09:13 -0400 Subject: [PATCH 06/20] fix: preserve precise reviewer tool failures --- internal/llm/contracts.go | 16 ++++- internal/llm/contracts_test.go | 20 +++++- internal/llmadapters/pi_rpc.go | 67 ++++++++++++++++---- internal/llmadapters/pi_rpc_test.go | 3 + internal/pipeline/pipeline.go | 98 ++++++++++++++++++++++++++++- internal/pipeline/pipeline_test.go | 31 +++++++++ internal/reviewplan/summary_test.go | 6 +- 7 files changed, 222 insertions(+), 19 deletions(-) diff --git a/internal/llm/contracts.go b/internal/llm/contracts.go index 62c4211b..94f400b5 100644 --- a/internal/llm/contracts.go +++ b/internal/llm/contracts.go @@ -346,9 +346,7 @@ func decodeCoverageStrings(name string, values []string) ([]string, error) { if strings.TrimSpace(value) == "" { return nil, fmt.Errorf("llm: %s entries must be non-empty", name) } - if utf8.RuneCountInString(value) > defaultMaxCoverageConstraintRunes { - return nil, fmt.Errorf("llm: %s entry length out of bounds", name) - } + value = truncateCoverageConstraint(value) if seen[value] { return nil, fmt.Errorf("llm: duplicate %s entry %q", name, value) } @@ -358,6 +356,18 @@ func decodeCoverageStrings(name string, values []string) ([]string, error) { return out, nil } +// truncateCoverageConstraint keeps a model-authored coverage diagnostic +// structurally valid while preserving its leading cause and marking loss. +func truncateCoverageConstraint(value string) string { + if utf8.RuneCountInString(value) <= defaultMaxCoverageConstraintRunes { + return value + } + const marker = "..." + keep := defaultMaxCoverageConstraintRunes - len(marker) + runes := []rune(value) + return string(runes[:keep]) + marker +} + func validateCoverageFileDisjoint(inspected, skipped []string) error { seen := map[string]bool{} for _, file := range inspected { diff --git a/internal/llm/contracts_test.go b/internal/llm/contracts_test.go index 8bfcfaf0..e09a38cd 100644 --- a/internal/llm/contracts_test.go +++ b/internal/llm/contracts_test.go @@ -155,7 +155,6 @@ func TestDecodeFindings(t *testing.T) { assertFindingsError(t, baseOpts, `{"schema_version":1,"agent_id":"agent-1","inspected_files":["main.go"],"skipped_files":["main.go"],"findings":[]}`, "both inspected and skipped") assertFindingsError(t, baseOpts, `{"schema_version":1,"agent_id":"agent-1","inspected_files":["main.go"],"constraints":[" "],"findings":[]}`, "constraints") assertFindingsError(t, baseOpts, `{"schema_version":1,"agent_id":"agent-1","inspected_files":["main.go"],"constraints":["one","two","three","four","five","six","seven","eight","nine","ten","eleven"],"findings":[]}`, "constraints cap exceeded") - assertFindingsError(t, baseOpts, `{"schema_version":1,"agent_id":"agent-1","inspected_files":["main.go"],"constraints":["`+strings.Repeat("x", defaultMaxCoverageConstraintRunes+1)+`"],"findings":[]}`, "constraints entry length") assertFindingsError(t, baseOpts, findingsFixture(`"schema_version":2,"agent_id":"agent-1","findings":[]`), "schema_version") assertFindingsError(t, baseOpts, findingsFixture(`"schema_version":1,"agent_id":"agent-1","findings":[],"extra":true`), "unknown field") assertFindingsError(t, baseOpts, findingsFixture(`"schema_version":1,"agent_id":"missing","findings":[]`), "unknown findings agent") @@ -179,6 +178,25 @@ func TestDecodeFindings(t *testing.T) { assertFindingsError(t, FindingsOptions{KnownAgents: baseOpts.KnownAgents, ChangedFiles: baseOpts.ChangedFiles, NewFindingID: newIDQueue("dup", "dup").next}, findingsFixture(`"schema_version":1,"agent_id":"agent-1","findings":[{"severity":"major","file_path":"main.go","anchor":{"kind":"file"},"body":"body"},{"severity":"minor","file_path":"main.go","anchor":{"kind":"file"},"body":"body"}]`), "duplicate") } +func TestDecodeFindingsBoundsOversizedConstraintWithoutRejecting(t *testing.T) { + longConstraint := strings.Repeat("x", defaultMaxCoverageConstraintRunes+25) + got, err := DecodeFindings([]byte(`{"schema_version":1,"agent_id":"agent-1","inspected_files":["main.go"],"constraints":["`+longConstraint+`"],"findings":[]}`), FindingsOptions{ + KnownAgents: map[string]bool{"agent-1": true}, + ChangedFiles: map[string]bool{"main.go": true}, + NewFindingID: newIDQueue("f-1").next, + }) + if err != nil { + t.Fatalf("DecodeFindings: %v", err) + } + if len(got.Constraints) != 1 { + t.Fatalf("constraints = %#v, want one bounded constraint", got.Constraints) + } + want := strings.Repeat("x", defaultMaxCoverageConstraintRunes-3) + "..." + if got.Constraints[0] != want { + t.Fatalf("constraint = %q, want deterministic truncation %q", got.Constraints[0], want) + } +} + func findingsFixture(fields string) string { return `{"inspected_files":["main.go"],` + fields + `}` } diff --git a/internal/llmadapters/pi_rpc.go b/internal/llmadapters/pi_rpc.go index a0df00a7..931c2e74 100644 --- a/internal/llmadapters/pi_rpc.go +++ b/internal/llmadapters/pi_rpc.go @@ -19,14 +19,15 @@ import ( ) const ( - piRPCPromptID = "prompt-1" - piRPCSystemPrompt = "You are a strict JSON API for code review structured output. Return exactly one JSON object that matches the requested schema. Do not include markdown fences, prose, explanations, or leading/trailing text. The first byte of your final answer must be { and the last byte must be }." - piRPCReviewerSystemPrompt = piRPCSystemPrompt + " Inspect the disposable repository only through the CR-owned cr_read, cr_search, cr_list, and cr_diff tools. Invoke cr_diff before cr_read, cr_search, or cr_list so the review starts from the pinned change. If cr_diff fails, record that exact tool failure as a constraint before inspecting allowed head files. These tools are read-only; do not request shell, write, edit, or any other tool." - piRPCReviewerToolTimeout = 15 * time.Second - piRPCReviewerToolNames = "cr_read,cr_search,cr_list,cr_diff" - piRPCToolEvidenceReserve = 256 - piRPCPreflightTimeout = 5 * time.Second - piRPCPreflightOutputBytes = 64 * 1024 + piRPCPromptID = "prompt-1" + piRPCSystemPrompt = "You are a strict JSON API for code review structured output. Return exactly one JSON object that matches the requested schema. Do not include markdown fences, prose, explanations, or leading/trailing text. The first byte of your final answer must be { and the last byte must be }." + piRPCReviewerSystemPrompt = piRPCSystemPrompt + " Inspect the disposable repository only through the CR-owned cr_read, cr_search, cr_list, and cr_diff tools. Invoke cr_diff before cr_read, cr_search, or cr_list so the review starts from the pinned change. If cr_diff fails, record that exact tool failure as a constraint before inspecting allowed head files. These tools are read-only; do not request shell, write, edit, or any other tool." + piRPCReviewerToolTimeout = 15 * time.Second + piRPCReviewerToolNames = "cr_read,cr_search,cr_list,cr_diff" + piRPCToolEvidenceReserve = 256 + piRPCToolDiagnosticMaxRunes = 128 + piRPCPreflightTimeout = 5 * time.Second + piRPCPreflightOutputBytes = 64 * 1024 ) // ErrPiRPCIncompatible reports that the installed Pi runtime cannot enforce @@ -421,6 +422,7 @@ type piRPCStream struct { diffToolStarted int diffToolCompleted int diffToolFailed int + diffToolError string } func (s *piRPCStream) run(ctx context.Context, cmd *exec.Cmd, stdout io.Reader, stderr io.Reader) { @@ -570,6 +572,9 @@ func (s *piRPCStream) observeReviewerToolEvent(event piRPCEvent) { if event.toolFailed { s.diffToolFailed++ } + if event.toolError != "" && s.diffToolError == "" { + s.diffToolError = event.toolError + } } func (s *piRPCStream) writeReviewerToolEvidence() { @@ -585,16 +590,49 @@ func (s *piRPCStream) writeReviewerToolEvidence() { case s.diffToolStarted > 0: status = "incomplete" } - evidence := []byte(fmt.Sprintf("codereview-pi-tool-evidence tool=cr_diff status=%s started=%d completed=%d failed=%d\n", status, s.diffToolStarted, s.diffToolCompleted, s.diffToolFailed)) + evidence := fmt.Sprintf("codereview-pi-tool-evidence tool=cr_diff status=%s started=%d completed=%d failed=%d", status, s.diffToolStarted, s.diffToolCompleted, s.diffToolFailed) + if status == "failed" { + evidence += fmt.Sprintf(" error=%q", boundPiRPCToolError(s.diffToolError)) + } + evidenceBytes := []byte(evidence + "\n") s.logLimitMu.Lock() defer s.logLimitMu.Unlock() - writeBytes := min(len(evidence), s.toolEvidenceBytesLeft) + writeBytes := min(len(evidenceBytes), s.toolEvidenceBytesLeft) if writeBytes > 0 { - s.WriteLog(evidence[:writeBytes]) + s.WriteLog(evidenceBytes[:writeBytes]) s.toolEvidenceBytesLeft -= writeBytes } } +func boundPiRPCToolError(value string) string { + value = strings.Join(strings.Fields(strings.TrimSpace(value)), " ") + runes := []rune(value) + if len(runes) <= piRPCToolDiagnosticMaxRunes { + return value + } + return string(runes[:piRPCToolDiagnosticMaxRunes-3]) + "..." +} + +func piRPCToolError(raw json.RawMessage) string { + var result map[string]json.RawMessage + if err := json.Unmarshal(raw, &result); err != nil { + return "" + } + if message := firstRawString(result, "error", "message"); message != "" { + return message + } + var content []map[string]json.RawMessage + if err := json.Unmarshal(result["content"], &content); err != nil { + return "" + } + for _, block := range content { + if message := firstRawString(block, "text", "content"); message != "" { + return message + } + } + return "" +} + func normalizePiRPCLogLine(line []byte) []byte { logLine := append([]byte(nil), line...) if len(logLine) == 0 { @@ -666,6 +704,7 @@ type piRPCEvent struct { toolStarted bool toolCompleted bool toolFailed bool + toolError string responseFailure string agentEnd bool } @@ -696,6 +735,12 @@ func parsePiRPCEvent(line []byte) (piRPCEvent, error) { if err := json.Unmarshal(raw["result"], &resultRaw); err == nil && rawBool(resultRaw, "isError") { event.toolFailed = true } + if event.toolFailed { + event.toolError = piRPCToolError(raw["result"]) + if event.toolError == "" { + event.toolError = "tool execution failed" + } + } } } if id := firstRawString(raw, "sessionId", "session_id"); id != "" { diff --git a/internal/llmadapters/pi_rpc_test.go b/internal/llmadapters/pi_rpc_test.go index b55467e4..45820248 100644 --- a/internal/llmadapters/pi_rpc_test.go +++ b/internal/llmadapters/pi_rpc_test.go @@ -320,6 +320,9 @@ func TestPiRPCReviewerLogCapPreservesDiffFailureEvidence(t *testing.T) { if !strings.Contains(string(logged), "codereview-pi-tool-evidence tool=cr_diff status=failed started=1 completed=1 failed=1") { t.Fatalf("reviewer log = %q, want bounded failed-invocation evidence", logged) } + if !strings.Contains(string(logged), `error="fixed diff unavailable"`) { + t.Fatalf("reviewer log = %q, want precise cr_diff failure", logged) + } } func TestPiRPCReviewerExtensionLoadsInInstalledPi(t *testing.T) { diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index ffdd8cb5..8e61483a 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -11,6 +11,7 @@ import ( "regexp" "slices" "sort" + "strconv" "strings" "sync" "time" @@ -307,8 +308,12 @@ const ( reviewerCoverageIncompleteSkipped = "incomplete_skipped" reviewerCoverageIncompleteFailed = "incomplete_failed" reviewerCoverageIncompleteUnassigned = "incomplete_unassigned" + reviewerCoverageIncompleteTool = "incomplete_tool" + reviewerToolDiagnosticMaxRunes = 300 ) +var reviewerDiagnosticPathRE = regexp.MustCompile("([A-Za-z]:[\\\\/]|/)[^[:space:]]+") + // SelectionSession describes the single LLM turn used for selection-only execution. type SelectionSession struct { ProviderReportedSessionID string @@ -2016,6 +2021,9 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p } return llm.Findings{}, sessionDraft{}, ledger.Session{}, nil, err } + if diagnostic := reviewerToolDiagnostic(logPath, artifacts.Dir); diagnostic != "" { + findings = appendReviewerToolDiagnostic(findings, diagnostic) + } return findings, session, ledgerSession, nil, nil } @@ -2108,6 +2116,77 @@ func sanitizeTaskErrorForMarkdown(err error) string { return value } +func reviewerToolDiagnostic(logPath, artifactDir string) string { + data, err := os.ReadFile(logPath) // #nosec G304 -- logPath is the run-owned reviewer log. + if err != nil { + return "" + } + for _, line := range strings.Split(string(data), "\n") { + if !strings.Contains(line, "codereview-pi-tool-evidence") || reviewerEvidenceField(line, "status") != "failed" { + continue + } + detail := reviewerEvidenceField(line, "error") + if detail == "" { + detail = "tool execution failed" + } + return normalizeReviewerToolDiagnostic("cr_diff: "+detail, artifactDir) + } + return "" +} + +func reviewerEvidenceField(line, field string) string { + marker := field + "=" + index := strings.Index(line, marker) + if index < 0 { + return "" + } + value := strings.TrimSpace(line[index+len(marker):]) + if value == "" { + return "" + } + if strings.HasPrefix(value, "\"") { + if decoded, err := strconv.Unquote(value); err == nil { + return decoded + } + return "" + } + fields := strings.Fields(value) + if len(fields) == 0 { + return "" + } + return fields[0] +} + +func normalizeReviewerToolDiagnostic(value, artifactDir string) string { + value = strings.Join(strings.Fields(strings.TrimSpace(value)), " ") + if clean := filepath.Clean(strings.TrimSpace(artifactDir)); clean != "." && clean != "" { + value = strings.ReplaceAll(value, clean, "") + } + value = reviewerDiagnosticPathRE.ReplaceAllString(value, "") + value = strings.ReplaceAll(value, "