feat(pipeline): delete the run workbench after a successful review

[zzwong]

Closes #598.

Every review run provisions a full repository checkout at workbench/repo, and nothing ever removed it. The only os.RemoveAll of that directory is internal/workbench/workbench.go:114, which clears a stale workbench before provisioning a new one — it is not a teardown. Retention is the 90-day LiveRetention window, so checkouts simply accumulated.

Measured on my data root before this change:

Run count: 1061
Oldest started: 2026-08-03      # 40 days — the age gate had never once fired
Artifact bytes: 57,672,996,172  # 57.7 GB

52.7 GB of that 57.7 GB was retained workbenches: 436 runs, none ever reclaimed. A single pull request with 85 runs held 9.8 GB.

What changed

The workbench is removed once a run reaches a successful terminal state, immediately after completed = true in execute.

• Failed and errored runs keep theirs. Those are the runs someone wants to inspect, and the teardown sits past every early-return path.
• Removal never fails the run. An error is surfaced through the existing emitWarning mechanism.
• data.keep_workbench opts out, defaulting to false. Threaded through pipeline.Options and populated by the review, benchmark, and respond commands.

Findings, rollups, dossiers and logs are untouched and keep the existing retention window. They are a small fraction of the bytes and they are the part worth keeping — the point is that one knob previously governed both.

LiveRetention and the prune/purge paths in internal/datalifecycle are unchanged.

Tests

New internal/pipeline/workbench_cleanup_test.go:

• TestDryRunRemovesWorkbenchAfterSuccess — workbench gone, findings.json and rollup.md still present.
• TestDryRunRetainsWorkbenchAfterFailure — a selection-provider failure leaves workbench/repo in place.
• TestDryRunRetainsWorkbenchWhenKeepWorkbenchEnabled.

Three existing tests that assert workbench preparation after a successful dry run now set KeepWorkbench: true, so they keep testing exactly what they tested before.

Verified not inert: neutering the teardown's guard so it always returns fails TestDryRunRemovesWorkbenchAfterSuccess with workbench stat err = <nil>, want removed after success.

Note

This reclaims new runs only. Existing accumulated state still needs cr data prune; at 14 days that drops 511 runs here.

[monit-reviewer]

Automated PR Review

Reviewed commit: 7a72eac163ed
Profile: pi-ds-gh - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	2
policies:conventions	2
structure:repo-health	2
documentation:docs	2

go:implementation-tests (2 findings)

Minor - internal/pipeline/pipeline.go:736

completed = true here means pipeline planning succeeded, not that the run reached a successful terminal state. Dry runs complete inside execute, but live runs deliberately do not (if !mode.live guard just above), so for live runs the checkout is deleted before reviewrun.continueRun posts through outbox.Post. When that post fails terminally the outbox completes the run as failed (internal/outbox/outbox.go), and a stale SHA completes it as aborted - both after the workbench is already gone. That contradicts the contract this PR documents (docs/checkout-native-review-contract.md: failed or errored runs retain the workbench for inspection) and the PR description. The three new tests cover dry-run selection failure only, so nothing pins the live path. Fix: key the teardown off the run's terminal outcome (keep dry-run teardown in execute, delete after a successful outbox.Post for live runs), or amend the contract to say cleanup is keyed to planning success; either way add a live-mode test for the post-failure case.

Minor - internal/pipeline/pipeline_test.go:333

Adding KeepWorkbench: true here (and in TestDryRunFastFallsBackForUnsupportedModel) preserves those tests' workbench-preparation assertions, but the same sweep missed TestDryRunReviewerFailureIsolation (pipeline_test.go:3142-3146). Those assertions stat result.Artifacts.WorkbenchDir/reviewers/<agent> and .../scratch/<agent> and expect os.ErrNotExist; now that a successful dry run deletes the entire workbench tree they pass for the wrong reason and no longer cover reviewer workspace/scratch cleanup after a successful reviewer task. Fix: set KeepWorkbench: true in that test's Options so the per-agent cleanup assertions keep exercising prepareReviewerWorkspace cleanup.

policies:conventions (2 findings)

Major - internal/config/config.go:426

This new default silently invalidates a repo-owned check. scripts/verify-large-pr-review.sh asserts the run workbench exists after a successful no-post review: assert_artifact_shape (lines 123-134) requires workbench/metadata.json, workbench/repo, and workbench/scratch, and it is called on the first successful run at line 463 (right after run_review at 461). assert_sentinel_source (line 169) also greps the sentinel inside $artifact_dir/workbench/repo and is called at lines 468 and 474. fail() exits 1 (line 46), so the harness now hard-fails with invariant=workbench_exists on runs that succeeded. Fix in this PR: either opt the harness into retention (point XDG_CONFIG_HOME at a temp dir whose config sets data.keep_workbench: true, alongside the existing XDG_DATA_HOME/XDG_CACHE_HOME isolation at line 461), or replace the workbench assertions with diff.patch-only source checks plus an explicit workbench absent assertion. Leaving it as-is means the next person who runs the large-PR verification gets a false failure.

Minor - docs/init-config-surface.md:71

Two gaps on this new contract row. (1) The Guidance column claims "Interactive init preserves the current value when editing retention", but the Evidence column cites only internal/pipeline teardown tests. No initcmd test covers preservation, unlike the sibling config.data.retention.enforcement row (line 70) which cites #290 init tests. The round trip appears to hold today (config.Normalize does not touch cfg.Data, and init saves the session's full config.File via saveConfig at internal/cmd/initcmd/initcmd.go:6406), but the claim is unverified — either add an init preservation test mirroring the #290 pattern or drop the clause. (2) The Ownership column makes this "Direct config-file management", yet the key currently appears only in internal design docs. README documents data-lifecycle config in prose (data.retention.* at lines 648 and 1569-1575), so add one line there: successful runs delete the run checkout and data.keep_workbench: true opts out. Otherwise users watching reclaimed disk have no discoverable switch.

structure:repo-health (2 findings)

Minor - internal/cmd/reviewcmd/reviewcmd.go:308

KeepWorkbench is the only opt-out protecting a user's retained checkouts, and its config-to-runtime wiring is untested at every boundary. The sibling data-policy fields on the same line are enforced by the existing tables — TestRetentionConfigToRuntimeFactory in reviewcmd_test.go and the equivalent table in respondcmd_test.go:84-116 assert got.Retention/got.RetentionManualOnly from config — but no test asserts got.KeepWorkbench, so dropping this assignment (here, in respondcmd.go:118, or in benchmarkcmd/executor.go:161) would compile and pass CI while silently deleting workbenches for users who opted out.

Fix: extend both existing retention-to-OpenRequest table tests with a keep_workbench: true case asserting got.KeepWorkbench, and add the same assertion where the benchmark executor's OpenRequest is captured. That keeps the new key on the same enforcement seam as data.retention.* instead of relying on the doc table and the pipeline-level tests alone.

Minor - internal/cmd/benchmarkcmd/executor.go:161

The knob reaches in-process cr benchmark run here but not the sibling selection-only path, leaving the leak class this PR targets open on cr benchmark select. benchmarkcmd/select.go passes each run's directory as pipeline.SelectionRequest.ArtifactDir; pipeline.SelectionOnly calls workbench.Prepare with ArtifactPathsFromDir(runDir), so workbench/repo is created under .cr-bench/results/<suite>/select/<timestamp>/<runID> and nothing reclaims it — there is no os.RemoveAll anywhere in internal/cmd/benchmarkcmd, and cr data prune/cr data purge only walk the data root, not .cr-bench. internal/app/runtime_test.go:579 asserting WorkbenchMetadataPath() exists after Select confirms the checkout is created there and persists. A suite with many runs against a large PR therefore still accumulates one full pin-checkout per run, with no retention, prune, or opt-out.

Fix: own teardown in internal/workbench (it already clears a stale WorkbenchDir at workbench.go:114) and call that one seam from both the pipeline's end of a successful run and the selection-only boundary, honoring keep_workbench for both; if .cr-bench retention is intentionally out of scope, say so explicitly in docs/checkout-native-review-contract.md next to the existing "SelectionOnly and benchmark callers still own their artifact directory choice" note.

documentation:docs (2 findings)

Minor - docs/checkout-native-review-contract.md:110

"once planning completes" is not what the code does and it conflicts with this document's own runtime sequence. execute removes the workbench at the very end of a successful run, after selection, reviewer execution, rollup, and plan build (completed = true then opts.removeWorkbench(prepared.artifacts), internal/pipeline/pipeline.go:735-736), so steps 4 and 5 of this contract ("Run orchestrator selection from dossier/workbench inputs", "Run specialist reviewers against per-reviewer disposable workspaces") happen while workbench/ still exists. As written, a maintainer can read it as the tree disappearing at the end of step 3. The next paragraph compounds this: it still calls workbench/metadata.json "a versioned durable artifact" (line 115), which is now true only for failed runs or with data.keep_workbench: true — scripts/verify-large-pr-review.sh:132 asserts that path survives a run. Fix: say removal happens "when a run reaches a successful terminal state, after rollup and plan build", and qualify the metadata sentence as surviving only for retained workbenches (failed/errored runs and data.keep_workbench: true).

