test(integration): validate combined draft stack [DO NOT MERGE]

.github/workflows/ci.yml

@@ -48,6 +48,18 @@ jobs:
         with:
           go-version-file: go.mod
 
+  test-race:
+    runs-on: ubuntu-latest
+    timeout-minutes: 20
+    env:
+      CGO_ENABLED: "1" # Required by Go's race detector.
+    steps:
+      - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5.1.0
+      - uses: open-cli-collective/.github/actions/go-test@b6382514b809d960c6fbdc85745a60c14cde04f1 # shared .github
+        with:
+          go-version-file: go.mod
+          target: test-race
+
   test-no1password-optout:
     runs-on: ubuntu-latest
     env:
@@ -70,6 +82,24 @@ jobs:
           go-version-file: go.mod
           target: test-static-smoke
 
+  pi-runtime:                              # installed Pi RPC runtime gate — not a required check
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5.1.0
+      - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0
+        with:
+          node-version: 22.22.3            # Pi 1.0 requires Node >= 22.19.0
+      # The Makefile owns the Pi version; test-pi-runtime fails if it differs.
+      # Read it first so a failed lookup stops the step instead of installing
+      # an unpinned (latest) Pi.
+      - run: |
+          pi_version="$(make -s pi-runtime-version)"
+          npm install -g --ignore-scripts "@earendil-works/pi-coding-agent@${pi_version}"
+      - uses: open-cli-collective/.github/actions/go-test@b6382514b809d960c6fbdc85745a60c14cde04f1 # shared .github
+        with:
+          go-version-file: go.mod
+          target: test-pi-runtime
+
   lint:
     runs-on: ubuntu-latest
     steps:

BENCHMARKING.md

@@ -130,6 +130,7 @@ cases:
     pr: https://github.com/OWNER/REPO/pull/123
     review_base_sha: 1111111
     review_head_sha: 2222222
+    without_discussion: true # default for pinned cases; false opts into saved sessions
     expected_base_sha: abc1234
     expected_head_sha: def5678
     anchors:
@@ -148,6 +149,34 @@ metadata for downstream reports and graders; they do not change what gets
 reviewed. All SHA fields must be non-empty 7 to 64 character hexadecimal SHAs
 when present.
 
+`without_discussion` is an optional per-case boolean for full-pipeline
+`benchmark run`. It defaults to `true` when both review SHAs are pinned, and
+`false` for unpinned cases. Setting it to `true` requires both review SHAs.
+Discussion-free runs do not resume or overwrite saved orchestrator/reviewer
+sessions or the PR's reviewer cohort, so candidates do not inherit earlier
+reviews. Both subprocess and `--in-process` runs apply the same setting.
+Explicit `false` opts into the normal saved-session behavior, including for
+pinned cases. Expected SHA metadata alone does not enable discussion-free mode.
+
+For cross-version comparisons with `--cr-bin`, isolated cases require a binary
+that supports `cr review --without-discussion` and reports
+`run.without_discussion: true` in review JSON. An older binary may reject the
+flag; a successful child exit without that marker is recorded as
+`discussion_isolation_unverified` and counted as a failed benchmark cell.
+The harness preserves the child's actual exit code and raw output and never
+retries without the flag. To deliberately benchmark an older binary, set
+`without_discussion: false` and account for possible saved-session contamination.
+Doctor and selected-case artifacts record the effective setting. Per-run
+metrics, manifests, summaries, and comparisons distinguish
+`requested_without_discussion` from `without_discussion_verified`; a missing
+marker is never treated as proof of isolation. `benchmark compare` rechecks
+the current review artifact; replacing or corrupting it invalidates prior
+verification and fails a cell that requested discussion-free execution.
+
+Selector-only `benchmark select` already uses a caller-owned selection run with
+no saved review sessions. It preserves this case setting in its recipe but does
+not need the full-review flag.
+
 Candidate `profile` must reference a configured profile. Candidate PR hosts must
 match the candidate profile's Git host. For the current full-pipeline
 `validate`, `doctor`, and `run` commands, candidates must declare explicit
@@ -270,8 +299,9 @@ When set on the case, `run` also passes:
 |------------|-------------|
 | `review_base_sha` | `--review-base-sha <sha>` |
 | `review_head_sha` | `--review-head-sha <sha>` |
