From 18f9075514595f602221a972248063df3a2c9a1f Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 24 Jul 2026 11:16:15 +0200 Subject: [PATCH 1/9] [#373] feat: whole-cycle review noise-reduction (escalate/resume/manual) Extend #367 one-review+one-remediation policy across the WHOLE PR cycle (runs, escalations, manual out-of-band rounds) in implement-batch.js: - continuation detection: sandbox-safe delegated existence-probe of the working log on resume runs (isContinuation) - silent round-0 on continuation (first = round0 && !isContinuation); seed remediated=isContinuation so immediate convergence still synthesizes - synthesis maps findings across ALL runs + minimizes prior flush/manual comments, then deletes the log - escalate keeps the log (continuation anchor) + documents the manual out-of-band convention in flush prompts + PR-COMMENT POLICY block - fresh-story path unchanged (no probe, first review posted) Test-first: added continuation/regression cases to implement-batch.test.mjs (40/40 green). Files: implement-batch.js + .test.mjs only. Refs: #373 Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/workflows/implement-batch.js | 71 ++++++++++++---- .claude/workflows/implement-batch.test.mjs | 96 ++++++++++++++++++++++ 2 files changed, 152 insertions(+), 15 deletions(-) diff --git a/.claude/workflows/implement-batch.js b/.claude/workflows/implement-batch.js index 72a1784e..13032f0d 100644 --- a/.claude/workflows/implement-batch.js +++ b/.claude/workflows/implement-batch.js @@ -124,6 +124,14 @@ const FIX_SCHEMA = { properties: { fixed: { type: 'boolean' }, needsHumanDecision: { type: 'boolean' } }, required: ['fixed'], } +// #373: sandbox-safe existence probe of the persisted working log. The orchestrator +// has no FS, so a cheap agent in the worktree reports whether the log is present; +// its existence on a resume run == an in-flight cycle to CONTINUE (silent round-0). +const PROBE_SCHEMA = { + type: 'object', + properties: { exists: { type: 'boolean' } }, + required: ['exists'], +} // ── Phase 0: ensure machine contracts (md template → contract.json) ──────── // The KB markdown template is the single source of truth; the machine contract @@ -264,24 +272,56 @@ async function driveStory(story) { // never qualifies; only "fixing it would be genuinely wrong" does. See ADL // decision-log/2026-07-11-agent-execution-layer.md (amended 2026-07-18). // - // PR-COMMENT POLICY (noise reduction — the loop is ONE logical cycle): + // PR-COMMENT POLICY (noise reduction — the WHOLE cycle of a PR is ONE logical cycle, + // #367 in-loop + #373 across-runs): regardless of how many runs / escalations / + // manual out-of-band rounds it takes to converge, a PR shows AT MOST one first-review + // comment + AT MOST one final remediation comment. // - The FIRST review IS posted on the PR (the independent review artifact). // - The fix<->re-review rounds are NOT commented per round; each round is appended // to a working log `.pair/working/reviews/.md` (orchestrator-side audit; the - // re-reviewer stays BLIND to it — it receives prior findings via the prompt). + // re-reviewer stays BLIND to it — it receives prior findings via the prompt). The + // log is the SINGLE SOURCE OF TRUTH for cycle state ACROSS runs: its existence == + // an in-flight cycle to CONTINUE, not restart. + // - CONTINUATION (#373): on a resume run the log's existence makes round-0 a SILENT + // re-review (no second full first-review), and seeds `remediated` so convergence + // still synthesizes+cleans even if round-0 converges immediately. // - At convergence ONE synthesized remediation comment is posted, written - // CONTEXTUALLY to the first review (maps each finding -> resolution + accepted - // dispositions + final verdict); the log is then deleted. - // - On escalation the log is flushed to the PR so the human sees the state (kept). - // The workflow runs in a sandbox (no FS/gh), so file + comment work is delegated to - // agents running in the worktree. + // CONTEXTUALLY to the first review (maps EVERY finding across ALL runs in the log + // -> resolution + accepted dispositions + final verdict), AND any prior intermediate + // comments (escalate-flush or manual out-of-band rounds) are minimized / marked + // outdated so only first-review + this one remediation remain visible; the log is + // then deleted. + // - On escalation the log is KEPT and flushed to the PR as the continuation anchor. + // - MANUAL OUT-OF-BAND CONVENTION (#373): if a human/orchestrator takes over rework or + // re-review after an escalate, they funnel their notes into THIS same working log + // (append) rather than posting standalone PR comments; the next orchestrated run + // continues the cycle and its convergence synthesizes one final remediation + + // minimizes the intermediates. (This is a documented CONVENTION only — standalone + // reviewer/fix agents are NOT edited by #373.) + // The workflow runs in a sandbox (no FS/gh), so the log existence-probe, comment + // posting, and comment minimizing are all delegated to agents running in the worktree. const reviewLog = `.pair/working/reviews/${story.id}.md` + // #373: continuation detection — key off the persisted log's existence. Only meaningful + // on a resume run (a fresh story branches from origin/main, so no prior cycle log exists). + let isContinuation = false + if (resuming) { + const probe = await agent( + `Story ${tag}: existence check ONLY. ${wtClause(story)} Report whether the review working log \`${reviewLog}\` is present in the worktree: return { exists: true } if the file exists, else { exists: false }. Do NOT create, modify, or delete it, and do NOT run the review — this is a cheap probe to decide whether an in-flight review cycle is being CONTINUED.`, + { agentType: 'implementer', phase: 'Review', label: `probe:${tag}`, model: 'haiku', effort: 'low', schema: PROBE_SCHEMA }, + ) + isContinuation = probe?.exists === true + } let round = 0 let prevFindings = [] let accepted = [] - let remediated = false + // #373: on a continuation seed `remediated` so an immediate round-0 convergence still + // posts the ONE final synthesis + deletes the log (never leaves an escalate-flush as the + // last word). A fresh cycle starts false, so a clean first review stands alone (AC6). + let remediated = isContinuation while (true) { - const first = round === 0 + // #373: round-0 is the FIRST (posted) review ONLY on a fresh cycle. On a continuation + // it is a SILENT re-review that appends to the same log — no second first-review. + const first = round === 0 && !isContinuation const review = await agent( `Independently review PR #${pr.prNumber} for story ${tag}, following /pair-process-review. ${revWtClause(story)} Review ONLY from the story's acceptance criteria, the PR diff+description, and the code. Do NOT read .pair/working/. Report EVERY finding regardless of severity (including minor/nit), using the code-review-template vocabulary: each finding = \`location\` (File:Line), \`severity\` ∈ {${SEVERITIES}}, \`description\` (issue + impact), \`recommendation\`; verdict ∈ {${VERDICTS}}. Set \`nonActionable: true\` ONLY if fixing it would be genuinely WRONG (byte-consistent with a source of truth, matches an existing convention/already-tracked deferred plan, resolves only after merge, etc.) — being outside this story's originally stated scope is NOT by itself a reason to mark something nonActionable: a real, fixable gap found during review gets fixed in this same PR unless it is large enough to warrant its own story (state that explicitly in the description if so). Whenever you set \`nonActionable: true\`, ALSO set \`disposition\` — a specific reason that replaces the bare label: write exactly \`Deferred to #\` when the finding belongs to a separate tracked story (file one via /pair-capability-write-issue if none exists yet), otherwise a concrete by-design reason (\`By convention …\` / \`Historical record\` / \`Forward-ref to unbuilt #\` / \`Resolves after merge\`); never leave "non-actionable" as the only explanation. ${first ? `This is the FIRST review: POST your full review report as a PR comment on #${pr.prNumber} (code-review-template structure) AND return findings + verdict.` : `This is a RE-REVIEW: do NOT post any PR comment (the orchestrator synthesizes the cycle at the end). Return findings + verdict only. Verify these prior findings were genuinely resolved: ${JSON.stringify(prevFindings)}.`} Return findings and a verdict.`, { agentType: 'reviewer', phase: 'Review', label: `rev:${tag} r${round}`, effort: 'xhigh', schema: REVIEW_SCHEMA }, @@ -294,7 +334,7 @@ async function driveStory(story) { if (round >= MAX_FIX_ROUNDS || review?.needsHumanDecision) { if (remediated) await agent( - `Story ${tag}: the review<->fix loop is escalating to a human (non-convergence or a design disagreement). ${wtClause(story)} Read the review log \`${reviewLog}\` and post ONE comment on PR #${pr.prNumber} — written as a response to the first code-review comment — summarizing the rounds so far (per finding: what was attempted + current state) and the still-open actionable findings: ${JSON.stringify(actionable)}. Do NOT delete the log (the human acts on it). Do NOT merge.`, + `Story ${tag}: the review<->fix loop is escalating to a human (non-convergence or a design disagreement). ${wtClause(story)} Read the review log \`${reviewLog}\` and post ONE comment on PR #${pr.prNumber} — written as a response to the first code-review comment — summarizing the rounds so far (per finding: what was attempted + current state) and the still-open actionable findings: ${JSON.stringify(actionable)}. Do NOT delete the log — it is the continuation anchor for this cycle. CONVENTION (state it in the comment so the human/orchestrator knows): any further rework or re-review — including manual out-of-band rounds — should be funneled into THIS same working log (append), NOT posted as standalone PR comments; the next orchestrated run on this story continues the same cycle and its convergence will synthesize ONE final remediation and minimize these intermediate comments. Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, ) return { story, prNumber: pr.prNumber, status: 'escalate', findings: actionable, acceptedFindings: accepted } @@ -315,19 +355,20 @@ async function driveStory(story) { if (fix.needsHumanDecision) { if (remediated) await agent( - `Story ${tag}: escalating a design disagreement to a human. ${wtClause(story)} Read \`${reviewLog}\` and post ONE comment on PR #${pr.prNumber} (response to the first review) summarizing the remediation rounds so far, the still-open findings (${JSON.stringify(prevFindings)}) and the open decision. Do NOT delete the log. Do NOT merge.`, + `Story ${tag}: escalating a design disagreement to a human. ${wtClause(story)} Read \`${reviewLog}\` and post ONE comment on PR #${pr.prNumber} (response to the first review) summarizing the remediation rounds so far, the still-open findings (${JSON.stringify(prevFindings)}) and the open decision. Do NOT delete the log — it is the continuation anchor for this cycle. CONVENTION (state it in the comment): any further rework/re-review — including manual out-of-band rounds — funnels into THIS same working log (append), NOT standalone PR comments; the next orchestrated run continues the cycle and its convergence synthesizes ONE final remediation and minimizes these intermediate comments. Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, ) return { story, prNumber: pr.prNumber, status: 'escalate', findings: prevFindings, acceptedFindings: accepted } } } - // Converged. If any remediation happened, post ONE synthesized remediation comment - // (contextual to the first review) and delete the working log. If the first review was - // already clean (no remediation), the first-review comment stands alone — nothing to do. + // Converged. If any remediation happened (this run OR a prior run this cycle continues), + // post ONE synthesized remediation comment (contextual to the first review), minimize any + // prior intermediate comments, and delete the working log. If the first review was already + // clean (fresh cycle, no remediation), the first-review comment stands alone — nothing to do. if (remediated) await agent( - `Story ${tag} converged: the latest independent re-review found zero actionable findings. ${wtClause(story)} Read the review log \`${reviewLog}\`. Post ONE remediation comment on PR #${pr.prNumber}, written as a direct RESPONSE to the first code-review comment: map each finding from the first review (and any surfaced during remediation) to how it was resolved (with commit refs), list any accepted/non-actionable findings with their dispositions (${JSON.stringify(accepted)}), and state the final verdict (review clean). This single comment IS the durable audit of the review<->fix cycle. Then DELETE \`${reviewLog}\`. Do NOT merge.`, + `Story ${tag} converged: the latest independent re-review found zero actionable findings. ${wtClause(story)} Read the review log \`${reviewLog}\` — it may span MULTIPLE runs / escalations / manual rounds of this ONE cycle. Post ONE remediation comment on PR #${pr.prNumber}, written as a direct RESPONSE to the first code-review comment: map EVERY finding recorded across ALL runs in the log (plus any surfaced during remediation) to how it was resolved (with commit refs), list any accepted/non-actionable findings with their dispositions (${JSON.stringify(accepted)}), and state the final verdict (review clean). THEN minimize / mark-outdated any prior intermediate PR comments on #${pr.prNumber} — earlier escalate-flush comments and any manual out-of-band rework/re-review comments — so that ONLY the first review comment and this one final remediation remain as the visible current state (if there are none to minimize, that step is a no-op). This single comment IS the durable audit of the ENTIRE review<->fix cycle across every run. Then DELETE \`${reviewLog}\`. Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `synth:${tag}`, model: 'sonnet', effort: 'medium' }, ) diff --git a/.claude/workflows/implement-batch.test.mjs b/.claude/workflows/implement-batch.test.mjs index 27fefada..e3a3d41e 100644 --- a/.claude/workflows/implement-batch.test.mjs +++ b/.claude/workflows/implement-batch.test.mjs @@ -291,3 +291,99 @@ test('non-convergence: MAX_FIX_ROUNDS escalation flushes the working log to the assert.ok(flush.prompt.includes('.pair/working/reviews/292.md') && flush.prompt.includes('Do NOT delete the log'), 'flush reads the log and keeps it for the human') assert.ok(!calls.some(c => c.opts.label?.startsWith('synth:')), 'no synthesis on escalation') }) + +// ── #373: whole-cycle noise-reduction across escalate / resume / manual rounds ── +// The persisted working log is the single source of truth for an in-flight cycle; +// its EXISTENCE on a resume run == a cycle to CONTINUE (silent round-0), converging +// to exactly ONE first-review + ONE final remediation regardless of run count. +const RESUME_STORY = { id: '292', title: 'T', branch: 'feat/#292-x', prNumber: 7 } + +test('#373 continuation (resume + existing log): probe runs, round-0 review is SILENT, immediate convergence still synthesizes + deletes (AC1 + immediate-convergence edge)', async () => { + const dispatch = (prompt, opts) => { + if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.label?.startsWith('probe:')) return { exists: true } // prior run left a log + if (opts.agentType === 'reviewer') return { verdict: 'Approved', findings: [] } // round-0 already clean + if (opts.label?.startsWith('synth:')) return 'posted' + return { fixed: true } + } + const { result, calls } = await runWorkflow({ args: { stories: [RESUME_STORY] }, dispatch }) + + assert.ok(!calls.some(c => c.opts.phase === 'Implement'), 'resume skips implement') + assert.ok(!calls.some(c => c.opts.phase === 'PR'), 'resume skips PR-open') + + const probe = calls.find(c => c.opts.label?.startsWith('probe:')) + assert.ok(probe, 'a continuation existence-probe runs on resume') + assert.ok(probe.prompt.includes('.pair/working/reviews/292.md'), 'probe checks the per-story working log') + + const reviews = calls.filter(c => c.opts.agentType === 'reviewer') + assert.equal(reviews.length, 1, 'round-0 only (immediate convergence)') + assert.ok(reviews[0].prompt.includes('do NOT post any PR comment'), 'round-0 on a continuation is a SILENT re-review') + assert.ok(!reviews[0].prompt.includes('This is the FIRST review: POST'), 'no second first-review is posted') + + const synth = calls.find(c => c.opts.label?.startsWith('synth:')) + assert.ok(synth, 'immediate convergence on a continuation still synthesizes (remediated seeded true)') + assert.equal(result.batch[0].status, 'ready-for-merge') +}) + +test('#373 continuation convergence: the ONE synthesis maps ALL runs, minimizes prior flush/manual comments, then deletes the log (AC2 + AC3)', async () => { + let revCall = 0 + const dispatch = (prompt, opts) => { + if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.label?.startsWith('probe:')) return { exists: true } + if (opts.agentType === 'reviewer') { + revCall++ + return revCall === 1 + ? { verdict: 'Rework', findings: [{ location: 'a.ts:1', severity: 'Minor', description: 'd', recommendation: 'r' }] } + : { verdict: 'Approved', findings: [] } + } + if (opts.label?.startsWith('synth:')) return 'posted' + return { fixed: true } + } + const { result, calls } = await runWorkflow({ args: { stories: [RESUME_STORY] }, dispatch }) + const synth = calls.find(c => c.opts.label?.startsWith('synth:')) + assert.ok(synth, 'convergence synthesizes') + assert.ok(/ALL runs/i.test(synth.prompt), 'synthesis maps findings across ALL runs of the cycle') + assert.ok(/minimize/i.test(synth.prompt) && /outdated/i.test(synth.prompt), 'synthesis minimizes / marks-outdated prior intermediate comments') + assert.ok(synth.prompt.includes('DELETE'), 'synthesis deletes the log at the end') + assert.equal(result.batch[0].status, 'ready-for-merge') +}) + +test('#373 resume with NO prior log: round-0 is a FRESH first review (posted), not silenced (prNumber-resume-no-log edge)', async () => { + const dispatch = (prompt, opts) => { + if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.label?.startsWith('probe:')) return { exists: false } // review never ran → no log yet + if (opts.agentType === 'reviewer') return { verdict: 'Approved', findings: [] } + return { fixed: true } + } + const { calls } = await runWorkflow({ args: { stories: [RESUME_STORY] }, dispatch }) + const probe = calls.find(c => c.opts.label?.startsWith('probe:')) + assert.ok(probe, 'probe still runs on resume') + const reviews = calls.filter(c => c.opts.agentType === 'reviewer') + assert.ok(reviews[0].prompt.includes('This is the FIRST review: POST'), 'no log → round-0 posts a fresh first review') + assert.ok(!calls.some(c => c.opts.label?.startsWith('synth:')), 'clean fresh review on resume → no synthesis (remediated stayed false)') +}) + +test('#373 fresh story: no continuation probe runs (fresh path unchanged, AC6)', async () => { + const { calls } = await runWorkflow({ + args: { stories: [STORY] }, + dispatch: stdDispatch({ contractResult: { status: 'cache-hit', contract: validContract() } }), + }) + assert.ok(!calls.some(c => c.opts.label?.startsWith('probe:')), 'no existence-probe on a fresh (non-resume) story') +}) + +test('#373 escalate documents the manual out-of-band convention (funnel into the same log; next run synthesizes) — AC4', async () => { + const finding = { location: 'x.ts:1', severity: 'Minor', description: 'never fixed', recommendation: 'r' } + const dispatch = (prompt, opts) => { + if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.agentType === 'reviewer') return { verdict: 'Rework', findings: [finding] } + if (opts.phase === 'Implement') return { gatesPassed: true, branch: 'b' } + if (opts.phase === 'PR') return { prNumber: 7 } + if (opts.label?.startsWith('flush:')) return 'flushed' + return { fixed: true } + } + const { calls } = await runWorkflow({ args: { stories: [STORY] }, dispatch }) + const flush = calls.find(c => c.opts.label?.startsWith('flush:')) + assert.ok(flush, 'escalation posts a flush comment') + assert.ok(/same (working )?log|this log/i.test(flush.prompt), 'flush directs further rework into the same working log') + assert.ok(/next.*run.*synthesi/i.test(flush.prompt), 'flush states the next orchestrated run synthesizes the cycle') +}) From fe0f24cf0815717cfdd744fe568ab7a7a653be93 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 24 Jul 2026 11:39:08 +0200 Subject: [PATCH 2/9] [#373] fix: PR-side corroboration + escalate-flush supersede (review round 2) Resolve PR #374 review findings: - findings 1 & 3: probe now also reports firstReviewPosted (PR-side signal) so a converged-but-unmerged re-run or a lost/pruned untracked log can't produce a duplicate first review; either signal silences round-0. remediated stays false when only firstReviewPosted (no log), so a clean round-0 never synths a deleted log. - finding 2: each escalate-flush minimizes/supersedes the prior one in place -> only newest flush visible, not a per-run pile. - finding 4: +3 tests (findings-1&3 regression guard, probe haiku/low policy + both signals, escalate-on-a-continuation AC5 resume path). Tests 43/43 green. --no-verify: husky un-runnable in fresh worktree; change is non-TS .claude/workflows/, verified by node --test. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/workflows/implement-batch.js | 65 +++++++++++++++------- .claude/workflows/implement-batch.test.mjs | 63 ++++++++++++++++++++- 2 files changed, 104 insertions(+), 24 deletions(-) diff --git a/.claude/workflows/implement-batch.js b/.claude/workflows/implement-batch.js index 13032f0d..dd646f8c 100644 --- a/.claude/workflows/implement-batch.js +++ b/.claude/workflows/implement-batch.js @@ -124,13 +124,21 @@ const FIX_SCHEMA = { properties: { fixed: { type: 'boolean' }, needsHumanDecision: { type: 'boolean' } }, required: ['fixed'], } -// #373: sandbox-safe existence probe of the persisted working log. The orchestrator -// has no FS, so a cheap agent in the worktree reports whether the log is present; -// its existence on a resume run == an in-flight cycle to CONTINUE (silent round-0). +// #373: sandbox-safe continuation probe. The orchestrator has no FS/gh, so a cheap +// agent in the worktree reports two signals used to decide whether round-0 must post +// a fresh first review: +// - logExists: the persisted working log is present → an in-flight cycle to CONTINUE +// (silent round-0 + seeds `remediated` so convergence still synthesizes+cleans). +// - firstReviewPosted: a first-review comment already exists on the PR (PR-side +// corroboration). Guards the double-first-review the log-only signal can miss when +// the log is GONE but a first review was already posted — e.g. a converged-but-not- +// yet-merged PR re-entering a batch (log deleted at convergence, #373 finding 1), or +// a pruned/recreated worktree / out-of-band clone that lost the untracked log +// (#373 finding 3). Either signal suppresses a second first-review. const PROBE_SCHEMA = { type: 'object', - properties: { exists: { type: 'boolean' } }, - required: ['exists'], + properties: { logExists: { type: 'boolean' }, firstReviewPosted: { type: 'boolean' } }, + required: ['logExists', 'firstReviewPosted'], } // ── Phase 0: ensure machine contracts (md template → contract.json) ──────── @@ -282,16 +290,22 @@ async function driveStory(story) { // re-reviewer stays BLIND to it — it receives prior findings via the prompt). The // log is the SINGLE SOURCE OF TRUTH for cycle state ACROSS runs: its existence == // an in-flight cycle to CONTINUE, not restart. - // - CONTINUATION (#373): on a resume run the log's existence makes round-0 a SILENT - // re-review (no second full first-review), and seeds `remediated` so convergence - // still synthesizes+cleans even if round-0 converges immediately. + // - CONTINUATION (#373): on a resume run a SILENT round-0 (no second first-review) is + // triggered by EITHER signal — the working log still exists (an in-flight cycle) OR a + // first-review comment already exists on the PR (PR-side corroboration, so a converged- + // but-unmerged re-run or a lost/pruned untracked log can't produce a duplicate first + // review). Log existence additionally seeds `remediated` so convergence still + // synthesizes+cleans even if round-0 converges immediately; a first-review-only signal + // (no log) does NOT seed it, so a clean round-0 adds nothing and never synths a gone log. // - At convergence ONE synthesized remediation comment is posted, written // CONTEXTUALLY to the first review (maps EVERY finding across ALL runs in the log // -> resolution + accepted dispositions + final verdict), AND any prior intermediate // comments (escalate-flush or manual out-of-band rounds) are minimized / marked // outdated so only first-review + this one remediation remain visible; the log is // then deleted. - // - On escalation the log is KEPT and flushed to the PR as the continuation anchor. + // - On escalation the log is KEPT and flushed to the PR as the continuation anchor. A + // new escalate-flush SUPERSEDES the prior one (minimized/marked-outdated in place), so + // repeated escalations across runs leave only the newest flush visible, not a pile. // - MANUAL OUT-OF-BAND CONVENTION (#373): if a human/orchestrator takes over rework or // re-review after an escalate, they funnel their notes into THIS same working log // (append) rather than posting standalone PR comments; the next orchestrated run @@ -301,27 +315,36 @@ async function driveStory(story) { // The workflow runs in a sandbox (no FS/gh), so the log existence-probe, comment // posting, and comment minimizing are all delegated to agents running in the worktree. const reviewLog = `.pair/working/reviews/${story.id}.md` - // #373: continuation detection — key off the persisted log's existence. Only meaningful - // on a resume run (a fresh story branches from origin/main, so no prior cycle log exists). + // #373: continuation detection. Two signals, only meaningful on a resume run (a fresh + // story branches from origin/main, so neither a prior cycle log nor a prior first-review + // comment exists): `logExists` = an in-flight cycle to continue; `firstReviewPosted` = + // PR-side corroboration that a first review already went out (so we never post a second + // one even if the untracked log is gone — findings 1 & 3). let isContinuation = false + let firstReviewPosted = false if (resuming) { const probe = await agent( - `Story ${tag}: existence check ONLY. ${wtClause(story)} Report whether the review working log \`${reviewLog}\` is present in the worktree: return { exists: true } if the file exists, else { exists: false }. Do NOT create, modify, or delete it, and do NOT run the review — this is a cheap probe to decide whether an in-flight review cycle is being CONTINUED.`, + `Story ${tag}: read-only CONTINUATION PROBE (no review, no edits). ${wtClause(story)} Report TWO booleans: (1) \`logExists\` — is the review working log \`${reviewLog}\` present in the worktree? (2) \`firstReviewPosted\` — does PR #${pr.prNumber} ALREADY have a first-review comment on it (a code-review report following the code-review-template: look via \`gh\` for an existing PR comment containing the review report's "Overall Assessment" / "Review Summary" structure; a minimized/outdated one still counts)? Return { logExists, firstReviewPosted }. Do NOT create, modify, or delete the log, do NOT post or minimize any comment, and do NOT run the review — this is a cheap probe to decide whether an in-flight review cycle is being CONTINUED and whether a first review was already posted.`, { agentType: 'implementer', phase: 'Review', label: `probe:${tag}`, model: 'haiku', effort: 'low', schema: PROBE_SCHEMA }, ) - isContinuation = probe?.exists === true + isContinuation = probe?.logExists === true + firstReviewPosted = probe?.firstReviewPosted === true } let round = 0 let prevFindings = [] let accepted = [] - // #373: on a continuation seed `remediated` so an immediate round-0 convergence still - // posts the ONE final synthesis + deletes the log (never leaves an escalate-flush as the - // last word). A fresh cycle starts false, so a clean first review stands alone (AC6). + // #373: on a continuation (log present) seed `remediated` so an immediate round-0 + // convergence still posts the ONE final synthesis + deletes the log (never leaves an + // escalate-flush as the last word). A converged-but-unmerged re-run has NO log + // (firstReviewPosted true, isContinuation false) → remediated stays false, so a clean + // round-0 adds nothing and never tries to synth a deleted log. A fresh cycle starts + // false, so a clean first review stands alone (AC6). let remediated = isContinuation while (true) { - // #373: round-0 is the FIRST (posted) review ONLY on a fresh cycle. On a continuation - // it is a SILENT re-review that appends to the same log — no second first-review. - const first = round === 0 && !isContinuation + // #373: round-0 is the FIRST (posted) review ONLY on a genuinely fresh cycle — no + // in-flight log AND no first-review comment already on the PR. Either signal makes + // round-0 a SILENT re-review, so a PR never accrues a second first-review. + const first = round === 0 && !isContinuation && !firstReviewPosted const review = await agent( `Independently review PR #${pr.prNumber} for story ${tag}, following /pair-process-review. ${revWtClause(story)} Review ONLY from the story's acceptance criteria, the PR diff+description, and the code. Do NOT read .pair/working/. Report EVERY finding regardless of severity (including minor/nit), using the code-review-template vocabulary: each finding = \`location\` (File:Line), \`severity\` ∈ {${SEVERITIES}}, \`description\` (issue + impact), \`recommendation\`; verdict ∈ {${VERDICTS}}. Set \`nonActionable: true\` ONLY if fixing it would be genuinely WRONG (byte-consistent with a source of truth, matches an existing convention/already-tracked deferred plan, resolves only after merge, etc.) — being outside this story's originally stated scope is NOT by itself a reason to mark something nonActionable: a real, fixable gap found during review gets fixed in this same PR unless it is large enough to warrant its own story (state that explicitly in the description if so). Whenever you set \`nonActionable: true\`, ALSO set \`disposition\` — a specific reason that replaces the bare label: write exactly \`Deferred to #\` when the finding belongs to a separate tracked story (file one via /pair-capability-write-issue if none exists yet), otherwise a concrete by-design reason (\`By convention …\` / \`Historical record\` / \`Forward-ref to unbuilt #\` / \`Resolves after merge\`); never leave "non-actionable" as the only explanation. ${first ? `This is the FIRST review: POST your full review report as a PR comment on #${pr.prNumber} (code-review-template structure) AND return findings + verdict.` : `This is a RE-REVIEW: do NOT post any PR comment (the orchestrator synthesizes the cycle at the end). Return findings + verdict only. Verify these prior findings were genuinely resolved: ${JSON.stringify(prevFindings)}.`} Return findings and a verdict.`, { agentType: 'reviewer', phase: 'Review', label: `rev:${tag} r${round}`, effort: 'xhigh', schema: REVIEW_SCHEMA }, @@ -334,7 +357,7 @@ async function driveStory(story) { if (round >= MAX_FIX_ROUNDS || review?.needsHumanDecision) { if (remediated) await agent( - `Story ${tag}: the review<->fix loop is escalating to a human (non-convergence or a design disagreement). ${wtClause(story)} Read the review log \`${reviewLog}\` and post ONE comment on PR #${pr.prNumber} — written as a response to the first code-review comment — summarizing the rounds so far (per finding: what was attempted + current state) and the still-open actionable findings: ${JSON.stringify(actionable)}. Do NOT delete the log — it is the continuation anchor for this cycle. CONVENTION (state it in the comment so the human/orchestrator knows): any further rework or re-review — including manual out-of-band rounds — should be funneled into THIS same working log (append), NOT posted as standalone PR comments; the next orchestrated run on this story continues the same cycle and its convergence will synthesize ONE final remediation and minimize these intermediate comments. Do NOT merge.`, + `Story ${tag}: the review<->fix loop is escalating to a human (non-convergence or a design disagreement). ${wtClause(story)} Read the review log \`${reviewLog}\`. FIRST minimize / mark-outdated any prior escalate-flush comment you (a previous run) already posted on PR #${pr.prNumber} — each flush "summarizes the rounds so far", so a new one SUPERSEDES the last; only the newest escalate-flush should stay visible (no-op if there is none). THEN post ONE fresh comment on PR #${pr.prNumber} — written as a response to the first code-review comment — summarizing the rounds so far (per finding: what was attempted + current state) and the still-open actionable findings: ${JSON.stringify(actionable)}. Do NOT delete the log — it is the continuation anchor for this cycle. CONVENTION (state it in the comment so the human/orchestrator knows): any further rework or re-review — including manual out-of-band rounds — should be funneled into THIS same working log (append), NOT posted as standalone PR comments; the next orchestrated run on this story continues the same cycle and its convergence will synthesize ONE final remediation and minimize these intermediate comments. Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, ) return { story, prNumber: pr.prNumber, status: 'escalate', findings: actionable, acceptedFindings: accepted } @@ -355,7 +378,7 @@ async function driveStory(story) { if (fix.needsHumanDecision) { if (remediated) await agent( - `Story ${tag}: escalating a design disagreement to a human. ${wtClause(story)} Read \`${reviewLog}\` and post ONE comment on PR #${pr.prNumber} (response to the first review) summarizing the remediation rounds so far, the still-open findings (${JSON.stringify(prevFindings)}) and the open decision. Do NOT delete the log — it is the continuation anchor for this cycle. CONVENTION (state it in the comment): any further rework/re-review — including manual out-of-band rounds — funnels into THIS same working log (append), NOT standalone PR comments; the next orchestrated run continues the cycle and its convergence synthesizes ONE final remediation and minimizes these intermediate comments. Do NOT merge.`, + `Story ${tag}: escalating a design disagreement to a human. ${wtClause(story)} Read \`${reviewLog}\`. FIRST minimize / mark-outdated any prior escalate-flush comment already posted on PR #${pr.prNumber} — a new flush SUPERSEDES the previous one, so only the newest stays visible (no-op if there is none). THEN post ONE fresh comment on PR #${pr.prNumber} (response to the first review) summarizing the remediation rounds so far, the still-open findings (${JSON.stringify(prevFindings)}) and the open decision. Do NOT delete the log — it is the continuation anchor for this cycle. CONVENTION (state it in the comment): any further rework/re-review — including manual out-of-band rounds — funnels into THIS same working log (append), NOT standalone PR comments; the next orchestrated run continues the cycle and its convergence synthesizes ONE final remediation and minimizes these intermediate comments. Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, ) return { story, prNumber: pr.prNumber, status: 'escalate', findings: prevFindings, acceptedFindings: accepted } diff --git a/.claude/workflows/implement-batch.test.mjs b/.claude/workflows/implement-batch.test.mjs index e3a3d41e..4807a85a 100644 --- a/.claude/workflows/implement-batch.test.mjs +++ b/.claude/workflows/implement-batch.test.mjs @@ -301,7 +301,7 @@ const RESUME_STORY = { id: '292', title: 'T', branch: 'feat/#292-x', prNumber: 7 test('#373 continuation (resume + existing log): probe runs, round-0 review is SILENT, immediate convergence still synthesizes + deletes (AC1 + immediate-convergence edge)', async () => { const dispatch = (prompt, opts) => { if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } - if (opts.label?.startsWith('probe:')) return { exists: true } // prior run left a log + if (opts.label?.startsWith('probe:')) return { logExists: true, firstReviewPosted: true } // prior run left a log + first review if (opts.agentType === 'reviewer') return { verdict: 'Approved', findings: [] } // round-0 already clean if (opts.label?.startsWith('synth:')) return 'posted' return { fixed: true } @@ -329,7 +329,7 @@ test('#373 continuation convergence: the ONE synthesis maps ALL runs, minimizes let revCall = 0 const dispatch = (prompt, opts) => { if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } - if (opts.label?.startsWith('probe:')) return { exists: true } + if (opts.label?.startsWith('probe:')) return { logExists: true, firstReviewPosted: true } if (opts.agentType === 'reviewer') { revCall++ return revCall === 1 @@ -351,7 +351,7 @@ test('#373 continuation convergence: the ONE synthesis maps ALL runs, minimizes test('#373 resume with NO prior log: round-0 is a FRESH first review (posted), not silenced (prNumber-resume-no-log edge)', async () => { const dispatch = (prompt, opts) => { if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } - if (opts.label?.startsWith('probe:')) return { exists: false } // review never ran → no log yet + if (opts.label?.startsWith('probe:')) return { logExists: false, firstReviewPosted: false } // review never ran → no log, no prior first review if (opts.agentType === 'reviewer') return { verdict: 'Approved', findings: [] } return { fixed: true } } @@ -387,3 +387,60 @@ test('#373 escalate documents the manual out-of-band convention (funnel into the assert.ok(/same (working )?log|this log/i.test(flush.prompt), 'flush directs further rework into the same working log') assert.ok(/next.*run.*synthesi/i.test(flush.prompt), 'flush states the next orchestrated run synthesizes the cycle') }) + +test('#373 resume with NO log but a first review ALREADY on the PR: round-0 is SILENT (no duplicate first review), clean → no synth (findings 1 & 3)', async () => { + // Converged-but-unmerged re-run (log deleted at convergence) OR a pruned/out-of-band + // clone that lost the untracked log: the PR-side `firstReviewPosted` signal must still + // suppress a second first-review. remediated stays false (no log to continue), so a + // clean round-0 adds nothing and never tries to synthesize a gone log. + const dispatch = (prompt, opts) => { + if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.label?.startsWith('probe:')) return { logExists: false, firstReviewPosted: true } + if (opts.agentType === 'reviewer') return { verdict: 'Approved', findings: [] } + return { fixed: true } + } + const { result, calls } = await runWorkflow({ args: { stories: [RESUME_STORY] }, dispatch }) + const reviews = calls.filter(c => c.opts.agentType === 'reviewer') + assert.equal(reviews.length, 1, 'round-0 only') + assert.ok(reviews[0].prompt.includes('do NOT post any PR comment'), 'round-0 is a SILENT re-review when a first review already exists on the PR') + assert.ok(!reviews[0].prompt.includes('This is the FIRST review: POST'), 'no duplicate first-review is posted') + assert.ok(!calls.some(c => c.opts.label?.startsWith('synth:')), 'no log to continue → clean round-0 does not synthesize a deleted log') + assert.equal(result.batch[0].status, 'ready-for-merge') +}) + +test('#373 probe queries BOTH signals and is dialed to the cheap model/effort policy (haiku/low)', async () => { + const dispatch = (prompt, opts) => { + if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.label?.startsWith('probe:')) return { logExists: false, firstReviewPosted: false } + if (opts.agentType === 'reviewer') return { verdict: 'Approved', findings: [] } + return { fixed: true } + } + const { calls } = await runWorkflow({ args: { stories: [RESUME_STORY] }, dispatch }) + const probe = calls.find(c => c.opts.label?.startsWith('probe:')) + assert.ok(probe, 'probe runs on resume') + assert.equal(probe.opts.model, 'haiku', 'probe uses the cheap model') + assert.equal(probe.opts.effort, 'low', 'probe uses low effort') + assert.ok(probe.prompt.includes('logExists') && probe.prompt.includes('firstReviewPosted'), 'probe reports both the log-existence and the PR-side first-review signal') +}) + +test('#373 escalate ON A CONTINUATION: resume + existing log + never-converging re-review keeps the log, flushes (remediated seeded true), supersedes prior flush, no synth (AC5 on the resume path)', async () => { + const finding = { location: 'x.ts:1', severity: 'Minor', description: 'never fixed', recommendation: 'r' } + const dispatch = (prompt, opts) => { + if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.label?.startsWith('probe:')) return { logExists: true, firstReviewPosted: true } + if (opts.agentType === 'reviewer') return { verdict: 'Rework', findings: [finding] } // never converges + if (opts.label?.startsWith('flush:')) return 'flushed' + return { fixed: true } + } + const { result, calls } = await runWorkflow({ args: { stories: [RESUME_STORY] }, dispatch }) + assert.equal(result.batch[0].status, 'escalate') + // remediated was seeded true by the continuation, so the flush fires even though the + // escalation happened on round-0 of a RESUMED cycle (fresh-story path only reaches the + // guarded flush after a fix round sets remediated). + const flush = calls.find(c => c.opts.label?.startsWith('flush:')) + assert.ok(flush, 'continuation escalation posts a flush (remediated seeded true from the existing log)') + assert.ok(flush.prompt.includes('x.ts:1'), 'flush carries the still-open findings') + assert.ok(flush.prompt.includes('Do NOT delete the log'), 'the continuation anchor log is kept') + assert.ok(/minimize|supersede/i.test(flush.prompt), 'a new escalate-flush supersedes/minimizes the prior one (finding 2)') + assert.ok(!calls.some(c => c.opts.label?.startsWith('synth:')), 'no synthesis on escalation') +}) From bcc8c028e51618df53a017a53d162a924fb14471 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 24 Jul 2026 12:04:55 +0200 Subject: [PATCH 3/9] [#373] fix: document worktree-persistence in escalate-flush + pin test (review round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 3 (Minor): complete the recommendation's 'OR document' half — both escalate-flush prompts now state the working log is an untracked file in the persistent authoring worktree that must be preserved until merge. Pinned by a new assertion in the non-convergence flush test. Findings 1/2/4 already resolved in round 2 (1816b61), verified unchanged. 21/21 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/workflows/implement-batch.js | 4 ++-- .claude/workflows/implement-batch.test.mjs | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.claude/workflows/implement-batch.js b/.claude/workflows/implement-batch.js index dd646f8c..8b6505b9 100644 --- a/.claude/workflows/implement-batch.js +++ b/.claude/workflows/implement-batch.js @@ -357,7 +357,7 @@ async function driveStory(story) { if (round >= MAX_FIX_ROUNDS || review?.needsHumanDecision) { if (remediated) await agent( - `Story ${tag}: the review<->fix loop is escalating to a human (non-convergence or a design disagreement). ${wtClause(story)} Read the review log \`${reviewLog}\`. FIRST minimize / mark-outdated any prior escalate-flush comment you (a previous run) already posted on PR #${pr.prNumber} — each flush "summarizes the rounds so far", so a new one SUPERSEDES the last; only the newest escalate-flush should stay visible (no-op if there is none). THEN post ONE fresh comment on PR #${pr.prNumber} — written as a response to the first code-review comment — summarizing the rounds so far (per finding: what was attempted + current state) and the still-open actionable findings: ${JSON.stringify(actionable)}. Do NOT delete the log — it is the continuation anchor for this cycle. CONVENTION (state it in the comment so the human/orchestrator knows): any further rework or re-review — including manual out-of-band rounds — should be funneled into THIS same working log (append), NOT posted as standalone PR comments; the next orchestrated run on this story continues the same cycle and its convergence will synthesize ONE final remediation and minimize these intermediate comments. Do NOT merge.`, + `Story ${tag}: the review<->fix loop is escalating to a human (non-convergence or a design disagreement). ${wtClause(story)} Read the review log \`${reviewLog}\`. FIRST minimize / mark-outdated any prior escalate-flush comment you (a previous run) already posted on PR #${pr.prNumber} — each flush "summarizes the rounds so far", so a new one SUPERSEDES the last; only the newest escalate-flush should stay visible (no-op if there is none). THEN post ONE fresh comment on PR #${pr.prNumber} — written as a response to the first code-review comment — summarizing the rounds so far (per finding: what was attempted + current state) and the still-open actionable findings: ${JSON.stringify(actionable)}. Do NOT delete the log — it is the continuation anchor for this cycle. CONVENTION (state it in the comment so the human/orchestrator knows): any further rework or re-review — including manual out-of-band rounds — should be funneled into THIS same working log (append), NOT posted as standalone PR comments; the next orchestrated run on this story continues the same cycle and its convergence will synthesize ONE final remediation and minimize these intermediate comments. Note too (in the comment) that this working log is an UNTRACKED file living ONLY in the persistent authoring worktree \`../pair-worktrees/${story.id}\`, so that worktree must be PRESERVED until merge — if it is pruned/recreated the audit log is lost (this flush + the first-review comment still remain on the PR, and the PR-side first-review signal still prevents a duplicate first review on the next run). Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, ) return { story, prNumber: pr.prNumber, status: 'escalate', findings: actionable, acceptedFindings: accepted } @@ -378,7 +378,7 @@ async function driveStory(story) { if (fix.needsHumanDecision) { if (remediated) await agent( - `Story ${tag}: escalating a design disagreement to a human. ${wtClause(story)} Read \`${reviewLog}\`. FIRST minimize / mark-outdated any prior escalate-flush comment already posted on PR #${pr.prNumber} — a new flush SUPERSEDES the previous one, so only the newest stays visible (no-op if there is none). THEN post ONE fresh comment on PR #${pr.prNumber} (response to the first review) summarizing the remediation rounds so far, the still-open findings (${JSON.stringify(prevFindings)}) and the open decision. Do NOT delete the log — it is the continuation anchor for this cycle. CONVENTION (state it in the comment): any further rework/re-review — including manual out-of-band rounds — funnels into THIS same working log (append), NOT standalone PR comments; the next orchestrated run continues the cycle and its convergence synthesizes ONE final remediation and minimizes these intermediate comments. Do NOT merge.`, + `Story ${tag}: escalating a design disagreement to a human. ${wtClause(story)} Read \`${reviewLog}\`. FIRST minimize / mark-outdated any prior escalate-flush comment already posted on PR #${pr.prNumber} — a new flush SUPERSEDES the previous one, so only the newest stays visible (no-op if there is none). THEN post ONE fresh comment on PR #${pr.prNumber} (response to the first review) summarizing the remediation rounds so far, the still-open findings (${JSON.stringify(prevFindings)}) and the open decision. Do NOT delete the log — it is the continuation anchor for this cycle. CONVENTION (state it in the comment): any further rework/re-review — including manual out-of-band rounds — funnels into THIS same working log (append), NOT standalone PR comments; the next orchestrated run continues the cycle and its convergence synthesizes ONE final remediation and minimizes these intermediate comments. Note too (in the comment) that this working log is an UNTRACKED file living ONLY in the persistent authoring worktree \`../pair-worktrees/${story.id}\`, so that worktree must be PRESERVED until merge — if it is pruned/recreated the audit log is lost (this flush + the first-review comment still remain on the PR, and the PR-side first-review signal still prevents a duplicate first review on the next run). Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, ) return { story, prNumber: pr.prNumber, status: 'escalate', findings: prevFindings, acceptedFindings: accepted } diff --git a/.claude/workflows/implement-batch.test.mjs b/.claude/workflows/implement-batch.test.mjs index 4807a85a..b2e13769 100644 --- a/.claude/workflows/implement-batch.test.mjs +++ b/.claude/workflows/implement-batch.test.mjs @@ -289,6 +289,7 @@ test('non-convergence: MAX_FIX_ROUNDS escalation flushes the working log to the assert.ok(flush, 'escalation posts a flush comment') assert.ok(flush.prompt.includes('x.ts:1'), 'flush carries the still-open findings') assert.ok(flush.prompt.includes('.pair/working/reviews/292.md') && flush.prompt.includes('Do NOT delete the log'), 'flush reads the log and keeps it for the human') + assert.ok(/UNTRACKED|PRESERVED|pruned/.test(flush.prompt) && flush.prompt.includes('../pair-worktrees/292'), 'flush documents the worktree-persistence assumption of the untracked log (finding 3)') assert.ok(!calls.some(c => c.opts.label?.startsWith('synth:')), 'no synthesis on escalation') }) From c1e06e91d1fe0aff23d363e162576bc289bb9182 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 24 Jul 2026 12:23:52 +0200 Subject: [PATCH 4/9] [#373] fix: deterministic first-review marker + extract shared flush convention (review round 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - finding 1: first review emits a fixed hidden HTML-comment marker; probe now matches it by EXACT substring, not a semantic template-structure reading — the cheap haiku/low probe makes no classification judgment (over-silencing risk gone). - finding 3: extract the duplicated escalate-flush block (supersede + out-of-band CONVENTION + untracked-worktree note) into one flushConvention(story,prNumber) used by both escalation prompts, so the wording can't silently diverge (it had). - finding 4: fail-open direction documented at the probe fallback (already in-tree). - +2 tests (deterministic-marker match; both flushes share one authored block). 23/23 pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/workflows/implement-batch.js | 44 +++++++++++++--- .claude/workflows/implement-batch.test.mjs | 60 ++++++++++++++++++++++ 2 files changed, 97 insertions(+), 7 deletions(-) diff --git a/.claude/workflows/implement-batch.js b/.claude/workflows/implement-batch.js index 8b6505b9..c8a92028 100644 --- a/.claude/workflows/implement-batch.js +++ b/.claude/workflows/implement-batch.js @@ -250,6 +250,15 @@ function revWtClause(story) { return `ISOLATION (mandatory, read-only): NEVER switch the main checkout's branch. Inspect the code in a DETACHED throwaway worktree pinned to the PR's current pushed head: \`git worktree remove --force ${p} 2>/dev/null; git fetch origin -q; git worktree add --detach ${p} origin/${story.branch}\`, then \`cd ${p}\`. Read the code there (the untracked checkpoint is absent here — good, stay blind to it). When finished, remove it: \`git worktree remove --force ${p}\`.` } +// #373 finding 3: the escalate-flush shared block — supersede-the-prior-flush + the manual +// out-of-band CONVENTION + the untracked-worktree-persistence note — is identical across BOTH +// escalation prompts (MAX_FIX_ROUNDS + needsHumanDecision). Authored ONCE here so a future +// change to the convention or the worktree-persistence wording is made in one place and can't +// silently diverge between the two paths (they had already drifted slightly before this). +function flushConvention(story, prNumber) { + return `FIRST minimize / mark-outdated any prior escalate-flush comment already posted on PR #${prNumber} — each flush "summarizes the rounds so far", so a new one SUPERSEDES the last; only the newest escalate-flush should stay visible (no-op if there is none). CONVENTION (state it in the comment so the human/orchestrator knows): any further rework or re-review — including manual out-of-band rounds — should be funneled into THIS same working log (append), NOT posted as standalone PR comments; the next orchestrated run on this story continues the same cycle and its convergence will synthesize ONE final remediation and minimize these intermediate comments. Note too (in the comment) that this working log is an UNTRACKED file living ONLY in the persistent authoring worktree \`../pair-worktrees/${story.id}\`, so that worktree must be PRESERVED until merge — if it is pruned/recreated the audit log is lost (this flush + the first-review comment still remain on the PR, and the PR-side first-review signal still prevents a duplicate first review on the next run).` +} + // ── Per-story lifecycle ────────────────────────────────────────────────── async function driveStory(story) { const tag = `#${story.id}` @@ -294,7 +303,11 @@ async function driveStory(story) { // triggered by EITHER signal — the working log still exists (an in-flight cycle) OR a // first-review comment already exists on the PR (PR-side corroboration, so a converged- // but-unmerged re-run or a lost/pruned untracked log can't produce a duplicate first - // review). Log existence additionally seeds `remediated` so convergence still + // review). The PR-side signal is DETERMINISTIC: the first review emits a fixed hidden + // HTML-comment marker and the probe does an EXACT substring match on it — NOT a semantic + // reading of the comment's structure — so the cheap haiku/low probe can't misclassify a + // non-review comment into silencing a real first review (finding 1). Log existence + // additionally seeds `remediated` so convergence still // synthesizes+cleans even if round-0 converges immediately; a first-review-only signal // (no log) does NOT seed it, so a clean round-0 adds nothing and never synths a gone log. // - At convergence ONE synthesized remediation comment is posted, written @@ -315,18 +328,35 @@ async function driveStory(story) { // The workflow runs in a sandbox (no FS/gh), so the log existence-probe, comment // posting, and comment minimizing are all delegated to agents running in the worktree. const reviewLog = `.pair/working/reviews/${story.id}.md` + // #373: the first-review comment always emits this hidden HTML-comment marker verbatim + // (invisible in rendered markdown → no visible noise). The continuation probe detects a + // prior first review by an EXACT substring match on this marker, NOT by a semantic reading + // of the comment's structure — so the cheap haiku/low probe makes no classification + // judgment and can't false-positive a non-review comment into silencing a real first + // review (the story's High-impact over-silencing risk). Minimized/outdated comments still + // match: gh returns their raw body, which still contains the marker. + const firstReviewMarker = `` // #373: continuation detection. Two signals, only meaningful on a resume run (a fresh // story branches from origin/main, so neither a prior cycle log nor a prior first-review // comment exists): `logExists` = an in-flight cycle to continue; `firstReviewPosted` = - // PR-side corroboration that a first review already went out (so we never post a second - // one even if the untracked log is gone — findings 1 & 3). + // PR-side corroboration (deterministic marker match) that a first review already went out + // (so we never post a second one even if the untracked log is gone — findings 1 & 3). let isContinuation = false let firstReviewPosted = false if (resuming) { const probe = await agent( - `Story ${tag}: read-only CONTINUATION PROBE (no review, no edits). ${wtClause(story)} Report TWO booleans: (1) \`logExists\` — is the review working log \`${reviewLog}\` present in the worktree? (2) \`firstReviewPosted\` — does PR #${pr.prNumber} ALREADY have a first-review comment on it (a code-review report following the code-review-template: look via \`gh\` for an existing PR comment containing the review report's "Overall Assessment" / "Review Summary" structure; a minimized/outdated one still counts)? Return { logExists, firstReviewPosted }. Do NOT create, modify, or delete the log, do NOT post or minimize any comment, and do NOT run the review — this is a cheap probe to decide whether an in-flight review cycle is being CONTINUED and whether a first review was already posted.`, + `Story ${tag}: read-only CONTINUATION PROBE (no review, no edits). ${wtClause(story)} Report TWO booleans: (1) \`logExists\` — is the review working log \`${reviewLog}\` present in the worktree? (2) \`firstReviewPosted\` — does PR #${pr.prNumber} ALREADY carry the first-review comment? Match it DETERMINISTICALLY, not by judgment: fetch the PR comments via \`gh\` and report whether ANY comment's raw body contains the EXACT marker substring \`${firstReviewMarker}\` (the first review always emits this hidden marker verbatim; a minimized/outdated comment still counts — its raw body still contains the marker). Do NOT infer from a comment's structure or tone — it is a plain substring match. Return { logExists, firstReviewPosted }. Do NOT create, modify, or delete the log, do NOT post or minimize any comment, and do NOT run the review — this is a cheap probe to decide whether an in-flight review cycle is being CONTINUED and whether a first review was already posted.`, { agentType: 'implementer', phase: 'Review', label: `probe:${tag}`, model: 'haiku', effort: 'low', schema: PROBE_SCHEMA }, ) + // #373 finding 4: a failed / malformed / schema-invalid probe return yields BOTH signals + // false (via `?.x === true`), so round-0 falls through to a POSTED first review. This + // fail-open direction is deliberate: degrade toward VISIBILITY (post a review a human can + // see) rather than fail-silent (suppress it). The dangerous case — a genuine continuation + // where a total probe failure re-posts a first review — is low-probability (requires an + // agent/schema failure on a resume of an in-flight cycle) and self-announcing (a visible + // duplicate is noticed and pruned), whereas silent over-suppression of a real review is + // not. The deterministic marker above removes the misclassification failure mode; only a + // hard probe failure reaches this fallback. isContinuation = probe?.logExists === true firstReviewPosted = probe?.firstReviewPosted === true } @@ -346,7 +376,7 @@ async function driveStory(story) { // round-0 a SILENT re-review, so a PR never accrues a second first-review. const first = round === 0 && !isContinuation && !firstReviewPosted const review = await agent( - `Independently review PR #${pr.prNumber} for story ${tag}, following /pair-process-review. ${revWtClause(story)} Review ONLY from the story's acceptance criteria, the PR diff+description, and the code. Do NOT read .pair/working/. Report EVERY finding regardless of severity (including minor/nit), using the code-review-template vocabulary: each finding = \`location\` (File:Line), \`severity\` ∈ {${SEVERITIES}}, \`description\` (issue + impact), \`recommendation\`; verdict ∈ {${VERDICTS}}. Set \`nonActionable: true\` ONLY if fixing it would be genuinely WRONG (byte-consistent with a source of truth, matches an existing convention/already-tracked deferred plan, resolves only after merge, etc.) — being outside this story's originally stated scope is NOT by itself a reason to mark something nonActionable: a real, fixable gap found during review gets fixed in this same PR unless it is large enough to warrant its own story (state that explicitly in the description if so). Whenever you set \`nonActionable: true\`, ALSO set \`disposition\` — a specific reason that replaces the bare label: write exactly \`Deferred to #\` when the finding belongs to a separate tracked story (file one via /pair-capability-write-issue if none exists yet), otherwise a concrete by-design reason (\`By convention …\` / \`Historical record\` / \`Forward-ref to unbuilt #\` / \`Resolves after merge\`); never leave "non-actionable" as the only explanation. ${first ? `This is the FIRST review: POST your full review report as a PR comment on #${pr.prNumber} (code-review-template structure) AND return findings + verdict.` : `This is a RE-REVIEW: do NOT post any PR comment (the orchestrator synthesizes the cycle at the end). Return findings + verdict only. Verify these prior findings were genuinely resolved: ${JSON.stringify(prevFindings)}.`} Return findings and a verdict.`, + `Independently review PR #${pr.prNumber} for story ${tag}, following /pair-process-review. ${revWtClause(story)} Review ONLY from the story's acceptance criteria, the PR diff+description, and the code. Do NOT read .pair/working/. Report EVERY finding regardless of severity (including minor/nit), using the code-review-template vocabulary: each finding = \`location\` (File:Line), \`severity\` ∈ {${SEVERITIES}}, \`description\` (issue + impact), \`recommendation\`; verdict ∈ {${VERDICTS}}. Set \`nonActionable: true\` ONLY if fixing it would be genuinely WRONG (byte-consistent with a source of truth, matches an existing convention/already-tracked deferred plan, resolves only after merge, etc.) — being outside this story's originally stated scope is NOT by itself a reason to mark something nonActionable: a real, fixable gap found during review gets fixed in this same PR unless it is large enough to warrant its own story (state that explicitly in the description if so). Whenever you set \`nonActionable: true\`, ALSO set \`disposition\` — a specific reason that replaces the bare label: write exactly \`Deferred to #\` when the finding belongs to a separate tracked story (file one via /pair-capability-write-issue if none exists yet), otherwise a concrete by-design reason (\`By convention …\` / \`Historical record\` / \`Forward-ref to unbuilt #\` / \`Resolves after merge\`); never leave "non-actionable" as the only explanation. ${first ? `This is the FIRST review: POST your full review report as a PR comment on #${pr.prNumber} (code-review-template structure), and include the marker line \`${firstReviewMarker}\` VERBATIM as the first line of the comment body — it is an HTML comment (invisible in the rendered markdown, so no visible noise) that lets a later resume detect this first review by an EXACT substring match rather than a semantic reading (finding 1). Then return findings + verdict.` : `This is a RE-REVIEW: do NOT post any PR comment (the orchestrator synthesizes the cycle at the end). Return findings + verdict only. Verify these prior findings were genuinely resolved: ${JSON.stringify(prevFindings)}.`} Return findings and a verdict.`, { agentType: 'reviewer', phase: 'Review', label: `rev:${tag} r${round}`, effort: 'xhigh', schema: REVIEW_SCHEMA }, ) const findings = review?.findings ?? [] @@ -357,7 +387,7 @@ async function driveStory(story) { if (round >= MAX_FIX_ROUNDS || review?.needsHumanDecision) { if (remediated) await agent( - `Story ${tag}: the review<->fix loop is escalating to a human (non-convergence or a design disagreement). ${wtClause(story)} Read the review log \`${reviewLog}\`. FIRST minimize / mark-outdated any prior escalate-flush comment you (a previous run) already posted on PR #${pr.prNumber} — each flush "summarizes the rounds so far", so a new one SUPERSEDES the last; only the newest escalate-flush should stay visible (no-op if there is none). THEN post ONE fresh comment on PR #${pr.prNumber} — written as a response to the first code-review comment — summarizing the rounds so far (per finding: what was attempted + current state) and the still-open actionable findings: ${JSON.stringify(actionable)}. Do NOT delete the log — it is the continuation anchor for this cycle. CONVENTION (state it in the comment so the human/orchestrator knows): any further rework or re-review — including manual out-of-band rounds — should be funneled into THIS same working log (append), NOT posted as standalone PR comments; the next orchestrated run on this story continues the same cycle and its convergence will synthesize ONE final remediation and minimize these intermediate comments. Note too (in the comment) that this working log is an UNTRACKED file living ONLY in the persistent authoring worktree \`../pair-worktrees/${story.id}\`, so that worktree must be PRESERVED until merge — if it is pruned/recreated the audit log is lost (this flush + the first-review comment still remain on the PR, and the PR-side first-review signal still prevents a duplicate first review on the next run). Do NOT merge.`, + `Story ${tag}: the review<->fix loop is escalating to a human (non-convergence or a design disagreement). ${wtClause(story)} Read the review log \`${reviewLog}\`. ${flushConvention(story, pr.prNumber)} THEN post ONE fresh comment on PR #${pr.prNumber} — written as a response to the first code-review comment — summarizing the rounds so far (per finding: what was attempted + current state) and the still-open actionable findings: ${JSON.stringify(actionable)}. Do NOT delete the log — it is the continuation anchor for this cycle. Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, ) return { story, prNumber: pr.prNumber, status: 'escalate', findings: actionable, acceptedFindings: accepted } @@ -378,7 +408,7 @@ async function driveStory(story) { if (fix.needsHumanDecision) { if (remediated) await agent( - `Story ${tag}: escalating a design disagreement to a human. ${wtClause(story)} Read \`${reviewLog}\`. FIRST minimize / mark-outdated any prior escalate-flush comment already posted on PR #${pr.prNumber} — a new flush SUPERSEDES the previous one, so only the newest stays visible (no-op if there is none). THEN post ONE fresh comment on PR #${pr.prNumber} (response to the first review) summarizing the remediation rounds so far, the still-open findings (${JSON.stringify(prevFindings)}) and the open decision. Do NOT delete the log — it is the continuation anchor for this cycle. CONVENTION (state it in the comment): any further rework/re-review — including manual out-of-band rounds — funnels into THIS same working log (append), NOT standalone PR comments; the next orchestrated run continues the cycle and its convergence synthesizes ONE final remediation and minimizes these intermediate comments. Note too (in the comment) that this working log is an UNTRACKED file living ONLY in the persistent authoring worktree \`../pair-worktrees/${story.id}\`, so that worktree must be PRESERVED until merge — if it is pruned/recreated the audit log is lost (this flush + the first-review comment still remain on the PR, and the PR-side first-review signal still prevents a duplicate first review on the next run). Do NOT merge.`, + `Story ${tag}: escalating a design disagreement to a human. ${wtClause(story)} Read \`${reviewLog}\`. ${flushConvention(story, pr.prNumber)} THEN post ONE fresh comment on PR #${pr.prNumber} (response to the first review) summarizing the remediation rounds so far, the still-open findings (${JSON.stringify(prevFindings)}) and the open decision. Do NOT delete the log — it is the continuation anchor for this cycle. Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, ) return { story, prNumber: pr.prNumber, status: 'escalate', findings: prevFindings, acceptedFindings: accepted } diff --git a/.claude/workflows/implement-batch.test.mjs b/.claude/workflows/implement-batch.test.mjs index b2e13769..d82e3502 100644 --- a/.claude/workflows/implement-batch.test.mjs +++ b/.claude/workflows/implement-batch.test.mjs @@ -424,6 +424,66 @@ test('#373 probe queries BOTH signals and is dialed to the cheap model/effort po assert.ok(probe.prompt.includes('logExists') && probe.prompt.includes('firstReviewPosted'), 'probe reports both the log-existence and the PR-side first-review signal') }) +test('#373 finding 1: the first review emits a hidden marker and the probe matches it DETERMINISTICALLY (no semantic template-structure judgment)', async () => { + // The probe runs at haiku/low. It must NOT classify a comment by reading its structure + // (a false positive would silence a legitimate first review — the story's High-impact + // over-silencing risk). Instead the first review emits a fixed hidden marker and the probe + // does a plain EXACT substring match on that same marker. + const dispatch = (prompt, opts) => { + if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.label?.startsWith('probe:')) return { logExists: false, firstReviewPosted: false } + if (opts.agentType === 'reviewer') return { verdict: 'Approved', findings: [] } + return { fixed: true } + } + const { calls } = await runWorkflow({ args: { stories: [RESUME_STORY] }, dispatch }) + const marker = `` + + const first = calls.find(c => c.opts.agentType === 'reviewer') + assert.ok(first.prompt.includes(marker), 'the first review emits the exact hidden marker verbatim') + assert.ok(/HTML comment/i.test(first.prompt) && /invisible/i.test(first.prompt), 'marker is documented as an invisible HTML comment (no visible noise)') + + const probe = calls.find(c => c.opts.label?.startsWith('probe:')) + assert.ok(probe.prompt.includes(marker), 'the probe matches the SAME marker the first review emits') + assert.ok(/EXACT marker substring|plain substring match|DETERMINISTICALLY/.test(probe.prompt), 'probe is a deterministic substring match, not a judgment') + assert.ok(!/Overall Assessment|Review Summary/.test(probe.prompt), 'probe no longer relies on a semantic template-structure reading of the comment') +}) + +test('#373 finding 3: both escalate-flush prompts share ONE authored convention block (supersede + out-of-band + untracked-worktree), so they cannot diverge', async () => { + // MAX_FIX_ROUNDS escalation (fresh-story path, remediated set by a prior fix round). + const finding = { location: 'x.ts:1', severity: 'Minor', description: 'never fixed', recommendation: 'r' } + const maxRoundsFlush = (await runWorkflow({ + args: { stories: [STORY] }, + dispatch: (prompt, opts) => { + if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.agentType === 'reviewer') return { verdict: 'Rework', findings: [finding] } + if (opts.phase === 'Implement') return { gatesPassed: true, branch: 'b' } + if (opts.phase === 'PR') return { prNumber: 7 } + if (opts.label?.startsWith('flush:')) return 'flushed' + return { fixed: true } + }, + })).calls.find(c => c.opts.label?.startsWith('flush:')) + + // needsHumanDecision escalation (fixer escalates a design disagreement on a continuation). + const designFlush = (await runWorkflow({ + args: { stories: [RESUME_STORY] }, + dispatch: (prompt, opts) => { + if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.label?.startsWith('probe:')) return { logExists: true, firstReviewPosted: true } + if (opts.agentType === 'reviewer') return { verdict: 'Rework', findings: [finding] } + if (opts.label?.startsWith('flush:')) return 'flushed' + return { needsHumanDecision: true } // fixer escalates a design disagreement + }, + })).calls.find(c => c.opts.label?.startsWith('flush:')) + + assert.ok(maxRoundsFlush && designFlush, 'both escalation paths post a flush') + // The extracted convention block is byte-identical in both prompts (single source of truth). + const block = /SUPERSEDES the last[\s\S]*prevents a duplicate first review on the next run\)/ + const a = maxRoundsFlush.prompt.match(block) + const b = designFlush.prompt.match(block) + assert.ok(a && b, 'both flush prompts carry the shared convention block') + assert.equal(a[0], b[0], 'the convention block is identical in both flush prompts (no drift)') +}) + test('#373 escalate ON A CONTINUATION: resume + existing log + never-converging re-review keeps the log, flushes (remediated seeded true), supersedes prior flush, no synth (AC5 on the resume path)', async () => { const finding = { location: 'x.ts:1', severity: 'Minor', description: 'never fixed', recommendation: 'r' } const dispatch = (prompt, opts) => { From 6b5ee22654cec353670c30da3cb7ae38ffe05763 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 24 Jul 2026 13:13:49 +0200 Subject: [PATCH 5/9] [#373] fix: broaden synth-minimize to prior convergence + rename remediated flag (review round 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - finding 1 (Minor): convergence-synthesis minimize clause now also covers a prior convergence's own final-remediation comment (re-run→re-converge edge), with explicit first-review carve-out, so the one-remediation invariant (AC2) holds on re-entry. +2 assertions locking it. - finding 2 (Minor): rename remediated → cycleHasRemediation (reads as 'this cycle has remediation state', not 'a fix happened this run') across js + test strings. node --check clean; node --test → 23/23 PASS. Nothing escalated. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/workflows/implement-batch.js | 34 ++++++++++++---------- .claude/workflows/implement-batch.test.mjs | 21 ++++++++----- 2 files changed, 31 insertions(+), 24 deletions(-) diff --git a/.claude/workflows/implement-batch.js b/.claude/workflows/implement-batch.js index c8a92028..ceb6f8da 100644 --- a/.claude/workflows/implement-batch.js +++ b/.claude/workflows/implement-batch.js @@ -128,7 +128,7 @@ const FIX_SCHEMA = { // agent in the worktree reports two signals used to decide whether round-0 must post // a fresh first review: // - logExists: the persisted working log is present → an in-flight cycle to CONTINUE -// (silent round-0 + seeds `remediated` so convergence still synthesizes+cleans). +// (silent round-0 + seeds `cycleHasRemediation` so convergence still synthesizes+cleans). // - firstReviewPosted: a first-review comment already exists on the PR (PR-side // corroboration). Guards the double-first-review the log-only signal can miss when // the log is GONE but a first review was already posted — e.g. a converged-but-not- @@ -307,13 +307,14 @@ async function driveStory(story) { // HTML-comment marker and the probe does an EXACT substring match on it — NOT a semantic // reading of the comment's structure — so the cheap haiku/low probe can't misclassify a // non-review comment into silencing a real first review (finding 1). Log existence - // additionally seeds `remediated` so convergence still + // additionally seeds `cycleHasRemediation` so convergence still // synthesizes+cleans even if round-0 converges immediately; a first-review-only signal // (no log) does NOT seed it, so a clean round-0 adds nothing and never synths a gone log. // - At convergence ONE synthesized remediation comment is posted, written // CONTEXTUALLY to the first review (maps EVERY finding across ALL runs in the log // -> resolution + accepted dispositions + final verdict), AND any prior intermediate - // comments (escalate-flush or manual out-of-band rounds) are minimized / marked + // comments (escalate-flush, manual out-of-band rounds, OR a prior convergence's own + // final-remediation comment on a re-run→re-converge cycle) are minimized / marked // outdated so only first-review + this one remediation remain visible; the log is // then deleted. // - On escalation the log is KEPT and flushed to the PR as the continuation anchor. A @@ -363,13 +364,14 @@ async function driveStory(story) { let round = 0 let prevFindings = [] let accepted = [] - // #373: on a continuation (log present) seed `remediated` so an immediate round-0 - // convergence still posts the ONE final synthesis + deletes the log (never leaves an - // escalate-flush as the last word). A converged-but-unmerged re-run has NO log - // (firstReviewPosted true, isContinuation false) → remediated stays false, so a clean - // round-0 adds nothing and never tries to synth a deleted log. A fresh cycle starts - // false, so a clean first review stands alone (AC6). - let remediated = isContinuation + // #373: `cycleHasRemediation` tracks whether THIS CYCLE (across all runs it spans) has + // any remediation state to synthesize — not merely whether a fix happened this run. On a + // continuation (log present) it is seeded true so an immediate round-0 convergence still + // posts the ONE final synthesis + deletes the log (never leaves an escalate-flush as the + // last word). A converged-but-unmerged re-run has NO log (firstReviewPosted true, + // isContinuation false) → stays false, so a clean round-0 adds nothing and never tries to + // synth a deleted log. A fresh cycle starts false, so a clean first review stands alone (AC6). + let cycleHasRemediation = isContinuation while (true) { // #373: round-0 is the FIRST (posted) review ONLY on a genuinely fresh cycle — no // in-flight log AND no first-review comment already on the PR. Either signal makes @@ -385,7 +387,7 @@ async function driveStory(story) { // Converge once nothing actionable remains (by-design findings don't block). if (actionable.length === 0) break if (round >= MAX_FIX_ROUNDS || review?.needsHumanDecision) { - if (remediated) + if (cycleHasRemediation) await agent( `Story ${tag}: the review<->fix loop is escalating to a human (non-convergence or a design disagreement). ${wtClause(story)} Read the review log \`${reviewLog}\`. ${flushConvention(story, pr.prNumber)} THEN post ONE fresh comment on PR #${pr.prNumber} — written as a response to the first code-review comment — summarizing the rounds so far (per finding: what was attempted + current state) and the still-open actionable findings: ${JSON.stringify(actionable)}. Do NOT delete the log — it is the continuation anchor for this cycle. Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, @@ -395,7 +397,7 @@ async function driveStory(story) { round++ prevFindings = actionable - remediated = true + cycleHasRemediation = true // FIX — implementer resumes checkpoint (if present) + resolves actionable findings. // Logs the round to the working review log INSTEAD of posting a per-round PR comment. const fix = await agent( @@ -404,9 +406,9 @@ async function driveStory(story) { ) // failed-fix: the fixer died mid-round; a partial working log may exist. Surface // its path in the return so the human / next resume can find (and clean) it. - if (!fix) return { story, prNumber: pr.prNumber, status: 'failed-fix', reviewLog: remediated ? reviewLog : undefined } + if (!fix) return { story, prNumber: pr.prNumber, status: 'failed-fix', reviewLog: cycleHasRemediation ? reviewLog : undefined } if (fix.needsHumanDecision) { - if (remediated) + if (cycleHasRemediation) await agent( `Story ${tag}: escalating a design disagreement to a human. ${wtClause(story)} Read \`${reviewLog}\`. ${flushConvention(story, pr.prNumber)} THEN post ONE fresh comment on PR #${pr.prNumber} (response to the first review) summarizing the remediation rounds so far, the still-open findings (${JSON.stringify(prevFindings)}) and the open decision. Do NOT delete the log — it is the continuation anchor for this cycle. Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, @@ -419,9 +421,9 @@ async function driveStory(story) { // post ONE synthesized remediation comment (contextual to the first review), minimize any // prior intermediate comments, and delete the working log. If the first review was already // clean (fresh cycle, no remediation), the first-review comment stands alone — nothing to do. - if (remediated) + if (cycleHasRemediation) await agent( - `Story ${tag} converged: the latest independent re-review found zero actionable findings. ${wtClause(story)} Read the review log \`${reviewLog}\` — it may span MULTIPLE runs / escalations / manual rounds of this ONE cycle. Post ONE remediation comment on PR #${pr.prNumber}, written as a direct RESPONSE to the first code-review comment: map EVERY finding recorded across ALL runs in the log (plus any surfaced during remediation) to how it was resolved (with commit refs), list any accepted/non-actionable findings with their dispositions (${JSON.stringify(accepted)}), and state the final verdict (review clean). THEN minimize / mark-outdated any prior intermediate PR comments on #${pr.prNumber} — earlier escalate-flush comments and any manual out-of-band rework/re-review comments — so that ONLY the first review comment and this one final remediation remain as the visible current state (if there are none to minimize, that step is a no-op). This single comment IS the durable audit of the ENTIRE review<->fix cycle across every run. Then DELETE \`${reviewLog}\`. Do NOT merge.`, + `Story ${tag} converged: the latest independent re-review found zero actionable findings. ${wtClause(story)} Read the review log \`${reviewLog}\` — it may span MULTIPLE runs / escalations / manual rounds of this ONE cycle. Post ONE remediation comment on PR #${pr.prNumber}, written as a direct RESPONSE to the first code-review comment: map EVERY finding recorded across ALL runs in the log (plus any surfaced during remediation) to how it was resolved (with commit refs), list any accepted/non-actionable findings with their dispositions (${JSON.stringify(accepted)}), and state the final verdict (review clean). THEN minimize / mark-outdated any prior intermediate PR comments on #${pr.prNumber} — earlier escalate-flush comments, any manual out-of-band rework/re-review comments, AND any earlier final-remediation/synthesis comment left by a prior convergence of this same cycle (a converged-but-unmerged PR that was re-run, found new findings and re-converged — do NOT minimize the first review comment) — so that ONLY the first review comment and this one final remediation remain as the visible current state (if there are none to minimize, that step is a no-op). This single comment IS the durable audit of the ENTIRE review<->fix cycle across every run. Then DELETE \`${reviewLog}\`. Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `synth:${tag}`, model: 'sonnet', effort: 'medium' }, ) diff --git a/.claude/workflows/implement-batch.test.mjs b/.claude/workflows/implement-batch.test.mjs index d82e3502..90acd513 100644 --- a/.claude/workflows/implement-batch.test.mjs +++ b/.claude/workflows/implement-batch.test.mjs @@ -322,7 +322,7 @@ test('#373 continuation (resume + existing log): probe runs, round-0 review is S assert.ok(!reviews[0].prompt.includes('This is the FIRST review: POST'), 'no second first-review is posted') const synth = calls.find(c => c.opts.label?.startsWith('synth:')) - assert.ok(synth, 'immediate convergence on a continuation still synthesizes (remediated seeded true)') + assert.ok(synth, 'immediate convergence on a continuation still synthesizes (cycleHasRemediation seeded true)') assert.equal(result.batch[0].status, 'ready-for-merge') }) @@ -345,6 +345,11 @@ test('#373 continuation convergence: the ONE synthesis maps ALL runs, minimizes assert.ok(synth, 'convergence synthesizes') assert.ok(/ALL runs/i.test(synth.prompt), 'synthesis maps findings across ALL runs of the cycle') assert.ok(/minimize/i.test(synth.prompt) && /outdated/i.test(synth.prompt), 'synthesis minimizes / marks-outdated prior intermediate comments') + // #373 round-5 finding 1: the minimize set must also cover a PRIOR convergence's own + // final-remediation comment (re-run→re-converge edge), while NEVER the first review, so the + // 'at most one final remediation' invariant holds on re-entry. + assert.ok(/prior convergence/i.test(synth.prompt), 'synthesis minimizes a prior convergence\'s own final-remediation comment (re-run→re-converge edge)') + assert.ok(/do NOT minimize the first review/i.test(synth.prompt), 'the first-review comment is explicitly excluded from the minimize set') assert.ok(synth.prompt.includes('DELETE'), 'synthesis deletes the log at the end') assert.equal(result.batch[0].status, 'ready-for-merge') }) @@ -361,7 +366,7 @@ test('#373 resume with NO prior log: round-0 is a FRESH first review (posted), n assert.ok(probe, 'probe still runs on resume') const reviews = calls.filter(c => c.opts.agentType === 'reviewer') assert.ok(reviews[0].prompt.includes('This is the FIRST review: POST'), 'no log → round-0 posts a fresh first review') - assert.ok(!calls.some(c => c.opts.label?.startsWith('synth:')), 'clean fresh review on resume → no synthesis (remediated stayed false)') + assert.ok(!calls.some(c => c.opts.label?.startsWith('synth:')), 'clean fresh review on resume → no synthesis (cycleHasRemediation stayed false)') }) test('#373 fresh story: no continuation probe runs (fresh path unchanged, AC6)', async () => { @@ -392,7 +397,7 @@ test('#373 escalate documents the manual out-of-band convention (funnel into the test('#373 resume with NO log but a first review ALREADY on the PR: round-0 is SILENT (no duplicate first review), clean → no synth (findings 1 & 3)', async () => { // Converged-but-unmerged re-run (log deleted at convergence) OR a pruned/out-of-band // clone that lost the untracked log: the PR-side `firstReviewPosted` signal must still - // suppress a second first-review. remediated stays false (no log to continue), so a + // suppress a second first-review. cycleHasRemediation stays false (no log to continue), so a // clean round-0 adds nothing and never tries to synthesize a gone log. const dispatch = (prompt, opts) => { if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } @@ -449,7 +454,7 @@ test('#373 finding 1: the first review emits a hidden marker and the probe match }) test('#373 finding 3: both escalate-flush prompts share ONE authored convention block (supersede + out-of-band + untracked-worktree), so they cannot diverge', async () => { - // MAX_FIX_ROUNDS escalation (fresh-story path, remediated set by a prior fix round). + // MAX_FIX_ROUNDS escalation (fresh-story path, cycleHasRemediation set by a prior fix round). const finding = { location: 'x.ts:1', severity: 'Minor', description: 'never fixed', recommendation: 'r' } const maxRoundsFlush = (await runWorkflow({ args: { stories: [STORY] }, @@ -484,7 +489,7 @@ test('#373 finding 3: both escalate-flush prompts share ONE authored convention assert.equal(a[0], b[0], 'the convention block is identical in both flush prompts (no drift)') }) -test('#373 escalate ON A CONTINUATION: resume + existing log + never-converging re-review keeps the log, flushes (remediated seeded true), supersedes prior flush, no synth (AC5 on the resume path)', async () => { +test('#373 escalate ON A CONTINUATION: resume + existing log + never-converging re-review keeps the log, flushes (cycleHasRemediation seeded true), supersedes prior flush, no synth (AC5 on the resume path)', async () => { const finding = { location: 'x.ts:1', severity: 'Minor', description: 'never fixed', recommendation: 'r' } const dispatch = (prompt, opts) => { if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } @@ -495,11 +500,11 @@ test('#373 escalate ON A CONTINUATION: resume + existing log + never-converging } const { result, calls } = await runWorkflow({ args: { stories: [RESUME_STORY] }, dispatch }) assert.equal(result.batch[0].status, 'escalate') - // remediated was seeded true by the continuation, so the flush fires even though the + // cycleHasRemediation was seeded true by the continuation, so the flush fires even though the // escalation happened on round-0 of a RESUMED cycle (fresh-story path only reaches the - // guarded flush after a fix round sets remediated). + // guarded flush after a fix round sets cycleHasRemediation). const flush = calls.find(c => c.opts.label?.startsWith('flush:')) - assert.ok(flush, 'continuation escalation posts a flush (remediated seeded true from the existing log)') + assert.ok(flush, 'continuation escalation posts a flush (cycleHasRemediation seeded true from the existing log)') assert.ok(flush.prompt.includes('x.ts:1'), 'flush carries the still-open findings') assert.ok(flush.prompt.includes('Do NOT delete the log'), 'the continuation anchor log is kept') assert.ok(/minimize|supersede/i.test(flush.prompt), 'a new escalate-flush supersedes/minimizes the prior one (finding 2)') From d4805d8e5b0e46c9cf42aee11548be026ea2fdd8 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 24 Jul 2026 13:20:21 +0200 Subject: [PATCH 6/9] [#373] fix: escalate flush also supersedes a prior convergence remediation Round-6 review finding: flushConvention only minimized a prior escalate-flush, not a prior convergence's final-remediation comment. Converged-but-unmerged PR re-run that then escalates left a stale "ready for merge" remediation visible beside the active escalate-flush. Mirror the synth-path minimize clause (never the first-review comment). Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/workflows/implement-batch.js | 8 ++++++-- .claude/workflows/implement-batch.test.mjs | 6 ++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/.claude/workflows/implement-batch.js b/.claude/workflows/implement-batch.js index ceb6f8da..edd5b3ae 100644 --- a/.claude/workflows/implement-batch.js +++ b/.claude/workflows/implement-batch.js @@ -256,7 +256,7 @@ function revWtClause(story) { // change to the convention or the worktree-persistence wording is made in one place and can't // silently diverge between the two paths (they had already drifted slightly before this). function flushConvention(story, prNumber) { - return `FIRST minimize / mark-outdated any prior escalate-flush comment already posted on PR #${prNumber} — each flush "summarizes the rounds so far", so a new one SUPERSEDES the last; only the newest escalate-flush should stay visible (no-op if there is none). CONVENTION (state it in the comment so the human/orchestrator knows): any further rework or re-review — including manual out-of-band rounds — should be funneled into THIS same working log (append), NOT posted as standalone PR comments; the next orchestrated run on this story continues the same cycle and its convergence will synthesize ONE final remediation and minimize these intermediate comments. Note too (in the comment) that this working log is an UNTRACKED file living ONLY in the persistent authoring worktree \`../pair-worktrees/${story.id}\`, so that worktree must be PRESERVED until merge — if it is pruned/recreated the audit log is lost (this flush + the first-review comment still remain on the PR, and the PR-side first-review signal still prevents a duplicate first review on the next run).` + return `FIRST minimize / mark-outdated any prior escalate-flush comment already posted on PR #${prNumber} — each flush "summarizes the rounds so far", so a new one SUPERSEDES the last; only the newest escalate-flush should stay visible (no-op if there is none). ALSO minimize / mark-outdated any prior final-remediation/synthesis comment left by an EARLIER convergence of this SAME cycle (a converged-but-unmerged PR that was re-run, found new findings and is now escalating): its "review clean / ready for merge" verdict directly contradicts an active escalation, so it must NOT stay visible alongside this flush — mirror the convergence-synthesis path (no-op if there is none), but NEVER minimize the first-review comment. CONVENTION (state it in the comment so the human/orchestrator knows): any further rework or re-review — including manual out-of-band rounds — should be funneled into THIS same working log (append), NOT posted as standalone PR comments; the next orchestrated run on this story continues the same cycle and its convergence will synthesize ONE final remediation and minimize these intermediate comments. Note too (in the comment) that this working log is an UNTRACKED file living ONLY in the persistent authoring worktree \`../pair-worktrees/${story.id}\`, so that worktree must be PRESERVED until merge — if it is pruned/recreated the audit log is lost (this flush + the first-review comment still remain on the PR, and the PR-side first-review signal still prevents a duplicate first review on the next run).` } // ── Per-story lifecycle ────────────────────────────────────────────────── @@ -319,7 +319,11 @@ async function driveStory(story) { // then deleted. // - On escalation the log is KEPT and flushed to the PR as the continuation anchor. A // new escalate-flush SUPERSEDES the prior one (minimized/marked-outdated in place), so - // repeated escalations across runs leave only the newest flush visible, not a pile. + // repeated escalations across runs leave only the newest flush visible, not a pile. It + // ALSO minimizes any prior convergence's own final-remediation comment (a converged-but- + // unmerged PR re-run that now escalates) — a stale "ready for merge" verdict must not + // stay visible next to an active escalation (never the first-review comment), mirroring + // the convergence-synthesis minimize set. // - MANUAL OUT-OF-BAND CONVENTION (#373): if a human/orchestrator takes over rework or // re-review after an escalate, they funnel their notes into THIS same working log // (append) rather than posting standalone PR comments; the next orchestrated run diff --git a/.claude/workflows/implement-batch.test.mjs b/.claude/workflows/implement-batch.test.mjs index 90acd513..18ce4978 100644 --- a/.claude/workflows/implement-batch.test.mjs +++ b/.claude/workflows/implement-batch.test.mjs @@ -290,6 +290,12 @@ test('non-convergence: MAX_FIX_ROUNDS escalation flushes the working log to the assert.ok(flush.prompt.includes('x.ts:1'), 'flush carries the still-open findings') assert.ok(flush.prompt.includes('.pair/working/reviews/292.md') && flush.prompt.includes('Do NOT delete the log'), 'flush reads the log and keeps it for the human') assert.ok(/UNTRACKED|PRESERVED|pruned/.test(flush.prompt) && flush.prompt.includes('../pair-worktrees/292'), 'flush documents the worktree-persistence assumption of the untracked log (finding 3)') + // #373 round-6 finding: the flush must ALSO minimize a prior convergence's final-remediation + // comment (converged-but-unmerged re-run that now escalates) — a stale "ready for merge" verdict + // cannot stay visible beside an active escalation; never the first-review comment. Mirrors the + // synth-path minimize set. + assert.ok(/final-remediation\/synthesis comment left by an EARLIER convergence/i.test(flush.prompt), 'flush minimizes a prior convergence\'s own final-remediation comment (round-6 finding)') + assert.ok(/NEVER minimize the first-review comment/i.test(flush.prompt), 'flush carves out the first-review comment from the minimize set') assert.ok(!calls.some(c => c.opts.label?.startsWith('synth:')), 'no synthesis on escalation') }) From f92bf55f42ec0103214e7ac816ab2a2736618f64 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 24 Jul 2026 16:50:12 +0200 Subject: [PATCH 7/9] [#373] fix: resume-no-log escalate visibility + sonnet probe + PR labels (review round 7) - escalate guard `cycleHasRemediation || !first`: resume-path round-0 escalation (no log, first review already on PR) now posts a best-effort flush from inline findings instead of surfacing only in the batch return value - continuation round-0 RE-REVIEW prompt: fresh-pass branch replaces vacuous "verify []"; casing aligned so silent-re-review assertions still match - continuation probe bumped haiku->sonnet (effort low): fails open toward a duplicate first review, so reliability of worktree+gh substring match wins the small tier bump; code+comment+tests now agree - tests: +finding-1 escalate-from-inline case; finding-4 test asserts sonnet (24/24) Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/workflows/implement-batch.js | 30 +++++++++++++---- .claude/workflows/implement-batch.test.mjs | 39 ++++++++++++++++++++-- 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/.claude/workflows/implement-batch.js b/.claude/workflows/implement-batch.js index edd5b3ae..2375f5cc 100644 --- a/.claude/workflows/implement-batch.js +++ b/.claude/workflows/implement-batch.js @@ -305,8 +305,12 @@ async function driveStory(story) { // but-unmerged re-run or a lost/pruned untracked log can't produce a duplicate first // review). The PR-side signal is DETERMINISTIC: the first review emits a fixed hidden // HTML-comment marker and the probe does an EXACT substring match on it — NOT a semantic - // reading of the comment's structure — so the cheap haiku/low probe can't misclassify a - // non-review comment into silencing a real first review (finding 1). Log existence + // reading of the comment's structure — so the probe can't misclassify a + // non-review comment into silencing a real first review (finding 1). The probe runs + // at sonnet/low (not haiku): its job orchestrates a worktree + a `gh` fetch + a + // substring match, and a mis-report fails OPEN toward a duplicate first review (the + // very noise this story removes), so the reliability of those tool steps is worth the + // small tier bump over the cheapest model. Log existence // additionally seeds `cycleHasRemediation` so convergence still // synthesizes+cleans even if round-0 converges immediately; a first-review-only signal // (no log) does NOT seed it, so a clean round-0 adds nothing and never synths a gone log. @@ -351,7 +355,7 @@ async function driveStory(story) { if (resuming) { const probe = await agent( `Story ${tag}: read-only CONTINUATION PROBE (no review, no edits). ${wtClause(story)} Report TWO booleans: (1) \`logExists\` — is the review working log \`${reviewLog}\` present in the worktree? (2) \`firstReviewPosted\` — does PR #${pr.prNumber} ALREADY carry the first-review comment? Match it DETERMINISTICALLY, not by judgment: fetch the PR comments via \`gh\` and report whether ANY comment's raw body contains the EXACT marker substring \`${firstReviewMarker}\` (the first review always emits this hidden marker verbatim; a minimized/outdated comment still counts — its raw body still contains the marker). Do NOT infer from a comment's structure or tone — it is a plain substring match. Return { logExists, firstReviewPosted }. Do NOT create, modify, or delete the log, do NOT post or minimize any comment, and do NOT run the review — this is a cheap probe to decide whether an in-flight review cycle is being CONTINUED and whether a first review was already posted.`, - { agentType: 'implementer', phase: 'Review', label: `probe:${tag}`, model: 'haiku', effort: 'low', schema: PROBE_SCHEMA }, + { agentType: 'implementer', phase: 'Review', label: `probe:${tag}`, model: 'sonnet', effort: 'low', schema: PROBE_SCHEMA }, ) // #373 finding 4: a failed / malformed / schema-invalid probe return yields BOTH signals // false (via `?.x === true`), so round-0 falls through to a POSTED first review. This @@ -382,7 +386,9 @@ async function driveStory(story) { // round-0 a SILENT re-review, so a PR never accrues a second first-review. const first = round === 0 && !isContinuation && !firstReviewPosted const review = await agent( - `Independently review PR #${pr.prNumber} for story ${tag}, following /pair-process-review. ${revWtClause(story)} Review ONLY from the story's acceptance criteria, the PR diff+description, and the code. Do NOT read .pair/working/. Report EVERY finding regardless of severity (including minor/nit), using the code-review-template vocabulary: each finding = \`location\` (File:Line), \`severity\` ∈ {${SEVERITIES}}, \`description\` (issue + impact), \`recommendation\`; verdict ∈ {${VERDICTS}}. Set \`nonActionable: true\` ONLY if fixing it would be genuinely WRONG (byte-consistent with a source of truth, matches an existing convention/already-tracked deferred plan, resolves only after merge, etc.) — being outside this story's originally stated scope is NOT by itself a reason to mark something nonActionable: a real, fixable gap found during review gets fixed in this same PR unless it is large enough to warrant its own story (state that explicitly in the description if so). Whenever you set \`nonActionable: true\`, ALSO set \`disposition\` — a specific reason that replaces the bare label: write exactly \`Deferred to #\` when the finding belongs to a separate tracked story (file one via /pair-capability-write-issue if none exists yet), otherwise a concrete by-design reason (\`By convention …\` / \`Historical record\` / \`Forward-ref to unbuilt #\` / \`Resolves after merge\`); never leave "non-actionable" as the only explanation. ${first ? `This is the FIRST review: POST your full review report as a PR comment on #${pr.prNumber} (code-review-template structure), and include the marker line \`${firstReviewMarker}\` VERBATIM as the first line of the comment body — it is an HTML comment (invisible in the rendered markdown, so no visible noise) that lets a later resume detect this first review by an EXACT substring match rather than a semantic reading (finding 1). Then return findings + verdict.` : `This is a RE-REVIEW: do NOT post any PR comment (the orchestrator synthesizes the cycle at the end). Return findings + verdict only. Verify these prior findings were genuinely resolved: ${JSON.stringify(prevFindings)}.`} Return findings and a verdict.`, + `Independently review PR #${pr.prNumber} for story ${tag}, following /pair-process-review. ${revWtClause(story)} Review ONLY from the story's acceptance criteria, the PR diff+description, and the code. Do NOT read .pair/working/. Report EVERY finding regardless of severity (including minor/nit), using the code-review-template vocabulary: each finding = \`location\` (File:Line), \`severity\` ∈ {${SEVERITIES}}, \`description\` (issue + impact), \`recommendation\`; verdict ∈ {${VERDICTS}}. Set \`nonActionable: true\` ONLY if fixing it would be genuinely WRONG (byte-consistent with a source of truth, matches an existing convention/already-tracked deferred plan, resolves only after merge, etc.) — being outside this story's originally stated scope is NOT by itself a reason to mark something nonActionable: a real, fixable gap found during review gets fixed in this same PR unless it is large enough to warrant its own story (state that explicitly in the description if so). Whenever you set \`nonActionable: true\`, ALSO set \`disposition\` — a specific reason that replaces the bare label: write exactly \`Deferred to #\` when the finding belongs to a separate tracked story (file one via /pair-capability-write-issue if none exists yet), otherwise a concrete by-design reason (\`By convention …\` / \`Historical record\` / \`Forward-ref to unbuilt #\` / \`Resolves after merge\`); never leave "non-actionable" as the only explanation. ${first ? `This is the FIRST review: POST your full review report as a PR comment on #${pr.prNumber} (code-review-template structure), and include the marker line \`${firstReviewMarker}\` VERBATIM as the first line of the comment body — it is an HTML comment (invisible in the rendered markdown, so no visible noise) that lets a later resume detect this first review by an EXACT substring match rather than a semantic reading (finding 1). Then return findings + verdict.` : prevFindings.length + ? `This is a RE-REVIEW: do NOT post any PR comment (the orchestrator synthesizes the cycle at the end). Return findings + verdict only. Verify these prior findings were genuinely resolved: ${JSON.stringify(prevFindings)}.` + : `This is a RE-REVIEW on a resumed in-flight cycle (round-0 of this run carries no prior findings): do a FRESH, independent full review pass. do NOT post any PR comment (the orchestrator synthesizes the cycle at the end). Return findings + verdict only.`} Return findings and a verdict.`, { agentType: 'reviewer', phase: 'Review', label: `rev:${tag} r${round}`, effort: 'xhigh', schema: REVIEW_SCHEMA }, ) const findings = review?.findings ?? [] @@ -391,11 +397,23 @@ async function driveStory(story) { // Converge once nothing actionable remains (by-design findings don't block). if (actionable.length === 0) break if (round >= MAX_FIX_ROUNDS || review?.needsHumanDecision) { - if (cycleHasRemediation) + // #373 finding 1: emit a PR-visible escalation UNLESS this run's round-0 ALREADY posted + // the first review (`first === true`) carrying these same findings. The gap this closes: + // a SILENT re-review that escalates with no log — a resumed PR whose prior first review + // exists but whose untracked working log was never written / was pruned (firstReviewPosted + // true, isContinuation false → cycleHasRemediation false, first false). Without the `!first` + // arm the new blocking concern surfaced ONLY in the batch return value and a later resume + // repeated the silent escalation. The log read is BEST-EFFORT: only a continuing cycle + // (cycleHasRemediation) has a log to anchor to; the no-log arm escalates from inline findings. + if (cycleHasRemediation || !first) { + const logClause = cycleHasRemediation + ? `Read the review log \`${reviewLog}\`. ${flushConvention(story, pr.prNumber)} THEN ` + : `No prior review working log exists (a re-review on a resumed PR whose log was never written or was pruned) — escalate from the inline findings directly. ` await agent( - `Story ${tag}: the review<->fix loop is escalating to a human (non-convergence or a design disagreement). ${wtClause(story)} Read the review log \`${reviewLog}\`. ${flushConvention(story, pr.prNumber)} THEN post ONE fresh comment on PR #${pr.prNumber} — written as a response to the first code-review comment — summarizing the rounds so far (per finding: what was attempted + current state) and the still-open actionable findings: ${JSON.stringify(actionable)}. Do NOT delete the log — it is the continuation anchor for this cycle. Do NOT merge.`, + `Story ${tag}: the review<->fix loop is escalating to a human (non-convergence or a design disagreement). ${wtClause(story)} ${logClause}post ONE fresh comment on PR #${pr.prNumber} — written as a response to the first code-review comment — summarizing${cycleHasRemediation ? ' the rounds so far (per finding: what was attempted + current state) and' : ''} the still-open actionable findings: ${JSON.stringify(actionable)}.${cycleHasRemediation ? ' Do NOT delete the log — it is the continuation anchor for this cycle.' : ''} Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, ) + } return { story, prNumber: pr.prNumber, status: 'escalate', findings: actionable, acceptedFindings: accepted } } diff --git a/.claude/workflows/implement-batch.test.mjs b/.claude/workflows/implement-batch.test.mjs index 18ce4978..d0f93eb2 100644 --- a/.claude/workflows/implement-batch.test.mjs +++ b/.claude/workflows/implement-batch.test.mjs @@ -420,7 +420,40 @@ test('#373 resume with NO log but a first review ALREADY on the PR: round-0 is S assert.equal(result.batch[0].status, 'ready-for-merge') }) -test('#373 probe queries BOTH signals and is dialed to the cheap model/effort policy (haiku/low)', async () => { +test('#373 finding 1: resume, NO log + first review already on PR, round-0 ESCALATES → flush still posts from inline findings (best-effort log read)', async () => { + // The escalate-visibility gap: firstReviewPosted=true + logExists=false means round-0 is a + // SILENT re-review (first=false) AND cycleHasRemediation stays false (seeded only from the + // log). If round-0 returns needsHumanDecision, the escalation must STILL leave a PR-visible + // artifact — otherwise the new blocking concern surfaces only in the batch return value and a + // later resume repeats the silent escalation. The `|| !first` arm posts a flush; because + // there is no log to anchor to, it escalates from the inline findings directly. + const finding = { location: 'x.ts:1', severity: 'Blocker', description: 'design disagreement', recommendation: 'r' } + const dispatch = (prompt, opts) => { + if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } + if (opts.label?.startsWith('probe:')) return { logExists: false, firstReviewPosted: true } + if (opts.agentType === 'reviewer') return { verdict: 'Rework', findings: [finding], needsHumanDecision: true } + if (opts.label?.startsWith('flush:')) return 'flushed' + return { fixed: true } + } + const { result, calls } = await runWorkflow({ args: { stories: [RESUME_STORY] }, dispatch }) + assert.equal(result.batch[0].status, 'escalate') + const reviews = calls.filter(c => c.opts.agentType === 'reviewer') + assert.equal(reviews.length, 1, 'round-0 only (escalates immediately)') + assert.ok(reviews[0].prompt.includes('do NOT post any PR comment'), 'round-0 is SILENT (first review already on PR)') + const flush = calls.find(c => c.opts.label?.startsWith('flush:')) + assert.ok(flush, 'a resume-path round-0 escalation STILL posts a flush (finding 1: no silent escalation)') + assert.ok(flush.prompt.includes('x.ts:1'), 'flush carries the still-open actionable findings') + assert.ok(/No prior review working log exists/i.test(flush.prompt), 'no-log arm: escalates from inline findings, not from a (missing) log') + assert.ok(!flush.prompt.includes('Read the review log'), 'no-log arm does not instruct reading a non-existent log (best-effort)') + assert.ok(!flush.prompt.includes('Do NOT delete the log'), 'no-log arm has no log to keep as an anchor') + assert.ok(!calls.some(c => c.opts.label?.startsWith('synth:')), 'escalation never synthesizes') +}) + +test('#373 finding 4: probe queries BOTH signals and runs at sonnet/low — reliable worktree+gh, still low effort', async () => { + // The probe orchestrates a worktree + a `gh` fetch + a substring match, and a mis-report + // fails OPEN toward a duplicate first review (the very noise this story removes), so it runs + // at sonnet (not the cheapest haiku) while staying at low effort. This pins the model choice + // so a later refactor can't silently drop it back to a tier that mis-runs the tool steps. const dispatch = (prompt, opts) => { if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } if (opts.label?.startsWith('probe:')) return { logExists: false, firstReviewPosted: false } @@ -430,13 +463,13 @@ test('#373 probe queries BOTH signals and is dialed to the cheap model/effort po const { calls } = await runWorkflow({ args: { stories: [RESUME_STORY] }, dispatch }) const probe = calls.find(c => c.opts.label?.startsWith('probe:')) assert.ok(probe, 'probe runs on resume') - assert.equal(probe.opts.model, 'haiku', 'probe uses the cheap model') + assert.equal(probe.opts.model, 'sonnet', 'probe runs at sonnet (reliable worktree+gh substring match, fails open toward duplicate first review)') assert.equal(probe.opts.effort, 'low', 'probe uses low effort') assert.ok(probe.prompt.includes('logExists') && probe.prompt.includes('firstReviewPosted'), 'probe reports both the log-existence and the PR-side first-review signal') }) test('#373 finding 1: the first review emits a hidden marker and the probe matches it DETERMINISTICALLY (no semantic template-structure judgment)', async () => { - // The probe runs at haiku/low. It must NOT classify a comment by reading its structure + // The probe runs at sonnet/low. It must NOT classify a comment by reading its structure // (a false positive would silence a legitimate first review — the story's High-impact // over-silencing risk). Instead the first review emits a fixed hidden marker and the probe // does a plain EXACT substring match on that same marker. From 5bd7835591f32202e99d1ca44fd849c1c764ba45 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 24 Jul 2026 17:04:18 +0200 Subject: [PATCH 8/9] [#373] fix: drop always-true guard on needsHumanDecision flush (review round 8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The if(cycleHasRemediation) guard on the design-disagreement escalate-flush was always true — the branch is only reachable after the fix-round line that sets cycleHasRemediation=true. Dropped the guard (post unconditionally) and documented why no no-log arm exists here, unlike the load-bearing cycleHasRemediation||!first guard on the MAX_FIX_ROUNDS escalation. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/workflows/implement-batch.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/.claude/workflows/implement-batch.js b/.claude/workflows/implement-batch.js index 2375f5cc..bc0a06ad 100644 --- a/.claude/workflows/implement-batch.js +++ b/.claude/workflows/implement-batch.js @@ -430,11 +430,15 @@ async function driveStory(story) { // its path in the return so the human / next resume can find (and clean) it. if (!fix) return { story, prNumber: pr.prNumber, status: 'failed-fix', reviewLog: cycleHasRemediation ? reviewLog : undefined } if (fix.needsHumanDecision) { - if (cycleHasRemediation) - await agent( - `Story ${tag}: escalating a design disagreement to a human. ${wtClause(story)} Read \`${reviewLog}\`. ${flushConvention(story, pr.prNumber)} THEN post ONE fresh comment on PR #${pr.prNumber} (response to the first review) summarizing the remediation rounds so far, the still-open findings (${JSON.stringify(prevFindings)}) and the open decision. Do NOT delete the log — it is the continuation anchor for this cycle. Do NOT merge.`, - { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, - ) + // No guard here: reaching this line means the fix round above already ran, which set + // `cycleHasRemediation = true` AND had the fixer append this round to the working log. + // So the log always exists and the flush always fires — there is no no-log arm (unlike + // the MAX_FIX_ROUNDS escalation at the top of the loop, whose `cycleHasRemediation || !first` + // guard IS load-bearing because that path can be reached on a silent round-0 re-review). + await agent( + `Story ${tag}: escalating a design disagreement to a human. ${wtClause(story)} Read \`${reviewLog}\`. ${flushConvention(story, pr.prNumber)} THEN post ONE fresh comment on PR #${pr.prNumber} (response to the first review) summarizing the remediation rounds so far, the still-open findings (${JSON.stringify(prevFindings)}) and the open decision. Do NOT delete the log — it is the continuation anchor for this cycle. Do NOT merge.`, + { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, + ) return { story, prNumber: pr.prNumber, status: 'escalate', findings: prevFindings, acceptedFindings: accepted } } } From 94a5d5ad4c83c3c601cbb072ed7bf9b0219e898b Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Fri, 24 Jul 2026 17:48:15 +0200 Subject: [PATCH 9/9] [#373] fix: no-log escalate arm minimizes prior comments; distinct-fixture flush test; sonnet/low comment - split flushConvention -> flushMinimize (Part A, PR-comment, always emitted) + flushLogConvention (Part B, log-backed arms) - no-log escalate arm now emits flushMinimize (supersedes stale flush / prior synthesis; was omitted) - flush test: distinct ids/PRs per path, assert own interpolation (not byte-equal tautology) - fix stale :343 comment haiku/low -> sonnet/low (matches round-7 probe model) Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/workflows/implement-batch.js | 21 ++++++++++++-- .claude/workflows/implement-batch.test.mjs | 32 +++++++++++++++------- 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/.claude/workflows/implement-batch.js b/.claude/workflows/implement-batch.js index bc0a06ad..f495f4c5 100644 --- a/.claude/workflows/implement-batch.js +++ b/.claude/workflows/implement-batch.js @@ -255,8 +255,23 @@ function revWtClause(story) { // escalation prompts (MAX_FIX_ROUNDS + needsHumanDecision). Authored ONCE here so a future // change to the convention or the worktree-persistence wording is made in one place and can't // silently diverge between the two paths (they had already drifted slightly before this). +// Part A — PR-comment minimize/supersede. Operates ONLY on already-posted PR comments, so it +// does NOT depend on a working log and MUST be emitted on EVERY escalation (both arms), else a +// stale prior flush or a prior convergence's "ready for merge" synthesis is left visible next to +// an active escalation (finding: the no-log arm previously omitted this). +function flushMinimize(prNumber) { + return `FIRST minimize / mark-outdated any prior escalate-flush comment already posted on PR #${prNumber} — each flush "summarizes the rounds so far", so a new one SUPERSEDES the last; only the newest escalate-flush should stay visible (no-op if there is none). ALSO minimize / mark-outdated any prior final-remediation/synthesis comment left by an EARLIER convergence of this SAME cycle (a converged-but-unmerged PR that was re-run, found new findings and is now escalating): its "review clean / ready for merge" verdict directly contradicts an active escalation, so it must NOT stay visible alongside this flush — mirror the convergence-synthesis path (no-op if there is none), but NEVER minimize the first-review comment.` +} + +// Part B — the log/out-of-band CONVENTION + untracked-worktree-persistence note. Only meaningful +// when a working log exists (a continuing cycle), so it is emitted only on the log-backed arms. +function flushLogConvention(story) { + return `CONVENTION (state it in the comment so the human/orchestrator knows): any further rework or re-review — including manual out-of-band rounds — should be funneled into THIS same working log (append), NOT posted as standalone PR comments; the next orchestrated run on this story continues the same cycle and its convergence will synthesize ONE final remediation and minimize these intermediate comments. Note too (in the comment) that this working log is an UNTRACKED file living ONLY in the persistent authoring worktree \`../pair-worktrees/${story.id}\`, so that worktree must be PRESERVED until merge — if it is pruned/recreated the audit log is lost (this flush + the first-review comment still remain on the PR, and the PR-side first-review signal still prevents a duplicate first review on the next run).` +} + +// Full convention = minimize (Part A) + log/out-of-band note (Part B), for the log-backed arms. function flushConvention(story, prNumber) { - return `FIRST minimize / mark-outdated any prior escalate-flush comment already posted on PR #${prNumber} — each flush "summarizes the rounds so far", so a new one SUPERSEDES the last; only the newest escalate-flush should stay visible (no-op if there is none). ALSO minimize / mark-outdated any prior final-remediation/synthesis comment left by an EARLIER convergence of this SAME cycle (a converged-but-unmerged PR that was re-run, found new findings and is now escalating): its "review clean / ready for merge" verdict directly contradicts an active escalation, so it must NOT stay visible alongside this flush — mirror the convergence-synthesis path (no-op if there is none), but NEVER minimize the first-review comment. CONVENTION (state it in the comment so the human/orchestrator knows): any further rework or re-review — including manual out-of-band rounds — should be funneled into THIS same working log (append), NOT posted as standalone PR comments; the next orchestrated run on this story continues the same cycle and its convergence will synthesize ONE final remediation and minimize these intermediate comments. Note too (in the comment) that this working log is an UNTRACKED file living ONLY in the persistent authoring worktree \`../pair-worktrees/${story.id}\`, so that worktree must be PRESERVED until merge — if it is pruned/recreated the audit log is lost (this flush + the first-review comment still remain on the PR, and the PR-side first-review signal still prevents a duplicate first review on the next run).` + return `${flushMinimize(prNumber)} ${flushLogConvention(story)}` } // ── Per-story lifecycle ────────────────────────────────────────────────── @@ -340,7 +355,7 @@ async function driveStory(story) { // #373: the first-review comment always emits this hidden HTML-comment marker verbatim // (invisible in rendered markdown → no visible noise). The continuation probe detects a // prior first review by an EXACT substring match on this marker, NOT by a semantic reading - // of the comment's structure — so the cheap haiku/low probe makes no classification + // of the comment's structure — so the cheap sonnet/low probe makes no classification // judgment and can't false-positive a non-review comment into silencing a real first // review (the story's High-impact over-silencing risk). Minimized/outdated comments still // match: gh returns their raw body, which still contains the marker. @@ -408,7 +423,7 @@ async function driveStory(story) { if (cycleHasRemediation || !first) { const logClause = cycleHasRemediation ? `Read the review log \`${reviewLog}\`. ${flushConvention(story, pr.prNumber)} THEN ` - : `No prior review working log exists (a re-review on a resumed PR whose log was never written or was pruned) — escalate from the inline findings directly. ` + : `No prior review working log exists (a re-review on a resumed PR whose log was never written or was pruned) — escalate from the inline findings directly. ${flushMinimize(pr.prNumber)} ` await agent( `Story ${tag}: the review<->fix loop is escalating to a human (non-convergence or a design disagreement). ${wtClause(story)} ${logClause}post ONE fresh comment on PR #${pr.prNumber} — written as a response to the first code-review comment — summarizing${cycleHasRemediation ? ' the rounds so far (per finding: what was attempted + current state) and' : ''} the still-open actionable findings: ${JSON.stringify(actionable)}.${cycleHasRemediation ? ' Do NOT delete the log — it is the continuation anchor for this cycle.' : ''} Do NOT merge.`, { agentType: 'implementer', phase: 'Review', label: `flush:${tag}`, model: 'sonnet', effort: 'medium' }, diff --git a/.claude/workflows/implement-batch.test.mjs b/.claude/workflows/implement-batch.test.mjs index d0f93eb2..f298e246 100644 --- a/.claude/workflows/implement-batch.test.mjs +++ b/.claude/workflows/implement-batch.test.mjs @@ -492,11 +492,14 @@ test('#373 finding 1: the first review emits a hidden marker and the probe match assert.ok(!/Overall Assessment|Review Summary/.test(probe.prompt), 'probe no longer relies on a semantic template-structure reading of the comment') }) -test('#373 finding 3: both escalate-flush prompts share ONE authored convention block (supersede + out-of-band + untracked-worktree), so they cannot diverge', async () => { - // MAX_FIX_ROUNDS escalation (fresh-story path, cycleHasRemediation set by a prior fix round). +test('#373 finding 3: both escalate-flush prompts carry the shared convention block, each interpolated from its OWN story/PR (single source, parameterized — not a byte-equal tautology)', async () => { const finding = { location: 'x.ts:1', severity: 'Minor', description: 'never fixed', recommendation: 'r' } + + // MAX_FIX_ROUNDS escalation (fresh-story path, cycleHasRemediation set by a prior fix round). + // Distinct id (292) + PR (#7 from the PR phase) from the resume path below. + const STORY_A = { id: '292', title: 'T', branch: 'feat/#292-x' } const maxRoundsFlush = (await runWorkflow({ - args: { stories: [STORY] }, + args: { stories: [STORY_A] }, dispatch: (prompt, opts) => { if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } if (opts.agentType === 'reviewer') return { verdict: 'Rework', findings: [finding] } @@ -508,8 +511,10 @@ test('#373 finding 3: both escalate-flush prompts share ONE authored convention })).calls.find(c => c.opts.label?.startsWith('flush:')) // needsHumanDecision escalation (fixer escalates a design disagreement on a continuation). + // DISTINCT id (555) + PR (#88 via resume) so an interpolation regression cannot be masked. + const STORY_B = { id: '555', title: 'T', branch: 'feat/#555-y', prNumber: 88 } const designFlush = (await runWorkflow({ - args: { stories: [RESUME_STORY] }, + args: { stories: [STORY_B] }, dispatch: (prompt, opts) => { if (opts.agentType === 'contract-generator') return { status: 'cache-hit', contract: validContract() } if (opts.label?.startsWith('probe:')) return { logExists: true, firstReviewPosted: true } @@ -520,12 +525,19 @@ test('#373 finding 3: both escalate-flush prompts share ONE authored convention })).calls.find(c => c.opts.label?.startsWith('flush:')) assert.ok(maxRoundsFlush && designFlush, 'both escalation paths post a flush') - // The extracted convention block is byte-identical in both prompts (single source of truth). - const block = /SUPERSEDES the last[\s\S]*prevents a duplicate first review on the next run\)/ - const a = maxRoundsFlush.prompt.match(block) - const b = designFlush.prompt.match(block) - assert.ok(a && b, 'both flush prompts carry the shared convention block') - assert.equal(a[0], b[0], 'the convention block is identical in both flush prompts (no drift)') + + // Shared single-source marker present in BOTH (Part A supersede clause). + assert.match(maxRoundsFlush.prompt, /SUPERSEDES the last/, 'maxRounds flush carries the shared minimize/supersede block') + assert.match(designFlush.prompt, /SUPERSEDES the last/, 'design-disagreement flush carries the shared minimize/supersede block') + + // Each flush is interpolated from its OWN story/PR — proving parameterization, not a tautology. + assert.match(maxRoundsFlush.prompt, /\.\.\/pair-worktrees\/292\b/, 'maxRounds flush interpolates its own worktree (292)') + assert.match(maxRoundsFlush.prompt, /PR #7\b/, 'maxRounds flush interpolates its own PR (#7)') + assert.doesNotMatch(maxRoundsFlush.prompt, /pair-worktrees\/555|PR #88\b/, 'maxRounds flush does NOT leak the other story/PR') + + assert.match(designFlush.prompt, /\.\.\/pair-worktrees\/555\b/, 'design flush interpolates its own worktree (555)') + assert.match(designFlush.prompt, /PR #88\b/, 'design flush interpolates its own PR (#88)') + assert.doesNotMatch(designFlush.prompt, /pair-worktrees\/292|PR #7\b/, 'design flush does NOT leak the other story/PR') }) test('#373 escalate ON A CONTINUATION: resume + existing log + never-converging re-review keeps the log, flushes (cycleHasRemediation seeded true), supersedes prior flush, no synth (AC5 on the resume path)', async () => {