fix(codex): skip the blocked re-read while the pane is quiet

While a codex dialog was up, the session re-read its screen every 2 s with
a synchronous `tmux capture-pane`, for as long as the dialog stayed up. That
read costs about 8 ms of blocked event loop (measured 2026-10-14), and an
approval can sit unanswered overnight: 1,800 execs an hour per waiting
session, measured as 300 in 10 quiet minutes in the unit test.

A dialog leaves the screen only through a repaint, and tmux sends every
repaint to the session's client as PTY output. So while the last reading
saw the dialog and nothing has been drawn since, it is still up and the
read is skipped; a safety-net read still runs every 30 s
(BLOCKED_QUIET_RECHECK_MS). The clear keeps its 2 to 4 s latency, because
the answer itself is output. Never skipped once a reading has missed the
dialog: tmux updates the pane before it sends the repaint, and the
confirming read must not wait for output that may already have arrived.

The session-state tests now change the fake screen the way a real pane
changes (with output), and two new tests pin the skip and the confirming
read (the second fails with the guard removed).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer committed 2026-10-14 05:53:58 +02:00
1 parent 9b3030f9a1
commit 1c1658d039
5 files changed
+109 -10

No files matched your search

+1 -1
View File
@@ -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). ⚠️ **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`. ⚠️ **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`.
+5 -1
View File
@@ -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 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 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 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 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 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 dialog). An unreadable capture changes nothing in either direction. A blocked screen wins
+11
View File
@@ -179,6 +179,17 @@ export const BLOCKED_MAX_TAIL_LINES = 24;
*/ */
export const BLOCKED_RECHECK_MS = 2000; 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 * 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 * read can land mid-repaint (a resize redraws the whole frame), and a clear followed by a
+19
View File
@@ -93,6 +93,7 @@ import {
WATCHING_TAIL_LINES, WATCHING_TAIL_LINES,
BLOCKED_TAIL_LINES, BLOCKED_TAIL_LINES,
BLOCKED_RECHECK_MS, BLOCKED_RECHECK_MS,
BLOCKED_QUIET_RECHECK_MS,
BLOCKED_CLEAR_READS, BLOCKED_CLEAR_READS,
IDLE_RECHECK_MS, IDLE_RECHECK_MS,
PANE_PROBE_MIN_INTERVAL_MS, PANE_PROBE_MIN_INTERVAL_MS,
@@ -613,6 +614,8 @@ export class Session extends EventEmitter {
private _screenBlocked = false; private _screenBlocked = false;
/** Consecutive readable captures without the dialog (cleared at BLOCKED_CLEAR_READS). */ /** Consecutive readable captures without the dialog (cleared at BLOCKED_CLEAR_READS). */
private _blockedGoneReads = 0; 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. */ /** Re-reads the screen while a dialog is up, so the alert clears with it. */
private _blockedRecheckTimer: NodeJS.Timeout | null = null; private _blockedRecheckTimer: NodeJS.Timeout | null = null;
/** /**
@@ -3548,6 +3551,8 @@ export class Session extends EventEmitter {
if (onScreen === null || !this._blockedLinePattern()) return; if (onScreen === null || !this._blockedLinePattern()) return;
if (onScreen) { if (onScreen) {
this._blockedGoneReads = 0; this._blockedGoneReads = 0;
// Stamped after the capture returned: output processed later is newer than it.
this._blockedSeenAt = Date.now();
if (!this._screenBlocked) { if (!this._screenBlocked) {
this._screenBlocked = true; this._screenBlocked = true;
this.emit('blockedChanged', true, frame); 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 // 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. // `_resetBuffers()` clears the state and whose repaint reads the screen afresh.
if (this._isStopped || !this._screenBlocked || !this._mux?.capturePaneText || !this._muxSession) return; 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; const text = this._mux.capturePaneText(this._muxSession.muxName) ?? null;
// Unreadable this time: keep the state, keep looking (_noteBlockedReading ignores a null). // Unreadable this time: keep the state, keep looking (_noteBlockedReading ignores a null).
if (text === null) { if (text === null) {
+73 -8
View File
@@ -25,7 +25,9 @@ import {
BLOCKED_MAX_TAIL_LINES, BLOCKED_MAX_TAIL_LINES,
BLOCKED_RECHECK_MS, BLOCKED_RECHECK_MS,
BLOCKED_CLEAR_READS, BLOCKED_CLEAR_READS,
BLOCKED_QUIET_RECHECK_MS,
IDLE_SILENCE_MS, IDLE_SILENCE_MS,
PANE_PROBE_MIN_INTERVAL_MS,
} from '../src/session-activity.js'; } from '../src/session-activity.js';
import type { SessionMode } from '../src/types.js'; import type { SessionMode } from '../src/types.js';
@@ -195,11 +197,20 @@ function feed(session: Session, data: string): void {
internals._detectInteractiveActivity(data); 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). */ /** 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 = { const mux = {
isAvailable: () => true, isAvailable: () => true,
capturePaneText: () => screen.text, capturePaneText: () => {
screen.captures = (screen.captures ?? 0) + 1;
return screen.text;
},
} as unknown as NonNullable<ConstructorParameters<typeof Session>[0]>['mux']; } as unknown as NonNullable<ConstructorParameters<typeof Session>[0]>['mux'];
return new Session({ return new Session({
workingDir: '/tmp', workingDir: '/tmp',
@@ -209,6 +220,16 @@ function withFakePane(screen: { text: string | null }, mode: SessionMode = 'code
} as ConstructorParameters<typeof Session>[0]); } as ConstructorParameters<typeof Session>[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. */ /** Output, then quiet: the moment the idle confirmation reads the screen. */
function settle(session: Session): void { function settle(session: Session): void {
for (let i = 0; i < 3; i++) { 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 // Answered: codex resumes the turn, and nothing settles while it runs. The timer is
// what notices, and one read alone is not enough. // 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); vi.advanceTimersByTime(BLOCKED_RECHECK_MS);
expect(session.screenBlocked).toBe(true); expect(session.screenBlocked).toBe(true);
vi.advanceTimersByTime(BLOCKED_RECHECK_MS * (BLOCKED_CLEAR_READS - 1)); vi.advanceTimersByTime(BLOCKED_RECHECK_MS * (BLOCKED_CLEAR_READS - 1));
@@ -291,9 +312,9 @@ describe('Session.screenBlocked', () => {
const changes = recordChanges(session); const changes = recordChanges(session);
settle(session); settle(session);
screen.text = fixture('working.txt'); redraw(session, screen, fixture('working.txt'));
vi.advanceTimersByTime(BLOCKED_RECHECK_MS); vi.advanceTimersByTime(BLOCKED_RECHECK_MS);
screen.text = dialog; redraw(session, screen, dialog);
vi.advanceTimersByTime(BLOCKED_RECHECK_MS * 3); vi.advanceTimersByTime(BLOCKED_RECHECK_MS * 3);
expect(session.screenBlocked).toBe(true); expect(session.screenBlocked).toBe(true);
expect(changes).toHaveLength(1); expect(changes).toHaveLength(1);
@@ -304,23 +325,67 @@ describe('Session.screenBlocked', () => {
const screen = { text: fixture('exec-approval.txt') as string | null }; const screen = { text: fixture('exec-approval.txt') as string | null };
const session = withFakePane(screen); const session = withFakePane(screen);
settle(session); 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); vi.advanceTimersByTime(BLOCKED_RECHECK_MS * 5);
expect(screen.captures! - before).toBeGreaterThanOrEqual(5);
expect(session.screenBlocked).toBe(true); expect(session.screenBlocked).toBe(true);
// ...and it is still watching: once the screen reads again without the dialog, it clears. // ...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); vi.advanceTimersByTime(BLOCKED_RECHECK_MS * BLOCKED_CLEAR_READS);
expect(session.screenBlocked).toBe(false); 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', () => { it('raises again for the next dialog after a clear', () => {
vi.useFakeTimers(); vi.useFakeTimers();
const screen = { text: fixture('exec-approval.txt') as string | null }; const screen = { text: fixture('exec-approval.txt') as string | null };
const session = withFakePane(screen); const session = withFakePane(screen);
const changes = recordChanges(session); const changes = recordChanges(session);
settle(session); settle(session);
screen.text = fixture('after-approve.txt'); redraw(session, screen, fixture('after-approve.txt'));
vi.advanceTimersByTime(BLOCKED_RECHECK_MS * BLOCKED_CLEAR_READS); vi.advanceTimersByTime(BLOCKED_RECHECK_MS * BLOCKED_CLEAR_READS);
screen.text = fixture('edit-approval.txt'); screen.text = fixture('edit-approval.txt');
settle(session); settle(session);