+| `without_discussion` | `--without-discussion` when effectively true; defaults to true for pinned cases |
 
-Unset fields are omitted. Benchmark candidate artifacts record reviewer
+Other unset fields are omitted. Benchmark candidate artifacts record reviewer
 `effort_source` as `inherited` or `override`, so reports can distinguish the
 two execution recipes even when the inherited effort is resolved later per
 agent. Effort values are `low`, `medium`, `high`, `xhigh`, or `max`; validation
@@ -280,8 +310,8 @@ rejects levels unsupported by the candidate profile's runtime. `claude_cli`,
 `anthropic_api` supports through `high`. Effective extended-effort support
 depends on the selected model and, for CLI adapters, the installed CLI.
 
-Posting, retry, approval, thread-resolution, session,
-and live-review flags are never taken from the suite.
+Apart from the per-case `without_discussion` setting, posting, retry, approval,
+thread-resolution, session, and live-review flags are never taken from the suite.
 
 `--cr-bin <path>` selects the binary used for child review runs. If omitted,
 `run` uses the current `cr` binary. `doctor` reports the binary it would use.

Makefile

@@ -1,4 +1,4 @@
-.PHONY: all build test test-cover test-static-smoke lint fmt tidy deps check install snapshot package-render-check release clean
+.PHONY: all build test test-race test-cover test-static-smoke test-pi-runtime pi-runtime-version check-pi-runtime-version lint fmt tidy deps check install snapshot package-render-check release clean
 
 # Standard keyring tags: enable 1Password support, keep passage disabled.
 GOFLAGS ?= -tags=keyring_nopassage
@@ -12,12 +12,36 @@ build:
 test:
 	go test -v ./...
 
+# Exercise SQLite startup/migration contention repeatedly without racing the
+# entire suite. Keep package execution and test scheduling bounded in CI.
+test-race:
+	go test -race -count=25 -cpu=4 -parallel=4 -p=1 -timeout=5m ./internal/ledger/... ./internal/dbmig/...
+
 test-cover:
 	go test -coverprofile=coverage.out ./...
 
 test-static-smoke:
 	go test -v ./internal/... ./cmd/... -count=1
 
+# Exact Pi version the installed-runtime gate requires. CI installs this
+# version; test-pi-runtime fails, rather than skips, when `pi --version` on PATH
+# reports anything else or Pi is missing.
+PI_RUNTIME_VERSION ?= 1.0.0
+
+# A blank pin would make the runtime tests skip and let npm install the latest
+# Pi, so both targets below require exactly one version.
+check-pi-runtime-version:
+	@if [ "$(words $(PI_RUNTIME_VERSION))" != 1 ]; then \
+		echo "PI_RUNTIME_VERSION must be exactly one Pi version" >&2; \
+		exit 1; \
+	fi
+
+test-pi-runtime: check-pi-runtime-version
+	CR_PI_RUNTIME_VERSION=$(strip $(PI_RUNTIME_VERSION)) go test -v -race -count=1 -run 'TestPiRPCRuntime|TestPiRPCReviewerExtensionLoadsInInstalledPi' ./internal/llmadapters
+
+pi-runtime-version: check-pi-runtime-version
+	@echo $(strip $(PI_RUNTIME_VERSION))
+
 lint:
 	golangci-lint run
 

README.md

