fix: restore bounded Pi RPC reviewer workspaces

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. |

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 <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 <effort>` | Override selection-stage effort only with `low`, `medium`, or `high`. Requires `--dry-run` or `--no-post`. |
 | `--selection-prompt <path>` | Load selection-stage instruction text from a file while preserving the structured JSON selection protocol. Requires `--dry-run` or `--no-post`. |
-| `--reviewer-model <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 <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 <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 <effort>` | Override reviewer-stage effort only with `low`, `medium`, or `high`. Requires `--dry-run` or `--no-post`. |
+| `--reviewer-effort <effort>` | Override reviewer-stage effort only with `low`, `medium`, or `high`. Available for dry-run, no-post, and live reviews. |
 | `--review-base-sha <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 <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 <name>` | 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`. |

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,

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()

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

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

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 {

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)
 	}
 }

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