diff --git a/.ai/contexts/trigger-watcher.md b/.ai/contexts/trigger-watcher.md index 7bc4a4df..d4193f34 100644 --- a/.ai/contexts/trigger-watcher.md +++ b/.ai/contexts/trigger-watcher.md @@ -184,6 +184,102 @@ The SGR-and-focus exemption above (PR #160) is correct and worth keeping, but it **cannot** be the cause of this symptom: exempting mouse reports cannot quiet a channel that carries none, since Claude Code never turns motion tracking on. +**Found it — CPR, fixed 2026-09-04.** A 340 s trace of two idle sessions +(`SWITCHBOARD_ACTIVITY_TRACE`, real machine, nobody at the keyboard) put a +number on the suspects above: + +| Shape | Count | out of 2,422 `pty.input` chunks | +|---|---|---| +| CPR / DECXCPR (`CSI [?] row ; col [; page] R`) | 1,427 | 59% | +| SGR mouse (`CSI < b ; x ; y M`/`m`) | 495 | 20% — already excluded, working | +| plain text, no control byte (real typing) | 473 | 20% — real keystrokes, correctly counted | +| DEL / CR | 27 | 1% — real keystrokes | +| OSC, DCS | 0 | not observed on this machine, ever | + +CPR alone, arriving roughly every 240 ms, kept `lastInputAt` reassigned +continuously: the 3000 ms quiet window (`DEFAULT_QUIET_MS`) could not open +between two sessions still on screen, whatever the pointer or the CLI's own +busy state were doing. The 2026-09-02 fix above was correct but aimed at 20% +of the traffic; the dominant 59% was untouched, which is why the symptom +outlived it. + +`composer-state.js`'s `reportLength()` now recognises CPR/DECXCPR the same +way it recognises SGR mouse and focus reports: a dedicated regex bounding +every numeric field (`CPR_PARAMS_RE`), not a bare check on the final byte. +`applyCsi` has no case for final `R` — falls through its default — so before +this fix a CPR chunk pushed the clock but never touched `text`/`cursor`/ +`pending`; the defect was confined to the quiet window, not to composer +content. Verified by reading the switch, not by a runtime test: once +`reportLength` claims `R`, `applyCsi` never sees it, so nothing post-fix can +exercise that unreachable case (see `test/composer-state.test.js`). + +**"The same way" is an analogy, not the safety argument — the two exemptions +rest on different properties.** What makes `SGR_MOUSE_PARAMS_RE` safe is that +its final bytes (`M`/`m`) only reach `reportLength` on a chunk starting with +`CSI <`, and no keyboard on this machine's key-event handler +(`public/terminal-manager.js`, `attachCustomKeyEventHandler`) or xterm.js's own +`onData` emits `<` as the third byte of a CSI sequence — the prefix is +terminal-report-only by construction, so bounding the numeric fields is +enough. Final `R` has no such prefix to lean on: xterm.js emits bare +`CSI n ; m R` for a *modified F3 keypress* (`xterm.js`, `case 114`: +`ESC+"[1;"+(mod+1)+"R"`, `1;2` through `1;8` for Shift/Alt/Ctrl combinations). +An earlier version of `CPR_PARAMS_RE` made the `?` optional (`\??`), so it +matched that shape too — a real keystroke silently exempted from the quiet +clock, the direction this whole guard exists to prevent, and strictly worse +than the CPR flood it was fixing. Caught before merge by checking a keyboard +source (xterm.js) rather than reasoning by analogy from the mouse case. + +The actual safety argument for `CPR_PARAMS_RE`, mandatory `?` included: DECXCPR +(`CSI ? row ; col [; page] R`) is what this terminal answers with, and it is +the *only* shape observed — 24,795 CPR chunks in the 2026-09-04 trace, `?` +present in all 24,795, present in zero of the responses this codebase has ever +seen without it. Requiring the `?` excludes exactly the modified-F3 shape +above and nothing measured. **Residual**: this is a measurement on one +CLI/terminal pairing, not a proof that no terminal ever answers CPR without +`?` (plain DSR-6, `CSI row ; col R`, is a legal *bare* CPR in the DEC standard +— just not one this xterm.js/ConPTY combination has been observed to send). +If a future terminal or `xterm.js` config starts answering bare CPR, this +regex will — correctly, per the "doubt resolves to busy" principle — count +that as a keystroke rather than false-exempt it; the failure mode of being +wrong here is a spurious wait, not a swallowed keypress. + +The end-to-end proof — a CPR flood no longer blocking `waitForComposerFree`'s +free condition — lives in `test/composer-state-quiet-window.test.js`, which +reimplements the one-line predicate against fake time rather than importing +`waitForComposerFree`: that function is not exported, and PR #168 edits it +directly, so adding an export here would create an avoidable conflict for a +one-line predicate that composer-state.js's own clock behaviour already +proves. + +**DSR, DA1/DA2, DECRPM, window-ops (`t`) — not added, zero occurrences +measured.** The brief that produced this fix asked for all of these as a +matter of course; the trace has none of them, on either the 103 s or the +340 s window, so they are deliberately left out rather than excluded on +reasoning alone — the same conservatism the SGR-params regex already applies +to mouse reports. DECRPM (`CSI ? Pd $ y`) additionally can't be told apart +from a bare final `y` with the current `matchEscape`: the intermediate `$` +byte is consumed but not returned in the match, so distinguishing it would +mean changing `matchEscape`'s return shape, not just adding a param regex — +out of scope for a fix this size. If a future CLI release starts querying any +of these (plausible — Claude Code already probes DECRPM-style modes for +things like synchronized-output support), re-run the trace and treat it the +same way CPR was: measure first, then add the exact shape, never the final +byte alone. + +**OSC and DCS replies — parsed differently, and both unmeasured.** OSC +replies (colour query, `10`/`11`) are matched whole by `matchEscape` (kind +`osc`) and, since nothing in `noteUserInput` applies to that kind, push +`lastInputAt` without touching `text`/`pending` — a defect of the same shape +as CPR's, just with zero observed occurrences here. DCS is worse and +structural, not a report-recognition gap: `matchEscape` has no DCS +introducer case, so `ESC P` falls into the generic 2-byte `esc` catch-all, +and everything up to the terminator (`ESC \` or the DECRQSS/XTGETTCAP +payload) is then walked byte-by-byte as literal text — a DCS reply, if one +ever arrived, would be typed into the composer. Not fixed here: zero measured +occurrences, and the fix is a `matchEscape` change (recognising and +discarding a whole DCS sequence), not a `reportLength` addition — a +different, larger piece of work than this PR's scope. + **`submitted`.** Every result carries it, compared by strict equality: `confirmed` (a busy rising edge was observed after our write), `assumed` (written, no failure seen, nothing observed after), `no` (nothing written, or diff --git a/composer-state.js b/composer-state.js index b36e2918..e24fdb9d 100644 --- a/composer-state.js +++ b/composer-state.js @@ -152,12 +152,15 @@ function matchEscape(buf, i) { } // ── Terminal reports ───────────────────────────────────────────────────────── -// Mouse and focus reports ride the same channel as keystrokes but are not user -// input: neither text nor activity. Recognition is deliberately strict — see -// .ai/contexts/trigger-watcher.md. +// Mouse, focus and cursor-position reports ride the same channel as +// keystrokes but are not user input: neither text nor activity. Recognition +// is deliberately strict — see .ai/contexts/trigger-watcher.md. const SGR_MOUSE_PARAMS_RE = /^<\d{1,10};\d{1,10};\d{1,10}$/; +// DECXCPR only, `?` mandatory: `CSI ? row ; col [; page] R` — see .ai/contexts/trigger-watcher.md. +const CPR_PARAMS_RE = /^\?\d{1,4};\d{1,4}(?:;\d{1,4})?$/; + /** * How many bytes of terminal report start at the sequence `seq` just matched. * 0 when the sequence is not a report. @@ -169,6 +172,8 @@ function reportLength(seq) { if ((final === 'M' || final === 'm') && SGR_MOUSE_PARAMS_RE.test(params)) return seq.len; // Focus in / focus out (CSI ?1004h). if ((final === 'I' || final === 'O') && params === '') return seq.len; + // Cursor position report. + if (final === 'R' && CPR_PARAMS_RE.test(params)) return seq.len; return 0; } diff --git a/test/composer-state-quiet-window.test.js b/test/composer-state-quiet-window.test.js new file mode 100644 index 00000000..c5b90e07 --- /dev/null +++ b/test/composer-state-quiet-window.test.js @@ -0,0 +1,67 @@ +// test/composer-state-quiet-window.test.js — end-to-end cover for the free +// condition `waitForComposerFree` (trigger-watcher.js) polls on. Reimplements +// the predicate against fake time rather than importing it — see +// .ai/contexts/trigger-watcher.md ("Found it — CPR") for why. +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { createComposerState, noteUserInput } = require('../composer-state'); + +const QUIET_MS = 3000; // trigger-watcher.js DEFAULT_QUIET_MS +const CPR = '\x1b[?59;3R'; + +function isFree(state, now) { + return state.pending === 0 && (now - (state.lastInputAt || 0)) >= QUIET_MS; +} + +test('quiet window: a realistic CPR flood no longer blocks it forever', () => { + // Measured 2026-09-04: 1,427 CPR chunks over ~340s of idle-session trace, + // roughly one every 238ms — far more often than the 3000ms quiet window. + const state = createComposerState(); + let now = 0; + let everFree = false; + for (let i = 0; i < 50; i++) { + now += 238; + noteUserInput(state, CPR, now); + if (isFree(state, now)) { everFree = true; break; } + } + assert.equal(everFree, true, 'the quiet window must open under CPR traffic alone'); +}); + +test('quiet window: still refuses while the user is actually typing', () => { + // The direction that must never flip: real keystrokes interleaved with the + // same CPR flood keep the composer busy. + const state = createComposerState(); + let now = 0; + for (let i = 0; i < 12; i++) { + now += 238; + noteUserInput(state, CPR, now); + if (i === 6) noteUserInput(state, 'x', now); // a keystroke lands mid-flood + } + assert.equal(state.pending, 1, 'the typed character is still sitting in the box'); + assert.equal(isFree(state, now), false, 'a non-empty composer is never free'); + + noteUserInput(state, '\r', (now += 100)); // submit + assert.equal(state.pending, 0); + assert.equal(isFree(state, now), false, 'still inside the quiet window right after submit'); + assert.equal(isFree(state, now + QUIET_MS), true, 'free once the quiet window elapses'); +}); + +test('quiet window: pre-fix behaviour never opens under the same flood (control)', () => { + // Same drive as the first test, but reimplementing the pre-fix predicate — + // "every CSI that is not mouse/focus counts as input" — to show the old + // code path genuinely could not pass. This is the failing case the fix + // replaces, kept here as documentation rather than a live assertion on + // production code (it does not import anything from composer-state.js). + let lastInputAt = 0; + let now = 0; + let everFree = false; + for (let i = 0; i < 50; i++) { + now += 238; + lastInputAt = now; // CPR was, pre-fix, indistinguishable from a keystroke + if ((now - lastInputAt) >= QUIET_MS) { everFree = true; break; } + } + assert.equal(everFree, false, 'documents the bug: the clock never opened before this fix'); +}); diff --git a/test/composer-state.test.js b/test/composer-state.test.js index 80737a85..2e73f83f 100644 --- a/test/composer-state.test.js +++ b/test/composer-state.test.js @@ -202,6 +202,11 @@ test('composer-state: an unrecognised sequence still counts as input', () => { ['\x1b[1;2O', 'a modified CSI O'], ['\x1bOM', 'SS3 M, not CSI M'], ['\x1b[<0;42;13X', 'the right shape with the wrong final byte'], + // Empty numeric fields: the near-miss a `\d*` mutant would let through + // silently, since `*` accepts zero digits where `{1,10}` requires one. + ['\x1b[<;42;13M', 'SGR report with an empty button field'], + ['\x1b[<0;;13M', 'SGR report with an empty x field'], + ['\x1b[<0;42;M', 'SGR report with an empty y field'], ]; for (const [seq, why] of cases) { const state = createComposerState(); @@ -236,6 +241,111 @@ test('composer-state: a typed ESC [ M is input, not a mouse report', () => { assert.equal(keyByKey.lastInputAt, 7000); }); +// ── Cursor position reports (CPR / DECXCPR) ───────────────────────────────── +// Regression cover for the 2026-09-04 measurement: CPR chunks dominated a +// real idle-session trace and, left unhandled, kept the quiet clock from ever +// opening. See .ai/contexts/trigger-watcher.md ("Found it — CPR") for the numbers. + +const CPR_DECX = '\x1b[?59;3R'; // DECXCPR, as measured +const CPR_PAGE = '\x1b[?59;3;1R'; // DECXCPR with a page field + +test('composer-state: a cursor position report is neither text nor activity', () => { + assertInert([CPR_DECX]); + assertInert([CPR_PAGE]); + // The measured cadence: back-to-back queries as the column advances. + assertInert(['\x1b[?59;3R', '\x1b[?59;4R', '\x1b[?59;6R', '\x1b[?59;8R']); +}); + +// A bare `CSI n;m R` with no `?` is not a report at all on this CLI/terminal +// pairing: it is what xterm.js sends for a modified F3 keypress (`case 114` +// in xterm.js, `ESC[1;R`). 24,795/24,795 CPR chunks measured in the +// 2026-09-04 trace carried the `?`; zero did not — see +// .ai/contexts/trigger-watcher.md ("Found it — CPR"). The `?` is therefore +// mandatory in CPR_PARAMS_RE, not optional: a bare `n;m R` must count as +// input, exactly like any other unrecognised sequence. +test('composer-state: a bare (non-DECXCPR) CPR-shaped sequence is a keystroke, not a report', () => { + const bareCases = [ + ['\x1b[24;80R', 'bare CPR: no `?`, plausible-looking but never measured'], + ['\x1b[1;2R', 'Shift+F3'], + ['\x1b[1;3R', 'Alt+F3'], + ['\x1b[1;4R', 'Alt+Shift+F3'], + ['\x1b[1;5R', 'Ctrl+F3'], + ['\x1b[1;6R', 'Ctrl+Shift+F3'], + ['\x1b[1;7R', 'Ctrl+Alt+F3'], + ['\x1b[1;8R', 'Ctrl+Alt+Shift+F3'], + ]; + for (const [seq, why] of bareCases) { + const state = createComposerState(); + noteUserInput(state, 'hi', 1000); + noteUserInput(state, seq, 9000); + assert.equal(state.lastInputAt, 9000, `${why} (${JSON.stringify(seq)}) must push the quiet clock`); + } +}); + +test('composer-state: a CPR mixing with a keystroke counts only the keystroke', () => { + const state = createComposerState(); + noteUserInput(state, 'hi', 1000); + noteUserInput(state, CPR_DECX + 'x', 5000); + assert.equal(state.pending, 3, 'the keystroke lands in the composer'); + assert.equal(state.lastInputAt, 5000, 'and pushes the quiet clock'); + // The discriminating half: further CPRs on their own, after the keystroke, + // must not advance the clock past it. + noteUserInput(state, CPR_DECX, 9000); + assert.equal(state.lastInputAt, 5000, 'a later CPR alone must not push the clock again'); +}); + +test('composer-state: a CPR split across chunks is never counted as text', () => { + const state = createComposerState(); + noteUserInput(state, '\x1b[?59;', 1000); + assert.equal(state.pending, 0, 'a half report is held back, not typed'); + assert.equal(state.partial, '\x1b[?59;'); + noteUserInput(state, '3R', 2000); + assert.equal(state.pending, 0, 'the completed report adds nothing'); + assert.equal(state.partial, ''); + assert.equal(state.lastInputAt, 1000, 'the completing chunk is silence'); +}); + +test('composer-state: a near-miss CPR still counts as input', () => { + // Same safety principle as the SGR near-misses: only the exact shape is + // exempt. Arbitrary params ending in R must resolve towards busy. + const cases = [ + ['\x1b[24R', 'CPR missing the column field'], + ['\x1b[24;80;1;1R', 'one field too many'], + ['\x1b[24;80;1;1;1R', 'two fields too many'], + ['\x1b[a;bR', 'non-numeric CPR parameters'], + ['\x1b[24;80;1;R', 'a dangling separator'], + ['\x1b[5R', 'CSI 5 R — not a position report at all'], + // Empty numeric fields on an otherwise well-formed DECXCPR: the near-miss + // a `\d*` mutant would let through silently, since `*` accepts zero + // digits where `{1,4}` requires one. + ['\x1b[?;3R', 'DECXCPR with an empty row field'], + ['\x1b[?59;R', 'DECXCPR with an empty column field'], + ]; + for (const [seq, why] of cases) { + const state = createComposerState(); + noteUserInput(state, 'hi', 1000); + noteUserInput(state, seq, 9000); + assert.equal(state.lastInputAt, 9000, `${why} must push the quiet clock`); + } +}); + +// NOTE: whether a CPR could corrupt `text`/`cursor`/`pending` (not just the +// clock) was checked by reading `applyCsi` rather than by a test here — see +// .ai/contexts/trigger-watcher.md ("Found it — CPR") for why no runtime +// assertion on this can ever go red post-fix. + +test('composer-state: ordinary typing still pushes the clock through a CPR flood', () => { + // The guarantee that matters: excluding CPR must not accidentally exclude + // real keystrokes that merely resemble one in shape. + const state = createComposerState(); + let t = 1000; + for (let i = 0; i < 20; i++) noteUserInput(state, CPR_DECX, (t += 50)); + assert.equal(state.lastInputAt, 0, 'a CPR flood alone must never look like typing'); + noteUserInput(state, 'x', (t += 50)); + assert.equal(state.lastInputAt, t, 'a real keystroke still pushes the clock'); + assert.equal(state.pending, 1); +}); + test('composer-state: a bare CSI M never swallows the bytes that follow it', () => { // Treating it as the head of a report misaligned the next chunk and left the // composer frozen on a phantom count.