@@ -119,16 +119,33 @@ cr init --non-interactive \
 ```
 
 Setup with Pi's local RPC runtime. Install Pi's coding agent and make sure the
-`pi` binary is available on `PATH` before running `cr review`. New installs
-should use the current npm package (`@earendil-works/pi-coding-agent`); existing
-installs from the previous npm scope can also work when CR's compatibility
-preflight confirms the reviewer controls it requires: RPC/system-prompt mode;
-`--no-builtin-tools` with an exact `--tools` allowlist; explicit `--extension` loading while
-`--no-extensions` disables discovery; and `--no-context-files`, `--no-approve`,
-`--no-skills`, `--no-prompt-templates`, `--no-themes`, and `--no-session`.
-CR preflights these capabilities before starting a Pi reviewer and returns an
+`pi` binary is available on `PATH` before running `cr review`. Use Pi 1.0 or
+newer from the current npm package (`@earendil-works/pi-coding-agent`). CR
+relies on Pi 1.0's RPC completion contract: it keeps reading events through
+automatic retries and compaction recovery until `agent_settled`, takes the
+answer only from the final completed assistant message, and sums usage from
+each completed assistant message and compaction summary. A final provider
+error or abort fails the task with Pi's diagnostic. Older runtimes, including
+installs from the previous npm scope, that end a run without `agent_settled`
+fail with an incompatible-runtime error after a short grace period instead of
+returning a result.
+
+CR also preflights the reviewer controls it requires before starting a Pi
+reviewer: RPC/system-prompt mode; `--no-builtin-tools` with an exact `--tools`
+allowlist; explicit `--extension` loading while `--no-extensions` disables
+discovery; and `--no-context-files`, `--no-approve`, `--no-skills`,
+`--no-prompt-templates`, `--no-themes`, and `--no-session`. It returns an
 incompatible-runtime error when any control is unavailable.
 
+Every CR-owned Pi request disables `AGENTS.md`/`CLAUDE.md` context discovery,
+supplies its own system prompt, and passes an explicit empty
+`--append-system-prompt` source. This also suppresses ambient `SYSTEM.md` and
+`APPEND_SYSTEM.md` inputs, including those in the selected agent directory.
+CR leaves Pi's selected agent directory and authentication storage in place;
+credentials are never copied into disposable scratch state, so Pi can persist
+OAuth refresh updates to its original store. Resource discovery remains
+disabled, and only reviewers load the CR-owned read-only tool extension.
+
 ```bash
 cr init --non-interactive \
   --llm-provider pi \
@@ -1235,6 +1252,7 @@ Review selection and execution flags:
 | `--reviewer-effort <effort>` | Override reviewer-stage effort with `low`, `medium`, `high`, `xhigh`, or `max`, subject to runtime support. Available for dry-run, no-post, and live reviews. |
 | `--review-base-sha <sha>` | Review this base commit SHA instead of the PR's current base SHA. Requires `--review-head-sha` and `--dry-run` or `--no-post`. |
 | `--review-head-sha <sha>` | Review this head commit SHA instead of the PR's current head SHA. Requires `--review-base-sha` and `--dry-run` or `--no-post`. |
+| `--without-discussion` | Replay the pinned review as a first pass. cr reads no review threads, thread outcomes, issue comments, or prior reviews, and neither resumes nor updates the PR's orchestrator session or reviewer cohort sessions, so the discussion is not in any selection, reviewer, or rollup prompt; the PR title, description, and diff still are. Reviewers also get a narrower tool surface: the review checkouts have no git remotes, `claude_cli` reviewers are denied WebFetch, WebSearch, and Bash commands such as `gh`, `curl`, `wget`, and `git fetch`/`pull`/`ls-remote`, and `codex_cli` reviewers run with web search and sandbox network access off. That blocks the obvious lookups but is not a network sandbox, so a reviewer with a shell could still reach the network another way. The run marker (`review-run.json`), dossier discussion artifacts, and `--json` run output record `without_discussion: true`, and such a run never resumes an incomplete run made with discussion (or the reverse). Requires `--review-base-sha`, `--review-head-sha`, and `--dry-run` or `--no-post`; cannot be combined with `--fresh-session`. |
 | `--session <name>` | Override the PR's default orchestrator session with a named live-review session. Reviewer cohorts remain PR-scoped. Not allowed with `--dry-run`, `--no-post`, or `--retry-posts`. |
 
 Review progress on stderr reports the merged reviewer catalog, final selected

docs/development.md

@@ -90,15 +90,91 @@ progress interface rather than importing Cobra or root options.
 ```bash
 make build   # compile the binary
 make test    # go test ./...
+make test-race # repeated race checks for ledger/dbmig
 make lint    # golangci-lint run
 make tidy    # go mod tidy and verify go.mod is unchanged
 make deps    # download and verify Go modules
 make check   # tidy + fmt + lint + test + build
 make clean   # remove build artifacts
+make test-pi-runtime  # installed Pi runtime gate (see Pi Runtime Gate)
 ```
 
 `make snapshot` runs a local GoReleaser snapshot build without publishing.
 