Minor - docs/init-config-surface.md:71

The new row is internally consistent with the code (KeepWorkbench defaults false and is threaded through review/benchmark/respond), and interactive init does preserve it (deps.saveConfig(plan.path, plan.cfg) re-encodes the loaded config), so the preservation claim holds. What is missing is the update to the docs this one contradicts: docs/review-guidance.md:114 still states "No separate retention setting is required for dossier or workbench cleanup", and that doc's runs/<run-id>/workbench/ section plus README's Retention section (README.md:1567-1579) describe workbench cleanup only via cr data prune/cr data purge. Because this row designates direct config-file editing as the only path for the field and keep_workbench appears nowhere in README.md, users and agents following the linked docs will not discover the knob that now governs it. Fix: update the review-guidance cleanup section to say a successful run deletes its workbench (failed runs always retain it, data.keep_workbench: true opts back in), and add one sentence to README's Retention section.

Reviewer Coverage

• go:implementation-tests — complete (broad); inspected 9 assigned files (11 inspected across reviewers): internal/app/runtime.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/reviewcmd/reviewcmd.go, internal/config/config.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/workbench_cleanup_test.go, internal/pipeline/workbench_integration_test.go; skipped: none; constraints: Read-only review: I could not run go test/go build; behavior and test-adequacy claims come from reading the pinned diff plus surrounding source. Scope kept to Go implementation and test invariants for the assigned files; docs are cited only as evidence for contract drift. cr_read ranged offsets behaved like byte offsets, so I navigated the large files with cr_search plus reads; cited line numbers come from search results and the diff.
• policies:conventions — complete (broad); inspected 6 assigned files (11 inspected across reviewers): docs/init-config-surface.md, internal/app/runtime.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/reviewcmd/reviewcmd.go, internal/config/config.go; skipped: none; constraints: Findings are anchored to the six assigned files; the concrete breakage below lives in scripts/verify-large-pr-review.sh, which is outside the assigned change set. Review is scoped to convention/policy drift; pipeline correctness of the teardown placement and test adequacy in internal/pipeline belong to other reviewers. Shared Open CLI Collective standards (../cli-common/docs, ../.github) are not present in this artifact-clone workbench; I did not infer their contents from memory. cr_read returned only a ~30-character fragment per call for every file tried, so file bodies were inspected via the pinned cr_diff plus cr_search line hits rather than full reads.
• structure:repo-health — complete (broad); inspected 5 assigned files (11 inspected across reviewers): internal/app/runtime.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/reviewcmd/reviewcmd.go, internal/config/config.go; skipped: none; constraints: Findings had to be anchored to the five assigned changed files, so repo-health issues living in other files under the same diff (internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, docs/, scripts/) are referenced from the nearest assigned anchor rather than filed on their own line. I could not anchor, and therefore did not file, two real but non-assigned observations: docs/checkout-native-review-contract.md still calls workbench/metadata.json a "versioned durable artifact" one paragraph below the new claim that a successful run removes the whole workbench/ tree; and the... I did not run any tests or build; all statements about test behavior and CI are based on reading the test/script sources. Rubric scope: this review covers structural/repo-health risk, not exhaustive defect discovery. Verified by reading: config data preservation across cr init is safe — cloneInitConfigFile (initcmd.go:4691) value-copies DataConfig, and the retention/secrets editors mutate full-config clones via configedit, so keep_workbench survives those flows. Verified by reading: in live mode the workbench is not needed after planning (internal/reviewrun and internal/threadrespond do not reference the workbench at all), so the teardown placement does not break posting.
• documentation:docs — complete (broad); inspected 2 assigned files (11 inspected across reviewers): docs/checkout-native-review-contract.md, docs/init-config-surface.md; skipped: none; constraints: Cross-doc staleness can only be anchored to the two assigned changed files per the output contract. Docs-only review of a code change: teardown behavior verified by reading internal/pipeline/pipeline.go (completed = true at 735, opts.removeWorkbench(prepared.artifacts) at 736) and by search, not by running tests. cr_read returns byte-ranged chunks rather than whole files, so non-changed docs (README, review-guidance) were sampled at the relevant sections only.

Inspected files (11)

• docs/checkout-native-review-contract.md
• docs/init-config-surface.md
• internal/app/runtime.go
• internal/cmd/benchmarkcmd/executor.go
• internal/cmd/respondcmd/respondcmd.go
• internal/cmd/reviewcmd/reviewcmd.go
• internal/config/config.go
• internal/pipeline/pipeline.go
• internal/pipeline/pipeline_test.go
• internal/pipeline/workbench_cleanup_test.go
• internal/pipeline/workbench_integration_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 6m 40s | $0.01 | opencode-go/deepseek-v4.1-flash | cr 0.10.302

Field	Value
Model	opencode-go/deepseek-v4.1-flash
Reviewers	go:implementation-tests, policies:conventions, structure:repo-health, documentation:docs
Engine	pi_rpc · opencode-go/deepseek-v4.1-flash
Reviewed by	cr · monit-reviewer
Duration	6m 40s wall · 20m 07s compute
Cost	$0.01
Tokens	22.5k in / 12.6k out

Per-workstream usage

• orchestrator-selection — opencode-go/deepseek-v4.1-flash
  • In: 6.8k
  • Out: 1.7k
  • Cache read: 0
  • Cache create: 0
  • Cost: $0.00
  • Duration: 9s
• go:implementation-tests — opencode-go/deepseek-v4.1-flash
  • In: 2.9k
  • Out: 2.1k
  • Cache read: 128.5k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 6m 02s
• policies:conventions — opencode-go/deepseek-v4.1-flash
  • In: 967
  • Out: 2.9k
  • Cache read: 80.9k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 5m 39s
• structure:repo-health — opencode-go/deepseek-v4.1-flash
  • In: 2.5k
  • Out: 2.1k
  • Cache read: 142.2k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 4m 56s
• documentation:docs — opencode-go/deepseek-v4.1-flash
  • In: 176
  • Out: 821
  • Cache read: 58.6k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 3m 04s
• orchestrator-rollup — opencode-go/deepseek-v4.1-flash
  • In: 9.2k
  • Out: 2.9k
  • Cache read: 0
  • Cache create: 0
  • Cost: $0.00
  • Duration: 15s

[monit-reviewer]

KeepWorkbench is the only opt-out protecting a user's retained checkouts, and its config-to-runtime wiring is untested at every boundary. The sibling data-policy fields on the same line are enforced by the existing tables — TestRetentionConfigToRuntimeFactory in reviewcmd_test.go and the equivalent table in respondcmd_test.go:84-116 assert got.Retention/got.RetentionManualOnly from config — but no test asserts got.KeepWorkbench, so dropping this assignment (here, in respondcmd.go:118, or in benchmarkcmd/executor.go:161) would compile and pass CI while silently deleting workbenches for users who opted out.

Fix: extend both existing retention-to-OpenRequest table tests with a keep_workbench: true case asserting got.KeepWorkbench, and add the same assertion where the benchmark executor's OpenRequest is captured. That keeps the new key on the same enforcement seam as data.retention.* instead of relying on the doc table and the pipeline-level tests alone.

Reply inline to this comment.

[zzwong]

Added. reviewcmd_test.go now covers all three states of the config-to-runtime wiring: flag absent with config true, flag absent with config false, and --keep-workbench passed with config false. Verified not inert - bypassing the Changed() guard so the flag value is assigned unconditionally fails the config_true_without_flag case.

[monit-reviewer]

That covers reviewcmd.go. The finding named two other seams where the same assignment can be dropped silently:

• the equivalent Retention/RetentionManualOnly table in respondcmd_test.go:84-116 (wiring at respondcmd.go:118)
• the OpenRequest captured in the benchmarkcmd/executor.go:161 test path

Both are still unguarded as far as this thread shows. Please extend the respondcmd_test.go table with the same three keep_workbench states, and assert got.KeepWorkbench wherever the benchmark executor's OpenRequest is captured. If you already added them, point me at the lines/commit and I'll close this out.

[monit-reviewer]

"once planning completes" is not what the code does and it conflicts with this document's own runtime sequence. execute removes the workbench at the very end of a successful run, after selection, reviewer execution, rollup, and plan build (completed = true then opts.removeWorkbench(prepared.artifacts), internal/pipeline/pipeline.go:735-736), so steps 4 and 5 of this contract ("Run orchestrator selection from dossier/workbench inputs", "Run specialist reviewers against per-reviewer disposable workspaces") happen while workbench/ still exists. As written, a maintainer can read it as the tree disappearing at the end of step 3. The next paragraph compounds this: it still calls workbench/metadata.json "a versioned durable artifact" (line 115), which is now true only for failed runs or with data.keep_workbench: true — scripts/verify-large-pr-review.sh:132 asserts that path survives a run. Fix: say removal happens "when a run reaches a successful terminal state, after rollup and plan build", and qualify the metadata sentence as surviving only for retained workbenches (failed/errored runs and data.keep_workbench: true).

