From e2198dbe7d5a564fd93a85d7ff411558dda0ca93 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Fri, 3 Jul 2026 07:07:04 -0700 Subject: [PATCH 1/7] fix: extend TUI idle-reap grace for do:release/do:pr/do:rpr review loops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A slow multi-reviewer pass (e.g. codex reading a large diff) can go silent in the wrapped TUI for well over the 3-minute default idle window, and the runner reaped the still-waiting release agent as a false idle-complete success before it ever reached the merge gate — observed on agent-61508f36, PR #2084, which sat open+unmerged for hours. Adds createReviewLoopTracker (server/lib/tuiHandshake.js), mirroring the existing merge-queue idle-grace mechanism (#2074): latches on multi-reviewer-loop chrome ("Review plan:", "Review pass", "Multi-Reviewer", "review loop"), extends the idle reaper's grace to 15 minutes while active, and surfaces a still-elapsed timeout as a needs-manual-finish review-loop-idle-timeout failure instead of a silent completed status. --- .changelog/NEXT.md | 5 ++ server/lib/tuiHandshake.js | 64 ++++++++++++++++++++++ server/lib/tuiHandshake.test.js | 46 ++++++++++++++++ server/services/agentTuiSpawning.js | 37 ++++++++++++- server/services/agentTuiSpawning.test.js | 70 +++++++++++++++++++++++- 5 files changed, 219 insertions(+), 3 deletions(-) create mode 100644 .changelog/NEXT.md diff --git a/.changelog/NEXT.md b/.changelog/NEXT.md new file mode 100644 index 000000000..fcf0abe55 --- /dev/null +++ b/.changelog/NEXT.md @@ -0,0 +1,5 @@ +# Unreleased Changes + +## Fixed + +- **CoS TUI agents running `/do:release`/`/do:pr`/`/do:rpr` no longer get prematurely reaped as "done" while a reviewer is still working** — the idle-timeout reaper only special-cased the `/do:next --swarm` merge-queue wait (#2074); a slow multi-reviewer pass (e.g. codex reading a large diff) could go silent past the default 3-minute idle window and get finalized as a false `idle-complete` success before the release ever reached the merge gate, leaving the PR open with the task dashboard showing "completed" (observed on agent-61508f36, PR #2084, which sat open+unmerged for hours). Added a `createReviewLoopTracker` (`server/lib/tuiHandshake.js`) that latches on multi-reviewer-loop chrome ("Review plan:", "Review pass", "Multi-Reviewer", "review loop") and extends the idle reaper's grace to 15 minutes while active — mirroring the merge-queue fix exactly, including surfacing a timeout as a needs-manual-finish failure (`reason: 'review-loop-idle-timeout'`) instead of a silent success. diff --git a/server/lib/tuiHandshake.js b/server/lib/tuiHandshake.js index dd3007f4c..2d316b07c 100644 --- a/server/lib/tuiHandshake.js +++ b/server/lib/tuiHandshake.js @@ -378,6 +378,70 @@ export function createMergeQueueTracker() { }; } +// Extended idle threshold applied while a `/do:release`, `/do:pr`, or `/do:rpr` +// multi-reviewer loop is waiting on a slow external reviewer (a Copilot cloud +// review, a headless codex/agy/claude review pass, an Ollama pass, or an +// arbitrary @ human reviewer). Observed 2026-07-02 (agent-61508f36): the +// review loop correctly backgrounds the reviewer and polls for it rather than +// blocking — but the reviewer itself can go silent in the wrapped TUI for well +// over the 3-minute default while it works (e.g. codex reading a large diff), +// and the runner reaped the still-waiting release agent as `idle-complete` (a +// false SUCCESS) before it ever reached the merge gate, leaving the release PR +// open and unmerged. Mirrors the merge-queue grace (#2074) exactly: 15 minutes +// comfortably covers one reviewer's silent working stretch while still +// bounding a genuinely-dead agent's reap. +export const REVIEW_LOOP_IDLE_TIMEOUT_MS = 900000; + +// Distinctive markers the multi-reviewer loop (do:release/do:pr/do:rpr) prints +// once it starts waiting on a reviewer pass. Detection is deliberately +// conservative, same rationale as MERGE_QUEUE_MARKERS above: a false POSITIVE +// only extends the (bounded) idle window, and a false NEGATIVE just preserves +// prior behavior. Matched against ANSI-stripped output, lower-cased. +const REVIEW_LOOP_MARKERS = [ + 'review plan:', + 'review pass', + 'multi-reviewer', + 'review loop', +]; + +/** + * True when a chunk of ANSI-stripped TUI output shows the multi-reviewer loop + * (do:release/do:pr/do:rpr) has started a reviewer pass. Callers MUST pass + * stripped output. Non-string / empty input yields false. + * + * @param {string} strippedText — ANSI-stripped output (a chunk or accumulator). + * @returns {boolean} + */ +export function isReviewLoopSignal(strippedText) { + if (typeof strippedText !== 'string' || !strippedText) return false; + const lower = strippedText.toLowerCase(); + return REVIEW_LOOP_MARKERS.some((marker) => lower.includes(marker)); +} + +/** + * Latching tracker for "this agent is waiting inside a multi-reviewer loop" + * (do:release/do:pr/do:rpr). Feed it each ANSI-stripped post-submit chunk via + * `observe(text)`; it becomes `active` the first time a review-loop marker + * appears and STAYS active thereafter (same latching rationale as + * createMergeQueueTracker — the failure mode is a silent external-reviewer + * wait, so a recency window would age the flag out exactly when the extended + * grace is needed). Once latched, the idle reaper uses + * REVIEW_LOOP_IDLE_TIMEOUT_MS instead of the 3-minute default. + * + * @returns {{ observe: (strippedText: string) => boolean, readonly active: boolean }} + */ +export function createReviewLoopTracker() { + let active = false; + return { + observe(strippedText) { + if (active) return true; + if (isReviewLoopSignal(strippedText)) active = true; + return active; + }, + get active() { return active; }, + }; +} + // ─── Buffer caps (defensive RAM bounds) ─────────────────────────────────── // // RAW caps stay small — the raw PTY stream is only used for paste-marker diff --git a/server/lib/tuiHandshake.test.js b/server/lib/tuiHandshake.test.js index 93ec80ea7..301109489 100644 --- a/server/lib/tuiHandshake.test.js +++ b/server/lib/tuiHandshake.test.js @@ -15,6 +15,9 @@ import { MERGE_QUEUE_IDLE_TIMEOUT_MS, isMergeQueueSignal, createMergeQueueTracker, + REVIEW_LOOP_IDLE_TIMEOUT_MS, + isReviewLoopSignal, + createReviewLoopTracker, rendersWorkCounter, PASTE_TO_ENTER_MIN_DELAY_MS, PASTE_TO_ENTER_FALLBACK_MS, @@ -290,6 +293,49 @@ describe('tuiHandshake — merge-queue idle suppression (#2074)', () => { }); }); +// Observed 2026-07-02 (agent-61508f36) — a do:release run's multi-reviewer +// loop correctly backgrounded a slow codex review and polled for it rather +// than blocking, but the reviewer's silent working stretch exceeded the +// 3-minute default and the runner reaped the still-waiting release as a false +// `idle-complete` success before it reached the merge gate, leaving PR #2084 +// open. Mirrors the merge-queue suppression above. +describe('tuiHandshake — review-loop idle suppression', () => { + it('extends the idle timeout well past the default 3-minute window', () => { + expect(REVIEW_LOOP_IDLE_TIMEOUT_MS).toBe(900000); + expect(REVIEW_LOOP_IDLE_TIMEOUT_MS).toBeGreaterThan(DEFAULT_TUI_IDLE_TIMEOUT_MS); + }); + + it('isReviewLoopSignal matches multi-reviewer-loop chrome (case-insensitive)', () => { + expect(isReviewLoopSignal('Review plan: [claude, codex] (mode: series, stop-mode: all)')).toBe(true); + expect(isReviewLoopSignal('--- Review pass 1/2: codex ---')).toBe(true); + expect(isReviewLoopSignal('## Multi-Reviewer Summary')).toBe(true); + expect(isReviewLoopSignal('You are a COPILOT REVIEW LOOP agent.')).toBe(true); + }); + + it('isReviewLoopSignal ignores ordinary implementation/output chrome', () => { + expect(isReviewLoopSignal('Editing server/services/agentTuiSpawning.js')).toBe(false); + expect(isReviewLoopSignal('● high · (12s · running tests)')).toBe(false); + expect(isReviewLoopSignal('')).toBe(false); + expect(isReviewLoopSignal(null)).toBe(false); + expect(isReviewLoopSignal(undefined)).toBe(false); + }); + + it('createReviewLoopTracker latches on first signal and stays active through silence', () => { + const tracker = createReviewLoopTracker(); + expect(tracker.active).toBe(false); + tracker.observe('implementing the fix, running the suite'); + expect(tracker.active).toBe(false); + // Enters the multi-reviewer loop — latches. + expect(tracker.observe('Review plan: [claude, codex] (mode: series, stop-mode: all)')).toBe(true); + expect(tracker.active).toBe(true); + // Subsequent quiet reviewer-wait chunks (no marker) must NOT un-latch it — + // the whole point is that the silent gap is when the grace is needed. + tracker.observe('waiting for codex...'); + tracker.observe(''); + expect(tracker.active).toBe(true); + }); +}); + describe('tuiHandshake.inferTuiCommand', () => { // Catch-all default also returns claude; the claude rows just confirm // an explicit match isn't accidentally tagged codex/antigravity/gemini. diff --git a/server/services/agentTuiSpawning.js b/server/services/agentTuiSpawning.js index 8adef0816..17736d3ed 100644 --- a/server/services/agentTuiSpawning.js +++ b/server/services/agentTuiSpawning.js @@ -26,12 +26,14 @@ import { DEFAULT_TUI_PROMPT_DELAY_MS, DEFAULT_TUI_IDLE_TIMEOUT_MS, MERGE_QUEUE_IDLE_TIMEOUT_MS, + REVIEW_LOOP_IDLE_TIMEOUT_MS, READY_POLL_INTERVAL_MS, READY_IDLE_THRESHOLD_MS, PASTE_MARKER_POLL_MS, countPasteMarkers, createWorkActivityTracker, createMergeQueueTracker, + createReviewLoopTracker, createInputReadyTracker, rendersWorkCounter, PASTE_TO_ENTER_MIN_DELAY_MS, @@ -283,6 +285,14 @@ export async function spawnTuiAgent({ // latched, the idle reaper uses the extended MERGE_QUEUE_IDLE_TIMEOUT_MS so a // still-working orchestrator isn't reaped mid-merge (issue #2074). const mergeQueue = createMergeQueueTracker(); + // Latches once the agent enters a do:release/do:pr/do:rpr multi-reviewer + // loop, whose external reviewer passes (codex reading a large diff, a + // Copilot cloud review, a human @ review) can go silent in the TUI + // for well over the default idle window. While latched, the idle reaper + // uses the extended REVIEW_LOOP_IDLE_TIMEOUT_MS so a still-waiting release + // isn't reaped as a false `idle-complete` success before it reaches the + // merge gate (issue observed on agent-61508f36, PR #2084). + const reviewLoop = createReviewLoopTracker(); // Tracks claude's interactive input-readiness (footer chrome) and its first-run // folder-trust gate. Gates the prompt paste for the claude TUI so we never // paste into a startup banner, a trust menu, or a returned shell prompt. @@ -639,6 +649,13 @@ export async function spawnTuiAgent({ emitLog('info', `TUI agent ${agentId} entered merge queue — idle reaper extended to ${Math.round(MERGE_QUEUE_IDLE_TIMEOUT_MS / 60000)}min`, { agentId, phase: 'merge-queue' }); await updateAgent(agentId, { metadata: { phase: 'merge-queue' } }); } + // Detect entry into a do:release/do:pr/do:rpr multi-reviewer loop so the + // idle reaper can extend its grace across a slow reviewer's silent + // working stretch (see reviewLoop declaration above for the incident). + if (promptSubmittedAt && stripped && !reviewLoop.active && reviewLoop.observe(stripped)) { + emitLog('info', `TUI agent ${agentId} entered review loop — idle reaper extended to ${Math.round(REVIEW_LOOP_IDLE_TIMEOUT_MS / 60000)}min`, { agentId, phase: 'review-loop' }); + await updateAgent(agentId, { metadata: { phase: 'review-loop' } }); + } lastOutputAt = now; if (firstOutputAt === null) firstOutputAt = lastOutputAt; @@ -901,7 +918,9 @@ export async function spawnTuiAgent({ // extended window still bounds a genuinely-dead orchestrator's reap. const effectiveIdleTimeoutMs = mergeQueue.active ? Math.max(tuiConfig.idleTimeoutMs, MERGE_QUEUE_IDLE_TIMEOUT_MS) - : tuiConfig.idleTimeoutMs; + : reviewLoop.active + ? Math.max(tuiConfig.idleTimeoutMs, REVIEW_LOOP_IDLE_TIMEOUT_MS) + : tuiConfig.idleTimeoutMs; if (idle >= effectiveIdleTimeoutMs) { // Reaped AFTER the extended merge-queue grace elapsed: the orchestrator // almost certainly died mid-merge with PRs opened/merged-but-uncleaned. @@ -918,6 +937,22 @@ export async function spawnTuiAgent({ }); return; } + // Reaped AFTER the extended review-loop grace elapsed: a reviewer + // (copilot/codex/agy/claude/ollama/@) likely hung, or the wait + // simply exceeded budget. Surface it as a needs-manual-finish FAILURE + // rather than the silent `status: completed` that let PR #2084 sit + // open+unmerged for hours while agent-61508f36 looked "done". + if (reviewLoop.active) { + finish({ + success: false, + exitCode: 1, + error: `TUI agent idled out after ${Math.round(effectiveIdleTimeoutMs / 60000)}min waiting inside the multi-reviewer loop — a reviewer may have hung or the wait exceeded budget; check the PR's review/merge state and finish manually.`, + reason: 'review-loop-idle-timeout', + }).catch(err => { + emitLog('error', `Failed to finalize TUI agent ${agentId}: ${err.message}`, { agentId }); + }); + return; + } // Distinguish a real (sentinel-less) completion from a never-submitted // prompt that just idled out. `lastOutputAt > promptSentAt` only proves // the TUI repainted SOMETHING — banner/status chrome churns even with the diff --git a/server/services/agentTuiSpawning.test.js b/server/services/agentTuiSpawning.test.js index 0e022144d..9ce854cf7 100644 --- a/server/services/agentTuiSpawning.test.js +++ b/server/services/agentTuiSpawning.test.js @@ -1010,16 +1010,22 @@ describe('spawnTuiAgent runtime', () => { // tested without standing up the full fake-timer PTY harness. describe('agentTuiSpawning — idle reap decision (#2074)', () => { const MERGE_QUEUE_IDLE_TIMEOUT_MS = 900000; + const REVIEW_LOOP_IDLE_TIMEOUT_MS = 900000; // Faithful copy of the idleTimer body's finalize-selection logic. - function decideIdleReap({ idle, baseIdleTimeoutMs, mergeQueueActive, workActive, rendersCounter }) { + function decideIdleReap({ idle, baseIdleTimeoutMs, mergeQueueActive, reviewLoopActive, workActive, rendersCounter }) { const effectiveIdleTimeoutMs = mergeQueueActive ? Math.max(baseIdleTimeoutMs, MERGE_QUEUE_IDLE_TIMEOUT_MS) - : baseIdleTimeoutMs; + : reviewLoopActive + ? Math.max(baseIdleTimeoutMs, REVIEW_LOOP_IDLE_TIMEOUT_MS) + : baseIdleTimeoutMs; if (idle < effectiveIdleTimeoutMs) return { action: 'wait', effectiveIdleTimeoutMs }; if (mergeQueueActive) { return { action: 'reap', success: false, reason: 'merge-queue-idle-timeout', effectiveIdleTimeoutMs }; } + if (reviewLoopActive) { + return { action: 'reap', success: false, reason: 'review-loop-idle-timeout', effectiveIdleTimeoutMs }; + } const noWorkButCounterExpected = !workActive && rendersCounter; if (noWorkButCounterExpected) { return { action: 'reap', success: false, reason: 'idle-no-activity', effectiveIdleTimeoutMs }; @@ -1063,3 +1069,63 @@ describe('agentTuiSpawning — idle reap decision (#2074)', () => { expect(r.reason).toBe('merge-queue-idle-timeout'); }); }); + +// Generalizes #2074's fix to do:release/do:pr/do:rpr's multi-reviewer loop — +// observed 2026-07-02 on agent-61508f36 (PR #2084): a slow codex review pass +// went silent past the 3-minute default and the still-waiting release agent +// was reaped as a false `idle-complete` success before it ever merged. +describe('agentTuiSpawning — idle reap decision (review loop)', () => { + const REVIEW_LOOP_IDLE_TIMEOUT_MS = 900000; + + function decideIdleReap({ idle, baseIdleTimeoutMs, mergeQueueActive, reviewLoopActive, workActive, rendersCounter }) { + const effectiveIdleTimeoutMs = mergeQueueActive + ? Math.max(baseIdleTimeoutMs, 900000) + : reviewLoopActive + ? Math.max(baseIdleTimeoutMs, REVIEW_LOOP_IDLE_TIMEOUT_MS) + : baseIdleTimeoutMs; + if (idle < effectiveIdleTimeoutMs) return { action: 'wait', effectiveIdleTimeoutMs }; + if (mergeQueueActive) { + return { action: 'reap', success: false, reason: 'merge-queue-idle-timeout', effectiveIdleTimeoutMs }; + } + if (reviewLoopActive) { + return { action: 'reap', success: false, reason: 'review-loop-idle-timeout', effectiveIdleTimeoutMs }; + } + const noWorkButCounterExpected = !workActive && rendersCounter; + if (noWorkButCounterExpected) { + return { action: 'reap', success: false, reason: 'idle-no-activity', effectiveIdleTimeoutMs }; + } + return { action: 'reap', success: true, reason: 'idle-complete', effectiveIdleTimeoutMs }; + } + + const BASE = 180000; + + it('does NOT reap at the 3-min default while in a review loop — grace extends to 15min', () => { + const r = decideIdleReap({ idle: BASE + 5000, baseIdleTimeoutMs: BASE, reviewLoopActive: true, workActive: true, rendersCounter: true }); + expect(r.action).toBe('wait'); + expect(r.effectiveIdleTimeoutMs).toBe(REVIEW_LOOP_IDLE_TIMEOUT_MS); + }); + + it('reaps a review-loop agent as needs-manual-finish once the EXTENDED window blows', () => { + const r = decideIdleReap({ idle: REVIEW_LOOP_IDLE_TIMEOUT_MS + 1, baseIdleTimeoutMs: BASE, reviewLoopActive: true, workActive: true, rendersCounter: true }); + expect(r.action).toBe('reap'); + expect(r.success).toBe(false); + expect(r.reason).toBe('review-loop-idle-timeout'); + }); + + it('leaves the pre-existing idle-complete path untouched when NOT in a review loop', () => { + const r = decideIdleReap({ idle: BASE + 1, baseIdleTimeoutMs: BASE, reviewLoopActive: false, workActive: true, rendersCounter: true }); + expect(r.action).toBe('reap'); + expect(r.success).toBe(true); + expect(r.reason).toBe('idle-complete'); + }); + + it('a review-loop reap takes precedence over the no-activity downgrade', () => { + const r = decideIdleReap({ idle: REVIEW_LOOP_IDLE_TIMEOUT_MS + 1, baseIdleTimeoutMs: BASE, reviewLoopActive: true, workActive: false, rendersCounter: true }); + expect(r.reason).toBe('review-loop-idle-timeout'); + }); + + it('a merge-queue reap takes precedence over a review-loop reap when both are (implausibly) active', () => { + const r = decideIdleReap({ idle: 900001, baseIdleTimeoutMs: BASE, mergeQueueActive: true, reviewLoopActive: true, workActive: true, rendersCounter: true }); + expect(r.reason).toBe('merge-queue-idle-timeout'); + }); +}); From 786559f705280df9aeb2dbbfdeefcd6490ad567b Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Fri, 3 Jul 2026 07:08:50 -0700 Subject: [PATCH 2/7] test: dedupe review-loop idle-reap decision tests into one describe block Local review gate caught a duplicated inline copy of decideIdleReap() across two describe blocks; consolidated into the existing #2074 block. --- server/services/agentTuiSpawning.test.js | 44 ++++-------------------- 1 file changed, 6 insertions(+), 38 deletions(-) diff --git a/server/services/agentTuiSpawning.test.js b/server/services/agentTuiSpawning.test.js index 9ce854cf7..1f6b23d72 100644 --- a/server/services/agentTuiSpawning.test.js +++ b/server/services/agentTuiSpawning.test.js @@ -1007,7 +1007,8 @@ describe('spawnTuiAgent runtime', () => { // a needs-manual-finish failure instead of a silent `status: completed`. This is // the exact decision the `idleTimer` interval makes; mirror it as a pure function // (the inline-copy pattern from subAgentSpawner.test.js) so the branch matrix is -// tested without standing up the full fake-timer PTY harness. +// tested without standing up the full fake-timer PTY harness. Generalized to +// do:release/do:pr/do:rpr's multi-reviewer loop below (agent-61508f36, PR #2084). describe('agentTuiSpawning — idle reap decision (#2074)', () => { const MERGE_QUEUE_IDLE_TIMEOUT_MS = 900000; const REVIEW_LOOP_IDLE_TIMEOUT_MS = 900000; @@ -1068,37 +1069,11 @@ describe('agentTuiSpawning — idle reap decision (#2074)', () => { const r = decideIdleReap({ idle: MERGE_QUEUE_IDLE_TIMEOUT_MS + 1, baseIdleTimeoutMs: BASE, mergeQueueActive: true, workActive: false, rendersCounter: true }); expect(r.reason).toBe('merge-queue-idle-timeout'); }); -}); - -// Generalizes #2074's fix to do:release/do:pr/do:rpr's multi-reviewer loop — -// observed 2026-07-02 on agent-61508f36 (PR #2084): a slow codex review pass -// went silent past the 3-minute default and the still-waiting release agent -// was reaped as a false `idle-complete` success before it ever merged. -describe('agentTuiSpawning — idle reap decision (review loop)', () => { - const REVIEW_LOOP_IDLE_TIMEOUT_MS = 900000; - - function decideIdleReap({ idle, baseIdleTimeoutMs, mergeQueueActive, reviewLoopActive, workActive, rendersCounter }) { - const effectiveIdleTimeoutMs = mergeQueueActive - ? Math.max(baseIdleTimeoutMs, 900000) - : reviewLoopActive - ? Math.max(baseIdleTimeoutMs, REVIEW_LOOP_IDLE_TIMEOUT_MS) - : baseIdleTimeoutMs; - if (idle < effectiveIdleTimeoutMs) return { action: 'wait', effectiveIdleTimeoutMs }; - if (mergeQueueActive) { - return { action: 'reap', success: false, reason: 'merge-queue-idle-timeout', effectiveIdleTimeoutMs }; - } - if (reviewLoopActive) { - return { action: 'reap', success: false, reason: 'review-loop-idle-timeout', effectiveIdleTimeoutMs }; - } - const noWorkButCounterExpected = !workActive && rendersCounter; - if (noWorkButCounterExpected) { - return { action: 'reap', success: false, reason: 'idle-no-activity', effectiveIdleTimeoutMs }; - } - return { action: 'reap', success: true, reason: 'idle-complete', effectiveIdleTimeoutMs }; - } - - const BASE = 180000; + // Generalizes the #2074 fix to do:release/do:pr/do:rpr's multi-reviewer loop — + // observed 2026-07-02 on agent-61508f36 (PR #2084): a slow codex review pass + // went silent past the 3-minute default and the still-waiting release agent + // was reaped as a false `idle-complete` success before it ever merged. it('does NOT reap at the 3-min default while in a review loop — grace extends to 15min', () => { const r = decideIdleReap({ idle: BASE + 5000, baseIdleTimeoutMs: BASE, reviewLoopActive: true, workActive: true, rendersCounter: true }); expect(r.action).toBe('wait'); @@ -1112,13 +1087,6 @@ describe('agentTuiSpawning — idle reap decision (review loop)', () => { expect(r.reason).toBe('review-loop-idle-timeout'); }); - it('leaves the pre-existing idle-complete path untouched when NOT in a review loop', () => { - const r = decideIdleReap({ idle: BASE + 1, baseIdleTimeoutMs: BASE, reviewLoopActive: false, workActive: true, rendersCounter: true }); - expect(r.action).toBe('reap'); - expect(r.success).toBe(true); - expect(r.reason).toBe('idle-complete'); - }); - it('a review-loop reap takes precedence over the no-activity downgrade', () => { const r = decideIdleReap({ idle: REVIEW_LOOP_IDLE_TIMEOUT_MS + 1, baseIdleTimeoutMs: BASE, reviewLoopActive: true, workActive: false, rendersCounter: true }); expect(r.reason).toBe('review-loop-idle-timeout'); From 27826990bb42f8f8042ac4fc1ca70b2d980d119d Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Fri, 3 Jul 2026 07:14:20 -0700 Subject: [PATCH 3/7] address review (claude): tighten review-loop markers to avoid false-positive latch 'review pass' / 'review loop' as bare substrings would latch on ordinary CoS agent narration (this project's own CLAUDE.md "self-review pass" convention, and this repo's bundled slashdo docs saying "review loop"), not just an actual do:release/do:pr/do:rpr multi-reviewer run. Replaced the review-pass marker with a digit/slash-anchored banner regex and dropped the bare 'review loop' substring. --- server/lib/tuiHandshake.js | 19 ++++++++++++++++--- server/lib/tuiHandshake.test.js | 13 ++++++++++++- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/server/lib/tuiHandshake.js b/server/lib/tuiHandshake.js index 2d316b07c..50dbb3c85 100644 --- a/server/lib/tuiHandshake.js +++ b/server/lib/tuiHandshake.js @@ -397,13 +397,26 @@ export const REVIEW_LOOP_IDLE_TIMEOUT_MS = 900000; // conservative, same rationale as MERGE_QUEUE_MARKERS above: a false POSITIVE // only extends the (bounded) idle window, and a false NEGATIVE just preserves // prior behavior. Matched against ANSI-stripped output, lower-cased. +// +// Kept narrower than a bare 'review pass' / 'review loop' substring match: +// this project's own CLAUDE.md convention text is "run a simplify/self-review +// pass before committing", whose substring "review pass" would otherwise +// latch this tracker for ANY CoS agent's ordinary self-review narration, not +// just a do:release/do:pr/do:rpr run — and this very repo's bundled slashdo +// docs contain the literal phrase "review loop" in prose (e.g. this file's +// own doc references), which a docs-editing agent could echo. The banner is +// matched by REVIEW_PASS_BANNER_PATTERN instead, which requires the digit/ +// slash shape ("Review pass 1/2") that only the real banner has. const REVIEW_LOOP_MARKERS = [ 'review plan:', - 'review pass', 'multi-reviewer', - 'review loop', ]; +// Matches the literal `--- Review pass {n}/{N}: {REVIEW_AGENT} ---` banner the +// multi-reviewer wrapper prints at the start of each reviewer pass. Anchored +// on the digit/slash shape so it can't match prose like "self-review pass". +const REVIEW_PASS_BANNER_PATTERN = /review pass\s+\d+\s*\/\s*\d+/i; + /** * True when a chunk of ANSI-stripped TUI output shows the multi-reviewer loop * (do:release/do:pr/do:rpr) has started a reviewer pass. Callers MUST pass @@ -415,7 +428,7 @@ const REVIEW_LOOP_MARKERS = [ export function isReviewLoopSignal(strippedText) { if (typeof strippedText !== 'string' || !strippedText) return false; const lower = strippedText.toLowerCase(); - return REVIEW_LOOP_MARKERS.some((marker) => lower.includes(marker)); + return REVIEW_LOOP_MARKERS.some((marker) => lower.includes(marker)) || REVIEW_PASS_BANNER_PATTERN.test(strippedText); } /** diff --git a/server/lib/tuiHandshake.test.js b/server/lib/tuiHandshake.test.js index 301109489..935ed881b 100644 --- a/server/lib/tuiHandshake.test.js +++ b/server/lib/tuiHandshake.test.js @@ -309,7 +309,6 @@ describe('tuiHandshake — review-loop idle suppression', () => { expect(isReviewLoopSignal('Review plan: [claude, codex] (mode: series, stop-mode: all)')).toBe(true); expect(isReviewLoopSignal('--- Review pass 1/2: codex ---')).toBe(true); expect(isReviewLoopSignal('## Multi-Reviewer Summary')).toBe(true); - expect(isReviewLoopSignal('You are a COPILOT REVIEW LOOP agent.')).toBe(true); }); it('isReviewLoopSignal ignores ordinary implementation/output chrome', () => { @@ -320,6 +319,18 @@ describe('tuiHandshake — review-loop idle suppression', () => { expect(isReviewLoopSignal(undefined)).toBe(false); }); + // Regression for a false-positive latch found in local review: the bare + // substrings 'review pass' / 'review loop' would match this project's own + // CLAUDE.md convention ("run a simplify/self-review pass before committing") + // and this repo's bundled slashdo docs ("Local Agent Code Review Loop"), + // latching the tracker for ANY CoS agent's ordinary narration — not just a + // do:release/do:pr/do:rpr run. + it('isReviewLoopSignal does NOT match ordinary self-review narration or doc prose', () => { + expect(isReviewLoopSignal('running the self-review pass before committing')).toBe(false); + expect(isReviewLoopSignal('## Local Agent Code Review Loop')).toBe(false); + expect(isReviewLoopSignal('You are a Copilot review loop agent.')).toBe(false); + }); + it('createReviewLoopTracker latches on first signal and stays active through silence', () => { const tracker = createReviewLoopTracker(); expect(tracker.active).toBe(false); From 2e52253bca4163b49ffb8adf589082fd2031a8a7 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Fri, 3 Jul 2026 07:20:21 -0700 Subject: [PATCH 4/7] address review (claude): anchor review-loop markers to rendered banner shapes Second-round finding: the bare 'multi-reviewer' substring and unanchored 'review plan:' still false-positived on this repo's own bundled slashdo docs (which describe these banners in prose dozens of times, including the literal instruction text "Review plan: {REVIEW_AGENTS}..."). Anchored all three markers to the actual rendered shape only runtime output has: a bracketed agent list, a digit/slash pass counter, or the literal Multi-Reviewer Summary heading. Also fixed the changelog wording to match. --- .changelog/NEXT.md | 2 +- server/lib/tuiHandshake.js | 38 ++++++++++++++++----------------- server/lib/tuiHandshake.test.js | 16 ++++++++------ 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/.changelog/NEXT.md b/.changelog/NEXT.md index fcf0abe55..673f3cbaf 100644 --- a/.changelog/NEXT.md +++ b/.changelog/NEXT.md @@ -2,4 +2,4 @@ ## Fixed -- **CoS TUI agents running `/do:release`/`/do:pr`/`/do:rpr` no longer get prematurely reaped as "done" while a reviewer is still working** — the idle-timeout reaper only special-cased the `/do:next --swarm` merge-queue wait (#2074); a slow multi-reviewer pass (e.g. codex reading a large diff) could go silent past the default 3-minute idle window and get finalized as a false `idle-complete` success before the release ever reached the merge gate, leaving the PR open with the task dashboard showing "completed" (observed on agent-61508f36, PR #2084, which sat open+unmerged for hours). Added a `createReviewLoopTracker` (`server/lib/tuiHandshake.js`) that latches on multi-reviewer-loop chrome ("Review plan:", "Review pass", "Multi-Reviewer", "review loop") and extends the idle reaper's grace to 15 minutes while active — mirroring the merge-queue fix exactly, including surfacing a timeout as a needs-manual-finish failure (`reason: 'review-loop-idle-timeout'`) instead of a silent success. +- **CoS TUI agents running `/do:release`/`/do:pr`/`/do:rpr` no longer get prematurely reaped as "done" while a reviewer is still working** — the idle-timeout reaper only special-cased the `/do:next --swarm` merge-queue wait (#2074); a slow multi-reviewer pass (e.g. codex reading a large diff) could go silent past the default 3-minute idle window and get finalized as a false `idle-complete` success before the release ever reached the merge gate, leaving the PR open with the task dashboard showing "completed" (observed on agent-61508f36, PR #2084, which sat open+unmerged for hours). Added a `createReviewLoopTracker` (`server/lib/tuiHandshake.js`) that latches on multi-reviewer-loop chrome — the literal `Review plan: [...]` banner, a `Review pass N/M` counter, or the `Multi-Reviewer Summary` report heading — and extends the idle reaper's grace to 15 minutes while active — mirroring the merge-queue fix exactly, including surfacing a timeout as a needs-manual-finish failure (`reason: 'review-loop-idle-timeout'`) instead of a silent success. diff --git a/server/lib/tuiHandshake.js b/server/lib/tuiHandshake.js index 50dbb3c85..137efb8e7 100644 --- a/server/lib/tuiHandshake.js +++ b/server/lib/tuiHandshake.js @@ -396,26 +396,23 @@ export const REVIEW_LOOP_IDLE_TIMEOUT_MS = 900000; // once it starts waiting on a reviewer pass. Detection is deliberately // conservative, same rationale as MERGE_QUEUE_MARKERS above: a false POSITIVE // only extends the (bounded) idle window, and a false NEGATIVE just preserves -// prior behavior. Matched against ANSI-stripped output, lower-cased. +// prior behavior. Matched against ANSI-stripped output. // -// Kept narrower than a bare 'review pass' / 'review loop' substring match: -// this project's own CLAUDE.md convention text is "run a simplify/self-review -// pass before committing", whose substring "review pass" would otherwise -// latch this tracker for ANY CoS agent's ordinary self-review narration, not -// just a do:release/do:pr/do:rpr run — and this very repo's bundled slashdo -// docs contain the literal phrase "review loop" in prose (e.g. this file's -// own doc references), which a docs-editing agent could echo. The banner is -// matched by REVIEW_PASS_BANNER_PATTERN instead, which requires the digit/ -// slash shape ("Review pass 1/2") that only the real banner has. -const REVIEW_LOOP_MARKERS = [ - 'review plan:', - 'multi-reviewer', -]; - -// Matches the literal `--- Review pass {n}/{N}: {REVIEW_AGENT} ---` banner the -// multi-reviewer wrapper prints at the start of each reviewer pass. Anchored -// on the digit/slash shape so it can't match prose like "self-review pass". +// All three patterns are anchored to the literal RENDERED shape rather than a +// bare substring, because this repo bundles the slashdo docs that DESCRIBE +// these banners in prose — `lib/slashdo/lib/multi-reviewer-loop.md` alone +// contains the word "multi-reviewer" dozens of times and the literal phrase +// "Review plan:" once (inside its own instruction text), and this project's +// CLAUDE.md convention text is "run a simplify/self-review pass before +// committing", whose substring "review pass" would otherwise latch on ANY +// CoS agent's ordinary narration or docs-editing — not just an actual +// do:release/do:pr/do:rpr run. Anchoring on the shape only the runtime output +// actually has (a rendered `[...]` agent list, a digit/slash pass counter, or +// the literal report heading) keeps the false-positive rate low without +// weakening true-positive detection of the real banners. +const REVIEW_PLAN_PATTERN = /review plan:\s*\[/i; const REVIEW_PASS_BANNER_PATTERN = /review pass\s+\d+\s*\/\s*\d+/i; +const REVIEW_SUMMARY_PATTERN = /multi-reviewer summary/i; /** * True when a chunk of ANSI-stripped TUI output shows the multi-reviewer loop @@ -427,8 +424,9 @@ const REVIEW_PASS_BANNER_PATTERN = /review pass\s+\d+\s*\/\s*\d+/i; */ export function isReviewLoopSignal(strippedText) { if (typeof strippedText !== 'string' || !strippedText) return false; - const lower = strippedText.toLowerCase(); - return REVIEW_LOOP_MARKERS.some((marker) => lower.includes(marker)) || REVIEW_PASS_BANNER_PATTERN.test(strippedText); + return REVIEW_PLAN_PATTERN.test(strippedText) + || REVIEW_PASS_BANNER_PATTERN.test(strippedText) + || REVIEW_SUMMARY_PATTERN.test(strippedText); } /** diff --git a/server/lib/tuiHandshake.test.js b/server/lib/tuiHandshake.test.js index 935ed881b..3861aef5a 100644 --- a/server/lib/tuiHandshake.test.js +++ b/server/lib/tuiHandshake.test.js @@ -319,16 +319,20 @@ describe('tuiHandshake — review-loop idle suppression', () => { expect(isReviewLoopSignal(undefined)).toBe(false); }); - // Regression for a false-positive latch found in local review: the bare - // substrings 'review pass' / 'review loop' would match this project's own - // CLAUDE.md convention ("run a simplify/self-review pass before committing") - // and this repo's bundled slashdo docs ("Local Agent Code Review Loop"), - // latching the tracker for ANY CoS agent's ordinary narration — not just a - // do:release/do:pr/do:rpr run. + // Regression for false-positive latches found across two rounds of local + // review: bare substrings ('review pass', 'review loop', 'multi-reviewer') + // would match this project's own CLAUDE.md convention ("run a simplify/ + // self-review pass before committing") and this repo's bundled slashdo docs + // (which say "review loop" and "multi-reviewer" dozens of times, including + // the literal instruction text "Review plan: {REVIEW_AGENTS}..."), latching + // the tracker for ANY CoS agent's ordinary narration or docs-editing — not + // just an actual do:release/do:pr/do:rpr run. it('isReviewLoopSignal does NOT match ordinary self-review narration or doc prose', () => { expect(isReviewLoopSignal('running the self-review pass before committing')).toBe(false); expect(isReviewLoopSignal('## Local Agent Code Review Loop')).toBe(false); expect(isReviewLoopSignal('You are a Copilot review loop agent.')).toBe(false); + expect(isReviewLoopSignal('the multi-reviewer wrapper dispatches each listed agent')).toBe(false); + expect(isReviewLoopSignal('Print the resolved plan before starting: `Review plan: {REVIEW_AGENTS} (mode: ...)`')).toBe(false); }); it('createReviewLoopTracker latches on first signal and stays active through silence', () => { From 5abfb2a4a01aaf7be60c90e336c586a49700e647 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Fri, 3 Jul 2026 07:25:42 -0700 Subject: [PATCH 5/7] address review (claude): reword changelog to avoid self-triggering the new detector The changelog entry describing the review-loop markers reproduced the literal trigger strings ("Review plan: [...]", "Multi-Reviewer Summary"), which would false-latch createReviewLoopTracker the next time a release run reads this file. Reworded to describe the banners without reproducing them, matching the care already taken in the source comments. --- .changelog/NEXT.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/NEXT.md b/.changelog/NEXT.md index 673f3cbaf..2fd664200 100644 --- a/.changelog/NEXT.md +++ b/.changelog/NEXT.md @@ -2,4 +2,4 @@ ## Fixed -- **CoS TUI agents running `/do:release`/`/do:pr`/`/do:rpr` no longer get prematurely reaped as "done" while a reviewer is still working** — the idle-timeout reaper only special-cased the `/do:next --swarm` merge-queue wait (#2074); a slow multi-reviewer pass (e.g. codex reading a large diff) could go silent past the default 3-minute idle window and get finalized as a false `idle-complete` success before the release ever reached the merge gate, leaving the PR open with the task dashboard showing "completed" (observed on agent-61508f36, PR #2084, which sat open+unmerged for hours). Added a `createReviewLoopTracker` (`server/lib/tuiHandshake.js`) that latches on multi-reviewer-loop chrome — the literal `Review plan: [...]` banner, a `Review pass N/M` counter, or the `Multi-Reviewer Summary` report heading — and extends the idle reaper's grace to 15 minutes while active — mirroring the merge-queue fix exactly, including surfacing a timeout as a needs-manual-finish failure (`reason: 'review-loop-idle-timeout'`) instead of a silent success. +- **CoS TUI agents running `/do:release`/`/do:pr`/`/do:rpr` no longer get prematurely reaped as "done" while a reviewer is still working** — the idle-timeout reaper only special-cased the `/do:next --swarm` merge-queue wait (#2074); a slow multi-reviewer pass (e.g. codex reading a large diff) could go silent past the default 3-minute idle window and get finalized as a false `idle-complete` success before the release ever reached the merge gate, leaving the PR open with the task dashboard showing "completed" (observed on agent-61508f36, PR #2084, which sat open+unmerged for hours). Added a `createReviewLoopTracker` (`server/lib/tuiHandshake.js`) that latches on multi-reviewer-loop chrome — the review-plan status line (agent list in brackets), a review-pass counter (N of M), or the aggregate report heading — and extends the idle reaper's grace to 15 minutes while active — mirroring the merge-queue fix exactly, including surfacing a timeout as a needs-manual-finish failure (`reason: 'review-loop-idle-timeout'`) instead of a silent success. (Deliberately worded here without reproducing the literal trigger strings — this changelog entry itself would otherwise false-latch the tracker once a future release run reads it.) From dc5671ae1300d76feec35fe3248bb2626e4f0104 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Fri, 3 Jul 2026 07:31:14 -0700 Subject: [PATCH 6/7] address review (codex): drop review-summary marker that collided with its own doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit codex review [P3] found that lib/slashdo/lib/multi-reviewer-loop.md's own aggregate-report example block renders the literal heading "## Multi-Reviewer Summary" as sample output, so REVIEW_SUMMARY_PATTERN would false-latch createReviewLoopTracker whenever an agent reads or quotes that one doc file. Removed the pattern — the review-plan banner alone is a complete signal (it prints once, unconditionally, before any reviewer pass begins, and the tracker latches permanently once set), and both remaining patterns were verified clean against every bundled slashdo doc. --- server/lib/tuiHandshake.js | 36 +++++++++++++++++++-------------- server/lib/tuiHandshake.test.js | 21 ++++++++++--------- 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/server/lib/tuiHandshake.js b/server/lib/tuiHandshake.js index 137efb8e7..893f58f74 100644 --- a/server/lib/tuiHandshake.js +++ b/server/lib/tuiHandshake.js @@ -398,21 +398,29 @@ export const REVIEW_LOOP_IDLE_TIMEOUT_MS = 900000; // only extends the (bounded) idle window, and a false NEGATIVE just preserves // prior behavior. Matched against ANSI-stripped output. // -// All three patterns are anchored to the literal RENDERED shape rather than a -// bare substring, because this repo bundles the slashdo docs that DESCRIBE -// these banners in prose — `lib/slashdo/lib/multi-reviewer-loop.md` alone -// contains the word "multi-reviewer" dozens of times and the literal phrase -// "Review plan:" once (inside its own instruction text), and this project's -// CLAUDE.md convention text is "run a simplify/self-review pass before -// committing", whose substring "review pass" would otherwise latch on ANY -// CoS agent's ordinary narration or docs-editing — not just an actual +// Both patterns are anchored to the literal RENDERED shape rather than a bare +// substring, because this repo bundles the slashdo docs that DESCRIBE these +// banners — `lib/slashdo/lib/multi-reviewer-loop.md` alone contains the word +// "multi-reviewer" dozens of times, the literal phrase "Review plan:" once +// (inside its own instruction text), AND a fully-rendered example of its own +// aggregate-report heading ("## Multi-Reviewer Summary", in the doc's sample +// output block) — so a THIRD marker keyed on that heading alone would latch +// on any agent reading/quoting that one doc file (codex review flagged this +// exact collision; verified via `grep -rn "multi-reviewer summary" +// lib/slashdo/` — it's the only bundled doc containing that literal string). +// This project's CLAUDE.md convention text is also "run a simplify/ +// self-review pass before committing", whose substring "review pass" would +// otherwise latch on ANY CoS agent's ordinary narration — not just an actual // do:release/do:pr/do:rpr run. Anchoring on the shape only the runtime output -// actually has (a rendered `[...]` agent list, a digit/slash pass counter, or -// the literal report heading) keeps the false-positive rate low without -// weakening true-positive detection of the real banners. +// actually has (a rendered `[...]` agent list, or a digit/slash pass counter) +// — verified clean against every bundled slashdo doc — keeps the +// false-positive rate low without weakening true-positive detection: the +// review-plan banner alone is a complete, sufficient signal (it prints once, +// unconditionally, before ANY reviewer pass begins) and the tracker latches +// permanently once set, so the pass-banner pattern only needs to catch the +// (rare) case where the plan banner itself was missed. const REVIEW_PLAN_PATTERN = /review plan:\s*\[/i; const REVIEW_PASS_BANNER_PATTERN = /review pass\s+\d+\s*\/\s*\d+/i; -const REVIEW_SUMMARY_PATTERN = /multi-reviewer summary/i; /** * True when a chunk of ANSI-stripped TUI output shows the multi-reviewer loop @@ -424,9 +432,7 @@ const REVIEW_SUMMARY_PATTERN = /multi-reviewer summary/i; */ export function isReviewLoopSignal(strippedText) { if (typeof strippedText !== 'string' || !strippedText) return false; - return REVIEW_PLAN_PATTERN.test(strippedText) - || REVIEW_PASS_BANNER_PATTERN.test(strippedText) - || REVIEW_SUMMARY_PATTERN.test(strippedText); + return REVIEW_PLAN_PATTERN.test(strippedText) || REVIEW_PASS_BANNER_PATTERN.test(strippedText); } /** diff --git a/server/lib/tuiHandshake.test.js b/server/lib/tuiHandshake.test.js index 3861aef5a..0f32e8fef 100644 --- a/server/lib/tuiHandshake.test.js +++ b/server/lib/tuiHandshake.test.js @@ -308,7 +308,6 @@ describe('tuiHandshake — review-loop idle suppression', () => { it('isReviewLoopSignal matches multi-reviewer-loop chrome (case-insensitive)', () => { expect(isReviewLoopSignal('Review plan: [claude, codex] (mode: series, stop-mode: all)')).toBe(true); expect(isReviewLoopSignal('--- Review pass 1/2: codex ---')).toBe(true); - expect(isReviewLoopSignal('## Multi-Reviewer Summary')).toBe(true); }); it('isReviewLoopSignal ignores ordinary implementation/output chrome', () => { @@ -319,20 +318,24 @@ describe('tuiHandshake — review-loop idle suppression', () => { expect(isReviewLoopSignal(undefined)).toBe(false); }); - // Regression for false-positive latches found across two rounds of local - // review: bare substrings ('review pass', 'review loop', 'multi-reviewer') - // would match this project's own CLAUDE.md convention ("run a simplify/ - // self-review pass before committing") and this repo's bundled slashdo docs - // (which say "review loop" and "multi-reviewer" dozens of times, including - // the literal instruction text "Review plan: {REVIEW_AGENTS}..."), latching - // the tracker for ANY CoS agent's ordinary narration or docs-editing — not - // just an actual do:release/do:pr/do:rpr run. + // Regression for false-positive latches found across three rounds of local + // review: bare substrings ('review pass', 'review loop', 'multi-reviewer', + // 'multi-reviewer summary') would match this project's own CLAUDE.md + // convention ("run a simplify/self-review pass before committing") and this + // repo's bundled slashdo docs — which say "review loop"/"multi-reviewer" + // dozens of times, include the literal instruction text "Review plan: + // {REVIEW_AGENTS}...", AND (codex review's finding) render a fully-formed + // example of their own "## Multi-Reviewer Summary" aggregate-report heading + // in a sample output block — latching the tracker for ANY CoS agent's + // ordinary narration or docs-editing, not just an actual do:release/do:pr/ + // do:rpr run. it('isReviewLoopSignal does NOT match ordinary self-review narration or doc prose', () => { expect(isReviewLoopSignal('running the self-review pass before committing')).toBe(false); expect(isReviewLoopSignal('## Local Agent Code Review Loop')).toBe(false); expect(isReviewLoopSignal('You are a Copilot review loop agent.')).toBe(false); expect(isReviewLoopSignal('the multi-reviewer wrapper dispatches each listed agent')).toBe(false); expect(isReviewLoopSignal('Print the resolved plan before starting: `Review plan: {REVIEW_AGENTS} (mode: ...)`')).toBe(false); + expect(isReviewLoopSignal('## Multi-Reviewer Summary')).toBe(false); }); it('createReviewLoopTracker latches on first signal and stays active through silence', () => { From 836c447990f4bf6f327ad4d6f948c42ca122f158 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Fri, 3 Jul 2026 07:36:37 -0700 Subject: [PATCH 7/7] address review (codex): buffer review-loop markers across PTY chunks codex review [P2] found reviewLoop.observe() only checked the current onData chunk, so a real TUI splitting the one-shot Review plan:/Review pass banner across two chunks (plausible during token-by-token streaming) would never latch the tracker, silently reintroducing the original idle-reap bug this PR fixes. createReviewLoopTracker now keeps a small bounded rolling tail (512 chars, well over the banner's ~60-char size) and tests the concatenated buffer, so a split marker is still detected on the very next chunk. --- server/lib/tuiHandshake.js | 21 ++++++++++++++++++++- server/lib/tuiHandshake.test.js | 13 +++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/server/lib/tuiHandshake.js b/server/lib/tuiHandshake.js index 893f58f74..c9a2e9a26 100644 --- a/server/lib/tuiHandshake.js +++ b/server/lib/tuiHandshake.js @@ -435,6 +435,13 @@ export function isReviewLoopSignal(strippedText) { return REVIEW_PLAN_PATTERN.test(strippedText) || REVIEW_PASS_BANNER_PATTERN.test(strippedText); } +// Rolling tail cap for createReviewLoopTracker's cross-chunk buffer (below). +// The banner text itself is well under 100 chars (e.g. "Review plan: [claude, +// codex] (mode: series, stop-mode: all)" is ~62), so this is generous +// headroom for intervening chrome without letting the buffer grow unbounded +// over a long-running session. +const REVIEW_LOOP_TAIL_CAP = 512; + /** * Latching tracker for "this agent is waiting inside a multi-reviewer loop" * (do:release/do:pr/do:rpr). Feed it each ANSI-stripped post-submit chunk via @@ -445,14 +452,26 @@ export function isReviewLoopSignal(strippedText) { * grace is needed). Once latched, the idle reaper uses * REVIEW_LOOP_IDLE_TIMEOUT_MS instead of the 3-minute default. * + * Keeps a small rolling buffer of the most recent REVIEW_LOOP_TAIL_CAP + * characters (codex review finding, iteration 2): a real TUI can deliver the + * one-shot `Review plan: [` / `Review pass N/M` banner split across two + * `onData` chunks — plausible during token-by-token streaming — so checking + * only the current chunk in isolation would miss it if the split lands + * mid-marker. Concatenating each new chunk onto the tail before testing means + * a marker split across a chunk boundary still appears whole on the very next + * observation. + * * @returns {{ observe: (strippedText: string) => boolean, readonly active: boolean }} */ export function createReviewLoopTracker() { let active = false; + let tail = ''; return { observe(strippedText) { if (active) return true; - if (isReviewLoopSignal(strippedText)) active = true; + if (typeof strippedText !== 'string' || !strippedText) return active; + tail = (tail + strippedText).slice(-REVIEW_LOOP_TAIL_CAP); + if (isReviewLoopSignal(tail)) active = true; return active; }, get active() { return active; }, diff --git a/server/lib/tuiHandshake.test.js b/server/lib/tuiHandshake.test.js index 0f32e8fef..4cf44f8ab 100644 --- a/server/lib/tuiHandshake.test.js +++ b/server/lib/tuiHandshake.test.js @@ -352,6 +352,19 @@ describe('tuiHandshake — review-loop idle suppression', () => { tracker.observe(''); expect(tracker.active).toBe(true); }); + + // Regression for codex review [P2] (iteration 2): a real TUI can deliver + // the banner split across two onData chunks (token-by-token streaming), + // so checking only the current chunk in isolation would miss it. + it('createReviewLoopTracker latches on a marker split across two chunks', () => { + const tracker = createReviewLoopTracker(); + // Split right before the '[' that anchors the pattern — neither half + // alone contains "review plan: [". + tracker.observe('Now starting the review loop. Review plan:'); + expect(tracker.active).toBe(false); + expect(tracker.observe(' [claude, codex] (mode: series, stop-mode: all)')).toBe(true); + expect(tracker.active).toBe(true); + }); }); describe('tuiHandshake.inferTuiCommand', () => {