+### Concurrency checks
+
+`make test-race` runs all tests in `internal/ledger` and `internal/dbmig` under
+the Go race detector with 25 repetitions. This includes concurrent ledger
+startup and allocation, the immediate transaction-lock DSN contract, and
+migration version re-checks. Repeating the existing contention tests makes
+timing-dependent failures more likely to surface without applying race
+detection to the entire suite.
+
+The target sets test `GOMAXPROCS` to four with `-cpu=4` and caps tests using
+`t.Parallel` at four with `-parallel=4`. Contention-test worker goroutines
+remain bounded by their own fixture counts. Packages run serially, with a
+five-minute timeout per package. The target requires
+`CGO_ENABLED=1` and a C compiler, even though the SQLite driver itself is
+pure Go. CI runs the same target in the separate `test-race` job on pull
+requests and pushes to `main`, with a 20-minute job timeout. The job uses the
+same pinned shared test action as `test`, including Go version selection from
+`go.mod` and the action's default dependency/build caching. Existing required
+check names and branch-protection settings are unchanged; requiring the new
+check is a separate maintainer decision after it has run successfully.
+
+## Pi Runtime Gate
+
+`make test-pi-runtime` runs the `pi_rpc` adapter against the installed Pi on
+`PATH` instead of the fake Pi the ordinary adapter tests use. It sets
+`CR_PI_RUNTIME_VERSION` to the Makefile's `PI_RUNTIME_VERSION` (currently
+`1.0.0`, printed by `make -s pi-runtime-version`). Both targets fail when
+`PI_RUNTIME_VERSION` is blank or holds more than one version. When
+`CR_PI_RUNTIME_VERSION` is set, the runtime tests fail if it is blank, if Pi is
+missing, or if `pi --version` reports any other version. When it is unset, as in
+`make test`, they skip. To install the pinned runtime:
+
+```bash
+pi_version="$(make -s pi-runtime-version)"
+npm install -g --ignore-scripts "@earendil-works/pi-coding-agent@${pi_version}"
+make test-pi-runtime
+```
+
+The tests make no paid or external calls:
+
+- Each test gives Pi a fresh `PI_CODING_AGENT_DIR` and `HOME` with
+  `PI_OFFLINE=1`.
+- The only model is a scripted OpenAI-compatible endpoint on loopback, with a
+  dummy key stored in the selected directory's `auth.json`.
+- Reviewer tool calls run the real `__pi-review-tool` helper, dispatched to the
+  test binary by `TestMain`.
+- Hostile instructions, extensions, and MCP servers are planted in the
+  actually selected agent directory as well as under the fixture `HOME`'s
+  default `.pi/agent` location and in the reviewer checkout. Hostile skills
+  are also planted under the fixture home and checkout. The tests fail if any
+  resource reaches a provider request or runs.
+- Both structured and reviewer cases plant each selected `AGENTS.md`,
+  `CLAUDE.md`, `AGENTS.override.md`, `SYSTEM.md`, and `APPEND_SYSTEM.md`
+  independently, so context-file precedence cannot hide an uncovered input.
+
+The Pi 1.0 prompt-resource contract is explicit: `--no-context-files` disables
+context discovery, CR's `--system-prompt` suppresses `SYSTEM.md` discovery,
+and `--append-system-prompt` with an empty argument supplies an explicit CLI
+source list that suppresses `APPEND_SYSTEM.md` discovery while contributing no
+text. See Pi's pinned [argument parser](https://github.com/earendil-works/pi/blob/v1.0.0/packages/coding-agent/src/cli/args.ts)
+and [resource loader](https://github.com/earendil-works/pi/blob/v1.0.0/packages/coding-agent/src/core/resource-loader.ts).
+
+CR does not redirect `PI_CODING_AGENT_DIR`, `HOME`, or Pi's credential store to
+achieve isolation. Authentication, model configuration, and OAuth refresh
+persistence continue to use the selected directory. The ordinary subprocess
+test verifies that a synthetic auth-state update remains in the selected store
+after scratch cleanup; it does not exercise a real OAuth provider or refresh.
+All installed-runtime evidence is local/mock-provider evidence, not deployed
+validation or an operating-system sandbox claim.
+
+The CI `pi-runtime` job installs the Makefile version on the Node release it
+pins and runs the same target. It is not a required check. To move to a new Pi
+release, change `PI_RUNTIME_VERSION` and run the target locally first.
+
 ## Repo-Local Shape
 
 - Module: `github.com/open-cli-collective/codereview-cli`