Reply inline to this comment.

[zzwong]

Reworded. The teardown does not happen once planning completes - for dry runs it happens at the end of execute, and for live runs after the post reaches a terminal outcome. The document now describes that sequence.

[monit-reviewer]

Thanks — the teardown rewording matches the code path: execute flips completed = true and calls opts.removeWorkbench(...) at the tail of a successful run (internal/pipeline/pipeline.go:735-736), after selection, reviewers, rollup, and plan build, so the workbench is still present during steps 4 and 5.

One item from the original comment is still open: the sentence calling workbench/metadata.json "a versioned durable artifact" (line 115). That only holds for retained workbenches — failed/errored runs and data.keep_workbench: true — and scripts/verify-large-pr-review.sh:132 asserts the path survives a run. Please confirm that sentence is qualified the same way.

I'm inferring the current state of the doc from your reply; I haven't re-read the updated file in this environment. Leaving the thread open until that second sentence is confirmed.

[monit-reviewer]

Automated PR Review

Reviewed commit: 9dbc2d050e87
Profile: pi-ds-gh - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	3
policies:conventions	2
structure:repo-health	2
documentation:docs	0

go:implementation-tests (3 findings)

Minor - internal/pipeline/pipeline.go:3364

RemoveWorkbench re-derives the workbench location with a hardcoded "workbench" literal instead of the package that owns artifact layout. runartifact.Paths already defines WorkbenchDir: filepath.Join(dir, "workbench") (internal/runartifact/runartifact.go:80) and ArtifactPathsFromDir exposes it; reviewrun.go:346 already uses that helper, even for the warning message printed one line above this call. Because os.RemoveAll returns nil when the path does not exist, relocating or renaming the workbench directory would make teardown a silent no-op — the 57 GB accumulation this PR fixes would come back with no warning and no test failure. Fix: return os.RemoveAll(ArtifactPathsFromDir(artifactDir).WorkbenchDir) so teardown and the warning text share one source of truth.

Minor - internal/pipeline/pipeline.go:739

The new default teardown also removes the checkout for the no-leak audit's own review runs, and that suite's filesystem scan was not pinned to keep it. internal/cmd/noleak/noleak_test.go:804 builds pipeline.Options (and :822 mirrors reviewrun.Options) without KeepWorkbench, so successful audit runs now delete workbench/ under the data root, while assertOwnedFilesDoNotLeak walks layout.DataRoot (noleak_test.go:1183-1210) asserting no seeded secret landed on disk. The checkout subtree — a clone of the fixture repo plus its .git — silently drops out of the scanned surface, which is the same vacuous-pass class already caught for TestDryRunReviewerFailureIsolation and fixed by adding KeepWorkbench: true to the pipeline tests. Fix: set KeepWorkbench: true in the audit harness's pipeline and reviewrun options too, so the no-leak scan keeps covering the workbench instead of shrinking without a signal. I inspected the harness but could not execute the suite here; if an audit review case ends non-success the workbench is retained and that run's subtree is still scanned.

Nits - internal/cmd/respondcmd/respondcmd.go:118

KeepWorkbench is inert on the respond path. internal/threadrespond does not import internal/pipeline or internal/workbench and neither creates nor removes a workbench, and reviewRunner.Respond reads only the respond options (internal/app/runtime.go:656), so req.KeepWorkbench is consumed solely by the review dry-run/live paths. The new assertion at internal/cmd/respondcmd/respondcmd_test.go:126 therefore pins a wire that has no behavior and suggests respond honors data.keep_workbench when it has no checkout to retain. Fix: drop the field from the respond request (or leave a one-line comment that respond runs have no workbench) rather than asserting an unused value.

policies:conventions (2 findings)

Minor - scripts/verify-large-pr-review.sh:494

The second run is asserted workbench-free (assert_artifact_shape "$second_artifacts" 0, line 496) but is invoked without any workbench flag, while the script deliberately "leaves normal cr config resolution alone" (usage text). Anyone whose config now sets the newly documented data.keep_workbench: true gets a false invariant=workbench_absent failure on a review that succeeded, turning this harness into a config-dependent check. The first run already controls the knob explicitly with the extra 1; make the second symmetric by appending --keep-workbench=false when keep_workbench is 0 (review_args+=(--keep-workbench=false)), which cmd.Flags().Changed honors as an explicit false.

Minor - docs/init-config-surface.md:71

Two accuracy issues on this new inventory row:

• Scripted owner is now incomplete. The cell says only Direct config-file management, but the same PR adds a per-run override. The config.profiles.<name>.fast row is the in-repo precedent: Direct config-file management; cr review --fast and --no-fast override it per invocation. Mirror that wording, and add --keep-workbench to the Runtime-only Flag Audit cr review rows, which enumerate every other review flag group (posting gates already names --no-resolve-threads with its durable counterpart review_policy.resolve_threads).
• Evidence cites a path the test does not exercise. Mutation semantics claims interactive init preserves the value "when editing retention", and Evidence points at init tests, but the test added in this PR (internal/cmd/initcmd/initcmd_test.go:6163, assertion at :6325) drives the menu as initMenuActionReviewProfiles then initMenuActionSave (initcmd_test.go:18412). The retention editor sits behind initMenuActionGlobalSettings (init_menu.go:105) and never runs there. Either narrow the wording to Interactive init preserves the current value. or assert KeepWorkbench in the retention-edit coverage that exists for data.retention.* (initcmd_test.go:11926).

structure:repo-health (2 findings)

Minor - internal/config/config.go:426

fileHasNoExplicitContent (internal/config/config.go:968) is the guard that decides whether config.Save may persist a config at all, and it enumerates every durable setting by hand (cfg.Data.Retention.MaxAgeDays, cfg.Data.Retention.Enforcement, plus the map fields). Adding the DataConfig.KeepWorkbench field without extending that predicate makes the guard wrong for the new field: a config whose only explicit content is data.keep_workbench: true is classified as empty and Save returns ErrInvalid: config is empty instead of writing. Today's CLI paths always carry profiles, so the impact is latent, but the new data setting is now inconsistent with the write boundary, and the same omission repeats for the next knob. Contrast internal/config/init_surface_doc_test.go, which derives the expected doc inventory from the schema by reflection so it cannot drift. Fix: add !cfg.Data.KeepWorkbench to fileHasNoExplicitContent and cover it with a Save test using a data-only config, e.g. File{Data: DataConfig{KeepWorkbench: true}}.

Minor - internal/cmd/reviewcmd/reviewcmd.go:132

--keep-workbench is added here but is missing from the cr review flag reference in README.md (the Modes, Review selection and execution flags, and Policy and output flags tables around README.md:1209-1255): every other flag registered in this file appears there, including --no-resolve-threads, --allow-self-approve, and --review-base-sha. The README is the durable map users and agents read, and this PR already touched it for the retention/config side (README.md:646-649), while docs/review-guidance.md:114-117 documents only the config knob. Net effect: the one flag that makes the new teardown default safe to inspect is the one that stays undiscoverable, even though scripts/verify-large-pr-review.sh:288 now depends on it. Fix: add a row to the cr review policy/output table, e.g. | --keep-workbench | Keep the run workbench checkout after a successful review. Overrides data.keep_workbench for this invocation. |.

Reviewer Coverage

