diff --git a/BENCHMARKING.md b/BENCHMARKING.md index 70cc22fd..a49c5472 100644 --- a/BENCHMARKING.md +++ b/BENCHMARKING.md @@ -402,6 +402,9 @@ The MVP measures rather than grades. Current benchmark summary artifacts include - provider-reported usage from child review or selector agent logs when available, including LLM call count, turns, tool activity, tokens, cost, and per-phase agent log summaries; +- CR-owned Pi `cr_diff` evidence counts when present, under `usage.pi_diff`: + `succeeded`, `failed`, `not_invoked`, and `incomplete` records aggregated + across the run's agent logs; - warning strings when child review output cannot be parsed or selector runs fail after partial execution; - benchmark artifact paths. @@ -430,6 +433,7 @@ missing telemetry. | Cache read | Provider or adapter reported cache-read tokens, when present in child review agent logs. | | Cache create | Provider or adapter reported cache-write/create tokens, when present in child review agent logs. | | Cost | Provider or adapter reported cost only. Do not use baked-in benchmark price tables for v1. | +| Pinned diff evidence | CR-owned `codereview-pi-tool-evidence tool=cr_diff` records only. `succeeded`, `failed`, `not_invoked`, and `incomplete` are counts per run; failed, not-invoked, or incomplete evidence is degraded tool execution and does not redefine the general benchmark quality grade. A missing `pi_diff` object means no qualifying evidence was observed, not zero successful calls. | | Selected agents | Selector-only benchmarks record selected reviewer IDs and files directly in suite summaries, JSONL, and comparison artifacts. Full-review benchmarks still rely on review artifacts and logs for downstream selection analysis. | | Observed SHAs | Record when available from review artifacts or downstream analysis. Expected SHAs in cases are comparison metadata. | | Anchor metrics | Computed by `comparison.json` and `comparison.md` when cases define anchors. They are placement-only. | diff --git a/README.md b/README.md index ad248767..ef4cd887 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 when CR's compatibility +preflight confirms the reviewer controls it 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 \ @@ -1130,9 +1135,9 @@ Review selection and execution flags: | `--selection-model ` | Exact provider model ID passthrough for the selection stage only. Bypasses the default medium-tier selection model resolution. Requires `--dry-run` or `--no-post`. | | `--selection-effort ` | Override selection-stage effort only with `low`, `medium`, or `high`. Requires `--dry-run` or `--no-post`. | | `--selection-prompt ` | Load selection-stage instruction text from a file while preserving the structured JSON selection protocol. Requires `--dry-run` or `--no-post`. | -| `--reviewer-model ` | Exact provider model ID passthrough for reviewer stages only. Bypasses reviewer agent `model_tier`, `model_id`, and profile model-map resolution. Requires `--dry-run` or `--no-post`. | +| `--reviewer-model ` | Exact provider model ID passthrough for reviewer stages only. Bypasses reviewer agent `model_tier`, `model_id`, and profile model-map resolution. Available for dry-run, no-post, and live reviews. | | `--reviewer-model-tier ` | Override the reviewer baseline tier only with `small`, `medium`, or `large`. This still respects higher agent `model_tier` floors. Requires `--dry-run` or `--no-post`. | -| `--reviewer-effort ` | Override reviewer-stage effort only with `low`, `medium`, or `high`. Requires `--dry-run` or `--no-post`. | +| `--reviewer-effort ` | Override reviewer-stage effort only with `low`, `medium`, or `high`. Available for dry-run, no-post, and live reviews. | | `--review-base-sha ` | Review this base commit SHA instead of the PR's current base SHA. Requires `--review-head-sha` and `--dry-run` or `--no-post`. | | `--review-head-sha ` | Review this head commit SHA instead of the PR's current head SHA. Requires `--review-base-sha` and `--dry-run` or `--no-post`. | | `--session ` | Override the PR's default orchestrator session with a named live-review session. Reviewer cohorts remain PR-scoped. Not allowed with `--dry-run`, `--no-post`, or `--retry-posts`. | 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..3c08b2a0 100644 --- a/docs/checkout-native-review-contract.md +++ b/docs/checkout-native-review-contract.md @@ -292,6 +292,22 @@ 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. +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 +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. @@ -322,6 +338,7 @@ The coverage status values are: - `complete_constrained`: a reviewer with `allowed_files` covered that narrowed assignment - `incomplete_skipped`: assigned files were skipped or not reported as inspected +- `incomplete_tool`: the fixed-diff tool was not invoked, did not complete, or failed - `incomplete_failed`: an isolated reviewer failure or missing reviewer result prevented coverage - `incomplete_unassigned`: changed files were not assigned to any selected diff --git a/internal/benchmark/metrics.go b/internal/benchmark/metrics.go index e9e68ee3..957f65c0 100644 --- a/internal/benchmark/metrics.go +++ b/internal/benchmark/metrics.go @@ -19,9 +19,38 @@ type RunMetrics struct { ToolResults int `json:"tool_results"` Tokens TokenMetrics `json:"tokens"` Cost CostMetrics `json:"cost"` + PiDiff *PiDiffMetrics `json:"pi_diff,omitempty"` Phases []PhaseMetrics `json:"phases,omitempty"` } +// PiDiffMetrics counts CR-owned cr_diff evidence records in a run. +type PiDiffMetrics struct { + Succeeded int `json:"succeeded"` + Failed int `json:"failed"` + NotInvoked int `json:"not_invoked"` + Incomplete int `json:"incomplete,omitempty"` +} + +func (m *PiDiffMetrics) add(other PiDiffMetrics) { + m.Succeeded += other.Succeeded + m.Failed += other.Failed + m.NotInvoked += other.NotInvoked + m.Incomplete += other.Incomplete +} + +func (m *PiDiffMetrics) addStatus(status string) { + switch status { + case "succeeded": + m.Succeeded++ + case "failed": + m.Failed++ + case "not_invoked": + m.NotInvoked++ + default: + m.Incomplete++ + } +} + // TokenMetrics records provider token usage. type TokenMetrics struct { Available bool `json:"available"` @@ -44,23 +73,24 @@ type CostMetrics struct { // PhaseMetrics summarizes one agent log. type PhaseMetrics struct { - Name string `json:"name"` - Role string `json:"role,omitempty"` - LogPath string `json:"log_path"` - Provider string `json:"provider,omitempty"` - Model string `json:"model,omitempty"` - StopReason string `json:"stop_reason,omitempty"` - LLMCalls int `json:"llm_calls"` - Turns int `json:"turns"` - ToolCalls int `json:"tool_calls"` - ToolResults int `json:"tool_results"` - Tokens TokenMetrics `json:"tokens"` - Cost CostMetrics `json:"cost"` + Name string `json:"name"` + Role string `json:"role,omitempty"` + LogPath string `json:"log_path"` + Provider string `json:"provider,omitempty"` + Model string `json:"model,omitempty"` + StopReason string `json:"stop_reason,omitempty"` + LLMCalls int `json:"llm_calls"` + Turns int `json:"turns"` + ToolCalls int `json:"tool_calls"` + ToolResults int `json:"tool_results"` + Tokens TokenMetrics `json:"tokens"` + Cost CostMetrics `json:"cost"` + PiDiff *PiDiffMetrics `json:"pi_diff,omitempty"` } // HasData reports whether metrics contain provider usage or activity. func (m RunMetrics) HasData() bool { - return len(m.Phases) > 0 || m.Turns > 0 || m.LLMCalls > 0 || m.ToolCalls > 0 || m.ToolResults > 0 || m.Tokens.Available || m.Cost.Available || m.Tokens.TotalTokens > 0 || m.Cost.Total > 0 + return len(m.Phases) > 0 || m.PiDiff != nil || m.Turns > 0 || m.LLMCalls > 0 || m.ToolCalls > 0 || m.ToolResults > 0 || m.Tokens.Available || m.Cost.Available || m.Tokens.TotalTokens > 0 || m.Cost.Total > 0 } // HasTokenUsage reports whether provider token telemetry was captured. @@ -81,6 +111,12 @@ func (m *RunMetrics) Add(other RunMetrics) { m.ToolResults += other.ToolResults m.Tokens.add(other.Tokens) m.Cost.add(other.Cost) + if other.PiDiff != nil { + if m.PiDiff == nil { + m.PiDiff = &PiDiffMetrics{} + } + m.PiDiff.add(*other.PiDiff) + } } // ExtractRunMetrics reads agent JSONL logs from a review artifact directory. @@ -110,6 +146,12 @@ func ExtractRunMetrics(artifactPath string) (RunMetrics, error) { metrics.LLMCalls += phase.LLMCalls metrics.Tokens.add(phase.Tokens) metrics.Cost.add(phase.Cost) + if phase.PiDiff != nil { + if metrics.PiDiff == nil { + metrics.PiDiff = &PiDiffMetrics{} + } + metrics.PiDiff.add(*phase.PiDiff) + } } return metrics, nil } @@ -130,6 +172,15 @@ func extractPhaseMetrics(logPath string) (PhaseMetrics, error) { for scanner.Scan() { var event map[string]any if err := json.Unmarshal(scanner.Bytes(), &event); err != nil { + if status, handled := parsePiDiffEvidence(scanner.Bytes()); handled { + if status != "" { + if phase.PiDiff == nil { + phase.PiDiff = &PiDiffMetrics{} + } + phase.PiDiff.addStatus(status) + } + continue + } return PhaseMetrics{}, fmt.Errorf("%s: %w", logPath, err) } accumulateEvent(&phase, event, partialFallbacks) @@ -378,7 +429,7 @@ func (m *CostMetrics) add(other CostMetrics) { } func phaseHasData(phase PhaseMetrics) bool { - return phase.LLMCalls > 0 || phase.Turns > 0 || phase.ToolCalls > 0 || phase.ToolResults > 0 || phase.Tokens.Available || phase.Cost.Available || phase.Tokens.TotalTokens > 0 || phase.Cost.Total > 0 + return phase.PiDiff != nil || phase.LLMCalls > 0 || phase.Turns > 0 || phase.ToolCalls > 0 || phase.ToolResults > 0 || phase.Tokens.Available || phase.Cost.Available || phase.Tokens.TotalTokens > 0 || phase.Cost.Total > 0 } func phaseName(logPath string) string { @@ -403,6 +454,38 @@ func phaseRole(name string) string { } } +const piDiffEvidencePrefix = "codereview-pi-tool-evidence" + +func parsePiDiffEvidence(line []byte) (string, bool) { + fields := strings.Fields(strings.TrimSpace(string(line))) + if len(fields) == 0 || fields[0] != piDiffEvidencePrefix { + return "", false + } + tool := "" + status := "" + for _, field := range fields[1:] { + key, value, ok := strings.Cut(field, "=") + if !ok { + continue + } + switch key { + case "tool": + tool = value + case "status": + status = value + } + } + if tool != "cr_diff" { + return "", true + } + switch status { + case "succeeded", "failed", "not_invoked", "incomplete": + return status, true + default: + return "incomplete", true + } +} + func stringValue(value any) string { text, _ := value.(string) return text diff --git a/internal/benchmark/metrics_test.go b/internal/benchmark/metrics_test.go index 608992d9..92847fe3 100644 --- a/internal/benchmark/metrics_test.go +++ b/internal/benchmark/metrics_test.go @@ -207,6 +207,37 @@ func TestExtractRunMetricsReturnsEmptyWhenLogsAreAbsent(t *testing.T) { } } +func TestExtractRunMetricsAggregatesPiDiffEvidence(t *testing.T) { + artifactPath := t.TempDir() + logDir := filepath.Join(artifactPath, "agent-logs") + if err := os.MkdirAll(logDir, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + writeLog(t, filepath.Join(logDir, "reviewer-success.jsonl"), `{"type":"turn_start"} +codereview-pi-tool-evidence tool=cr_diff status=succeeded started=1 completed=1 failed=0 +`) + writeLog(t, filepath.Join(logDir, "reviewer-failed.jsonl"), `{"type":"turn_start"} +codereview-pi-tool-evidence tool=cr_diff status=failed started=1 completed=1 failed=1 error="fixed diff unavailable" +`) + writeLog(t, filepath.Join(logDir, "reviewer-not-invoked.jsonl"), `{"type":"turn_start"} +codereview-pi-tool-evidence tool=cr_diff status=not_invoked started=0 completed=0 failed=0 +`) + + metrics, err := ExtractRunMetrics(artifactPath) + if err != nil { + t.Fatalf("ExtractRunMetrics: %v", err) + } + if metrics.PiDiff == nil { + t.Fatal("PiDiff = nil, want aggregated evidence") + } + if metrics.PiDiff.Succeeded != 1 || metrics.PiDiff.Failed != 1 || metrics.PiDiff.NotInvoked != 1 || metrics.PiDiff.Incomplete != 0 { + t.Fatalf("PiDiff = %#v, want one success, failure, and non-invocation", metrics.PiDiff) + } + if len(metrics.Phases) != 3 { + t.Fatalf("phases = %d, want evidence-only phases retained", len(metrics.Phases)) + } +} + func writeLog(t *testing.T, path string, body string) { t.Helper() if err := os.WriteFile(path, []byte(body), 0o600); err != nil { diff --git a/internal/cmd/benchmarkcmd/benchmarkcmd_test.go b/internal/cmd/benchmarkcmd/benchmarkcmd_test.go index 22455239..9823711e 100644 --- a/internal/cmd/benchmarkcmd/benchmarkcmd_test.go +++ b/internal/cmd/benchmarkcmd/benchmarkcmd_test.go @@ -809,6 +809,7 @@ func TestRenderReportMarkdownTreatsActivityOnlyUsageAsUnavailable(t *testing.T) Usage: &benchmark.RunMetrics{ Tokens: benchmark.TokenMetrics{Available: true}, Cost: benchmark.CostMetrics{Available: true}, + PiDiff: &benchmark.PiDiffMetrics{Succeeded: 1, Failed: 2, NotInvoked: 3, Incomplete: 4}, }, }, }, @@ -828,7 +829,7 @@ func TestRenderReportMarkdownTreatsActivityOnlyUsageAsUnavailable(t *testing.T) if !strings.Contains(report, "| `run1` | `candidate1` | `case1` | 0 | 1 | n/a | n/a |") { t.Fatalf("report missing activity-only n/a row:\n%s", report) } - if !strings.Contains(report, "| `run2` | `candidate2` | `case2` | 0 | 0 | 0 | $0.000000 |") { + if !strings.Contains(report, "| `run2` | `candidate2` | `case2` | 0 | 0 | 0 | $0.000000 | succeeded=1; failed=2; not_invoked=3; incomplete=4 |") { t.Fatalf("report missing explicit zero usage row:\n%s", report) } } diff --git a/internal/cmd/benchmarkcmd/run.go b/internal/cmd/benchmarkcmd/run.go index 8f946189..b20bb8d2 100644 --- a/internal/cmd/benchmarkcmd/run.go +++ b/internal/cmd/benchmarkcmd/run.go @@ -836,10 +836,10 @@ func renderReportMarkdown(summary benchmarkSuiteSummary) string { } var b strings.Builder writeReportHeader(&b, "Benchmark Report", summary) - b.WriteString("| Run | Candidate | Case | Exit | Findings | Tokens | Cost |\n") - b.WriteString("| --- | --- | --- | ---: | ---: | ---: | ---: |\n") + b.WriteString("| Run | Candidate | Case | Exit | Findings | Tokens | Cost | Pi diff |\n") + b.WriteString("| --- | --- | --- | ---: | ---: | ---: | ---: | --- |\n") for _, run := range summary.Runs { - fmt.Fprintf(&b, "| `%s` | `%s` | `%s` | %d | %d | %s | %s |\n", run.RunID, run.CandidateID, run.CaseID, run.ExitCode, run.FindingCount, usageTokensCell(run.Usage), usageCostCell(run.Usage)) + fmt.Fprintf(&b, "| `%s` | `%s` | `%s` | %d | %d | %s | %s | %s |\n", run.RunID, run.CandidateID, run.CaseID, run.ExitCode, run.FindingCount, usageTokensCell(run.Usage), usageCostCell(run.Usage), piDiffCell(run.Usage)) } return b.String() } @@ -873,6 +873,13 @@ func usageCostCell(usage *benchmark.RunMetrics) string { return fmt.Sprintf("$%.6f", usage.Cost.Total) } +func piDiffCell(usage *benchmark.RunMetrics) string { + if usage == nil || usage.PiDiff == nil { + return "n/a" + } + return fmt.Sprintf("succeeded=%d; failed=%d; not_invoked=%d; incomplete=%d", usage.PiDiff.Succeeded, usage.PiDiff.Failed, usage.PiDiff.NotInvoked, usage.PiDiff.Incomplete) +} + func durationMS(duration time.Duration) int64 { if duration < 0 { return 0 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/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index 2ecfcd42..28e57ce2 100644 --- a/internal/cmd/reviewcmd/reviewcmd.go +++ b/internal/cmd/reviewcmd/reviewcmd.go @@ -118,9 +118,9 @@ func RegisterWithFactory(rootCmd *cobra.Command, opts *root.Options, factory Run cmd.Flags().StringVar(&flags.selectionModel, "selection-model", "", "Override selection model for dry-run review") cmd.Flags().StringVar(&flags.selectionEffort, "selection-effort", "", "Override selection effort for dry-run review") cmd.Flags().StringVar(&flags.selectionPrompt, "selection-prompt", "", "Override selection instructions from a file for dry-run review") - cmd.Flags().StringVar(&flags.reviewerModel, "reviewer-model", "", "Override reviewer models for dry-run review") + cmd.Flags().StringVar(&flags.reviewerModel, "reviewer-model", "", "Override reviewer model for this review") cmd.Flags().StringVar(&flags.reviewerModelTier, "reviewer-model-tier", "", "Override reviewer baseline model tier for dry-run review") - cmd.Flags().StringVar(&flags.reviewerEffort, "reviewer-effort", "", "Override reviewer effort for dry-run review") + cmd.Flags().StringVar(&flags.reviewerEffort, "reviewer-effort", "", "Override reviewer effort for this review") cmd.Flags().StringVar(&flags.reviewBaseSHA, "review-base-sha", "", "Review this base commit SHA instead of the PR's current base SHA; requires --dry-run and --review-head-sha") cmd.Flags().StringVar(&flags.reviewHeadSHA, "review-head-sha", "", "Review this head commit SHA instead of the PR's current head SHA; requires --dry-run and --review-base-sha") cmd.Flags().IntVar(&flags.maxAgents, "max-agents", 0, "Maximum selected reviewer agents") @@ -182,9 +182,9 @@ func runReview(ctx context.Context, cmd *cobra.Command, opts *root.Options, fact if reviewerEffortChanged && !modelprefs.Effort(reviewerEffort).Valid() { return exitcode.Usage(fmt.Errorf("--reviewer-effort must be one of low, medium, high")) } - stageOverrideChanged := selectionModelChanged || selectionEffortChanged || selectionPromptChanged || reviewerModelChanged || reviewerModelTierChanged || reviewerEffortChanged - if stageOverrideChanged && !flags.dryRun { - return exitcode.Usage(fmt.Errorf("--selection-model, --selection-effort, --selection-prompt, --reviewer-model, --reviewer-model-tier, and --reviewer-effort require --dry-run or --no-post")) + dryRunOnlyOverrideChanged := selectionModelChanged || selectionEffortChanged || selectionPromptChanged || reviewerModelTierChanged + if dryRunOnlyOverrideChanged && !flags.dryRun { + return exitcode.Usage(fmt.Errorf("--selection-model, --selection-effort, --selection-prompt, and --reviewer-model-tier require --dry-run or --no-post")) } if reviewBaseChanged != reviewHeadChanged { return exitcode.Usage(fmt.Errorf("--review-base-sha and --review-head-sha must be set together")) diff --git a/internal/cmd/reviewcmd/reviewcmd_test.go b/internal/cmd/reviewcmd/reviewcmd_test.go index 21f0f485..e58cf870 100644 --- a/internal/cmd/reviewcmd/reviewcmd_test.go +++ b/internal/cmd/reviewcmd/reviewcmd_test.go @@ -580,9 +580,7 @@ func TestReviewLiveRejectsStageOverridesBeforeRuntimeFactory(t *testing.T) { {name: "selection model", args: []string{"--selection-model", "bench-model"}}, {name: "selection effort", args: []string{"--selection-effort", "high"}}, {name: "selection prompt", args: []string{"--selection-prompt", "selection.md"}}, - {name: "reviewer model", args: []string{"--reviewer-model", "bench-model"}}, {name: "reviewer model tier", args: []string{"--reviewer-model-tier", "medium"}}, - {name: "reviewer effort", args: []string{"--reviewer-effort", "high"}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -897,6 +895,27 @@ func TestReviewLiveCallsRunnerAndRendersText(t *testing.T) { } } +func TestReviewLivePassesReviewerModelAndEffortOverrides(t *testing.T) { + runner := &fakeRunner{liveResult: testLiveResult(false)} + cmd, _ := newTestCommand(t, testConfig(), fakeFactory(runner)) + + if err := root.Execute(cmd, []string{ + "review", "https://github.com/open-cli-collective/codereview-cli/pull/29", + "--rerun", + "--reviewer-model", "openai-codex/gpt-5.6-luna", + "--reviewer-effort", "high", + }); err != nil { + t.Fatalf("Execute: %v", err) + } + if len(runner.liveRequests) != 1 { + t.Fatalf("live runner calls = %d, want 1", len(runner.liveRequests)) + } + req := runner.liveRequests[0] + if req.ReviewerModelOverride != "openai-codex/gpt-5.6-luna" || req.ReviewerEffortOverride != "high" { + t.Fatalf("reviewer overrides = model:%q effort:%q, want Luna/high", req.ReviewerModelOverride, req.ReviewerEffortOverride) + } +} + func TestReviewFreshSessionPropagatesWithoutChangingRunMode(t *testing.T) { liveRunner := &fakeRunner{liveResult: testLiveResult(false)} liveCmd, _ := newTestCommand(t, testConfig(), fakeFactory(liveRunner)) diff --git a/internal/llm/adapter.go b/internal/llm/adapter.go index 11777860..8b506871 100644 --- a/internal/llm/adapter.go +++ b/internal/llm/adapter.go @@ -36,6 +36,7 @@ type Adapter interface { // workspace. type ReviewerWorkspaceMode string +// Diff tool evidence terminal states. const ( // ReviewerWorkspaceNone means the adapter cannot inspect a caller-provided workspace. ReviewerWorkspaceNone ReviewerWorkspaceMode = "none" @@ -59,6 +60,7 @@ type ReviewerWorkspaceCapable interface { type ReviewerWorkspaceRequest struct { RepoDir string ScratchDir string + DiffPath string Env []string AllowedFiles []string MaxToolOutputBytes int @@ -125,9 +127,31 @@ type Stream interface { // Response is the completed LLM result. type Response struct { - StructuredOutput []byte - Usage Usage - DurationMS int64 + StructuredOutput []byte + Usage Usage + DurationMS int64 + ReviewerToolEvidence *ReviewerToolEvidence +} + +// DiffToolStatus records the terminal state of the required reviewer diff tool. +type DiffToolStatus string + +const ( + // DiffToolStatusNotInvoked means the reviewer never invoked cr_diff. + DiffToolStatusNotInvoked DiffToolStatus = "not_invoked" + // DiffToolStatusIncomplete means the reviewer started but did not complete cr_diff. + DiffToolStatusIncomplete DiffToolStatus = "incomplete" + // DiffToolStatusSucceeded means the reviewer completed cr_diff without failure. + DiffToolStatusSucceeded DiffToolStatus = "succeeded" + // DiffToolStatusFailed means the reviewer observed a cr_diff failure. + DiffToolStatusFailed DiffToolStatus = "failed" +) + +// ReviewerToolEvidence records bounded, machine-significant reviewer tool state. +// It is absent when the adapter does not provide reviewer tool evidence. +type ReviewerToolEvidence struct { + DiffStatus DiffToolStatus `json:"diff_status"` + DiffDiagnostic string `json:"diff_diagnostic,omitempty"` } // Usage records nullable usage metrics. diff --git a/internal/llm/contracts_test.go b/internal/llm/contracts_test.go index a70c6599..d2e4f3a6 100644 --- a/internal/llm/contracts_test.go +++ b/internal/llm/contracts_test.go @@ -157,7 +157,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") diff --git a/internal/llmadapters/pi_rpc.go b/internal/llmadapters/pi_rpc.go index b319938f..36e1a323 100644 --- a/internal/llmadapters/pi_rpc.go +++ b/internal/llmadapters/pi_rpc.go @@ -9,17 +9,32 @@ 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. 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 + piRPCPreflightRegistration = "codereview-pi-reviewer-tools-registered cr_read,cr_search,cr_list,cr_diff" ) +// 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 +53,12 @@ type PiRPCAdapter struct { timeout time.Duration scratchDirFactory ScratchDirFactory fastModeModels []string + preflightMu sync.Mutex + preflightReady bool } var _ llm.Adapter = (*PiRPCAdapter)(nil) +var _ llm.ReviewerWorkspaceCapable = (*PiRPCAdapter)(nil) // NewPiRPCAdapter returns a Pi RPC subprocess adapter. func NewPiRPCAdapter(opts PiRPCOptions) *PiRPCAdapter { @@ -71,6 +89,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 +118,39 @@ 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 { + if err := a.ensureReviewerRuntime(ctx); err != nil { + return nil, err + } + } + 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 +160,243 @@ 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.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 } -func (a *PiRPCAdapter) buildArgs(req Request, _ string) ([]string, error) { +func (a *PiRPCAdapter) ensureReviewerRuntime(ctx context.Context) error { + a.preflightMu.Lock() + defer a.preflightMu.Unlock() + if a.preflightReady { + return nil + } + if err := a.preflightReviewerRuntime(ctx); err != nil { + return err + } + a.preflightReady = true + return nil +} + +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) }() + repoDir := filepath.Join(preflightDir, "repo") + scratchDir := filepath.Join(preflightDir, "scratch") + for _, dir := range []string{repoDir, scratchDir} { + if err := os.Mkdir(dir, 0o700); err != nil { + return fmt.Errorf("%w: create preflight %s directory: %w", ErrPiRPCIncompatible, filepath.Base(dir), err) + } + } + diffPath := filepath.Join(preflightDir, "diff.patch") + if err := os.WriteFile(diffPath, []byte("preflight diff\n"), 0o600); err != nil { + return fmt.Errorf("%w: write preflight diff: %w", ErrPiRPCIncompatible, err) + } + configPath := filepath.Join(scratchDir, "review-tools.json") + config, err := json.Marshal(map[string]any{ + "repo_dir": repoDir, + "diff_path": diffPath, + "allowed_files": []string{}, + "max_output_bytes": piRPCPreflightOutputBytes, + "timeout_ms": piRPCReviewerToolTimeout.Milliseconds(), + }) + if err != nil { + return fmt.Errorf("%w: marshal preflight tool config: %w", ErrPiRPCIncompatible, err) + } + if err := os.WriteFile(configPath, append(config, '\n'), 0o600); err != nil { + return fmt.Errorf("%w: write preflight tool config: %w", ErrPiRPCIncompatible, err) + } + executable, err := os.Executable() + if err != nil { + return fmt.Errorf("%w: locate CR executable: %w", ErrPiRPCIncompatible, err) + } + extensionPath := filepath.Join(scratchDir, "cr-review-tools.mjs") + extension := piRPCReviewerExtension(executable, configPath, repoDir, piRPCPreflightOutputBytes, piRPCReviewerToolTimeout, piRPCPreflightRegistration) + if err := os.WriteFile(extensionPath, []byte(extension), 0o600); err != nil { + return fmt.Errorf("%w: write preflight extension: %w", ErrPiRPCIncompatible, err) + } + args := append(append([]string(nil), a.commandArgsPrefix...), + "--mode", "rpc", + "--system-prompt", piRPCReviewerSystemPrompt, + "--no-builtin-tools", + "--tools", piRPCReviewerToolNames, + "--extension", extensionPath, + "--no-extensions", + "--no-skills", + "--no-prompt-templates", + "--no-themes", + "--no-context-files", + "--no-approve", + "--no-session", + ) + cmd := exec.CommandContext(ctx, a.command, args...) // #nosec G204 -- adapter command and fixed help argument come from trusted runtime configuration. + cmd.Dir = repoDir + cmd.Env = append(os.Environ(), a.env...) + cmd.Stdin = strings.NewReader(`{"id":"state-1","type":"get_state"}` + "\n") + stdout := &boundedPiRPCPreflightCapture{remaining: piRPCPreflightOutputBytes / 2} + stderr := &boundedPiRPCPreflightCapture{remaining: piRPCPreflightOutputBytes / 2} + cmd.Stdout = stdout + cmd.Stderr = stderr + 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) + } + if !piRPCPreflightReady(stdout.String(), stderr.String()) { + return fmt.Errorf("%w: reviewer extension preflight did not return required state and registration evidence", ErrPiRPCIncompatible) + } + return nil +} + +func piRPCPreflightReady(stdout, stderr string) bool { + return piRPCPreflightReceivedState(stdout) && piRPCPreflightReceivedRegistration(stderr) +} + +func piRPCPreflightReceivedState(output string) bool { + scanner := bufio.NewScanner(strings.NewReader(output)) + scanner.Buffer(make([]byte, 0, 4*1024), piRPCPreflightOutputBytes) + for scanner.Scan() { + var response struct { + ID string `json:"id"` + Success bool `json:"success"` + } + if err := json.Unmarshal(scanner.Bytes(), &response); err == nil && response.ID == "state-1" && response.Success { + return true + } + } + return false +} + +func piRPCPreflightReceivedRegistration(output string) bool { + scanner := bufio.NewScanner(strings.NewReader(output)) + scanner.Buffer(make([]byte, 0, 4*1024), piRPCPreflightOutputBytes) + for scanner.Scan() { + if strings.TrimSpace(scanner.Text()) == piRPCPreflightRegistration { + return true + } + } + return false +} + +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 +406,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 +486,16 @@ 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 + toolEvidenceBytesLeft int + diffToolStarted int + diffToolCompleted int + diffToolFailed int + diffToolError string } func (s *piRPCStream) run(ctx context.Context, cmd *exec.Cmd, stdout io.Reader, stderr io.Reader) { @@ -218,7 +503,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) @@ -230,6 +515,9 @@ func (s *piRPCStream) run(ctx context.Context, cmd *exec.Cmd, stdout io.Reader, } waitErr := cmd.Wait() <-stderrDone + evidence := s.reviewerToolEvidence() + s.writeReviewerToolEvidence(evidence) + scanResult.response.ReviewerToolEvidence = evidence result := subprocessResult{response: scanResult.response} switch { @@ -269,7 +557,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,11 +567,12 @@ 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 } + s.observeReviewerToolEvent(event) if event.responseFailure != "" { s.Cancel() result.err = fmt.Errorf("llm pi rpc: prompt failed: %s", event.responseFailure) @@ -304,6 +593,132 @@ 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 (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++ + } + if event.toolError != "" && s.diffToolError == "" { + s.diffToolError = event.toolError + } +} + +func (s *piRPCStream) reviewerToolEvidence() *llm.ReviewerToolEvidence { + if !s.allowReviewerTools { + return nil + } + status := llm.DiffToolStatusNotInvoked + switch { + case s.diffToolFailed > 0: + status = llm.DiffToolStatusFailed + case s.diffToolCompleted > 0: + status = llm.DiffToolStatusSucceeded + case s.diffToolStarted > 0: + status = llm.DiffToolStatusIncomplete + } + evidence := &llm.ReviewerToolEvidence{DiffStatus: status} + if status == llm.DiffToolStatusFailed { + evidence.DiffDiagnostic = boundPiRPCToolError(s.diffToolError) + } + return evidence +} + +func (s *piRPCStream) writeReviewerToolEvidence(evidence *llm.ReviewerToolEvidence) { + if evidence == nil || s.toolEvidenceBytesLeft <= 0 { + return + } + line := fmt.Sprintf("codereview-pi-tool-evidence tool=cr_diff status=%s started=%d completed=%d failed=%d", evidence.DiffStatus, s.diffToolStarted, s.diffToolCompleted, s.diffToolFailed) + if evidence.DiffStatus == llm.DiffToolStatusFailed { + line += fmt.Sprintf(" error=%q", evidence.DiffDiagnostic) + } + evidenceBytes := []byte(line + "\n") + s.logLimitMu.Lock() + defer s.logLimitMu.Unlock() + writeBytes := min(len(evidenceBytes), s.toolEvidenceBytesLeft) + if writeBytes > 0 { + 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 { @@ -371,6 +786,11 @@ type piRPCEvent struct { structuredOutput []byte usage Usage toolUse bool + toolName string + toolStarted bool + toolCompleted bool + toolFailed bool + toolError string responseFailure string agentEnd bool } @@ -389,6 +809,26 @@ 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") + 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 event.toolFailed { + event.toolError = piRPCToolError(raw["result"]) + if event.toolError == "" { + event.toolError = "tool execution failed" + } + } + } + } if id := firstRawString(raw, "sessionId", "session_id"); id != "" { event.sessionID = id } @@ -416,6 +856,80 @@ 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, registrationMarker string) string { + quoted := func(value string) string { + data, _ := json.Marshal(value) + return string(data) + } + markerStatement := "" + if registrationMarker != "" { + markerStatement = " process.stderr.write(" + quoted(registrationMarker+"\n") + ");\n" + } + 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) { + let diffAttempted = false; + const headTool = (tool) => (_id, params, signal) => { + if (!diffAttempted) return Promise.resolve({ content: [{ type: "text", text: "cr_diff must be invoked before inspecting repository files" }], details: {}, isError: true }); + return runTool(tool, params, signal); + }; + 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: headTool("cr_read") }); + 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: headTool("cr_search") }); + pi.registerTool({ name: "cr_list", label: "CR List", description: "List repository files.", parameters: { type: "object", properties: { path: { type: "string" } }, additionalProperties: false }, execute: headTool("cr_list") }); + 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) => { diffAttempted = true; return runTool("cr_diff", params, signal); } }); +` + markerStatement + ` +} +` +} + 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..e2f21b37 --- /dev/null +++ b/internal/llmadapters/pi_rpc_extension_unix_test.go @@ -0,0 +1,83 @@ +//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; } });\nvoid tools.cr_diff.execute('call-1', {}, 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) }) +} + +func TestPiRPCReviewerExtensionRequiresDiffBeforeHeadTools(t *testing.T) { + extension := piRPCReviewerExtension("review-tool", "config.json", "/repo", 2048, time.Second, "") + for _, want := range []string{ + "let diffAttempted = false", + "if (!diffAttempted)", + "cr_diff must be invoked before inspecting repository files", + "diffAttempted = true", + "execute: headTool(\"cr_read\")", + } { + if !strings.Contains(extension, want) { + t.Fatalf("extension missing diff-ordering enforcement %q:\n%s", want, extension) + } + } +} diff --git a/internal/llmadapters/pi_rpc_test.go b/internal/llmadapters/pi_rpc_test.go index 9a95c7b7..52669167 100644 --- a/internal/llmadapters/pi_rpc_test.go +++ b/internal/llmadapters/pi_rpc_test.go @@ -13,6 +13,8 @@ import ( "strings" "testing" "time" + + "github.com/open-cli-collective/codereview-cli/internal/llm" ) func TestPiRPCLaunchSafetyAndSuccess(t *testing.T) { @@ -95,6 +97,470 @@ 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) + 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) + } + } + 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) + } + if response.ReviewerToolEvidence == nil || response.ReviewerToolEvidence.DiffStatus != llm.DiffToolStatusNotInvoked { + t.Fatalf("reviewer tool evidence = %#v, want not-invoked cr_diff", response.ReviewerToolEvidence) + } + 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=not_invoked started=0 completed=0 failed=0") { + t.Fatalf("reviewer log = %q, want bounded no-invocation evidence", logged) + } +} + +func TestPiRPCReviewerReportsIncompleteDiffEvidence(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("reviewer-diff-incomplete", 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) + } + response, err := stream.Wait(context.Background()) + if err != nil { + t.Fatalf("Wait: %v", err) + } + if response.ReviewerToolEvidence == nil || response.ReviewerToolEvidence.DiffStatus != llm.DiffToolStatusIncomplete { + t.Fatalf("reviewer tool evidence = %#v, want incomplete cr_diff", response.ReviewerToolEvidence) + } +} + +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) + } + if response.ReviewerToolEvidence == nil || response.ReviewerToolEvidence.DiffStatus != llm.DiffToolStatusFailed || response.ReviewerToolEvidence.DiffDiagnostic != "fixed diff unavailable" { + t.Fatalf("reviewer tool evidence = %#v, want failed cr_diff with bounded diagnostic", response.ReviewerToolEvidence) + } + 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) + } + 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) { + piPath, err := exec.LookPath("pi") + if err != nil { + t.Skip("Pi is not installed") + } + 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, piRPCPreflightRegistration) + 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) + } + if !piRPCPreflightReceivedRegistration(string(output)) { + t.Fatalf("Pi extension output = %s, want registration completion marker", output) + } + if err := NewPiRPCAdapter(PiRPCOptions{Command: piPath}).preflightReviewerRuntime(context.Background()); err != nil { + t.Fatalf("installed Pi reviewer preflight: %v", err) + } +} + +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) { + t.Fatalf("Start error = %v, want classified Pi compatibility error", err) + } + if stream != nil { + t.Fatalf("stream = %#v, want nil before reviewer launch", stream) + } +} + +func TestPiRPCReviewerPreflightDoesNotCacheCanceledContext(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: piRPCHelperEnv("success", filepath.Join(tempDir, "record.json")), + }) + canceled, cancel := context.WithCancel(context.Background()) + cancel() + req := Request{Prompt: "review", ReviewerWorkspace: &ReviewerWorkspaceRequest{ + RepoDir: repoDir, ScratchDir: scratchDir, DiffPath: diffPath, MaxToolOutputBytes: 2048, + }} + if stream, err := adapter.Start(canceled, req); err == nil || stream != nil { + t.Fatalf("canceled Start = (%#v, %v), want preflight failure", stream, err) + } + stream, err := adapter.Start(context.Background(), req) + if err != nil { + t.Fatalf("healthy Start after cancellation: %v", err) + } + if _, err := stream.Wait(context.Background()); err != nil { + t.Fatalf("healthy Wait after cancellation: %v", err) + } +} + +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 empty isolated repository", record.Cwd, record.CwdEntries) + } + if samePath(t, record.Cwd, repoRootForTest(t)) { + t.Fatalf("preflight cwd = repository root %q", record.Cwd) + } + assertFlagValue(t, record.AdapterArgs, "--mode", "rpc") + assertFlagValue(t, record.AdapterArgs, "--tools", piRPCReviewerToolNames) + for _, flag := range []string{"--no-builtin-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 extensionPath := flagValue(record.AdapterArgs, "--extension"); extensionPath == "" || filepath.Base(extensionPath) != "cr-review-tools.mjs" { + t.Fatalf("preflight extension = %q, want generated reviewer extension", extensionPath) + } + if got := strings.Count(record.Extension, "pi.registerTool"); got != 4 { + t.Fatalf("preflight extension registers %d tools, want 4", got) + } + if !strings.Contains(record.Extension, piRPCPreflightRegistration) { + t.Fatalf("preflight extension = %q, want registration-completion marker", record.Extension) + } + if _, err := os.Stat(mutationPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("hostile resource mutation stat = %v, want resource undiscovered", err) + } +} + +func TestPiRPCPreflightReceivedStateRequiresOneSuccessfulStateResponse(t *testing.T) { + for _, tt := range []struct { + name string + output string + want bool + }{ + {name: "valid response", output: `{"id":"state-1","success":true}` + "\n", want: true}, + {name: "substrings in diagnostic", output: `warning: {"id":"state-1"} {"success":true}` + "\n", want: false}, + {name: "wrong response id", output: `{"id":"prompt-1","success":true}` + "\n", want: false}, + {name: "failed response", output: `{"id":"state-1","success":false}` + "\n", want: false}, + } { + t.Run(tt.name, func(t *testing.T) { + if got := piRPCPreflightReceivedState(tt.output); got != tt.want { + t.Fatalf("piRPCPreflightReceivedState(%q) = %t, want %t", tt.output, got, tt.want) + } + }) + } +} + +func TestPiRPCPreflightReceivedRegistrationRequiresExactMarkerLine(t *testing.T) { + if piRPCPreflightReceivedRegistration("prefix " + piRPCPreflightRegistration + " suffix\n") { + t.Fatal("registration marker embedded in diagnostic was accepted") + } + if !piRPCPreflightReceivedRegistration(piRPCPreflightRegistration + "\n") { + t.Fatal("exact registration marker was not accepted") + } +} + +func TestPiRPCPreflightRequiresResponsesOnTheirExpectedStreams(t *testing.T) { + state := `{"id":"state-1","success":true}` + "\n" + marker := piRPCPreflightRegistration + "\n" + if !piRPCPreflightReady(state, marker) { + t.Fatal("preflight readiness = false, want valid separated streams") + } + if piRPCPreflightReady(marker+state, "") || piRPCPreflightReady("", state+marker) { + t.Fatal("preflight accepted state or marker from the wrong stream") + } +} + func TestPiRPCLogStripsCumulativeStreamingPartials(t *testing.T) { recordPath := filepath.Join(t.TempDir(), "record.json") logPath := filepath.Join(t.TempDir(), "pi-rpc.jsonl") @@ -330,7 +796,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 +813,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 +859,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,12 +897,32 @@ 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 strings.Contains(record.Extension, piRPCPreflightRegistration) { + fmt.Fprintln(os.Stderr, piRPCPreflightRegistration) + } } if recordPath != "" { data, _ := json.Marshal(record) // #nosec G703 -- helper writes only to a t.TempDir path supplied by the parent test. _ = os.WriteFile(recordPath, data, 0o600) } + if command["type"] == "get_state" { + if os.Getenv("LLM_PI_RPC_HELP_UNSUPPORTED") == "1" { + fmt.Println(`{"id":"state-1","success":false,"error":"unsupported"}`) + } else { + fmt.Println(`{"id":"state-1","success":true}`) + } + os.Exit(0) + } switch os.Getenv("LLM_HELPER_MODE") { case "success": @@ -427,6 +964,34 @@ 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 "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 "reviewer-diff-incomplete": + 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":"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 +1029,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 +1081,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/llmlifecycle/lifecycle.go b/internal/llmlifecycle/lifecycle.go index 747a7e99..7d2c6b4f 100644 --- a/internal/llmlifecycle/lifecycle.go +++ b/internal/llmlifecycle/lifecycle.go @@ -200,28 +200,29 @@ func (d SessionDraft) ToLedger(runID string) ledger.Session { // Metadata is the durable descriptor for one structured LLM task. type Metadata struct { - SchemaVersion int `json:"schema_version"` - TaskID string `json:"task_id"` - Phase string `json:"phase"` - DependencyTaskIDs []string `json:"dependency_task_ids,omitempty"` - InputFingerprint string `json:"input_fingerprint"` - AgentID string `json:"agent_id,omitempty"` - Status Status `json:"status"` - SessionRowID string `json:"session_row_id,omitempty"` - ProviderSessionID string `json:"provider_session_id,omitempty"` - Adapter string `json:"adapter"` - Model string `json:"model"` - Effort string `json:"effort,omitempty"` - LogPath string `json:"log_path,omitempty"` - ValidatedOutputPath string `json:"validated_output_path,omitempty"` - Error string `json:"error,omitempty"` - TokensIn *int `json:"tokens_in,omitempty"` - TokensOut *int `json:"tokens_out,omitempty"` - CacheRead *int `json:"cache_read,omitempty"` - CacheCreate *int `json:"cache_create,omitempty"` - CostUSD *float64 `json:"cost_usd,omitempty"` - Speed string `json:"speed,omitempty"` - Attempts []AttemptMetadata `json:"attempts,omitempty"` + SchemaVersion int `json:"schema_version"` + TaskID string `json:"task_id"` + Phase string `json:"phase"` + DependencyTaskIDs []string `json:"dependency_task_ids,omitempty"` + InputFingerprint string `json:"input_fingerprint"` + AgentID string `json:"agent_id,omitempty"` + Status Status `json:"status"` + SessionRowID string `json:"session_row_id,omitempty"` + ProviderSessionID string `json:"provider_session_id,omitempty"` + Adapter string `json:"adapter"` + Model string `json:"model"` + Effort string `json:"effort,omitempty"` + LogPath string `json:"log_path,omitempty"` + ValidatedOutputPath string `json:"validated_output_path,omitempty"` + Error string `json:"error,omitempty"` + TokensIn *int `json:"tokens_in,omitempty"` + TokensOut *int `json:"tokens_out,omitempty"` + CacheRead *int `json:"cache_read,omitempty"` + CacheCreate *int `json:"cache_create,omitempty"` + CostUSD *float64 `json:"cost_usd,omitempty"` + Speed string `json:"speed,omitempty"` + ReviewerToolEvidence *llm.ReviewerToolEvidence `json:"reviewer_tool_evidence,omitempty"` + Attempts []AttemptMetadata `json:"attempts,omitempty"` } // AttemptMetadata records one invalid structured-output attempt. @@ -411,6 +412,7 @@ func LoadStructured[T any](ctx context.Context, req Request, decode llm.Decoder[ } draft := SessionDraftFromLedger(session) draft.Response.Usage.Speed = meta.Speed + draft.Response.ReviewerToolEvidence = meta.ReviewerToolEvidence progress := progressResult(meta, llm.StructuredResult[T]{SessionID: meta.ProviderSessionID}, true, draft.Response.Usage) loadProgress(req.Progress, NewProgressEvent(req, ResumeSessionID(meta)), progress) return Result[T]{Value: value, Draft: draft, Session: session, Cached: true}, true, nil @@ -675,24 +677,25 @@ func BaseMetadata(req Request, draft SessionDraft) Metadata { fingerprint = Fingerprint(adapterName(req.Adapter), req.TaskID, req.Phase, req.Model, req.Effort, req.Prompt, req.DependencyTaskIDs) } return Metadata{ - SchemaVersion: SchemaVersion, - TaskID: req.TaskID, - Phase: req.Phase, - DependencyTaskIDs: append([]string(nil), req.DependencyTaskIDs...), - InputFingerprint: fingerprint, - AgentID: agentID, - SessionRowID: draft.RowID, - ProviderSessionID: draft.ProviderSessionID, - Adapter: adapterName(req.Adapter), - Model: draft.Model, - Effort: draft.Effort, - LogPath: req.LogPath, - TokensIn: draft.Response.Usage.TokensIn, - TokensOut: draft.Response.Usage.TokensOut, - CacheRead: draft.Response.Usage.CacheRead, - CacheCreate: draft.Response.Usage.CacheCreate, - CostUSD: draft.Response.Usage.CostUSD, - Speed: draft.Response.Usage.Speed, + SchemaVersion: SchemaVersion, + TaskID: req.TaskID, + Phase: req.Phase, + DependencyTaskIDs: append([]string(nil), req.DependencyTaskIDs...), + InputFingerprint: fingerprint, + AgentID: agentID, + SessionRowID: draft.RowID, + ProviderSessionID: draft.ProviderSessionID, + Adapter: adapterName(req.Adapter), + Model: draft.Model, + Effort: draft.Effort, + LogPath: req.LogPath, + TokensIn: draft.Response.Usage.TokensIn, + TokensOut: draft.Response.Usage.TokensOut, + CacheRead: draft.Response.Usage.CacheRead, + CacheCreate: draft.Response.Usage.CacheCreate, + CostUSD: draft.Response.Usage.CostUSD, + Speed: draft.Response.Usage.Speed, + ReviewerToolEvidence: draft.Response.ReviewerToolEvidence, } } @@ -820,6 +823,7 @@ func SessionDraftFromMetadata(meta Metadata) SessionDraft { Model: meta.Model, Effort: meta.Effort, Response: llm.Response{ + ReviewerToolEvidence: meta.ReviewerToolEvidence, Usage: llm.Usage{ TokensIn: meta.TokensIn, TokensOut: meta.TokensOut, @@ -856,6 +860,7 @@ func loadOptionalTaskSession(ctx context.Context, store Store, runID string, met } draft := SessionDraftFromLedger(session) draft.Response.Usage.Speed = meta.Speed + draft.Response.ReviewerToolEvidence = meta.ReviewerToolEvidence return session, draft, nil } diff --git a/internal/llmlifecycle/lifecycle_test.go b/internal/llmlifecycle/lifecycle_test.go index 73b3d4b2..71643ae4 100644 --- a/internal/llmlifecycle/lifecycle_test.go +++ b/internal/llmlifecycle/lifecycle_test.go @@ -7,6 +7,7 @@ import ( "errors" "os" "path/filepath" + "reflect" "strconv" "strings" "testing" @@ -49,6 +50,10 @@ func TestRunStructuredPersistsAndLoadsSucceededTask(t *testing.T) { Response: llm.Response{ StructuredOutput: []byte(`Here is JSON: {"ok":true}`), DurationMS: 123, + ReviewerToolEvidence: &llm.ReviewerToolEvidence{ + DiffStatus: llm.DiffToolStatusFailed, + DiffDiagnostic: "fixed diff unavailable", + }, Usage: llm.Usage{ TokensIn: intPtr(10), TokensOut: intPtr(5), @@ -103,6 +108,9 @@ func TestRunStructuredPersistsAndLoadsSucceededTask(t *testing.T) { if !cached.Cached || !cached.Value.OK { t.Fatalf("cached result = %#v, want cached ok", cached) } + if got := cached.Draft.Response.ReviewerToolEvidence; got == nil || got.DiffStatus != llm.DiffToolStatusFailed || got.DiffDiagnostic != "fixed diff unavailable" { + t.Fatalf("cached reviewer tool evidence = %#v, want persisted failure evidence", got) + } if len(cachedAdapter.Requests()) != 0 { t.Fatalf("cached adapter requests = %d, want 0", len(cachedAdapter.Requests())) } @@ -212,6 +220,31 @@ func TestSessionDraftFromMetadataRestoresAgentID(t *testing.T) { } } +func TestSessionDraftFromMetadataRestoresReviewerToolEvidence(t *testing.T) { + evidence := &llm.ReviewerToolEvidence{ + DiffStatus: llm.DiffToolStatusFailed, + DiffDiagnostic: "fixed diff unavailable", + } + meta := BaseMetadata(lifecycleRequest(t, newLifecycleStore(), &llm.FakeAdapter{}), SessionDraft{ + Response: llm.Response{ReviewerToolEvidence: evidence}, + }) + if !reflect.DeepEqual(meta.ReviewerToolEvidence, evidence) { + t.Fatalf("metadata reviewer tool evidence = %#v, want %#v", meta.ReviewerToolEvidence, evidence) + } + encoded, err := json.Marshal(meta) + if err != nil { + t.Fatalf("Marshal metadata: %v", err) + } + var restored Metadata + if err := json.Unmarshal(encoded, &restored); err != nil { + t.Fatalf("Unmarshal metadata: %v", err) + } + draft := SessionDraftFromMetadata(restored) + if !reflect.DeepEqual(draft.Response.ReviewerToolEvidence, evidence) { + t.Fatalf("reviewer tool evidence = %#v, want %#v", draft.Response.ReviewerToolEvidence, evidence) + } +} + func TestRunStructuredFailsClosedWhenCachedSessionIsMissing(t *testing.T) { ctx := context.Background() store := newLifecycleStore() @@ -512,6 +545,9 @@ func TestRunStructuredLoadsIsolatedFailureWithoutRerun(t *testing.T) { SessionID: "provider-session-1", Response: llm.Response{ StructuredOutput: []byte(`{"ok":"still-not-bool"}`), + ReviewerToolEvidence: &llm.ReviewerToolEvidence{ + DiffStatus: llm.DiffToolStatusIncomplete, + }, Usage: llm.Usage{ TokensIn: intPtr(34), TokensOut: intPtr(13), @@ -533,7 +569,7 @@ func TestRunStructuredLoadsIsolatedFailureWithoutRerun(t *testing.T) { cachedAdapter := &llm.FakeAdapter{NameValue: "fake-llm"} progress.loads = nil req.Adapter = cachedAdapter - _, err = RunStructured(ctx, req, decodeLifecyclePayload) + cached, err := RunStructured(ctx, req, decodeLifecyclePayload) taskErr = nil if !errors.As(err, &taskErr) || taskErr.Status() != StatusFailedIsolated { t.Fatalf("RunStructured cached isolated error = %v, want cached isolated task error", err) @@ -541,6 +577,9 @@ func TestRunStructuredLoadsIsolatedFailureWithoutRerun(t *testing.T) { if len(cachedAdapter.Requests()) != 0 { t.Fatalf("cached adapter requests = %d, want 0", len(cachedAdapter.Requests())) } + if got := cached.Draft.Response.ReviewerToolEvidence; got == nil || got.DiffStatus != llm.DiffToolStatusIncomplete { + t.Fatalf("cached isolated reviewer tool evidence = %#v, want incomplete evidence", got) + } if len(progress.loads) != 1 { t.Fatalf("cached isolated progress loads = %#v, want one cached progress result", progress.loads) } 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 cbcf02eb..019c0faa 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -308,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 @@ -703,6 +707,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 { @@ -854,7 +861,7 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu result.Findings = findings result.ReviewerFailures = reviewerFailures result.reviewerFastDelivered = reviewerFastDelivery(prepared.fastRequested, reviewerSessions) - reviewerCoverage := buildReviewerCoverage(selection.SelectedAgents, reviewerResults, reviewerFailures, prepared.changedFiles) + reviewerCoverage := buildReviewerCoverage(selection.SelectedAgents, reviewerResults, reviewerFailures, prepared.changedFiles, reviewerToolEvidenceByAgent(reviewerSessions)) result.ReviewerCoverage = reviewerCoverage result.Sessions = appendSessionsIfPresent(result.Sessions, reviewerLedgerSessions...) @@ -964,7 +971,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) { @@ -2117,6 +2124,34 @@ func sanitizeTaskErrorForMarkdown(err error) string { return value } +func reviewerToolDiagnostic(evidence *llm.ReviewerToolEvidence, artifactDir string) string { + if evidence == nil || evidence.DiffStatus == llm.DiffToolStatusSucceeded { + return "" + } + detail := strings.TrimSpace(evidence.DiffDiagnostic) + if detail == "" && evidence.DiffStatus == llm.DiffToolStatusNotInvoked { + detail = "not invoked" + } + if detail == "" { + detail = "tool " + string(evidence.DiffStatus) + } + return normalizeReviewerToolDiagnostic("cr_diff: "+detail, artifactDir) +} + +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, "