From a238112808fa781ede1a9aab4c8ac53b1fc86dc5 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 17:35:57 +0000 Subject: [PATCH 1/2] chore(loop): run two isolated reviewers for pr-review (#71) The pr-review stage claimed two independent reviewers but ran a single pi process in one session, so both lenses shared one context and one model. pi has no sub-agent mechanism, so skills/review/SKILL.md's "spawn two reviewer agents" could not be satisfied by the engine. runPi() gains a --tools allowlist and buildPrompt() a reviewSlot. For pr-review, entry.mjs now calls runDoubleReview() (shared/review.mjs), which runs two pi processes with separate --session-dir, a different --model for the second lens (LOOP_REVIEW_MODEL, else the first different DeepSeek model) and --tools read,grep,find,ls,bash. mergeReviews() accepts only when both approve and never counts a missing verdict as approval (retry for transient, failed otherwise), so the two verdicts meet in exactly one place. Host change; the owner's reviewed merge is the checker. --- scripts/loop/README.md | 46 +++++++- scripts/loop/entry.mjs | 80 ++++++++++---- scripts/loop/shared/agent.mjs | 195 ++++++++++++++++++++++++++++++++- scripts/loop/shared/review.mjs | 93 ++++++++++++++++ 4 files changed, 386 insertions(+), 28 deletions(-) create mode 100644 scripts/loop/shared/review.mjs diff --git a/scripts/loop/README.md b/scripts/loop/README.md index 1d99ed7..0446c74 100644 --- a/scripts/loop/README.md +++ b/scripts/loop/README.md @@ -300,6 +300,31 @@ GitHub event ─▶ loop.yml (concurrency group `loop` = platform-level single w `tokens`/`durationMs`/`model`/`outcome`); full transcripts go to the artifact store (90 days). +### Isolated double review (`pr-review`) + +`skills/review/SKILL.md` requires two independent reviewers, but pi has no +sub-agent mechanism — one process can only do two passes over one context. The host +(`shared/review.mjs`) therefore runs **two `pi` processes** for the `pr-review` +stage: + +| | Reviewer A | Reviewer B | +| --- | --- | --- | +| session | `/reviewer-a` | `/reviewer-b` | +| model | `LOOP_MODEL` | `LOOP_REVIEW_MODEL`, else the first different DeepSeek model | +| tools | `read,grep,find,ls,bash` | `read,grep,find,ls,bash` | +| result | `.reviewer-a.result.json` / `.md` | `.reviewer-b.result.json` / `.md` | + +They run in sequence (one shared wall-clock budget) and cannot see each other's +session or result file; `mergeReviews` is the only place their verdicts meet. The +changeset passes only when **both** approve (`accepted`); otherwise `rejected`. A +reviewer that produced no verdict is never counted as approval — a transient exit +becomes `retry`, anything else `failed`. The read-only allowlist has no `edit`/`write` +tool, so a reviewer writes its own result file with a `bash` heredoc; "read-only" +means "no edit/write tools", not a filesystem sandbox. + +Each reviewer prompt carries its own lens and review slot, and the product-stage +commit/push instructions are suppressed for it (the reviewer's job is the verdict). + ### Trigger → action (workflow) | Event | Action | @@ -860,8 +885,26 @@ the first `auto` run is a verification step rather than a milestone. `skipped` while step 6 stays `failure`, so the only red step is the one that actually failed. +- [x] D35 (#71) The `pr-review` stage claimed two isolated reviewers but ran one + `pi` process in one session, so both lenses shared a context and a model — + the isolation `skills/review/SKILL.md` promised was nominal. pi has no + sub-agent mechanism, so the promise was unfulfillable as written. The host + now launches two `pi` processes for `pr-review` (`shared/review.mjs`): separate + `--session-dir`, a different `--model` for the second lens, and a read-only + `--tools read,grep,find,ls,bash` allowlist; `mergeReviews` accepts only when + both approve and never counts a missing verdict as approval. The skill was + reworded to describe the host mechanism instead of telling the agent to spawn + sub-agents. Demonstrated with a fake-engine harness: two invocations, distinct + session dirs and models, both carrying the allowlist, and the accept/reject/ + inconclusive merge paths. + ## Environment facts +- **Double review (#71):** `pr-review` runs two isolated `pi` processes + (`shared/review.mjs`) — separate session dirs, different models, read-only tools — + and merges their verdicts. `LOOP_REVIEW_MODEL` overrides the second model; + without it the host picks the first DeepSeek catalogue entry that is not + `LOOP_MODEL` (default: `deepseek-v4-flash` → `deepseek-v4-pro`). - Phase (2026-09-10): **A1 implementation landed; A0 remains the running host until the 2026-09-15 switchover.** Write boundary in A1 = `report`. - Engine: pi (verified) for A1; A0 runs were driven by an interactive opencode @@ -880,7 +923,8 @@ the first `auto` run is a verification step rather than a milestone. the agent wanted to do arrived as a proposal checklist in the report. - pi's built-in DeepSeek catalogue (credential-gated, printed per run): `deepseek-v4-flash`, `deepseek-v4-flash-vision-exp`, `deepseek-v4-pro`. - There is no `deepseek-flash` — see D19. + There is no `deepseek-flash` — see D19. `LOOP_REVIEW_MODEL` defaults to a + different entry from this catalogue so the second reviewer lens is heterogeneous. - **Infrastructure smoke**: `gh workflow run loop.yml -f smoke=true` exercises checkout → toolchain → pi install → R2 pull → push and **skips the agent entirely**, so "is the pipeline healthy?" is answerable without spending diff --git a/scripts/loop/entry.mjs b/scripts/loop/entry.mjs index 1a324d5..85f6cab 100644 --- a/scripts/loop/entry.mjs +++ b/scripts/loop/entry.mjs @@ -18,6 +18,7 @@ * LOOP_EVENT_NAME / LOOP_EVENT_ACTION / LOOP_EVENT_JSON / LOOP_REPO * LOOP_WRITE_LEVEL (report|auto, default report) * LOOP_MODEL (pi's --model, e.g. deepseek/deepseek-flash) + * LOOP_REVIEW_MODEL (optional second-lens model; defaults to a different DeepSeek model) * LOOP_RUN_TIMEOUT_MS, LOOP_SESSION_DIR, GITHUB_STEP_SUMMARY */ @@ -35,6 +36,7 @@ import { } from './shared/state.mjs'; import { route, eventKey, stageForIssue } from './shared/route.mjs'; import { buildPrompt, runPi, parseEvents, summarize, classify } from './shared/agent.mjs'; +import { runDoubleReview } from './shared/review.mjs'; const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); const S = makeState(ROOT); @@ -290,35 +292,69 @@ async function doRun({ decision, ctx, repo, writeLevel }) { + `${task ? `task #${task.id}, ` : 'system-level, '}stage=${stage}` + `${decision.mode === 'readonly' ? ', readonly' : ''})`); - const prompt = buildPrompt({ - task, stage, runId: rid, writeLevel, mode: decision.mode, repo, - lead: [ - `trigger: ${ctx.eventName}.${ctx.action || '(none)'} — ${decision.reason}`, - decision.loopTask ? `linked issue: #${decision.loopTask}` : null, - ].filter(Boolean), - }); + const lead = [ + `trigger: ${ctx.eventName}.${ctx.action || '(none)'} — ${decision.reason}`, + decision.loopTask ? `linked issue: #${decision.loopTask}` : null, + ].filter(Boolean); + const promptOptions = { task, stage, runId: rid, writeLevel, mode: decision.mode, repo, lead }; const sessionDir = process.env.LOOP_SESSION_DIR ?? path.join(os.tmpdir(), 'loop-pi', rid); const timeoutMs = Number(process.env.LOOP_RUN_TIMEOUT_MS ?? 55 * 60 * 1000); - const res = await runPi({ - prompt, cwd: ROOT, sessionDir, - model: process.env.LOOP_MODEL, - timeoutMs, - writeLevel, - log, - }); - const events = parseEvents(res.stdout); - const { usage, stopReason, toolCalls, turns } = summarize(events); - - // the agent's result file (the orchestration layer is the single writer for finish) - const result = await readJson(path.join(S.reportsDir, `${rid}.result.json`)); - const cls = classify({ code: res.code, signal: res.signal, stderr: res.stderr, timedOut: res.timedOut, stopReason }); + let usage = null; + let toolCalls = 0; + let turns = 0; + let result; + let cls; + if (stage === 'pr-review') { + // Two isolated reviewer processes (#71): separate sessions, different models, + // read-only tools. `runDoubleReview` returns the merged verdict as `result`. + const review = await runDoubleReview({ + promptOptions, reportsDir: S.reportsDir, sessionDir, timeoutMs, writeLevel, + cwd: ROOT, log, warn, + }); + result = review.result; + usage = review.usage; + toolCalls = review.toolCalls; + turns = review.turns; + // Keep `cls` shaped like `classify()` so the shared logging below stays uniform; + // `mergeReviews` is the judge for this stage, not a single agent's exit code. + cls = { + exit: result.outcome === 'retry' ? 3 : result.outcome === 'failed' ? 1 : 0, + kind: 'review', + reason: result.note, + }; + } else { + const prompt = buildPrompt(promptOptions); + const res = await runPi({ + prompt, cwd: ROOT, sessionDir, + model: process.env.LOOP_MODEL, + timeoutMs, + writeLevel, + log, + }); + const summary = summarize(parseEvents(res.stdout)); + usage = summary.usage; + toolCalls = summary.toolCalls; + turns = summary.turns; + // the agent's result file (the orchestration layer is the single writer for finish) + result = await readJson(path.join(S.reportsDir, `${rid}.result.json`)); + cls = classify({ + code: res.code, signal: res.signal, stderr: res.stderr, + timedOut: res.timedOut, stopReason: summary.stopReason, + }); + } let outcome; let note; let push = null; - if (result?.outcome && outcomeAllowed(stage, result.outcome)) { + if (stage === 'pr-review') { + // `mergeReviews` already collapsed the two isolated verdicts (accepted/rejected), + // or classified an inconclusive review (retry/failed). The single-agent + // `result.outcome` path must not apply here. + outcome = result.outcome; + note = result.note; + } else if (result?.outcome && outcomeAllowed(stage, result.outcome)) { outcome = result.outcome; - note = result.note ?? `agent decision (exit=${res.code})`; + note = result.note ?? `agent decision (exit=${cls.exit})`; } else if (result?.outcome) { outcome = 'failed'; note = `agent returned outcome=${result.outcome}, but stage=${stage} does not allow it ` diff --git a/scripts/loop/shared/agent.mjs b/scripts/loop/shared/agent.mjs index 2ad5e1e..90706c7 100644 --- a/scripts/loop/shared/agent.mjs +++ b/scripts/loop/shared/agent.mjs @@ -21,11 +21,158 @@ import { spawn } from 'node:child_process'; +/** + * Read-only tool allowlist for the two isolated reviewers (#71): no `edit`/`write`, + * so a reviewer cannot change the diff it judges. `bash` is included because the + * reviewer needs `git diff` / `gh pr diff` and may run the check gates; it can still + * write through the shell, so "read-only" means "no edit/write tools", not a + * filesystem sandbox. + */ +export const READONLY_REVIEW_TOOLS = 'read,grep,find,ls,bash'; + +/** + * Candidate models for the second reviewer lens. + * + * The loop runs on DeepSeek (the workflow installs only `DEEPSEEK_API_KEY` and + * validates `LOOP_MODEL` against that catalogue), so "a different model" means one of + * the other DeepSeek entries. `LOOP_REVIEW_MODEL` overrides the choice; without it, + * pick the first entry that is not `LOOP_MODEL`. Both reviewers get an explicit + * `--model`, so neither can silently inherit the other's. + */ +export const REVIEW_MODEL_CANDIDATES = [ + 'deepseek/deepseek-v4-pro', + 'deepseek/deepseek-v4-flash-vision-exp', + 'deepseek/deepseek-v4-flash', +]; + +export function pickReviewModel(baseModel, override = null) { + const base = baseModel || null; + if (override && override !== base) return override; + return REVIEW_MODEL_CANDIDATES.find((m) => m !== base) ?? base; +} + +/** Result-file name for one isolated reviewer (path joins it to `state/reports`). */ +export const reviewerResultName = (runId, slot) => `${runId}.reviewer-${slot}.result.json`; + +const REVIEWER_LENS = { + a: 'correctness & regression', + b: 'standards, safety & maintainability', +}; + +/** + * A reviewer's verdict, tolerating the small wording variations a model produces. + * Falls back to the file's `outcome` when `decision.verdict` is missing. + */ +const reviewerVerdict = (r) => { + const fallback = r?.result?.outcome === 'accepted' ? 'approve' + : r?.result?.outcome === 'rejected' ? 'request-changes' : ''; + const raw = String(r?.result?.decision?.verdict ?? fallback) + .trim().toLowerCase().replace(/[\s_]+/g, '-'); + if (raw === 'approve' || raw === 'approved') return 'approve'; + if (raw === 'request-changes' || raw === 'changes-requested' || raw === 'request-change') { + return 'request-changes'; + } + return null; +}; + +/** + * Merge the two isolated reviewer runs into the single result `finishRun` consumes. + * + * Conservative by construction: the changeset passes only when **both** reviewers + * approve. A reviewer that produced no verdict is never counted as approval — a + * transient engine failure keeps the task claimable (`retry`), anything else goes to + * the human inbox (`failed`). This function is the only place the two verdicts meet; + * the reviewer processes cannot see each other. + */ +export function mergeReviews(reviewers) { + const describe = (r) => `reviewer ${r.slot} (${REVIEWER_LENS[r.slot] ?? r.slot}, ` + + `${r.model ?? 'default model'})`; + const missing = reviewers.filter((r) => reviewerVerdict(r) === null); + + if (missing.length) { + const transient = missing.some((r) => r.cls?.kind === 'retry'); + const outcome = transient ? 'retry' : 'failed'; + const who = missing.map(describe).join(', '); + const reasons = missing.map((r) => r.cls?.reason ?? 'no result file').join('; '); + return { + outcome, + note: `${who} produced no verdict (${reasons}); the double review is inconclusive`, + comment: null, + decision: { + verdict: 'inconclusive', + confidence: 'high', + reason: `Isolated double review inconclusive: ${who} produced no verdict, ` + + 'which is never counted as approval.', + autoReview: 'inconclusive', + reviewers: reviewers.map((r) => ({ + slot: r.slot, model: r.model, sessionDir: r.sessionDir, + verdict: reviewerVerdict(r), outcome: r.result?.outcome ?? null, + })), + }, + }; + } + + const pass = reviewers.every((r) => reviewerVerdict(r) === 'approve'); + const body = reviewers.map((r) => { + const detail = r.result?.comment || r.result?.note || '(no detail provided)'; + return `**Reviewer ${r.slot} — ${REVIEWER_LENS[r.slot] ?? r.slot}** ` + + `(model \`${r.model ?? 'default'}\`, session \`${r.sessionDir}\`): ` + + `**${reviewerVerdict(r)}**\n\n${String(detail).trim()}`; + }).join('\n\n'); + + const header = pass + ? '**Loop pr-review — approve** :white_check_mark: — two isolated reviewers' + : '**Loop pr-review — request-changes** :warning: — two isolated reviewers'; + return { + outcome: pass ? 'accepted' : 'rejected', + note: `${pass ? 'both' : 'not both'} isolated reviewers approve ` + + `(A: ${reviewers[0]?.model ?? 'default'}, B: ${reviewers[1]?.model ?? 'default'})`, + comment: `${header}\n\n${body}`, + decision: { + verdict: pass ? 'approve' : 'request-changes', + confidence: 'high', + reason: pass + ? 'Both isolated reviewers approve: separate pi sessions, different models, ' + + 'read-only tool sets.' + : 'At least one isolated reviewer requested changes; the changeset is not accepted.', + autoReview: pass ? 'pass' : 'fail', + reviewers: reviewers.map((r) => ({ + slot: r.slot, model: r.model, sessionDir: r.sessionDir, + verdict: reviewerVerdict(r), outcome: r.result?.outcome ?? null, + })), + }, + }; +} + +/** Sum pi usage across the two reviewer runs (authoritative per-run on message_end). */ +export function sumUsage(usages) { + const us = (usages ?? []).filter(Boolean); + if (!us.length) return null; + const total = (k) => us.reduce((n, u) => n + (typeof u[k] === 'number' ? u[k] : 0), 0); + const out = { + input: total('input'), + output: total('output'), + totalTokens: total('totalTokens'), + cost: { total: 0 }, + }; + for (const u of us) { + const c = u.cost; + if (c && typeof c === 'object') { + for (const [k, v] of Object.entries(c)) { + if (typeof v === 'number') out.cost[k] = (out.cost[k] ?? 0) + v; + } + } + } + return out; +} + /** * Prompt language matches skills/AGENTS.md (English) so the agent does not read * Chinese instructions inside English norms. */ -export function buildPrompt({ task, stage, runId, writeLevel, mode, repo, lead = [] }) { +export function buildPrompt({ + task, stage, runId, writeLevel, mode, repo, lead = [], reviewSlot = null, +}) { const L = []; L.push(`You are one automated run of the tiny-oss loop. Stage: ${stage}.`); L.push(''); @@ -50,6 +197,29 @@ export function buildPrompt({ task, stage, runId, writeLevel, mode, repo, lead = L.push('## Event context'); for (const l of lead) L.push(`- ${l}`); } + if (reviewSlot) { + const lens = reviewSlot === 'a' + ? 'Reviewer A — Correctness & regression' + : 'Reviewer B — Standards, safety & maintainability'; + L.push(''); + L.push(`## Isolated reviewer ${reviewSlot.toUpperCase()}`); + L.push('You are one of two reviewers for this pull request. Apply only your lens:'); + L.push(`**${lens}** as defined in \`skills/review/SKILL.md\`.`); + L.push('The other reviewer runs in a separate pi process with its own session and a'); + L.push('different model; you cannot see its work and it cannot see yours. Do not run the'); + L.push('other lens, do not spawn sub-agents, and do not read the other reviewer\'s files or'); + L.push('session. The host merges your two verdicts after both runs finish.'); + L.push(''); + L.push('You hold a **read-only tool set**: the allowlist is `read,grep,find,ls,bash` —'); + L.push('there is no `edit` or `write` tool. Write your report and result file with a `bash`'); + L.push('heredoc, and keep every other command read-only. Read-only here means no'); + L.push('edit/write tools; it is not a sandbox, so do not use `bash` to change the tree.'); + L.push(''); + L.push('Set `decision.verdict` to `approve` or `request-changes` and put your numbered'); + L.push('problem list (`[L] file:line — problem — suggested fix`) in the result'); + L.push('file\'s `comment` field. Use outcome `accepted` for `approve` and `rejected` for'); + L.push('`request-changes`.'); + } L.push(''); L.push(`## Write boundary: ${writeLevel}`); L.push('### GitHub: the host writes, you do not'); @@ -69,7 +239,13 @@ export function buildPrompt({ task, stage, runId, writeLevel, mode, repo, lead = L.push(''); L.push('Allowed: reading GitHub, editing the working tree, running the repo gates,'); L.push('reading and writing `state/`.'); - if (writeLevel === 'report') { + if (reviewSlot) { + L.push(''); + L.push('### Reviewer boundary: read-only, verdict only'); + L.push('Do **not** commit, push, open or merge a pull request, and do not modify the'); + L.push('working tree — your job is the verdict, not the fix. The host merges the two'); + L.push('reviewers\' verdicts and performs any GitHub write.'); + } else if (writeLevel === 'report') { L.push(''); L.push('### `report` mode: the host writes nothing either'); L.push('**Do not commit.** No branch will be pushed — a commit would be discarded with'); @@ -101,10 +277,16 @@ export function buildPrompt({ task, stage, runId, writeLevel, mode, repo, lead = L.push('Do not execute code from the incoming change (no `pnpm install`/`pnpm test` on it).'); } L.push(''); + const reportFile = reviewSlot + ? `state/reports/${runId}.reviewer-${reviewSlot}.md` + : `state/reports/${runId}.md`; + const resultFile = reviewSlot + ? `state/reports/${reviewerResultName(runId, reviewSlot)}` + : `state/reports/${runId}.result.json`; L.push('## Closing ritual (mandatory)'); - L.push(`1. Write your human-readable report to \`state/reports/${runId}.md\``); + L.push(`1. Write your human-readable report to \`${reportFile}\``); L.push(' (findings, evidence, and — under report boundary — the GitHub actions you propose).'); - L.push(`2. Write your machine-readable result to \`state/reports/${runId}.result.json\`:`); + L.push(`2. Write your machine-readable result to \`${resultFile}\`:`); L.push(' ```json'); L.push(' { "outcome": "", "note": "",'); L.push(' "comment": "",'); @@ -209,7 +391,8 @@ export function agentEnv(writeLevel, base = process.env) { } export function runPi({ - prompt, cwd, sessionDir, model, timeoutMs = 3600000, writeLevel = 'report', log = () => {}, + prompt, cwd, sessionDir, model, tools = null, + timeoutMs = 3600000, writeLevel = 'report', log = () => {}, }) { return new Promise((resolve) => { const args = [ @@ -220,6 +403,8 @@ export function runPi({ '-p', 'Follow the loop run instructions provided on stdin.', ]; if (model) args.push('--model', model); + // An allowlist for the reviewer runs (#71): only the named tools are enabled. + if (tools) args.push('--tools', tools); // The engine is the template layer's instance variable {{engine}} (05 §4.2): defaults // to pi, overridable with LOOP_ENGINE_CMD (e.g. run the same contract locally through diff --git a/scripts/loop/shared/review.mjs b/scripts/loop/shared/review.mjs new file mode 100644 index 0000000..6a92a03 --- /dev/null +++ b/scripts/loop/shared/review.mjs @@ -0,0 +1,93 @@ +/** + * Isolated double review for the `pr-review` stage (#71). + * + * `skills/review/SKILL.md` requires two independent reviewers, but pi has no + * sub-agent mechanism: one `pi` process can only do two passes over one context. The + * host therefore starts **two separate pi processes** — separate `--session-dir`, a + * different `--model` for the second lens, and a read-only `--tools` allowlist — and + * merges their verdicts with `mergeReviews`. The two processes share no session and no + * result file, so neither can see the other's reasoning; the merge is the only place + * their verdicts meet. + * + * The issue's option A (two runs in one job) is what this implements. Option B (a + * workflow matrix) would need `.github/workflows/**`, which the job token cannot push. + */ + +import path from 'node:path'; + +import { readJson } from './state.mjs'; +import { + buildPrompt, runPi, parseEvents, summarize, classify, + reviewerResultName, pickReviewModel, mergeReviews, sumUsage, + READONLY_REVIEW_TOOLS, +} from './agent.mjs'; + +/** The two lenses, run in order (each gets half the remaining wall-clock budget). */ +const LENS_SLOTS = [ + { slot: 'a', lens: 'correctness & regression' }, + { slot: 'b', lens: 'standards, safety & maintainability' }, +]; + +/** + * Run reviewer A and reviewer B and merge their verdicts. + * + * The first reviewer uses `LOOP_MODEL`; the second uses a different model + * (`LOOP_REVIEW_MODEL`, or the first DeepSeek catalogue entry that differs from + * `LOOP_MODEL`). Both run with `--tools read,grep,find,ls,bash` and their own session + * directory under `sessionDir`. + * + * @returns {{result: object, usage: object|null, toolCalls: number, turns: number, + * runs: object[], models: object}} + */ +export async function runDoubleReview({ + promptOptions, reportsDir, sessionDir, timeoutMs, + writeLevel = 'report', cwd, log = () => {}, warn = () => {}, +}) { + const baseModel = process.env.LOOP_MODEL ?? null; + const override = process.env.LOOP_REVIEW_MODEL ?? null; + const reviewModel = pickReviewModel(baseModel, override); + if (override && override === baseModel) { + warn(`LOOP_REVIEW_MODEL=${override} equals LOOP_MODEL=${baseModel}; using ` + + `${reviewModel} for the second lens so the two reviewers differ (#71)`); + } + const models = { a: baseModel, b: reviewModel }; + + // One shared budget for both reviewers: a hung first run must not leave the second + // one unbounded inside a job that still has to push state and close out. + const deadline = Date.now() + timeoutMs; + const runs = []; + for (const { slot, lens } of LENS_SLOTS) { + const dir = path.join(sessionDir, `reviewer-${slot}`); + const resultPath = path.join(reportsDir, reviewerResultName(promptOptions.runId, slot)); + const prompt = buildPrompt({ ...promptOptions, reviewSlot: slot }); + const remaining = Math.max(60_000, deadline - Date.now()); + const res = await runPi({ + prompt, cwd, sessionDir: dir, model: models[slot], + tools: READONLY_REVIEW_TOOLS, timeoutMs: remaining, writeLevel, log, + }); + const events = parseEvents(res.stdout); + const { usage, stopReason, toolCalls, turns } = summarize(events); + // The reviewer writes its own result file (read-only tools, so via `bash`); a + // missing file is handled by mergeReviews, never treated as approval. + const result = await readJson(resultPath); + const cls = classify({ + code: res.code, signal: res.signal, stderr: res.stderr, + timedOut: res.timedOut, stopReason, + }); + log(`reviewer ${slot} (${lens}): model=${models[slot]} session=${dir} ` + + `exit=${res.code}${cls.kind === 'ok' ? '' : ` (${cls.kind}: ${cls.reason})`}`); + runs.push({ + slot, lens, model: models[slot], sessionDir: dir, resultPath, + result, cls, usage, stopReason, toolCalls, turns, + }); + } + + return { + result: mergeReviews(runs), + usage: sumUsage(runs.map((r) => r.usage)), + toolCalls: runs.reduce((n, r) => n + r.toolCalls, 0), + turns: runs.reduce((n, r) => n + r.turns, 0), + runs, + models, + }; +} From 4b8a340434174edec37c4929c0b4b3095ed6719f Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 17:35:58 +0000 Subject: [PATCH 2/2] chore(norms): reword review skill for host-driven double review (#71) The skill told the agent to spawn two reviewer agents, which pi cannot do. It now describes the host mechanism: two isolated reviewer processes (separate sessions, different models, read-only tools) whose verdicts the host merges, and states that "read-only" means no edit/write tools rather than a filesystem sandbox. Norms-layer proposal; the owner merges it. --- skills/review/SKILL.md | 47 ++++++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/skills/review/SKILL.md b/skills/review/SKILL.md index 9c850ad..43428d7 100644 --- a/skills/review/SKILL.md +++ b/skills/review/SKILL.md @@ -1,15 +1,25 @@ --- name: review -description: Independent code review of a changeset by two separate reviewer - agents (maker/checker separation). Each reviewer checks a distinct lens, - produces a numbered diff-problem list, and the work is only done when every - accepted problem is fixed and re-verified. +description: Independent code review of a changeset by two isolated reviewer runs + (maker/checker separation). Each run applies one distinct lens, produces a + numbered diff-problem list, and the changeset only passes when both approve. --- # Double review a changeset -The implementing agent (maker) must not be the only judge. Spawn two reviewer -agents with isolation (separate worktree or equivalent). They review the diff, -not the intent. +The implementing agent (maker) must not be the only judge. The loop host runs +**two isolated reviewer processes** for `pr-review`: separate `pi` sessions +(`--session-dir`), a different `--model` for the second lens, and a read-only tool +allowlist (`read,grep,find,ls,bash`; no `edit`/`write`). Each run applies exactly one +lens and never sees the other's session or result file; the host merges the two +verdicts only after both finish. Do not try to spawn sub-agents — pi has no such +mechanism, and the isolation above is the host's job, not the reviewer's. + +"Read-only" here means no `edit`/`write` tools, not a filesystem sandbox: `bash` is +present because a reviewer needs `git`/`gh` and the check gates, and it can still write +through the shell. A reviewer writes its report and result file with a `bash` heredoc +and keeps every other command read-only. + +Review the diff, not the intent. ## Reviewer A — Correctness & regression - Does the change do what the task state says it should? @@ -29,14 +39,21 @@ not the intent. - Verdict: `approve` | `request-changes` - Numbered problems: `[L] file:line — problem — suggested fix` - Severity: L1 must-fix / L2 should-fix / L3 nit +- Put the problem list in the result file's `comment` field; the host copies it into + the PR comment. -## Resolution -- Maker fixes every L1 and L2 problem (L3 optional), re-runs `verify`, and - re-reviews the diff; if a reviewer problem is deliberately rejected, the - rejection must be recorded with a reason in the PR body. -- Merge the two verdicts: both `approve` → `autoReview: pass` in run record; - otherwise the PR carries the problems and the task stays un-merged. +## Resolution (the host, after both runs) +- Merge the two verdicts: both `approve` → `autoReview: pass` in the run record + (outcome `accepted`); otherwise the PR carries both reviewers' problems and the + task stays un-merged (outcome `rejected`). +- A reviewer that produced no verdict is **never** counted as approval: a transient + failure returns the task to `retry`, anything else goes to the human inbox + (`failed`). +- The maker fixes every L1 and L2 problem (L3 optional), re-runs `verify`, and the + change is re-pushed; if a reviewer problem is deliberately rejected, the rejection + must be recorded with a reason in the PR body. ## Done when -- Both reviewers issued a verdict; all L1/L2 problems resolved or explicitly - declined with reasons; `verify` gates green; verdict recorded in run log. +- Both reviewer runs issued a verdict into their own session and result file; all + L1/L2 problems resolved or explicitly declined with reasons; `verify` gates green; + verdict recorded in the run log.