• go:implementation-tests — complete (constrained); inspected 17 assigned files (22 inspected across reviewers): internal/app/runtime.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/executor_test.go, internal/cmd/benchmarkcmd/select.go, internal/cmd/benchmarkcmd/select_test.go, internal/cmd/initcmd/initcmd_test.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/respondcmd/respondcmd_test.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_test.go, internal/config/config.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/workbench_cleanup_test.go, internal/pipeline/workbench_integration_test.go, internal/reviewrun/reviewrun.go, internal/reviewrun/reviewrun_test.go; skipped: none; constraints: No Go tooling or tests were executed; this is a static review with read-only tools. Statements about test behavior come from reading the diff and tests, not from running them. Scope is limited to Go implementation and test adequacy for the assigned files; README, docs, and scripts/verify-large-pr-review.sh were only considered where they affect Go wiring. The no-leak audit harness is outside the assigned changed files, so that finding is anchored to the changed line that introduced the default teardown. cr_read returned unusable output for every file in this environment (truncated/garbled fragments), so file inspection relied on the pinned diff plus cr_search line hits; full function bodies could not be read.
• policies:conventions — complete (constrained); inspected 9 assigned files (22 inspected across reviewers): README.md, docs/init-config-surface.md, docs/review-guidance.md, internal/app/runtime.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/reviewcmd/reviewcmd.go, internal/config/config.go, scripts/verify-large-pr-review.sh; skipped: none; constraints: Read-only review: no go test, shell harness, or cr invocation was executed; every claim comes from diff and source inspection. Scope was the pinned diff plus the nine assigned files; internal/cmd/initcmd/init_menu.go was read only to confirm which menu action reaches the retention editor. The optional sibling checkouts ../cli-common/docs and ../.github are not present in the workbench, so shared Open CLI Collective standards were available only through repo-local breadcrumbs (AGENTS.md, CONTRIBUTING.md, docs/development.md).
• structure:repo-health — complete (constrained); inspected 5 assigned files (22 inspected across reviewers): internal/app/runtime.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/reviewcmd/reviewcmd.go, internal/config/config.go; skipped: none; constraints: All findings are anchored to the five assigned files, even where the remedy lands in README.md or docs (outside the assigned set). Read-only review: no tests or commands were executed; behavior and test-coverage claims come from reading source and test files. cr_diff is served in small windows (~100-400 chars per call against a 45,421-unit diff), so full-diff paging was impractical; I reviewed the assigned files at head plus targeted cr_search lookups. docs/checkout-native-review-contract.md:140 still describes SelectionOnly/benchmark callers as fee-owning their artifact directory; that doc is outside the assigned set, so I could not anchor a finding on it.
• documentation:docs — complete (constrained); inspected 2 assigned files (22 inspected across reviewers): docs/checkout-native-review-contract.md, docs/init-config-surface.md; skipped: none; constraints: Assigned docs-only scope: docs/checkout-native-review-contract.md and docs/init-config-surface.md. README.md, docs/review-guidance.md, and code were inspected only as cross-doc/code context; findings cannot be anchored to them per the allowed_files list. I inspected the head revision of both assigned files; no tool failures occurred. I verified doc claims against internal/pipeline/pipeline.go (teardown after completed = true, skipped for live mode), internal/reviewrun/reviewrun.go (removeWorkbenchAfterPost allowlist), and internal/cmd/benchmarkcmd/select.go (caller-owned reclaim). The previously open item about workbench/metadata.json being described unconditionally as a durable artifact is resolved at the head revision: docs/checkout-native-review-contract.md:118-120 now scopes it to retained workbenches (failed/errored runs and successes with `data.keep_workbench: tr...

Inspected files (22)

• README.md
• docs/checkout-native-review-contract.md
• docs/init-config-surface.md
• docs/review-guidance.md
• internal/app/runtime.go
• internal/cmd/benchmarkcmd/executor.go
• internal/cmd/benchmarkcmd/executor_test.go
• internal/cmd/benchmarkcmd/select.go
• internal/cmd/benchmarkcmd/select_test.go
• internal/cmd/initcmd/initcmd_test.go
• internal/cmd/respondcmd/respondcmd.go
• internal/cmd/respondcmd/respondcmd_test.go
• internal/cmd/reviewcmd/reviewcmd.go
• internal/cmd/reviewcmd/reviewcmd_test.go
• internal/config/config.go
• internal/pipeline/pipeline.go
• internal/pipeline/pipeline_test.go
• internal/pipeline/workbench_cleanup_test.go
• internal/pipeline/workbench_integration_test.go
• internal/reviewrun/reviewrun.go
• internal/reviewrun/reviewrun_test.go
• scripts/verify-large-pr-review.sh

8 PR discussion threads considered. 6 summarized; 6 resolved.

---

Completed in 9m 14s | $0.01 | opencode-go/deepseek-v4.1-flash | cr dev

Field	Value
Model	opencode-go/deepseek-v4.1-flash
Reviewers	go:implementation-tests, policies:conventions, structure:repo-health, documentation:docs
Engine	pi_rpc · opencode-go/deepseek-v4.1-flash
Reviewed by	cr · monit-reviewer
Duration	9m 14s wall · 19m 15s compute
Cost	$0.01
Tokens	10.4k in / 14.6k out

Per-workstream usage

• go:implementation-tests — opencode-go/deepseek-v4.1-flash
  • In: 171
  • Out: 3.6k
  • Cache read: 107.1k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 7m 19s
• policies:conventions — opencode-go/deepseek-v4.1-flash
  • In: 258
  • Out: 5.0k
  • Cache read: 118.4k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 5m 26s
• structure:repo-health — opencode-go/deepseek-v4.1-flash
  • In: 202
  • Out: 1.3k
  • Cache read: 101.5k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 4m 24s
• documentation:docs — opencode-go/deepseek-v4.1-flash
  • In: 696
  • Out: 2.3k
  • Cache read: 60.9k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 1m 54s
• orchestrator-rollup — opencode-go/deepseek-v4.1-flash
  • In: 9.1k
  • Out: 2.4k
  • Cache read: 0
  • Cache create: 0
  • Cost: $0.00
  • Duration: 10s

[monit-reviewer]

fileHasNoExplicitContent (internal/config/config.go:968) is the guard that decides whether config.Save may persist a config at all, and it enumerates every durable setting by hand (cfg.Data.Retention.MaxAgeDays, cfg.Data.Retention.Enforcement, plus the map fields). Adding the DataConfig.KeepWorkbench field without extending that predicate makes the guard wrong for the new field: a config whose only explicit content is data.keep_workbench: true is classified as empty and Save returns ErrInvalid: config is empty instead of writing. Today's CLI paths always carry profiles, so the impact is latent, but the new data setting is now inconsistent with the write boundary, and the same omission repeats for the next knob. Contrast internal/config/init_surface_doc_test.go, which derives the expected doc inventory from the schema by reflection so it cannot drift. Fix: add !cfg.Data.KeepWorkbench to fileHasNoExplicitContent and cover it with a Save test using a data-only config, e.g. File{Data: DataConfig{KeepWorkbench: true}}.

Reply inline to this comment.

[monit-reviewer]

--keep-workbench is added here but is missing from the cr review flag reference in README.md (the Modes, Review selection and execution flags, and Policy and output flags tables around README.md:1209-1255): every other flag registered in this file appears there, including --no-resolve-threads, --allow-self-approve, and --review-base-sha. The README is the durable map users and agents read, and this PR already touched it for the retention/config side (README.md:646-649), while docs/review-guidance.md:114-117 documents only the config knob. Net effect: the one flag that makes the new teardown default safe to inspect is the one that stays undiscoverable, even though scripts/verify-large-pr-review.sh:288 now depends on it. Fix: add a row to the cr review policy/output table, e.g. | --keep-workbench | Keep the run workbench checkout after a successful review. Overrides data.keep_workbench for this invocation. |.

Reply inline to this comment.

[monit-reviewer]

Two accuracy issues on this new inventory row:

• Scripted owner is now incomplete. The cell says only Direct config-file management, but the same PR adds a per-run override. The config.profiles.<name>.fast row is the in-repo precedent: Direct config-file management; cr review --fast and --no-fast override it per invocation. Mirror that wording, and add --keep-workbench to the Runtime-only Flag Audit cr review rows, which enumerate every other review flag group (posting gates already names --no-resolve-threads with its durable counterpart review_policy.resolve_threads).
• Evidence cites a path the test does not exercise. Mutation semantics claims interactive init preserves the value "when editing retention", and Evidence points at init tests, but the test added in this PR (internal/cmd/initcmd/initcmd_test.go:6163, assertion at :6325) drives the menu as initMenuActionReviewProfiles then initMenuActionSave (initcmd_test.go:18412). The retention editor sits behind initMenuActionGlobalSettings (init_menu.go:105) and never runs there. Either narrow the wording to Interactive init preserves the current value. or assert KeepWorkbench in the retention-edit coverage that exists for data.retention.* (initcmd_test.go:11926).

Reply inline to this comment.

[monit-reviewer]

KeepWorkbench is inert on the respond path. internal/threadrespond does not import internal/pipeline or internal/workbench and neither creates nor removes a workbench, and reviewRunner.Respond reads only the respond options (internal/app/runtime.go:656), so req.KeepWorkbench is consumed solely by the review dry-run/live paths. The new assertion at internal/cmd/respondcmd/respondcmd_test.go:126 therefore pins a wire that has no behavior and suggests respond honors data.keep_workbench when it has no checkout to retain. Fix: drop the field from the respond request (or leave a one-line comment that respond runs have no workbench) rather than asserting an unused value.

Reply inline to this comment.

[monit-reviewer]

The new default teardown also removes the checkout for the no-leak audit's own review runs, and that suite's filesystem scan was not pinned to keep it. internal/cmd/noleak/noleak_test.go:804 builds pipeline.Options (and :822 mirrors reviewrun.Options) without KeepWorkbench, so successful audit runs now delete workbench/ under the data root, while assertOwnedFilesDoNotLeak walks layout.DataRoot (noleak_test.go:1183-1210) asserting no seeded secret landed on disk. The checkout subtree — a clone of the fixture repo plus its .git — silently drops out of the scanned surface, which is the same vacuous-pass class already caught for TestDryRunReviewerFailureIsolation and fixed by adding KeepWorkbench: true to the pipeline tests. Fix: set KeepWorkbench: true in the audit harness's pipeline and reviewrun options too, so the no-leak scan keeps covering the workbench instead of shrinking without a signal. I inspected the harness but could not execute the suite here; if an audit review case ends non-success the workbench is retained and that run's subtree is still scanned.

Reply inline to this comment.

[monit-reviewer]

RemoveWorkbench re-derives the workbench location with a hardcoded "workbench" literal instead of the package that owns artifact layout. runartifact.Paths already defines WorkbenchDir: filepath.Join(dir, "workbench") (internal/runartifact/runartifact.go:80) and ArtifactPathsFromDir exposes it; reviewrun.go:346 already uses that helper, even for the warning message printed one line above this call. Because os.RemoveAll returns nil when the path does not exist, relocating or renaming the workbench directory would make teardown a silent no-op — the 57 GB accumulation this PR fixes would come back with no warning and no test failure. Fix: return os.RemoveAll(ArtifactPathsFromDir(artifactDir).WorkbenchDir) so teardown and the warning text share one source of truth.

Reply inline to this comment.

[monit-reviewer]

The second run is asserted workbench-free (assert_artifact_shape "$second_artifacts" 0, line 496) but is invoked without any workbench flag, while the script deliberately "leaves normal cr config resolution alone" (usage text). Anyone whose config now sets the newly documented data.keep_workbench: true gets a false invariant=workbench_absent failure on a review that succeeded, turning this harness into a config-dependent check. The first run already controls the knob explicitly with the extra 1; make the second symmetric by appending --keep-workbench=false when keep_workbench is 0 (review_args+=(--keep-workbench=false)), which cmd.Flags().Changed honors as an explicit false.

Reply inline to this comment.

[monit-reviewer]

Automated PR Review

Reviewed commit: 9d9c10a591bf
Profile: pi-ds-gh - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	3
policies:conventions	0
structure:repo-health	3
documentation:docs	1

go:implementation-tests (3 findings)

Major - internal/reviewrun/reviewrun.go:327

Teardown is wired only into continueRun, so a live run that posts successfully through the gate's own path never reclaims its checkout. reviewrun.Run returns gateio.StatusRetryPostsExecuted (internal/reviewrun/reviewrun.go:149) after gateio.executeRetryPosts (internal/gateio/gateio.go:623) calls outbox.Post for the same run and completes it with a deletable outcome (approved/request_changes/comment/nothing_to_review) — precisely the outcomes this allowlist treats as delete-safe. That run's workbench is on disk because the earlier post failed and the new default retained it, so cr review followed by a successful cr review --retry-posts leaves a full checkout behind until the 90-day retention window or an explicit prune: exactly the accumulation this PR exists to stop, for the runs most likely to be retried. It also contradicts docs/checkout-native-review-contract.md ("A live run reaches that state when its outbox post succeeds"), and no test covers it — the three new reviewrun tests only exercise continueRun. Fix: call the same helper from the StatusRepairExecuted, StatusRetryPostsExecuted, StatusApprovalOverride branch using result.Outbox and result.Run (repair/approval allocate fresh run dirs with no workbench, so the call is a harmless no-op there). Test: after a live run whose post fails terminally, run again with Flags.RetryPosts and assert workbench/ is gone; a retry that fails again must retain it.

Minor - internal/pipeline/pipeline.go:739

The new default teardown also applies to the no-leak audit's own review runs, so that suite silently lost filesystem-scan coverage. The harness builds pipeline.Options (internal/cmd/noleak/noleak_test.go:804) and reviewrun.Options (:822) without KeepWorkbench, so every successful review case in TestCommandSurfacesDoNotLeakSeededSecrets now deletes workbench/ under layout.DataRoot before assertOwnedFilesDoNotLeak walks it (:1181). shouldSkipOwnedPath (:1224) only exempts workbench/repo, so the drop is workbench/metadata.json plus any workspace leftovers — content the scan used to read and the one place a seeded secret embedded in checkout metadata (for example a credentialed remote URL) would have been caught. Nothing in the harness asserts the workbench exists, so no test fails; the scan just becomes vacuous for that tree. Fix: set KeepWorkbench: true in the harness's pipelineOpts and reviewrun.Options, as the other workbench-asserting tests did, or add an explicit workbench/metadata.json existence assertion to the review cases so this cannot silently regress again.

Minor - internal/pipeline/pipeline.go:3364

RemoveWorkbench re-derives the workbench location from a hardcoded "workbench" literal instead of the package that owns artifact layout. runartifact.Paths.WorkbenchDir is filepath.Join(dir, "workbench") (internal/runartifact/runartifact.go:80) and ArtifactPathsFromDir exposes it — the same helper the warning line immediately above the reviewrun call site already uses (internal/reviewrun/reviewrun.go:346), and the same helper the new benchmark-select test asserts against. Because os.RemoveAll returns nil for a missing path, a future layout change (rename/relocate, or an extra path segment) would silently stop reclaiming checkouts with no test failing: the run reports success and the warning branch never fires. Fix: take ArtifactPaths (both callers either have one or can compute it) and remove artifacts.WorkbenchDir, or keep the string signature and use ArtifactPathsFromDir(artifactDir).WorkbenchDir; the TrimSpace(artifactDir) == "" guard then becomes unnecessary. TestDryRunRemovesWorkbenchAfterSuccess and TestSelectReclaimsSelectionWorkbench already pin behavior, so this is a path-ownership change only.

structure:repo-health (3 findings)

Minor - internal/config/config.go:426

Invariant: config.Save (internal/config/config.go:916) refuses to write configs it classifies as empty via fileHasNoExplicitContent (config.go:968, returning invalid("config is empty") at config.go:921). That predicate is a hand-maintained allowlist of durable settings; the data section is represented only by cfg.Data.Retention.MaxAgeDays == nil && cfg.Data.Retention.Enforcement == "" (config.go:975-976).

The new DataConfig.KeepWorkbench is not in that list, so a config whose only explicit content is data.keep_workbench: true is classified as empty and Save rejects it. That is exactly the ownership model this PR now documents for the key (docs/init-config-surface.md:71, Ownership = "Direct config-file management"), and the only test coverage is the fully empty File{} case (internal/config/config_test.go:86), so the gap is invisible.

Impact: a minimal user-authored config that only sets the new key cannot be written back by any config.Save caller, and the failure message ("config is empty") points at nothing. This is the kind of enumerated-field guard that silently rots each time a data.* field is added.

Fix: add && !cfg.Data.KeepWorkbench to the predicate and a Save test for a keep_workbench-only config. Better: replace the enumeration with a check over the marshaled YAML (e.g. the serialized File is empty), so future data.* fields cannot drift out of the guard.

Minor - internal/cmd/reviewcmd/reviewcmd.go:132

Invariant: user-facing CLI flags are discoverable from versioned references — the README review flag tables and the Runtime-only Flag Audit in docs/init-config-surface.md — because agents and scripts read those, not the Cobra registration.

Every other flag registered in this file appears in both places (--no-resolve-threads: README.md:1243 and docs/init-config-surface.md:244; --review-base-sha/--review-head-sha: docs/init-config-surface.md:241). --keep-workbench appears in no durable doc: repo-wide it exists only in this registration, reviewcmd_test.go:939, and scripts/verify-large-pr-review.sh:288. The new inventory row docs/init-config-surface.md:71 also still states Ownership = "Direct config-file management" even though a per-run override now exists, unlike the config.profiles.<name>.fast row that documents its --fast/--no-fast override in the same cell.

Impact: the only flag that prevents workbench deletion is undiscoverable from the map humans and agents actually read, and the config-surface doc now under-states the key's override surface. Nothing enforces README flag coverage, so this drift is unguarded.

Fix: add --keep-workbench to the README review flag table and to the flag-audit rows, update the inventory row's Ownership cell to mirror the fast precedent, and state that an explicit --keep-workbench=false forces deletion when config sets it true. A small test asserting every registered cr review flag appears in README.md would keep this from recurring.

Minor - internal/cmd/respondcmd/respondcmd.go:118

Invariant: fields on app.OpenRequest are a per-command request contract; a command should only populate knobs its runtime actually consumes.

The respond path is inert for this field. cr respond opens a runtime and calls reviewRunner.Respond (internal/app/runtime.go:654), which dispatches with only the r.respond options (runtime.go:656), and internal/threadrespond never creates, references, or removes a workbench (no workbench hits in that package; it writes run artifacts only). app.OpenRequest.KeepWorkbench is consumed only by buildReviewRunner's pipeline/reviewrun options (runtime.go:414, runtime.go:447). The new assertion at internal/cmd/respondcmd/respondcmd_test.go:126 therefore pins a wire with no behavior, and would keep passing even if the knob disappeared from the pipeline entirely.

Impact: the PR body and docs describe data.keep_workbench as threaded through review/benchmark/respond; the respond leg is a no-op that future readers and agents will read as a live contract, and a test now enshrines it.

Fix: drop KeepWorkbench from the respond OpenRequest and delete that table case, or keep the wiring with an explicit comment that respond has no workbench lifecycle and replace the assertion with one that pins that fact (respond runs leave no workbench/ directory).

documentation:docs (1 finding)

Minor - docs/init-config-surface.md:71

The new config.data.keep_workbench row records the Scripted owner as Direct config-file management, but this PR also adds cr review --keep-workbench (internal/cmd/reviewcmd/reviewcmd.go:132, applied only when cmd.Flags().Changed("keep-workbench") reports it, so an explicit config true is not clobbered). That is a per-invocation override of this durable field, and the doc's own precedent for recording one is the adjacent row at line 60: Direct config-file management; cr review --fastand--no-fast override it per invocation. This doc is the binding contract for the init/config surface (#176-#187) and the "final non-interactive ownership decision", so leaving the flag out makes the row incomplete in exactly the place scripted-install authors will read it. Concrete fix: (1) make the Scripted owner cell read Direct config-file management; cr review --keep-workbench overrides it per invocation.; (2) add --keep-workbench to the Runtime-only Flag Audit, because the six existing cr review rows (lines 237-244) enumerate every other review flag and a reader auditing cr review would not see this one — e.g. append a row | cr reviewartifact retention |--keep-workbench| One-shot override of durabledata.keep_workbench; the durable default stays in config. |.

Reviewer Coverage

• go:implementation-tests — complete (constrained); inspected 17 assigned files (22 inspected across reviewers): internal/app/runtime.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/executor_test.go, internal/cmd/benchmarkcmd/select.go, internal/cmd/benchmarkcmd/select_test.go, internal/cmd/initcmd/initcmd_test.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/respondcmd/respondcmd_test.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_test.go, internal/config/config.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/workbench_cleanup_test.go, internal/pipeline/workbench_integration_test.go, internal/reviewrun/reviewrun.go, internal/reviewrun/reviewrun_test.go; skipped: none; constraints: I did not run go build/go test; no test results were observed, so claims about which existing tests still pass and about CI state are inferred from code paths. I read the full pinned diff plus targeted head-file reads at the teardown seams (pipeline.execute, reviewrun.Run/continueRun, gateio.executeRetryPosts, config.Save) rather than whole files. Scope limited to Go implementation quality and test adequacy; doc, README, and shell-harness wording issues were deliberately excluded. internal/cmd/noleak/noleak_test.go is not an assigned changed file, so that finding is anchored to the changed teardown line in internal/pipeline/pipeline.go.
• policies:conventions — complete (constrained); inspected 9 assigned files (22 inspected across reviewers): README.md, docs/init-config-surface.md, docs/review-guidance.md, internal/app/runtime.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/reviewcmd/reviewcmd.go, internal/config/config.go, scripts/verify-large-pr-review.sh; skipped: none; constraints: No build, test, lint, or script execution was available; all findings rest on static reading of the diff and repository sources. Repo-local agent guidance (.codereview/agents/) is scope/triage configuration, not a behavioral convention contract, so it was used only as context. Reviewed the pinned PR diff plus repo-local convention sources: AGENTS.md, CLAUDE.md, docs/development.md, docs/review-guidance.md, docs/init-config-surface.md, scripts/README.md, and the repo-local .codereview agent prompts. Several concerns already carry unresolved inline threads on this diff (init-config-surface row wording, README flag reference, config.Save emptiness guard, respond-path plumbing); findings below are limited to what the repo-local convention sources make concrete. Sibling checkouts of the canonical shared standards are not reachable from this workbench: ../cli-common/docs and ../.github cannot be read (path traversal denied), so shared-policy compliance is judged only from repo-local breadcrumbs and docs.
• structure:repo-health — complete (constrained); inspected 5 assigned files (22 inspected across reviewers): internal/app/runtime.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/respondcmd/respondcmd.go, internal/cmd/reviewcmd/reviewcmd.go, internal/config/config.go; skipped: none; constraints: No tests were run; all findings are static analysis of the diff and its consumers. Scope was limited to the five assigned files; risks in internal/pipeline/pipeline.go, internal/cmd/benchmarkcmd/select.go, scripts/, and docs/ were left to reviewers who own those anchors. cr_read returned only ~20-40 characters of content per call, so file inspection relied on cr_search line hits plus the pinned diff rather than full-file reads.
• documentation:docs — complete (constrained); inspected 2 assigned files (22 inspected across reviewers): docs/checkout-native-review-contract.md, docs/init-config-surface.md; skipped: none; constraints: Docs-only scope: the two assigned files; code claims were cross-checked against the pinned diff plus reads/searches of internal/pipeline, internal/reviewrun, internal/cmd/benchmarkcmd/select.go, internal/config, and internal/cmd/reviewcmd. No tests, CLI runs, or script execution were performed; all behavior claims are from source inspection of the pinned review SHA, not from observed runs. The Runtime-only Flag Audit table (docs/init-config-surface.md:230-244) is unchanged by this PR, so the finding is anchored to the new inventory row at line 71.

Inspected files (22)

• README.md
• docs/checkout-native-review-contract.md
• docs/init-config-surface.md
• docs/review-guidance.md
• internal/app/runtime.go
• internal/cmd/benchmarkcmd/executor.go
• internal/cmd/benchmarkcmd/executor_test.go
• internal/cmd/benchmarkcmd/select.go
• internal/cmd/benchmarkcmd/select_test.go
• internal/cmd/initcmd/initcmd_test.go
• internal/cmd/respondcmd/respondcmd.go
• internal/cmd/respondcmd/respondcmd_test.go
• internal/cmd/reviewcmd/reviewcmd.go
• internal/cmd/reviewcmd/reviewcmd_test.go
• internal/config/config.go
• internal/pipeline/pipeline.go
• internal/pipeline/pipeline_test.go
• internal/pipeline/workbench_cleanup_test.go
• internal/pipeline/workbench_integration_test.go
• internal/reviewrun/reviewrun.go
• internal/reviewrun/reviewrun_test.go
• scripts/verify-large-pr-review.sh

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 7m 52s | $0.01 | opencode-go/deepseek-v4.1-flash | cr dev

Field	Value
Model	opencode-go/deepseek-v4.1-flash
Reviewers	go:implementation-tests, policies:conventions, structure:repo-health, documentation:docs
Engine	pi_rpc · opencode-go/deepseek-v4.1-flash
Reviewed by	cr · monit-reviewer
Duration	7m 52s wall · 16m 47s compute
Cost	$0.01
Tokens	12.1k in / 15.6k out

Per-workstream usage

• go:implementation-tests — opencode-go/deepseek-v4.1-flash
  • In: 1.2k
  • Out: 5.4k
  • Cache read: 121.2k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 7m 14s
• policies:conventions — opencode-go/deepseek-v4.1-flash
  • In: 173
  • Out: 2.5k
  • Cache read: 112.6k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 4m 04s
• structure:repo-health — opencode-go/deepseek-v4.1-flash
  • In: 260
  • Out: 3.0k
  • Cache read: 70.7k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 3m 22s
• documentation:docs — opencode-go/deepseek-v4.1-flash
  • In: 257
  • Out: 1.6k
  • Cache read: 73.2k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 1m 52s
• orchestrator-rollup — opencode-go/deepseek-v4.1-flash
  • In: 10.2k
  • Out: 3.1k
  • Cache read: 0
  • Cache create: 0
  • Cost: $0.00
  • Duration: 13s

[monit-reviewer]

Invariant: user-facing CLI flags are discoverable from versioned references — the README review flag tables and the Runtime-only Flag Audit in docs/init-config-surface.md — because agents and scripts read those, not the Cobra registration.

Every other flag registered in this file appears in both places (--no-resolve-threads: README.md:1243 and docs/init-config-surface.md:244; --review-base-sha/--review-head-sha: docs/init-config-surface.md:241). --keep-workbench appears in no durable doc: repo-wide it exists only in this registration, reviewcmd_test.go:939, and scripts/verify-large-pr-review.sh:288. The new inventory row docs/init-config-surface.md:71 also still states Ownership = "Direct config-file management" even though a per-run override now exists, unlike the config.profiles.<name>.fast row that documents its --fast/--no-fast override in the same cell.

Impact: the only flag that prevents workbench deletion is undiscoverable from the map humans and agents actually read, and the config-surface doc now under-states the key's override surface. Nothing enforces README flag coverage, so this drift is unguarded.

Fix: add --keep-workbench to the README review flag table and to the flag-audit rows, update the inventory row's Ownership cell to mirror the fast precedent, and state that an explicit --keep-workbench=false forces deletion when config sets it true. A small test asserting every registered cr review flag appears in README.md would keep this from recurring.

Reply inline to this comment.

[zzwong]

Fixed in f3bebd3. --keep-workbench is now in the README review flag table and in the Runtime-only Flag Audit, matching the surrounding row format, and both state that an explicit --keep-workbench=false forces deletion even when config sets it true.

[monit-reviewer]

Thanks — the README review flag table and Runtime-only Flag Audit additions cover the main discoverability gap, and stating that explicit --keep-workbench=false forces deletion is the right clarification.

Two items from the finding are still open, and I haven't verified the f3bebd3 diff myself:

1. The inventory row at docs/init-config-surface.md:71 still lists Ownership as "Direct config-file management" even though a per-run --keep-workbench override now exists. The config.profiles.<name>.fast row documents its --fast/--no-fast override in that same cell — mirror that precedent so the override surface isn't understated.
2. The recurrence guard: a small test asserting every registered cr review flag appears in README.md. Without it, this drift is unguarded and will come back silently on the next flag.

If both are in f3bebd3 or a follow-up, point me at the lines and I'll close this out.

[monit-reviewer]

Automated PR Review

Reviewed commit: f3bebd3c46da
Profile: pi-ds-gh - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	3
policies:conventions	1
structure:repo-health	1
documentation:docs	0

go:implementation-tests (3 findings)

Minor - internal/reviewrun/reviewrun.go:340

The deletable-outcome allowlist is exercised for exactly one outcome: every new workbench assertion in reviewrun_test.go (lines 84-189) plans reviewplan.OutcomeComment. Nothing pins that ledger.OutcomeApproved, OutcomeRequestChanges, and OutcomeNothingToReview belong in this clause, and nothing asserts that OutcomeAborted/OutcomeIncomplete retain (only OutcomeFailed is covered, incidentally, by the failing-post tests). A dropped or misspelled entry here fails in the silent direction: the run keeps its full checkout and every existing test stays green — the precise accumulation this PR exists to stop, on the approve/request-changes paths that most live runs take. Note the retain clause below is dead relative to default, so this one clause carries the entire contract. Fix: add a focused TestRemoveWorkbenchAfterPostOutcomeClassification in reviewrun_test.go that builds an artifact dir under t.TempDir(), writes workbench/repo, calls removeWorkbenchAfterPost(Options{}, outbox.Result{Outcome: tc.outcome}, ledger.Run{ArtifactPath: dir}) and asserts presence/absence across all eight ledger.Outcome values.

Nits - internal/cmd/noleak/noleak_test.go:289

The guard that keeps the workbench half of the owned-file scan from being vacuous is itself gated on a test-case display name: if strings.HasPrefix(tc.name, "review ") && !tc.wantErr. Because assertOwnedFilesDoNotLeak silently passes over an empty data root, this reintroduces the failure mode the guard was added to prevent — rename one of the five review cases, or add a successful review case named review-dry-run/live review, and the workbench scan stops being covered with no failing test. Fix: add an explicit wantWorkbench bool field to commandCase, set it on the review cases, and gate the assertion on the field so the coupling lives in the table instead of in a string match. (The pre-existing Chdir HasPrefix(tc.name, "review ") at line 263 has the same shape but predates this change.)

Minor - internal/cmd/reviewcmd/reviewcmd_test.go:939

This table covers config-true-without-flag, config-false-without-flag, and --keep-workbench (true) with config false, but never a Changed flag carrying a false value while config is true. That is the only case where the cmd.Flags().Changed("keep-workbench") guard at reviewcmd.go:294 affects the outcome in the deleting direction, and it is a documented contract: README.md:1244 and README.md:1587 both state an explicit --keep-workbench=false forces deletion even when data.keep_workbench: true, and docs/init-config-surface.md:71 repeats it in the flag audit. A mistake like if cmd.Flags().Changed("keep-workbench") && flags.keepWorkbench would pass all three existing rows and silently retain checkouts for scripted --keep-workbench=false callers. Fix: add {name: "explicit false overrides config true", configValue: true, flags: []string{"--keep-workbench=false"}, want: false} to this table.

policies:conventions (1 finding)

Minor - docs/init-config-surface.md:245

--keep-workbench now appears in this Runtime-only Flag Audit row and in both cr review README flag surfaces, but nothing enforces that correspondence, so the drift this PR had to fix by hand can silently return. The config half of the same change is guarded: internal/config/init_surface_doc_test.go fails whenever a durable config.* leaf path is missing from the Durability inventory (initSurfaceInventoryPaths), which is why row 71 could not be forgotten. The flag half has no analogue — a repo-wide search finds no test that reads README.md or this audit table, and no Flags().VisitAll usage — yet this change needed three hand edits (README policy/output table, README supported-values table, this audit row) and only landed after review feedback called the missing rows out.

Smallest fix that preserves intent: add a flag-parity assertion next to the existing doc-contract test, modeled on initSurfaceInventoryPaths. Register the cr review command, walk its flags, and assert every flag name appears in README.md's cr review tables and in this audit table (scope the check to one command first if a whole-tree version feels too broad); an unlisted flag then fails CI instead of shipping. No behavior change is implied.

structure:repo-health (1 finding)

Minor - internal/config/config.go:977

fileHasNoExplicitContent (internal/config/config.go:968-977) is a hand-maintained mirror of the File schema, and this PR had to add a third arm for DataConfig.KeepWorkbench to keep config.Save from rejecting a valid keep-workbench-only file with config: invalid: config is empty (internal/config/config.go:921). The predicate has no mechanical link to the schema, while the neighbouring docs inventory is reflection-guarded: configSchemaLeafPaths(reflect.TypeOf(File{}), "config") in internal/config/init_surface_doc_test.go:67 fails the build when a durable key has no inventory row. Impact: the next data.* key (or any new top-level durable block) that forgets this allowlist produces a spurious hard failure for users, and it is caught only if a key-specific regression test such as the new TestSaveAcceptsKeepWorkbenchOnlyConfig happens to be written. Fix, in order of preference: (1) single-source the emptiness check by marshaling the File and testing whether the document has any explicit mapping path — yamlDocumentHasMappingPath already exists immediately below this function; or (2) if that refactor is out of scope, add the enforceability guard now in this package: reuse configSchemaLeafPaths, set one leaf at a time to a non-zero value, and assert Save never returns the empty-config error. Either way the per-key allowlist arm disappears.

Reviewer Coverage

• go:implementation-tests — complete (constrained); inspected 17 assigned files (22 inspected across reviewers): internal/app/runtime.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/benchmarkcmd/executor_test.go, internal/cmd/benchmarkcmd/select.go, internal/cmd/benchmarkcmd/select_test.go, internal/cmd/initcmd/initcmd_test.go, internal/cmd/noleak/noleak_test.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_test.go, internal/config/config.go, internal/config/config_test.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/workbench_cleanup_test.go, internal/pipeline/workbench_integration_test.go, internal/reviewrun/reviewrun.go, internal/reviewrun/reviewrun_test.go; skipped: none; constraints: I inspected internal/gateio and internal/benchmark only for call-site context; findings are anchored only to files in the pinned change. Read-only review of the pinned diff at head f3bebd3; I did not run go test or the shell harness, so all coverage claims are from reading test code, not from observed results. Thread resolutions in the discussion were taken as reported (fixes claimed in f3bebd3) and were spot-checked by reading the head files, not by re-running the mutations the author described. scripts/verify-large-pr-review.sh is outside the assigned file set and was not reviewed.
• policies:conventions — complete (constrained); inspected 8 assigned files (22 inspected across reviewers): README.md, docs/init-config-surface.md, docs/review-guidance.md, internal/app/runtime.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/reviewcmd/reviewcmd.go, internal/config/config.go, scripts/verify-large-pr-review.sh; skipped: none; constraints: No tests, linters, or scripts/verify-large-pr-review.sh were executed. Behavior claims come from reading the pinned diff and head sources only. Sibling shared-standards checkouts are unreachable in this workbench: cr_list on .. and ../cli-common/docs both returned 'path traversal denied'. Shared Open CLI Collective standards could not be consulted; checks used repo-local contracts. Started from the pinned cr_diff (succeeded), then read assigned heads. docs/checkout-native-review-contract.md, internal/pipeline/*, and internal/reviewrun/* are outside the assigned set, so teardown-convention drift in those files is not reported here. Two residual gaps in assigned files are already tracked by open review threads and are not re-filed: the registered-flag/README guard test request on internal/cmd/reviewcmd/reviewcmd.go, and the script's dependence on a user config leaving data.keep_workbench false at `scripts/verify-large-pr...
• structure:repo-health — complete (constrained); inspected 4 assigned files (22 inspected across reviewers): internal/app/runtime.go, internal/cmd/benchmarkcmd/executor.go, internal/cmd/reviewcmd/reviewcmd.go, internal/config/config.go; skipped: none; constraints: Assigned scope was the four changed files; this is a repo-health review of structural drift, not exhaustive issue discovery. The discussion's prior findings on config.go:426 and reviewcmd.go:132 were read; I did not independently re-run any test in this environment. Unassigned changed files (pipeline.go, reviewrun.go, select.go, docs, scripts) were only considered through their diff hunks for context. cr_read returns character-ranged windows rather than whole files, so head-file inspection was targeted (small ranged reads plus cr_search) rather than full-file review.
• documentation:docs — complete (constrained); inspected 2 assigned files (22 inspected across reviewers): docs/checkout-native-review-contract.md, docs/init-config-surface.md; skipped: none; constraints: Behavior claims were verified by searching the head checkout, not by running Go tests: I did not execute the test suite. Evaluated and deliberately not filed: the note at docs/checkout-native-review-contract.md:140 ('still own their artifact directory choice') could be read as pre-fix cleanup language, but line 141 scopes it to caller-owned artifact roots, so I judged it accurate. Scope was the two assigned files. The same workbench contract also lives in README.md, docs/review-guidance.md, and scripts/verify-large-pr-review.sh; those are outside my allowed file set. cr_diff returned only a small window per call, so I reconstructed both doc hunks incrementally (README->contract @ -107,8 +107,17; init-config-surface @ -68,6 +68,7 and @ -241,6 +242,7) instead of reading the whole 52 KB pinned diff.

Inspected files (22)

• README.md
• docs/checkout-native-review-contract.md
• docs/init-config-surface.md
• docs/review-guidance.md
• internal/app/runtime.go
• internal/cmd/benchmarkcmd/executor.go
• internal/cmd/benchmarkcmd/executor_test.go
• internal/cmd/benchmarkcmd/select.go
• internal/cmd/benchmarkcmd/select_test.go
• internal/cmd/initcmd/initcmd_test.go
• internal/cmd/noleak/noleak_test.go
• internal/cmd/reviewcmd/reviewcmd.go
• internal/cmd/reviewcmd/reviewcmd_test.go
• internal/config/config.go
• internal/config/config_test.go
• internal/pipeline/pipeline.go
• internal/pipeline/pipeline_test.go
• internal/pipeline/workbench_cleanup_test.go
• internal/pipeline/workbench_integration_test.go
• internal/reviewrun/reviewrun.go
• internal/reviewrun/reviewrun_test.go
• scripts/verify-large-pr-review.sh

7 PR discussion threads considered. 6 summarized; 6 resolved.

---

Completed in 6m 59s | $0.01 | opencode-go/deepseek-v4.1-flash | cr dev

Field	Value
Model	opencode-go/deepseek-v4.1-flash
Reviewers	go:implementation-tests, policies:conventions, structure:repo-health, documentation:docs
Engine	pi_rpc · opencode-go/deepseek-v4.1-flash
Reviewed by	cr · monit-reviewer
Duration	6m 59s wall · 16m 05s compute
Cost	$0.01
Tokens	9.1k in / 10.7k out

Per-workstream usage

• go:implementation-tests — opencode-go/deepseek-v4.1-flash
  • In: 281
  • Out: 3.4k
  • Cache read: 144.0k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 4m 10s
• policies:conventions — opencode-go/deepseek-v4.1-flash
  • In: 398
  • Out: 836
  • Cache read: 114.8k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 5m 06s
• structure:repo-health — opencode-go/deepseek-v4.1-flash
  • In: 249
  • Out: 3.8k
  • Cache read: 60.8k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 2m 39s
• documentation:docs — opencode-go/deepseek-v4.1-flash
  • In: 344
  • Out: 1.3k
  • Cache read: 88.7k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 4m 01s
• orchestrator-rollup — opencode-go/deepseek-v4.1-flash
  • In: 7.9k
  • Out: 1.4k
  • Cache read: 0
  • Cache create: 0
  • Cost: $0.00
  • Duration: 7s

[monit-reviewer]

This table covers config-true-without-flag, config-false-without-flag, and --keep-workbench (true) with config false, but never a Changed flag carrying a false value while config is true. That is the only case where the cmd.Flags().Changed("keep-workbench") guard at reviewcmd.go:294 affects the outcome in the deleting direction, and it is a documented contract: README.md:1244 and README.md:1587 both state an explicit --keep-workbench=false forces deletion even when data.keep_workbench: true, and docs/init-config-surface.md:71 repeats it in the flag audit. A mistake like if cmd.Flags().Changed("keep-workbench") && flags.keepWorkbench would pass all three existing rows and silently retain checkouts for scripted --keep-workbench=false callers. Fix: add {name: "explicit false overrides config true", configValue: true, flags: []string{"--keep-workbench=false"}, want: false} to this table.

Reply inline to this comment.

[monit-reviewer]

fileHasNoExplicitContent (internal/config/config.go:968-977) is a hand-maintained mirror of the File schema, and this PR had to add a third arm for DataConfig.KeepWorkbench to keep config.Save from rejecting a valid keep-workbench-only file with config: invalid: config is empty (internal/config/config.go:921). The predicate has no mechanical link to the schema, while the neighbouring docs inventory is reflection-guarded: configSchemaLeafPaths(reflect.TypeOf(File{}), "config") in internal/config/init_surface_doc_test.go:67 fails the build when a durable key has no inventory row. Impact: the next data.* key (or any new top-level durable block) that forgets this allowlist produces a spurious hard failure for users, and it is caught only if a key-specific regression test such as the new TestSaveAcceptsKeepWorkbenchOnlyConfig happens to be written. Fix, in order of preference: (1) single-source the emptiness check by marshaling the File and testing whether the document has any explicit mapping path — yamlDocumentHasMappingPath already exists immediately below this function; or (2) if that refactor is out of scope, add the enforceability guard now in this package: reuse configSchemaLeafPaths, set one leaf at a time to a non-zero value, and assert Save never returns the empty-config error. Either way the per-key allowlist arm disappears.

Reply inline to this comment.

[monit-reviewer]

The guard that keeps the workbench half of the owned-file scan from being vacuous is itself gated on a test-case display name: if strings.HasPrefix(tc.name, "review ") && !tc.wantErr. Because assertOwnedFilesDoNotLeak silently passes over an empty data root, this reintroduces the failure mode the guard was added to prevent — rename one of the five review cases, or add a successful review case named review-dry-run/live review, and the workbench scan stops being covered with no failing test. Fix: add an explicit wantWorkbench bool field to commandCase, set it on the review cases, and gate the assertion on the field so the coupling lives in the table instead of in a string match. (The pre-existing Chdir HasPrefix(tc.name, "review ") at line 263 has the same shape but predates this change.)

Reply inline to this comment.

[monit-reviewer]

The deletable-outcome allowlist is exercised for exactly one outcome: every new workbench assertion in reviewrun_test.go (lines 84-189) plans reviewplan.OutcomeComment. Nothing pins that ledger.OutcomeApproved, OutcomeRequestChanges, and OutcomeNothingToReview belong in this clause, and nothing asserts that OutcomeAborted/OutcomeIncomplete retain (only OutcomeFailed is covered, incidentally, by the failing-post tests). A dropped or misspelled entry here fails in the silent direction: the run keeps its full checkout and every existing test stays green — the precise accumulation this PR exists to stop, on the approve/request-changes paths that most live runs take. Note the retain clause below is dead relative to default, so this one clause carries the entire contract. Fix: add a focused TestRemoveWorkbenchAfterPostOutcomeClassification in reviewrun_test.go that builds an artifact dir under t.TempDir(), writes workbench/repo, calls removeWorkbenchAfterPost(Options{}, outbox.Result{Outcome: tc.outcome}, ledger.Run{ArtifactPath: dir}) and asserts presence/absence across all eight ledger.Outcome values.

Reply inline to this comment.

[monit-reviewer]

--keep-workbench now appears in this Runtime-only Flag Audit row and in both cr review README flag surfaces, but nothing enforces that correspondence, so the drift this PR had to fix by hand can silently return. The config half of the same change is guarded: internal/config/init_surface_doc_test.go fails whenever a durable config.* leaf path is missing from the Durability inventory (initSurfaceInventoryPaths), which is why row 71 could not be forgotten. The flag half has no analogue — a repo-wide search finds no test that reads README.md or this audit table, and no Flags().VisitAll usage — yet this change needed three hand edits (README policy/output table, README supported-values table, this audit row) and only landed after review feedback called the missing rows out.

Smallest fix that preserves intent: add a flag-parity assertion next to the existing doc-contract test, modeled on initSurfaceInventoryPaths. Register the cr review command, walk its flags, and assert every flag name appears in README.md's cr review tables and in this audit table (scope the check to one command first if a whole-tree version feels too broad); an unlisted flag then fails CI instead of shipping. No behavior change is implied.

Reply inline to this comment.