chore(loop): isolated double review for pr-review

[github-actions]

Closes #71

loop-task: #71

Why

skills/review/SKILL.md required two independent reviewers, but pr-review ran one pi process in one session: reviewers A and B were two passes of the same context and the same model. pi has no sub-agent mechanism (@earendil-works/pi-coding-agent@0.85.1, Design Principles), so "spawn two reviewer agents" could not be satisfied by the engine and the isolation the norms promised was nominal.

What

Option A from the issue — two runs in one job. Options B (workflow matrix) and C (pi-subagents) both need .github/workflows/** or a new dependency, which the job token cannot push.

• scripts/loop/shared/agent.mjs: runPi() gains a --tools allowlist; buildPrompt() gains reviewSlot (per-reviewer report/result files <rid>.reviewer-<slot>.*, names lens A/B, states the read-only allowlist, suppresses the product-stage commit/push instructions). Adds READONLY_REVIEW_TOOLS, pickReviewModel, reviewerResultName, mergeReviews, sumUsage.
• scripts/loop/shared/review.mjs (new): runDoubleReview() launches two pi processes with separate --session-dir (…/reviewer-a, …/reviewer-b), a different --model for the second lens (LOOP_REVIEW_MODEL, else the first different DeepSeek catalogue entry: flash → pro) and --tools read,grep,find,ls,bash, then merges their verdicts.
• scripts/loop/entry.mjs: pr-review calls runDoubleReview; the merged verdict replaces the single-agent result.outcome path. Every other stage is unchanged.
• skills/review/SKILL.md (norms proposal): describes the host-driven mechanism instead of "spawn two reviewer agents", and states that "read-only" means no edit/write tools, not a filesystem sandbox.
• scripts/loop/README.md: documents the mechanism (new "Isolated double review" subsection, LOOP_REVIEW_MODEL env fact, defect-log entry D35).

mergeReviews accepts only when both approve (accepted); otherwise rejected. A reviewer that produced no verdict is never counted as approval — a transient engine exit is retry, anything else failed. The two verdicts meet in exactly one place.

Because the read-only allowlist has no write/edit tool, each reviewer writes its report and result file with a bash heredoc; the prompt says so explicitly.

Verification

• Fake-engine harness (LOOP_ENGINE_CMD), 32/32 checks: exactly two invocations, separate session dirs, different models, both carrying --tools read,grep,find,ls,bash, and the accept / request-changes / missing-verdict merge paths. The captured argv showed --session-dir …/reviewer-a vs …/reviewer-b and --model deepseek/deepseek-v4-flash vs deepseek/deepseek-v4-pro.
• node --check on the three edited .mjs host files: OK.
• pnpm lint 0 warnings/0 errors; pnpm fmt:check clean; pnpm build + pnpm check:types exit 0; Azure + TOS signature oracles passed; pnpm exec vitest run --browser.headless with pnpm serve → 24 files passed, 255 passed / 26 skipped.

Review boundary

The scripts/loop/** change is a loop host PR — the owner's reviewed merge is the checker. skills/review/SKILL.md is a norms-layer proposal and must not be merged by the loop; the owner approves both in this one PR. No signer, src, dependency or workflow file is touched.

[Alex1990]

Use one model - deepseek/deepseek-v4-flash

[Alex1990]

Modify pr to use one model - deepseek/deepseek-v4-flash

[github-actions]

Coverage

Metric	%	Covered / Total
Lines	91.73%	1121/1222
Statements	90.42%	1209/1337
Functions	92.76%	218/235
Branches	76.97%	575/747

[github-actions]

Loop pr-review — request-changes ⚠️ — two isolated reviewers

Reviewer a — correctness & regression (model deepseek/deepseek-v4-flash, session /home/runner/work/_temp/pi-sessions/reviewer-a): request-changes

Reviewer A — Correctness & regression (isolated run, lens A)

Verdict: request-changes (confidence: high). The mechanism works — verified first-hand with a LOOP_ENGINE_CMD fake-engine harness calling runDoubleReview(): two spawns, --session-dir .../reviewer-a vs .../reviewer-b, --model deepseek/deepseek-v4-flash vs deepseek/deepseek-v4-pro, both --tools read,grep,find,ls,bash; accept / request-changes / missing / transient-crash merge paths all behave as documented (31/31 assertions). node --check OK, pnpm lint 0/0, pnpm fmt:check clean; no src/signer/workflow/dependency file touched. Three should-fix items remain.

1. [L2] scripts/loop/shared/agent.mjs:93 — A reviewer that timed out/aborted (classify() → kind: 'abort', exit: 2) with no verdict is merged as failed (human inbox), while the single-agent path routes the same cls.exit === 2 to retry (entry.mjs:364). Reproduced: reviewer A approves, reviewer B SIGTERM → merged failed. The shared budget (review.mjs:63, remaining = max(60_000, …)) makes reviewer B the most timeout-prone step, and classify() explicitly says provider flakiness must not flood the inbox. Fix: const transient = missing.some((r) => r.cls?.kind === 'retry' || r.cls?.kind === 'abort').
2. [L2] scripts/loop/shared/agent.mjs:50 — pickReviewModel returns any LOOP_REVIEW_MODEL override unchecked (pickReviewModel(flash, 'deepseek/does-not-exist') → 'deepseek/does-not-exist'). pi silently falls back to its default model for an invalid --model (the D19 hazard the workflow guards for LOOP_MODEL), so reviewer B can end up on the same model as A, defeating the "different model" criterion. .github/workflows/** is human-only, so the guard belongs in the host. Fix: honor the override only when REVIEW_MODEL_CANDIDATES.includes(override), else fall through; and/or warn when models.b === models.a.
3. [L2] scripts/loop/shared/agent.mjs:298 — the closing-ritual block if (writeLevel === 'auto') is not gated on reviewSlot, so a reviewer prompt still says "add the PR proposal the host will execute" right after the read-only reviewer boundary. Confirmed in this run's own prompt. The PR body's "suppresses the product-stage commit/push instructions" is therefore inaccurate. Fix: if (writeLevel === 'auto' && !reviewSlot).
4. [L3] scripts/loop/shared/review.mjs:72 — readJson (state.mjs:119) throws on malformed JSON, so one invalid reviewer result file aborts the whole run (no finishRun) instead of being treated as "no verdict". Fix: try/catch → missing verdict.
5. [L3] repo-wide — no committed regression test for mergeReviews/runDoubleReview; the fake-engine harness is not in the tree. Suggest committing it.
6. [L3] scripts/loop/shared/agent.mjs:87 — on an inconclusive review comment is null, so a failed review only applies needs-triage with no explanation on the PR. Consider a short comment.

Reviewer b — standards, safety & maintainability (model deepseek/deepseek-v4-pro, session /home/runner/work/_temp/pi-sessions/reviewer-b): request-changes

Reviewer B — standards, safety & maintainability: request-changes

1. [L2] scripts/loop/shared/agent.mjs:93 — mergeReviews counts only cls.kind === 'retry' as transient, but classify() maps reviewer timeout (agent.mjs:461) and signal termination (:463-464) to kind: 'abort'; a timed-out or signalled reviewer therefore yields failed (human inbox) instead of retry — a silent change from the single-agent path, which maps exit 2 to retry — and the shared wall-clock budget makes a starved second reviewer timing out at its 60s floor a concrete trigger. Suggested fix: also treat abort as transient.

2. [L2] scripts/loop/shared/review.mjs:72 — await readJson(resultPath) is unguarded: readJson throws on malformed JSON (only ENOENT returns null), and the reviewer writes JSON by hand via a bash heredoc with no validating write tool, so a slightly malformed result file crashes the whole host run (no finishRun, task left locked processing until the lock TTL expires) instead of flowing into mergeReviews's no-verdict path. Suggested fix: safe-read and treat a malformed file as null so it classifies as no-verdict.

3. [L2] scripts/loop/shared/agent.mjs:298-308 — the closing-ritual if (writeLevel === 'auto') push-proposal block ('For a product stage, add the PR proposal the host will execute…') is not gated on reviewSlot, so reviewers in auto mode still receive the product-stage commit/push instructions the PR claims to suppress (I received it in my own prompt, right after 'your job is the verdict, not the fix'). Functionally inert (the host ignores a reviewer push) but contradicts the PR description and the reviewer boundary. Suggested fix: gate on !reviewSlot.

4. [L3] scripts/loop/shared/review.mjs:25 — the comment 'each gets half the remaining wall-clock budget' is inaccurate: reviewer A receives the full timeoutMs, reviewer B only the remainder (floored at 60s). Reword to match the code.

5. [L3] No committed regression tests for the new pure functions (mergeReviews, reviewerVerdict, pickReviewModel, sumUsage) — the PR's 32/32 fake-engine harness is ephemeral, so the 'both approve → accept' invariant has no in-repo protection.