[US-373] chore: whole-cycle review noise-reduction (escalate/resume/manual path)

[rucka]

PR Information

PR Title: [US-373] chore: whole-cycle review noise-reduction (escalate/resume/manual path)
Story/Epic: #373 (extends #367 / #314 bundle — workflow-tooling / review noise-reduction)
Type: Chore (tech-debt, tooling)
Priority: P2 (Could-Have)
Labels: tech-debt

Summary

What Changed

Extends the #367 "one review + one final remediation" noise policy from a single run to the whole PR review cycle in implement-batch.js — across escalations, resume runs, and manual out-of-band rounds:

• Continuation detection (two signals): on a resume run, a cheap sandbox-safe agent probe (PROBE_SCHEMA, haiku/low effort) returns { logExists, firstReviewPosted }. logExists = .pair/working/reviews/<id>.md is present (an in-flight cycle to CONTINUE). firstReviewPosted = PR-side corroboration that a first review already went out, matched DETERMINISTICALLY: the first review emits a fixed hidden HTML-comment marker (<!-- pair:first-review #<id> PR#<n> -->) and the probe does an EXACT substring match on it — no semantic template-structure judgment — so the cheap probe can't misclassify a non-review comment into silencing a real first review (round-4 finding 1). A failed/malformed probe fails OPEN (both signals false → post a first review), documented at the fallback.
• Silent round-0 on continuation: const first = round === 0 && !isContinuation && !firstReviewPosted — a resumed cycle's round-0 review is a silent re-review appended to the same log, never a second posted "first review", even if the untracked log was lost (the PR-side signal still suppresses a duplicate).
• Seeded remediated: let remediated = isContinuation — so an immediate round-0 convergence on a continuation still triggers the final synthesis + log cleanup (never leaves an escalate-flush comment as the last visible word).
• Whole-cycle synthesis: the convergence-synthesis prompt now reads across ALL runs recorded in the log and additionally minimizes/marks-outdated any prior escalate-flush or manual out-of-band comments, so only the first review + the one final remediation remain visible.
• Escalate = continuation anchor (regression guard, unchanged behavior confirmed by test): the log is kept, never deleted, on escalation — it's the anchor the next resume continues from.
• Documented manual out-of-band convention: the PR-COMMENT POLICY block + escalate-flush prompts now instruct a human/orchestrator taking over rework after an escalate to append notes into the same working log rather than posting standalone PR comments, so the next orchestrated run's synthesis covers them too.
• Fresh-story path is unchanged: no probe runs, first review still posts normally (AC6 regression case).

Why This Change

A PR that takes several runs/escalations to converge previously showed a pile of per-round comments (escalate-flush, manual intermediate notes) instead of a clean first-review + one-final-remediation shape, defeating the point of #367's noise reduction whenever a cycle didn't converge in a single run.

Story Context

User Story: As a maintainer driving Pair stories through implement-batch, I want the "one review + one final remediation" noise policy to hold across the WHOLE review↔remediation cycle of a PR — through escalations, resume runs, and manual out-of-band rounds — so that a PR that took several runs/escalations to converge still shows exactly one review + one final remediation, not a per-round pile of comments.

Acceptance Criteria: AC1–AC6 in issue #373 (continuation → silent round-0; convergence → single whole-cycle synthesis + log delete; prior flush/manual comments minimized; documented manual out-of-band convention; escalate keeps log — regression guard; fresh-story path unchanged — regression guard). All six covered by dedicated test cases (see Testing below).

Changes Made

Implementation Details

• Continuation probe: PROBE_SCHEMA + sandbox-safe existence-check agent call, gated on resuming
• Silent round-0 gate: first = round === 0 && !isContinuation
• Seeded remediated: isContinuation seeds it so immediate convergence on resume still synthesizes
• Whole-cycle synthesis + comment minimize: synthesis prompt updated to read all runs from the log and minimize prior flush/manual comments before deleting the log
• Docs: PR-COMMENT POLICY comment block + escalate-flush prompts document the manual out-of-band convention
• Escalate-flush supersede: both flush prompts (MAX_FIX_ROUNDS + needsHumanDecision) minimize any prior escalate-flush before posting a fresh one, so repeated escalations leave only the newest flush visible
• flushConvention(story, prNumber) helper: the shared supersede + out-of-band-convention + untracked-worktree-persistence block is authored ONCE and interpolated into both escalation prompts, so it can't silently diverge (round-4 finding 3)

Files Changed

• Modified: .claude/workflows/implement-batch.js, .claude/workflows/implement-batch.test.mjs
• Added: none
• Deleted: none
• Renamed: none

Testing

Test Coverage

• Unit Tests: 10 new #373 cases added to implement-batch.test.mjs (continuation silent round-0 + immediate-convergence edge; whole-cycle synthesis mapping + minimize + delete; resume-with-no-log fresh-review edge; fresh-story-unchanged regression; escalate documents the manual out-of-band convention; probe queries both signals at haiku/low; resume-no-log-but-first-review-on-PR silent; escalate-on-a-continuation supersede; deterministic first-review-marker match; both-flushes-share-one-convention-block) plus a worktree-persistence assertion added to the non-convergence escalate test — 24/24 green in the file, including the pre-existing [#314] feat: wire template-override resolution into skills #367 cases (no regressions)

Test Results

Test Suite: implement-batch.test.mjs — 24 passed, 0 failed
node --test .claude/workflows/implement-batch.test.mjs

Testing Strategy

• Happy Path: fresh story end-to-end — unchanged (AC6)
• Edge Cases: resume with existing log (AC1/AC2), resume with NO log (fresh-review edge), immediate convergence on continuation round-0 (never leaves escalate-flush as last word)
• Error Handling: non-convergence keeps the log, no synthesis (AC5, regression guard on [#314] feat: wire template-override resolution into skills #367 escalate behavior)

Reviewer Guide

Review Focus Areas

1. Continuation-gate correctness: first = round === 0 && !isContinuation must never suppress a legitimate first review on a genuinely fresh cycle (see the resume-with-no-log test).
2. Whole-cycle synthesis prompt: verify it instructs mapping findings from all runs in the log (not just the latest) and minimizing prior intermediate comments before deleting the log.
3. Escalate path regression: confirm the log is still kept (never deleted) on non-convergence/design-disagreement escalation.

Testing the Changes

node --test .claude/workflows/implement-batch.test.mjs

Key Test Scenarios

1. Resume with existing log → round-0 silent, single synthesis maps all runs + minimizes + deletes log
2. Resume with NO log → round-0 posts as a fresh first review
3. Fresh story (no resume) → probe never runs, behavior identical to [#314] feat: wire template-override resolution into skills #367

Dependencies & Related Work

Related PRs

• Follows: [#314] feat: wire template-override resolution into skills #367 (in-loop noise-reduction) — MERGED. This PR builds directly on its reviewLog / synthesis machinery (~lines 278-332 pre-change).

Follow-up Work

• None — explicitly out of scope: standalone reviewer/fix agent defaults, pair-process-review, templates, pair-next, pair-cli are NOT touched; the manual out-of-band convention is documented (code comments) only, not enforced by editing those files.

Scope Note

Change confined to .claude/workflows/implement-batch.js + .claude/workflows/implement-batch.test.mjs only, per story mutex note (must not be batched with any other story editing this file).

[rucka]

Code Review — PR #374

Review Information

PR Number: #374
Author: rucka (+ Claude Opus 4.8)
Reviewer: Independent reviewer (automated, /pair-process-review)
Review Date: 2026-07-24
Story/Epic: #373 (extends #367 / #314 bundle — review noise-reduction)
Review Type: Chore / refactor (tooling, tech-debt, P2)
Estimated Review Time: ~30 min

Review Summary

Overall Assessment

• Approved with Comments — Minor / clarification issues noted, can merge

Key Changes Summary

Extends the #367 "one review + one final remediation" comment-noise policy from a single run to the whole PR review cycle in .claude/workflows/implement-batch.js: a sandbox-safe existence-probe of the persisted working log on resume runs (isContinuation), a silent round-0 on continuation (first = round === 0 && !isContinuation), seeded remediated = isContinuation so immediate round-0 convergence still synthesizes, a whole-cycle synthesis prompt that maps findings across ALL runs and minimizes prior intermediate comments, and documentation of a manual out-of-band convention in the escalate-flush prompts + PR-COMMENT POLICY block. Confined to implement-batch.js + its .test.mjs (152/-15, 2 files).

Business Value Validation

Delivers the stated value: a PR that converges over several runs/escalations ends in exactly one first-review + one final remediation. All six AC are satisfied for the story's stated scope.

Code Review Checklist — verified

• Requirements Met — AC1–AC6 all traced to code and covered by tests (see below).
• Business Logic — Continuation gate, remediated seeding, and synthesis prompt are internally consistent. first correctly false only on a continuation; escalate paths (both MAX_FIX_ROUNDS and needsHumanDecision) both keep the log and carry the convention text.
• Error Handling — Probe failure/absence degrades toward posting a review (fail-open, not silencing) — the safer default.
• Consistency — probe/fix/flush/synth all use the authoring worktree (wtClause), only the reviewer uses the detached read-only worktree (revWtClause); the log is written and read in the same worktree. Correct.
• Style — one-line opts style preserved; .claude/workflows/ is outside the prettier gate (per DoD). No new files.

AC traceability

AC	Where	Verified
AC1 continuation → silent round-0	first = round===0 && !isContinuation (L324); re-review prompt "do NOT post any PR comment"	✅ test L301
AC2 convergence → one synthesis maps all runs + delete	remediated=isContinuation (L320); synth prompt "ALL runs … DELETE" (L371)	✅ test L328
AC3 prior flush/manual comments minimized	synth prompt "minimize / mark-outdated … only first review + this one remediation"	✅ test L328
AC4 documented manual out-of-band convention	PR-COMMENT POLICY block (L295-300) + both flush prompts	✅ test L374
AC5 escalate keeps log, no synthesis	escalate returns before synth, "Do NOT delete the log"	✅ pre-existing test L275
AC6 fresh path unchanged	resuming gate skips probe; first=true	✅ test L366

Security Review

No secrets, auth, PII, or external-input handling introduced. The orchestrator runs sandboxed (no FS/gh); all file/comment work is delegated. story.id/tag flow only into natural-language prompt text, not executed shell. Security: green — 0 findings.

Testing Review

18/18 pass in implement-batch.test.mjs (40/40 across the full workflow test glob — reconciles the commit's "40/40" with the PR body's "18/18 in the file"). 5 new #373 cases cover continuation-silent + immediate-convergence, whole-cycle synthesis mapping/minimize/delete, resume-with-no-log fresh-review edge, fresh-story-no-probe regression, and escalate-documents-convention. Assertions are specific (prompt-content + step-label based), mirroring the #367 harness. Two coverage gaps noted below (Minor).

Detailed Review Comments

Positive Feedback

• Clean, minimal control-flow delta: a single flag (isContinuation), a one-token change to first, and a seeded remediated express the whole behavior. Well-commented with tech-debt: extend review noise-reduction to the escalate / manual-rework path (one final remediation for the whole cycle) #373 rationale inline.
• remediated = isContinuation seeding is a neat way to guarantee immediate-convergence-on-resume still synthesizes and never leaves an escalate-flush as the last visible word.
• Fail-open probe (absent/failed → treat as fresh, post a review) is the correct safety bias.
• Both escalate paths (round-limit and design-disagreement) were updated symmetrically.

Questions / Minor Issues

Questions ❓

• implement-batch.js:307-324 — Re-run on a converged-but-unmerged PR re-posts a second first-review. At convergence the log is DELETED (SSOT gone). If the batch is re-run on a story that is already ready-for-merge but not yet merged, the probe returns exists:false → isContinuation=false → first=true → round-0 posts a second first-review comment — the exact pile-up the story fights. This is outside the story's stated scope (which targets escalated/interrupted cycles), is consistent with the documented "log existence == in-flight" SSOT rule, is pre-existing [#314] feat: wire template-override resolution into skills #367 behavior for the converged case (not a regression), and re-running a ready PR before merge is not the intended flow. Impact is bounded but real. Recommendation: consider a guard (detect an existing first-review comment / converged marker on the PR before posting a fresh first review), or track as a follow-up story.
• implement-batch.js:337-339 & 358-360 — Repeated escalations before convergence accumulate multiple escalate-flush comments. Each escalation posts a flush but does not minimize the prior flush; minimization happens only at final convergence. A cycle that escalates N times shows N flush comments during the non-converged window. The converged end-state is clean (AC-compliant), and an escalated PR is intentionally showing state to the human, but since each flush "summarizes the rounds so far" it supersedes the previous one. Recommendation: on escalation, also minimize the prior escalate-flush comment (supersede-in-place), or document that intermediate escalate comments are intentionally cumulative.

Minor Issues 💡

• implement-batch.js:303-313 — Continuation SSOT is an untracked file in the persistent authoring worktree. If the worktree is pruned/recreated from origin before merge, or a human works out-of-band in a different clone (which the flush comment explicitly invites), .pair/working/reviews/<id>.md is absent → probe returns exists:false → a second first-review is posted. Largely by-design (the design assumes the worktree persists until merge and treats no-log as a fresh review per the AC edge case), but the fragility — untracked SSOT + silent fall-through to a double first-review — is worth surfacing. Recommendation: note the persistence assumption in the flush comment, or corroborate continuation with a PR-side signal.
• implement-batch.test.mjs — Two small coverage gaps. (a) Escalate-on-a-continuation (a resumed cycle whose round-0 re-review fails to converge → keeps log + flushes) is not directly tested; AC5 is only exercised on the fresh path. (b) The probe's cheap-model/effort policy (model:'haiku', effort:'low'), a stated risk-mitigation, is not pinned by a test the way other per-step efforts are. Recommendation: add a continuation-escalate case and assert the probe opts.

Adoption Compliance

Level noted: change is dev-tooling only (no product bounded-context). No new dependencies, no new architectural decision → no ADR required. Matches the story classification (risk:yellow, cost:green).

Tech Debt

This PR is tracked tech-debt (P2, tech-debt label) and never blocks. No new debt introduced beyond the residual edge cases surfaced above.

Risk Assessment

Risk	Impact	Probability	Mitigation
Over-silencing a legitimate first review	High	Low	isContinuation gate + fresh-path regression test (AC6) + resume-no-log edge test; fail-open probe
Residual double-first-review on re-run of a converged/lost-log PR	Med	Low	Surfaced as Question above; bounded to a non-intended re-run flow

Overall Decision

Approved with Comments. All six acceptance criteria are met for the story's stated scope, tests are green, security is clean, and the change is minimal and well-documented. The findings are non-blocking Questions/Minor edge-case and coverage notes; the two Questions are candidates for a small follow-up rather than in-PR blockers on a P2 tech-debt chore. Merge is the human gate — this review does not merge.

[rucka]

Reply to the first review comment above.

Escalating to human — non-convergence after 4 review rounds

Summary of the review<->fix cycle so far (full detail in .pair/working/reviews/373.md, kept — not deleted — for this escalation):

Round 2 — 4 findings (2 Questions, 2 Minor):

• Re-run on a converged-but-unmerged PR re-posts a second "first review" → resolved via a PR-side firstReviewPosted corroboration signal alongside log-existence.
• Repeated escalations accumulate multiple escalate-flush comments → resolved: both flush prompts now minimize/mark-outdated any prior escalate-flush before posting a fresh one (supersede-in-place).
• Continuation SSOT is an untracked file; a lost/pruned worktree could silently re-post a first review → resolved by the same PR-side corroboration signal.
• Two test-coverage gaps → resolved: 3 new tests added.
  All confirmed still resolved in round 3 (no regression).

Round 3 — completed the "OR document" half of the untracked-SSOT finding: both escalate-flush prompts now explicitly state the log is untracked and must be preserved until merge, pinned with a new assertion.

Round 4 — independent re-review raised 4 more findings (3 Minor, 1 Questions), all resolved:

• Probe's job had grown to semantic classification while staying at haiku/low → made deterministic via a fixed hidden marker (<!-- pair:first-review #<id> PR#<n> -->) matched by exact substring instead of judgment.
• PR body Testing section was stale → updated.
• Two escalate-flush prompts duplicated a ~200-word block (drift risk, and they'd already drifted) → extracted into a shared flushConvention() helper used by both.
• Fail-open direction on a failed/malformed probe wasn't documented → confirmed intended (degrade toward visibility) and documented at the fallback.

Still-open actionable findings from the latest independent review, not yet addressed:

1. [Minor] .claude/workflows/implement-batch.js:424 (synth prompt) vs the business rule at :294-295 — the convergence-synthesis "minimize" step enumerates only prior escalate-flush and manual out-of-band rework/re-review comments to minimize, but omits a prior final-remediation (synth) comment. Concrete failure case: a PR converges (synth Setup & Project Management Integration #1, log deleted, ready-for-merge), gets re-driven with its prNumber (an explicitly supported resume flow), regresses, re-converges (synth Collaborative Knowledge Base #2) — synth Collaborative Knowledge Base #2's enumeration doesn't include synth Setup & Project Management Integration #1, so the PR can end up with TWO visible final-remediation comments, contradicting "AT MOST one final remediation comment." Bounded/cosmetic (requires the atypical re-drive-an-already-converged-unmerged-PR flow), and the prompt's own GOAL clause would likely still steer a competent agent to minimize the stale synth — an enumeration-clarity gap, not a hard logic bug. Recommendation: add "and any prior final-remediation comment from an earlier convergence of this cycle" to the synth prompt's explicit minimize list.

2. [Minor] .claude/workflows/implement-batch.test.mjs:351-385 (tech-debt: extend review noise-reduction to the escalate / manual-rework path (one final remediation for the whole cycle) #373 finding 3 test) — the test guarding the shared flushConvention block ("so they cannot diverge") drives both escalation paths with fixtures that have identical id ('292') AND identical prNumber (7), so flushConvention produces a byte-identical string for both by construction — assert.equal(a[0], b[0]) is tautologically satisfied and would still pass even if the refactor were reverted to two duplicated-but-textually-identical literals. It doesn't actually prove the single-source/no-divergence property claimed. Recommendation: give the two fixtures distinct ids/prNumbers and assert each flush prompt contains its own correctly-interpolated block, proving parameterization + presence in both paths rather than byte-equality across identical inputs.

Escalating per the review<->fix loop's non-convergence path — human decision needed on whether/how to address these two before merge. Not merging; log preserved at .pair/working/reviews/373.md.

[rucka]

Code Review — PR #374

Review Information

• PR: [US-373] chore: whole-cycle review noise-reduction (escalate/resume/manual path) #374 — [US-373] chore: whole-cycle review noise-reduction (escalate/resume/manual path)
• Story: tech-debt: extend review noise-reduction to the escalate / manual-rework path (one final remediation for the whole cycle) #373 (tech-debt, risk:yellow, P2)
• Reviewer: Independent review (/pair-process-review)
• Review Date: 2026-07-24
• Review Type: Chore / tooling (control-flow + prompts in .claude/workflows/implement-batch.js)

Review Summary

Overall Assessment

• Approved with Comments — minor issues noted, can merge

Key Changes

Extends #367's "one review + one final remediation" noise policy from a single run to the whole PR cycle (resume runs, escalations, manual out-of-band rounds), all inside implement-batch.js (+ tests):

• A sandbox-safe continuation probe (haiku/low) on resume runs reporting two signals — logExists (in-flight working log present) and firstReviewPosted (PR-side corroboration via an exact hidden-marker substring match, not a semantic read).
• Silent round-0 on continuation (first = round === 0 && !isContinuation && !firstReviewPosted).
• Seeded remediated = isContinuation so an immediate round-0 convergence on a continuation still synthesizes + cleans.
• Whole-cycle synthesis mapping all runs in the log + minimizing prior intermediate comments before deleting the log.
• Escalate-flush supersede + a shared flushConvention() authored once (DRY, drift-proof), documenting the manual out-of-band convention and untracked-worktree persistence.

Business Value

Delivers the intended noise reduction: a PR that takes several runs/escalations to converge shows one first review + one final remediation instead of a per-round pile.

Acceptance Criteria Verification

AC	Status	Evidence
AC1 — continuation → silent round-0, appends same log	Met	first gate (js:377) + re-review "do NOT post any PR comment" (js:379); test #373 continuation (resume + existing log)
AC2 — convergence → ONE whole-cycle synthesis + log delete	Met (see Minor 1 for an edge)	remediated=isContinuation (js:372), synth maps ALL runs + DELETE (js:424); test #373 continuation convergence
AC3 — prior flush/manual comments minimized	Met	synth minimize (js:424) + flushConvention supersede (js:259); tests cover both
AC4 — documented manual out-of-band convention in PR-COMMENT POLICY block	Met	policy block js:322-327 + flushConvention js:259; test #373 escalate documents the manual out-of-band convention
AC5 — non-convergence keeps log, no synthesis	Met	escalate "Do NOT delete the log", no synth; tests non-convergence + #373 escalate ON A CONTINUATION
AC6 — fresh-story path unchanged	Met	no probe when !resuming; test #373 fresh story + retained #367 test

The High-impact "over-silencing a legitimate first review" risk is well-mitigated: round-0 is silenced only on logExists OR a deterministic marker match; the resume-with-no-log and fresh-story paths post normally; the probe fails open toward visibility. Solid.

Testing Review

• node --test .claude/workflows/implement-batch.test.mjs → 23 passed, 0 failed (10 new tech-debt: extend review noise-reduction to the escalate / manual-rework path (one final remediation for the whole cycle) #373 cases + a worktree-persistence assertion on the escalate test; pre-existing [#314] feat: wire template-override resolution into skills #367 cases retained — no regressions).
• node --check on the JS clean.
• Scope confirmed: only implement-batch.js + implement-batch.test.mjs changed (matches the story mutex note).

Detailed Review Comments

Positive

• The deterministic hidden-marker match (replacing a semantic template read) genuinely removes the haiku misclassification failure mode — the right fix for the over-silencing risk.
• Extracting flushConvention() so both escalation prompts share one byte-identical block (with a test pinning identity) is good drift-proofing.
• Fail-open probe direction is deliberate and documented; worktree separation (authoring worktree for the log-probe/synth, detached review worktree for the blind reviewer) is correct and consistent.

Minor Issues

• [implement-batch.js:424] — synthesis minimize does not cover a prior remediation/synthesis comment. The convergence-synthesis prompt minimizes "prior intermediate PR comments — earlier escalate-flush comments and any manual out-of-band rework/re-review comments." On a converged-but-unmerged PR that is re-run and finds new actionable findings then re-converges (exactly the re-entry path the new firstReviewPosted signal was added for), a second remediation comment is posted while the first remediation comment (R1, from the earlier convergence, not an escalate-flush nor a manual comment) is not in the enumerated minimize set — so two remediation comments can remain visible, undercutting AC2 / the "at most one final remediation" business-rule invariant in that edge. Impact is cosmetic (extra comment; human still sees the latest) and probability is low, but it is a real, in-scope gap. Recommendation: broaden the minimize clause to also cover any prior remediation/synthesis comment authored by the review cycle (e.g. "…and any earlier final-remediation comment from a prior convergence"), so the invariant holds on re-entry. One-line prompt tweak — fixable in this PR.

• [implement-batch.js:379] — round-0 re-review on a continuation is passed an empty prevFindings. On a genuine continuation the round-0 re-review prompt says "Verify these prior findings were genuinely resolved: []" — the orchestrator is (correctly) blind to the working log and the reviewer is told not to read .pair/working/, so cross-run findings can't be threaded. The re-review therefore runs as a fresh independent review that simply doesn't post. No finding is missed (the reviewer re-derives at xhigh effort), so this is correct by design (reviewer independence requires staying blind to the log). Reported for transparency only — the empty-list phrasing on continuation round-0 is slightly misleading to a future reader.

• [implement-batch.js:372] — remediated is semantically overloaded. It now means "this cycle has/continues remediation state" rather than "a remediation happened this run" (seeded = isContinuation). The adjacent comment documents the intent, so this is a low-priority readability nit; a name like cycleHasRemediation would read more truthfully. Optional.

Risk Assessment

Confined control-flow + prompt change in the delivery orchestrator; no production runtime, no data, no secrets/auth. Worst case is comment noise or a re-run — consistent with the story's risk:yellow. Human merge gate unchanged; workflow never merges.

Verdict

Approved with Comments. All six acceptance criteria are met, the over-silencing risk is well-guarded, and the test suite is green with no regressions. The findings are all Minor: one real (but low-probability, cosmetic) gap against the "one final remediation" invariant on the converged-unmerged-re-entry path (worth the one-line prompt fix here), plus two transparency/readability notes. None block merge (and per R7.2 a tech-debt story never blocks). Merge is the human gate.

[rucka]

Code Review — PR #374 (US-373)

Review Information

PR Number: #374
Story/Epic: #373 (tech-debt — review noise-reduction, extends #367 / #314 bundle)
Review Type: Chore / tooling (control-flow + prompt edits)
Reviewer: Independent reviewer (pair-process-review)
Review Date: 2026-07-24

Review Summary

Overall Assessment

• Approved with Comments — Minor issues noted, can merge

Key Changes Summary

Extends #367's "one review + one final remediation" noise policy from a single run to the WHOLE PR cycle in implement-batch.js, across escalate / resume / manual out-of-band rounds:

• A sandbox-safe continuation probe (PROBE_SCHEMA, haiku/low) returns two signals — logExists (in-flight cycle to continue) and firstReviewPosted (PR-side corroboration via a deterministic hidden-marker substring match).
• Silent round-0 gate first = round === 0 && !isContinuation && !firstReviewPosted.
• cycleHasRemediation (renamed from remediated) is seeded with isContinuation, so an immediate round-0 convergence on a resume still synthesizes + cleans.
• Whole-cycle synthesis reads ALL runs from the log and minimizes prior flush / manual / prior-convergence comments (never the first review).
• flushConvention() extracts the shared escalate-flush block (supersede + out-of-band convention + untracked-worktree note) so the two escalation prompts can't diverge.

Business Value Validation

The change delivers the stated value: a PR that took several runs/escalations to converge now shows exactly one first-review + one final remediation. All six acceptance criteria are met by the diff (not just claimed) and backed by dedicated tests.

Functionality Review — AC verification

AC	Requirement	Verdict	Evidence
AC1	Resume w/ log → silent round-0, appends to same log	Met	first gate (L383) + re-review prompt (L385); test "continuation … round-0 SILENT"
AC2	Convergence → ONE synthesis mapping ALL runs + delete log	Met	synth prompt (L430) "map EVERY finding across ALL runs"; test AC2+AC3
AC3	Prior flush/manual comments minimized	Met	synth + flushConvention minimize set; tests assert minimize/outdated
AC4	Documented manual out-of-band convention	Met	PR-COMMENT POLICY block (L327-332) + flushConvention (L258-260); test AC4
AC5	Escalate keeps log, no synth	Met	escalate returns before synth; "Do NOT delete the log"; tests assert no synth
AC6	Fresh story unchanged	Met	probe gated on resuming; test "no probe on fresh story"

Tests: 23/23 pass locally (node --test .claude/workflows/implement-batch.test.mjs), including the pre-existing #367 cases — no regressions. The deterministic hidden-marker match is a strong mitigation of the story's headline "High-impact over-silencing" risk.

Code Quality Assessment

• Naming: remediated → cycleHasRemediation accurately reflects cross-run semantics; no stale remediated references remain (grep-clean).
• Comments: thorough, and correctly document the fail-open direction (finding 4), the marker-survives-minimization behavior, and the seed rationale.
• DRY: flushConvention removes the prior drift between the two escalation prompts — good.
• Scope: confined to implement-batch.js + .test.mjs; no new files (matches DoD + mutex note).
• Style: one-line opts style preserved (.claude/workflows/ is outside the prettier gate).

Detailed Review Comments

Positive Feedback

• The two-signal design (log + PR-side marker) correctly handles the lost/pruned-log and converged-but-unmerged-re-run cases, and the deterministic substring marker removes the misclassification failure mode a semantic probe would have had.
• cycleHasRemediation seeding is carefully reasoned: seeded on isContinuation (log present) but NOT on firstReviewPosted-only, precisely to avoid synthesizing a deleted log.
• Test coverage is genuinely adversarial (immediate-convergence edge, no-log edge, both-flush-blocks-identical byte assertion, deterministic-marker assertion).

Minor Issues 💡

• [.claude/workflows/implement-batch.js:393-399] — Escalate-visibility gap on the resume-no-log path. When the probe returns logExists=false, firstReviewPosted=true (converged-but-unmerged re-run, or a pruned/lost log) and the round-0 re-review returns needsHumanDecision, cycleHasRemediation is false (not seeded, since isContinuation=false) so the if (cycleHasRemediation) guard skips the flush. Round-0 was also silent (first=false). Result: the escalation posts no PR-visible artifact — the new blocking concern is surfaced only in the batch return value, not on the PR, and a subsequent resume would repeat the silent escalation. Impact: narrow edge (requires a design-disagreement on the first silent re-review of a re-run), and the log is genuinely absent so a "read the log" flush would degrade anyway. Recommendation: allow the escalate flush to post from the inline ${actionable} findings even when there is no log (guard the "read the log" step as best-effort), so a resume-path escalation stays visible on the PR. Non-blocking.

• [.claude/workflows/implement-batch.js:385] — On a continuation round-0, prevFindings is [], so the RE-REVIEW prompt reads "Verify these prior findings were genuinely resolved: []" — a vacuous instruction. Cosmetic only (the agent still performs a fresh independent pass). Recommendation: when prevFindings is empty, phrase round-0 as a fresh re-review rather than "verify []".

• [PR [US-373] chore: whole-cycle review noise-reduction (escalate/resume/manual path) #374 metadata] — The PR carries no GitHub labels, though story tech-debt: extend review noise-reduction to the escalate / manual-rework path (one final remediation for the whole cycle) #373 has tech-debt + risk:yellow and the PR body text claims Labels: tech-debt. Classification tags were not applied to the PR. Recommendation: apply tech-debt and risk:yellow to PR [US-373] chore: whole-cycle review noise-reduction (escalate/resume/manual path) #374 (publish-pr is meant to copy the story's classification tags). Non-blocking.

Questions ❓

• [.claude/workflows/implement-batch.js:352-354] — The continuation probe runs at haiku/low but is instructed to create/reuse a git worktree AND run gh to fetch and substring-match PR comments. Is haiku reliable for the worktree+gh steps here? A silently mis-reporting probe fails open (re-posts a duplicate first review) — documented as low-probability and self-announcing, so acceptable, but worth confirming the model choice holds in practice. Non-blocking.

Testing Review

• Unit coverage adequate for a prompt/control-flow module; edge and regression cases present. Tests assert on prompt content + step labels via the dispatch harness (the established [#314] feat: wire template-override resolution into skills #367 pattern). All 23 green.
• Note (non-blocking): the two Minor code findings above are not covered by a test (no test exercises logExists=false, firstReviewPosted=true + round-0 needsHumanDecision).

Risk Assessment

Risk	Impact	Probability	Mitigation
Over-silencing a legitimate first review	High	Low	Deterministic marker + fail-open + fresh-cycle regression test (AC6)
Resume-path round-0 escalation invisible on PR	Low	Low	Surfaced in batch return; Minor finding above

Overall

Solid, well-tested, well-documented change that meets every acceptance criterion and preserves the #367 behavior. Findings are Minor / Questions and non-blocking.

Verdict: Approved with Comments.

[rucka]

Reply to the first review comment above.

Escalating to human — non-convergence after 8 review rounds

Summary of the review<->fix cycle so far (full detail in .pair/working/reviews/373.md, kept — not deleted — for this escalation).

Round 2 — 4 findings (2 Questions, 2 Minor), all resolved: converged-but-unmerged re-run duplicate first review (fixed via PR-side firstReviewPosted corroboration); repeated escalations piling up multiple flush comments (fixed: supersede-in-place); untracked-log SSOT loss risk (same corroboration signal); 2 test-coverage gaps closed.

Round 3 — completed the "OR document" half of the untracked-SSOT finding (flush prompts now state the log must be preserved until merge).

Round 4 — 4 more findings (3 Minor, 1 Questions), all resolved: probe's growing semantic judgment made deterministic via a fixed hidden marker + exact substring match; stale PR body Testing section updated; duplicated ~200-word flush block extracted into shared flushConvention(); fail-open direction on a malformed probe confirmed/documented.

Round 5 — 2 findings (Minor), both resolved: synthesis-minimize set missed a prior convergence's own remediation comment (broadened minimize clause); remediated flag renamed to cycleHasRemediation for clarity.

Round 6 — 1 finding (Minor), resolved: the escalate path (unlike the synth path) didn't supersede a stale prior-convergence remediation — fixed by extending the shared flushConvention with the same minimize instruction.

Round 7 — 4 findings (3 Minor, 1 Questions), all resolved: escalate-visibility gap on the resume-no-log path (broadened guard to cycleHasRemediation || !first, best-effort log read); vacuous "verify these prior findings: []" on continuation round-0 (three-way prompt: first/re-review/fresh); PR #374 missing GitHub labels (applied tech-debt + risk:yellow); probe model bumped haiku→sonnet for reliability.

Round 8 — 1 finding (Minor), resolved: dead-always-true if (cycleHasRemediation) guard on the needsHumanDecision escalate-flush dropped, with a comment contrasting it against the genuinely load-bearing MAX_FIX_ROUNDS guard.

Still-open actionable findings (this round)

1. [Minor] .claude/workflows/implement-batch.js:411-413 — The no-log escalate arm posts a fresh flush but issues NO comment minimize/supersede instruction (unlike the cycleHasRemediation arm at :410, which interpolates flushConvention). Two concrete reachable failure cases: (a) a converged-but-unmerged PR re-driven with its prNumber — convergence deleted the log, probe returns logExists=false, firstReviewPosted=true; if round-0 escalates via needsHumanDecision, this arm is taken and the prior convergence's "review clean / ready for merge" synthesis comment stays visible next to the new escalation; (b) a pruned/recreated worktree resume where a prior escalate-flush exists on the PR but the log is gone and round-0 escalates — the prior flush is not superseded, two flushes pile up. Both violate the code's own asserted invariant ("only the newest escalate-flush should stay visible" / "at most one first-review + one final remediation visible"). Impact bounded/cosmetic, low-probability, but real and in-scope for tech-debt: extend review noise-reduction to the escalate / manual-rework path (one final remediation for the whole cycle) #373. Not tested (the no-log-arm test asserts the flush posts/carries findings but never asserts any minimize). Recommendation: split flushConvention into a PR-comment-minimize part (always emitted on any escalation) plus a log/convention part (only when a log exists), so both escalate arms uphold the invariant.

2. [Minor] .claude/workflows/implement-batch.js:343 — Stale comment: still reads "...so the cheap haiku/low probe makes no classification judgment" even though the probe was bumped to sonnet in round 7 (line 358 model: 'sonnet', and the round-7 rationale comment at lines 309-313 says "runs at sonnet/low"). Two comments in the same file now disagree on the probe's model. Recommendation: change "haiku/low" to "sonnet/low" at line 343.

3. [Minor] .claude/workflows/implement-batch.test.mjs:495-528 (fixtures at :76 and :306) — The test asserting "both escalate-flush prompts share ONE authored convention block ... so they cannot diverge" doesn't actually prove parameterization/single-source: both fixtures (STORY id 292, RESUME_STORY id 292/prNumber 7) share the same id/prNumber, so flushConvention produces a byte-identical block by construction — the assertion would still pass even if the shared helper were reverted to two textually-identical inline literals. This is the round-4 finding-2 escalation, not addressed in code (fixtures unchanged). Recommendation: give the two fixtures distinct ids/prNumbers and assert each flush prompt contains its own correctly-interpolated ../pair-worktrees/<id> (and PR#) block.

4. [Minor] PR [US-373] chore: whole-cycle review noise-reduction (escalate/resume/manual path) #374 description — Testing section — States "23 passed, 0 failed" / "23/23 green" / "10 new tech-debt: extend review noise-reduction to the escalate / manual-rework path (one final remediation for the whole cycle) #373 cases" but the suite now has 24 tests (verified 24/24 locally at PR head). A prior round already flagged and "fixed" this same section, and it has drifted stale again by one test. Recommendation: update to 24/24 (and the new-case count if changed).

None of these 4 have been addressed in this fix round — escalating per the non-convergence policy. Working log preserved (not deleted) at .pair/working/reviews/373.md for the human to act on. PR not merged.