fix: restore bounded Pi RPC reviewer workspaces - #540
Merged
Conversation
zzwong
changed the title
zzwong
force-pushed
the
zzwong/INT-672/pi-rpc-reviewer-workspace
branch
from
August 4, 2026 04:21
c012564 to
27d59c8
Compare
zzwong
changed the title
# Conflicts: # internal/llm/contracts.go # internal/llm/contracts_test.go # internal/pipeline/pipeline.go # internal/pipeline/pipeline_test.go # internal/reviewplan/summary.go
zzwong
marked this pull request as ready for review
August 4, 2026 16:37
monit-reviewer
requested changes
Aug 4, 2026
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 4e6a228e51fb
Profile: pi-codex-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 1 |
| structure:repo-health | 0 |
| architecture:solid-reviewer-agnostic | 3 |
| harness-engineering:repo-health | 1 |
go:implementation-tests (1 finding)
Major - internal/llmadapters/pi_rpc.go:795
The
cr_diff-before-head-inspection contract is only placed in the system prompt;runToolpermitscr_read,cr_search, andcr_listimmediately. A reviewer can therefore inspect checkout files without ever reading the pinned diff, and the stream merely logsstatus=not_invokedwithout failing or marking the review incomplete. Enforce the ordering in the generated extension (track acr_diffattempt/result and reject head tools until it occurs), then add a behavioral test that a read-before-diff request is denied while the specified failed-diff path remains usable for recording a constraint.
policies:conventions (1 finding)
Minor - README.md:117
The
cr reviewflags table still says--reviewer-modeland--reviewer-effortrequire--dry-runor--no-post, but this change explicitly permits both on live reviews. Update those two rows to document the new live-review behavior; otherwise users will avoid a supported override or assume a live invocation is invalid.
architecture:solid-reviewer-agnostic (3 findings)
Blocking - internal/pipeline/pipeline.go:2131
[U-L1, U-L2]
reviewerToolDiagnosticrecognizes onlystatus=failed, although the adapter also emitsnot_invokedandincomplete. Since tool ordering is enforced only by the prompt, a reviewer can inspect head files without callingcr_diff, return valid coverage, and still be marked complete—allowing a clean approval without the required pinned-change inspection. Carry typed tool evidence through the response/session contract instead of scraping logs, classify every non-succeededstate asincomplete_tool, and add end-to-end coverage fornot_invokedandincomplete.
Major - internal/llmadapters/pi_rpc.go:212
[U-L1] Compatibility depends on the literal human-facing help phrase
explicit -e paths still work. A semantically compatible Pi release that merely rewords its help will disable all Pi reviewers, even though the required flags and behavior still exist. Replace this prose match with a behavioral probe that loads a generated no-op extension under the discovery-disabled flags, or consume a documented machine-readable capability/version contract.
Minor - internal/llmadapters/pi_rpc.go:121
[U-L1]
sync.Oncepermanently caches failures caused by the first caller's context. If that firstStartis canceled or its preflight times out transiently, every later reviewer invocation on the adapter returns the stale error even with a healthy context. Cache only successful preflight results or deterministic incompatibility; allow context cancellation and temporary execution failures to retry, with a regression test covering a canceled first call followed by a successful call.
harness-engineering:repo-health (1 finding)
Minor - internal/reviewplan/summary.go:283
incomplete_toolis introduced as a durable coverage status but is neither humanized here nor listed in the checkout-native reviewer-output contract. Rollups therefore render the raw enum without the warning marker, and future agents cannot discover this status from the documented contract. Add an explicit⚠️ incomplete (tool failure)label and document the status and its semantics.
Reviewer Coverage
go:implementation-tests— complete (broad); inspected 27 of 30 files:cmd/cr/main.go,cmd/cr/main_test.go,internal/benchmark/metrics.go,internal/benchmark/metrics_test.go,internal/cmd/benchmarkcmd/benchmarkcmd_test.go,internal/cmd/benchmarkcmd/run.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/reviewcmd/reviewcmd_test.go,internal/llm/adapter.go,internal/llm/contracts_test.go,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_extension_unix_test.go,internal/llmadapters/pi_rpc_test.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/workbench_integration_test.go,internal/pireviewtool/path_other.go,internal/pireviewtool/path_unix.go,internal/pireviewtool/path_windows.go,internal/pireviewtool/tool.go,internal/pireviewtool/tool_test.go,internal/reviewplan/summary.go,internal/reviewplan/summary_test.go,internal/workbench/workbench.go,internal/workbench/workbench_test.go; skipped: none; Read-only review; I inspected source and tests but could not execute the test suite.policies:conventions— complete (broad); inspected 10 of 30 files:BENCHMARKING.md,README.md,cmd/cr/main.go,cmd/cr/main_test.go,docs/checkout-native-review-contract.md,internal/cmd/benchmarkcmd/benchmarkcmd_test.go,internal/cmd/benchmarkcmd/run.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/reviewcmd/reviewcmd_test.go; skipped: none; Shared Open CLI Collective source-of-truth docs were not available in the review checkout; review used the repo-local conventions prompt and changed repository surfaces.structure:repo-health— complete (broad); inspected 12 of 30 files:cmd/cr/main.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pireviewtool/path_other.go,internal/pireviewtool/path_unix.go,internal/pireviewtool/path_windows.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go,internal/workbench/workbench.go; skipped: none; Read-only review; I inspected implementation and tests but did not execute the stated verification commands. Review focused on assigned changed files and structural boundary risks, not exhaustive functional/security testing.architecture:solid-reviewer-agnostic— complete (broad); inspected 7 of 30 files:internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go,internal/workbench/workbench.go; skipped: none; Review was limited to the assigned architectural boundaries and nearby tests/docs. The external harness-engineering PRINCIPLES.md path was not accessible through the permitted repository tools. The read-only CR tools do not support running tests, builds, or linters; verification claims were not independently executed.harness-engineering:repo-health— complete (broad); inspected 9 of 30 files:BENCHMARKING.md,README.md,docs/checkout-native-review-contract.md,internal/llmadapters/pi_rpc.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go,internal/workbench/workbench.go; skipped: none; constraints: none
Inspected files (30)
BENCHMARKING.mdREADME.mdcmd/cr/main.gocmd/cr/main_test.godocs/checkout-native-review-contract.mdinternal/benchmark/metrics.gointernal/benchmark/metrics_test.gointernal/cmd/benchmarkcmd/benchmarkcmd_test.gointernal/cmd/benchmarkcmd/run.gointernal/cmd/pireviewtoolcmd/pireviewtoolcmd.gointernal/cmd/reviewcmd/reviewcmd.gointernal/cmd/reviewcmd/reviewcmd_test.gointernal/llm/adapter.gointernal/llm/contracts_test.gointernal/llmadapters/pi_rpc.gointernal/llmadapters/pi_rpc_extension_unix_test.gointernal/llmadapters/pi_rpc_test.gointernal/pipeline/artifacts.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.gointernal/pipeline/workbench_integration_test.gointernal/pireviewtool/path_other.gointernal/pireviewtool/path_unix.gointernal/pireviewtool/path_windows.gointernal/pireviewtool/tool.gointernal/pireviewtool/tool_test.gointernal/reviewplan/summary.gointernal/reviewplan/summary_test.gointernal/workbench/workbench.gointernal/workbench/workbench_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 4m 40s | $0.31 | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol | cr dev
| Field | Value |
|---|---|
| Model | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health, architecture:solid-reviewer-agnostic, harness-engineering:repo-health |
| Engine | pi_rpc · openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewed by | cr · monit-reviewer |
| Duration | 4m 40s wall · 10m 21s compute |
| Cost | $0.31 |
| Tokens | 35.8k in / 6.3k out |
Per-workstream usage
orchestrator-selection— openai-codex/gpt-5.6-terra- In: 8.8k
- Out: 962
- Cache read: 0
- Cache create: 0
- Cost: $0.04
- Duration: 19s
go:implementation-tests— openai-codex/gpt-5.6-terra- In: 6.1k
- Out: 786
- Cache read: 55.8k
- Cache create: 0
- Cost: $0.04
- Duration: 2m 11s
policies:conventions— openai-codex/gpt-5.6-terra- In: 4.8k
- Out: 643
- Cache read: 24.1k
- Cache create: 0
- Cost: $0.03
- Duration: 54s
structure:repo-health— openai-codex/gpt-5.6-terra- In: 5.2k
- Out: 567
- Cache read: 45.6k
- Cache create: 0
- Cost: $0.03
- Duration: 1m 34s
architecture:solid-reviewer-agnostic— openai-codex/gpt-5.6-sol- In: 770
- Out: 2.1k
- Cache read: 94.7k
- Cache create: 0
- Cost: $0.11
- Duration: 4m 02s
harness-engineering:repo-health— openai-codex/gpt-5.6-terra- In: 5.9k
- Out: 861
- Cache read: 58.9k
- Cache create: 0
- Cost: $0.04
- Duration: 1m 09s
orchestrator-rollup— openai-codex/gpt-5.6-terra- In: 4.3k
- Out: 355
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.02
- Duration: 8s
monit-reviewer
requested changes
Aug 4, 2026
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 53acbdc87614
Profile: pi-codex-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| architecture:solid-reviewer-agnostic | 1 |
| harness-engineering:repo-health | 0 |
| structure:harness-engineering | 0 |
| documentation:docs | 1 |
architecture:solid-reviewer-agnostic (1 finding)
Major - internal/pipeline/pipeline.go:2131
U-L2 (Errors are contracts): the provider-neutral pipeline derives machine-significant coverage state by reopening an adapter log and string-matching
codereview-pi-tool-evidence,status=, anderror=. Logs are observability output, not typed lifecycle state; missing, malformed, or reformatted evidence silently returns no diagnostic and can report a reviewer complete after an incomplete diff-tool call. This also couples pipeline behavior to Pi's private text format. Add a narrow typed tool-evidence status and bounded diagnostic tollm.Response, persist it throughllmlifecyclemetadata, and consume that value here; retain the text marker only for logs and benchmarking. Add end-to-end coverage for theincompletestatus as well asnot_invokedandfailed.
documentation:docs (1 finding)
Minor - README.md:120
The compatibility condition incorrectly treats
pi --helpas the capability check. CR actually runs an isolated RPC/extension behavioral preflight, specifically to avoid relying on help-text wording. Say that prior-scope installs can work when CR's compatibility preflight succeeds (and optionally list the required controls), rather than instructing users to gate support on what--helpreports.
Reviewer Coverage
go:implementation-tests—⚠️ incomplete (skipped files); inspected 13 assigned files (24 inspected across reviewers):internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_extension_unix_test.go,internal/llmadapters/pi_rpc_test.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/workbench_integration_test.go,internal/pireviewtool/path_other.go,internal/pireviewtool/path_unix.go,internal/pireviewtool/path_windows.go,internal/pireviewtool/tool.go,internal/pireviewtool/tool_test.go; skipped:cmd/cr/main.go,cmd/cr/main_test.go,internal/benchmark/metrics.go,internal/benchmark/metrics_test.go,internal/cmd/benchmarkcmd/benchmarkcmd_test.go,internal/cmd/benchmarkcmd/run.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/reviewcmd/reviewcmd_test.go,internal/llm/contracts_test.go,internal/reviewplan/summary.go,internal/reviewplan/summary_test.go,internal/threadanalysis/threadanalysis.go,internal/threadanalysis/threadanalysis_test.go,internal/workbench/workbench.go,internal/workbench/workbench_test.go; constraints: Read-only review; I inspected implementation and tests but did not execute the test suite.policies:conventions— complete (broad); inspected 9 assigned files (24 inspected across reviewers):BENCHMARKING.md,README.md,cmd/cr/main.go,cmd/cr/main_test.go,docs/checkout-native-review-contract.md,internal/cmd/benchmarkcmd/run.go,internal/cmd/reviewcmd/reviewcmd.go,internal/pipeline/pipeline.go,internal/reviewplan/summary.go; skipped: none; constraints: Shared CLI standards are referenced by URL but their contents were not available through the permitted repository tools.structure:repo-health— complete (broad); inspected 11 assigned files (24 inspected across reviewers):README.md,cmd/cr/main.go,docs/checkout-native-review-contract.md,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/reviewplan/summary.go,internal/threadanalysis/threadanalysis.go,internal/workbench/workbench.go; skipped: none; constraints: Review was limited to assigned changed files and read-only repository inspection; tests were not executed.architecture:solid-reviewer-agnostic— complete (broad); inspected 7 assigned files (24 inspected across reviewers):internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go,internal/workbench/workbench.go; skipped: none; constraints: Review used the pinned diff and repository text only; CR-owned tools cannot execute tests or builds. The external harness-engineering PRINCIPLES.md path is outside the disposable repository and was not inspectable with permitted tools.harness-engineering:repo-health— complete (broad); inspected 6 assigned files (24 inspected across reviewers):README.md,docs/checkout-native-review-contract.md,internal/llmadapters/pi_rpc.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go; skipped: none; constraints: nonestructure:harness-engineering— complete (broad); inspected 8 assigned files (24 inspected across reviewers):internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/pipeline/pipeline.go,internal/pireviewtool/path_other.go,internal/pireviewtool/path_unix.go,internal/pireviewtool/path_windows.go,internal/pireviewtool/tool.go; skipped: none; constraints: Read-only review; tests were inspected but not executed.documentation:docs— complete (broad); inspected 3 assigned files (24 inspected across reviewers):BENCHMARKING.md,README.md,docs/checkout-native-review-contract.md; skipped: none; constraints: Review limited to the three assigned documentation changes; implementation was inspected only to verify their claims.
Inspected files (24)
BENCHMARKING.mdREADME.mdcmd/cr/main.gocmd/cr/main_test.godocs/checkout-native-review-contract.mdinternal/cmd/benchmarkcmd/run.gointernal/cmd/pireviewtoolcmd/pireviewtoolcmd.gointernal/cmd/reviewcmd/reviewcmd.gointernal/llm/adapter.gointernal/llmadapters/pi_rpc.gointernal/llmadapters/pi_rpc_extension_unix_test.gointernal/llmadapters/pi_rpc_test.gointernal/pipeline/artifacts.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.gointernal/pipeline/workbench_integration_test.gointernal/pireviewtool/path_other.gointernal/pireviewtool/path_unix.gointernal/pireviewtool/path_windows.gointernal/pireviewtool/tool.gointernal/pireviewtool/tool_test.gointernal/reviewplan/summary.gointernal/threadanalysis/threadanalysis.gointernal/workbench/workbench.go
6 PR discussion threads considered. 5 summarized; 5 resolved.
Completed in 4m 35s | $0.33 | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol | cr dev
| Field | Value |
|---|---|
| Model | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health, architecture:solid-reviewer-agnostic, harness-engineering:repo-health, structure:harness-engineering, documentation:docs |
| Engine | pi_rpc · openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewed by | cr · monit-reviewer |
| Duration | 4m 35s wall · 11m 36s compute |
| Cost | $0.33 |
| Tokens | 35.5k in / 7.2k out |
Per-workstream usage
orchestrator-selection— openai-codex/gpt-5.6-terra- In: 6.9k
- Out: 1.1k
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.03
- Duration: 22s
go:implementation-tests— openai-codex/gpt-5.6-terra- In: 6.3k
- Out: 1.2k
- Cache read: 40.4k
- Cache create: 0
- Cost: $0.04
- Duration: 1m 14s
policies:conventions— openai-codex/gpt-5.6-terra- In: 3.9k
- Out: 497
- Cache read: 24.1k
- Cache create: 0
- Cost: $0.02
- Duration: 1m 42s
structure:repo-health— openai-codex/gpt-5.6-terra- In: 1.8k
- Out: 1.2k
- Cache read: 47.6k
- Cache create: 0
- Cost: $0.03
- Duration: 1m 17s
architecture:solid-reviewer-agnostic— openai-codex/gpt-5.6-sol- In: 2.5k
- Out: 1.1k
- Cache read: 103.9k
- Cache create: 0
- Cost: $0.10
- Duration: 2m 45s
harness-engineering:repo-health— openai-codex/gpt-5.6-terra- In: 1.0k
- Out: 370
- Cache read: 27.1k
- Cache create: 0
- Cost: $0.01
- Duration: 1m 14s
structure:harness-engineering— openai-codex/gpt-5.6-terra- In: 4.8k
- Out: 683
- Cache read: 36.4k
- Cache create: 0
- Cost: $0.03
- Duration: 1m 06s
documentation:docs— openai-codex/gpt-5.6-terra- In: 3.7k
- Out: 674
- Cache read: 35.3k
- Cache create: 0
- Cost: $0.03
- Duration: 1m 43s
orchestrator-rollup— openai-codex/gpt-5.6-terra- In: 4.6k
- Out: 350
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.02
- Duration: 9s
monit-reviewer
requested changes
Aug 4, 2026
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 6b0c643208e3
Profile: pi-codex-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| architecture:solid-reviewer-agnostic | 2 |
| harness-engineering:repo-health | 0 |
go:implementation-tests (1 finding)
Minor - internal/llmadapters/pi_rpc.go:594
reviewerToolEvidencehas a distinctincompletebranch for acr_diffstart without completion, but the adapter tests only exercisenot_invokedandfailed. The pipeline test injects incomplete evidence directly, so it would still pass if Pi event parsing stopped recordingtool_execution_startand the adapter incorrectly returnednot_invoked. Add a helper mode that emits acr_diffstart followed byagent_endwithout an end event, then assert the adapter response reportsDiffToolStatusIncomplete.
architecture:solid-reviewer-agnostic (2 findings)
Blocking - internal/llmlifecycle/lifecycle.go:224
U-L1 (behavioral contract), U-T1: reviewer tool evidence is persisted here but is lost when a run-owned task is loaded from cache. The succeeded path at lines 413-414 and
loadOptionalTaskSessionat lines 860-861 rebuild from the ledger and overlay onlySpeed; ledger sessions do not containReviewerToolEvidence. Because the pipeline adds thecr_diff:constraint only after lifecycle persistence, a resumed reviewer can lose failed/not-invoked evidence and be reported as complete, potentially allowing approval. Overlaymeta.ReviewerToolEvidencewhenever rebuilding a draft from ledger, and add a run-owned cached reviewer regression provingincomplete_toolsurvives resume.
Major - internal/llmadapters/pi_rpc.go:195
U-L1 (behavioral contract): the compatibility preflight loads a no-op extension, so successful
get_stateproves only that Pi accepts the flags—not that it supports the production extension'spi.registerToolAPI or can register the four required tools. Such a runtime is cached as ready and fails only after a reviewer starts, contrary to the adapter contract's explicit unsupported-capability failure. Load the actual generated extension with isolated dummy configuration during preflight and deterministically verify that exactly the four CR tools register.
Reviewer Coverage
go:implementation-tests—⚠️ incomplete (skipped files); inspected 28 assigned files (31 inspected across reviewers):cmd/cr/main.go,cmd/cr/main_test.go,internal/benchmark/metrics.go,internal/benchmark/metrics_test.go,internal/cmd/benchmarkcmd/run.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/reviewcmd/reviewcmd_test.go,internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_extension_unix_test.go,internal/llmadapters/pi_rpc_test.go,internal/llmlifecycle/lifecycle.go,internal/llmlifecycle/lifecycle_test.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/workbench_integration_test.go,internal/pireviewtool/path_other.go,internal/pireviewtool/path_unix.go,internal/pireviewtool/path_windows.go,internal/pireviewtool/tool.go,internal/pireviewtool/tool_test.go,internal/reviewplan/summary.go,internal/reviewplan/summary_test.go,internal/threadanalysis/threadanalysis.go,internal/workbench/workbench.go,internal/workbench/workbench_test.go; skipped:internal/cmd/benchmarkcmd/benchmarkcmd_test.go,internal/llm/contracts_test.go,internal/threadanalysis/threadanalysis_test.go; constraints: Review was limited to static inspection through CR read/search/diff tools; no tests were executed.policies:conventions— complete (broad); inspected 7 assigned files (31 inspected across reviewers):BENCHMARKING.md,README.md,cmd/cr/main.go,docs/checkout-native-review-contract.md,internal/cmd/reviewcmd/reviewcmd.go,internal/llmadapters/pi_rpc.go,internal/reviewplan/summary.go; skipped: none; constraints: Reviewed assigned changed files and repo-local conventions prompt. Shared CLI standards were not present in the review context, so no requirements were inferred from their canonical external URL.structure:repo-health— complete (broad); inspected 11 assigned files (31 inspected across reviewers):cmd/cr/main.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go,internal/threadanalysis/threadanalysis.go,internal/workbench/workbench.go; skipped: none; constraints: Reviewed the assigned changed files and their cross-package contracts; did not execute tests or runtime probes.architecture:solid-reviewer-agnostic— complete (broad); inspected 8 assigned files (31 inspected across reviewers):internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go,internal/workbench/workbench.go; skipped: none; constraints: CR tools are read-only and provide no test execution, so I inspected implementation and tests but did not runmake check. The external harness principles path was outside the disposable repository and unavailable through the permitted CR tools.harness-engineering:repo-health— complete (broad); inspected 7 assigned files (31 inspected across reviewers):BENCHMARKING.md,README.md,docs/checkout-native-review-contract.md,internal/llmadapters/pi_rpc.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go; skipped: none; constraints: No commands or tests were executed; conclusions are from pinned-diff and repository inspection. Review scoped to the seven assigned changed files; I inspected related lifecycle, benchmark, and regression-test context only to validate their stated contracts.
Inspected files (31)
BENCHMARKING.mdREADME.mdcmd/cr/main.gocmd/cr/main_test.godocs/checkout-native-review-contract.mdinternal/benchmark/metrics.gointernal/benchmark/metrics_test.gointernal/cmd/benchmarkcmd/run.gointernal/cmd/pireviewtoolcmd/pireviewtoolcmd.gointernal/cmd/reviewcmd/reviewcmd.gointernal/cmd/reviewcmd/reviewcmd_test.gointernal/llm/adapter.gointernal/llmadapters/pi_rpc.gointernal/llmadapters/pi_rpc_extension_unix_test.gointernal/llmadapters/pi_rpc_test.gointernal/llmlifecycle/lifecycle.gointernal/llmlifecycle/lifecycle_test.gointernal/pipeline/artifacts.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.gointernal/pipeline/workbench_integration_test.gointernal/pireviewtool/path_other.gointernal/pireviewtool/path_unix.gointernal/pireviewtool/path_windows.gointernal/pireviewtool/tool.gointernal/pireviewtool/tool_test.gointernal/reviewplan/summary.gointernal/reviewplan/summary_test.gointernal/threadanalysis/threadanalysis.gointernal/workbench/workbench.gointernal/workbench/workbench_test.go
2 PR discussion threads considered. 2 summarized; 0 resolved.
Completed in 4m 23s | $0.35 | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol | cr dev
| Field | Value |
|---|---|
| Model | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health, architecture:solid-reviewer-agnostic, harness-engineering:repo-health |
| Engine | pi_rpc · openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewed by | cr · monit-reviewer |
| Duration | 4m 23s wall · 10m 11s compute |
| Cost | $0.35 |
| Tokens | 44.1k in / 5.2k out |
Per-workstream usage
orchestrator-selection— openai-codex/gpt-5.6-terra- In: 7.5k
- Out: 910
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.03
- Duration: 19s
go:implementation-tests— openai-codex/gpt-5.6-terra- In: 4.3k
- Out: 1.2k
- Cache read: 69.1k
- Cache create: 0
- Cost: $0.05
- Duration: 2m 22s
policies:conventions— openai-codex/gpt-5.6-terra- In: 10.8k
- Out: 311
- Cache read: 26.1k
- Cache create: 0
- Cost: $0.04
- Duration: 30s
structure:repo-health— openai-codex/gpt-5.6-terra- In: 1.5k
- Out: 272
- Cache read: 32.3k
- Cache create: 0
- Cost: $0.02
- Duration: 1m 34s
architecture:solid-reviewer-agnostic— openai-codex/gpt-5.6-sol- In: 14.9k
- Out: 1.9k
- Cache read: 107.0k
- Cache create: 0
- Cost: $0.19
- Duration: 3m 26s
harness-engineering:repo-health— openai-codex/gpt-5.6-terra- In: 1.6k
- Out: 445
- Cache read: 43.5k
- Cache create: 0
- Cost: $0.02
- Duration: 1m 51s
orchestrator-rollup— openai-codex/gpt-5.6-terra- In: 3.4k
- Out: 129
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.01
- Duration: 6s
monit-reviewer
requested changes
Aug 4, 2026
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: dbd8e76af906
Profile: pi-codex-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| architecture:solid-reviewer-agnostic | 1 |
| harness-engineering:repo-health | 0 |
| structure:harness-engineering | 0 |
| documentation:docs | 0 |
go:implementation-tests (1 finding)
Major - internal/llmadapters/pi_rpc.go:259
The compatibility preflight treats any successful
get_stateresponse containing two substrings as proof that the reviewer extension works. It never decodes the response or verifies thatcr_read,cr_search,cr_list, andcr_diffwere registered. A Pi version that accepts these flags but ignores/rejects the extension registration will be cached as ready and launch reviewers without their required inspection surface. Decode the state response and assert the exact required registrations (or issue deterministic calls against each tool); add a fake-runtime regression whereget_statesucceeds but registrations are absent.
architecture:solid-reviewer-agnostic (1 finding)
Major - internal/pipeline/pipeline.go:2485
U-L2: This recovers machine-significant tool-failure state by matching a prefix in model-authored constraint prose. A reviewer can emit a legitimate
cr_diff:constraint even when typed evidence says the tool succeeded, causingincomplete_toolcoverage and coercing an approval to comment; wording changes can also silently break classification. CarryReviewerToolEvidencealongside the reviewer result intobuildReviewerCoverage, derive status only fromDiffStatus, and render the normalized diagnostic separately without rewriting or parsingFindings.Constraints.
Reviewer Coverage
go:implementation-tests—⚠️ incomplete (skipped files); inspected 26 assigned files (29 inspected across reviewers):cmd/cr/main.go,cmd/cr/main_test.go,internal/benchmark/metrics.go,internal/benchmark/metrics_test.go,internal/cmd/benchmarkcmd/run.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/reviewcmd/reviewcmd_test.go,internal/llm/adapter.go,internal/llm/contracts_test.go,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_extension_unix_test.go,internal/llmadapters/pi_rpc_test.go,internal/llmlifecycle/lifecycle.go,internal/llmlifecycle/lifecycle_test.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pireviewtool/path_unix.go,internal/pireviewtool/path_windows.go,internal/pireviewtool/tool.go,internal/pireviewtool/tool_test.go,internal/reviewplan/summary.go,internal/reviewplan/summary_test.go,internal/threadanalysis/threadanalysis.go,internal/workbench/workbench.go; skipped:internal/cmd/benchmarkcmd/benchmarkcmd_test.go,internal/pipeline/workbench_integration_test.go,internal/pireviewtool/path_other.go,internal/threadanalysis/threadanalysis_test.go,internal/workbench/workbench_test.go; constraints: Static review only; I did not execute Go tests or an installed Pi runtime.policies:conventions— complete (broad); inspected 5 assigned files (29 inspected across reviewers):README.md,cmd/cr/main.go,cmd/cr/main_test.go,internal/cmd/benchmarkcmd/run.go,internal/cmd/reviewcmd/reviewcmd.go; skipped: none; constraints: Read-only review tools do not permit running tests or validating runtime behavior. Reviewed only the five assigned changed files and repo-local conventions; shared CLI standards were not available in the repository context.structure:repo-health— complete (broad); inspected 10 assigned files (29 inspected across reviewers):cmd/cr/main.go,internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go,internal/threadanalysis/threadanalysis.go,internal/workbench/workbench.go; skipped: none; constraints: Review was limited to repository reads and the pinned diff; tests and runtime Pi compatibility probes were not executed.architecture:solid-reviewer-agnostic— complete (broad); inspected 6 assigned files (29 inspected across reviewers):internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go; skipped: none; constraints: Review was limited to the six assigned changed files and their surrounding tests/docs. The allowed CR tools are read-only, so I could not run build, test, lint, or race verification. The external SignalFT PRINCIPLES.md path was not accessible through repository-scoped CR tools; I used repo-local architecture guidance.harness-engineering:repo-health— complete (broad); inspected 7 assigned files (29 inspected across reviewers):README.md,docs/checkout-native-review-contract.md,internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go; skipped: none; constraints: nonestructure:harness-engineering— complete (broad); inspected 6 assigned files (29 inspected across reviewers):internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go,internal/workbench/workbench.go; skipped: none; constraints: Read-only review; I inspected implementation and tests but did not execute the test suite.documentation:docs— complete (broad); inspected 3 assigned files (29 inspected across reviewers):BENCHMARKING.md,README.md,docs/checkout-native-review-contract.md; skipped: none; constraints: Reviewed the assigned documentation and relevant implementation statically; no runtime Pi installation or command execution was available.
Inspected files (29)
BENCHMARKING.mdREADME.mdcmd/cr/main.gocmd/cr/main_test.godocs/checkout-native-review-contract.mdinternal/benchmark/metrics.gointernal/benchmark/metrics_test.gointernal/cmd/benchmarkcmd/run.gointernal/cmd/pireviewtoolcmd/pireviewtoolcmd.gointernal/cmd/reviewcmd/reviewcmd.gointernal/cmd/reviewcmd/reviewcmd_test.gointernal/llm/adapter.gointernal/llm/contracts_test.gointernal/llmadapters/pi_rpc.gointernal/llmadapters/pi_rpc_extension_unix_test.gointernal/llmadapters/pi_rpc_test.gointernal/llmlifecycle/lifecycle.gointernal/llmlifecycle/lifecycle_test.gointernal/pipeline/artifacts.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.gointernal/pireviewtool/path_unix.gointernal/pireviewtool/path_windows.gointernal/pireviewtool/tool.gointernal/pireviewtool/tool_test.gointernal/reviewplan/summary.gointernal/reviewplan/summary_test.gointernal/threadanalysis/threadanalysis.gointernal/workbench/workbench.go
3 PR discussion threads considered. 3 summarized; 0 resolved.
Completed in 6m 46s | $0.36 | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol | cr dev
| Field | Value |
|---|---|
| Model | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health, architecture:solid-reviewer-agnostic, harness-engineering:repo-health, structure:harness-engineering, documentation:docs |
| Engine | pi_rpc · openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewed by | cr · monit-reviewer |
| Duration | 6m 46s wall · 13m 08s compute |
| Cost | $0.36 |
| Tokens | 33.7k in / 7.4k out |
Per-workstream usage
orchestrator-selection— openai-codex/gpt-5.6-terra- In: 8.0k
- Out: 1.1k
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.04
- Duration: 22s
go:implementation-tests— openai-codex/gpt-5.6-terra- In: 2.4k
- Out: 827
- Cache read: 68.1k
- Cache create: 0
- Cost: $0.04
- Duration: 1m 31s
policies:conventions— openai-codex/gpt-5.6-terra- In: 2.5k
- Out: 188
- Cache read: 25.1k
- Cache create: 0
- Cost: $0.02
- Duration: 42s
structure:repo-health— openai-codex/gpt-5.6-terra- In: 2.3k
- Out: 776
- Cache read: 46.6k
- Cache create: 0
- Cost: $0.03
- Duration: 1m 21s
architecture:solid-reviewer-agnostic— openai-codex/gpt-5.6-sol- In: 2.1k
- Out: 2.4k
- Cache read: 125.4k
- Cache create: 0
- Cost: $0.14
- Duration: 5m 21s
harness-engineering:repo-health— openai-codex/gpt-5.6-terra- In: 6.5k
- Out: 834
- Cache read: 26.1k
- Cache create: 0
- Cost: $0.04
- Duration: 54s
structure:harness-engineering— openai-codex/gpt-5.6-terra- In: 4.1k
- Out: 741
- Cache read: 36.4k
- Cache create: 0
- Cost: $0.03
- Duration: 1m 40s
documentation:docs— openai-codex/gpt-5.6-terra- In: 1.7k
- Out: 315
- Cache read: 29.2k
- Cache create: 0
- Cost: $0.02
- Duration: 1m 05s
orchestrator-rollup— openai-codex/gpt-5.6-terra- In: 4.1k
- Out: 290
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.02
- Duration: 8s
monit-reviewer
requested changes
Aug 4, 2026
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: ea0579686d64
Profile: pi-codex-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| architecture:solid-reviewer-agnostic | 2 |
| harness-engineering:repo-health | 0 |
architecture:solid-reviewer-agnostic (2 findings)
Blocking - internal/llmadapters/pi_rpc.go:250
U-L1: The preflight contract requires a successful state response on stdout and the registration marker on stderr, but both streams are written into the same capture. This can accept a marker emitted on stdout and concurrent writes can splice otherwise valid lines, so the compatibility gate does not honor its channel-specific readiness semantics. Capture stdout and stderr separately, parse state only from stdout and registration only from stderr, and add a regression proving a stdout marker is rejected.
Major - internal/pireviewtool/tool.go:137
U-L1: The advertised bounded-tool capability rejects binary-file access, but
cr_readaccepts every regular file and passes its bytes throughreadRange; only search performs binary detection. A reviewer can therefore read binary checkout content and receive corrupted replacement text through Node's UTF-8 conversion. Apply the same binary classification before serving read ranges and add directcr_readbinary coverage.
Reviewer Coverage
go:implementation-tests— complete (broad); inspected 31 assigned files (34 inspected across reviewers):cmd/cr/main.go,cmd/cr/main_test.go,internal/benchmark/metrics.go,internal/benchmark/metrics_test.go,internal/cmd/benchmarkcmd/benchmarkcmd_test.go,internal/cmd/benchmarkcmd/run.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/reviewcmd/reviewcmd_test.go,internal/llm/adapter.go,internal/llm/contracts_test.go,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_extension_unix_test.go,internal/llmadapters/pi_rpc_test.go,internal/llmlifecycle/lifecycle.go,internal/llmlifecycle/lifecycle_test.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/workbench_integration_test.go,internal/pireviewtool/path_other.go,internal/pireviewtool/path_unix.go,internal/pireviewtool/path_windows.go,internal/pireviewtool/tool.go,internal/pireviewtool/tool_test.go,internal/reviewplan/summary.go,internal/reviewplan/summary_test.go,internal/threadanalysis/threadanalysis.go,internal/threadanalysis/threadanalysis_test.go,internal/workbench/workbench.go,internal/workbench/workbench_test.go; skipped: none; constraints: Static review only; repository tools are read-only and no tests were executed.policies:conventions— complete (broad); inspected 10 assigned files (34 inspected across reviewers):BENCHMARKING.md,README.md,cmd/cr/main.go,cmd/cr/main_test.go,docs/checkout-native-review-contract.md,internal/cmd/benchmarkcmd/benchmarkcmd_test.go,internal/cmd/benchmarkcmd/run.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/reviewcmd/reviewcmd_test.go; skipped: none; constraints: Shared Open CLI Collective source-of-truth documentation was not present in the repository context; this review assessed only conventions evidenced by the supplied repo context and changed files.structure:repo-health— complete (broad); inspected 10 assigned files (34 inspected across reviewers):cmd/cr/main.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/reviewplan/summary.go,internal/threadanalysis/threadanalysis.go,internal/workbench/workbench.go; skipped: none; constraints: Inspected assigned changed source files and the pinned diff; did not execute tests or runtime probes.architecture:solid-reviewer-agnostic— complete (broad); inspected 8 assigned files (34 inspected across reviewers):internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go,internal/workbench/workbench.go; skipped: none; constraints: The external SignalFT PRINCIPLES.md path was outside the disposable repository and could not be inspected with the allowed tools. Verification commands could not be run because only CR read-only repository tools were available.harness-engineering:repo-health— complete (broad); inspected 8 assigned files (34 inspected across reviewers):BENCHMARKING.md,README.md,docs/checkout-native-review-contract.md,internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go; skipped: none; constraints: Read-only review; I inspected the pinned diff and assigned source/docs but did not execute tests.
Inspected files (34)
BENCHMARKING.mdREADME.mdcmd/cr/main.gocmd/cr/main_test.godocs/checkout-native-review-contract.mdinternal/benchmark/metrics.gointernal/benchmark/metrics_test.gointernal/cmd/benchmarkcmd/benchmarkcmd_test.gointernal/cmd/benchmarkcmd/run.gointernal/cmd/pireviewtoolcmd/pireviewtoolcmd.gointernal/cmd/reviewcmd/reviewcmd.gointernal/cmd/reviewcmd/reviewcmd_test.gointernal/llm/adapter.gointernal/llm/contracts_test.gointernal/llmadapters/pi_rpc.gointernal/llmadapters/pi_rpc_extension_unix_test.gointernal/llmadapters/pi_rpc_test.gointernal/llmlifecycle/lifecycle.gointernal/llmlifecycle/lifecycle_test.gointernal/pipeline/artifacts.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.gointernal/pipeline/workbench_integration_test.gointernal/pireviewtool/path_other.gointernal/pireviewtool/path_unix.gointernal/pireviewtool/path_windows.gointernal/pireviewtool/tool.gointernal/pireviewtool/tool_test.gointernal/reviewplan/summary.gointernal/reviewplan/summary_test.gointernal/threadanalysis/threadanalysis.gointernal/threadanalysis/threadanalysis_test.gointernal/workbench/workbench.gointernal/workbench/workbench_test.go
1 PR discussion threads considered. 1 summarized; 0 resolved.
Completed in 4m 16s | $0.38 | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol | cr dev
| Field | Value |
|---|---|
| Model | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health, architecture:solid-reviewer-agnostic, harness-engineering:repo-health |
| Engine | pi_rpc · openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewed by | cr · monit-reviewer |
| Duration | 4m 16s wall · 9m 54s compute |
| Cost | $0.38 |
| Tokens | 57.5k in / 5.0k out |
Per-workstream usage
orchestrator-selection— openai-codex/gpt-5.6-terra- In: 8.5k
- Out: 1.0k
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.04
- Duration: 19s
go:implementation-tests— openai-codex/gpt-5.6-terra- In: 27.9k
- Out: 1.5k
- Cache read: 107.0k
- Cache create: 0
- Cost: $0.12
- Duration: 1m 30s
policies:conventions— openai-codex/gpt-5.6-terra- In: 2.8k
- Out: 274
- Cache read: 22.0k
- Cache create: 0
- Cost: $0.02
- Duration: 1m 23s
structure:repo-health— openai-codex/gpt-5.6-terra- In: 1.5k
- Out: 753
- Cache read: 52.7k
- Cache create: 0
- Cost: $0.03
- Duration: 2m 10s
architecture:solid-reviewer-agnostic— openai-codex/gpt-5.6-sol- In: 12.8k
- Out: 968
- Cache read: 112.1k
- Cache create: 0
- Cost: $0.15
- Duration: 3m 23s
harness-engineering:repo-health— openai-codex/gpt-5.6-terra- In: 537
- Out: 329
- Cache read: 49.7k
- Cache create: 0
- Cost: $0.02
- Duration: 59s
orchestrator-rollup— openai-codex/gpt-5.6-terra- In: 3.4k
- Out: 215
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.01
- Duration: 7s
monit-reviewer
requested changes
Aug 4, 2026
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 28a98f36fd8f
Profile: pi-codex-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| architecture:solid-reviewer-agnostic | 1 |
| harness-engineering:repo-health | 0 |
architecture:solid-reviewer-agnostic (1 finding)
Major - internal/pireviewtool/tool.go:192
U-L1/U-T1:
rejectBinaryFileapplies the whole-file binary contract to only the first 8 KiB. A valid UTF-8 rune split at the probe boundary is rejected, while a NUL or invalid sequence after byte 8192 remains readable through pagination. This violates the documented binary-file exclusion. Stream-validate the file while preserving partial UTF-8 sequences before returning pages, and add regressions for both boundary-spanning valid UTF-8 and binary bytes after the probe boundary.
Reviewer Coverage
go:implementation-tests— complete (broad); inspected 31 assigned files (34 inspected across reviewers):cmd/cr/main.go,cmd/cr/main_test.go,internal/benchmark/metrics.go,internal/benchmark/metrics_test.go,internal/cmd/benchmarkcmd/benchmarkcmd_test.go,internal/cmd/benchmarkcmd/run.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/reviewcmd/reviewcmd_test.go,internal/llm/adapter.go,internal/llm/contracts_test.go,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_extension_unix_test.go,internal/llmadapters/pi_rpc_test.go,internal/llmlifecycle/lifecycle.go,internal/llmlifecycle/lifecycle_test.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/workbench_integration_test.go,internal/pireviewtool/path_other.go,internal/pireviewtool/path_unix.go,internal/pireviewtool/path_windows.go,internal/pireviewtool/tool.go,internal/pireviewtool/tool_test.go,internal/reviewplan/summary.go,internal/reviewplan/summary_test.go,internal/threadanalysis/threadanalysis.go,internal/threadanalysis/threadanalysis_test.go,internal/workbench/workbench.go,internal/workbench/workbench_test.go; skipped: none; constraints: Static review only; CR tools are read-only and cannot execute Go tests.policies:conventions— complete (broad); inspected 8 assigned files (34 inspected across reviewers):BENCHMARKING.md,README.md,cmd/cr/main.go,cmd/cr/main_test.go,docs/checkout-native-review-contract.md,internal/cmd/benchmarkcmd/run.go,internal/cmd/reviewcmd/reviewcmd.go,internal/reviewplan/summary.go; skipped: none; constraints: The referenced shared CLI and automation source-of-truth repositories were not present in the review context, so this review assessed only visible repo-local conventions.structure:repo-health— complete (broad); inspected 11 assigned files (34 inspected across reviewers):cmd/cr/main.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go,internal/threadanalysis/threadanalysis.go,internal/workbench/workbench.go; skipped: none; constraints: Read-only repository inspection; I did not execute the test suite or runtime Pi preflight.architecture:solid-reviewer-agnostic— complete (broad); inspected 8 assigned files (34 inspected across reviewers):internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go,internal/workbench/workbench.go; skipped: none; constraints: Review was limited to the assigned files plus relevant repo-local guidance and tests. The external harness-engineering PRINCIPLES.md path is outside the disposable repository and unavailable through the allowed tools. The read-only CR tools cannot run tests, lint, or builds; verification evidence was inspected, not executed.harness-engineering:repo-health— complete (broad); inspected 8 assigned files (34 inspected across reviewers):BENCHMARKING.md,README.md,docs/checkout-native-review-contract.md,internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go; skipped: none; constraints: Inspected the pinned diff and assigned source/docs through read-only CR tools; did not execute tests or inspect unassigned files.
Inspected files (34)
BENCHMARKING.mdREADME.mdcmd/cr/main.gocmd/cr/main_test.godocs/checkout-native-review-contract.mdinternal/benchmark/metrics.gointernal/benchmark/metrics_test.gointernal/cmd/benchmarkcmd/benchmarkcmd_test.gointernal/cmd/benchmarkcmd/run.gointernal/cmd/pireviewtoolcmd/pireviewtoolcmd.gointernal/cmd/reviewcmd/reviewcmd.gointernal/cmd/reviewcmd/reviewcmd_test.gointernal/llm/adapter.gointernal/llm/contracts_test.gointernal/llmadapters/pi_rpc.gointernal/llmadapters/pi_rpc_extension_unix_test.gointernal/llmadapters/pi_rpc_test.gointernal/llmlifecycle/lifecycle.gointernal/llmlifecycle/lifecycle_test.gointernal/pipeline/artifacts.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.gointernal/pipeline/workbench_integration_test.gointernal/pireviewtool/path_other.gointernal/pireviewtool/path_unix.gointernal/pireviewtool/path_windows.gointernal/pireviewtool/tool.gointernal/pireviewtool/tool_test.gointernal/reviewplan/summary.gointernal/reviewplan/summary_test.gointernal/threadanalysis/threadanalysis.gointernal/threadanalysis/threadanalysis_test.gointernal/workbench/workbench.gointernal/workbench/workbench_test.go
2 PR discussion threads considered. 2 summarized; 0 resolved.
Completed in 6m 14s | $0.33 | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol | cr dev
| Field | Value |
|---|---|
| Model | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health, architecture:solid-reviewer-agnostic, harness-engineering:repo-health |
| Engine | pi_rpc · openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewed by | cr · monit-reviewer |
| Duration | 6m 14s wall · 10m 19s compute |
| Cost | $0.33 |
| Tokens | 35.5k in / 5.6k out |
Per-workstream usage
orchestrator-selection— openai-codex/gpt-5.6-terra- In: 8.9k
- Out: 961
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.04
- Duration: 19s
go:implementation-tests— openai-codex/gpt-5.6-terra- In: 8.6k
- Out: 1.5k
- Cache read: 60.9k
- Cache create: 0
- Cost: $0.06
- Duration: 1m 27s
policies:conventions— openai-codex/gpt-5.6-terra- In: 2.7k
- Out: 231
- Cache read: 39.4k
- Cache create: 0
- Cost: $0.02
- Duration: 52s
structure:repo-health— openai-codex/gpt-5.6-terra- In: 5.8k
- Out: 1.0k
- Cache read: 50.7k
- Cache create: 0
- Cost: $0.04
- Duration: 1m 21s
architecture:solid-reviewer-agnostic— openai-codex/gpt-5.6-sol- In: 2.6k
- Out: 1.5k
- Cache read: 158.2k
- Cache create: 0
- Cost: $0.14
- Duration: 5m 14s
harness-engineering:repo-health— openai-codex/gpt-5.6-terra- In: 924
- Out: 261
- Cache read: 25.1k
- Cache create: 0
- Cost: $0.01
- Duration: 56s
orchestrator-rollup— openai-codex/gpt-5.6-terra- In: 6.2k
- Out: 205
- Cache read: 0
- Cache create: 0
- Cost: $0.02
- Duration: 6s
monit-reviewer
approved these changes
Aug 4, 2026
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 3f9a163abfc7
Profile: pi-codex-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| architecture:solid-reviewer-agnostic | 2 |
| harness-engineering:repo-health | 0 |
go:implementation-tests (1 finding)
Minor - internal/llmadapters/pi_rpc.go:603
writeLogcaps arbitrary byte chunks, so it can leave a partial JSONL event immediately before the cap marker.benchmark.ExtractRunMetricstreats that resulting line as malformed JSON (it only recognizes standalone tool-evidence lines) and returns an error, losing the Pi-diff telemetry precisely when the bounded-log path is exercised. Write only complete newline-delimited log records before the cap marker, or make the metrics parser explicitly tolerate this known truncation marker; add a regression that extracts metrics from the capped-log fixture.
architecture:solid-reviewer-agnostic (2 findings)
Minor - internal/reviewplan/summary.go:321
U-O1/U-L1: coverage-status behavior is distributed across raw-string switches, and the new case already diverges.
pipelineassignsincomplete_toolonly after a reviewer result exists, so empty skipped files and constraints are known values, butcoverageResultProducedomits this status and renders them as unknown. Define a typedreviewplan.CoverageStatuswith centralizedResultProduced,Label, and completeness behavior, then use its constants frompipeline; at minimum includeincomplete_toolhere and add regression coverage.
Minor - internal/llmlifecycle/lifecycle.go:224
U-G1:
reviewer_tool_evidenceis a new durable metadata field that changes resumed coverage and approval behavior, but the canonicaldocs/llm-task-artifacts.mdschema does not mention or classify it. Document the field, its status values, whether it is load-bearing, and why schema version 1 remains safe for older artifacts without it.
Reviewer Coverage
go:implementation-tests— complete (broad); inspected 31 assigned files (34 inspected across reviewers):cmd/cr/main.go,cmd/cr/main_test.go,internal/benchmark/metrics.go,internal/benchmark/metrics_test.go,internal/cmd/benchmarkcmd/benchmarkcmd_test.go,internal/cmd/benchmarkcmd/run.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/cmd/reviewcmd/reviewcmd.go,internal/cmd/reviewcmd/reviewcmd_test.go,internal/llm/adapter.go,internal/llm/contracts_test.go,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_extension_unix_test.go,internal/llmadapters/pi_rpc_test.go,internal/llmlifecycle/lifecycle.go,internal/llmlifecycle/lifecycle_test.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/workbench_integration_test.go,internal/pireviewtool/path_other.go,internal/pireviewtool/path_unix.go,internal/pireviewtool/path_windows.go,internal/pireviewtool/tool.go,internal/pireviewtool/tool_test.go,internal/reviewplan/summary.go,internal/reviewplan/summary_test.go,internal/threadanalysis/threadanalysis.go,internal/threadanalysis/threadanalysis_test.go,internal/workbench/workbench.go,internal/workbench/workbench_test.go; skipped: none; constraints: Inspected the pinned diff and relevant implementation/tests through CR read-only tools; no tests were executed.policies:conventions— complete (broad); inspected 9 assigned files (34 inspected across reviewers):BENCHMARKING.md,README.md,cmd/cr/main.go,docs/checkout-native-review-contract.md,internal/cmd/benchmarkcmd/run.go,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/cmd/reviewcmd/reviewcmd.go,internal/pipeline/pipeline.go,internal/reviewplan/summary.go; skipped: none; constraints: Shared Open CLI Collective source-of-truth docs were not present in the review checkout; reviewed only visible repo-local guidance and assigned changed surfaces.structure:repo-health— complete (broad); inspected 12 assigned files (34 inspected across reviewers):README.md,cmd/cr/main.go,docs/checkout-native-review-contract.md,internal/cmd/pireviewtoolcmd/pireviewtoolcmd.go,internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go,internal/workbench/workbench.go; skipped: none; constraints: Inspected the assigned changed files and pinned diff only; did not execute tests.architecture:solid-reviewer-agnostic— complete (broad); inspected 8 assigned files (34 inspected across reviewers):internal/llm/adapter.go,internal/llmadapters/pi_rpc.go,internal/llmlifecycle/lifecycle.go,internal/pipeline/artifacts.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go,internal/workbench/workbench.go; skipped: none; constraints: Read-only review tools were available; I could not run build, test, lint, or race checks. Review focused on the eight assigned changed files, with surrounding tests and architecture documentation inspected for context.harness-engineering:repo-health— complete (broad); inspected 8 assigned files (34 inspected across reviewers):BENCHMARKING.md,README.md,cmd/cr/main.go,docs/checkout-native-review-contract.md,internal/llmadapters/pi_rpc.go,internal/pipeline/pipeline.go,internal/pireviewtool/tool.go,internal/reviewplan/summary.go; skipped: none; constraints: Static review only: CR-owned tools are read-only, so I did not execute tests or an installed-Pi smoke test.
Inspected files (34)
BENCHMARKING.mdREADME.mdcmd/cr/main.gocmd/cr/main_test.godocs/checkout-native-review-contract.mdinternal/benchmark/metrics.gointernal/benchmark/metrics_test.gointernal/cmd/benchmarkcmd/benchmarkcmd_test.gointernal/cmd/benchmarkcmd/run.gointernal/cmd/pireviewtoolcmd/pireviewtoolcmd.gointernal/cmd/reviewcmd/reviewcmd.gointernal/cmd/reviewcmd/reviewcmd_test.gointernal/llm/adapter.gointernal/llm/contracts_test.gointernal/llmadapters/pi_rpc.gointernal/llmadapters/pi_rpc_extension_unix_test.gointernal/llmadapters/pi_rpc_test.gointernal/llmlifecycle/lifecycle.gointernal/llmlifecycle/lifecycle_test.gointernal/pipeline/artifacts.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.gointernal/pipeline/workbench_integration_test.gointernal/pireviewtool/path_other.gointernal/pireviewtool/path_unix.gointernal/pireviewtool/path_windows.gointernal/pireviewtool/tool.gointernal/pireviewtool/tool_test.gointernal/reviewplan/summary.gointernal/reviewplan/summary_test.gointernal/threadanalysis/threadanalysis.gointernal/threadanalysis/threadanalysis_test.gointernal/workbench/workbench.gointernal/workbench/workbench_test.go
1 PR discussion threads considered. 1 summarized; 0 resolved.
Completed in 5m 23s | $0.37 | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol | cr dev
| Field | Value |
|---|---|
| Model | openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health, architecture:solid-reviewer-agnostic, harness-engineering:repo-health |
| Engine | pi_rpc · openai-codex/gpt-5.6-terra, openai-codex/gpt-5.6-sol |
| Reviewed by | cr · monit-reviewer |
| Duration | 5m 23s wall · 10m 40s compute |
| Cost | $0.37 |
| Tokens | 29.2k in / 6.4k out |
Per-workstream usage
orchestrator-selection— openai-codex/gpt-5.6-terra- In: 9.1k
- Out: 1.0k
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.04
- Duration: 22s
go:implementation-tests— openai-codex/gpt-5.6-terra- In: 1.7k
- Out: 1.8k
- Cache read: 73.2k
- Cache create: 0
- Cost: $0.05
- Duration: 1m 47s
policies:conventions— openai-codex/gpt-5.6-terra- In: 3.3k
- Out: 513
- Cache read: 13.8k
- Cache create: 0
- Cost: $0.02
- Duration: 1m 02s
structure:repo-health— openai-codex/gpt-5.6-terra- In: 5.7k
- Out: 244
- Cache read: 65.0k
- Cache create: 0
- Cost: $0.03
- Duration: 1m 13s
architecture:solid-reviewer-agnostic— openai-codex/gpt-5.6-sol- In: 2.6k
- Out: 2.4k
- Cache read: 216.6k
- Cache create: 0
- Cost: $0.19
- Duration: 4m 28s
harness-engineering:repo-health— openai-codex/gpt-5.6-terra- In: 3.4k
- Out: 296
- Cache read: 39.4k
- Cache create: 0
- Cost: $0.02
- Duration: 1m 42s
orchestrator-rollup— openai-codex/gpt-5.6-terra- In: 3.4k
- Out: 148
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.01
- Duration: 5s
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Checkout-native Pi RPC reviewers need bounded access to their disposable review checkout. The previous Pi adapter disabled every tool, so migrated reviews could select Pi but could not inspect the repository content required to complete a review.
This change restores Pi RPC reviewer workspaces through an explicit
permission_boundedcapability. Pi receives only four CR-owned read-only tools for the disposable checkout and fixed diff, while non-reviewer Pi calls continue to run without tools.Changes
cr_diffbefore head-file inspection and preserve compact invocation evidence within the existing aggregate log cap.Scope limits
Verification
make checkgo test ./... -count=1go test ./internal/pipeline -count=1including reused-cohort summary accounting coveragegolangci-lint rungit diff --checkTarget-environment dogfood
Status: PENDING. Owner: Aaron.
After installing the branch build, run a live Pi RPC review against a representative pull request and confirm that the reviewer can inspect assigned files and the pinned diff through only the four CR-owned tools, produces a completed structured review, and leaves the disposable checkout unchanged. If the runtime capability preflight fails or the review cannot complete within the bounded contract, remove the branch build and continue using the existing supported reviewer adapters.