diff --git a/docs/2026-08-12-agent-status-open-tasks.md b/docs/2026-08-12-agent-status-open-tasks.md new file mode 100644 index 00000000..6a1da3ae --- /dev/null +++ b/docs/2026-08-12-agent-status-open-tasks.md @@ -0,0 +1,91 @@ +# Open tasks — viewport-relative cursor + server-side agent sub-status + +Register for the work started 2026-08-12. Plans: [`plans/2026-08-12-viewport-relative-cursor-positioning.md`](./plans/2026-08-12-viewport-relative-cursor-positioning.md) and [`plans/2026-08-12-agent-sub-status-server-side.md`](./plans/2026-08-12-agent-sub-status-server-side.md), both in [#537](https://github.com/RonenMars/threadbase-streamer/pull/537). +Evidence: [`2026-08-12-claude-pty-capture.md`](./2026-08-12-claude-pty-capture.md). + +## Done + +| What | Where | +|---|---| +| Both plans, five and three review rounds | [streamer#537](https://github.com/RonenMars/threadbase-streamer/pull/537) | +| PlanCup implemented — viewport-relative `H`/`f`/`A`/`B` with clamps | [mobile#654](https://github.com/RonenMars/threadbase-mobile/pull/654) — green, mergeable | +| PTY capture (output-heavy turn, no tool use) | `~/.threadbase/captures/`, README committed | +| PlanIndicator Part A (streamer) | branch `feat/agent-sub-status`, uncommitted | + +## Blocked / awaiting a decision + +**Merge [mobile#654](https://github.com/RonenMars/threadbase-mobile/pull/654).** 10/10 CI green, `MERGEABLE`, body records verification item 4 as satisfied. Nothing outstanding. + +**Second PTY capture — tool-heavy turn.** In progress with `consultant-for-streamer`. The only artifact that can still change anything. Settles, in priority order: +1. `↑` vs `↓` — whether the arrow tracks direction-of-dominance. Decides whether the struck "different sub-field" argument is permanently dead. +2. `hooks…` — needs a turn that actually triggers a hook (a Stop hook is cheapest). +3. The `thinking` state — `WORKING` matching while `TOKENS` does not; sample early in a turn. +4. Whether any deferred #652 sequence (`S`/`T`/`L`/`M`/`A`/`D`/`f`) fires mid-turn. + +Until it lands, **both plans rest on one output-dominated turn with no tool use**, and that qualifier belongs on every citation. + +## PlanIndicator — remaining work + +### Part A (streamer) — branch `feat/agent-sub-status`, not yet a PR + +Complete and verified: `AgentPhase` union, `subStatus` on both types, unconditional-key serialisation, `onPhaseChange` callback, clear at `markReady`, deduping `setPhase`, derive wired into the existing scrape pass, scoped `session_phase` frame, pty-host event + protocol v3, compatibility doc. +`tsc` clean, 21 tests, contract tests verified against deliberately-broken variants. + +Committed as `0ea09bc`, pushed, **deliberately not a PR** — four verification gaps remain, tracked in [streamer#541](https://github.com/RonenMars/threadbase-streamer/issues/541): + +- [ ] Integration test through a fake PTY, mirroring `__tests__/session-status-line.test.ts:54`. +- [ ] Assert the scrape pass stays non-fatal — a throw inside `detectLivePrompts` is swallowed into a `warn` at both call sites, so a regression there is silent. +- [ ] Direct `setPhase` / `markReady` unit tests — the change-guard and the turn-end clear, currently covered only indirectly. +- [ ] Full-suite run. + +### Part A2 (streamer) — the Claude derive. **Blocked on capture 2.** + +`parseAgentPhase`'s Claude branch deliberately returns `null`. Reporting no phase is the correct pre-feature behaviour; a guessed phase is worse than none. +Do not write it until the marker grammar is re-verified — and reuse `terminalChrome`'s grammar rather than re-deriving glyph classes, which is how #647 drifted. + +### Part B (mobile) — [mobile#653](https://github.com/RonenMars/threadbase-mobile/issues/653). **Blocked on Part A shipping.** + +Not "add a field and render it". Ordered by risk: + +1. ~~Clearing contract~~ — resolved in Part A (always-emit, explicit `null`). +2. Cache-handler reconciliation — [mobile#655](https://github.com/RonenMars/threadbase-mobile/issues/655), independent, see below. +3. Add `subStatus` to `types/api.ts` **and** to `SessionPresentationInput` (`lib/sessionPresentation.ts:58-72`) — that type is standalone, not derived from `Session`, so adding it in one place leaves the derive blind. +4. Gate on `presentation.live`, **not** raw `status` — `deriveSessionPresentation` has branches that never consult `status`, so a raw gate can render a phase beside a badge reading "Idle" or "External". +5. Render via the existing colour token if possible; a phase-specific colour is ~20 files across 17 theme objects. Never a hardcoded hex. +6. Question-card suppression — copy `ThinkingBubble.tsx:107-134`, which covers both the structured and PTY-scraped gates. +7. No client-side time decay. The app's idiom is `processLiveness === 'gone'`: the server decides liveness and says so. + +Tree mode (`TreeRow.tsx:63`) is deliberately out of scope. + +## Filed, unstarted + +| Issue | Summary | +|---|---| +| [mobile#655](https://github.com/RonenMars/threadbase-mobile/issues/655) | `LiveConversationView.tsx:143` replaces the whole session object where two other writers merge, wiping REST-only fields while mounted. Pre-existing; P2 (badge cannot be mislabelled — `lifecycle`/`ownership` are always on the frame). Worth landing before Part B. | +| [mobile#656](https://github.com/RonenMars/threadbase-mobile/issues/656) | `processLiveness: 'gone'` is never emitted by the streamer, so three mobile production paths are unreachable — including the whole `stale` branch. Open question: whether a vanished external session lingers in the eager cache. Turns on whether React Query's focus manager is wired to `AppState`. | +| [streamer#539](https://github.com/RonenMars/threadbase-streamer/issues/539) | `CODEX_BUSY_STATUS_RE`'s `\b` does not stop a path from false-hitting; a Codex session under a dir named `Working`/`Starting` never reads as ready. | +| [streamer#541](https://github.com/RonenMars/threadbase-streamer/issues/541) | The four verification gaps in Part A, above. Blocks its PR. | +| [mobile#668](https://github.com/RonenMars/threadbase-mobile/issues/668) | The deferred `VirtualTerminal` CSI family, filed as one issue because the capture shows none of it is reachable mid-turn. P3, unblocked only by a tool-heavy capture. | +| [mobile#669](https://github.com/RonenMars/threadbase-mobile/issues/669) | `useTerminalStream`'s WS effect omits `provider` from its deps, so live frames are filtered by a stale provider's chrome rules. Independent of #647, which is why it survived that PR's abandonment. | + +**The #652 deferred family is uniformly downgraded, not reordered** — [mobile#668](https://github.com/RonenMars/threadbase-mobile/issues/668). The capture shows `S`/`T`/`L`/`M`/`A`/`D`/`f` never emitted and `r`/`J`/`c`/`ESC7`/`ESC8` firing once at startup. Nothing in it is reachable mid-turn. Only capture 2 can change that. + +## Deliberately not filed + +- **Putting PTY geometry on the wire.** The follow-up is *conditional*, not scheduled: whoever adds resize adds the wire field in the same change, which is the policy `src/pty-host/protocol.ts:38-41` already states. Filing it would create a TODO nobody actions for a value that cannot currently vary. +- **Housekeeping from the capture session** — `~/dev/pty-capture-tmp/.remember/` and `~/.claude/projects/-Users-ronenmars-dev-pty-capture-tmp/`. Conversation history, safe to delete, not repo work. +- **The stale `tb-mobile-worktrees/feat-agent-sub-status` worktree** at the closed #647 branch (`0a58200c`). Do not reuse it; it carries the client-side implementation this work replaces. + +## Method notes earned here + +Four defects were caught *after* sign-off. All four came from measurement, not from reading. Both rules below are in the plans and were each violated by the person who had just written them down. + +**Run the test that killed the thing you are replacing, before you propose the replacement.** +Stated as a construction practice, not a review one — the failure happens at proposal time. Three successive attempts to promote an item out of the deferred queue (`2J`, `CSI r`, `ESC7`/`ESC8`) were the same artifact relabelled, each proposed by someone who had written the disqualifying caveat into the same message. *Stating a caveat is not applying it.* + +**Ask per statistic whether it is sensitive to how the file was assembled.** +The answer differs between statistics on one line of evidence: frequencies were inflated by snapshot duplication, while the maximum and the distinct set were exact. Over-correcting into a blanket "magnitude is unreliable" nearly discarded `max = 40`, the one figure the constant rests on. **Under-trusting is the quieter failure** — it never yields a wrong answer, it just discards a right one and nobody goes back to check. + +**Hand disagreements over as evidence, not conclusions.** Offsets and per-snapshot counts can be re-run by the recipient; an unfalsifiable claim can only be accepted or rejected. That is also what makes a claim cheap to test against yourself. + +**A zero is evidence of absence only if the same command returns non-zero on a file you know contains a match.** Five distinct ways to produce a clean-looking zero were hit for real here; they are catalogued in the capture README. diff --git a/docs/2026-08-12-claude-pty-capture.md b/docs/2026-08-12-claude-pty-capture.md new file mode 100644 index 00000000..681b55b0 --- /dev/null +++ b/docs/2026-08-12-claude-pty-capture.md @@ -0,0 +1,134 @@ + + +# Claude Code PTY capture — 2026-08-12 + +Raw PTY bytes from a live Claude Code session, escapes intact. Produced to settle open +questions in PlanCup (tb-mobile #652) and PlanIndicator (tb-streamer #538). + +## Files + +- `snap-NNN.raw` — 36 distinct snapshots of `session.outputBuffer`, polled ~0.7s apart + through one full turn via `GET /api/sessions/:id/output`. +- `claude-v2.1.228-turn.raw` — all snapshots concatenated (1,123,773 bytes). **Snapshots + overlap heavily; see "Counting" before trusting any number taken from this file.** + +## Provenance + +- Claude Code **v2.1.228**, spawned by tb-streamer at its fixed **120x40** geometry. +- Server flags in effect: `--model claude-opus-4-8 --effort high`. +- Prompt: "Write out the numbers 1 through 60, each on its own line, with a one-sentence + interesting fact about that number." Pure text generation — **no tool use**. +- Throwaway project dir `~/dev/pty-capture-tmp`; session stopped after capture. + +## Findings + +Real footer status lines recovered (escapes stripped): + +``` +·Marinating… (25s · ↓ 1.1k tokens) mid-turn +✻Baked for 26s end-of-turn +``` + +Markers: `↓` 31, `↑` **0**, `tokens` 31, `hooks…` **0**, `esc to interrupt` **0**. +The absent `↑` matters — `parseStatusLine`'s fixture uses `↑ 3.4k tokens`, so the +"input vs output arrow" distinction is **contradicted by this capture**. Likely the arrow +tracks whichever direction dominates the turn (this one was output-heavy), but that is +untested. A tool-heavy turn should settle it. + +**Absolute cursor rows addressed: only 1, 37 and 40. Maximum exactly 40** — the 120x40 +geometry is measured here, not assumed. + +### CSI final bytes — corrected reading + +``` +during the turn: G 51327 B 10075 C 8222 m 8213 H 4395 K 3805 +startup only: r 1 J 1 c 1 ESC7 1 ESC8 1 +never emitted: S T L M A D f +``` + +The startup row deserves care. A naive count over the concatenated file reports **30** for +each of `r`, `J`, `c`, `ESC7`, `ESC8`. That is an artifact: each snapshot is a *full ring +buffer dump*, so every snapshot that has not yet wrapped replays the session's startup +bytes. Verified per-snapshot — `CSI r` sits at offset 2 in exactly 30 of 36 snapshots and +is absent from the six 65536-byte wrapped ones; `ESC7`/`ESC8` sit at offsets 0 and 5 in the +same 30. **True count for each is 1, at startup.** + +The whole startup sequence is these 8 bytes, once: + +``` +ESC 7 ESC [ r ESC 8 (save cursor, reset margins to full screen, restore cursor) +``` + +Every `CSI r` in the capture is **argument-less** (verified: the only parameter string +present is empty). Bare `ESC[r` *resets* margins to the full screen — it does not set a +scroll region. So an emulator that models only the full screen and ignores `r` reaches the +correct end state; `case 'r': break` is right, not a defect. Likewise a startup `2J` +annihilates an empty grid, which is harmless. + +**Consequence for the tb-mobile #652 follow-ups: nothing in the deferred family is +reachable mid-turn in this capture.** `S`/`T`/`L`/`M`/`A`/`D`/`f` never appear at all, and +`r`/`J`/DECSC/DECRC appear exactly once each during terminal init. The queue should be +uniformly downgraded pending a capture that shows any of them firing mid-session — there is +no item here that deserves promoting above the others. + +## Caveats — read before citing + +1. **One turn, one version, no tool use.** A tool-heavy turn may emit sequences this one + does not. Absence here is not absence in general. `hooks…` and the tokens-absent + "thinking" state were never observed for exactly this reason. +2. **The ring buffer is a 64KB byte-level tail cut** (`pty-manager.ts:820-825`), so a + snapshot can begin mid-escape and mid-UTF-8. At most one malformed sequence per + snapshot head. +3. **Snapshots overlap.** Presence/absence is reliable; magnitude is not. Derive + per-snapshot before quoting any count — see the startup artifact above. + +## Counting + +Two independent ways to get a wrong number out of this file: + +- **`grep -c` counts matching LINES, not occurrences**, and this capture contains **zero + newlines** — it is one unterminated line. `grep -acoE $'\033\\[[0-9;]*r'` returns `1` + where the true occurrence count is `30`. `-c` silently overrides `-o`. Use + `grep -aoE … | wc -l`. A zero is unaffected (zero lines containing ⇒ zero occurrences), + but every non-zero count taken with `-c` understates. +- **Snapshot overlap inflates**, as above. The two errors push in opposite directions and + can cancel, which is worse than either alone. + +## Five ways this capture yields a clean-looking zero + +Every one of these was hit for real while analysing it or the code around it: + +1. **Binary-file silence.** `file(1)` calls a raw dump `data`; grep then skips it and + prints nothing. `tb-streamer/src/pty-manager.ts` trips this too — it contains a raw + byte, so plain `grep` returns empty for patterns that are certainly present. Use `-a`. +2. **Malformed pattern that errors like a miss.** `grep` here resolves to **ugrep**, and + neither it nor `/usr/bin/grep` accepts `$'\x1b\['` — that is ESC plus a *literal* `[`, + read as an unterminated bracket expression. It errors; add `2>/dev/null` and it becomes + an innocent-looking zero. Use `$'\033\\['` (double backslash) or `-F`. +3. **Text that is never contiguous in the stream you search.** Regexing the raw bytes for + `↓ N tokens` returns zero while 31 `↓` characters are present — the footer is assembled + by absolute cursor moves and exists only after rendering. This is the documented reason + `parseStatusLine` reads rendered lines rather than raw bytes. Strip CSI first. +4. **`-c` on a file with no newlines**, as above. +5. **Searching a field name instead of its values.** `activity` looked unpopulated in + tb-streamer because its only assignment builds the value in a different file from the + rest of the response shape; grepping the enum member (`active_writing`) found it. Grep + the values a field can take, not just its name. + +**The rule that covers all five: a zero is evidence of absence only if the same command +returns non-zero on a file you know contains a match.** Always run the positive control. + +```sh +printf '\033[40;1Htest\033[5S\n' > /tmp/poscontrol.raw +/usr/bin/grep -aoE $'\033\\[[0-9;]*S' /tmp/poscontrol.raw | wc -l # must print 1 +/usr/bin/grep -aoE $'\033\\[[0-9;]*S' claude-v2.1.228-turn.raw | wc -l + +/usr/bin/grep -aoE $'\033\\[[0-9;]*[A-Za-z]' claude-v2.1.228-turn.raw \ + | sed 's/.*\[//; s/[0-9;]*//' | sort | uniq -c | sort -rn +``` + +`CSI S` = **0** against that control = 1. That is the citation for #652's deferral. diff --git a/docs/plans/2026-08-12-agent-sub-status-server-side.md b/docs/plans/2026-08-12-agent-sub-status-server-side.md new file mode 100644 index 00000000..e9223102 --- /dev/null +++ b/docs/plans/2026-08-12-agent-sub-status-server-side.md @@ -0,0 +1,322 @@ +# PlanIndicator — derive agent sub-status server-side (streamer + mobile) + +Cross-repo. + +**Worktrees — outside each repo root, branched from the current `origin/main`:** + +| Repo | Path | Branch | Base | +|---|---|---|---| +| streamer | `/Users/ronenmars/dev/ai-tools/tb-streamer-worktrees/feat-agent-sub-status` | `feat/agent-sub-status` | `origin/main` @ `0eb2c53` — **created, `npm ci` done** | +| mobile | `/Users/ronenmars/dev/ai-tools/tb-mobile-worktrees/feat-agent-sub-status-render` | `feat/agent-sub-status-render` | `origin/main` @ `86eeb2a1` — create when Part B starts | + +Work in the worktree, never in the main checkout, and never nested inside the repo root — a nested worktree is a full second copy that Jest, ESLint, TypeScript and Metro all walk into. +Rebase onto `origin/main` before opening either PR. +On the mobile side `npm ci` must run in the worktree: a symlinked `node_modules` makes Metro bundle the main repo and hides the worktree's own lockfile drift. + +Do **not** reuse the stale `tb-mobile-worktrees/feat-agent-sub-status` worktree at `0a58200c` — that is the abandoned PR #647 branch, behind `main` and carrying the client-side implementation this plan replaces. It was **closed, not merged**, so none of that code is on `main`; see Part B. + +Independent of [PlanCup](./2026-08-12-viewport-relative-cursor-positioning.md) — neither blocks the other. + +## Goal + +Show whether a live agent is *thinking*, *streaming*, *running hooks*, *acting*, or *waiting for input* — a phase axis inside `running`, which `SessionStatus` does not model today (`src/types.ts:9`: `"running" | "waiting_input" | "idle"`). + +Derive it **here**, in the streamer, off the rendered PTY screen, and emit it as an additive field. +Mobile renders it and nothing more. + +## Why this layer + +An earlier attempt (tb-mobile PR #647, now closed) derived it on the client by scraping mobile's own terminal emulator. +It never worked: mobile's `VirtualTerminal` mis-handles absolute cursor positioning, so the status line it searched for was never where it looked. +Beyond that specific bug, the client is the wrong layer, because this server already has everything the derive needs: + +- A real `@xterm/headless` screen per session at the exact geometry the TUI paints against — `src/pty-manager.ts:42-43`, 120x40. +- A throttled screen-scrape already running on every chunk — `detectLivePrompts` at `src/pty-manager.ts:857`/`:881`, `SCRAPE_THROTTLE_MS = 300` at `:80`. +- An established precedent for scraping this exact footer and shipping the result — `src/services/questions/parseStatusLine.ts`, which extracts `model` / `effort` / `permissionMode`. + +## What ALREADY exists — do NOT rebuild + +- **`parseStatusLine.ts`** — the module to copy, not to extend. Pure, no I/O, scans rendered lines bottom-up (*"the footer is the last thing painted, and older scrollback can contain text that looks like a footer row"*), returns all-optional fields, and degrades to an empty object rather than throwing or inventing a value. +- **`detectLivePrompts`'s single screen read** — `src/pty-manager.ts:942`, `getOutputLines(sessionId, 60)`. Every downstream detector shares it. A new derive hooks in here and needs **no second read**. +- **The additive-callback pattern** — `onPermissionChange` (`src/types.ts:566-577`), *"Additive; absent in tests that omit it."* +- **The additive-field pattern on the wire** — `effort` / `permissionMode` at `src/types.ts:385-394`, and `statusSource` / `statusConfidence` / `statusUpdatedAt` at `:359-367`. **Streamer-side only** — verified absent from mobile's `types/api.ts` and from all mobile source, so they are not a liveness gate the client already has. Do not plan against them as an existing mobile input. +- **Codex already classifies its own phase.** `src/codex-pty-runner.ts:40-50` exports `CODEX_PROMPT_READY_TEXT = "Ready"` and `CODEX_BUSY_STATUS_RE = /\b(?:Starting|Working)\b/`; `:185` isolates the status bar; `:362` maintains a `turnBusy` set. Its scrape pass is `detectScreenState` at `:1024`. +- **Mobile delivery needs no new subscription — but it does need design.** A field inside `Session` reaches the client through the merge at `tb-mobile/app/_layout.tsx:164-166` and `useSessionDetail` (`hooks/useSession.ts:291-303`) with no new wiring. What it does *not* get for free is a coherent lifetime: three call sites write that cache key with incompatible semantics, and absence cannot express "cleared". See Part B1/B2 — that is the real client-side cost. + +## The must-fix: the phase is cleared server-side, or this ships #647's bug again + +`src/pty-manager.ts:1103-1108` warns: + +> Claude's TUI does differential repaints and doesn't always retransmit the box border once it's already drawn — so a mid-conversation return to idle can go undetected forever if the last chunk didn't happen to carry it. + +An earlier draft cited that as evidence for *rejecting* the elapsed counter, and never ran it against the accepted design. Running it, as the standing rule requires: **it applies with full force.** A phase field stamped by the scrape pass and never cleared latches on any session that stops emitting — the last chunk said `streaming`, no further chunk arrives, the field says `streaming` forever. That is exactly the bug PR #647 shipped, relocated from the render layer to the server. + +The only mitigation in the earlier draft was on the *client* (gate on `statusUpdatedAt`), which pushes the liveness obligation onto every consumer — precisely how #647 failed. + +**The rule, which belongs in Part A:** + +> The phase is a **refinement of `status === "running"`** and is cleared server-side at the turn-end transition. It is never a free-standing state with its own lifecycle. + +The machinery already exists. `markReady` (`src/pty-manager.ts:1168`) is the single idempotent `running → waiting_input` transition; it already sets `statusSource` and `statusUpdatedAt` and already fires `onStatusChange`. Clear the phase there. It is driven by `hasWaitingForInputOsc` (`src/services/questions/detectPermissionGate.ts:101`, used at `pty-manager.ts:900`), which the codebase describes as *authoritative — it arrives even when no further chunk will (the turn is over)*: the one signal that survives the differential-repaint problem. + +With that in place, the client-side freshness gate becomes defence in depth rather than the only defence. + +## The signal question — settled, but not the way the earlier draft framed it + +### The test-lock argument was wrong; drop it + +An earlier draft claimed `__tests__/parse-status-line.test.ts:61-64` locks this feature out. It does not. The test asserts that **`parseStatusLine` extracts none of *its own three fields*** from that line — not that the line carries no usable signal. And this plan proposes a *separate module*, so the test does not constrain the design at all. + +### The rejected sub-field is not the one the derive reads + +`parseStatusLine.ts:9-12` rejects the elapsed counter, and its fixture is `(56s · ↑ 3.4k tokens)` — **`↑`, input tokens**. The derive keys on **`↓`, output tokens**, plus the presence of a duration-bearing parenthetical, `hooks…`, and a ` for s` end marker. Different sub-fields of the same parenthetical. What was rejected is *the numeric value of a monotonically-advancing counter*; what this reads is *the presence of markers*. + +### The argument that survives the person who wrote that comment + +Not "a phase lags less than a timer" — that is a matter of degree and he can simply disagree. The argument is about signal class: + +- The elapsed counter is a **continuous function of wall-clock time**, sampled at output events. Between samples it is wrong and the error grows without bound. No sampling strategy repairs that, which is why the comment prescribes local animation instead. **He was right.** +- The phase is a **step function whose transitions are themselves output events.** `thinking → streaming` happens *because* tokens began painting; `→ hooks` because hook output painted. The repaint condition and the transition condition are the same condition, so discrete sampling at output events is **exact** for this signal, not approximate. +- The single exception is the exit edge, which is not an output event — and that is exactly `:1103-1108`. Which is why the clearing rule above is mandatory rather than nice-to-have. + +**Caveat to discharge first:** the `↑`/`↓` split comes from a module derived against two captured turns. Re-verify on a fresh capture before building on it. `GET /api/sessions/:id/output` returns the raw ring buffer verbatim, so one curl against a live session settles this, `hooks…`, and the end marker at once. + +## The plumbing cost is real, and the cheap path is a trap + +`parseStatusLine` is **REST-only** — called once, from `handleGetSession` at `src/server.ts:4522-4537`, and never from the WS broadcast path, `session_list`, or `managedToResponse`. +So `effort` and `permissionMode` are absent from every `session_update` frame today. +That is structural, not an oversight: `managedToResponse` (`src/session-store.ts:222-311`) is synchronous and has no PTY handle. +A *static* field can be scraped lazily on GET. A *live phase* cannot. + +### Rejected: riding `onStatusChange` + +There is a cheaper path and it must not be taken. `onStatusChange` is already in `PTYManagerOptions` (`src/types.ts:564`) and **already relayed across the pty-host boundary** (`src/pty-host/remote-session-runner.ts:279`), so riding it would need no new callback, no new event, and no version bump. + +Its handler (`src/server.ts:959-1060`) does, *per invocation*: `sessionStore.updateManaged`, `managedSessionsRepo.recordStatus`, a scanner index refresh, file-watcher teardown, a **global** `wsHub.broadcast`, an APNs Live Activity update, and a push notification check. `recordStatus` (`src/db/repositories/managed-sessions.repository.ts:229-247`) runs its update statement **unconditionally, with no same-status guard** — so at `SCRAPE_THROTTLE_MS = 300` that is roughly three durable SQLite writes per second per active session to carry a cosmetic field, and the throttle is a floor, not a ceiling. The notifiers do guard on `previousStatus`, so it probably would not spam pushes; "probably would not spam push notifications" is not a property to ship. + +### The actual work + +1. **Optional field on `ManagedSession` and `SessionResponse`** — needed for the GET path and the reconnect baseline. Stamped by the scrape pass, not read on demand. +2. **A new dedicated callback in `PTYManagerOptions`**, mirroring `onPermissionChange` — deliberately *not* wired into the status funnel. +3. **A scoped, bespoke frame — not the global broadcast, and not a `SessionResponse` copy.** `src/server.ts:1050` is `wsHub.broadcast` — every client, every session. Use `broadcastToClients` (`src/ws-hub.ts:74`), which exists precisely for *"high-frequency per-session messages (terminal_output, user_message)"* because global broadcast *"made broadcast() cost scale with connections × active sessions."* + + **Send a minimal frame, following that path's existing tenants** — `terminal_output` and `user_message` are bespoke minimal messages, not session copies: + + ```ts + { type: "session_phase", sessionId: string, phase: Phase | null, updatedAt: string } + ``` + + **Why not a session copy:** `managedToResponse` computes `elapsedMs: (s.completedAt ?? new Date()).getTime() - s.startedAt.getTime()` (`src/session-store.ts:277`). For a live session `completedAt` is null, so **`elapsedMs` changes on every call** — every phase frame would deliver a session object differing from the last whether or not the phase changed. Mobile's `{...prev, ...msg.session}` then produces a new object identity ~3×/second, re-rendering every React consumer of that session for the whole turn, and re-running the `lifecycle`/`lifecycleSource` derivation each tick. It appears in no test and on device reads as "the app got sluggish during turns." Same class as the `recordStatus` write amplification above: a high-frequency signal routed through machinery built for low-frequency whole-object updates. + + The bespoke frame also puts no mapper in the live path, so the null-erasure hazard below applies only to the GET path. And its `updatedAt` gives mobile a freshness signal without mirroring `statusUpdatedAt` — which keeps the app's "server decides liveness and says so" idiom intact and avoids introducing its first time-decay gate. +4. **A pty-host protocol event, and a `PTY_HOST_PROTOCOL_VERSION` bump** (currently `2` at `src/pty-host/protocol.ts:48`). Unavoidable: `:32-36` is explicit that *"the detectors that fire them run in the host, so every one of those callbacks needs an event here or the feature silently stops working when the flag is on."* +5. **Clear the phase at `markReady`** — see the must-fix above. +6. **On the GET path, use an unconditional key — the mapper will otherwise erase the null silently.** `managedToResponse` builds every optional field as `...(s.x != null && { x: s.x })`, and `!= null` is the loose comparison that catches **null and undefined alike**. There are **19** instances of that idiom in the file, so an implementer matching the surrounding style will almost certainly write it — converting explicit-null into absence, at which point mobile's merge keeps the previous value and the label latches. That is #647's bug arriving through the mapper instead of through the missing clear, defeating the very contract added to prevent it. + + The correct in-file precedent is three lines away at `src/session-store.ts:280`: + + ```ts + completedAt: s.completedAt?.toISOString() ?? null, + ``` + + An unconditional key carrying an explicit null. Copy that, **and comment it so it is not tidied into the guard block later** — it looks like it belongs there, and moving it would break the clearing contract without touching anything named `subStatus`. + + Type it **`Phase | null`, not `Phase?`** — the optional form is `Phase | undefined` and cannot hold an explicit null. Same on mobile's `Session`. + +A `SessionRunner` method (`src/types.ts:640-662`) is **not** needed: as a `PTYManagerOptions` callback, every runner already accepts it. + +### One semantic rule: always emit the field + +`null` when there is no phase — on the live frame and on `SessionResponse` alike. **Never let absence carry meaning.** The whole problem is that a merge cannot express absence, so do not create a state that depends on it. An "absent means no information, null means cleared" distinction is a third state someone will get wrong, and it is the same shape as the bug just closed. + +## PART A — streamer + +### A1. Codex first — but define the full enum up front + +Codex is the cheap half and validates the whole transport before anyone fights Claude's footer. +Its status bar is a plain word on the last non-blank rendered line, already isolated and already classified. +Ship the field, the callback, the protocol event and the mobile render against Codex, end to end. + +**The hazard: Codex's phase axis is binary and Claude's is not.** `CODEX_BUSY_STATUS_RE = /\b(?:Starting|Working)\b/` against `CODEX_PROMPT_READY_TEXT = "Ready"`, and the captured note is explicit that a Codex turn walks Ready → Working → Ready with no other state, so *"claiming otherwise would be invention."* If Codex lands first unguarded, the field's type gets fixed by a two-valued provider — and the failure mode is shipping a boolean or a two-member enum that Claude's four-or-five phases then cannot fit without a breaking change to a field mobile has already been told to render. + +Mitigation, cheap: **define the full enum in A1** even though Codex only ever emits two of its members, and add a contract test that an unrecognised phase value is ignored rather than coerced. That gets the transport proof without baking the shape. The enum lives in **exactly one place** — this is the two-copies-of-the-grammar problem from #647 in a new costume. + +### A2. Claude derive + +New pure module beside `parseStatusLine.ts`, same contract: no I/O, rendered lines in, all-optional out, empty result on no match, bottom-up scan. + +Never parse raw bytes — the footer is assembled by absolute cursor moves and has no contiguous byte form, which is the documented reason `parseStatusLine` reads rendered lines. + +Reuse `lib/terminalChrome.ts`-equivalent grammar rather than re-deriving glyph classes. +Two independently-maintained copies of the same TUI grammar already drifted once: PR #647's end-of-turn glyph class was a subset of the one that already existed, and the missing glyphs stranded the label. + +### A3. Wire it up + +Call from `detectLivePrompts`'s existing screen read. Note the throttle is a **floor, not a ceiling** — OSC and footer triggers bypass it, and a session with an open gate passes unconditionally. +A throw inside `detectLivePrompts` is swallowed into a `warn` at both call sites (`:857`, `:1118`), so a silent regression is easy: test the failure path explicitly. + +### A4. Never add a value to `SessionStatus` + +`src/types.ts:409-419` documents the rule: `VALID_STATUSES` rejects unknown values and the store drops sessions outside the requested set, so a new status string makes those sessions **vanish** from already-shipped apps. +The phase rides as a separate additive field, carrying `statusSource` / `statusConfidence` / `statusUpdatedAt`. + +**The general rule, worth writing down because the next feature will hit it: additive *fields* are safe; additive *values* in an existing union are not.** Anything a shipped client filters or switches on inherits the vanishing hazard. `VALID_STATUSES` is the documented case; `SessionLifecycle` (`src/types.ts:16-21`), `SessionOwnership`, `ProcessLiveness` and `StatusSource` are all in the same family. + +### A5. Unknown providers make no claim + +`getTerminalChromeFilter` already routes an unrecognised provider to passthrough — *"prefer passthrough over wrong Claude filters."* +Follow that, not PR #647's "Claude's rules are the safe default," which would give a third provider Claude's grammar. + +## PART B — mobile + +**Part B is not "add a field and render it."** An earlier draft of this plan claimed the client side was nearly free because the field arrives through existing wiring. It does arrive — but with **three different lifetimes depending on which screen is mounted**, and the clearing semantics do not exist at all. That is the part of Part B that needs design, and the streamer work should not be scheduled against the cheaper estimate. + +### B1. The cache problem — solve this before anything else + +Three call sites write the same `['session', serverId, sessionId]` key with **incompatible semantics**: + +| site | operation | effect on a field the frame omits | +|---|---|---| +| `app/_layout.tsx:164-166` | `{ ...prev, ...msg.session }` — merge | **sticky**: keeps the old value forever | +| `components/conversation/LiveConversationView.tsx:143` | `qc.setQueryData(key, msg.session)` — **whole-object replace** | **destroyed** | +| `lib/eagerCacheSync.ts:25-31` | `{ ...s, ...session }` — merge | sticky (home-screen row) | + +**This is a live bug today, not a new one.** `effort` and `permissionMode` are REST-only and absent from every `session_update` frame, so they are wiped whenever `LiveConversationView` is mounted and restored on the next refetch. Both handlers are subscribed simultaneously on the session detail screen, so whichever fires last wins. **File it as its own bug** — it is not caused by this feature and should not be fixed inside it. + +**But `subStatus` does not inherit it, and the replace is actually the *safer* semantic.** Work the four cases: + +| | replace (`LiveConversationView`) | merge (`_layout`, eager cache) | +|---|---|---| +| **with explicit null** | → null ✓ | → null ✓ | +| **field absent** | → cleared ✓ | → **latched** ✗ | + +An absent field is *cleared* under replace and *sticky* under merge. So the always-emit-null contract exists to serve the **merge** paths, not the replace one — and with it in place all three converge on the same value. Item 2 is therefore a genuine pre-existing bug but **not a blocker for this feature**. + +**Its severity is higher than "two fields go blank", though.** In `types/api.ts`, `status` is required (`:23`) but every field driving presentation classification is optional — `lifecycle?` (`:68`), `ownership?` (`:94`), `processLiveness?` (`:99`), `activity?` (`:104`). `services/ws-client.ts:15` types the frame as a complete `Session`, but the WS path demonstrably does not populate everything REST does, and because those fields are optional TypeScript never catches the gap. If `session_update` omits `ownership`/`processLiveness`, the whole-object replace does not blank metadata — it **flips the session out of the `external_live` or `stale` branch**, changing what the badge *says* for as long as the conversation screen is mounted. A visibly wrong label, not a missing detail. + +**Resolved — it stays a low-severity cleanup.** All six `session_update` emit sites broadcast `sessionStore.get()`, which returns `managedToResponse` for anything managed. That frame carries `lifecycle` and `ownership` **unconditionally** (`src/session-store.ts:250`, `:272`) — and those are precisely the two fields the mislabelling argument depended on. It omits `processLiveness`, `activity`, and `effort`/`permissionMode`. + +The scenario is unreachable for a second reason: **`processLiveness` has exactly one assignment site in the whole streamer** — `src/session-store.ts:325`, hard-coded to `"alive"` on the external path. `"gone"` and `"unknown"` are never produced. A `stale` branch that the server cannot cause the client to enter cannot be wrongly exited. + +Fixing `LiveConversationView.tsx:143` to merge is still worth doing — a whole-object replace against a frame that is a strict subset of the REST shape is fragile regardless — but it is **robustness, not a live defect**, and it does not outrank this feature. + +**A separate finding, pointing the other way — file it on its own.** Mobile handles `processLiveness: 'gone'` and `'unknown'` (`__tests__/unit/lib/externalSession.test.ts` asserts both, calling `'unknown'` *"a new-server signal"*) and `SessionCard.tsx:83` synthesises `'alive'` locally — while the streamer emits only `'alive'` from a single line. Mobile is defensively branching on states the server cannot express, which likely means a presentation branch is dead and the app's real staleness detection comes from the pid fallback. Different bug shape from "field gets blanked"; give it its own investigation. + +**Methodology note for whoever writes that up:** `activity` is assigned in `server.ts:3683`, not in `session-store.ts` where the rest of the response is built, and it only surfaces if you grep the enum value (`active_writing`) rather than the field name. One response shape assembled across two files is exactly what produces a confident wrong answer — **grep the values a field can take, not just its name.** + +### B2. The clearing contract — the plan must specify it, not the implementer + +`{ ...prev, ...msg.session }` does not remove a key that is missing from the frame, and `JSON.stringify` drops `undefined`. So **a server that simply stops emitting `subStatus` cannot clear it.** The client keeps the last phase until a full refetch. + +That is PR #647's latching failure reappearing one layer down — the pill latched at the render layer, this latches at the cache layer. Pick one and write it into the wire contract: + +- the server emits an explicit `subStatus: null` on phase exit, or +- the client treats "fresh `statusUpdatedAt` + absent `subStatus`" as cleared. + +This surfaced by applying the rule earned on PlanCup: *any input used to reject an alternative must be run against the accepted design.* This plan rejects the client-side layer partly because #647's pill latched onto idle sessions — and the accepted design latches too, for the same root cause. Second time that rule has paid. + +### B3. The rest + +1. Add `subStatus` to `types/api.ts` `Session`, in the additive block with `model` (`:43`), `effort` (`:47`) and `permissionMode` (`:53`). Type it `Phase | null`, never optional — see the always-emit rule. +2. **Also add it to `SessionPresentationInput` (`lib/sessionPresentation.ts:58-72`) — adding it to `Session` alone is not enough.** That type is **standalone and structural**, enumerating eleven fields explicitly rather than deriving from `Session`, so `deriveSessionPresentation` cannot see a field that is not on its own input type. This reads like a data-flow step and is actually a type declaration in a second file plus the derive logic. Small, but it is exactly what turns "add a field" into an afternoon. +3. **Gate on `presentation.live`, not on raw `status`.** `deriveSessionPresentation` does not classify off `status` alone: the `external_live` branch keys off `ownership` + `processLiveness`, `stale` keys off `processLiveness === 'gone'`, and `SessionPresentationInput.status` is a loose `string` because *"Runtime may still emit legacy / on_hold values not in SessionStatus."* So a session can present as **stale** or **external** while `status` is still `'running'` — and a raw-`status` gate would render a phase indicator beside a badge reading "Idle" or "External", the indicator contradicting the label it decorates. + + `deriveSessionPresentation` already returns `live: boolean`, and `SessionStatusBadge.tsx:47` already uses it to drive the `LiveDot` the indicator sits beside. Gating on it makes the contradiction **structurally impossible**, and it follows a rule the codebase states outright at `components/sessions/SessionCard.tsx:79-82` — *"name it here so the badge and the accessibility label agree on one answer rather than each deriving its own."* A raw-`status` gate would be a third derivation of liveness. + + It also agrees with the data by construction: external sessions have no streamer-owned PTY, so no scrape, so no phase. + + **Tree mode is deliberately out of scope.** `components/sessions/tree/TreeRow.tsx:63` renders its own `LiveDot` from an `isLive` prop; it is not a badge, so skipping it breaks nothing, but putting the indicator there would mean reconciling a third liveness derivation. +4. **Do NOT build client-side time decay.** The app has no time-decay precedent, and its established idiom is the opposite: the one thing mobile calls "stale" (`lib/sessionPresentation.ts:211-221`) keys off `session.processLiveness === 'gone'`, an explicit server enum. **The server decides liveness and says so; the client never infers it from a timestamp.** A freshness gate would also have nowhere to live — `sessionPresentation.ts` is a pure `session → presentation` function, and a pure function cannot decay: nothing re-invokes it when the clock crosses a threshold, so the label would clear only on the next unrelated re-render. Making it tick means a timer and a formerly-deterministic function becoming time-dependent. + + Follow the `processLiveness` idiom instead: the server clears `subStatus` explicitly at turn end (the must-fix above), and mobile renders it only while `status === 'running'`. No clock, no timer, no extra field. If a freshness gate is kept anyway, the plan must name where the tick comes from and accept that `sessionPresentation.ts` stops being pure. +4. **Theming cost is bimodal — pick a branch explicitly.** `colorForToken` is **module-private** to `components/sessions/SessionStatusBadge.tsx:18`, not a shared helper, and it is a closed switch over two closed five-value unions (`SessionColorToken` at `lib/sessionPresentation.ts:18`, `Theme.status` at `constants/theme.ts:24-30`). + - **Reuse an existing token** (everything running-ish renders `running`): ~zero extra work. + - **Give the phase its own colour**: extend the union, extend the `Theme` interface, add a switch branch, and add the colour to **every theme object — 17 of them**, including the three `appleGlassThemes` variants. Missing one is a type error, so it fails loudly rather than shipping invisible — which is exactly the failure mode `#3fb950` did not have. + + Either way: **never a hardcoded hex.** The app ships four light themes on which #647's pill was invisible. +5. Hide it whenever a question card is showing — including the PTY-scraped gate, not just the structured one. `components/conversation/ThinkingBubble.tsx:107-134` builds `card` from `activeQuestion ? … : questionBlock ? … : null` and returns card-only when set, so it genuinely covers both paths. Copy it; do not re-invent it. + +### There is nothing to delete — PR #647 was closed, not merged + +An earlier draft of this plan specified removing `lib/agentSubStatus.ts`, its test, `getTailLines` from `services/virtual-terminal.ts`, the `useTerminalStream` derive, and four locale keys including a byte-identical Russian `status.working`. + +**None of that exists on `origin/main`.** Verified: `git cat-file -e origin/main:lib/agentSubStatus.ts` fails, `getTailLines` appears zero times in `git show origin/main:services/virtual-terminal.ts`, and `"working"` appears zero times in all four `locales/*/sessions.json`. PR #647 is `CLOSED` with `mergedAt=null`. The draft described the abandoned branch's tree as if it were `main`. + +Two consequences: + +- **The deletion task is zero, not small.** +- **There is no ordering conflict with PlanCup.** The two plans do not touch a common line, so they land in either order, independently. + +The i18n rule still applies in the **forward** direction: if the server-side labels are new strings, `en` + `ar` + `he` + `ru` must be added in the same commit or `__tests__/i18n-completeness.test.ts` fails, and a key with no `t()` reference fails `i18n-unused-keys`. + +### Part B, ordered by risk + +The earlier "add a field and render it" estimate was true of exactly one item on this list. + +| # | work | notes | +|---|---|---| +| 1 | **Wire contract for clearing `subStatus`** | Blocking. Cross-repo, so it belongs in **Part A's field definition**, not here — otherwise the streamer ships a field mobile cannot turn off. | +| 2 | **Reconcile the cache handlers** | **Pre-existing bug, not a blocker for this field** — see below. Its own PR, and possibly a higher priority than this feature. | +| 3 | Add `subStatus` to `types/api.ts` | The only genuinely trivial item. | +| 4 | Render | Cheap if it reuses a colour token; ~20 files if it needs its own. | +| 5 | Question-card suppression | Cheap — the pattern exists and is correct. | +| ~~6~~ | ~~Deletion~~ | **Zero. The section was void.** | + +**Not part of this feature, file separately:** the stale-`provider` closure in `hooks/useTerminalStream.ts` — `provider` is read at `:104` and `:146` but omitted from the effect deps at `:291`. Verified. The same effect feeds the chrome filter. + +## Verification + +- Streamer unit tests for the pure derive over captured real screens, mirroring `__tests__/parse-status-line.test.ts`. +- Integration through a fake PTY, mirroring `__tests__/session-status-line.test.ts:54`. +- Explicitly test that a derive failure leaves the field absent and does not break the PTY path. +- **Exercise it with the pty-host flag on**, or the feature silently no-ops in that mode. +- Contract test: `subStatus` absent means mobile renders exactly as it does today. +- Mobile badge render tests, including that a stale `statusUpdatedAt` clears the indicator. +- `npm run test:i18n` in mobile — a dedicated CI job at `.github/workflows/test.yml:185`. +- **Update `docs/compatibility/tb-mobile.md`.** Its header requires it before any change to a response shape, status value, or WebSocket event. + +## Review log + +Each validation round is appended here so the reasoning survives the merge. + +### Round 1 — both reviewers, 2026-08-12 + +The draft survived on architecture and failed on almost every specific. Four corrections were load-bearing. + +- **The must-fix, found by applying the standing rule from PlanCup.** The draft cited `pty-manager.ts:1103-1108` as evidence for rejecting the elapsed counter, then never ran that input against the accepted design. It applies in full: a phase stamped and never cleared latches exactly as #647's pill did, one layer down. The clearing rule at `markReady` is now mandatory, and the client freshness gate is demoted to defence in depth. **This is the second time that rule has caught a defect the reviewers had already approved.** +- **The test-lock argument was simply wrong.** `parse-status-line.test.ts:61-64` asserts `parseStatusLine` extracts none of *its own three fields*; it says nothing about the line carrying signal, and this plan proposes a separate module. Worse, the rejected fixture reads `↑` (input tokens) while the derive keys on `↓` (output tokens) — a different sub-field. The blocker was overstated; the signal-class argument replaces it. +- **The plumbing list had a wrong item and a trap.** `server.ts:1050` is the *global* broadcast; the scoped `broadcastToClients` (`ws-hub.ts:74`) exists for exactly this message class. And the tempting shortcut — riding `onStatusChange`, which is already relayed across the pty-host boundary — routes a cosmetic field through `recordStatus`, which runs its update statement with no same-status guard: ~3 durable SQLite writes/second/session, plus APNs and push handlers. Rejected explicitly so nobody rediscovers it as a saving. +- **Part B's "add a field and render it" was false, and the deletion section described code that does not exist.** PR #647 is `CLOSED` with `mergedAt=null`, so `lib/agentSubStatus.ts`, `getTailLines`, the `useTerminalStream` derive and the four locale keys are all absent from `origin/main` — verified individually. The deletion task is zero, and there is consequently **no ordering conflict with PlanCup**. Meanwhile the real client cost is a three-way cache-semantics conflict (`LiveConversationView.tsx:143` replaces the whole object on the same key `app/_layout.tsx:165` merges into — a live bug today for `effort`/`permissionMode`) plus a clearing contract the plan had not specified. + +Process note on how the deletion error survived: an exploration pass that reads a feature branch and a plan written against `main` produce identical-looking file paths. The check that distinguishes them is `git show origin/main:`, not `ls`. + +Three further mobile corrections, all verified directly: + +- **The `statusSource`/`statusConfidence`/`statusUpdatedAt` trio is streamer-side only** — zero hits anywhere in mobile source. The draft cited it as an existing client-side liveness input; it is not one, and a freshness gate would have depended on a field mobile does not have. +- **There is no time-decay precedent, and the natural home cannot host one.** `sessionPresentation.ts` is pure, so it cannot decay without a tick source. The app's actual idiom is `processLiveness === 'gone'` — the server decides liveness and says so. The plan now follows that instead of inventing decay, which also removes the second field. +- **`colorForToken` is module-private**, not a shared helper, and both unions it bridges are closed at five values. A phase-specific colour is a ~20-file change across 17 theme objects. Recorded so nobody discovers it mid-implementation. + +**Citation audit.** Line-anchor drift has now occurred twice, so every `path:line` reference in both plans was extracted and machine-checked against the cited file. Three errors found and fixed: `pty-manager.ts:165-176` → `:42-43` for the geometry constants (`:165-176` is `createScreen()`'s body — the same right-quote-wrong-anchor slip corrected in PlanCup), `turnBusy` `:358` → `:362`, and the stale-`provider` effect deps `useTerminalStream.ts:182` → `:291`. Re-run `scratchpad/audit-citations.sh` before merge. + +### Round 2 — both reviewers, 2026-08-12 + +Three further defects, and they share a shape distinct from the one the standing rule catches. + +- **The mapper would have erased the clearing signal.** `managedToResponse` guards every optional field with `!= null`, which catches null and undefined alike — 19 instances, so a style-matching implementer writes it by default. Explicit-null becomes absence, the merge keeps the old value, and the label latches. This is #647's bug arriving a third way: not through a missing clear, but through the serialiser silently discarding the clear. Fixed with the unconditional-key precedent at `session-store.ts:280`, plus `Phase | null` rather than `Phase?`. +- **A `SessionResponse` frame would have re-rendered the app ~3×/second for an entire turn.** `elapsedMs` recomputes `new Date()` on every call for a live session (`session-store.ts:277`), so every phase frame differs from the last regardless of the phase, and mobile's spread produces a fresh object identity each time. Replaced with a bespoke `session_phase` frame, matching what the scoped path's existing tenants already do. +- **Absence must never carry meaning.** Always emit the field, `null` when there is no phase. An "absent = unknown, null = cleared" distinction is a third state that reintroduces the same failure. + +**The pattern worth keeping.** All three are the accepted design *inheriting a hazard from machinery it reused* — the status funnel's unconditional DB writes, the mapper's null-dropping idiom, the response builder's recomputed `elapsedMs`. The standing rule from PlanCup catches a different kind (an input used against a rejected option but never the accepted one). This kind needs its own question: + +> **What does the thing I am reusing already do on every call?** + +Both questions belong in the review checklist for anything that rides existing plumbing at high frequency. + +### Round 3 — mobile gate correction and the broadcast-path answer, 2026-08-12 + +- **The gate moved from raw `status` to `presentation.live`.** `deriveSessionPresentation` has branches that never consult `status` — `external_live` keys off `ownership` + `processLiveness`, `stale` off `processLiveness === 'gone'` — and `status` is a loose `string` by design. A raw-`status` gate could therefore render a phase indicator beside a badge reading "Idle" or "External". Gating on `presentation.live`, which the badge already uses for its `LiveDot`, makes that structurally impossible and follows the codebase's stated rule at `SessionCard.tsx:79-82`: one answer for liveness, not a third derivation. +- **`SessionPresentationInput` is standalone**, enumerating eleven fields rather than deriving from `Session`, so the field must be added there too. Adding it to `Session` alone would leave the derive unable to see it. +- **The staleness worry was unfounded** and the reasoning is worth keeping: `status` and `subStatus` travel inside the same object through all three cache paths, so they update atomically and cannot disagree. A timestamp and a status can drift apart; two fields of one object cannot. +- **The broadcast-path question resolved in favour of the existing ordering.** `session_update` carries `lifecycle` and `ownership` unconditionally — the two fields the mislabelling argument needed — and `processLiveness` has a single assignment site hard-coded to `"alive"`, so `"gone"` is never emitted. The cache replace is robustness work, not a live defect. + +**Both reviewers have signed off on PlanIndicator.** diff --git a/docs/plans/2026-08-12-viewport-relative-cursor-positioning.md b/docs/plans/2026-08-12-viewport-relative-cursor-positioning.md new file mode 100644 index 00000000..a1042f6b --- /dev/null +++ b/docs/plans/2026-08-12-viewport-relative-cursor-positioning.md @@ -0,0 +1,475 @@ +# PlanCup — make absolute cursor positioning viewport-relative (mobile) + +Single-repo change in **tb-mobile**: `services/virtual-terminal.ts` plus its unit tests. + +**Worktree — already created, branched from the current `origin/main`:** + +``` +/Users/ronenmars/dev/ai-tools/tb-mobile-worktrees/fix-viewport-relative-cup +branch: fix/viewport-relative-cup base: origin/main @ 86eeb2a1 +``` + +Work there, not in the main checkout. Worktrees live **outside** the repo root: a nested one is a full second copy of the tree that Jest, ESLint, TypeScript and Metro all walk into, which on 2026-08-01 produced two phantom test failures from a stale branch. +Rebase onto `origin/main` before opening the PR; do not branch from anything older. +`npm ci` has been run in the worktree — it needs a **real** `node_modules`, never a symlink to the main checkout's, or Metro silently bundles the main repo and the worktree's own lockfile drift stays invisible. + +This plan ships **alone**. It is justified entirely by a shipped transcript-corruption bug and makes no reference to the agent-status indicator. +The indicator plan ([2026-08-12-agent-sub-status-server-side.md](./2026-08-12-agent-sub-status-server-side.md)) does not depend on it either — the two are independent. + +## Goal + +`tb-mobile/services/virtual-terminal.ts` resolves absolute cursor positioning (`CSI H` / `CSI f`) against its whole append-only scrollback grid instead of against the 40-row viewport the TUI actually paints against. +Every footer repaint therefore lands in the middle of the transcript and overwrites conversation content. +Make absolute row addressing viewport-relative, and add the regression test the current suite structurally cannot express. + +## The bug + +`services/virtual-terminal.ts:243-248`: + +```ts +case 'H': +case 'f': + this.row = Math.max(0, (args[0] || 1) - 1) + this.col = Math.max(0, (args[1] || 1) - 1) + this.ensureRow(this.row) + break +``` + +`this.row` is an index into `this.grid`, which is append-only up to `MAX_ROWS = 10_000`. +A TUI paints absolute cursor moves against a fixed-height screen, and this streamer spawns every PTY at 120x40 — `src/pty-manager.ts:42-43` (Claude) and `src/codex-pty-runner.ts:36-37` (Codex). +Claude paints its footer at rows ~31-40 (`\x1b[40;1H`, `\x1b[35;3H`, `\x1b[38;3H` all appear in captures). +So once the grid is taller than 40 rows, `CSI 40;1H` addresses grid row 39 — deep inside scrollback — rather than the bottom of the screen. + +Measured against `origin/main`'s emulator, feeding N plain transcript lines and then one real footer repaint: + +| seeded lines | footer's distance from end | transcript rows destroyed | +|---|---|---| +| 0, 20, 35 | 0 | 0 | +| 41 | 2 | 2 lost + 1 mangled | +| 45 / 60 / 100 / 200 / 500 | 6 / 21 / 61 / 161 / 461 | 2 lost + 1 mangled | + +The mangled row is the visible symptom: `"transcript line 37"` becomes `"tr✻ Brewing… (12s · ↑ 3.4k tokens)"`. +A spinner ticks continuously through a turn, so this repeats for the whole turn. + +### It fires on every resume, not only on long sessions + +`terminal_replay` carries `PTYManager.getOutputLines(sessionId, 200)` — up to 200 rows of already-rendered, escape-free text (`src/server.ts:1184` → `src/pty-manager.ts:701-719`). +Mobile feeds that whole payload into the emulator through `feedHistory` (`hooks/useTerminalStream.ts:99-108`, called at `:187-203`), so after replay the grid is ~200 rows and `this.row ≈ 199`. +The *first live* `terminal_output` frame then carries raw CUP that lands at absolute rows 0-39: the very top of the replayed transcript. +The corruption is not "after 41 lines of output" — it is immediate, on every session resume. + +### This streamer already fixed the same bug class + +`src/pty-manager.ts:695-697`, above `getOutputLines`: + +> Claude's absolute-cursor repaints resolve to where they actually paint — unlike the old raw-byte slice, which scrambled order after a TUI repaint and made replayed conversations appear out of order on resume. + +And `src/pty-manager.ts:39-41`, on the geometry constants themselves: + +> PTY geometry. The headless render terminal (`session.screen`) MUST match these so Claude's absolute cursor moves (`ESC[;H`) resolve to the same screen coordinates the real TUI is painting against. + +The server solved it by rendering into a real 120x40 headless xterm.js. +Mobile's hand-rolled emulator is the remaining un-fixed half of the same problem. + +## What ALREADY exists — do NOT rebuild + +- **One emulator, one construction site.** `VirtualTerminal` is constructed only at `hooks/useTerminalStream.ts:51-54`, one per hook call. No other file in tb-mobile imports it. There is no second copy anywhere in the ecosystem — `tb-scanner` has no terminal code at all. +- **`ensureRow`'s trim needs no changes, and that is evidence *for* deriving the origin.** `services/virtual-terminal.ts:300-309` shrinks `grid.length` and `this.row` by the same `excess` when the grid passes `MAX_ROWS`. Both terms move together, so a derived origin rides along free. Do **not** read this as a task: an implementer who adds viewport state to maintain here has built the `viewTop` field that step 3 forbids. +- **The output filters are a three-layer stack, and it changes how the tests must be written.** `getRawLines()` (`:106-110`) drops empty rows **and** whole box-border rows (`BOX_BORDER_RE`, `:30`); `getLines()` (`:116-118`) drops provider chrome on top. So grid-row index is not output-line index — assert on content, never index. Critically, `lib/terminalChrome.ts:34` matches a *correctly placed* footer exactly, so it is filtered out of `getLines()`, while the mangled `tr✻ Brewing…` fails the `^` anchor and survives. **That asymmetry is why this bug is user-visible**, and it is what a naive "footer lands near the bottom" assertion gets wrong. +- **The two feed paths carry different data.** WS replay is escape-free rendered text. The HTTP fallback — query at `hooks/useTerminalStream.ts:81-97`, fed by the effect at `:111-129` — is the raw PTY byte ring buffer, cut with a byte-level `subarray` (`tb-streamer/src/pty-manager.ts:820-825`), so it can begin mid-escape-sequence *and* mid-UTF-8 codepoint. Both go through the same `feed()`. +- **Test house style exists** — module-level `const ESC` / `const CSI`, a `feedAndGet` helper, `describe('VirtualTerminal – ')` with an en dash, `expect(vt.getLines()).toEqual([...])`. The scrollback-cap suite re-declares `MAX_ROWS` locally rather than importing it. + +## The change + +### 1. Add the viewport origin + +```ts +// The TUI paints against a fixed-geometry screen — the streamer spawns every PTY +// at 120x40 (tb-streamer src/pty-manager.ts:42-43, src/codex-pty-runner.ts:36-37). +// Absolute cursor moves address that VIEWPORT, not the whole scrollback grid. +const VIEWPORT_ROWS = 40 + +private viewportTop(): number { + return Math.max(0, this.grid.length - VIEWPORT_ROWS) +} +``` + +### 2. Apply it to `CSI H` / `CSI f` — **clamped** + +```ts +this.row = this.viewportTop() + Math.min(Math.max(0, (args[0] || 1) - 1), VIEWPORT_ROWS - 1) +``` + +Measured effect: the footer's distance from the end of the grid becomes **3 at every seed** from 0 to 500, and at seed 500 the only row touched is the bottom-of-viewport row where a footer legitimately paints. + +**The clamp is not optional — without it this fix is a regression.** Real terminals clamp CUP to the screen. Unclamped, each out-of-range absolute move appends a screenful of blanks (`viewportTop() + 199` → `ensureRow` grows the grid → next origin is higher → repeat), and enough of them push the grid past `MAX_ROWS = 10_000`, at which point the trim starts evicting **real transcript from the top**. Measured — 200 seeded rows, N stray `CSI 200;1H`, counting surviving transcript rows: + +| strays | `main` today | fix, unclamped | fix + clamp | +|---|---|---|---| +| 0 / 1 / 5 / 20 / 60 / 61 | 200 | 200 | 200 | +| **62** | 200 | **79** | 200 | +| **70 / 100** | 200 | **0** | 200 | + +Today's code sets `row = 199` and grows nothing, losing nothing — so the unclamped fix is *strictly worse than the status quo* for this input. The HTTP fallback's byte-level `subarray` can begin mid-escape, which is exactly how a garbage row number gets in. + +#### The invariant + +State this in the file as an invariant, not as a consequence — it is the load-bearing property of the whole design: + +> `viewportTop() ≡ grid.length - VIEWPORT_ROWS`, so `viewportTop() + (VIEWPORT_ROWS - 1) ≡ grid.length - 1` whenever the grid is at least a screen tall. With the clamp, CUP's maximum target **is** the last row, so **CUP provably never grows the grid.** + +That is what makes it sound to read `viewportTop()` *before* `ensureRow` runs. Swapping those two lines silently breaks it, and nothing in the suite would catch the swap — so the comment is doing real work. + +Worked example, `CSI 40;1H` on a 200-row grid: `viewportTop()` = 161 (read first), `row` = 161 + 39 = 200, `ensureRow(200)` finds `201 <= 200` false and does not grow. Lands on the last existing row. + +### 3. Apply it to `CSI A` / `CSI B` too + +Both are measured corruption in the same class, and both are two lines. + +```ts +case 'A': this.row = Math.max(this.viewportTop(), this.row - n); break +case 'B': this.row = Math.min(this.row + n, this.viewportTop() + VIEWPORT_ROWS - 1) + this.ensureRow(this.row); break +``` + +- **`CSI A` clamps at grid row 0**, so cursor-up from the footer region walks into scrollback and the next `putChar` overwrites transcript. Measured on a 200-row grid after `CSI 40;1H`: `CSI 45A` destroys `transcript line 155` — five rows *above* `viewportTop()` — and `CSI 100A` lands the cursor 99 rows from the end, deep in scrollback. +- **`CSI B` grows the grid from a cursor move**, shifting the derived origin mid-frame. Measured: painting `CSI 38;1H AAA`, then `CSI 10B`, then `CSI 38;1H BBB` — the *same* absolute row — leaves `AAA` at 2-from-end while `BBB` lands at 0. The frame tears. + +**Verified cost: zero test churn.** The `A` tests (`virtual-terminal.test.ts:45`, `:464`, `:500`, `:503`) and the `B` test (`:56`) all run on 1–3-row grids, where `viewportTop()` is 0 and behaviour is byte-identical. + +### 4. The viewport origin is DERIVED, never stored + +Add this as a comment in the file, and treat it as the invariant the whole design rests on: + +> The viewport origin is **derived**, never stored — `grid.length` is the single source of truth. Do not introduce a `viewTop` field. The model holds as long as `grid.length` changes only by appending at the bottom. + +This is what makes the narrow scope safe. Because `viewportTop()` is recomputed from `grid.length` at each absolute move, any prior splice is already absorbed by the time the next one resolves, so the out-of-scope handlers cannot desynchronise it. Measured — 100 seeded rows, one mid-op, then a footer repaint: + +| mid-op | footer distance from end | scrollback lost | +|---|---|---| +| none (control) | 1 | 0 | +| `CSI 5S` | 1 | 5 — pre-existing, out of scope (see below) | +| `CSI 5T` / `CSI 5M` / `CSI 5L` / `CSI 10B` | 1 | 0 | + +A **stored** `viewTop` would have to be maintained correctly in every splice, and each out-of-scope op becomes a way to desynchronise it — which is precisely the two-mental-models trap. Derived state has no such failure mode, and there is nothing to clear in `reset()` (`:141-149`), because it already sets `grid = [[]]`, which *is* the origin. + +#### The newline hazard, resolved by measurement + +The strongest theoretical objection to a derived origin: `feed()` grows the grid unconditionally on `\n` (`:75-79`), so a `\n` landing between two absolute moves of the same frame would make `viewportTop()` return a larger value for the second move than the first, tearing the frame in half. + +Measured against the prototype, 200 seeded rows in every case: + +| scenario | result | +|---|---| +| A. three-row frame, no interleaved newline (control) | rows land 2 / 1 / 0 from end | +| B. same frame with a `\n` injected between the absolute moves | 2 / 1 / 0 — **no drift** | +| C. frame split across three separate `feed()` chunks (the real WS case) | 2 / 1 / 0 | +| D. output newlines arriving between chunks | superseded frame's row drifts; the current frame still lands at 0 | +| E. five grow-then-repaint cycles | footer 1 row from end; **one** seeded row lost | + +The hazard does not materialise, because a `\n` advances the cursor as well as the grid: the next absolute move recomputes the origin against the new length, so the frame's rows stay consecutive relative to the current bottom. This is the property a stored origin would not have. + +Case E's single lost row is `transcript line 199` — the bottom-most seeded row, which a footer legitimately paints over because it is inside the viewport. It is an artifact of a synthetic seed that never scrolls, not a defect. Compare `main`, which loses two rows and mangles a third *on every repaint*. + +### Rewrite the `MAX_ROWS` comment + +`services/virtual-terminal.ts:15-19` currently reads: + +> Kept well above any TUI screen height so absolute cursor positioning (H/f) never hits the trim. + +That is the reasoning that produced this bug: it keeps CUP away from the *trim* while leaving CUP addressing the wrong *rows*. Replace it. + +## Scope: the cursor-moving handlers — `H`, `f`, `A`, `B` + +Two independent principles converge on the same boundary, which is why it is worth stating both. + +**Structural — this is the primary rule.** Handlers that only **move the cursor** become viewport-relative; handlers that **mutate grid structure** do not. After `A`/`B` are clamped, no cursor move can change `grid.length`, so the only remaining growth is `\n` at the bottom — a genuine screen scroll. `L`/`M`/`T`/`J` would need a viewport *bottom* and a scroll-region model, not just an origin. + +The rule is **complete over the handlers that exist today**: the row-affecting cursor moves are exactly `A`, `B`, `H`, `f`, and all four are in scope. `C`, `D`, `G` are column-only, so the viewport is irrelevant to them. `ESC 7`/`8` and `CSI s`/`u` are unimplemented — whoever adds saved-cursor state joins it to the cursor-moving set. + +**Measured impact — the secondary rule, used as a severity gate** for what a follow-up is worth doing. Not "does a test have to change," which separates nothing, since `CSI S` and `CSI 2J` both have tests asserting their broken behaviour. + +The two rules do not conflict anywhere here: `A`/`B` are cursor-moving *and* measurably corrupting (in by both), `S` and `2J` are grid-mutating *and* unverified (out by both). + +| defect | positioning impact | scrollback impact | verdict | +|---|---|---|---| +| CUP `H`/`f` | measured; continuous; every turn, every resume | measured | **in** | +| `CSI A` | measured — reaches 5+ rows above `viewportTop()` | measured | **in** | +| `CSI B` | measured — tears a frame mid-paint | none | **in** | +| `CSI S` | **none** — measured | real, frequency unverified | out | +| `CSI 2J`/`3J` | none | total, frequency unverified; `2J`-vs-`3J` semantics unsettled | out | + +**Result: 152/152 green across all eleven terminal-related suites, `tsc` clean, zero test rewrites.** + +### `CSI S` is out, and the reason it was nearly in was a false claim + +An earlier draft of this plan asserted that `:285-288` "never adjusts `this.row`, so the cursor silently points at a different logical line afterwards." **That is wrong.** Measured with identical viewport-relative CUP in both variants — 60 rows, `CSI 30;1H`, `CSI nS`, then a write: + +| `CSI S` implementation | content rows clobbered, n=1 | n=5 | +|---|---|---| +| current (splice grid top) | `line0`, **`line51`** | `line0`…`line4`, **`line55`** | +| push blanks + `row += n` | **`line51`** | **`line55`** | + +Both land the write on the *same* row: the splice's implicit shift compensates exactly, so the current code is internally consistent on the positioning axis. `CSI S`'s only real defect is destroying the `n` oldest scrollback rows — and **nobody has verified that Claude's TUI emits SU at all.** Unverified frequency puts it in the same bucket as `2J`, and the structural boundary excludes it independently: `S` mutates grid structure. + +**Severity if it does fire, for whoever picks up the follow-up.** The mobile terminal renders a *scrollback transcript*, not a 40-row screen — `components/terminal/TerminalOutput.tsx:286-297` feeds a FlashList with `maintainVisibleContentPosition` and an explicit scroll-to-top button at `:317`. So `splice(0, n)` deletes history the user can physically scroll to, and it does so **invisibly**: the splice shrinks `grid.length` by `n`, so the derived origin shifts by `n` too, the viewport shows identical content, and the scroll never appears. Silent, unrecoverable, detectable only by absence. Do not let the follow-up treat this as cosmetic. + +Leave a one-line comment on the `S` case pointing at its follow-up, so the file does not silently hold two models with nothing marking the boundary. Cheap mitigation for the legibility concern; costs no test. + +**The one contingency that would flip this.** If Claude's TUI does emit SU, `CSI S` is live transcript loss of the same class as the headline bug and belongs in this PR. One grep for `ESC [ S` over a real captured PTY chunk log settles it. **Run it before implementing; if SU appears, pull `CSI S` back in.** + +**Getting a capture is a one-liner, not a blocker.** Neither repo has raw PTY captures checked in — `e2e/fixtures/terminal-output.json` is a 3-line synthetic with zero escape bytes — but `GET /api/sessions/:id/output` returns `session.outputBuffer` **verbatim**, the unrendered raw ring buffer (`tb-streamer/src/server.ts:5484-5493` → `src/pty-manager.ts:687-691`). One curl against any live session mid-turn produces a genuine capture with no new instrumentation. The same artifact answers the frequency question for `L`, `M`, `T`, `0J` and `2J` as well — one capture for the whole follow-up family. + +**The follow-up issue must lead with the frequency check, not the fix — and must not sit indefinitely.** The invisibility finding above changes how this defect can ever be resolved. A deferred issue for a *visible* bug gets closed when a user hits it and files a report; an invisible one has no such route, because neither the user nor plan step 6 can observe it. So: grep first, and if nobody can produce a capture, **that absence is itself the answer** — apply the append-blanks fix on the structural-boundary grounds rather than waiting for evidence that has no way to arrive. That keeps the deferral honest instead of turning it into a permanent shelf. + +## Explicitly OUT of scope + +Every one of these is a real defect in the same file and the same family. None is fixed here; each gets a follow-up issue so the diff stays reviewable and the regression risk stays bounded. + +- `CSI S` (`:285-288`) splices the grid's oldest rows instead of appending blanks at the viewport bottom, destroying scrollback. **Not a cursor bug** — do not carry that claim into the follow-up issue. +- `CSI 2J` and `CSI 3J` are treated identically as total grid annihilation (`:249-260`). A real terminal clears only the viewport on `2J`; `3J` clears scrollback. Two existing tests assert the destructive behaviour (`virtual-terminal.test.ts:490`, `:556`), and the `2J`-vs-`3J` split is itself unsettled. +- `CSI 1J` (erase from start of screen to cursor) falls through the `if/else if` silently, with no counter increment. +- `CSI 0J` truncates the grid to `row + 1`, permanently deleting scrollback below the cursor. +- `CSI r` (DECSTBM) is parsed and discarded (`:295-296`); no scroll region is modelled, so `S`/`T`/`L`/`M`/`\n` all operate on the whole grid. +- `CSI L` / `M` / `T` splice absolute scrollback with no viewport bottom. +- `ESC 7` / `ESC 8` / `ESC M` (DECSC/DECRC/RI) are silently dropped at `:193-194`, and `CSI s` / `u` are in `IGNORED_CSI` — there is no saved-cursor state in the class at all. +- **A CUP split across a `feed()` boundary is dropped and printed as literal text.** Measured: `feed('\x1b[40;')` then `feed('1HFOOTER')` yields the row `"1HFOOTER"` with `truncatedEscapeCount` 1, because `parseCSI` (`:206-209`) discards accumulated params on a short chunk and only a bare trailing ESC survives via `pendingEsc`. Pre-existing and unchanged by this fix; it degrades to visible junk rather than corruption. Worth knowing, because escape-free-vs-raw-bytes is not the only chunking axis. + +**The boundary is not hermetic — one consequence to expect.** `CSI 0J` (`:254-259`), `K`, `L` and `M` all read `this.row`, so their behaviour on grids taller than a screen changes as a *consequence* of this fix even though they are out of scope. Most visible: `\x1b[H\x1b[J` today truncates the grid to one row (total transcript loss); afterwards it truncates to `viewportTop() + 1`, keeping scrollback. That is an improvement, but it is an untested behaviour change and belongs here rather than being discovered in review. The existing `0J` test (`:92-98`) is on a 3-row grid and is unaffected. + +## Settled questions + +**Is the narrow scope right? — Yes: the four cursor-moving handlers, conditional on the derived origin and the clamp.** +A derived origin is self-correcting across every out-of-scope op (measured above), so the two-mental-models risk is real only if the origin is *stored* — which step 4 forbids. The structural boundary and the measured-impact test independently pick the same four handlers; see the scope table. + +**Where does 40 come from? — Hard-code it. Do not put geometry on the wire.** + +The instinct to eliminate the duplication is wrong here, for a documented reason. `src/pty-host/protocol.ts:38-41`: + +> Deliberately absent: `resize`. The plan lists it, but PTY dimensions are the fixed `PTY_COLS`/`PTY_ROWS` constants and `SessionRunner` has no resize method, so the verb would be one nothing could ever send. Add it with the caller that needs it. + +Geometry is structurally constant. A wire field would carry a value that cannot vary, guarding a scenario the protocol has explicitly declined to support. The glyph-class drift in the abandoned PR #647 happened because two copies of a fact that *did* vary drifted; 40 cannot drift while resize does not exist. + +Two corrections to the premise, worth recording so nobody re-opens this: + +- `PTY_HOST_PROTOCOL_VERSION` (`protocol.ts:48`) governs the **streamer↔pty-host IPC** boundary. Mobile never sees it. A `rows`/`cols` field on `terminal_replay` would be a streamer↔mobile WS change — additive-optional, no bump. The indicator plan's version bump does not subsidise this one; they are different boundaries. +- `PTY_COLS`/`PTY_ROWS` are plain non-exported `const`s in two files. Even the wire approach would need an export and a shared constant first. + +**Wiring it would not even remove the constant.** Mobile must keep a fallback for streamers that do not send geometry, so the wire option is the hard-coded constant *plus* a negotiation path — and it would block a shipped-corruption fix on a cross-repo release. This is the objection that comes back, so it is recorded here rather than re-argued. + +**Failure direction, which decides how bad a wrong constant is.** Too large → origin too low → today's bug at reduced scale. Too small → **with the clamp** → footer rows pile onto the bottom row instead of corrupting transcript. The clamp is what makes a wrong constant *degrade* rather than *corrupt*, and that is half the justification for hard-coding at all. + +So: hard-code `VIEWPORT_ROWS = 40` with a comment naming both streamer constants, and make the follow-up **conditional rather than scheduled** — whoever adds resize adds the wire field in the same change, which is the policy `protocol.ts:38-41` already states. That attaches a trigger to the risk instead of leaving an open TODO nobody actions. + +**Rejected: deriving the origin from the highest row an absolute move has addressed.** Two measured failure modes. +*Multi-row repaint collapses* — a footer painted top-down in one chunk (rows 31, 35, 38, 40) sees the derived height rise as the repaint proceeds, so every row lands at the current bottom and overwrites the last; three of four footer rows are destroyed. +*One stray escape poisons it permanently* — a single `CSI 200;1H` puts the footer 61 rows from the end forever, because the tracked maximum is monotonic with no recovery path. The HTTP fallback path can begin mid-escape-sequence, which is exactly how a garbage row number gets in. + +## Verification + +1. **New regression test seeding >40 rows before a footer repaint.** This is the property the current suite cannot express. All five existing CUP tests (`virtual-terminal.test.ts:79`, `:95`, `:143`, `:490`, `:556`) have at most 3 grid rows at the cursor move, so `viewportTop()` is 0 and they are provably byte-identical under this change. No test in the repo combines a large grid with any cursor operation — that is the coverage hole that let this ship. +2. **Three assertions, not one — and shape the fixture like the real trigger.** "The footer lands within the last few rows" *fails after the fix* if written against `getLines()`, for the wrong reason: `lib/terminalChrome.ts:34` matches a correctly-placed footer exactly and filters it out, while the mangled hybrid survives the `^` anchor. Split it: + - on `getLines()`: every seeded transcript line still present **and** no line matches `/Brewing/`. Fails today (two lost, mangled hybrid present), passes after. **This assertion carries the test.** + - on `getRawLines()`: footer text among the last few entries. + - invariant: `CSI 40;1H` on a >40-row grid does not change `getRawLines().length`. + + Build the fixture as the production path does: 200 escape-free rows joined with `\n` — what `feedHistory` receives at `useTerminalStream.ts:202` — then one live frame carrying `\x1b[40;1H`. +3. **A clamp test — and not the obvious one, which discriminates nothing.** The naive assertion ("`CSI 200;1H` on a >40-row grid leaves `getRawLines().length` unchanged") **passes on clamped and unclamped code alike.** Verified by building both variants and running it: unclamped appends ~160 *blank* rows, and `getRawLines()` drops empty rows, so the growth is invisible to the very assertion meant to catch it. The plan's own warning about `getRawLines()` applies to the guard test itself. + + Assert the actual harm instead: **70 repetitions**, past the eviction cliff at 62, where the grid crosses `MAX_ROWS` and the trim evicts real transcript. Measured — clamped 200/200 survivors, unclamped **0/200**. +4. **Validate the constant itself, not just the implementation.** Every other step tests the code against the assumption that the viewport is 40 rows. Feed a **real captured PTY log** and assert the footer rows land contiguously at the bottom. A wrong constant shows up immediately as a fixed offset. Since the constant is hard-coded, this is the check that earns it. +5. **The HTTP fallback path, concretely.** Feed a chunk that *begins inside a CSI* — the byte-level `subarray` at `tb-streamer/src/pty-manager.ts:820-825` can start mid-escape and mid-UTF-8 — and assert only that nothing throws and no transcript row is lost. Note `useTerminalStream.ts:77` swaps to `getRawLines()` at low parse confidence, the likely state on that path. +6. **Run the consumer suites, not just `--testPathPattern "virtual-terminal"`.** That pattern is a path regex and matches no consumer test — it excludes `__tests__/unit/hooks/useTerminalStream.{emptyReplay,seq,userMessages,watchdog}.test.tsx`, `__tests__/integration/components/TerminalView.test.tsx`, `__tests__/unit/components/terminal/`, and `__tests__/unit/utils/terminalSession.test.ts`, and `emptyReplay` exercises the very replay path this plan names as the production trigger. Use `--testPathPattern "useTerminalStream|TerminalView|terminal"`. Prototype baseline: **152 passed across 11 suites**, `tsc --noEmit` clean. +7. `npx tsc --noEmit --pretty false` and `npx eslint` on the changed files. +8. On device: resume a long session, let a turn run, and confirm the top of the transcript survives the spinner ticks. + +**Standing rule, earned the hard way on this plan: any input used to reject an alternative must be run against the accepted design.** +The unclamped-CUP regression survived two reviewers because the stray `CSI 200;1H` was used adversarially against the *rejected* track-highest-row option and never against the *accepted* one. The asymmetry was in the testing, not in the designs — and the accepted design turned out to be strictly worse than `main` for that exact input. This rule generalises well past this PR. + +**Expect a fixture artifact, do not chase it.** With a synthetic plain-line seed, footer rows interleave with surviving transcript rows (32-34, 36-37, 39 keep old content) because the seed never clears them. That is the fixture, not a bug — one more reason the headline test should use a real capture. + +## Review log + +Each validation round is appended here so the reasoning survives the merge. + +### Round 1 — streamer-side review, 2026-08-12 + +Verdict: approve with changes. Six applied, three load-bearing. + +- **Step 4 inverted.** The original text asked for stored viewport state maintained through `ensureRow`'s trim and `reset()`. That is exactly what would create the two-mental-models incoherence the scope question worried about. Replaced with the derived-origin invariant, backed by a measured table showing positioning survives every out-of-scope op. +- **`CSI S` primitive replaced.** The original "splice grid top + adjust `this.row`" fixes the cursor but leaves the scrollback destruction. Re-measured independently: splice loses 6 scrollback rows, push-blanks loses 0. +- **Wire-geometry option rejected on documented grounds** — `protocol.ts:38-41` deliberately omits `resize`, so geometry cannot vary; the follow-up is now conditional on resize landing rather than a scheduled issue. Two premise errors corrected: `PTY_HOST_PROTOCOL_VERSION` is the IPC boundary and invisible to mobile, and `PTY_COLS`/`PTY_ROWS` are non-exported consts. +- **Track-highest-row option rejected** with two measured failure modes: multi-row repaints collapse onto one row, and a single stray `CSI 200;1H` poisons the origin permanently with no recovery path. +- **Citation corrected** — the "old raw-byte slice" comment is at `pty-manager.ts:695-697`, not `:174-176` (which is `createScreen()`'s body). Verified directly. Geometry comment at `:39-41` added as a second reference. +- **Verification widened** — the narrow `--testPathPattern` matched no consumer test; added full-suite runs, a test that validates the constant itself against a real capture, and a `CSI S` test. + +The single-construction-site claim was independently re-checked and holds: `app/session/_layout.tsx:10` and `utils/stripBoxDrawing.ts:6` mention `VirtualTerminal` only in comments, so `hooks/useTerminalStream.ts:53` really is the only one. + +### Round 2 — prototype + scope ruling, 2026-08-12 + +Prototyping the round-1 plan in the mobile worktree found a contradiction, and chasing it overturned a claim both reviewers had accepted. + +- **The prototype broke a test**, `virtual-terminal.test.ts:116-121`, which asserts `line1` is *destroyed* by `CSI 1S`. That put `CSI S` in exactly the position `2J` was excluded from, so the stated scope principle no longer separated them. +- **Chasing it showed the `CSI S` cursor claim was false.** Measured with identical CUP in both variants: old and new clobber the *same* content row (`line51` at n=1, `line55` at n=5); the splice's implicit shift compensates exactly. The plan had asserted a cursor bug that does not exist, and round 1 endorsed it. `CSI S`'s only defect is scrollback destruction, at unverified frequency. +- **Scope principle replaced.** "Does a test have to change" separates nothing; **measured impact** does, and it drops `CSI S` and `2J` into the same bucket without any restatement. `CSI H`/`f` alone. +- **Result: one behavioural change, zero test rewrites, 82/82 on the emulator suites and 152/152 across all eleven terminal suites, `tsc` clean.** +- **The newline hazard was measured and does not exist** (table under step 3). It was the strongest theoretical argument against a derived origin. +- A replacement assertion for the deferred `CSI S` work is recorded below so the follow-up does not have to re-derive it. Note the trap: asserting the cursor holds its screen row passes under *both* implementations and discriminates nothing. + +```ts +it('scroll up (CSI S) pushes rows into scrollback instead of deleting them', () => { + const vt = new VirtualTerminal() + vt.feed('line1\nline2\nline3') + vt.feed(`${CSI}1S`) + expect(vt.getLines()).toEqual(['line1', 'line2', 'line3']) +}) +``` + +Verified: `['line1','line2','line3']` corrected, `['line2','line3']` today. A 3-row grid pins only scrollback survival; when the follow-up lands, test it where the viewport is real (50 rows, `CSI 40;1H`, `CSI 5S`) and assert survivor count plus that `line 0` is still present. + +### Round 3 — mobile-side review, 2026-08-12 + +The mobile reviewer converged independently on all three round-1/2 changes, then found a defect neither the streamer reviewer nor I had caught. + +- **Ship-blocker: the unclamped CUP was a regression.** Each out-of-range absolute move appended a screenful of blanks; past 62 strays the grid crossed `MAX_ROWS` and the trim evicted real transcript. Independently reproduced: 62 strays → 79 of 200 rows survive, 70 → 0, against 200 on `main`. **The plan was using a stray `CSI 200;1H` to reject one alternative while leaving the accepted design open to the same input.** Clamp added; 200/200 at every stray count. +- **`CSI A` and `CSI B` pulled into scope**, both measured. `CSI 45A` from the footer region destroys a row five above `viewportTop()`; `CSI 100A` lands the cursor 99 rows from the end. `CSI 10B` mid-frame makes the same absolute row resolve to two different places, tearing the frame. Zero test churn — every `A`/`B` test runs on a 1–3-row grid. +- **The scope boundary is now principled rather than post-hoc.** Cursor-moving handlers become viewport-relative; grid-mutating handlers do not. This independently excludes `CSI S`, agreeing with the round-2 measured-impact ruling — two different tests, same four handlers. +- **Verification item 2 would not have caught the bug for the right reason** — `terminalChrome.ts:34` filters a *correctly placed* footer out of `getLines()` while the mangled hybrid survives the `^` anchor. Split into three assertions. +- **Stale round-1 bullet deleted** — "any new viewport state must be decremented in `ensureRow`" survived the rewrite and directly instructed the implementer to build the `viewTop` field step 4 forbids. +- Added: the non-hermetic-boundary consequence (`0J`/`K`/`L`/`M` read `this.row`, so `\x1b[H\x1b[J` changes from total transcript loss to keeping scrollback), the feed-boundary CUP truncation, the failure-direction note, and the consumer evidence for `CSI S` severity. +- Citation nit fixed: the HTTP fallback is the query at `useTerminalStream.ts:81-97` but the feed is the effect at `:111-129`. + +### Round 4 — streamer-side re-review, 2026-08-12 + +Approved for commit. All three round-3 findings independently reproduced: the stray-CUP table matched at every count, `CSI 45A` destroyed a transcript row above `viewportTop()`, and the `CSI 10B` frame tear showed the same absolute row resolving two rows apart. Zero test churn re-confirmed against `virtual-terminal.test.ts:41` and `:52`. + +Three documentation requirements applied: the invariant stated as an invariant, the adversarial-input rule added to verification, and the follow-up reframed to lead with the frequency check. The structural boundary is adopted as the **primary** scope rule with measured-impact demoted to a severity gate, and the boundary was checked complete over the handlers that exist today. + +The `CSI S` deferral holds — impact is severity × frequency, and the consumer evidence raises only severity. But the reviewer's own diagnosis of how the clamp regression slipped through is worth preserving verbatim, because it is the process lesson rather than the code one: + +> The failure wasn't that I missed an input — it's that I tested the *rejected* option adversarially and the *accepted* one only on well-formed data. The asymmetry was in my testing, not in the designs. + +### Round 5 — mobile-side sign-off, 2026-08-12 + +Accepted the `CSI S` deferral and withdrew the objection. Its own reasoning, worth keeping because it is the cleanest statement of why the boundary holds: + +> I proposed the boundary … and then immediately carved out an exception for the one grid-mutating handler I happened to like. That is special pleading, and the tell is that my justification for the exception ("pure data loss, unambiguous semantics, 3 lines") never once referenced the boundary. It was a severity argument wearing a scope argument's clothes. + +It also confirmed the deferral is **safe, not merely defensible**: leaving the old `splice(0, n)` in place cannot destabilise the derived origin, because the splice shrinks `grid.length` and every absolute index by the same `n`, so a subsequent `viewportTop() + k` addresses the same content. That is the same compensation that falsifies the positioning claim — the two findings are one fact seen from opposite sides. + +Final state independently re-verified on the worktree: all three handlers as specified, `tsc --noEmit` clean, 11 suites / 152 tests green. + +**Both reviewers approved. Ready to commit.** + +### Round 6 — implementation, 2026-08-12 + +Implemented as specified. Two of this plan's own verification steps turned out to be wrong, both for the same reason. + +- **The clamp test specified in verification item 3 discriminated nothing.** Independently confirmed by building clamped and unclamped variants: the naive length assertion passes on both, because unclamped growth is *blank* rows and `getRawLines()` drops those. Rewritten to 70 repetitions past the eviction cliff — 200/200 survivors clamped, **0/200** unclamped. +- **Item 2's third assertion was unsatisfiable as written.** "`CSI 40;1H` does not change `getRawLines().length`" is false once the footer is painted: the move targets a blank bottom row, which only becomes visible after it has content, so the count legitimately rises by one. Assert the invariant on the *move alone*, then paint, then assert content. + +Both are the `getRawLines()` filter blind spot — the same property this plan warns about for the regression test, applied to the guard tests. The general lesson: **a test written against a filtered view can be blind to exactly the growth it is meant to detect.** + +The practice that caught it, stated the way the implementer corrected it rather than the flattering way: **run the broken variant *before* deciding what a test proves, because the label a green run suggests is frequently wrong.** The `CSI B` test was nearly shipped labelled a regression test on the strength of a green run; it is actually a guard — it passes pre-fix, because pre-fix saturates rather than growing. That was found by accident, not foresight. Written as "verify your tests fail correctly" this sounds like generic diligence; written as a step performed *before* labelling, it is a specific action with a specific failure it prevents, which is the form that survives contact with someone in a hurry. + +**Build the variant from the final source, not from `main`.** Comparing pre-fix against post-fix confounds the clamp with everything else that changed. Comparing clamped against unclamped *built from the same final file* isolates the one variable, and it is what makes the 200/200-vs-0/200 result attributable to the clamp rather than to anything else in the diff. + +Which tests discriminate, verified individually rather than assumed: + +| test | pre-fix | H clamp removed | B clamp removed | +|---|---|---|---| +| footer lands at bottom | FAIL | — | — | +| cursor-up (`CSI A`) clamp | FAIL | — | — | +| out-of-range row clamp | pass | **FAIL** | — | +| cursor-down (`CSI B`) clamp | pass | — | **FAIL** | +| chunk begins inside an escape | pass | pass | pass | + +Two reproduce the original bug, two guard clamps against being dropped in review, one is a no-throw contract test. The comments say which is which so nobody later reads a guard as a regression test. + +### Round 7 — validated against a real capture, 2026-08-12 + +A capture was produced: Claude Code **v2.1.228**, spawned by the streamer at its real 120x40, one complete turn, 1,123,773 bytes, durable at `~/.threadbase/captures/` with a provenance README. + +**Verification item 4 is DISCHARGED, and the fix is confirmed correct against a reference emulator.** The capture was rendered three ways — by `@xterm/headless` at 120x40 (the emulator this streamer itself trusts for replay, so its rendering *is* the correct answer), by `origin/main`, and by the fix: + +| emulator | visible rows | footer distance from end | +|---|---|---| +| `@xterm/headless` @120x40 — **reference** | 38 | 5 | +| **with the fix** | **37** | **4** | +| `origin/main` | 64 | 31 | + +The fix reproduces the reference; `main` does not. The single-row delta is the `────` separator row, which mobile's `BOX_BORDER_RE` filter deliberately drops — fully accounting for the difference. Had `VIEWPORT_ROWS` been wrong, the two would diverge by the error. + +Corroborating: **CUP targets exactly two absolute rows in the whole turn — 37 and 40**, 1456 times each. A 40-row screen is the only geometry consistent with that, and nothing addresses a row beyond 40. + +*Caveats, scoped per statistic — this matters, see below:* the raw file is snapshot-concatenated, so **frequencies are inflated**. But **the maximum and the distinct set are exact**: replaying bytes cannot introduce a row that was never addressed, nor raise a maximum. `max = 40` is therefore not a weak number, it is the strongest one here, and it is what the constant rests on. Confirmed per-snapshot independently of the concatenation. The emulator comparison is unaffected either way — all three received identical input. The separate, real caveat is one turn with no tool use. + +**A garbled tail appears in the output and is not a defect.** Rows like `"❯ 7Gof ed list witspatial dimensions…"` appear identically in the `@xterm/headless` rendering, so they are an artifact of overlapping snapshots in the capture, not of any emulator. + +### The CSI histogram settles the deferred family + +Measured over the same capture, with a positive control: + +| final | count | consequence | +|---|---|---| +| `G` | 51327 | column addressing, unaffected | +| `B` | 10075 | **load-bearing** — the `B` clamp matters | +| `C` | 8222 | column, unaffected | +| `m` | 8213 | SGR, ignored by design | +| `H` | 4395 | the fix's primary target | +| `K` | 3805 | line erase | +| `J` | **30** | **all `2J`** — see below | +| `r` | **30** | all bare `ESC[r` — see below | +| `A` `D` `f` `L` `M` `S` `T` | **0** | never emitted | + +#### Ask per statistic whether it is sensitive to how the file was assembled + +Not "trust this table" or "distrust it" — the answer differs *between statistics on the same line of evidence*: + +| statistic | sensitive to snapshot duplication? | +|---|---| +| frequencies (the counts below) | **yes** — inflated, never read as traffic | +| maximum, distinct set (e.g. `max row = 40`) | **no** — exact; replay cannot add a value or raise a max | +| presence / absence | **no** — reliable | + +This is the mirror of the mistake that produced the whole correction chain. First all three of us over-trusted counts that duplication had inflated; then the caveat written to fix that was applied so broadly it would have discounted `max = 40`, the one figure the constant rests on. **Under-trusting is the quieter failure** — it never produces a wrong answer, it just discards a right one, and nobody goes back to check. + +`turn.raw` is a concatenation of 36 snapshots, each a **full ring-buffer dump**, so every snapshot replays the session's startup bytes. Verified: each `snap-NNN.raw` contains exactly one `CSI 2J`, all at the same offset 59, and the first gap between `2J` occurrences in `turn.raw` is 4742 bytes — precisely `snap-000.raw`'s length. + +**So every count of ~30 in this table is one startup event replayed once per snapshot, not thirty events during the turn.** Counts in the thousands are inflated by the same overlap. Absence is still absence and a maximum is still a maximum, which is why the item-4 and `CSI S` results survive; nothing else about magnitude does. + +- **`CSI S` is never emitted.** The deferral was correct, and its follow-up can be downgraded or closed. `L`, `M`, `T` likewise. +- **`CSI A` is never emitted**, so the `A` clamp that shipped is defensive rather than reachable — the same category as `CSI S`, except that fixing it cost nothing and it rode along free. `CSI f` likewise: only `H` is used. Worth knowing before anyone treats the `A` test as a regression test rather than a guard. +- **`CSI 2J` does NOT fire 30 times per turn — it fires once, at startup, on an empty grid.** An earlier revision of this entry read the union count as mid-turn traffic and promoted the follow-up on that basis. That was wrong. Startup `2J` on an empty grid is harmless, so `2J` stays deferred at its original priority, and its frequency remains genuinely unverified pending a capture that is a true byte stream. +- **`CSI r` is always bare `ESC[r`** — *reset* margins to full screen, not a region setup. **Close this follow-up rather than promoting it.** The emulator models only the full screen, so `case 'r': break` reaches the correct end state. No parameterised `r` appears anywhere. +- **`ESC 7` / `ESC 8` (DECSC/DECRC) are the same artifact, not a separate finding.** A draft of this entry recorded them as "the one deferred item confirmed reachable." They are not. The first twelve bytes of every snapshot are `ESC7 ESC[r ESC8 ESC[?25…` — an eight-byte startup restore that *brackets* the `CSI r`. All three are one event. + +#### The corrected picture + +``` +during the turn: G 51327 B 10075 C 8222 m 8213 H 4395 K 3805 +startup only (1x): r J c ESC7 ESC8 +never emitted: S T L M A D f +``` + +**Nothing in the deferred family is reachable mid-turn.** Downgrade the follow-up queue uniformly rather than reordering it — reordering is motion without movement. + +Three successive attempts to promote something out of that queue — `2J`, then `CSI r`, then `ESC7`/`ESC8` — were the same artifact wearing different labels, proposed by three different people. Each time, the proposer applied the artifact test to *someone else's* item and not to their own replacement. The `ESC7 ESC[r ESC8` bytes are contiguous at offsets 0, 2 and 5: one item was dismissed as a startup artifact in the same message that proposed its immediate neighbour as the reachable alternative, on evidence that condemns both equally. + +**That is the standing rule firing a fourth time, and it needs restating as a construction practice rather than a review one:** + +> **Run the test that killed the thing you are replacing, before you propose the replacement.** + +The original form — *any input used to reject an alternative must be run against the accepted design* — reads as something a reviewer does. Stated that way it caught the unclamped-CUP regression only because a reviewer happened to apply it. Stated as a construction practice it belongs to whoever proposes, which is where the failure actually occurs. Note also that stating the caveat is not applying it: every one of these three proposals was made by someone who had *written down* the overlap caveat in the same message. + +**The mechanism that let the rule survive a reviewer who did not apply it: each disagreement was handed over as evidence rather than as a conclusion.** Byte offsets and per-snapshot counts can be re-run by the recipient; an unfalsifiable claim can only be accepted or rejected. That is also what makes a claim cheap to test *against yourself*, which is the step every one of these three proposals skipped. + +The 30-of-36 split has a mechanism too: six snapshots are exactly 65536 bytes (`OUTPUT_BUFFER_MAX`), so their ring buffer had wrapped and lost the startup prefix. Thirty retained it. + +**Absence in one capture is not absence in general** — one turn, one version, and **no tool use**, so anything tool-triggered is unsampled. A tool-heavy capture is the only thing that can change this table, and it would simultaneously close `↑`, `hooks…` and the thinking state for the indicator plan. **That makes it the single highest-value artifact still outstanding across both plans.** + +#### Method note, since two of us were bitten + +`grep -c` counts matching *lines*, not occurrences, and this capture is nearly a single line. `grep -acoE $'\033\\[r'` returns 1 where the true count is 30. Zero lines does imply zero occurrences, so the `CSI S` = 0 result is unaffected — but **every non-zero count must use `grep -aoE … | wc -l`.** One of the counts in the first draft of this entry was taken the wrong way. + +Final: 157/157 across eleven terminal suites (up from 152), `tsc` and `eslint` clean.