diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index d2674ec5..76b414d2 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -296,7 +296,7 @@ So: `_confirmIdle()` (session.ts) requires the pane to go quiet, and then asks t ⚠️ **ALERT ONLY, never a keystroke.** The text is pane-derived and the agent writes most of the pane, so the pattern must end on chrome only the CLI draws at the FOOT of the screen, where a dialog replaces the composer (codex's: a `› N.` option row, then its key-hint row as the last row, allowing for that row to wrap; a transcript that quotes every word still has codex's column-0 composer row below it). The item is `answerInTerminal` (#610's shape), the answer route refuses it, push carries no `approvalId`, and Codeman never answers such a dialog itself, so a forged match costs a false alert and nothing more. The message every surface shows is built from the CLI's registry label, never from the screen (a question's text and a command preview are the agent's to choose). -⚠️ **Raise on one reading, clear on two, and keep reading while it is up.** The raise needs no timer: a dialog is drawn at the END of a burst of output, so the settle probe (pane quiet, then capture) sees it. The clear does: answering resumes the turn, a busy pane never reaches the settle path again until the turn ends, and the red alert outlived the dialog by minutes, so `_blockedRecheckTimer` re-reads the screen every `BLOCKED_RECHECK_MS` (2 s, one `capture-pane`, only while a dialog is up; cleared in `_clearAllTimers()`). It clears after `BLOCKED_CLEAR_READS` (2) readable captures in a row without the dialog, because one capture can land inside a full-frame repaint, and a clear followed by a re-raise sends the alert, push and webhook twice for one dialog. An unreadable capture (`null`) changes nothing in either direction. A new pane (`_resetBuffers()`) and a PTY exit clear it, announced, while the listeners are still attached. The working probe decides blocked-over-working from THIS capture, never from `_screenBlocked`, which lags a vanished dialog by design. +⚠️ **Raise on one reading, clear on two, and keep reading while it is up.** The raise needs no timer: a dialog is drawn at the END of a burst of output, so the settle probe (pane quiet, then capture) sees it. The clear does: answering resumes the turn, a busy pane never reaches the settle path again until the turn ends, and the red alert outlived the dialog by minutes, so `_blockedRecheckTimer` re-reads the screen every `BLOCKED_RECHECK_MS` (2 s, one `capture-pane`, only while a dialog is up; cleared in `_clearAllTimers()`). ⚠️ That read is a synchronous exec (about 8 ms of blocked event loop, measured 2026-10-14) and a dialog can sit unanswered overnight, so the timer SKIPS it while the pane is quiet: a dialog leaves the screen only through a repaint, which reaches the session as PTY output, so nothing drawn since the last reading that saw the dialog (`_lastActivityAt < _blockedSeenAt`) means it is still up; a safety-net read still runs every `BLOCKED_QUIET_RECHECK_MS` (30 s). Never once a reading has MISSED the dialog (`_blockedGoneReads > 0`): tmux updates the pane before it sends this client the repaint, and the confirming read must not wait for output. It clears after `BLOCKED_CLEAR_READS` (2) readable captures in a row without the dialog, because one capture can land inside a full-frame repaint, and a clear followed by a re-raise sends the alert, push and webhook twice for one dialog. An unreadable capture (`null`) changes nothing in either direction. A new pane (`_resetBuffers()`) and a PTY exit clear it, announced, while the listeners are still attached. The working probe decides blocked-over-working from THIS capture, never from `_screenBlocked`, which lags a vanished dialog by design. ⚠️ **Hooks win.** `raiseScreenBlocked()` does nothing for a session whose CLI passes `hooksAvailableForMode()`, so a CLI that declared both could never alert twice for one dialog; and the inbox item gets no `capture` callback (only the frame, as context), because the inbox's option parser and staleness sweep are shaped for Claude's dialogs and the SESSION owns this item's lifetime. ⚠️ Codex's `Implement this plan?` after a plan turn is deliberately not matched: its `enter select · esc back` footer is the same as `/model`, `/permissions` and `/review`, menus the user opened at the keyboard, and the plan turn has ended there. Measured fixtures (codex-cli 0.162.0, 2026-10-14, widths 200 to 36): `test/fixtures/codex-dialogs/`. Tests: `test/codex-blocked-detection.test.ts` (the pattern and the session state), `test/screen-blocked.test.ts` (what the server raises), the codex blocks of `test/routes/session-wait-routes.test.ts` and `test/routes/approval-routes.test.ts`. diff --git a/docs/cli-registry.md b/docs/cli-registry.md index 2d54fb87..445bb43c 100644 --- a/docs/cli-registry.md +++ b/docs/cli-registry.md @@ -162,7 +162,11 @@ Three rules hold for any CLI that declares one: - The state raises on one reading (the session's settle probe, which reads the screen once the pane goes quiet, which is when a dialog has just been drawn), and while it is up a timer re-reads the screen every 2 s (`BLOCKED_RECHECK_MS`), because answering resumes the turn and - a busy pane would not reach the settle path again until the turn ended. It clears only after + a busy pane would not reach the settle path again until the turn ended. The read is skipped + while the pane has drawn nothing since the last reading that saw the dialog (a dialog leaves + only through a repaint), with a safety-net read every 30 s (`BLOCKED_QUIET_RECHECK_MS`), so a + dialog left unanswered for hours does not cost a synchronous `tmux capture-pane` every 2 s. + It clears only after two readable captures in a row without the dialog (`BLOCKED_CLEAR_READS`), so a capture taken mid-repaint cannot clear and re-raise it (a second alert, push and webhook for one dialog). An unreadable capture changes nothing in either direction. A blocked screen wins diff --git a/src/session-activity.ts b/src/session-activity.ts index 97f728a3..5b6d1140 100644 --- a/src/session-activity.ts +++ b/src/session-activity.ts @@ -179,6 +179,17 @@ export const BLOCKED_MAX_TAIL_LINES = 24; */ export const BLOCKED_RECHECK_MS = 2000; +/** + * The longest a session showing a blocking dialog goes without re-reading a QUIET pane. + * A dialog leaves the screen only through a repaint, and every repaint reaches the + * session as PTY output, so while nothing has been drawn since the last reading that saw + * the dialog it is still up, and the BLOCKED_RECHECK_MS read is skipped. That read is a + * synchronous `tmux capture-pane`, about 8 ms of blocked event loop (measured 2026-10-14), + * and a dialog can sit unanswered overnight: one per 2 s per waiting session for hours. + * This bound is only the safety net under that reasoning, never the clear's latency. + */ +export const BLOCKED_QUIET_RECHECK_MS = 30_000; + /** * Consecutive readable captures without the dialog before the blocked state clears. One * read can land mid-repaint (a resize redraws the whole frame), and a clear followed by a diff --git a/src/session.ts b/src/session.ts index 9ed2b9e5..829c81b3 100644 --- a/src/session.ts +++ b/src/session.ts @@ -93,6 +93,7 @@ import { WATCHING_TAIL_LINES, BLOCKED_TAIL_LINES, BLOCKED_RECHECK_MS, + BLOCKED_QUIET_RECHECK_MS, BLOCKED_CLEAR_READS, IDLE_RECHECK_MS, PANE_PROBE_MIN_INTERVAL_MS, @@ -613,6 +614,8 @@ export class Session extends EventEmitter { private _screenBlocked = false; /** Consecutive readable captures without the dialog (cleared at BLOCKED_CLEAR_READS). */ private _blockedGoneReads = 0; + /** When the last reading that SAW the dialog was taken. See `_armBlockedRecheck()`. */ + private _blockedSeenAt = 0; /** Re-reads the screen while a dialog is up, so the alert clears with it. */ private _blockedRecheckTimer: NodeJS.Timeout | null = null; /** @@ -3548,6 +3551,8 @@ export class Session extends EventEmitter { if (onScreen === null || !this._blockedLinePattern()) return; if (onScreen) { this._blockedGoneReads = 0; + // Stamped after the capture returned: output processed later is newer than it. + this._blockedSeenAt = Date.now(); if (!this._screenBlocked) { this._screenBlocked = true; this.emit('blockedChanged', true, frame); @@ -3571,6 +3576,20 @@ export class Session extends EventEmitter { // No pane left to read (detached): nothing can change until a re-attach, whose // `_resetBuffers()` clears the state and whose repaint reads the screen afresh. if (this._isStopped || !this._screenBlocked || !this._mux?.capturePaneText || !this._muxSession) return; + // A dialog leaves the screen only through a repaint, and tmux sends every repaint + // to this client as PTY output (`_markActivity`). So while the last reading saw + // the dialog and nothing has been drawn since, it is still up: skip the capture + // (synchronous, see BLOCKED_QUIET_RECHECK_MS) and look again next interval. Never + // once a reading has missed it: the confirming read must happen on a quiet pane + // too, or the clear would wait for output that may never come. + if ( + this._blockedGoneReads === 0 && + this._lastActivityAt < this._blockedSeenAt && + Date.now() - this._blockedSeenAt < BLOCKED_QUIET_RECHECK_MS + ) { + this._armBlockedRecheck(); + return; + } const text = this._mux.capturePaneText(this._muxSession.muxName) ?? null; // Unreadable this time: keep the state, keep looking (_noteBlockedReading ignores a null). if (text === null) { diff --git a/test/codex-blocked-detection.test.ts b/test/codex-blocked-detection.test.ts index 6b73a900..86addd6a 100644 --- a/test/codex-blocked-detection.test.ts +++ b/test/codex-blocked-detection.test.ts @@ -25,7 +25,9 @@ import { BLOCKED_MAX_TAIL_LINES, BLOCKED_RECHECK_MS, BLOCKED_CLEAR_READS, + BLOCKED_QUIET_RECHECK_MS, IDLE_SILENCE_MS, + PANE_PROBE_MIN_INTERVAL_MS, } from '../src/session-activity.js'; import type { SessionMode } from '../src/types.js'; @@ -195,11 +197,20 @@ function feed(session: Session, data: string): void { internals._detectInteractiveActivity(data); } +/** + * What the pane shows, and how often the session has read it: every read is a synchronous + * `tmux capture-pane` on the live server. + */ +type FakeScreen = { text: string | null; captures?: number }; + /** A session whose mux answers captures from `screen.text` (null = unreadable). */ -function withFakePane(screen: { text: string | null }, mode: SessionMode = 'codex'): Session { +function withFakePane(screen: FakeScreen, mode: SessionMode = 'codex'): Session { const mux = { isAvailable: () => true, - capturePaneText: () => screen.text, + capturePaneText: () => { + screen.captures = (screen.captures ?? 0) + 1; + return screen.text; + }, } as unknown as NonNullable[0]>['mux']; return new Session({ workingDir: '/tmp', @@ -209,6 +220,16 @@ function withFakePane(screen: { text: string | null }, mode: SessionMode = 'code } as ConstructorParameters[0]); } +/** + * The pane changes the only way a real one can: tmux redraws it to the attached client, + * which reaches the session as output. A cursor move and an erase, with no composer glyph + * in it, so it marks activity without arming the idle confirmation. + */ +function redraw(session: Session, screen: FakeScreen, text: string | null): void { + screen.text = text; + feed(session, '\x1b[2;1H\x1b[K'); +} + /** Output, then quiet: the moment the idle confirmation reads the screen. */ function settle(session: Session): void { for (let i = 0; i < 3; i++) { @@ -269,7 +290,7 @@ describe('Session.screenBlocked', () => { // Answered: codex resumes the turn, and nothing settles while it runs. The timer is // what notices, and one read alone is not enough. - screen.text = fixture('working.txt'); + redraw(session, screen, fixture('working.txt')); vi.advanceTimersByTime(BLOCKED_RECHECK_MS); expect(session.screenBlocked).toBe(true); vi.advanceTimersByTime(BLOCKED_RECHECK_MS * (BLOCKED_CLEAR_READS - 1)); @@ -291,9 +312,9 @@ describe('Session.screenBlocked', () => { const changes = recordChanges(session); settle(session); - screen.text = fixture('working.txt'); + redraw(session, screen, fixture('working.txt')); vi.advanceTimersByTime(BLOCKED_RECHECK_MS); - screen.text = dialog; + redraw(session, screen, dialog); vi.advanceTimersByTime(BLOCKED_RECHECK_MS * 3); expect(session.screenBlocked).toBe(true); expect(changes).toHaveLength(1); @@ -304,23 +325,67 @@ describe('Session.screenBlocked', () => { const screen = { text: fixture('exec-approval.txt') as string | null }; const session = withFakePane(screen); settle(session); - screen.text = null; + // The pane moved, so the session looks, and tmux answers nothing it can read. + redraw(session, screen, null); + const before = screen.captures!; vi.advanceTimersByTime(BLOCKED_RECHECK_MS * 5); + expect(screen.captures! - before).toBeGreaterThanOrEqual(5); expect(session.screenBlocked).toBe(true); // ...and it is still watching: once the screen reads again without the dialog, it clears. - screen.text = fixture('after-approve.txt'); + redraw(session, screen, fixture('after-approve.txt')); vi.advanceTimersByTime(BLOCKED_RECHECK_MS * BLOCKED_CLEAR_READS); expect(session.screenBlocked).toBe(false); }); + it('does not re-read a quiet pane every interval while a dialog sits unanswered', () => { + // Each read is a synchronous `tmux capture-pane` (about 8 ms of blocked event loop, + // measured 2026-10-14), and a dialog can sit unanswered overnight. It leaves the + // screen only through a repaint, which reaches the session as output, so a pane that + // drew nothing since the last reading that saw the dialog is still showing it. + vi.useFakeTimers(); + const screen: FakeScreen = { text: fixture('exec-approval.txt') }; + const session = withFakePane(screen); + settle(session); + expect(session.screenBlocked).toBe(true); + + const quietMs = 10 * 60_000; + const before = screen.captures!; + vi.advanceTimersByTime(quietMs); + // The safety-net read only, never one per BLOCKED_RECHECK_MS (that would be 300 here). + expect(screen.captures! - before).toBeLessThanOrEqual(quietMs / BLOCKED_QUIET_RECHECK_MS); + expect(session.screenBlocked).toBe(true); + + // The answer repaints the pane, and the clear comes as fast as before. + redraw(session, screen, fixture('after-approve.txt')); + vi.advanceTimersByTime(BLOCKED_RECHECK_MS * BLOCKED_CLEAR_READS); + expect(session.screenBlocked).toBe(false); + }); + + it('takes the confirming read even when no output follows the reading that missed the dialog', () => { + // tmux updates the pane before it sends this client the repaint, so a capture can see + // the dialog gone while the session has been sent nothing yet. Skipping a quiet pane + // is right only while the last reading SAW the dialog: once one reading missed it, + // waiting for output could leave the alert up for the whole safety-net interval. + vi.useFakeTimers(); + const screen: FakeScreen = { text: fixture('exec-approval.txt') }; + const session = withFakePane(screen); + settle(session); + vi.advanceTimersByTime(PANE_PROBE_MIN_INTERVAL_MS); + screen.text = fixture('after-approve.txt'); // and not one byte of output, from here on + (session as unknown as { _probePaneWorking(): boolean | null })._probePaneWorking(); + expect(session.screenBlocked).toBe(true); + vi.advanceTimersByTime(BLOCKED_RECHECK_MS); + expect(session.screenBlocked).toBe(false); + }); + it('raises again for the next dialog after a clear', () => { vi.useFakeTimers(); const screen = { text: fixture('exec-approval.txt') as string | null }; const session = withFakePane(screen); const changes = recordChanges(session); settle(session); - screen.text = fixture('after-approve.txt'); + redraw(session, screen, fixture('after-approve.txt')); vi.advanceTimersByTime(BLOCKED_RECHECK_MS * BLOCKED_CLEAR_READS); screen.text = fixture('edit-approval.txt'); settle(session);