diff --git a/CLAUDE.md b/CLAUDE.md index 7ef88de4..6d135a3c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -334,6 +334,12 @@ Frontend JS modules have `@fileoverview` with `@dependency`/`@loadorder` tags. L **Shell keyboard accessory bar + one-shot Ctrl** (issue #262, `keyboard-accessory.js`): a **shell**-mode session automatically swaps the mobile accessory bar for terminal controls (Ctrl, Esc, Tab, four arrows, paste, dismiss); every other mode keeps the agent bar. `setMode()` now records the user's `extendedKeyboardBar` preference as the **base** layout and `refreshForActiveSession()` (called from `selectSession`) resolves base-vs-shell, so a settings save during a shell session cannot yank the bar away and switching back restores the user's choice. ⚠️ **Ctrl is a ONE-SHOT modifier applied in `terminal.onData`, not in a keydown handler**: a virtual keyboard emits no usable key events, so the character only exists as onData text. The hook sits AFTER `shouldSuppressTerminalQueryResponse` (xterm answers DA/CPR through onData too, and one of those would silently spend the modifier) and BEFORE every send path, so the control byte follows the normal control-char route. ⚠️ **Not every onData chunk is a keystroke**, and the query filter is not enough on its own: xterm ALSO emits mouse and focus reports on its own initiative, so the hook skips them via `isTerminalFocusOrMouseReport()` (they still reach the PTY, they just don't count as the next key). The mouse half is live — a shell session keeps the NARROW strip, so mouse DECSETs reach the browser and one tap while vim/htop runs spent the armed modifier silently (measured). The focus half is defense in depth: `FOCUS_ESCAPE_FILTER` in `session.ts` strips `\x1b[?1004h` from every PTY read, so `sendFocusMode` never turns on today; if it ever did, the bar's own post-key refocus would emit `\x1b[I` and eat the modifier before the user typed. ⚠️ It must disarm on ALL of: use, second tap, any other accessory key, session switch, keyboard dismissal, and a layout swap; a modifier left armed turns the next innocent keystroke into a control byte. ⚠️ **onData is not the only input path** — with `cjkInputEnabled` on, the CJK textarea owns the keyboard (onData returns early for everything it swallows, and the focus router sends `terminal.focus()` there, which is where the bar refocuses after every key), so `_handleCjkInput()` applies the modifier too. It is that module's single choke point to the PTY, so one call covers typed characters, IME flushes, Enter, backspace and arrows. Without it an armed modifier could neither fire NOR be spent, and survived to a later keystroke. Mapping is `ctrlByteFor()` (`code & 0x1f` over @A-Z[\]^_ and a-z, plus Ctrl+Space=NUL / Ctrl+?=DEL); characters with no control equivalent pass through unchanged, like a hardware keyboard. ⚠️ The armed style is `.accessory-btn.accessory-btn-ctrl.armed` (0,3,0) in BOTH stylesheets, and it cannot outrank mobile.css's light-skin repaint at **(0,3,1)** (`:is()` inherits its most specific argument, and that list holds `.btn-toolbar.btn-shell`) — so that rule excludes the state by hand as `.accessory-btn:not(.armed)`. Without the exclusion the armed button renders identically to a resting one on all four light skins, which is worse than no armed style at all. +**Terminal resilience: replay clears, renderer liveness, fetch deadlines**: three rules that each close a way the terminal silently stops being correct. ⚠️ **A replay clear MUST be in-stream, never `reset()`/`clear()`.** xterm's `write()` is asynchronously queued while `Terminal.reset()` is synchronous and, per upstream, "does not clear input buffers and does not reset the parser" — so bytes queued just before a reset are parsed AFTER it and fuse into the snapshot written next. **Measured** against the real xterm in this repo: `write('p8'); reset(); write('rmissions')` renders `p8rmissions`; the queued `\x1bc` renders `rmissions` and clears scrollback. `_resetTerminalForReplay()` (app.js) is the ONE clear, a single queued `\x1bc` (RIS), and all three replay paths go through it; RIS rather than `\x1b[3J\x1b[H\x1b[2J` because the erase leaves modes, charsets, scroll regions and SGR state alone. Callers may still chunk the content — ordering in the queue is what matters, not writing it in one call. ⚠️ **The renderer watchdog reads xterm privates and CANNOT be covered by the gate.** `_kickRenderer()` (terminal-ui.js) cancels a stale `_core._renderService._renderDebouncer._animationFrame` and forces a repaint. **Verified against xterm 6.0.0** (jsdom, after `open()`): the field path resolves, a forced stale handle genuinely makes `refreshRows` a no-op, and the kick schedules a fresh frame. **Reasoned, not reproduced here**: the premise that iOS discards scheduled rAF callbacks when a PWA backgrounds, which is what leaves the handle stale — that half wants a real-device pass. Codeman has exactly ONE xterm for the whole page load, so one backgrounding would wedge it until a reload. `_renderService` only exists after `open()`, which needs a real DOM, and the gate runs in node — so `test/xterm-private-api.test.ts` pins the RESOLVED lockfile version (not the `^6.0.0` range, which a real upgrade slips through) and a bump means re-verifying by hand. Every access is optional-chained on purpose: a renamed field must degrade to a no-op, never throw on a 2s timer. ⚠️ **Every terminal capture carries a deadline, and the helper reads the BODY** (`_fetchTerminalCapture`, app.js). `await fetch()` settles on response HEADERS, so clearing the timer there leaves the body — the multi-megabyte `?full=1` capture this exists for — unbounded: **measured** at 4026ms under a 1000ms deadline before the fix. The helper therefore returns `{json, headers, headersAt}` rather than a `Response`, and `_terminalCaptureInflight` is scoped the same way so a body still streaming counts toward a capture starting beside it. It degrades to a plain fetch where `AbortController` is missing — the deadline is a safety net, not a dependency. Tests: `test/terminal-resilience.test.ts` (pure decisions), `test/xterm-private-api.test.ts`. + +**WebSocket output-gap reconcile** (`_wsOutputGapSession`, app.js): terminal OUTPUT frames carry no sequence number (input frames do — `seq`+`cid`, at-most-once, ACKed), so a dropped socket leaves a hole nothing replays. ⚠️ **The gap is narrower than "the device went offline"**: if the network drops, SSE drops with it and `handleInit`'s keepTerminal branch already calls `_onSessionNeedsRefresh`. The uncovered case is the WS dying while SSE stays up (half-open socket, proxy idle-timeout, ping timeout), because `_onSSETerminal` discards every SSE terminal frame while `_wsReady` is true and `_wsReady` only flips in `ws.onclose`. Reaching `onclose` at all means the drop was unintentional (`_disconnectWs` nulls the handler first), so the session is marked and the next successful open reconciles. ⚠️ **The marker must be cleared by EVERY path that repaints that session's buffer** — `_markTerminalBufferReconciled()` is called from `_onSessionNeedsRefresh`'s finally, from `selectSession` after its load, and from `_cleanupSessionData`. `selectSession` loads the buffer and only THEN calls `_connectWs`, so without that clear the socket opening afterwards replays the whole buffer a second time on top of the one just written. Sequencing the output frames is the real fix and is not done. This is reasoned from the code path, not observed on a device. + +**Service worker: precache and cache key are BUILD-GENERATED** (`sw.js` + `scripts/build.mjs`): the build content-hashes assets and rewrites two exact declarations in `sw.js` — `const BUILD_ID = 'dev';` and `const HASHED_ASSETS = [];`. ⚠️ **Each must appear exactly once or the build THROWS**, which is deliberate: the list used to be hand-maintained with PRE-hash names, so every entry 404'd in production and `cache.add().catch(() => {})` hid it (15 of 23 verified failing against a running instance). The dev literals are valid on their own, so dev serves an unrewritten worker with an empty precache. ⚠️ **`caches.match` must pass `ignoreSearch: true`**: `renderIndexHtml` runs `cacheBustAssets`, which appends `?v=` to every same-origin `.js`/`.css` reference INCLUDING content-hashed names, so the page requests `/app..js?v=` while the cache holds `/app..js`. Without it no precached entry is reachable and the install downloads ~1.3MB that can never be served — once per deploy, since `CACHE_NAME` now carries the build id. That per-build key is what makes `activate`'s cleanup actually delete anything; it used to be the constant `'codeman-v1'`, so assets from every past release accumulated forever. Contract pinned by `test/sw-precache-manifest.test.ts`, which PARSES the `HASHABLE` list out of `build.mjs` rather than copying it. + **Dismissing the on-screen keyboard** (PRs #279/#280, `terminal-ui.js`): the terminal parks focus on a hidden textarea that nothing used to release, so TWO gestures now blur it, and they own different regions. **(1)** `_installMobileKeyboardDismiss()` — a document-level `touchend` that fires only while the terminal input actually holds focus, **never inside `#terminalContainer`** (tap classification owns that) and **never on a control** (`MOBILE_KEYBOARD_DISMISS_EXEMPT_SELECTOR`, matched with `closest()` so an icon inside a button counts). Session tabs are covered by the selector's `[tabindex]:not([tabindex="-1"])` arm, which is what stops a tab tap from blurring and then being re-focused by `selectSession()`. **(2)** In `_handleMobileTerminalTap`, a second tap on **inert `content`** (`startedWithTerminalFocus`) blurs instead of re-focusing. ⚠️ Scoped to `content` on purpose: the prompt row (`input`) keeps focus-then-position so a second tap still places the caret, and actionable rows blur earlier via `_isActionableMobileTerminalTap`. ⚠️ **A scroll ends in `touchend` too** — dismissing there closes the keyboard and drops the composer mid-read, so travel is tracked from `touchstart` and multi-touch is never a tap. Both classifiers MUST share one threshold: `initTerminal`'s `TAP_THRESHOLD` reads `MOBILE_KEYBOARD_DISMISS_TAP_SLOP`, since a gesture the terminal calls a scroll and the dismiss handler calls a tap is exactly that bug. ⚠️ **The gate excludes `test/mobile/**`, so CI cannot see the only test covering (1)** — run `npm run test:mobile -- test/mobile/keyboard.test.ts` by hand and diff the FAIL list against master. (Not `npm test --`: the gate's config excludes that path, so a file filter pointing into it matches nothing and exits green having run zero tests.) That blind spot is why merging the two PRs, which conflicted semantically but not textually, produced a red suite with two green CI checks. **Phone toolbar: Enter replaces Shell** (post-1.8.0): inside `@media (max-width: 599px)` `btn-shell` is `display:none` and `btn-enter` takes its slot (`order: 4`); starting a shell moved into the Run dropdown (`Terminal / Shell` → `setRunMode('shell')` → `run()` → `runShell()`, button label "Run SH"). `runMode` is `z.string().max(20)` server-side, so new modes need no schema change. Desktop and tablet keep the green Run Shell button unchanged. diff --git a/scripts/build.mjs b/scripts/build.mjs index 1fbb34d8..2ab8ecd5 100644 --- a/scripts/build.mjs +++ b/scripts/build.mjs @@ -146,10 +146,44 @@ console.log('\n[build] content-hash cache busting'); html = html.replaceAll(`"${original}"`, `"${hashed}"`); } writeFileSync(join(distPublic, 'index.html'), html); + + // Rewrite sw.js from the SAME manifest that just renamed the files. + // + // The service worker's precache list used to be maintained by hand with the + // pre-hash names, so after this step every entry in it pointed at a file that + // no longer existed and `cache.add(...).catch(() => {})` hid it. Deriving it + // here is the only way the two cannot drift. + // + // The cache key gets the build hash for the same reason: `activate` deletes + // every cache that is not the current one, so a constant key meant that + // cleanup never ran and hashed assets from every past release piled up. + const swPath = join(distPublic, 'sw.js'); + let sw = readFileSync(swPath, 'utf8'); + const hashedAssets = Object.values(manifest); + const buildId = createHash('md5').update(hashedAssets.join('|')).digest('hex').slice(0, 12); + // Rewrite the two declarations. Anchored on the full `const … = …;` text so + // each pattern occurs exactly once and cannot collide with prose in sw.js's + // own comments — an earlier cut used bare `__BUILD_ID__` sentinels and the + // first match landed in the comment that documented them, leaving the real + // constant untouched and still producing a plausible-looking cache key. + const swEdits = [ + ["const BUILD_ID = 'dev';", `const BUILD_ID = '${buildId}';`], + ['const HASHED_ASSETS = [];', `const HASHED_ASSETS = [${hashedAssets.map((p) => JSON.stringify(p)).join(', ')}];`], + ]; + for (const [from, to] of swEdits) { + const hits = sw.split(from).length - 1; + if (hits !== 1) { + throw new Error(`sw.js: expected exactly one \`${from}\`, found ${hits} — precache would ship stale`); + } + sw = sw.replace(from, to); + } + writeFileSync(swPath, sw); + console.log(' Hashed files:'); for (const [orig, hashed] of Object.entries(manifest)) { console.log(` ${orig} -> ${hashed}`); } + console.log(` sw.js: cache bucket codeman-${buildId}, ${hashedAssets.length} precached assets`); } // 6. Compress with gzip + brotli diff --git a/src/web/public/app.js b/src/web/public/app.js index 4c688899..4f1f3405 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -66,7 +66,18 @@ const _crashDiag = { // concurrent clients (desktop + phone) don't clobber each other. _pageId: Date.now().toString(36) + '-' + Math.random().toString(36).slice(2, 8), log(msg) { - const entry = `${new Date().toISOString().slice(11,23)} ${msg}`; + // Entries are joined with '\n' into ONE localStorage value and beaconed to + // the server, and some call sites interpolate text this client does not + // control (a WebSocket close `reason` arrives from the server). A newline + // in there forges extra entries in the trail; an unbounded string can fill + // the storage quota and silently kill every later breadcrumb. Flatten and + // cap. CodemanDiag is loaded before app.js, but guard anyway — a + // diagnostic that can throw is worse than no diagnostic. + const flat = + typeof CodemanDiag !== 'undefined' && CodemanDiag.sanitizeDiagEntry + ? CodemanDiag.sanitizeDiagEntry(msg) + : String(msg == null ? '' : msg).replace(/[\r\n\u2028\u2029]+/g, ' ').slice(0, 300); + const entry = `${new Date().toISOString().slice(11,23)} ${flat}`; this._entries.push(entry); if (this._entries.length > this._maxEntries) this._entries.shift(); try { localStorage.setItem('codeman-crash-diag', this._entries.join('\n')); } catch {} @@ -675,6 +686,10 @@ class CodemanApp { this._wsReady = false; // True when WS is open and ready for I/O this._wsState = 'disconnected'; // connecting | connected | reconnecting | fallback | disconnected this._wsLastRecvAt = 0; // ms timestamp of the last frame received on the active WS + // Session whose socket dropped unintentionally, so output produced during + // the outage is missing from its buffer. Output frames carry no sequence + // number, so the only recovery is to refetch on the next successful open. + this._wsOutputGapSession = null; // Terminal write batching with DEC 2026 sync support this.pendingWrites = []; @@ -2475,6 +2490,81 @@ class CodemanApp { } } + /** + * Fetch a terminal capture under a deadline. + * + * Every terminal fetch used to run with no timeout at all, including + * `?full=1`, which _maybeRefetchFullHistory itself calls "unbounded-ish work: + * at the default history limit it can be megabytes". On a stalled mobile link + * that request hangs on the browser default with no retry, and the load-state + * machinery stays armed behind it. + * + * The budget scales with what is being asked for and with how many captures + * are already running (see CodemanFetchDeadline): a full scrollback on a slow + * uplink legitimately needs longer than a tail, and eight tabs resuming must + * not all expire together because each assumed it had the link to itself. + * + * An abort surfaces as a rejected promise, which every caller already handles — + * they wrap these in try/catch and log. That is the point: a timeout becomes a + * recoverable error instead of an indefinite hang. + * + * ⚠️ **The body is read HERE, and that is the whole point.** `await fetch()` + * settles on response HEADERS, not the body, so clearing the deadline when it + * resolves leaves the body — the multi-megabyte `?full=1` capture this exists + * for — completely unbounded. Measured against a server that sends headers + * immediately and stalls the body: `fetch()` resolved at 30ms, the timer was + * cleared there, and the body completed at 4026ms unaborted under a 1000ms + * deadline. Reading the body inside the helper is what makes the deadline + * cover the transfer rather than just the handshake. `_terminalCaptureInflight` + * is scoped the same way, so a body still streaming counts toward the budget + * of a capture starting beside it. + * + * Returns the PARSED envelope plus the response headers, because two callers + * read `server-timing`, and `headersAt` because those same callers measure + * header-vs-body time and can no longer observe that moment themselves. + * + * @param {string} url + * @param {{full?: boolean}} [opts] + * @returns {Promise<{json: unknown, headers: Headers|undefined, headersAt: number}>} + */ + async _fetchTerminalCapture(url, opts = {}) { + const deadlineMs = + typeof CodemanFetchDeadline !== 'undefined' + ? CodemanFetchDeadline.terminalFetchDeadlineMs({ + full: !!opts.full, + inflight: this._terminalCaptureInflight || 0, + }) + : 45000; + // AbortSignal.timeout() is not on every browser Codeman supports, so drive + // it from a controller and always clear the timer — an uncancelled one + // would abort a LATER request that reused this controller's signal. + // + // Degrade to a plain fetch where AbortController is missing rather than + // throwing: a capture with no deadline is the behaviour every caller had + // before this helper existed, while a ReferenceError here would take out + // terminal replay entirely. The deadline is a safety net, not a dependency. + const canAbort = typeof AbortController === 'function'; + const controller = canAbort ? new AbortController() : null; + const timer = controller ? setTimeout(() => controller.abort(), deadlineMs) : null; + this._terminalCaptureInflight = (this._terminalCaptureInflight || 0) + 1; + try { + const res = await (controller ? fetch(url, { signal: controller.signal }) : fetch(url)); + const headersAt = performance.now(); + // Still inside the deadline: an abort here rejects the body stream, which + // is exactly the case a header-only timeout could not reach. + const json = await res.json(); + return { json, headers: res.headers, headersAt }; + } catch (err) { + if (err?.name === 'AbortError') { + _crashDiag.log(`TERMINAL FETCH TIMEOUT after ${deadlineMs}ms`); + } + throw err; + } finally { + if (timer !== null) clearTimeout(timer); + this._terminalCaptureInflight = Math.max(0, (this._terminalCaptureInflight || 1) - 1); + } + } + async _onSessionNeedsRefresh(event = {}) { // Server sends this after SSE backpressure clears — terminal data was dropped, // so reload the buffer to recover from any display corruption. @@ -2493,15 +2583,16 @@ class CodemanApp { // TUI modes still recover the whole picture, with the downgrade guard for // repaint-mode panes whose tmux capture can be smaller than xterm's buffer. const useFullHistory = this.sessions.get(sessionId)?.mode !== 'shell'; - let res = await fetch( + let capture = await this._fetchTerminalCapture( useFullHistory ? `/api/sessions/${sessionId}/terminal?full=1` - : `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}` + : `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`, + { full: useFullHistory } ); - let data = (await res.json())?.data ?? {}; + let data = capture.json?.data ?? {}; if (useFullHistory && data.terminalBuffer && this._replayWouldShrinkBuffer(data.terminalBuffer)) { - res = await fetch(`/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`); - data = (await res.json())?.data ?? {}; + capture = await this._fetchTerminalCapture(`/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`); + data = capture.json?.data ?? {}; } // Bail on a tab switch mid-fetch: writing here would paint this session's // history into the terminal the user is now looking at. The window is two @@ -2514,8 +2605,11 @@ class CodemanApp { // meaningless across it — distance from the bottom is what survives. const before = this.terminal.buffer?.active; const linesFromBottom = before ? Math.max(0, (before.baseY || 0) - (before.viewportY || 0)) : 0; - this.terminal.clear(); - this.terminal.reset(); + // One queued clear, not clear()+reset(): both of those are synchronous + // and skip xterm's write queue, so live bytes still parsing would land + // after them and fuse into the buffer written below. See + // _resetTerminalForReplay. + this._resetTerminalForReplay(); await this.chunkedTerminalWrite(data.terminalBuffer); // A tail fetch can be partial, and the banner would otherwise keep // describing the pre-refresh buffer (#258). @@ -2538,9 +2632,25 @@ class CodemanApp { console.error('needsRefresh reload failed:', err); } finally { if (this._terminalRefreshOwner === refreshOwner) this._terminalRefreshOwner = null; + // Any completed reload for this session IS the reconcile, whoever asked + // for it — handleInit's SSE-reconnect branch and selectSession both land + // here or do the same work. Leaving the marker set would make the next + // ws.onopen replay the whole buffer a second time. + this._markTerminalBufferReconciled(sessionId); } } + /** + * Drop the "this session lost output" marker. + * + * Called from every path that repaints a session's buffer from the server, so + * the ws.onopen reconcile fires once and only when nothing else already did + * the work. See the ws.onclose note for what the marker means. + */ + _markTerminalBufferReconciled(sessionId) { + if (sessionId && this._wsOutputGapSession === sessionId) this._wsOutputGapSession = null; + } + async _onSessionClearTerminal(data) { if (data.id === this.activeSessionId) { // Skip if selectSession is already loading the buffer — clearTerminal arriving @@ -2551,11 +2661,12 @@ class CodemanApp { // Fetch buffer, clear terminal, write buffer, resize (no Ctrl+L needed) try { - const res = await fetch(`/api/sessions/${data.id}/terminal`); - const termData = (await res.json())?.data ?? {}; + const capture = await this._fetchTerminalCapture(`/api/sessions/${data.id}/terminal`); + const termData = capture.json?.data ?? {}; - this.terminal.clear(); - this.terminal.reset(); + // Queued clear — see _resetTerminalForReplay for why clear()+reset() + // cannot do this job. + this._resetTerminalForReplay(); if (termData.terminalBuffer) { // Strip any DEC 2026 markers and write raw content // (markers don't help here - this is a static buffer reload, not live Ink redraws) @@ -2875,6 +2986,18 @@ class CodemanApp { // Flush any durably-queued input over the fresh socket (covers frames a // prior half-open socket silently dropped, and input typed while offline). this._onWsReady(sessionId); + // Reconcile the output hole this drop left (see the ws.onclose note). + // Only after an unintentional close — a first connect has no gap, and + // refetching there would duplicate the buffer selectSession just wrote. + if (this._wsOutputGapSession === sessionId) { + this._wsOutputGapSession = null; + _crashDiag.log(`WS REOPEN: reconciling output gap for ${sessionId}`); + // Fire-and-forget: this is recovery, and a failure here must not stop + // the socket coming up. _onSessionNeedsRefresh already guards against + // running while a buffer load is in flight and against a tab switch + // landing this session's history in another session's terminal. + void this._onSessionNeedsRefresh({ id: sessionId }); + } } }; @@ -2922,6 +3045,30 @@ class CodemanApp { `WS CLOSE code=${event.code} reason=${event.reason || ''} action=${plan.action} attempts=${this._wsReconnectAttempts || 0}` ); + // Output frames carry no sequence number, so a dropped socket leaves a + // hole with nothing to replay it. ws.onopen re-sends dims and flushes + // queued INPUT; `needsRefresh` fires only on external-CLI startup and on + // SSE backpressure drain, never here. + // + // ⚠️ The gap this closes is NARROWER than "the device went offline". If + // the network drops, SSE drops with it and `handleInit`'s keepTerminal + // branch already reconciles on reconnect. The uncovered case is the WS + // dying while SSE stays up — a half-open socket, a proxy idle-timeout, + // a ping timeout — because `_onSSETerminal` discards every SSE terminal + // frame while `_wsReady` is true, and `_wsReady` only flips here, in + // onclose. Detecting a half-open socket takes up to the ping+pong window, + // and that whole span produces output nothing writes to the terminal. + // + // Reaching onclose at all means the drop was NOT intentional + // (_disconnectWs nulls this handler first), so mark the gap and let the + // next successful open reconcile from the server's buffer. + // + // Scoped to the session that actually lost bytes: a user who switches + // sessions during an outage gets a clean intentional disconnect for the + // new one, and its freshly-loaded buffer must not be refetched because a + // DIFFERENT session's socket dropped. + this._wsOutputGapSession = sessionId; + const stillActive = this.activeSessionId === sessionId; if (plan.action === 'give-up') { this._wsState = stillActive ? 'fallback' : 'disconnected'; @@ -5745,9 +5892,29 @@ class CodemanApp { } } + /** + * Clear the terminal for a replay, IN STREAM. + * + * xterm's `write()` is asynchronously queued (the WriteBuffer parses in ~12ms + * slices) while `Terminal.reset()` is synchronous and, by upstream's own + * documentation, "does not clear input buffers and does not reset the parser, + * thus the terminal will continue to apply pending input data". So bytes + * queued just before a `reset()` are parsed AFTER it and fuse into whatever + * snapshot is written next — measured upstream as `p8rmissions` rendered + * where `bypass permissions` belonged. + * + * A queued clear cannot race that way: it lands after the leftovers and + * before the snapshot, whatever the queue held. This function used to follow + * the sync `reset()` with a queued `\x1b[3J\x1b[H\x1b[2J`, which already got + * that right for CONTENT. RIS (`\x1bc`) additionally resets modes, charsets, + * scroll regions and SGR state, so leftover bytes cannot park the terminal in + * alt-screen or an odd scroll region and survive the clear. + * + * Callers may write the replacement content in as many chunks as they like — + * ordering within the queue is what matters, not writing it all at once. + */ _resetTerminalForReplay() { - this.terminal.reset(); - this.terminal.write('\x1b[3J\x1b[H\x1b[2J'); + this.terminal.write('\x1bc'); } _recordTerminalLoadTiming(timing) { @@ -5811,9 +5978,9 @@ class CodemanApp { this._fullHistoryRepullInFlight = true; try { const requestStartedAt = performance.now(); - const res = await fetch(`/api/sessions/${sessionId}/terminal?full=1`); - const headersReceivedAt = performance.now(); - const payload = (await res.json())?.data ?? {}; + const capture = await this._fetchTerminalCapture(`/api/sessions/${sessionId}/terminal?full=1`, { full: true }); + const headersReceivedAt = capture.headersAt; + const payload = capture.json?.data ?? {}; const bodyParsedAt = performance.now(); const buffer = payload.terminalBuffer; const timing = { @@ -5826,7 +5993,7 @@ class CodemanApp { bodyAndJsonMs: bodyParsedAt - headersReceivedAt, resetAndParseMs: 0, totalMs: 0, - serverTiming: res.headers?.get?.('server-timing') || '', + serverTiming: capture.headers?.get?.('server-timing') || '', refused: false, }; // Bail on a tab switch mid-fetch: writing here would paint another session's @@ -6299,17 +6466,18 @@ class CodemanApp { const useFullHistory = session?.mode !== 'shell' && !this._fullHistoryLoaded.has(sessionId); if (useFullHistory) this._fullHistoryLoaded.add(sessionId); const fetchStartedAt = performance.now(); - const res = await fetch( + const capture = await this._fetchTerminalCapture( useFullHistory ? `/api/sessions/${sessionId}/terminal?full=1` - : `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}` + : `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`, + { full: useFullHistory } ); - const headersReceivedAt = performance.now(); + const headersReceivedAt = capture.headersAt; if (this._isStaleSelect(selectGen)) { this._clearTerminalLoadState(sessionId, selectGen); return; } - const data = (await res.json())?.data ?? {}; + const data = capture.json?.data ?? {}; const bodyParsedAt = performance.now(); _crashDiag.log(`FETCH_DONE: ${data.terminalBuffer ? (data.terminalBuffer.length/1024).toFixed(0) + 'KB' : 'empty'} truncated=${data.truncated}`); @@ -6379,7 +6547,7 @@ class CodemanApp { cacheResetAndParseMs, freshResetAndParseMs, selectToReplayCompleteMs: performance.now() - _selStart, - serverTiming: res.headers?.get?.('server-timing') || '', + serverTiming: capture.headers?.get?.('server-timing') || '', }; // Buffer load complete — unblock live SSE writes. chunkedTerminalWrite calls // _finishBufferLoad after ordering the fetched snapshot in xterm; if we skipped @@ -6389,6 +6557,11 @@ class CodemanApp { if (this._isLoadingBuffer) { this._finishBufferLoad(bufferLoadOwner, { flushQueued: bufferWasEmpty }); } + // This load repainted the session from the server, so any pending + // output-gap marker is already satisfied. Selecting a session runs BEFORE + // _connectWs, so without this the socket opening afterwards would replay + // the whole buffer again on top of the one just written. + this._markTerminalBufferReconciled(sessionId); // Drop the guard so user input clears state normally this._restoringFlushedState = false; @@ -6529,6 +6702,9 @@ class CodemanApp { // Shared cleanup for all session data — called from both closeSession() and session:deleted handler _cleanupSessionData(sessionId) { this.closeTabRailActionMenu?.(); + // A dead session has no buffer to reconcile; leaving the marker set would + // make a later socket for a REUSED id reconcile against nothing. + this._markTerminalBufferReconciled(sessionId); // If the deleted session is currently being renamed, abort the rename // so the inline doesn't ghost as a stale tab on screen. if (this._activeRename?.sessionId === sessionId) { diff --git a/src/web/public/constants.js b/src/web/public/constants.js index be9780b0..3ca58bfc 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -1442,8 +1442,129 @@ function terminalLogicalLine(buffer, row, cols, maxRows) { return { startRow, endRow, text, offsetToCell, cellToOffset }; } +// ── Renderer liveness ────────────────────────────────────────────────────── +// +// iOS DISCARDS scheduled requestAnimationFrame callbacks when a PWA goes to +// the background — not deferred, never delivered. xterm's RenderDebouncer only +// clears its `_animationFrame` handle from INSIDE that callback: +// +// refresh() { +// if (this._animationFrame !== undefined) return; // <- stale forever +// this._animationFrame = requestAnimationFrame(() => this._innerRefresh()); +// } +// _innerRefresh() { this._animationFrame = undefined; ... } // never runs +// +// So after one backgrounding the handle is permanently non-undefined and EVERY +// later render request returns on line one. Parsing is decoupled from +// rendering, so bytes keep filling the buffer correctly and nothing throws — +// the terminal is simply frozen. Closing and reopening fixes it because that +// constructs a new Terminal, and therefore a new debouncer. +// +// Codeman is MORE exposed than a per-session-terminal app: there is exactly one +// xterm instance for the whole page load, so a single backgrounding can wedge +// it until a full reload. +// +// This is the pure decision half. The signature that distinguishes this from +// every other way a terminal can look stuck is that bytes were WRITTEN and the +// element is VISIBLE, yet onRender has not fired since: +// +// frozen = wroteAt > renderedAt && now - wroteAt >= threshold && visible +// +// Deliberately NOT a "no output at all" check: a quiet terminal is the normal +// state and must never be kicked. And `visible` is required because a hidden +// terminal legitimately stops rendering (xterm pauses it), so kicking there +// would fire constantly on every backgrounded tab. +const RENDER_STALL_MS = 4000; + +// How often the watchdog checks. Deliberately coarse: the failure it catches is +// permanent until healed, so detecting it a second late costs nothing, while a +// tight interval would burn a wakeup per second on every idle phone. +const RENDER_LIVENESS_POLL_MS = 2000; + +/** + * Should the renderer be kicked? Pure so the CI gate can cover it — the DOM + * half (cancelling the stale handle) lives in terminal-ui.js. + * + * @param {{wroteAt:number, renderedAt:number, now:number, visible:boolean, + * thresholdMs?:number}} s + * @returns {boolean} + */ +function shouldKickRenderer(s) { + if (!s || !s.visible) return false; + const wroteAt = Number(s.wroteAt) || 0; + const renderedAt = Number(s.renderedAt) || 0; + const now = Number(s.now) || 0; + // Nothing written yet — a fresh terminal has no render to be missing. + if (wroteAt <= 0) return false; + // A render landed at or after the last write: the pipeline is alive. + if (renderedAt >= wroteAt) return false; + const threshold = Number.isFinite(s.thresholdMs) && s.thresholdMs > 0 ? s.thresholdMs : RENDER_STALL_MS; + return now - wroteAt >= threshold; +} + +// ── Fetch deadlines ──────────────────────────────────────────────────────── +// +// No terminal fetch carried any deadline, including `?full=1`, which the code +// itself describes as "unbounded-ish work: at the default history limit it can +// be megabytes". On a stalled mobile link that request hangs on the browser +// default with no retry and no path back to a usable terminal short of a +// reload. +// +// A single fixed timeout is wrong in both directions — too short for a full +// scrollback capture on a slow uplink, too long for a small tail on a dead +// connection. So the deadline is scaled by what is actually being asked for, +// and by how many captures are already in flight: on a slow link those bytes +// must drain before this request's own bytes start moving, and its timer is +// already running the whole time. +const FETCH_DEADLINE_TAIL_MS = 15000; +const FETCH_DEADLINE_FULL_MS = 45000; +const FETCH_DEADLINE_MAX_MS = 120000; + +/** + * Deadline in ms for a terminal capture. + * + * @param {{full?:boolean, inflight?:number}} s - `full` = the ?full=1 capture; + * `inflight` = captures already running (this one included or not, it only + * scales the budget). + * @returns {number} + */ +function terminalFetchDeadlineMs(s) { + const full = !!(s && s.full); + const base = full ? FETCH_DEADLINE_FULL_MS : FETCH_DEADLINE_TAIL_MS; + const inflight = Math.max(0, Number(s && s.inflight) || 0); + // Each already-queued capture gets the newcomer one more base budget to wait + // through. Linear rather than clever: the point is only that eight tabs + // resuming do not all time out together because each assumed it was alone. + return Math.min(FETCH_DEADLINE_MAX_MS, base * (1 + inflight)); +} + +// ── Diagnostics hygiene ──────────────────────────────────────────────────── +// +// The crash trail is joined with '\n' into ONE localStorage value and beaconed +// to the server, and at least one call site interpolates server-controlled text +// (a WebSocket close `reason`). An embedded newline there forges extra entries +// in the trail; an unbounded string can fill the storage quota. Both are cheap +// to close, and the trail is something a user may be asked to paste into an +// issue. +const DIAG_ENTRY_MAX_CHARS = 300; + +/** Flatten a diagnostic message to one bounded, newline-free line. */ +function sanitizeDiagEntry(msg) { + return String(msg == null ? '' : msg) + .replace(/[\r\n\u2028\u2029]+/g, ' ') + .slice(0, DIAG_ENTRY_MAX_CHARS); +} + if (typeof window !== 'undefined') { window.CodemanHistoryFormat = { formatHistoryBytes, computeHistoryTruncationNotice, computeRewriteScrollLine }; window.CodemanFilePaths = { absoluteFilePathPattern, previewsInFileViewer, FILE_PREVIEW_EXTENSIONS }; window.CodemanTerminalLines = { terminalLogicalLine }; + window.CodemanRenderLiveness = { shouldKickRenderer, RENDER_STALL_MS, RENDER_LIVENESS_POLL_MS }; + window.CodemanFetchDeadline = { + terminalFetchDeadlineMs, + FETCH_DEADLINE_TAIL_MS, + FETCH_DEADLINE_FULL_MS, + FETCH_DEADLINE_MAX_MS, + }; + window.CodemanDiag = { sanitizeDiagEntry, DIAG_ENTRY_MAX_CHARS }; } diff --git a/src/web/public/sw.js b/src/web/public/sw.js index efe17981..966d302c 100644 --- a/src/web/public/sw.js +++ b/src/web/public/sw.js @@ -18,7 +18,16 @@ * @see src/push-store.ts -- server-side VAPID key management and subscription CRUD */ -const CACHE_NAME = 'codeman-v1'; +// Build identity. scripts/build.mjs rewrites this declaration after it content- +// hashes the assets; the literal below is what dev serves, and dev wants a +// stable key. +// +// Why the cache key MUST carry it: `activate` deletes every cache whose key is +// not the current one, so the old constant key meant that cleanup never deleted +// anything — hashed assets from every release ever deployed accumulated in one +// bucket until the origin hit its storage quota. +const BUILD_ID = 'dev'; +const CACHE_NAME = `codeman-${BUILD_ID}`; // Reverse-proxy base path: the worker is served at `/sw.js`, so its own // location tells us the mount prefix ('' at root, or '/codeman'). Every URL below @@ -27,27 +36,27 @@ const CACHE_NAME = 'codeman-v1'; const SW_BASE = self.location.pathname.replace(/\/sw\.js$/, ''); const B = (p) => (p && p[0] === '/' ? SW_BASE + p : p); +// Content-hashed assets. scripts/build.mjs rewrites this declaration with the +// filenames it actually emitted; dev has no hashing, so the empty literal below +// is correct there and the unhashed modules are simply cached on first use by +// the runtime handler further down. +// +// This list used to be maintained by hand with the PRE-hash names, which the +// build then renamed — so in production every entry 404'd and the silent +// `.catch()` in install swallowed all of it. Measured against a running +// instance: 15 of 23 entries failed. Offline still worked, because the fetch +// handler caches every successful GET at runtime, but the precache warmed +// nothing while looking like it did. Deriving it from the same manifest that +// renames the files is the only thing that keeps the two from drifting again. +const HASHED_ASSETS = []; + // Core app shell -- cached on install for instant startup const APP_SHELL = [ '/', - '/styles.css', - '/mobile.css', - '/constants.js', - '/app.js', - '/api-client.js', - '/terminal-ui.js', - '/session-ui.js', - '/settings-ui.js', - '/panels-ui.js', - '/notification-manager.js', - '/mobile-handlers.js', - '/keyboard-accessory.js', - '/voice-input.js', + ...HASHED_ASSETS.map((p) => '/' + p), '/vendor/xterm.min.js', '/vendor/xterm-addon-fit.min.js', '/vendor/xterm-addon-unicode11.min.js', - '/vendor/xterm-zerolag-input.js', - '/vendor/xterm-predictive-echo.js', '/vendor/xterm.css', '/icon-192.png', '/icon-512.png', @@ -102,7 +111,17 @@ self.addEventListener('fetch', (event) => { } return response; }) - .catch(() => caches.match(request)) + // ignoreSearch, or the precache can never be hit. `renderIndexHtml` runs + // `cacheBustAssets`, which appends `?v=` to EVERY same-origin + // `.js`/`.css` reference — content-hashed names included, so the page asks + // for `/app.556be563.js?v=1789423735875` while the precache stored + // `/app.556be563.js`. `caches.match` is query-sensitive by default, so + // every precached entry was unreachable and only `/`, the icons and the + // manifest could ever be served offline. + // + // It also makes runtime-cached entries survive an mtime change: the same + // file re-requested under a new `?v=` still matches the copy already held. + .catch(() => caches.match(request, { ignoreSearch: true })) ); }); diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 7ecc994c..a366be0f 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -528,6 +528,14 @@ Object.assign(CodemanApp.prototype, { this.terminal.onRender(() => this._syncMobileHelperTextareaToCursor()); } + // Renderer liveness — see _startRenderLivenessWatchdog. Registered for every + // device, not just touch: the rAF-discard behaviour is worst on an iOS PWA + // but a stale handle wedges the debouncer identically anywhere it happens. + this.terminal.onRender(() => { + this._lastRenderAt = Date.now(); + }); + this._startRenderLivenessWatchdog(); + // CJK IME input — textarea in index.html, just wire up send this._cjkInput = null; if (typeof CjkInput !== 'undefined') { @@ -3290,7 +3298,105 @@ Object.assign(CodemanApp.prototype, { return performance.now() - this._lastUserScrollUpAt < window.CodemanTerminalInput.USER_SCROLL_STICKY_SUPPRESS_MS; }, + /** + * Watchdog for a frozen renderer. + * + * iOS DISCARDS scheduled requestAnimationFrame callbacks when a PWA goes to + * the background — not deferred, never delivered. xterm's RenderDebouncer + * only clears its `_animationFrame` handle from INSIDE that callback, so once + * one is dropped the handle stays permanently non-undefined and every later + * `refresh()` returns on its first line. Parsing is decoupled from rendering, + * so bytes keep filling the buffer correctly and nothing throws: the terminal + * is simply frozen until the page is reloaded. + * + * Codeman is more exposed than an app that mounts a terminal per session — + * there is exactly ONE xterm instance for the whole page load, so a single + * backgrounding can wedge it for the rest of the session. + * + * The heal is what `_innerRefresh` would have done: cancel the stale handle, + * clear the field, and request a full repaint (which schedules a fresh rAF). + * Cancelling a genuinely pending handle is harmless — the full repaint that + * follows covers whatever it was going to draw. + * + * Discipline for reaching into xterm privates, and it is not optional: every + * access is optional-chained and the whole body is wrapped, so a shape change + * upstream degrades to a no-op. A self-heal that can break the terminal it is + * healing is worse than no self-heal. + * + * ⚠️ The field path (`_core._renderService._renderDebouncer._animationFrame`) + * is validated against xterm 6.x and CANNOT be covered by the CI gate: + * `_renderService` is only constructed by `Terminal.open()`, which needs a + * real DOM, and the gate runs in node. `test/xterm-private-api.test.ts` pins + * the dependency RANGE instead, so a major bump fails there and sends someone + * to re-check this by hand; `test/terminal-resilience.test.ts` covers the + * decision half. If the path ever goes stale the watchdog silently stops + * healing — that is the failure mode to watch for, and why the range guard + * exists at all. + */ + _startRenderLivenessWatchdog() { + this._stopRenderLivenessWatchdog(); + this._lastRenderAt = Date.now(); + this._lastTerminalWriteAt = 0; + this._renderLivenessTimer = setInterval(() => { + try { + if (typeof CodemanRenderLiveness === 'undefined') return; + const kick = CodemanRenderLiveness.shouldKickRenderer({ + wroteAt: this._lastTerminalWriteAt || 0, + renderedAt: this._lastRenderAt || 0, + now: Date.now(), + // A hidden terminal legitimately stops rendering (xterm pauses it), + // so only a VISIBLE one that owes us a frame counts as frozen. + visible: document.visibilityState === 'visible' && !!this.terminal?.element?.isConnected, + }); + if (!kick) return; + const kicked = this._kickRenderer(); + _crashDiag.log(`RENDER STALL: kick=${kicked}`); + // Treat the kick as the render for accounting purposes either way, so a + // terminal we cannot heal logs once per stall rather than every tick. + this._lastRenderAt = Date.now(); + } catch { + /* a watchdog must never throw into the interval */ + } + }, RENDER_LIVENESS_POLL_MS); + }, + + _stopRenderLivenessWatchdog() { + if (this._renderLivenessTimer) { + clearInterval(this._renderLivenessTimer); + this._renderLivenessTimer = null; + } + }, + + /** + * Do what xterm's dropped `_innerRefresh` would have done. Never throws. + * @returns {boolean} true if a stale handle was found and cleared. + */ + _kickRenderer() { + try { + const renderService = this.terminal?._core?._renderService; + const debouncer = renderService?._renderDebouncer; + if (!debouncer || typeof renderService.refreshRows !== 'function') return false; + const handle = debouncer._animationFrame; + if (handle === undefined) return false; // not wedged — nothing to clear + try { + cancelAnimationFrame(handle); + } catch { + /* a stale handle may no longer be cancellable; clearing it is the point */ + } + debouncer._animationFrame = undefined; + renderService.refreshRows(0, Math.max(0, (this.terminal.rows || 1) - 1)); + return true; + } catch { + return false; + } + }, + batchTerminalWrite(data) { + // Feed the renderer watchdog. Recorded before the buffer-load early return + // below: a write that is queued rather than written still means the pipeline + // owes us a frame once it drains. + this._lastTerminalWriteAt = Date.now(); + // If a buffer load (chunkedTerminalWrite) is in progress, queue live events // to prevent interleaving historical buffer data with live SSE data. // This is critical: interleaving causes cursor position chaos with Ink redraws. diff --git a/test/history-truncation-notice.test.ts b/test/history-truncation-notice.test.ts index ef542eef..b24fe3b6 100644 --- a/test/history-truncation-notice.test.ts +++ b/test/history-truncation-notice.test.ts @@ -127,7 +127,11 @@ describe('the in-terminal truncation line is gone (static guard)', () => { expect(app).toContain("session?.mode !== 'shell' && !this._fullHistoryLoaded.has(sessionId)"); expect(app).toContain("!restoredSnapshot && session?.mode !== 'shell'"); expect(app).toContain('`/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`'); - expect(app).toContain('fetch(`/api/sessions/${sessionId}/terminal?full=1`)'); + // Every terminal capture now goes through _fetchTerminalCapture, which adds + // an abort deadline (a `?full=1` body can be megabytes and used to hang + // indefinitely on a stalled mobile link). The URL and the full-vs-tail + // decision this guard exists to pin are unchanged. + expect(app).toContain('this._fetchTerminalCapture(`/api/sessions/${sessionId}/terminal?full=1`, { full: true })'); expect(app).toContain("if (this.sessions.get(sessionId)?.mode !== 'shell')"); expect(app).toContain("if (session?.mode === 'shell')"); expect(app).toContain("if (!force && session?.mode === 'shell') return;"); diff --git a/test/sw-precache-manifest.test.ts b/test/sw-precache-manifest.test.ts new file mode 100644 index 00000000..09d1e31b --- /dev/null +++ b/test/sw-precache-manifest.test.ts @@ -0,0 +1,100 @@ +// Port: none (static source contract — no browser, no server). +// +// The service worker's precache list used to be maintained by hand with the +// PRE-hash filenames, while scripts/build.mjs renamed those same files to +// content-hashed names and rewrote only index.html. So in production every +// precache entry pointed at a file that no longer existed, and +// `cache.add(url).catch(() => {})` in the install handler swallowed all of it. +// Measured against a running instance: 15 of 23 entries 404'd. +// +// Nothing caught it because nothing could: the two lists lived in different +// files, in different languages, with no shared symbol. The fix is to derive +// the list from the build's own manifest — and this test pins the contract that +// makes that derivation possible, because the failure mode is silent in both +// directions. A renamed anchor in sw.js means the build throws (loud, fine). A +// build that stops rewriting means the worker precaches nothing while still +// looking correct (silent, not fine). +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +const root = resolve(import.meta.dirname, '..'); +const sw = readFileSync(resolve(root, 'src/web/public/sw.js'), 'utf8'); +const build = readFileSync(resolve(root, 'scripts/build.mjs'), 'utf8'); + +// The exact declarations scripts/build.mjs rewrites. They must appear EXACTLY +// once: the build asserts the same thing and throws otherwise, so a second +// occurrence (in a comment, say) fails the build rather than shipping stale. +const BUILD_ID_ANCHOR = "const BUILD_ID = 'dev';"; +const HASHED_ASSETS_ANCHOR = 'const HASHED_ASSETS = [];'; + +describe('service worker precache contract', () => { + it('sw.js carries exactly one of each anchor the build rewrites', () => { + expect(sw.split(BUILD_ID_ANCHOR).length - 1).toBe(1); + expect(sw.split(HASHED_ASSETS_ANCHOR).length - 1).toBe(1); + }); + + it('build.mjs rewrites those exact anchors', () => { + expect(build).toContain(BUILD_ID_ANCHOR); + expect(build).toContain(HASHED_ASSETS_ANCHOR); + }); + + // The cache key must vary per build, or `activate`'s cleanup — which deletes + // every cache whose key is not the current one — never deletes anything, and + // hashed assets from every past release accumulate until the origin hits its + // storage quota. That is what the old constant 'codeman-v1' did. + it('derives the cache name from the build id rather than a constant', () => { + expect(sw).toContain('const CACHE_NAME = `codeman-${BUILD_ID}`;'); + expect(sw).not.toMatch(/const CACHE_NAME = ['"]codeman-v\d+['"]/); + }); + + // The whole point of the rewrite: the shell is derived, not hand-listed. + it('builds the app shell from the hashed manifest', () => { + expect(sw).toContain("...HASHED_ASSETS.map((p) => '/' + p)"); + }); + + // The regression itself: any pre-hash filename hand-listed in APP_SHELL will + // 404 in production, because the build renames it. + // + // The HASHABLE list is PARSED out of scripts/build.mjs rather than copied + // here. A hand-kept duplicate would be the same drift this whole PR exists to + // fix — it would go stale the first time someone adds an asset to the build, + // and then silently stop covering it. + it('never hand-lists a filename the build content-hashes', () => { + const block = build.slice( + build.indexOf('const HASHABLE = ['), + build.indexOf('];', build.indexOf('const HASHABLE = [')) + ); + const hashedByBuild = [...block.matchAll(/'([^']+)'/g)].map((m) => m[1]); + // Guard the parse itself: an empty list would make this test vacuously pass. + expect(hashedByBuild.length, 'failed to parse HASHABLE out of scripts/build.mjs').toBeGreaterThan(10); + expect(hashedByBuild).toContain('app.js'); + + const shell = sw.slice(sw.indexOf('const APP_SHELL'), sw.indexOf('].map(B);')); + for (const name of hashedByBuild) { + expect(shell, `APP_SHELL must not hand-list ${name} — the build renames it`).not.toContain(`'/${name}'`); + } + }); + + // Dev serves sw.js unrewritten, so the literals must be valid on their own: + // an empty precache plus the unhashed modules cached on first use. + it('is valid unrewritten, for dev', () => { + expect(() => new Function(sw.replace(/self\./g, 'globalThis.'))).not.toThrow(); + }); + + // Without ignoreSearch the whole precache is unreachable, which is subtle + // enough to be re-broken by anyone tidying this handler. + // + // `renderIndexHtml` runs `cacheBustAssets`, which appends `?v=` to + // EVERY same-origin `.js`/`.css` reference — content-hashed names included. + // Observed on a running instance: `src="app.556be563.js?v=1789423735875"`. + // `caches.match` is query-sensitive by default, so a precache keyed on + // `/app.556be563.js` can never serve that request, and the install would be + // downloading ~1.3MB per deploy that nothing can ever read back. + it('falls back to the cache ignoring the cache-busting query string', () => { + expect(sw).toContain('caches.match(request, { ignoreSearch: true })'); + expect(sw, 'a bare caches.match(request) cannot match the ?v= URLs cacheBustAssets emits').not.toMatch( + /caches\.match\(request\)\s*\)/ + ); + }); +}); diff --git a/test/terminal-resilience.test.ts b/test/terminal-resilience.test.ts new file mode 100644 index 00000000..7dbee74c --- /dev/null +++ b/test/terminal-resilience.test.ts @@ -0,0 +1,246 @@ +// Port: none (pure helpers — no browser, no server). +// +// Three small decision functions behind the mobile terminal resilience work, +// pinned here because the code that consumes them lives in app.js / +// terminal-ui.js, which the CI gate cannot execute. Keeping the decision pure +// and the DOM half thin is what makes any of this testable without a browser. +// +// The renderer-liveness case is the one worth reading. iOS DISCARDS scheduled +// requestAnimationFrame callbacks when a PWA backgrounds — never delivered, not +// deferred — and xterm's RenderDebouncer only clears its `_animationFrame` +// handle from inside that callback. One drop leaves the handle permanently set, +// so every later refresh() returns immediately and the terminal freezes while +// its buffer keeps updating correctly. Codeman has exactly one xterm instance +// per page load, so a single backgrounding can wedge it until a reload. +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import vm from 'node:vm'; +import { createServer, type ServerResponse } from 'node:http'; +import type { AddressInfo } from 'node:net'; +import { describe, expect, it } from 'vitest'; + +function loadConstants() { + const context = vm.createContext({ window: {}, globalThis: {} }); + const source = readFileSync(resolve(import.meta.dirname, '../src/web/public/constants.js'), 'utf8'); + vm.runInContext(source, context, { filename: 'constants.js' }); + const w = context.window as { + CodemanRenderLiveness: { + shouldKickRenderer: (s: { + wroteAt: number; + renderedAt: number; + now: number; + visible: boolean; + thresholdMs?: number; + }) => boolean; + RENDER_STALL_MS: number; + RENDER_LIVENESS_POLL_MS: number; + }; + CodemanFetchDeadline: { + terminalFetchDeadlineMs: (s: { full?: boolean; inflight?: number }) => number; + FETCH_DEADLINE_TAIL_MS: number; + FETCH_DEADLINE_FULL_MS: number; + FETCH_DEADLINE_MAX_MS: number; + }; + CodemanDiag: { + sanitizeDiagEntry: (msg: unknown) => string; + DIAG_ENTRY_MAX_CHARS: number; + }; + }; + return w; +} + +describe('shouldKickRenderer', () => { + const { CodemanRenderLiveness } = loadConstants(); + const { shouldKickRenderer, RENDER_STALL_MS } = CodemanRenderLiveness; + + // The signature of the real failure: bytes were written, the element is + // visible, and no frame has been produced since. + it('kicks when a visible terminal owes a frame past the threshold', () => { + expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 500, now: 1000 + RENDER_STALL_MS, visible: true })).toBe( + true + ); + }); + + it('does not kick before the threshold elapses', () => { + expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 500, now: 1000 + RENDER_STALL_MS - 1, visible: true })).toBe( + false + ); + }); + + // A render at or after the last write means the pipeline is alive. This is + // the common case on every healthy terminal and must never kick. + it('does not kick when a render landed after the last write', () => { + expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 1000, now: 99_999, visible: true })).toBe(false); + expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 1200, now: 99_999, visible: true })).toBe(false); + }); + + // A hidden terminal legitimately stops rendering — xterm pauses it. Kicking + // there would fire on every backgrounded tab, forever. + it('never kicks a hidden terminal', () => { + expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 500, now: 99_999, visible: false })).toBe(false); + }); + + // A quiet terminal is the normal state, not a stalled one. Gating on "no + // render recently" instead of "owes a frame" would kick every idle session. + it('never kicks a terminal that has never been written to', () => { + expect(shouldKickRenderer({ wroteAt: 0, renderedAt: 0, now: 99_999, visible: true })).toBe(false); + }); + + it('tolerates missing and malformed input rather than throwing', () => { + expect(shouldKickRenderer(undefined as never)).toBe(false); + expect(shouldKickRenderer({} as never)).toBe(false); + expect(shouldKickRenderer({ wroteAt: NaN, renderedAt: NaN, now: NaN, visible: true } as never)).toBe(false); + }); + + it('polls coarsely enough not to wake an idle phone every second', () => { + expect(CodemanRenderLiveness.RENDER_LIVENESS_POLL_MS).toBeGreaterThanOrEqual(1000); + }); +}); + +describe('terminalFetchDeadlineMs', () => { + const { CodemanFetchDeadline } = loadConstants(); + const { terminalFetchDeadlineMs, FETCH_DEADLINE_TAIL_MS, FETCH_DEADLINE_FULL_MS, FETCH_DEADLINE_MAX_MS } = + CodemanFetchDeadline; + + // A full scrollback capture can be megabytes where a tail is one frame, so a + // single fixed timeout is wrong in both directions on a mobile link. + it('gives a full capture more budget than a tail', () => { + expect(terminalFetchDeadlineMs({ full: true })).toBeGreaterThan(terminalFetchDeadlineMs({ full: false })); + expect(terminalFetchDeadlineMs({ full: false })).toBe(FETCH_DEADLINE_TAIL_MS); + expect(terminalFetchDeadlineMs({ full: true })).toBe(FETCH_DEADLINE_FULL_MS); + }); + + // Eight tabs resuming must not all expire together because each assumed it + // had the link to itself. + it('scales with captures already in flight', () => { + const alone = terminalFetchDeadlineMs({ full: false, inflight: 0 }); + const queued = terminalFetchDeadlineMs({ full: false, inflight: 3 }); + expect(queued).toBeGreaterThan(alone); + }); + + it('is bounded — a stuck link still fails eventually', () => { + expect(terminalFetchDeadlineMs({ full: true, inflight: 1000 })).toBe(FETCH_DEADLINE_MAX_MS); + }); + + it('treats absent and nonsense input as a lone tail fetch', () => { + expect(terminalFetchDeadlineMs({})).toBe(FETCH_DEADLINE_TAIL_MS); + expect(terminalFetchDeadlineMs({ inflight: -5 } as never)).toBe(FETCH_DEADLINE_TAIL_MS); + expect(terminalFetchDeadlineMs({ inflight: NaN } as never)).toBe(FETCH_DEADLINE_TAIL_MS); + }); +}); + +describe('sanitizeDiagEntry', () => { + const { CodemanDiag } = loadConstants(); + const { sanitizeDiagEntry, DIAG_ENTRY_MAX_CHARS } = CodemanDiag; + + // The crash trail is joined with '\n' into one localStorage value and + // beaconed, and at least one call site interpolates a WebSocket close + // `reason`, which the server controls. A newline there forges entries. + it('collapses every newline form so an entry cannot forge another', () => { + expect(sanitizeDiagEntry('WS CLOSE reason=a\nFAKE ENTRY')).toBe('WS CLOSE reason=a FAKE ENTRY'); + expect(sanitizeDiagEntry('a\r\nb')).toBe('a b'); + expect(sanitizeDiagEntry('a
b
c')).toBe('a b c'); + }); + + it('bounds length so one entry cannot exhaust the storage quota', () => { + const out = sanitizeDiagEntry('x'.repeat(DIAG_ENTRY_MAX_CHARS * 3)); + expect(out).toHaveLength(DIAG_ENTRY_MAX_CHARS); + }); + + it('never throws on the values a diagnostic call site can actually pass', () => { + expect(sanitizeDiagEntry(null)).toBe(''); + expect(sanitizeDiagEntry(undefined)).toBe(''); + expect(sanitizeDiagEntry(42)).toBe('42'); + expect(sanitizeDiagEntry({ toString: () => 'obj' })).toBe('obj'); + }); +}); + +// ── The deadline must cover the BODY, not just the handshake ──────────────── +// +// `await fetch()` settles on response HEADERS. Clearing the abort timer there +// leaves the body — the multi-megabyte `?full=1` capture the deadline exists +// for — completely unbounded; it only ever covered a server that accepts a +// connection and never replies at all. +// +// Measured on the pre-fix shape against a server that sends headers immediately +// and stalls the body 4s under a 1s deadline: fetch resolved at 30ms, the timer +// was cleared there, and the body completed at 4026ms unaborted. +// +// This exercises the real property with a real socket rather than asserting on +// source text, because the bug was a lifetime mistake that reads correctly. +describe('terminal capture deadline covers the response body', () => { + // Mirrors _fetchTerminalCapture's lifetime: one timer spanning headers AND + // body, cleared only once the body has been read. + async function captureUnderDeadline(url: string, deadlineMs: number) { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), deadlineMs); + try { + const res = await fetch(url, { signal: controller.signal }); + const headersAt = performance.now(); + const json = await res.json(); + return { json, headers: res.headers, headersAt }; + } finally { + clearTimeout(timer); + } + } + + async function serve(handler: (res: ServerResponse) => void) { + const srv = createServer((_req, res) => handler(res)); + await new Promise((r) => srv.listen(0, '127.0.0.1', r)); + const { port } = srv.address() as AddressInfo; + return { url: `http://127.0.0.1:${port}/`, close: () => srv.close() }; + } + + it('aborts a stalled body instead of waiting on it forever', async () => { + let finish: NodeJS.Timeout | undefined; + const { url, close } = await serve((res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.write(' '); // headers out immediately, body never completes in time + finish = setTimeout(() => res.end('{"data":{}}'), 5000); + }); + try { + await expect(captureUnderDeadline(url, 300)).rejects.toThrow(/abort/i); + } finally { + if (finish) clearTimeout(finish); + close(); + } + }); + + // The two tests above exercise the PATTERN against a real socket, using a + // local mirror — so on their own they would still pass if the real helper + // regressed to clearing its timer at headers. This pins the real one. + it('_fetchTerminalCapture reads the body before releasing its deadline', () => { + const app = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8'); + const start = app.indexOf('async _fetchTerminalCapture('); + expect(start, 'helper not found — renamed?').toBeGreaterThan(-1); + const body = app.slice(start, app.indexOf('\n }', start)); + const jsonAt = body.indexOf('await res.json()'); + const finallyAt = body.indexOf('} finally {'); + expect(jsonAt, 'the body must be read inside the helper, not by callers').toBeGreaterThan(-1); + expect(finallyAt).toBeGreaterThan(-1); + expect( + jsonAt, + 'await res.json() must run BEFORE the finally that clears the abort timer — ' + + 'fetch() settles on headers, so a timer cleared there leaves the body unbounded' + ).toBeLessThan(finallyAt); + // And the returned shape the five call sites destructure. + expect(body).toContain('return { json, headers: res.headers, headersAt };'); + }); + + it('returns the parsed envelope and headers on a healthy response', async () => { + const { url, close } = await serve((res) => { + res.writeHead(200, { 'Content-Type': 'application/json', 'server-timing': 'db;dur=12' }); + res.end('{"data":{"terminalBuffer":"hello"}}'); + }); + try { + const out = await captureUnderDeadline(url, 5000); + // Callers read `capture.json?.data`, `capture.headers.get(...)` and + // `capture.headersAt` — all three must survive. + expect((out.json as { data: { terminalBuffer: string } }).data.terminalBuffer).toBe('hello'); + expect(out.headers.get('server-timing')).toBe('db;dur=12'); + expect(typeof out.headersAt).toBe('number'); + } finally { + close(); + } + }); +}); diff --git a/test/xterm-private-api.test.ts b/test/xterm-private-api.test.ts new file mode 100644 index 00000000..36a0b891 --- /dev/null +++ b/test/xterm-private-api.test.ts @@ -0,0 +1,75 @@ +// Port: none (dependency-range guard — no browser, no server). +// +// terminal-ui.js's `_kickRenderer` reaches into xterm internals to unwedge a +// frozen RenderDebouncer: +// +// terminal._core._renderService._renderDebouncer._animationFrame +// terminal._core._renderService.refreshRows(start, end) +// +// There is no public API for any of it — xterm exposes no way to ask "are you +// still producing frames" or "drop your stale animation handle" — and the bug +// it heals (iOS discarding a scheduled rAF, leaving that handle permanently set +// so every later refresh() early-returns) is otherwise unrecoverable without a +// page reload. +// +// That path CANNOT be asserted in this suite. `_renderService` is constructed +// by `Terminal.open()`, which needs a real DOM, and the CI gate runs in node — +// a headless Terminal reports `_renderService: undefined`, so a test here would +// pass whether or not the field still exists, which is worse than no test. +// +// So this guards the next best thing: the dependency range those field names +// were verified against. A major bump fails here, loudly, and sends someone to +// re-verify `_kickRenderer` by hand in a browser. The failure mode being +// defended against is silent — every access in `_kickRenderer` is +// optional-chained, so a renamed field degrades it to a permanent no-op with no +// error, no log, and a terminal that simply freezes again. +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +const root = resolve(import.meta.dirname, '..'); +const lock = JSON.parse(readFileSync(resolve(root, 'package-lock.json'), 'utf8')) as { + packages: Record; +}; +const terminalUi = readFileSync(resolve(root, 'src/web/public/terminal-ui.js'), 'utf8'); + +// The exact version `_kickRenderer`'s field path was verified against. +// +// Read from the LOCKFILE, not package.json. The declared range is `^6.0.0`, so +// asserting on that string is the wrong test in both directions: a real upgrade +// to 6.4.0 — which can absolutely rename a private field — resolves inside the +// range and slips through, while an innocuous range edit that changes nothing +// about the installed code fails. The lockfile is what actually ships. +const VERIFIED_XTERM_VERSION = '6.0.0'; + +describe('xterm private-API dependency guard', () => { + it('pins the resolved xterm version _kickRenderer was verified against', () => { + expect( + lock.packages['node_modules/@xterm/xterm']?.version, + 'xterm moved off the verified version — re-verify _kickRenderer in a real browser ' + + '(terminal-ui.js: _core._renderService._renderDebouncer._animationFrame), then update ' + + 'VERIFIED_XTERM_VERSION here. The accessor is optional-chained, so a renamed field ' + + 'degrades to a silent no-op and the freeze it heals comes back unnoticed.' + ).toBe(VERIFIED_XTERM_VERSION); + }); + + // If someone deletes the watchdog, this guard is pointless noise — keep the + // two tied together so the range check cannot outlive what it protects. + it('is guarding a watchdog that still exists', () => { + expect(terminalUi).toContain('_kickRenderer()'); + expect(terminalUi).toContain('_renderDebouncer'); + expect(terminalUi).toContain('_animationFrame'); + }); + + // Every private read must stay optional-chained. This is the property that + // makes reaching into internals acceptable at all: upstream can rename + // anything and the worst case is that healing stops, never that the terminal + // throws on a timer every two seconds. + it('reads every private field defensively', () => { + expect(terminalUi).toContain('this.terminal?._core?._renderService'); + const body = terminalUi.slice(terminalUi.indexOf('_kickRenderer() {')); + const fn = body.slice(0, body.indexOf('\n },')); + expect(fn).toContain('try {'); + expect(fn).toContain('catch'); + }); +});