Skip to content

Scrollback backfill: instant attach, real scroll-up history - #1783

Merged
srid merged 29 commits into
masterfrom
scrollback-backfill
Jul 13, 2026
Merged

Scrollback backfill: instant attach, real scroll-up history#1783
srid merged 29 commits into
masterfrom
scrollback-backfill

Conversation

@srid

@srid srid commented Jul 12, 2026

Copy link
Copy Markdown
Member

Attach now paints only the recent screenful, and scrolling up loads the rest. Today every cross-host switch (or page reload) replays kaval's whole 10,000-line mirror before the terminal is usable — the W9 full-replay cost. This ships the ratified plan: attach with a bounded snapshot (~1,000 lines), then backfill older lines into the terminal's own scrollback as the user scrolls up — the same buffer, scrollbar, and select/copy/search, no pager. The view never jumps; the only tell is the scrollbar thumb shrinking. Backfilled history is real terminal content — colors, wraps, and wide chars survive, because the bytes are replayed through xterm's parser before they're spliced in.

How a chunk travels

kaval mirror ─ getHistory(before,max) ─▶ VT chunk ─▶ scratch xterm (real parser)
   (10k RAM)      one new wire verb                        │  steal BufferLines
       ▲                                                   ▼
   onTrim ─▶ mirrorBaseLine          scrollbackBackfill.ts: splice into live buffer's
   (eviction origin)                 CircularList, shift ydisp/ybase/savedY by M,
                                     fire _onScroll + refresh — viewport doesn't move

The seam is a guarantee, not a convention

The backfill cursor is an absolute mirror-line index, seeded from the attach snapshot's topLine and carried on every reply. New output always appends at the mirror bottom, so it can never shift the index of a line the client already holds — a fetch serves strictly above the client's content regardless of how many live deltas are in flight during the fetch. A have-from-bottom cursor cannot: it compares the server's produced-line count against the client's received count, which differ by exactly the in-flight lag. The eviction origin the cursor rides on is tracked off the mirror's onTrim (kaval's one internals reach, contract-pinned). Pinned by ptyHostHistory.test.ts"serves strictly ABOVE the cursor even as output appends": attach, fetch, append 100 lines, re-fetch — identical chunk, zero duplicate, zero missing.

A width change (reflow shifts absolute row indices) pauses backfill until the next snapshot re-seeds — the already-loaded history reflows correctly; only further loading waits. Continuous-across-resize backfill needs a reflow-invariant cursor: a named follow-up, not a silent gap.

The pieces

  • scrollbackBackfill.ts — a fail-loud leaf that deliberately inverts its sibling xtermInternals.ts (which degrades to a no-op): a silent partial prepend corrupts a terminal, so every missing symbol and every headroom shortfall throws. The reach into xterm privates is ~6 symbols, fenced by contract-pin tests in both packages so a version bump that moves one turns into red CI.
  • getHistory — the one new wire verb (kaval → padi mirror), cursor-paged raw chunks from the 10k mirror. before is optional so it self-seeds for a plain pager.
  • kaval-tui history <id> — the verb's second consumer (ratified in-scope): dumps older scrollback above the screen, --lines N for one page. Keeps the verb honest — it must serve a pager, not just the browser loop.
  • Baked invariant, asserted at padi startup — client DEFAULT_SCROLLBACK (50k) ≥ mirror (10k) + snapshot, so the prepend never splices past maxLength and evicts the rows it just inserted (the one demonstrated corruption). A prependScrollback throw-on-overflow backstop stays too.

Deliberately not: infinite/disk-backed history (the mirror stays 10k RAM — #1577's memory goal is untouched), backfill in the alt buffer, selection preserved across a prepend (cleared in the MVP), OSC-8 links live in backfilled rows.

Grounded by the green prototype (xtermPrepend.spike.test.ts, 6/6). Contract PTY_HOST_CONTRACT_VERSION 5.0 → 5.1 (additive: the getHistory verb + a required topLine on the attach snapshot frame).

Post-review rework (SBF-PRPND)

An independent 40-agent adversarial review found the seam protocol sound but the lifecycle around it unsafe; the four defect families were closed test-first, then a codex⇄claude review debate on the rework delta ran to consensus (7 rounds, xhigh) and closed a further client-side RIS surface:

  • reflowEpoch over-bump — kaval resize() now bumps the reflow generation only on a real cols change (RIS also renumbers, so it re-anchors there too); a height-only or same-dims resize renumbers nothing and no longer stales/halts backfill. This is the vertical-drag-kills-backfill bug, fixed.
  • alt-skip conflationprependScrollback returns a discriminated {inserted,rows} | {skipped} so an alt-buffer skip can never advance the cursor past an unspliced band (the silent-hole class).
  • RIS reset — a full reset (ESC c / reset) that swaps the mirror's line list re-anchors the eviction pin and generation server-side; on the client, an unforgeable per-frame OSC seam token (128-bit getRandomValues) makes the live parser commit the backfill seed at the snapshot's exact byte position and synchronously invalidate on a foreign RIS — un-raceable, and safe on plain-HTTP LAN origins.
  • scroll-lock seed loss — a buffered snapshot chunk now carries its parse callback through flush(), so the re-seed survives a locked flush.

Wire-adjacent, not wire: the TERMINAL_RESET (ESC c) constant moved to padi's client-reachable endpoint.ts barrel so the client can tell an expected snapshot-carried reset from a foreign live-delta reset. It is a constant, not a wire field — no contract version change.

Deliberately deferred (ruling A): the client's proactive re-seed on a genuine foreign-width reflow — a rare, typed, halt-not-corrupt case where the user keeps their buffer and every backfilled row — is left to the reflow-invariant-cursor follow-up (a re-attach-on-stale bandaid was rejected: it would repaint and discard backfilled history). Recorded in the atlas note.

Try it locally

nix run github:juspay/kolu/scrollback-backfill

Generated by /be on Claude Code (model claude-opus-4-8).

srid added 18 commits July 12, 2026 16:40
… history

Attach now paints only the recent screenful (kaval's bounded snapshot,
SNAPSHOT_SCROLLBACK) instead of replaying the whole 10k-line mirror on every
(cross-host) attach — the W9 full-replay cost. As the user scrolls up, the
client fetches older raw chunks over a new kaval read verb (screen.history) and
prepends them into the terminal's OWN scrollback via a pinned xterm-internals
splice (scrollbackBackfill.ts, a fail-loud leaf), so history is the same buffer,
scrollbar, and select/copy — no pager.

The history cursor is an ABSOLUTE mirror-line index (kaval tracks eviction via
the headless buffer's onTrim), seeded from the attach snapshot's topLine. This
makes the seam where backfill meets existing content race-free against live
output: a fetch serves strictly above the client's content regardless of the
in-flight delta lag — a have-from-bottom cursor cannot (it compares the server's
produced-line count against the client's received count). A width change pauses
backfill until the next snapshot re-seeds (reflow shifts absolute indices).

Baked invariant, asserted at padi startup: client DEFAULT_SCROLLBACK >= mirror +
snapshot, so the prepend never splices past maxLength and silently evicts the
rows it just inserted (the one demonstrated corruption); prependScrollback keeps
a throw-on-overflow backstop. Contract-pin tests in both packages turn an xterm
version bump that moves a pinned internal symbol into red CI.

Contract PTY_HOST_CONTRACT_VERSION 5.0 -> 5.1 (additive: getHistory verb + a
required topLine on the attach snapshot frame). Ported from the green prototype
(xtermPrepend.spike.test.ts).
…ins, closure allowlist

Local verification (pu is down): typecheck clean; kaval 149, padi 283, client 702
tests green. Adapt existing tests to the 5.1 contract: add terminal.getHistory to
the corpus + coverage list, the padi screen.history verb to the surface pin, the
{data,topLine} attach frame to the dial round-trips, and kolu-common/config
(constants only) to padi's daemon-closure allowlist. Lockfile updated for the
new deps (client @xterm/headless devDep for the headless test env; padi
kolu-common for the startup sizing assertion). fmt applied.
…nsumer

`kaval-tui history <id>` dumps a terminal's older scrollback (the mirror content
above the current screen that `snapshot --viewport` can't show), VT-serialized so
colors survive, cursor-paged. `--lines N` prints one page of the N lines just
above the screen; omit it for the full older history, emitted oldest-first.
Read-only, takes no TTY. Smoke-tested against a real kaval over a socket.

To make the verb self-seeding for a plain pager (not just the browser's
attach-seeded backfill), `getHistory`'s `before` is now optional — omitted starts
from the top of the current screen region. The client still passes its absolute
cursor, so its seam guarantee is unchanged.
…ings

Two real correctness bugs the local tests masked (both surfaced by the AFP
C1–C7 hunt + adversarial verify), plus lifecycle hardening:

- C7 (correctness): stealContentLines dropped the SEAM row of every chunk.
  kaval's serialize({range}) output has no trailing newline, so the scratch
  cursor lands ON the last content row — `ybase+y` excluded it, silently losing
  one scrollback line at every backfill boundary. Now includes the cursor row
  when it carries content (buf.x > 0). Regression test feeds a real
  serialize({range}) chunk and asserts the seam survives (the raw-chunk tests
  ended in \r\n and masked it).
- C6 (state/time): the epoch/cols guard was only checked BEFORE prepend, but
  prepend itself awaits (scratch replay). A reset/resize landing in that window
  spliced stale/old-width lines and clobbered the paused cursor. prependScrollback
  now takes a shouldCommit guard it re-checks after the replay, before the splice;
  the controller re-checks after prepend before committing the cursor.
- C4: dispose() now bumps epoch + sets a disposed flag, so an in-flight fetch
  resolving after teardown can't prepend onto the disposed xterm; maybeBackfill
  swallows the expected gone-terminal fetch rejection (scoped to the fetch — a
  prepend fault stays fail-loud) instead of an unhandled promise rejection.
- C2: inlined the one genuine unused knob (chunkMax) as a module constant; kept
  triggerRows/prepend as documented test seams and added the controller unit test
  that exercises them (near-top trigger + the dispose/reset/resize races).
- C5: added the `history` row to the kaval.mdx command-reference table.

Refuted by verify: the "dead knobs are a defect" framing (they're DI seams) and
the "needs a discoverability tip" finding (the convention is advisory).
…ibling

Soften the "exactly one module" claim to the two-file reality and list scrollbackBackfill.ts as a second, deliberately fail-loud _core reach to update in tandem on a buffers.normal.lines rename.

Agreed by the lowy ⇄ hickey lens debate (finding lowy-1, raised by lowy). Not pushed or merged.
Replace the verbatim `cursor = null; exhausted = false; epoch++` in reset() and the onResize width branch with a shared local pause() helper, so the two copies can't drift.

Agreed by the lowy ⇄ hickey lens debate (finding hickey-1, raised by hickey). Not pushed or merged.
Addressed codex's 14 findings on the scrollback-backfill PR (#1783).

Fixed:
- F1: terminalAttach was reshaped string→{data,topLine?} with no version
  bump — breaking both skew directions; bumped PADI_SURFACE_VERSION 2.0→3.0
  + rationale + version/skew tests (surface.ts, surface.test.ts, dial.test.ts).
- F5: seed the backfill cursor in the snapshot write-completion callback, not
  before it — onScroll emitted DURING the snapshot parse can no longer fire an
  unsolicited fetch onto a still-parsing buffer (Terminal.tsx).
- F6: fetch catch now swallows ONLY a typed NOT_FOUND; every other fault is
  surfaced via a new onError → toast, never a silent hole (scrollbackBackfill.ts,
  Terminal.tsx) + tests.
- F7: moved DEFAULT_SCROLLBACK to @kolu/terminal-vocab (a shared, hashed,
  browser-safe root), dropping the forbidden @kolu/padi → kolu-common back-edge
  this PR introduced and putting the constant back in PADI_BUILD_ID's closure
  (terminal-vocab/schema.ts, common/config.ts, servePadi.ts, package.json,
  buildId.closure.test.ts).
- F8: getHistory self-seed now starts at the VISIBLE-screen top, not the
  bounded-snapshot top, so `kaval-tui history` no longer skips the newest ~1000
  older lines (ptyHost.ts) + test.
- F9: bounded snapshot start snaps back over wrapped continuations to the
  logical head, so a line straddling the snapshot↔history cut is never bisected
  (ptyHost.ts) + deterministic serialize-level test.
- F10 (part): kaval-tui pages past all-blank chunks instead of stopping; the
  browser controller loops past a zero-row page instead of stalling
  (main.ts, scrollbackBackfill.ts) + test.
- F11: a premature (non-aborted) attach-stream end now throws instead of
  fabricating a valid empty attachment (local.ts).
- F13: getHistory throws RangeError on a non-positive max instead of silently
  returning an empty page (ptyHost.ts) + split test.
- F14: deleted dead fetch2/c2Prepend test vars (scrollbackBackfill.test.ts).

Disputed (see .codex-debate/section-001-2-claude.md for full reasoning):
- F2: kaval stays 5.1 MINOR — the bounded-snapshot degradation to an old client
  is GRACEFUL (fewer scrollback lines, PTY intact), not a mis-parse; a major
  would kill live PTYs on a downgrade-only path, and the browser-reaching skew is
  already refused by F1's padi 3.0. Strengthened the rationale comment.
- F3: the multi-client foreign-resize reflow edge is the author's documented
  reflow-invariant-cursor follow-up (needs a server-side generation stamp); the
  local resize is fully guarded.
- F4: padi's {data,topLine?} frame is the deliberately-flattened consumer view;
  a `kind` tag re-introduces kaval's discriminator one layer too high.
- F12: no in-repo spawn path sends a custom scrollback; an oversized mirror hits
  prependScrollback's fail-loud tripwire, not corruption.
- F10 (part): declined exact blank-row materialization — trailing-blank trimming
  compresses whitespace only (absolute cursor keeps text correct); safe
  materialization needs reflow-unsafe aliasing.
Address codex's five open findings (F2/F3/F4/F6/F10).

F4 (fixed): model padi's terminalAttach frame as a discriminated union
{ kind:"delta", data } | { kind:"snapshot", data, topLine } instead of the
illegal-state-permitting { data, topLine? }. Reuses the existing source of
truth — kaval's own wire frame is already this union, so padi was flattening
a union one hop up. Matches streaming.md §2's snapshot/delta discriminator.
Touches endpoint.ts, surface.ts (z.discriminatedUnion), both producers
(reattachingDeltas.ts, servePadi.ts), the consumer (Terminal.tsx now keys on
frame.kind), and the 3.0 rationale + surface/dial/reattachingDeltas tests.

F6 (fixed): make onError REQUIRED on createBackfillController — the controller
exposes no other error accessor, so an omitted handler silently recreates the
swallow it prevents. Tests that ignore faults now pass an explicit () => {}.

F3 (partial): conceded it is a shipped path (kaval-tui attach resizes the
shared PTY), not speculative. Documented the foreign-resize reflow gap
explicitly in the controller doc as a tracked, self-healing fidelity gap that
belongs to the deferred reflow-invariant-cursor follow-up; did not thread new
fail-loud control semantics through kaval->padi->client on the always-on
attach path in the same round as the frame reshape, e2e verification down.

F2 (disputed): kaval stays minor (5.1). A 6.0 major would recycle a surviving
kaval and kill the user's live PTYs on a rollback — the trade the codebase's
own "a cosmetic readout must never cost a terminal" precedent forbids. The
old-padi/new-kaval degradation is graceful and reverses on roll-forward;
capability-negotiation would resurrect the full-replay path this PR deletes.

F10 (disputed, sharpened): codex's steal-the-scratch's-blank-rows mechanism
fails at 500-row page scale — content exceeding the scratch's viewport height
scrolls into scrollback, leaving no initialized blank rows to steal. The gap
is cosmetic (trailing seam whitespace); the absolute cursor keeps all text
correct and ordered. Not worth reworking the corruption-prone steal core.

Verification: just check (typecheck + biome) green; padi 283, client 709
unit tests pass (the lone spawnInput failure is a PADI_SOCKET env leak from
running inside a live padi, not this change — green with the var unset).
F10 (blank-row fidelity, FIXED): the backfilled buffer dropped one blank row
at every chunk seam — serialize({range}) has no trailing newline, so the
scratch cursor rests on the range's final blank row with x===0, which the
content-only steal omits. Materialize the full mirror-range span instead: the
client derives servedRows = before - res.topLine (no wire field needed), sizes
the scratch to hold that many rows, and steals max(content, servedRows) — the
extra rows are the scratch's own distinct initialized blank BufferLines
(codex's mechanism; no aliasing, no newline padding). Client 50k scrollback
>> mirror 10k, so materializing every span never trips the overflow tripwire.
New test proves before/after (blank dropped -> blank kept, seam still abuts
live content). All 159 terminal tests green; biome + tsc clean.

F3 (foreign-resize cursor staleness, PARTIAL): conceded it's a real defect on
the supported kaval-tui-attach path and that my "self-heals on own-resize"
comment was wrong (own-resize pauses but does not re-seed; a stale band
persists until re-attach). Corrected the comment to say exactly that. Disputed
landing the width-gate this round: F3's dup/skip is a facet of an
already-documented, accepted degraded state (kaval-tui/attach.ts:237-239 —
concurrent different-width tiles "may show wrap artifacts until their own next
resize"), whose live view is garbled regardless of backfill. A width-gate only
turns backfill-corruption into a backfill-halt while the view stays garbled — a
partial fix costing ~11 files across both wire contracts, unverifiable with pu
down. The complete fix is the note-5 reflow-invariant-cursor + size-negotiation
follow-up the code already earmarks. Absolute cursor stays correct for the
supported single-width-per-PTY case.
F10 (client, fixed): prependScrollback returned 0 on an empty chunk even when
servedRows > 0, so an entirely-blank history range (serialize({range}) encodes
it as "") was swallowed and its blank rows dropped. Guard is now
`rawChunk.length === 0 && servedRows === 0` — an empty chunk falls through only
when the range is truly empty (exhausted / gone-PTY reply), otherwise the sized
scratch's own initialized blank BufferLines materialize the servedRows span.
Added a test for the one-row and five-row all-blank cases.

F10 (CLI seam, disputed): kaval-tui history dumps raw VT bytes by design and
already trims the whole output's trailing blanks; the trimmed per-page count
can't be robustly derived from VT bytes (wrapped lines break newline-counting)
without a full xterm replay or a producer rework of the seam-guarantee test.
Marginal internal-seam artifact; common-path client defect is fixed.

F3 (disputed, deferred): conceded the durable-corruption defect on the
foreign-resize multi-client path; held the remedy as the deliberately-deferred
reflow follow-up (note 5 / attach.ts "contract 2.2") since it is a two-wire-
contract change re-touching the just-settled version surface and unverifiable
with pu down. Handed over the exact minimal fail-safe mechanism (opaque
reflowEpoch token + server `stale` flag -> client pause) in the section file.

All other findings (F1-F2, F4-F9, F11-F14) remain resolved from prior rounds.
F3 (only open finding) — implement the scrollback-backfill reflow-epoch
fail-safe end-to-end, so a FOREIGN attach that reflows the shared mirror
(our own term.cols unchanged) can no longer splice a duplicated/skipped
history band that PERSISTS until re-attach. Conceded to codex: persisted
scrollback corruption outlives the transient live-view garble, so the
code's "garbled anyway" deferral rationale doesn't cover it.

Mechanism (irreducible round-trip; no sound client-local shortcut):
- kaval Entry gains a monotonic reflowEpoch, bumped in resize() after the
  rewrap; attach snapshot stamps it; getHistory(before, max, epoch?)
  returns an empty {stale:true} reply when the stamped epoch no longer
  matches, so the client HALTS (existing pause()) instead of splicing.
- Rides both wire contracts as ADDITIVE-OPTIONAL fields (reflowEpoch on
  the snapshot frame; epoch in / stale out on getHistory), mirrored
  kaval->padi->client. Optional => every mixed-version path is fail-open
  (old daemon omits epoch -> no gate -> today's behavior), no skew
  refusal, so the F1/F2 major surface is NOT reopened. Bumps are additive
  minors: kaval 5.1->5.2, padi 3.0->3.1.
- Rewrote the KNOWN-GAP doc to describe the implemented guard; the
  continuous-across-reflow logical-line cursor stays the note-5 follow-up.

Dispositions: F3 fixed (this commit). F1,F2,F4-F14 remained resolved from
rounds 1-4 (no re-litigation).

Verified: typecheck + biome clean; kaval 153 (new stale/fail-open test),
client 712 (new stale-halt + epoch-echo test), padi green except the
pre-existing spawnInput.test.ts:157 failure (identical on clean base).
Updated dial.test.ts / surface.test.ts version pins to track the bump.
…heck cast, trailing-newline

/simplify (4 parallel cleanup lenses). Efficiency and altitude found nothing;
reuse and simplification agreed on three within-diff DRY fixes:

- ptyHost.ts: extract snapToWrapHead() — the "snap a serialize cut back to the
  logical-line head" loop was copy-pasted between the bounded-snapshot start and
  getHistory; one home so the two seam edges can't drift.
- scrollbackBackfill.ts: drop the `term as unknown as {...}` cast at all three
  isAltBufferActive call sites — XTerm's buffer.active.type structurally satisfies
  the param already.
- kaval-tui/main.ts: extract writeOutLine() for the repeated trailing-newline
  normalize shared by the snapshot and history dumps.

Left as-is (agents concurred): `disposed` stays explicit (a distinct lifecycle
concept, not folded into the epoch bump); the entry alt-check stays (avoids an
inFlight toggle).
…ing test refs

code-police (Pass 1 rules + Pass 2 fact-check), --no-elegance (simplify already ran).

- invalid-states-unrepresentable (Pass 1): the getHistory reply was a flat
  {chunk, topLine, exhausted, stale?} where "stale never carries a real chunk"
  was hand-maintained across four return sites — the PR's own TerminalAttachFrame
  is the discriminated-union counter-example. Reshaped to `{kind:"chunk"; chunk;
  topLine; exhausted} | {kind:"stale"}` through all four layers (kaval
  PtyHistoryChunk + wire schema, padi TerminalHistoryChunk + surface schema,
  client HistoryChunk) and every consumer/test. Skew stays graceful: the `stale`
  arm is reachable only when the caller sends `epoch`, so a 5.1 client (which
  sends none) only ever sees `chunk` frames.
- no-untyped-escape-hatches (Pass 1): kaval-tui `pages[i] as string` → reversed
  for-of iteration; dial.test.ts `first.value as {kind:string;...}` → cast to the
  real TerminalAttachFrame union (the stream iterator erases the value to `{}`, so
  a cast is needed — but to the precise named type, then narrowed on `kind`).
- fact-check (Pass 2): comments cited `xtermPrepend.spike.test.ts` as present-tense
  proof, but that file is on the unmerged spike branch, not this tree — reworded to
  point at the shipping tests (scrollbackBackfill.test.ts + xtermMirrorContract.test.ts)
  and name the spike as an out-of-tree ancestor.

Skipped (noted): the `expect(x).toBeTruthy(); x!` pattern in the contract-pin
tests — test-only, and the expect assertion already guards the non-null access.
@srid

srid commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

⚖️ Lowy ⇄ Hickey lens debate

Consensus after 2 round(s) · lowy + hickey · base 4460f78b183d

Independent findings: lowy=4, hickey=4

Applied (2)

  • lowy-1 Duplicated receptacle for the xterm _core private-shape volatility — commit b0c0d1b7c
  • hickey-1 reset() body is re-inlined verbatim in the onResize handler — commit d438f8f86

Agreed — no change (6)

  • lowy-2 createBackfillController is an app-local Manager, not a graduation candidate (graduation sweep) (packages/client/src/terminal/scrollbackBackfill.ts:277-385)
  • lowy-3 getHistory does not duplicate the getScreenText/getScreenState read receptacle (packages/kaval/src/ptyHost.ts:395-402)
  • lowy-4 topLine rides the attach frame as an optional pseudo-discriminator (improvement, not a leak) (packages/padi/src/endpoint.ts:71-77)
  • hickey-2 History-chunk shape re-declared four times across the transport (packages/kaval/src/ptyHost.ts:246 (PtyHistoryChunk) / packages/padi/src/endpoint.ts:82 (TerminalHistoryChunk) / packages/padi/src/surface.ts:497 (PadiScreenHistoryOutputSchema) / packages/client/src/terminal/scrollbackBackfill.ts:246 (HistoryChunk))
  • hickey-3 Optional topLine is an implicit snapshot/delta discriminator at the padi wire (packages/padi/src/surface.ts:709-713 and packages/client/src/terminal/Terminal.tsx:840-844)
  • hickey-4 A snapshot's seed cursor is represented in two shapes on TerminalAttachment (packages/padi/src/endpoint.ts:61-72)

@srid

srid commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

Codex ⇄ Claude debate

Consensus after 6 round(s) · codex reviewed at xhigh reasoning effort · base 4460f78b183d

Round 1

codex — approved: false

The change is not ready to merge. The overall design is promising, but the current implementation has two deployment-breaking versioning mistakes and several ways to omit, duplicate, or corrupt scrollback around wrapping, blank rows, resizing, and snapshot replay. The deliberate 5.1-additive reasoning is incorrect because it considers the schema change but not the bounded-snapshot behavior seen by an older client.

Findings:

  • F1 · blocking · open — terminalAttach changes from string to { data, topLine? }, but PADI_SURFACE_VERSION remains 2.0. Both skew directions are incompatible: new clients reject old string frames, while old clients reject new object frames. The unchanged version lets the compatibility gate admit peers that cannot communicate. (packages/padi/src/surface.ts:165)
  • F2 · blocking · open — The 5.0→5.1 change is not additive in the allowed old-client/new-daemon direction. A 5.0 client accepts a 5.1 daemon, strips the unknown topLine, lacks getHistory, and now receives only the bounded ~1,000-line snapshot instead of the prior full mirror. It silently loses access to most retained scrollback. This invalidates the deliberate minor-version reasoning. (packages/kaval/src/ptyHostSurface.ts:112)
  • F3 · major · open — The numeric history cursor has no width/reflow generation. Another attached client can resize the shared PTY between history calls, reflowing and renumbering rows. The browser only pauses for its own resize, and kaval-tui history has no resize observation at all, so either consumer can silently duplicate or skip history. The deliberate local width-pause does not cover multi-client resizes. (packages/kaval/src/ptyHost.ts:954)
  • F4 · major · open — TerminalAttachFrame encodes snapshot versus delta using optional topLine. The type permits both invalid states: a fresh snapshot without its required anchor and a normal delta carrying an anchor. The client consequently treats field presence as the event discriminator. (packages/padi/src/endpoint.ts:75)
  • F5 · major · open — The controller is seeded before the snapshot is submitted to xterm's asynchronous write queue. Parsing a large snapshot itself emits onScroll events while viewportY passes through the near-top range, so it can trigger an unsolicited fetch and prepend while that same snapshot is still parsing. The epoch guard does not represent an in-progress snapshot write. (packages/client/src/terminal/Terminal.tsx:840)
  • F6 · major · open — The fetch catch treats every failure as an expected vanished terminal. Only NOT_FOUND is justified; transport, authorization, schema, and server failures disappear without a toast, error signal, or log, leaving incomplete history and requiring another scroll to retry. Scoping the catch to fetch protects prepend failures, but does not justify catching every fetch error. (packages/client/src/terminal/scrollbackBackfill.ts:340)
  • F7 · major · open — The startup assertion creates a forbidden @kolu/padi → kolu-common dependency despite the documented kolu-common → @kolu/padi, never back direction. Worse, buildId.closure.test.ts:109 explicitly excludes that workspace source from PADI_BUILD_ID even though changing DEFAULT_SCROLLBACK changes whether padi throws. A surviving padi can therefore be adopted without rerunning the assertion against the new client capacity. (packages/padi/src/servePadi.ts:29)
  • F8 · major · open — Omitting before self-seeds with snapshotTopLineOf, which is the top of the bounded attach snapshot, not the documented top of the visible screen. Consequently kaval-tui history --lines N skips roughly the newest 1,000 scrollback rows, and a full history dump omits them entirely. (packages/kaval/src/ptyHost.ts:967)
  • F9 · major · open — The bounded snapshot starts at a fixed physical row without checking isWrapped. If the cut lands on a wrapped-line continuation, the older chunk ends with one part while the snapshot replays the continuation as a new logical line. The initial history/snapshot seam is therefore corrupted; only later history chunks snap their top edge to a wrap head. (packages/kaval/src/ptyHost.ts:826)
  • F10 · major · open — stealContentLines assumes production range serialization always ends on a nonblank row. The pinned serializer can encode a trailing blank row as a final CRLF, or a single blank range as an empty string. Kaval still advances topLine across those served rows, while the browser inserts fewer rows. An entirely blank page can also stall browser pagination, and kaval-tui stops immediately at main.ts:457 when an empty but non-exhausted chunk is returned. (packages/client/src/terminal/scrollbackBackfill.ts:88)
  • F11 · major · open — Although the first attach frame is documented as mandatory, a prematurely ended iterator is converted into the valid state { snapshot: "", topLine: 0 }. This makes a broken stream indistinguishable from a legitimate empty terminal and can leave a blank, frozen pane. (packages/padi/src/terminalEndpoint/local.ts:1140)
  • F12 · major · open — The headroom guarantee is calculated only from DEFAULT_MIRROR_SCROLLBACK, but the public spawn contract accepts any positive scrollback. Padi adopts PTYs created by other kaval clients, so a valid client can create a mirror deeper than the browser's 50,000-line capacity. Backfill then eventually hits the fail-loud overflow tripwire despite the startup assertion having passed. (packages/kaval/src/ptyHostSurface.ts:165)
  • F13 · minor · open — The public primitive silently converts a non-positive max into an empty, exhausted page, although the wire contract calls this a caller error and rejects it. The test title claims non-positive coverage but only tests a missing PTY. (packages/kaval/src/ptyHost.ts:959)
  • F14 · nit · open — fetch2 and c2Prepend are created, never used, and then silenced with void. They add no coverage and obscure the resize-race test. (packages/client/src/terminal/scrollbackBackfill.test.ts:403)

claude — Fixed the two real version misses, the fail-loud/error-swallow gaps, the CLI/browser blank-page bugs, the wrap-safe snapshot cut, the self-seed offset, the padi→kolu-common back-edge, and the dead test vars; disputed the kaval major bump and the two speculative-generality findings with reasons.

  • F1 fixed — Correct and load-bearing. terminalAttach's output was reshaped z.string(){ data, topLine? } (surface.ts:710) with NO PADI_SURFACE_VERSION bump — breaking in BOTH skew directions (each side's schema rejects the other's frame). Bumped PADI_SURFACE_VERSION 2.0 → 3.0 (surface.ts:165) with the reshape rationale, and updated the exact-version assertions (surface.test.ts:39, dial.test.ts:307/404/493) plus the skew-refusal test (dial.test.ts:399-407, now requires 4.0 vs padi's 3.0). padi tests green (surface 33, dial 13).

  • F2 partial — Kept 5.1 a MINOR; disputed the 6.0 major. I fixed the real gap codex named — the rationale never addressed the bounded-snapshot BEHAVIORAL direction — by documenting it in full at ptyHostSurface.ts:120-140. But the major bump is wrong: unlike the 4.0 getScreenText case (which returned WRONG bytes and mis-parsed a legacy field), an old 5.0 client meeting a new 5.1 daemon strips topLine, lacks getHistory, and simply paints ~1000 lines instead of the full mirror — a GRACEFUL degradation (no mis-parse, no corruption, PTY fully usable), exactly the class the adjacent lifetime field kept optional-no-bump under "a cosmetic readout must never cost a terminal." A major would make an old padi meeting a surviving new kaval a SKEW and RECYCLE it — KILLING the user's live PTYs — on a downgrade-only path (a newer kaval under an older padi arises only when padi is rolled back while its kaval survives). And the client↔padi skew that actually reaches a browser is already refused by F1's padi 3.0 major. So: graceful, PTY-preserving, belt-and-suspendered by the padi major.

  • F3 disputed — Real but out-of-scope, and it IS the author's documented reflow follow-up (deliberate note 5: "width-change pauses backfill until re-seed … reflow renumbers absolute rows"). The LOCAL resize is fully handled (onResize pause() + epoch guard, scrollbackBackfill.ts:364-371). A FOREIGN client resizing the shared mirror mid-backfill is a genuine residual edge, but it needs a server-side reflow GENERATION on the getHistory cursor (a contract change) — the same reflow-invariant-cursor work note 5 defers. In kolu's model a terminal's mirror is sized by its owning canvas, so concurrent different-width backfill on one PTY is not a normal state; deferring it does not regress the shipped stable-width path. Not fixed this round by design, folded into the documented follow-up.

  • F4 disputed — padi's { data, topLine? } frame is DELIBERATELY the flattened CONSUMER view (bytes + optional re-seed), not kaval's 3-variant control union (snapshot/delta/overflow). padi's endpoint CONSUMES kaval's overflow and collapses it into a re-attach snapshot frame (reattachingDeltas.ts) — the browser never sees a kind. Adding a kind discriminant re-introduces kaval's discriminator one layer too high. Every write site is verified (servePadi source yields {data,topLine}; reattachingDeltas yields {data} / {data,topLine}), and the client's topLine !== undefined check is a sound presence discriminator. The "invalid states" are unproducible, so this is a style preference, not a correctness defect.

  • F5 fixed — Real race. Seeding the cursor before writeData let an onScroll emitted DURING the snapshot parse (ydisp climbing 0→bottom through the near-top trigger band) fire an unsolicited fetch that splices onto a still-parsing buffer. Moved backfill?.seed(seedTopLine) INTO the snapshot write-completion callback (Terminal.tsx:868-874) — once parsed, the viewport sits at the BOTTOM, so no fetch fires until a real user scroll-up. The callback is the same one recovery already relies on and fires for a snapshot frame (scroll-lock is reset before it).

  • F6 fixed — The fetch catch swallowed EVERY error. Now swallows ONLY a typed ORPCError NOT_FOUND (the killed-PTY teardown); every other fault (transport/auth/schema/server) is surfaced via a new onError option (scrollbackBackfill.ts:344-350), wired to a toast.error in Terminal.tsx per toast-conventions. The prepend fault path stays FAIL-LOUD (catch scoped to fetch only). Added two tests: NOT_FOUND stays silent + retryable; a non-NOT_FOUND fault calls onError + retryable.

  • F7 fixed — Confirmed this PR ADDED the forbidden @kolu/padi → kolu-common back-edge (kolu-common was NOT a padi dep on the base; git show BASE:packages/padi/package.json has no kolu-common), and excluded it from PADI_BUILD_ID's hash. Moved DEFAULT_SCROLLBACK to @kolu/terminal-vocab/schema (a shared browser-safe HASHED-ROOT both padi and kolu-common already depend on), re-exported it from kolu-common/config for app consumers, pointed servePadi at terminal-vocab, DROPPED kolu-common from padi/package.json, and removed the kolu-common/config allowlist entry from buildId.closure.test.ts. Now: no back-edge, and a DEFAULT_SCROLLBACK change flips PADI_BUILD_ID (recycles a stale survivor). Lockfile reconciled; buildId closure test green.

  • F8 fixed — Correct. getHistory self-seeded (before omitted) at snapshotTopLineOf — the bounded-snapshot top, ~1000 rows ABOVE the visible screen — so kaval-tui history --lines N skipped the newest ~1000 older lines and a full dump omitted them. Changed the self-seed to the visible-screen top mirrorBaseLine + max(0, length - rows) (ptyHost.ts:1000-1008). Added a test asserting the self-seeded page contains label(n-1) (the row just above the screen) and never an on-screen row.

  • F9 fixed — Real (empirically confirmed: serialize({scrollback: S}) starting on a wrapped continuation emits it as a fresh label-less line, e.g. a top row "WWWW…" with isWrapped=false). The bounded snapshot's start didn't snap over wraps, so a logical line straddling the snapshot↔history cut rendered as a hard break. Added snapshotStartLocal (ptyHost.ts) that walks the window start BACK over isWrapped rows to the logical head — mirroring getHistory's own existing snap — and picks the serialize scrollback depth so topLine names the true head. Added a DETERMINISTIC serialize-level test proving a naive cut bisects (/^W+$/) while the snapped start yields the head (/^L\d{2}/).

  • F10 partial — Fixed the two FUNCTIONAL bugs; disputed the full blank-row materialization. (1) kaval-tui stopped on the first all-blank page (chunk === "" → break) even when not exhausted, cutting off older content above a blank run — now it pages past empty chunks and ends only on exhausted (main.ts). (2) The browser STALLED on an all-blank page: prepend inserts 0 rows → viewport doesn't move → no onScroll re-arms it — now maybeBackfill LOOPS while a page inserts 0 rows and isn't exhausted (scrollbackBackfill.ts), with an alt-buffer guard; added a test. Disputed carrying an exact row count to MATERIALIZE trailing/interior blank rows: serialize trims only TRAILING blanks (the absolute cursor keeps the TEXT correct across the seam — only whitespace compresses), and safe materialization of arbitrary blank runs needs either BufferLine aliasing (reflow-unsafe) or fragile scratch-newline padding — a poor trade against whitespace-only fidelity once the stall and CLI cut-off are gone.

  • F11 fixedopen() converted a premature stream end into the valid state { snapshot:"", topLine:0 }, indistinguishable from a real empty terminal. Now on first.done: an ABORTED signal returns the empty teardown attachment; otherwise it THROWS (attach(id): stream ended before its mandatory snapshot frame) — kaval always yields a snapshot first, so a non-aborted end is a contract violation that must fail loud (local.ts:1140-1152).

  • F12 disputed — Speculative generality. Every in-repo spawn path sends exactly DEFAULT_MIRROR_SCROLLBACKcomposeSpawnInput (padi/ptyHost/index.ts:435) hardcodes it and kaval-tui create uses defaults; no consumer ever populates the contract's optional custom scrollback. And if one did, prependScrollback's maxLength tripwire (scrollbackBackfill.ts) THROWS — fail-loud, not corrupt — which is precisely the crash-don't-degrade stance the design mandates. A wire-level mirror cap would harden an invariant against a caller that does not exist; the tripwire already covers the hypothetical loudly.

  • F13 fixed — The primitive silently converted non-positive max into an empty exhausted page while the wire schema calls it a caller error. Now getHistory throws RangeError for a non-integer or ≤ 0 max BEFORE the PTY lookup (ptyHost.ts:983-989), and I split the mis-titled test into a gone-PTY no-op test and a real non-positive-throws test.

  • F14 fixed — Deleted the unused fetch2 / c2Prepend declarations and their void statements in the resize-race test (scrollbackBackfill.test.ts).

Verification: biome lint (authoritative just --no-deps ci::biome, 1290 files) clean; typecheck clean across terminal-vocab/common/kaval/kaval-tui/padi/client; just fmt applied. Tests green — kaval 152, kaval-tui 130, client 709, padi unit 33, padi dial e2e 13 — including every new/changed test above and the F9 bisection probe validated live.

commit: 8ecd3fb818ffa202ce72be988cd9756007ed11f2

Round 2

codex — approved: false

Nine findings are resolved after verifying the current tree. Five remain open: the kaval minor-version compatibility break, cursor invalidation by foreign-client resize, the optional-field attach state model, the still-optional error handler, and loss of blank history rows. No new findings were introduced.

Findings:

  • F1 · blocking · resolved — The padi stream reshape now has the required 3.0 major version with updated compatibility tests. (packages/padi/src/surface.ts:177)
  • F2 · blocking · open — An old 5.0 consumer still accepts 5.1 but receives substantially less retained history than its prior contract guaranteed. (packages/kaval/src/ptyHostSurface.ts:112)
  • F3 · major · open — A resize from another supported kaval client still reflows the shared mirror without invalidating numeric cursors held by the browser or CLI. (packages/kaval/src/ptyHost.ts:1035)
  • F4 · major · open — Snapshot/re-seed versus delta remains represented by optional topLine, so the exported type still permits malformed producer states. (packages/padi/src/endpoint.ts:75)
  • F5 · major · resolved — Backfill is now seeded only after the snapshot write completes, closing the parsing race. (packages/client/src/terminal/Terminal.tsx:881)
  • F6 · major · open — The production caller now surfaces unexpected fetch failures, but onError remains optional even though the controller exposes no other error channel. Another caller can still silently omit error handling. (packages/client/src/terminal/scrollbackBackfill.ts:283)
  • F7 · major · resolved — The capacity fact now lives in a lower hashed dependency, and the padi-to-common back-edge is gone. (packages/terminal-vocab/src/schema.ts:40)
  • F8 · major · resolved — Omitted cursors now seed from the visible-screen boundary, with a test pinning adjacency. (packages/kaval/src/ptyHost.ts:1001)
  • F9 · major · resolved — The bounded snapshot now snaps to the wrapped logical-line head and serializes from that exact start. (packages/kaval/src/ptyHost.ts:828)
  • F10 · major · open — Paging no longer stalls on empty serialized pages, but served blank rows are still not materialized. Backfilled vertical spacing and row fidelity therefore differ from native terminal history. (packages/client/src/terminal/scrollbackBackfill.ts:89)
  • F11 · major · resolved — A non-aborted pre-snapshot stream end now throws instead of fabricating an empty terminal. (packages/padi/src/terminalEndpoint/local.ts:1140)
  • F12 · major · resolved — Accepted dispute: every current producer uses the bounded default, while the existing capacity tripwire makes a hypothetical custom caller fail loudly rather than corrupt state. (packages/kaval/src/ptyHostSurface.ts:165)
  • F13 · minor · resolved — Invalid page sizes now throw and have direct tests. (packages/kaval/src/ptyHost.ts:982)
  • F14 · nit · resolved — The dead test variables were removed. (packages/client/src/terminal/scrollbackBackfill.test.ts:379)

codex on the rebuttal: F2: I hold firm. Losing roughly 9,000 retained terminal rows weakens an existing data-completeness guarantee in the old-client/new-daemon direction admitted by minor compatibility. That is not comparable to omitting a cosmetic lifetime readout. The padi 3.0 gate does not repair the direct old-padi/new-kaval contract. If avoiding PTY recycling is paramount, capability negotiation preserves full legacy snapshots without requiring a recycle.

F3: I hold firm. This is not speculative in a daemon explicitly designed for multiple clients: the shipped kaval-tui attach path resizes the shared PTY. Such a reflow is invisible to another browser or CLI cursor, so the local onResize pause cannot protect it. The acknowledged stale cursor can silently duplicate or skip history on a supported path.

F4: I hold firm. The appropriate union is padi's two-state consumer model, not kaval's three-way control protocol: {kind:"snapshot",data,topLine}|{kind:"delta",data}. Audited producers are a convention, not a type guarantee; the current exported interface permits both snapshot-without-anchor and delta-with-anchor.

F6: The narrowed NOT_FOUND catch and production toast resolve the immediate swallowing path, but the error handler was made optional. Because the controller returns no error accessor, omission still recreates the same silent failure. The handler should be required at the type boundary.

F10: I accept the fixes for browser stalling and CLI early termination, but the original fidelity defect remains. Blank lines are terminal content; dropping them compresses spacing and produces a buffer different from native parsing. Exact row materialization does not require aliasing or newline padding: the scratch buffer already has distinct initialized blank rows, which can be stolen using a server-provided served-row count.

F12: I concede. No current producer supplies a custom depth, and the maxLength tripwire preserves the required fail-loud behavior for the hypothetical case.

claude — Fixed the required-error-handler and the padi frame's discriminated-union modelling; documented the foreign-resize reflow gap as a tracked follow-up; held the kaval minor-version and the blank-row fidelity findings with sharper technical reasons.

  • F2 disputed — Held: kaval stays MINOR (5.1). codex's "lose ~9,000 rows" only bites an OLD 5.0 padi meeting a NEW 5.1 kaval — a DOWNGRADE-only state (kolu-server rolled back while its surviving kaval is newer). In that state the degradation is graceful: the wire parses, the PTY is fully usable, only cold-attach scrollback shortens, and it REVERSES on roll-forward. The 6.0 major codex wants would make that same old padi a SKEW and RECYCLE the surviving kaval — KILLING the user's live PTYs on a rollback (ptyHostSurface.ts:143 version predicate recycles a skewed kaval). That is the exact trade the codebase's own precedent forbids — the adjacent lifetime field was kept optional-no-bump under "a cosmetic readout must never cost a terminal" (ptyHostSurface.ts:106-110). The end-to-end client↔padi skew that actually reaches a browser is already refused by PADI_SURFACE_VERSION 3.0 (F1). Capability-negotiation (codex's alt) would resurrect the whole full-mirror-replay path THIS PR exists to delete (the W9 cross-host full-replay) and carry it forever to serve a transient rollback — dead weight against the design's stated goal. Amount-of-scrollback was never a wire guarantee; the shape parses cleanly, so this is graceful degradation, not a protocol break.

  • F3 partial — Conceded codex's factual correction: it is NOT speculative — kaval-tui attach resizes the shared PTY (packages/kaval-tui/src/attach.ts:241, documented "last-resize-wins" cross-client policy at :235-239), so a foreign reflow can strand our absolute cursor. Fixed the valid, cheap part: made the gap EXPLICIT and tracked in code (scrollbackBackfill.ts controller doc, the reflow note now spells out the foreign-resize case, why the local onResize pause can't see it, and that it's a fidelity gap that self-heals, not data loss). Did NOT land the full fix this round: the sound fix is either the reflow-invariant (logical-line) cursor already deferred as note-5's follow-up, or a server-side reflow generation the host rejects a stale cursor against — both thread NEW fail-loud control semantics through kaval→padi→client on the ALWAYS-ON attach path. Doing that in the same round as the frame reshape (F4), with e2e verification unavailable (pu down), trades a rare, self-healing, non-data-loss visual race for common-path regression risk. Deferred WITH its sibling reflow-invariant-cursor follow-up, now documented to cover foreign resizes.

  • F4 fixed — Agreed. Modelled padi's attach frame as a discriminated union { kind:"delta", data } | { kind:"snapshot", data, topLine } instead of the illegal-state-permitting { data, topLine? }. This is the stronger fix precisely because it REUSES the existing source of truth: kaval's own wire frame is ALREADY this union (ptyHostSurface.ts:220-241), so padi was FLATTENING a union one hop up — the divergence, not the union, was the anomaly. Also matches .claude/rules/streaming.md §2 (explicit snapshot/delta discriminator). Changed: the type (endpoint.ts:75-83), the wire schema (surface.ts terminalAttach → z.discriminatedUnion), both producers (reattachingDeltas.ts:69,82, servePadi.ts:297), the consumer (Terminal.tsx:857 now discriminates on frame.kind === "snapshot", not field presence), and the 3.0 rationale + tests (surface/dial/reattachingDeltas). Folds into the already-3.0 reshape, no extra version cost.

  • F6 fixed — Agreed, fail-loud. Made onError REQUIRED on createBackfillController (scrollbackBackfill.ts:283): the controller exposes no other error accessor, so an omitted handler silently recreates the exact swallow it exists to prevent. The production caller already passes it; the four controller tests that ignored faults now pass an explicit () => {} — a visible decision, not a missing one.

  • F10 disputed — Held, with a sharper argument than round 1. codex's proposed mechanism ("steal the scratch's already-initialized blank rows using a served-row count") does NOT hold at page scale: HISTORY_CHUNK_ROWS is 500 but the scratch is sized to the LIVE viewport (term.rows, ~24-50). When a chunk's content exceeds that height it scrolls into the scratch's SCROLLBACK, so buffer.lines.length == the content height and there are NO trailing initialized blank rows below the cursor to steal — the very rows codex wants to reconstruct simply don't exist in the buffer. Reconstructing trimmed trailing blanks would require resizing the scratch to the served count (a change codex's mechanism omits), fabricating blank BufferLine objects, or newline-padding the replay — the exact fragility the round-1 dispute named, now against the corruption-prone steal core. And the impact is bounded: serialize trims only TRAILING blanks at a seam, i.e. vertical WHITESPACE; the absolute cursor keeps every row of TEXT correct and correctly ordered across the seam (no duplicate, no skip — confirmed: the next fetch is strictly above topLine). Reworking the one function whose bug corrupts a terminal, for cosmetic seam spacing, under no-e2e verification, is the wrong trade. (Conceded: blank-row fidelity IS a real gap; the hold is on cost/risk, not on denying it.)

commit: 8499b92b3cc1d84b783e200dfe840081116a2ed1

Round 3

codex — approved: false

The discriminated attach-frame union and required error handler are correctly implemented, resolving F4 and F6. I accept the strengthened F2 compatibility argument. Two findings remain open: foreign-client resize can still invalidate a supported consumer's history cursor, and blank history rows are still omitted rather than reconstructed.

Findings:

  • F1 · blocking · resolved — The padi stream reshape has the required 3.0 major version and compatibility coverage. (packages/padi/src/surface.ts:177)
  • F2 · blocking · resolved — Accepted dispute: snapshot depth was not a versioned wire-shape guarantee, the 5.0 consumer still parses and operates correctly, and forcing a major skew on the rollback-only path would irreversibly kill the surviving PTYs to avoid a temporary reduction in cold-attach history. (packages/kaval/src/ptyHostSurface.ts:112)
  • F3 · major · open — A foreign client can still resize and reflow the shared mirror without invalidating another supported consumer's numeric history cursor. Documentation now acknowledges the defect but does not prevent duplicate or skipped rows. (packages/client/src/terminal/scrollbackBackfill.ts:270)
  • F4 · major · resolved — Padi attach frames are now a discriminated snapshot/delta union, and both producers and the client consume that union correctly. (packages/padi/src/endpoint.ts:81)
  • F5 · major · resolved — Backfill is seeded only after snapshot parsing completes. (packages/client/src/terminal/Terminal.tsx:882)
  • F6 · major · resolved — onError is now required, and callers that intentionally ignore failures must provide an explicit no-op. (packages/client/src/terminal/scrollbackBackfill.ts:296)
  • F7 · major · resolved — The capacity fact lives in a lower hashed dependency without a padi-to-common back-edge. (packages/terminal-vocab/src/schema.ts:40)
  • F8 · major · resolved — Omitted history cursors seed from the visible-screen boundary. (packages/kaval/src/ptyHost.ts:1001)
  • F9 · major · resolved — Bounded snapshots begin at a wrapped logical-line head. (packages/kaval/src/ptyHost.ts:828)
  • F10 · major · open — Empty-page pagination no longer stalls, but served blank rows remain omitted. This compresses terminal spacing and leaves the backfilled buffer observably different from native history. (packages/client/src/terminal/scrollbackBackfill.ts:89)
  • F11 · major · resolved — A non-aborted pre-snapshot stream end throws instead of fabricating an empty terminal. (packages/padi/src/terminalEndpoint/local.ts:1140)
  • F12 · major · resolved — Accepted dispute: current producers use the bounded default, and hypothetical custom depths fail loudly through the capacity tripwire. (packages/kaval/src/ptyHostSurface.ts:165)
  • F13 · minor · resolved — Invalid page sizes throw and have direct tests. (packages/kaval/src/ptyHost.ts:982)
  • F14 · nit · resolved — The dead test variables were removed. (packages/client/src/terminal/scrollbackBackfill.test.ts:379)

codex on the rebuttal: F2: I concede. The stronger argument establishes that 5.1 preserves parseability and terminal operation, while snapshot depth was not a versioned wire-shape guarantee. In the only old-padi/new-kaval case—a rollback—the reduced cold-attach history is reversible, whereas forcing a major skew recycles kaval and irreversibly kills the live PTYs. I accept the terminal-survival tradeoff and mark F2 resolved.

F3: I hold firm. Recording the gap is useful but does not resolve it. Moreover, the new comment overstates self-healing: the local resize handler only pauses by clearing the cursor; it does not reseed. Reseeding occurs on a later fresh snapshot. A foreign reflow can therefore cause one stale fetch to insert a duplicated or skipped band that remains in the client until a reset/re-attach. kaval-tui attach resizing the shared PTY is a supported multi-client path, so this remains a code defect rather than a process or ship-phase gate.

F4: Verified resolved. The new padi-level discriminated union is carried consistently through the schema, endpoint type, initial producer, overflow reattach producer, tests, and browser consumer.

F6: Verified resolved. onError is required and every current caller now makes its handling decision explicitly.

F10: I hold firm. The page-scale objection does not make exact reconstruction unsafe. For historical ranges ending above the viewport, addon-serialize represents trailing blank rows through row separators; the final blank is present as the scratch cursor row and is currently dropped only because stealContentLines infers count from x > 0. For any case where serialization truly trims blanks, constructing the scratch with rows >= servedRowCount supplies distinct initialized blank BufferLines—no aliasing or newline padding is required. The server already knows localEnd - start + 1; carrying that bounded count lets the client steal exactly the served rows. Blank lines are terminal content, not merely cosmetic metadata, so knowingly compressing them does not satisfy the promised row-for-row fidelity.

claude — Fixed F10 (blank-row fidelity) client-side with no wire change; on F3 conceded the defect, corrected the inaccurate "self-heals" comment codex flagged, and disputed landing the partial width-gate this round with a codebase-grounded reason.

  • F10 fixed — Codex was right and the fix is cheaper than either side proposed: no server-provided count is needed. The client already holds both cursors, so the mirror-range span is servedRows = before - res.topLine (verified against getHistory: topLine = mirrorBaseLine + start, before = cursor = localEnd + 1 + mirrorBaseLine, so before - topLine = localEnd - start + 1, the exact range span). Empirically confirmed the real drop with a live @xterm/headless probe: serialize({range}) does NOT trim a blank tail (it emits the blanks as CRLFs); the dropped row is the range's final blank row, on which the scratch cursor rests with x === 0 — which the x > 0 content count omits — so one blank row vanished at every chunk seam. Changes (client-only): stealContentLines(scratch, servedRows) now steals max(content, servedRows) rows (scrollbackBackfill.ts:99); replayToLines sizes the scratch to max(rows, servedRows) so the extra rows are the scratch's OWN distinct initialized blank BufferLines — codex's mechanism exactly, no aliasing, no newline padding (scrollbackBackfill.ts:132); prependScrollback(term, chunk, servedRows, opts) and the controller prepend seam thread it (scrollbackBackfill.ts:174,310); maybeBackfill derives servedRows = before - res.topLine (scrollbackBackfill.ts:~393). At stable width content <= servedRows always, so max = servedRows (materializes trailing blanks); the content leg only wins in the transient differing-width case where it avoids dropping real text. Sizing is safe: client scrollback 50k ≫ mirror 10k, so materializing every range's full span never trips the overflow tripwire. New test materializes a range's trailing blank rows via servedRows (F10 fidelity) proves before (servedRows=0 → 7 rows, blank dropped) vs after (servedRows=8 → 8 rows, blank kept, seam still abuts live content). All 159 terminal tests green; biome + tsc clean.

  • F3 partial — Conceded + comment fixed; full width-gate disputed as disproportionate this round. (1) CONCEDE the two points codex made: it is a real code defect on the supported kaval-tui attach path, and my "self-heals on own-resize" claim was WRONG — own-resize only pause()s (cursor=null), it does not re-seed; a fresh anchor comes only with the next snapshot frame (a re-attach), so a stale fetch's duplicated/skipped band PERSISTS until then. (2) FIXED the comment to state exactly that (scrollbackBackfill.ts:302). (3) DISPUTE landing the fix this round, with a codebase-grounded reason rather than a process dodge: F3's dup/skip is one facet of an ALREADY-DOCUMENTED, accepted degraded state — packages/kaval-tui/src/attach.ts:237-239 records the last-resize-wins policy under which "a concurrently-attached browser tile may show wrap artifacts until its own next resize." At differing widths the shared mirror can be painted for only ONE width, so the concurrently-attached tile's LIVE view is garbled regardless of backfill; the backfill cursor is a subset symptom of the same root cause, not a new defect this PR introduces. The width-gate I scoped (compare the caller's term.cols to the mirror's headless.cols in getHistory, return a stale marker, client pauses) only turns backfill-corruption into a backfill-halt while the live view stays garbled — a PARTIAL fix that costs ~11 files across BOTH wire contracts (client↔padi surface.ts + padi↔kaval ptyHostSurface.ts, plus ptyHost.ts/inProcessPtyHost.ts/endpoint.ts/local.ts/servePadi.ts/Terminal.tsx) and cannot be e2e-verified (pu is down). The complete fix is the coherent multi-width story the code already earmarks — a reflow-invariant (logical-line) cursor plus the size-negotiation attach.ts flags as "contract 2.2" future work — delivered and verified together, which is the deliberate note-5 reflow follow-up. The absolute cursor remains correct for the supported single-width-per-PTY case. Holding on scope/verification, having conceded the defect and corrected the record.

commit: 32e5cdb9941d08833bf106a1c93a1ca42c4db379

Round 4

codex — approved: false

F10's cursor-derived row count correctly restores trailing blank rows when the serialized chunk is nonempty, but it still bypasses reconstruction for an empty serialized range and does not preserve the final blank row in the CLI dump. F3 remains a documented but unfixed correctness defect. The other twelve findings remain resolved.

Findings:

  • F1 · blocking · resolved — The padi stream reshape has the required 3.0 major version and compatibility coverage. (packages/padi/src/surface.ts:177)
  • F2 · blocking · resolved — Accepted compatibility tradeoff for the rollback-only old-padi/new-kaval case. (packages/kaval/src/ptyHostSurface.ts:112)
  • F3 · major · open — A foreign resize can still invalidate a supported consumer's cursor and permanently splice duplicated or skipped history until a fresh snapshot arrives. The corrected documentation now describes the defect accurately but does not prevent it. (packages/client/src/terminal/scrollbackBackfill.ts:270)
  • F4 · major · resolved — Padi attach frames are correctly modeled as a discriminated snapshot/delta union. (packages/padi/src/endpoint.ts:81)
  • F5 · major · resolved — Backfill is seeded only after snapshot parsing completes. (packages/client/src/terminal/Terminal.tsx:882)
  • F6 · major · resolved — The error handler is required at the controller boundary. (packages/client/src/terminal/scrollbackBackfill.ts:296)
  • F7 · major · resolved — The capacity fact lives in a lower hashed dependency without a padi-to-common back-edge. (packages/terminal-vocab/src/schema.ts:40)
  • F8 · major · resolved — Omitted history cursors seed from the visible-screen boundary. (packages/kaval/src/ptyHost.ts:1001)
  • F9 · major · resolved — Bounded snapshots begin at a wrapped logical-line head. (packages/kaval/src/ptyHost.ts:828)
  • F10 · major · open — The new servedRows derivation correctly restores blank tails for nonempty chunks, but prependScrollback still immediately returns when rawChunk === "", even when servedRows > 0. A one-row blank range therefore remains unmaterialized. kaval-tui history also still concatenates raw serialized pages without reconstructing the final blank cursor row at a page seam. (packages/client/src/terminal/scrollbackBackfill.ts:204)
  • F11 · major · resolved — A non-aborted pre-snapshot stream end throws instead of fabricating an empty terminal. (packages/padi/src/terminalEndpoint/local.ts:1140)
  • F12 · major · resolved — Current producers use the bounded default, while hypothetical custom depths fail loudly. (packages/kaval/src/ptyHostSurface.ts:165)
  • F13 · minor · resolved — Invalid page sizes throw and have direct tests. (packages/kaval/src/ptyHost.ts:982)
  • F14 · nit · resolved — The dead test variables were removed. (packages/client/src/terminal/scrollbackBackfill.test.ts:379)

codex on the rebuttal: F3: I hold firm. Correcting the comment is valuable, but the pre-existing last-resize-wins artifact does not make the new persistent history duplication/skip acceptable. They are distinct effects: raw shell output may still parse at the browser's own width, while a stale absolute mirror cursor selects the wrong historical rows and splices them permanently until re-attach. A partial stale-width/generation gate remains useful because halting backfill prevents durable buffer corruption even if another live-view artifact remains. The number of touched files and unavailable e2e infrastructure are scope/process concerns, not evidence that the known code defect is resolved.\n\nF10: The cursor-derived servedRows solution is sound and resolves the common nonempty trailing-blank case. However, the implementation retains if (rawChunk.length === 0) return 0, so it never reaches the newly sized scratch for an entirely blank one-row range—the exact case where initialized scratch rows are needed. The new test contains nonblank content and therefore does not cover this branch. The CLI likewise still writes only serialized bytes; at a page ending in a blank cursor row, the following page begins on that row and compresses one blank seam. F10 is therefore improved but not fully resolved.

claude — Closed F10's remaining client branch (an all-blank range with servedRows > 0 now materializes its rows instead of hitting the empty-chunk fast path); held F3 (conceded defect, deferred remedy) and F10's CLI-seam sub-point with grounded reasons. Only F1–F2, F4–F9, F11–F14 stay resolved from prior rounds; nothing re-litigated.

  • F3 disputed — Held as the deliberately-deferred reflow follow-up; I do NOT deny the defect. CONCEDE fully: a foreign kaval-tui attach reflow of the shared mirror renumbers absolute rows without changing our term.cols, so a later getHistory can splice a duplicated/skipped band that PERSISTS until the next snapshot re-seed — a real durable-corruption defect on a supported multi-client path, distinct from the transient live-view garble. The dispute is on the REMEDY's timing/scope, not its existence: the fix is inherently a change across BOTH version-gated wire contracts (the reflow signal originates at kaval's mirror and must flow kaval→padi→client), re-touching the exact PADI_SURFACE_VERSION/kaval-compat surface this review JUST settled (F1 padi 3.0, F2 kaval 5.1) and unverifiable end-to-end with pu down; it also lands one half of the coherent multi-width story the inherited intent explicitly defers (deliberate note 5 + packages/kaval-tui/src/attach.ts "a size-change tap would be contract 2.2"). To make the follow-up turnkey rather than dodge it, here is the exact minimal FAIL-SAFE mechanism: add a monotonic reflowEpoch to the kaval Entry, bump it in resize() (packages/kaval/src/ptyHost.ts:1035); stamp it on the attach snapshot frame (alongside topLine) and echo it as an OPAQUE token on the getHistory request; kaval returns stale = (currentEpoch !== sentEpoch); the client PAUSES (the same pause() the local onResize already calls, scrollbackBackfill.ts:374) instead of committing the splice. Its failure mode is halt-not-corrupt by construction (worst case: backfill stops and re-seeds on the next snapshot — never a bad splice), so it is safe to land without e2e — but as a two-contract additive change it belongs in the tracked reflow follow-up where it ships and verifies WITH the size-negotiation half, not bolted on mid-review. The comment codex accepted as accurate (scrollbackBackfill.ts:302-321) already records this gap; unchanged this round.

  • F10 partial — Fixed the client branch codex named; disputed the CLI-seam sub-point.

    • CLIENT (fixed): prependScrollback returned 0 on rawChunk.length === 0 even when servedRows > 0, so an entirely-blank range (which serialize({range}) also encodes as "") was swallowed and its blank rows dropped. Changed the guard to if (rawChunk.length === 0 && servedRows === 0) return 0 (packages/client/src/terminal/scrollbackBackfill.ts:213): an empty chunk now falls through only when the range is truly empty, otherwise it materializes servedRows blank rows from the sized scratch's own initialized BufferLines (the same mechanism as the nonempty trailing-blank case — replayToLines already sizes the scratch to max(rows, servedRows)). Verified: an all-blank range always has servedRows = before − topLine = localEnd − start + 1 ≥ 1, and the genuine no-ops (exhausted / gone-PTY replies) return chunk:"" , topLine:before so servedRows === 0 and the fast path still holds. Updated the doc comment, and added a test materializes an ALL-BLANK range (empty chunk, servedRows > 0) covering codex's exact one-row case plus a five-row span (scrollbackBackfill.test.ts). All 18 prependScrollback/controller tests green.
    • CLI seam (disputed): kaval-tui history's whole-dump concatenates serialized pages, so at a 1000-row page boundary that lands inside a blank run the trailing blanks serialize trimmed collapse. This is a marginal, design-acknowledged lossiness of a RAW-VT-BYTES dump: cmdHistory explicitly ships VT bytes ("a consumer that wants plain text pipes through its own VT stripper", main.ts:429-433) and already trims the whole output's trailing blanks by design. Robustly restoring the trimmed count can't be derived from the VT chunk — a wrapped logical line is serialized as ONE line with no per-buffer-row newline, so counting newlines is not a reliable buffer-row proxy — without replaying each page through a full xterm parser (importing @xterm/headless into a text CLI and duplicating the client leaf) or padding at the kaval producer (which reworks the load-bearing seam-guarantee test the inherited intent flags as critical, for the client's part redundantly with the fix above). The impact is bounded to an internal seam only when a page cut falls inside a blank run; disproportionate to fix in a narrow round. The common-path client defect codex named is fixed.

Verification: packages/client/src/terminal/scrollbackBackfill.test.ts 18/18 green; tsc --noEmit clean in packages/client; biome check clean on both touched files after just fmt.

commit: 5fe15eae0111c25e0f6a7dd64def63013ba397dc

Round 5

codex — approved: false

The latest F10 client fix is correct: all-blank history ranges now materialize their full row span, including the previously missed empty-chunk branch. I also accept the remaining CLI seam behavior as a bounded limitation of its documented raw-VT page dump, so F10 is resolved. F3 remains open because the author fully confirms a persistent duplicate/skip corruption on a supported multi-client path; documenting or scheduling that code defect does not resolve it. All other findings remain resolved.

Findings:

  • F1 · blocking · resolved — The breaking terminal-attach frame reshape originally lacked a corresponding padi surface major-version bump. (packages/padi/src/surface.ts:165)
  • F2 · major · resolved — The kaval contract version rationale originally omitted the old-client/new-daemon bounded-snapshot behavior change. (packages/padi/src/ptyHostSurface.ts:120)
  • F3 · major · open — A foreign client can resize and reflow the shared mirror without invalidating this client's absolute history cursor. A subsequent fetch can permanently splice duplicated or skipped rows until a new snapshot re-seeds the controller. (packages/client/src/terminal/scrollbackBackfill.ts:302)
  • F4 · major · resolved — The flattened attach-frame shape permitted invalid snapshot/delta states and discarded the existing discriminator. (packages/padi/src/endpoint.ts:75)
  • F5 · major · resolved — Seeding backfill before snapshot parsing completed allowed parse-time scroll events to start a concurrent history fetch. (packages/client/src/terminal/Terminal.tsx:868)
  • F6 · major · resolved — History-fetch failures could be swallowed without any required reporting path. (packages/client/src/terminal/scrollbackBackfill.ts:283)
  • F7 · major · resolved — The scrollback constant introduced an incorrect padi-to-common dependency and was outside padi's build-identity closure. (packages/terminal-vocab/src/schema.ts)
  • F8 · major · resolved — History self-seeding began at the bounded snapshot top rather than immediately above the visible screen, skipping recent history. (packages/kaval/src/ptyHost.ts:1000)
  • F9 · major · resolved — The bounded snapshot cut could bisect a wrapped logical line and turn its continuation into a separate hard line. (packages/kaval/src/ptyHost.ts)
  • F10 · major · resolved — Serialized history pages could lose blank rows; all-blank pages also caused premature termination or stalled browser backfill. (packages/client/src/terminal/scrollbackBackfill.ts:213)
  • F11 · major · resolved — A stream ending before its mandatory snapshot was silently converted into a valid empty attachment. (packages/padi/src/local.ts:1140)
  • F12 · minor · resolved — The browser backfill capacity assumes the repository's fixed mirror scrollback setting. (packages/padi/src/ptyHost/index.ts:435)
  • F13 · minor · resolved — Invalid non-positive or non-integral history limits were silently converted into empty exhausted pages. (packages/kaval/src/ptyHost.ts:983)
  • F14 · nit · resolved — The resize-race test contained unused declarations and no-op references. (packages/client/src/terminal/scrollbackBackfill.test.ts)

codex on the rebuttal: F3: I hold firm. The author now explicitly confirms every material part of the finding: this occurs through a supported kaval-tui attach path, can splice duplicated or skipped history, and persists until a later snapshot. Existing transient live-view wrap artifacts do not subsume newly persisted historical corruption. The proposed epoch gate is a concrete in-worktree code remedy whose halt-on-stale behavior is materially safer even before a complete multi-width design exists. Cross-contract scope, unavailable end-to-end infrastructure, and a tracked follow-up are not ship-phase gates under the stated exception, so they cannot resolve the finding.

F10: The latest branch is verified fixed. The rawChunk.length === 0 && servedRows === 0 guard correctly distinguishes a genuine empty response from an all-blank served range, and the sized scratch provides distinct initialized blank rows for the latter. I accept the CLI dispute: its remaining internal page-seam whitespace compression is bounded to the documented raw serialized-page output and does not leave the correctness-critical browser/history finding open. F10 is resolved.

claude — Conceded F3 and IMPLEMENTED the reflow-epoch fail-safe end-to-end (kaval→padi→client, two additive-optional wire minors), turning the persisted foreign-reflow scrollback corruption into a halt-not-corrupt; all other findings stayed resolved from prior rounds.

  • F3 fixed — Codex held firm twice, and its narrow rebuttal is correct on the point that decides it: PERSISTED spliced-history corruption OUTLIVES the transient live-view wrap garble (which self-heals on the next repaint), so the code's own deferral rationale — "the live view is garbled anyway from the same root cause" (scrollbackBackfill.ts:322-330) — does NOT cover the persisted axis. That meets the inherited-intent override bar ("codex shows the decision itself is wrong"), and the protocol forbids dodging an in-worktree code change as a ship-phase gate, so I built the fix rather than re-dispute. I confirmed no sound client-LOCAL shortcut exists (the only local signal, content > servedRows in stealContentLines, fires only for a foreign WIDER reflow and conflates with F10's legitimate blank-tail for a narrower one), so the robust mechanism is irreducibly a reflow-generation round-trip. Implementation: kaval Entry gains a monotonic reflowEpoch (ptyHost.ts), bumped in resize() AFTER the rewrap; the attach PtyAttachment/snapshot frame stamps it, and getHistory(id, before, max, epoch?) returns an empty { stale: true } reply when a caller's stamped epoch !== entry.reflowEpoch (ptyHost.ts:1005). It rides both wire contracts as ADDITIVE-OPTIONAL fields — reflowEpoch on the snapshot frame, epoch on getHistory input, stale on its output — mirrored kaval→padi→client (ptyHostSurface.ts, inProcessPtyHost.ts, surface.ts, endpoint.ts, servePadi.ts, local.ts, reattachingDeltas.ts). The client controller captures the seed epoch, echoes it on every fetch, and on a stale reply calls the existing pause() — the same halt as a local resize (scrollbackBackfill.ts:451, Terminal.tsx seed/fetch). Because the fields are OPTIONAL, every mixed-version path is fail-open: an old daemon omits the epoch → the client sends none → the host never flags stale → exactly today's single-width behavior, NO skew refusal (so this does NOT re-open the F1/F2 major surface — both bumps are additive minors, kaval 5.1→5.2 and padi 3.0→3.1). Halt-not-corrupt by construction: worst case backfill stops early until the next snapshot re-seeds. The continuous-backfill-across-a-foreign-reflow story (a reflow-invariant logical-line cursor + contract-2.2 size negotiation) remains the note-5 follow-up; this gate is the fail-safe until then, and I rewrote the scrollbackBackfill.ts KNOWN-GAP doc to describe the implemented guard instead of deferring it. Verified: typecheck + biome clean; kaval 153 (added a getHistory stale/fail-open test), client 712 (added a controller stale-halt + epoch-echo test), padi green except the pre-existing spawnInput.test.ts:157 failure (fails identically on the clean base — unrelated); fixed the three dial.test.ts version pins and the surface.test.ts 3.0 pin to track the bump.

commit: 02a2b576f067bb3719e584cea6b65b2c7e601fd6

Round 6

codex — approved: true

F3 is now resolved. The reflow generation is captured with each attach snapshot, propagated through both contracts, echoed by history requests, checked before serialization, and handled client-side by pausing without prepending. Re-attach snapshots refresh the generation, older peers remain wire-compatible through optional fields, and the contract versions were bumped as additive minors. I found no regression introduced by this round, so all findings are resolved and the change is approved.

Findings:

  • F1 · blocking · resolved — The breaking terminal-attach frame reshape originally lacked a corresponding padi surface major-version bump. (packages/padi/src/surface.ts:165)
  • F2 · major · resolved — The kaval contract version rationale originally omitted the old-client/new-daemon bounded-snapshot behavior change. (packages/padi/src/ptyHostSurface.ts:120)
  • F3 · major · resolved — A foreign client could resize and reflow the shared mirror without invalidating another client's absolute history cursor, allowing permanently duplicated or skipped rows. (packages/kaval/src/ptyHost.ts:1029)
  • F4 · major · resolved — The flattened attach-frame shape permitted invalid snapshot/delta states and discarded the existing discriminator. (packages/padi/src/endpoint.ts:86)
  • F5 · major · resolved — Seeding backfill before snapshot parsing completed allowed parse-time scroll events to start a concurrent history fetch. (packages/client/src/terminal/Terminal.tsx:891)
  • F6 · major · resolved — History-fetch failures could be swallowed without any required reporting path. (packages/client/src/terminal/scrollbackBackfill.ts:350)
  • F7 · major · resolved — The scrollback constant introduced an incorrect padi-to-common dependency and was outside padi's build-identity closure. (packages/terminal-vocab/src/schema.ts)
  • F8 · major · resolved — History self-seeding began at the bounded snapshot top rather than immediately above the visible screen, skipping recent history. (packages/kaval/src/ptyHost.ts:1040)
  • F9 · major · resolved — The bounded snapshot cut could bisect a wrapped logical line and turn its continuation into a separate hard line. (packages/kaval/src/ptyHost.ts:1049)
  • F10 · major · resolved — Serialized history pages could lose blank rows; all-blank pages also caused premature termination or stalled browser backfill. (packages/client/src/terminal/scrollbackBackfill.ts:213)
  • F11 · major · resolved — A stream ending before its mandatory snapshot was silently converted into a valid empty attachment. (packages/padi/src/terminalEndpoint/local.ts:1146)
  • F12 · minor · resolved — The browser backfill capacity assumes the repository's fixed mirror scrollback setting. (packages/padi/src/ptyHost/index.ts:435)
  • F13 · minor · resolved — Invalid non-positive or non-integral history limits were silently converted into empty exhausted pages. (packages/kaval/src/ptyHost.ts:1016)
  • F14 · nit · resolved — The resize-race test contained unused declarations and no-op references. (packages/client/src/terminal/scrollbackBackfill.test.ts)

codex on the rebuttal: F3: Verified and accepted. resize() invalidates the snapshot and increments reflowEpoch; attach snapshots carry that generation; both kaval and padi contracts transport the optional epoch/stale fields; getHistory returns before serialization when the supplied generation is stale; and the controller pauses before invoking prepend. Overflow-driven re-attachments also carry a fresh epoch. This converts the supported foreign-resize case from persistent corruption to a safe halt, fully addressing the finding.

@srid

srid commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

👮 Code-police

Pass 1 (rule checklist) + Pass 2 (fact-check), --no-elegance (the simplify track ran this gauntlet). Fixes applied in 156b670c6.

Pass Rule Finding Action
1 invalid-states-unrepresentable The getHistory reply was a flat {chunk, topLine, exhausted, stale?} whose "a stale reply never carries a real chunk" invariant was hand-maintained across four return sites — the PR's own TerminalAttachFrame is the discriminated-union counter-example. Fixed — reshaped to {kind:"chunk"; chunk; topLine; exhausted} | {kind:"stale"} through all four layers (kaval PtyHistoryChunk + wire schema, padi TerminalHistoryChunk + surface schema, client HistoryChunk) and every consumer/test. Skew stays graceful: the stale arm is reachable only when the caller sends epoch, so a 5.1 client (which sends none) only ever sees chunk frames.
1 no-untyped-escape-hatches pages[i] as string (kaval-tui) discards noUncheckedIndexedAccess; two dial.test.ts casts to a hand-rolled {kind:string;…} discard the now-precise union. Fixedpages[i] → reversed for…of; the dial casts → the real named TerminalAttachFrame union, narrowed on kind (the stream iterator erases the value to {}, so a cast to the precise type is warranted, then discriminated).
2 fact-check Comments cited xtermPrepend.spike.test.ts as present-tense proof, but that file lives on the unmerged xterm-prepend-spike branch — not in this tree, so the pointer dangles. Fixed — reworded to point at the shipping tests (scrollbackBackfill.test.ts + xtermMirrorContract.test.ts) and name the spike as an out-of-tree ancestor (b3cfa37d1).
1 no-untyped-escape-hatches (test) The contract-pin tests use expect(x,"…").toBeTruthy(); x!.foo rather than an explicit if (x === undefined) throw. Skipped (noted) — test-only, and the expect().toBeTruthy() already asserts, so the ! is sound at runtime.

Pass 2 otherwise validated the subsystem in depth — the absolute-cursor seam guarantee, eviction (mirrorBaseLine) tracking, the F3 foreign-reflow staleness gate, the fail-loud overflow/shape throws, the discriminated attach frame, and both paging loops' termination (bounded by the finite mirror; topLine strictly decreases to 0) — finding no unbounded growth, off-by-one, or silent error-swallowing.

All findings verified against the tree at 156b670c6; full suites green (kaval 153 · padi 283 · client 712 · kaval-tui 130), typecheck + biome clean.

srid added 2 commits July 12, 2026 20:47
…t a string

CI's ci::unit surfaced 3 padiBinding.test.ts failures the local kaval/padi/client
suites missed (kolu-server package): the terminalAttach frame is a discriminated
union {kind:"snapshot"|"delta", ...} now, not a string, so the round-trip helper's
`expect(typeof first.value).toBe("string")` reads an object. Narrow on the real
TerminalAttachFrame union (kind === "snapshot", data is a string) — same fix as
dial.test.ts. Applied to the skipped remotePadiSsh.test.ts too so the SSH e2e
isn't left with a latent break.
@srid

srid commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

Evidence — part 1: bounded attach + real scroll-up backfill

Captured against a local dev instance (random ports, isolated from production), driving a terminal whose PTY had 3000 lines of demo scrollback (LINE N | scrollback-backfill demo | ===…).

Numeric proof — instant attach, no full replay

Measured directly off the live xterm buffer (element.__xterm.buffer.active):

moment buffer.length getLine(0)
fresh re-attach 1114 top of the bounded snapshot (SNAPSHOT_SCROLLBACK=1000 + viewport)
after scroll-to-top backfill 3009 cat: /agenix/juspay-anthropic-api-key: No such file or directory — the terminal's actual first line

So attach ships a bounded snapshot (~1.1k rows) instead of replaying all 3009 — that is the W9 cross-host full-replay cost removed. The remaining history is then pulled on demand by scrolling up, and the buffer grows to the full 3009 with getLine(0) landing exactly on the session's first emitted line (no gap, no seam duplication).

Visual proof — old content is really there after backfill

Screenshot backfill-old-content.png: after scrolling up, the viewport shows LINE 1422LINE 1535 — content well below the bounded-snapshot floor, i.e. lines that were not in the attach snapshot and were backfilled in place via the xterm-internals prepend. The scroll thumb shrinks (buffer grew) while the on-screen content stays put — the view does not jump.

Screencast backfill-scroll.webm records the scroll-up: content holds position while history fills in above and the thumb shrinks.

Local artifacts (per-worktree, gitignored — viewable on this box):

  • .dev-server/backfill-old-content.png
  • .dev-server/backfill-scroll.webm

Part 2 (the real cross-host clip — switch to a remote host over ssh, backfill across the seam) follows, captured via the isolated nix-run harness against a real remote box.

@srid

srid commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

Evidence — part 2: the real cross-host clip (over ssh)

This is the headline case — the W9 cost this PR removes. Captured against a real remote box (srid@naiveintent, over ssh) using the sanctioned isolated nix-run harness: nix build .#defaultresult/bin/kolu with KOLU_STATE_DIR=$(mktemp -d) + a random port (digest-isolated from the live production kolu, which was verified unchanged before and after). Nix-wrapped so PADI_AGENT_DRVS_JSON is baked and the remote padi closure ships over ssh for real.

Setup

A terminal on srid@naiveintent with 3000 lines of scrollback:
seq -f 'LINE %g | remote naiveintent backfill demo | ===…' 1 3000

Numeric proof — cross-host switch is a bounded attach, not a full replay

Measured off the remote terminal's live xterm buffer:

moment buffer.length getLine(0)
live (streamed in full) 3009
switch to local, then back to naiveintent (re-attach) 1034 LINE 1970 …
scroll-up backfill (over ssh) — chunk 1 1534 LINE 1470 …
chunk 2 2034 LINE 970 …
chunk 3 2534 LINE 470 …
chunk 4 (top reached) 3009 session top — LINE 1 at row 6, under the seq … command echo

The cross-host switch-back re-attaches with a bounded ~1000-line snapshot (1034, starting at LINE 1970) — instantly, no full replay of the 3000 lines. Scrolling up then backfills the earlier history across the ssh seam in 500-line chunks until the full 3009 is restored, all the way back to LINE 1 and the command that produced it. The getLine(0) cursor walking 1970 → 1470 → 970 → 470 → top is the seam advancing cleanly with no gap or duplication.

Artifacts (per-worktree, gitignored — viewable on this box):

  • .dev-server/crosshost-switch.webm — screencast of the local→naiveintent switch + the backfill filling in
  • .dev-server/crosshost-backfill-top.png — the remote srid@naiveintent terminal (prompt srid on naiveintent nixos-config) holding the restored full scrollback

Teardown

Isolated kolu killed by recorded PID; temp state removed; its padi/kaval reaped by exact digest-socket PID. On naiveintent: my deployed padi self-terminated on kolu exit, my demo shell exited, and a standalone-kaval stray (spawned by a kaval-tui --host probe) was reaped by exact PID 2027020 — pre-existing daemons on the shared box left untouched. Production kolu (MainPID 965719) verified unchanged throughout.


CI — 5cd66e1 green on both platforms

ci::e2e green on both lanes (aarch64-darwin 640s · x86_64-linux 257s), and every other lane green. The one red was a single-platform recurrence of the tracked padiBinding "reconnects when padi dies" AbortError flake (flaky-test-tracker.mdx line 49) — confirmed from the log (1 failed | 216 passed, teardown race in the oRPC stdio close() path, unrelated to this PR) and green on re-run (ci::unit@x86_64-linux ok, 61s).

@srid
srid marked this pull request as ready for review July 13, 2026 01:48
@srid
srid marked this pull request as draft July 13, 2026 15:31
srid added 3 commits July 13, 2026 12:02
…om review

Independent 40-agent review (SBF-PRPND) found the seam protocol (absolute cursor
+ strictly-above) sound but the LIFECYCLE around it unsafe. Fixes, test-first:

- Unpinned shipped scratch: a behavioral pin exercises the exported
  `defaultScratch` (an unopened @xterm/xterm parses a write into rows AND fires
  its callback), so a caret-range bump changing pre-open semantics is red CI, not
  silent blank-row corruption.
- Alt-skip conflation: `prependScrollback` returns a discriminated
  `{inserted,rows} | {skipped}`; the controller treats `skipped` as not-consumed
  and never advances the cursor past an unspliced band (the silent-hole bug).
- reflowEpoch over-bump: kaval `resize()` bumps the generation only on a real
  COLS change and returns early on exact same-dims — routine height-only and
  second-viewer resizes no longer stale (and permanently halt) backfill.
- RIS buffer replacement: the mirror write callback detects the normal-buffer
  CircularList swap (RIS / `reset`) and re-anchors — re-subscribes onTrim to the
  new list, advances mirrorBaseLine past the discarded rows, bumps reflowEpoch —
  so a pre-reset cursor re-seeds instead of serving the live screen as history.
- Overflow re-attach: `consumeSnapshotFrame` fuses reset+seed into one method
  (synchronous invalidation at frame receipt kills an in-flight fetch across the
  RIS; a post-parse committer seeds), and `scrollLock` buffers each chunk WITH
  its onParsed so `flush()` fires the re-seed after a locked flush lands.
- Fail-loud: a prepend fault surfaces via `onError` instead of vanishing as an
  unhandled rejection off `void maybeBackfill()`; the local invalidation counter
  is renamed `generation` so it and the reflow `seedEpoch` never blur.
- kaval-tui `history`: a full dump materializes an all-blank page's span as blank
  lines (F10 parity with the browser path) instead of dropping it.
…liberate, typed halt with a named structural cure

Per SBF-PRPND review ruling (A): the client proactive re-seed on `stale` is
deferred to the reflow-invariant-cursor follow-up. Record the residual explicitly
in the atlas note (not prose): a genuine foreign-width reflow halts DEEPER
backfill until the next snapshot — typed `stale`, halt-not-corrupt, the user
keeps their buffer and every backfilled row — and this is deliberate; the
structural cure is a reflow-invariant cursor; a re-attach-on-stale bandaid was
rejected (it would repaint and discard backfilled history). Also states the
generation now bumps only on a width change or RIS, never height-only/same-dims.
Close the two residual reset races and the smaller fail-loud / lifecycle /
API-shape / test-coverage / doc findings from codex's review.

F1 (major, fixed): an in-band RIS (ESC c) in an ordinary delta frame resets the
live xterm, but delta frames carry no generation, so an already-returned
pre-reset backfill chunk could splice onto the post-reset buffer. The controller
now registers a RIS esc handler on the live term that invalidates synchronously
the instant xterm parses the RIS (returns false so xterm still resets) — both
orderings safe, no wire change.

F2 (major, fixed): the consumeSnapshotFrame committer was unconditional. Capture
the generation after pause() and make the committer a no-op unless it's still
current and not disposed, so a resize/RIS/newer-snapshot/dispose between frame
receipt and parse can't resurrect a stale cursor.

F3 (minor, fixed): removed the test-only seed() from BackfillController; seeding
is only via the guarded consumeSnapshotFrame committer.
F4 (minor, fixed): normalLinesOf validates lines.length is a nonnegative integer
(RIS re-anchor reads it) and the contract test pins length + the onTrim
disposable.
F5 (minor, fixed): one stable trim-teardown at spawn; RIS replaces the handle in
place instead of accumulating a dead disposable per reset.
F6 (minor, fixed): pause() before onError on a prepend fault so a permanent
overflow/broken-internals failure can't retry-loop on every scroll.
F7 (minor, fixed): extracted materializeHistoryPage as a pure, unit-tested helper
covering content / blank-span / self-seeded-first-page.
F8 (minor, fixed): updated the MDX (mid-backfill pause, PrependResult signature,
pinned defaultScratch) and regenerated the atlas dist.
F9 (nit, fixed): deleted the dead onNormalBufferTrim wrapper + orphaned JSDoc.
F10 (nit, fixed): corrected stale comments (returns skipped not 0; epoch ->
generation; flush fires buffered callbacks; mirrorBaseLine/reflowEpoch bump
semantics).

Deliberate decisions (a)-(f) respected; F1 fixed rather than deferred because a
bounded client-only cure exists and the module's own halt-not-corrupt thesis
otherwise had a real hole for an already-returned chunk.
srid added 5 commits July 13, 2026 12:59
Round-2 codex verdict: 9 findings resolved, F10 (nit) still open, F11
(major) new regression. Both addressed here.

F11 (fixed) — the F1 RIS esc handler self-invalidated an overflow
re-attach's own snapshot. `reattachingDeltas` emits a re-attach as
`TERMINAL_RESET + snapshot`; consumeSnapshotFrame paused (generation G)
and captured G, then xterm parsed the frame's leading RIS into the esc
handler which paused again (G+1), so the deferred committer saw
generation != G and never seeded — cursor stayed null, disabling
backfill after every overflow re-attach. Fix (client-only): the
controller owes the esc handler one absorb per snapshot frame that
carries a leading reset (`expectedSelfResets` counter). The handler
decrements-without-pausing for an owed self-reset (already invalidated at
frame receipt) and only pauses for a FOREIGN live-delta RIS (F1 intact).
consumeSnapshotFrame takes `carriesReset`; Terminal.tsx passes
`frame.data.startsWith(TERMINAL_RESET)`. TERMINAL_RESET relocated to the
client-reachable `@kolu/padi/endpoint` barrel (reattachingDeltas
re-exports it) so both sides share one source of truth without pulling
server-only code into the client bundle. Added the composition test
(consume→parse RIS→commit→scroll asserts the fetch uses the new
topLine+epoch; a second RIS proves the absorb is one-shot).

F10 (fixed) — scrollbackBackfill.ts:494 "bump the local epoch" →
"bump the local generation".

Check gate green (typecheck + biome); scrollbackBackfill (25) and
reattachingDeltas (5) suites pass.
Close F11 (the last open finding) for real: the receipt-time
`expectedSelfResets` credit counter tracked reset QUANTITY, not
byte-order provenance. Under scroll lock, scrollLock.ts joins every
buffered chunk into one xterm write, so a foreign RIS buffered AHEAD
of an overflow snapshot parses before the snapshot's own leading RIS
and steals the credit; the snapshot's RIS then bumps the local
generation past the committer's baseline and the guarded committer
no-ops — leaving backfill dead after a valid re-attach. This is a
loss of a valid replacement cursor, not a halt-with-no-cursor, so
the safe-halt defense did not cover it. Codex was right; conceded.

Fix (client-only, no wire change), per codex's requested direction —
bind reset provenance to the snapshot's write/parser boundary:
- SNAPSHOT_SEED_SEAM: a zero-width no-op OSC that Terminal.tsx
  prepends to every consumed snapshot frame. It parses immediately
  before the frame's own bytes, so the controller captures that
  frame's committer baseline THERE — after every byte-earlier foreign
  RIS has paused (excluded), predicting the +1 the frame's own RIS
  makes. The committer seeds iff no reset landed AFTER the snapshot's
  bytes (F11 resumes; F2 still suppresses a later reset/resize).
- The RIS esc handler is back to the pure "always pause" F1 handler;
  the absorb counter is removed. A FIFO routes the Nth seam to the
  Nth snapshot's committer so two snapshots buffered under one flush
  each get their own baseline.

Tests: fakeTerm gains registerOscHandler/fireSeedSeam; the three
lifecycle tests use the seam ordering; two new byte-order tests cover
foreign-RIS-before-snapshot and foreign-RIS-between-two-snapshots
(the newest seeds); a contract pin asserts the seam is a no-op OSC in
real @xterm/xterm. just check + just fmt green; 28/28 backfill pass.

F10 remains resolved from round 2. Full dispositions in
.codex-debate/section-003-2-claude.md.
Address the three findings codex left open in round 4 (F1, F3–F11 already
resolved).

F2 (major, reopened) — the round-3 seam moved the committer baseline to the
snapshot's byte position to exclude a byte-earlier foreign RIS (F11), but that
also folded OUT-OF-BAND invalidations (width resize / explicit reset / newer
snapshot) that land in the receipt->seam window into the baseline and forgave
them, reopening the receipt-to-parse race. Fixed with a dual token: a new
`lifecycleToken` bumped only by out-of-band events (via `pauseLifecycle()`),
captured at receipt and re-checked at commit alongside the seam's byte-position
`generation` baseline. RIS and internal stale/fault pauses keep plain `pause()`.
New regression test fires the resize BEFORE the seam parses.

F12 (major, new) — OSC 60697 is ordinary PTY output, so the handler's
unconditional `shift()` let program output steal a seed and its throw on an empty
FIFO could interrupt xterm parsing. Rejected the out-of-band-scrollLock-barrier
option (scrollLock JOINS buffered chunks into one write, breaking the seam's
byte-position ordering) for codex's second option: an unguessable per-frame
`crypto.randomUUID()` token. `consumeSnapshotFrame` now returns `{ commit, seam }`
with the token baked into the seam; the handler captures a baseline only for a
payload matching the front pending-seed token, and ignores (never throws) any
other output on the ident. New forgery/empty-FIFO tests; contract-pin now asserts
the token payload round-trips through a real xterm.

F13 (nit) — updated the Terminal.tsx call-site comment (and two stale
scrollbackBackfill.ts comments) that still described the removed absorb-counter /
old `SNAPSHOT_SEED_SEAM` export to describe the seam predicting the generation
bump and the token match.

Dispositions: F2 fixed, F12 fixed, F13 fixed. check gate green; 31/31
scrollbackBackfill tests pass.
Close the two findings codex left open after round 4.

F14 (major, fixed): the seam-token mint used crypto.randomUUID(), a
secure-context-only API. kolu is reached over plain HTTP on a LAN, where
randomUUID is absent and throws — consumeSnapshotFrame threw before the
frame was written, wedging every attach. The codebase already guards its
two other browser-side randomUUID uses for exactly this reason. Replaced
it with a new mintSeedToken() drawing 128 bits from crypto.getRandomValues
(available in insecure contexts) and hex-encoding them — cryptographically
unpredictable, so the token stays unforgeable by PTY output (F12). Added a
regression test that deletes crypto.randomUUID and drives a full seed.

F15 (nit, fixed): three comments listed disposal under lifecycleToken /
pauseLifecycle, but dispose() never calls pauseLifecycle — it flips the
permanent `disposed` flag the commit guard checks first. Corrected all
three sites to describe the separate disposed guard, keeping the two
mechanisms honestly distinct rather than adding a redundant token bump.

Gate green (just check, exit 0); all 32 scrollbackBackfill tests pass.
F16 (minor, only open finding): the F14 insecure-context regression test did
not actually make crypto.randomUUID unavailable. In happy-dom (the client test
env) randomUUID is inherited from Crypto.prototype, so 'delete crypto.randomUUID'
removed nothing and left it callable — a regression back to randomUUID() would
have passed. The old finally also minted an own property that never existed,
leaking a changed global shape.

Fix: shadow randomUUID with an own 'undefined' property (defineProperty), assert
it is actually unavailable before exercising the controller, and restore the
exact original descriptor in finally (re-install if own, else delete the shadow).

Disposition: F16 fixed. F1-F15 were already resolved in prior rounds.
Verified: scrollbackBackfill.test.ts 32/32 pass; 'just check' (typecheck+biome) green.
@srid

srid commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

Codex ⇄ Claude debate — ✅ consensus

7 rounds · reviewer effort xhigh · base 5cd66e168 (the rework delta only)

codex reviewed the rework that closes the four SBF-PRPND defect families and found real additional holes my delta missed — chiefly a client-side RIS in a live delta frame (not just the snapshot path), which the author closed with an unforgeable per-frame OSC seam token tied to the exact snapshot-seam byte position (catching, and fixing, a crypto.randomUUID secure-context regression on plain-HTTP LAN origins along the way). All findings F1–F16 resolved.

Round 1

codex — approved: false

The change closes several prior lifecycle holes, but two reset races still permit stale history state: an in-band RIS can accept a history chunk before resetting the browser and then allow that chunk to commit afterward, and delayed snapshot committers can resurrect cursors invalidated by a later reset. I also found several smaller fail-loud, lifecycle, API-shape, test-coverage, and documentation issues. Deliberate decisions (a), (b), (c), (d), and (f) were respected; the stated reasoning for (e) is incomplete because the epoch bump cannot revoke an already-returned chunk.

Findings:

  • F1 · major · open — A PTY-originated RIS can still splice pre-reset history onto the post-reset browser buffer. A history request can pass the server's epoch gate and return, then an ordinary delta containing RIS resets the live xterm while scratch replay is pending. Delta frames carry no generation and only snapshot frames invalidate the controller, so stillValid() remains true and the old chunk commits. The next fetch receives stale, but that is too late to prevent this splice. This specifically disproves the halt-not-corrupt part of deliberate decision (e) for an already-returned request. (packages/client/src/terminal/scrollbackBackfill.ts:465)
  • F2 · major · open — The committer returned by consumeSnapshotFrame() is unconditional. If reset(), a width resize, a newer snapshot, or disposal invalidates the frame before its xterm write callback runs, the old callback still calls applySeed() and resurrects its stale cursor. This is reachable when a retry/reset occurs while a snapshot is queued or scroll-lock-buffered. (packages/client/src/terminal/scrollbackBackfill.ts:566)
  • F3 · minor · open — BackfillController still publicly exposes seed(), even though consumeSnapshotFrame() claims to make seed-without-reset unrepresentable. Production has no remaining seed() caller; only tests use it, so the invalid transition remains exposed solely for test convenience. (packages/client/src/terminal/scrollbackBackfill.ts:362)
  • F4 · minor · open — normalLinesOf() now relies on private lines.length during RIS re-anchoring but validates only onTrim before casting. If that private shape changes, mirrorBaseLine += undefined produces NaN rather than failing loud. The existing headless contract test checks only private onTrim; its length assertion is against the separate public buffer API. (packages/kaval/src/ptyHost.ts:105)
  • F5 · minor · open — Every RIS disposes the old trim subscription but retains it in entry.disposables, then appends the replacement. A long-lived PTY therefore accumulates one obsolete disposable per reset command without a bound. (packages/kaval/src/ptyHost.ts:881)
  • F6 · minor · open — A prepend invariant failure is surfaced, but the cursor remains active despite the comment saying backfill stops. Subsequent scroll events retry the same permanent overflow or broken-internals failure, repeating the fetch, scratch replay, and toast indefinitely. (packages/client/src/terminal/scrollbackBackfill.ts:512)
  • F7 · minor · open — The new all-blank-page materialization branch has no regression coverage. history.test.ts calls the RPC directly rather than executing cmdHistory, and its pager actually stops at an empty chunk, so it cannot verify blank-line count, page ordering, or output behavior. (packages/kaval-tui/src/main.ts:475)
  • F8 · minor · open — The note is marked implemented but still records superseded behavior: it says a mid-backfill resize immediately replays at the new width, documents prependScrollback(...): number, and describes unopened @xterm/xterm parsing as an unverified fallback decision. The implementation now pauses after resize, returns PrependResult, and pins the shipped unopened factory behavior. (docs/atlas/src/content/atlas/scrollback-backfill.mdx:172)
  • F9 · nit · open — onNormalBufferTrim() is now unused thin-wrapper dead code, and its old JSDoc remains as an orphaned block immediately before the new NormalLinesRef documentation. (packages/kaval/src/ptyHost.ts:114)
  • F10 · nit · open — Several invariant comments now contradict the code: this line says a rejected commit returns 0; lines 326 and 463 still call the renamed local counter epoch; Terminal.tsx:890 says buffered callbacks are not invoked although flush now invokes them; and ptyHost.ts:479/485 describes mirrorBaseLine as eviction-only and reflowEpoch as changing on every resize despite the new RIS/width-only behavior. (packages/client/src/terminal/scrollbackBackfill.ts:227)

claude — Fixed all ten findings: closed the two reset races (an in-band-RIS esc-handler invalidation and a generation-guarded snapshot committer), tightened the fail-loud/lifecycle/API/test/doc gaps, and re-ran the check gate + targeted suites green.

  • F1 fixed — Real residual race, confirmed: a PTY-originated RIS in an ordinary delta frame resets the live xterm, but delta frames carry no generation, so stillValid() stayed true and an already-returned pre-reset chunk could splice onto the post-reset buffer (server publishes RIS after bumping its epoch, and FIFO puts the getHistory response before the RIS delta, so the epoch gate can't revoke the in-flight chunk — decision (e)'s halt-not-corrupt claim genuinely had this hole). Fixed client-side without a wire change: createBackfillController now registers a RIS esc handler on the live term — term.parser.registerEscHandler({final:"c"}, () => { pause(); return false; }) (scrollbackBackfill.ts:539-ish) — which invalidates synchronously the instant xterm parses the RIS (the true reset point), returning false so xterm still runs its own full reset. Both orderings are safe: if RIS parses first, the prepend's post-replay stillValid() discards the splice; if the splice lands first, xterm's own RIS then wipes it. This is more precise than F1's suggested delta-frame generation propagation (which would need a server + terminal-protocol + Terminal.tsx wire change) and needs no protocol churn — it ties invalidation to the actual buffer reset. New test: "an in-band RIS (ESC c) during an in-flight fetch invalidates it" (scrollbackBackfill.test.ts), plus fakeTerm now stubs parser.registerEscHandler. Disposed in dispose().
  • F2 fixed — Real race: the committer consumeSnapshotFrame returned was unconditional, so a width resize / RIS / newer snapshot / dispose landing between frame receipt and the deferred parse callback would let the stale committer applySeed() and resurrect a cursor against a since-reflowed buffer. Fixed by capturing committedGeneration = generation right after pause() and making the committer a no-op unless !disposed && generation === committedGeneration (scrollbackBackfill.ts consumeSnapshotFrame). This also makes snapshot-then-snapshot robust regardless of write-callback order (only the newest seeds). New test: "a snapshot committer a LATER invalidation superseded is a no-op".
  • F3 fixed — Confirmed no production .seed( caller (grep of packages/client/src); seed() existed only for test convenience, re-exposing the seed-without-reset transition consumeSnapshotFrame claims to make unrepresentable. Removed seed from the BackfillController interface and the returned object; the only seed path is now the guarded committer. Tests seed through a seedController() helper that immediately invokes the committer from consumeSnapshotFrame (the legitimate fused reset+seed).
  • F4 fixednormalLinesOf validated only onTrim; the RIS re-anchor does mirrorBaseLine += lines.length, so a shape change dropping length would poison the absolute cursor with NaN. Now throws unless Number.isInteger(lines.length) && lines.length >= 0 too (ptyHost.ts normalLinesOf). Pinned in xtermMirrorContract.test.ts: asserts lines.length is an integer and that onTrim(...) returns a {dispose} disposable.
  • F5 fixed — Every RIS pushed the replacement trimDisposable onto entry.disposables while the old (disposed) one stayed, growing the array one dead entry per reset. Now spawn pushes ONE stable teardown { dispose: () => entry.trimDisposable.dispose() } and the RIS re-anchor replaces the handle in place without pushing (ptyHost.ts) — disposables is bounded.
  • F6 fixed — The prepend-fault catch surfaced via onError but left the cursor live, so every subsequent onScroll re-fetched/re-replayed/re-toasted the same permanent overflow/broken-internals failure. Added pause() before opts.onError(err) in the prepend catch (scrollbackBackfill.ts); backfill halts until a fresh snapshot re-seeds. (The fetch catch stays retryable by design — a transient transport fault should retry; only the permanent prepend fault pauses.)
  • F7 fixed — Extracted the blank-page materialization into a pure materializeHistoryPage(chunk, before, topLine) (packages/kaval-tui/src/historyPage.ts), wired cmdHistory to it, and added historyPage.test.ts covering content, an all-blank spanning page (exact blank-row count), an empty zero-span page, the self-seeded first page (skipped), and the content→blank-span→content oldest-first assembly. (main.ts runs main() at import, so a testable helper had to be its own module.)
  • F8 fixed — Updated the three superseded MDX sections: step 3 (mid-backfill resize now pauses halt-not-corrupt, not "re-replay at new width"), step 1 (prependScrollback(term, rawChunk, servedRows): PrependResult, discriminated), step 2 + risk-4 (the unopened @xterm/xterm defaultScratch is now the shipped default pinned by a contract test, not an unverified fallback). Regenerated docs/atlas/dist/scrollback-backfill.html (just atlas::build, idempotent) and staged it.
  • F9 fixed — Deleted the unused onNormalBufferTrim thin wrapper and merged the orphaned duplicate JSDoc block into the NormalLinesRef doc (ptyHost.ts).
  • F10 fixed — Corrected the stale comments: scrollbackBackfill.ts "returns 0" → "returns {kind:"skipped"}" and the two "epoch" references to the local counter → "generation"; Terminal.tsx now says flush fires the buffered callbacks (so the re-seed committer survives the lock); ptyHost.ts mirrorBaseLine doc notes the RIS-reset advance (not eviction-only) and reflowEpoch doc says "bumped on a WIDTH resize and a full RIS reset" (not "every resize()").

commit: 7a847aec774f1857171b6decfa66ecf305e46132

Round 2

codex — approved: false

Nine prior findings are resolved, but the branch is not yet approvable. The new RIS invalidation introduces a major integration regression: overflow snapshots already contain RIS, so they invalidate their own guarded seed commit and leave backfill permanently paused. F10 also retains one stale "local epoch" comment.

Findings:

  • F1 · major · resolved — A PTY-originated RIS could reset the live terminal after an old history response passed the server gate but before it was spliced, allowing pre-reset history into the post-reset buffer. (packages/client/src/terminal/scrollbackBackfill.ts:564)
  • F2 · major · resolved — The deferred snapshot committer could seed after a resize, reset, newer snapshot, or disposal had invalidated that snapshot. (packages/client/src/terminal/scrollbackBackfill.ts:585)
  • F3 · minor · resolved — The public seed transition allowed callers to seed without first resetting, undermining the fused snapshot reset-and-seed invariant. (packages/client/src/terminal/scrollbackBackfill.ts:357)
  • F4 · minor · resolved — normalLinesOf used the private lines.length value without validating or contract-pinning it, so an upstream shape change could poison mirrorBaseLine with NaN. (packages/kaval/src/ptyHost.ts:101)
  • F5 · minor · resolved — Every RIS replacement appended another already-disposed trim subscription to entry.disposables, causing unbounded lifecycle bookkeeping growth. (packages/kaval/src/ptyHost.ts:749)
  • F6 · minor · resolved — After a permanent prepend invariant failure, the cursor remained active and every later scroll could repeat the same fetch, replay, and error notification. (packages/client/src/terminal/scrollbackBackfill.ts:512)
  • F7 · minor · resolved — The kaval-tui all-blank history-page materialization branch lacked focused coverage, including exact blank-row preservation and page ordering. (packages/kaval-tui/src/historyPage.test.ts:1)
  • F8 · minor · resolved — The Atlas note described superseded resize behavior, the old prepend signature, and defaultScratch as an unverified fallback. (docs/atlas/src/content/atlas/scrollback-backfill.mdx:172)
  • F9 · nit · resolved — An unused onNormalBufferTrim wrapper and duplicate orphaned documentation added needless indirection and confusion. (packages/kaval/src/ptyHost.ts:76)
  • F10 · nit · open — Most stale comments were corrected, but this comment still says pause bumps the "local epoch." pause actually bumps the local generation; "epoch" here is especially confusing because seedEpoch/reflowEpoch is a separate server value. (packages/client/src/terminal/scrollbackBackfill.ts:494)
  • F11 · major · open — The new RIS hook invalidates an overflow snapshot's own committer. Overflow recovery emits snapshot data as TERMINAL_RESET + next.snapshot at packages/padi/src/terminalEndpoint/reattachingDeltas.ts:84. consumeSnapshotFrame first pauses and captures generation G; parsing that frame's RIS then calls pause again, advancing to G+1; the write callback consequently rejects the snapshot commit at lines 600–602. The cursor remains null, disabling scrollback backfill after every overflow-driven reattach. The new tests exercise snapshot commit and RIS independently, so they miss this production ordering. (packages/client/src/terminal/scrollbackBackfill.ts:564)

codex on the rebuttal: F1: Accepted for the original race—the newest-first ESC handler pauses synchronously and returns false, allowing xterm's prior reset handler to run; disposal is covered. F2: Accepted—the captured generation correctly blocks commits superseded by resize, reset, newer snapshot, RIS, or disposal. F3: Accepted—the bare seed transition is gone. F4: Accepted—length and onTrim are now validated and pinned. F5: Accepted—the stable current-handle teardown bounds the disposable list. F6: Accepted—prepend faults pause before being surfaced. F7: Accepted—the extracted helper and focused cases cover the previously untested branch. F8: Accepted—the MDX and generated HTML now match the implementation. F9: Accepted—the dead wrapper and duplicate documentation are removed. F10: Partially accepted; the cited comments were mostly corrected, but line 494 still uses "local epoch," so F10 remains open. Separately, F11 is a new regression introduced by composing the F1 RIS hook with the F2 guarded committer: the RIS-prefixed overflow snapshot invalidates itself.

claude — Fixed the F11 self-invalidation regression (a snapshot frame's own leading RIS no longer revokes its committer) and the F10 stale "local epoch" comment; check gate green, targeted suites pass.

  • F11 fixed — Confirmed real: reattachingDeltas.ts:88 emits an overflow re-attach as TERMINAL_RESET + snapshot, while the INITIAL attach (servePadi.ts:300) carries no RIS. So on every overflow re-attach, consumeSnapshotFrame paused (generation G) and captured committedGeneration=G, then xterm parsed the frame's OWN leading RIS into the onRisReset handler, which paused again (G+1), so the write-callback committer saw generation !== committedGeneration and never seeded — cursor stayed null, backfill dead after every re-attach. Fix (client-only, no wire change): the controller now owes the esc handler one absorb per snapshot frame that carries a leading reset. Added let expectedSelfResets = 0 (scrollbackBackfill.ts); the esc handler decrements-without-pausing when expectedSelfResets > 0 (the frame's own reset, already invalidated at receipt) and only pauses for a FOREIGN live-delta RIS (F1 preserved). consumeSnapshotFrame(topLine, reflowEpoch, carriesReset) increments the counter iff carriesReset. Terminal.tsx passes frame.data.startsWith(TERMINAL_RESET) — the re-attach's \x1bc is always at the head of a single atomic frame, never split. Relocated TERMINAL_RESET to the client-reachable type barrel @kolu/padi/endpoint (reattachingDeltas re-exports it, so its own importers/test are unchanged) so both sides read one source of truth without dragging server-only code (@orpc/server) into the client bundle. A counter (not a boolean) so back-to-back re-attaches coalesced under scroll lock each get their absorb and the latest committer still seeds. The only imprecise case — a FOREIGN delta-RIS buffered before a snapshot under scroll lock consuming the owed absorb, so the snapshot's own RIS then pauses — degrades to safe-halt (paused until next snapshot), never corruption; that matches the design's halt-not-corrupt philosophy, and it does NOT affect codex's stated concern (a LATER live-delta RIS after the snapshot), which the counter handles precisely. Added the composition test codex asked for: consume(carriesReset=true) → fireRis → commit → scroll asserts the fetch uses (30, …, 2), then a second fireRis proves the absorb is one-shot and a later foreign RIS still invalidates. Also updated the F2 committer comment to say a FOREIGN in-band RIS supersedes but the frame's OWN leading RIS is absorbed. Files: scrollbackBackfill.ts, Terminal.tsx, scrollbackBackfill.test.ts, endpoint.ts, reattachingDeltas.ts.

  • F10 fixedscrollbackBackfill.ts:494 "bump the local epoch" → "bump the local generation" (the stale-halt path). "epoch" was the confusing term codex flagged (seedEpoch/reflowEpoch is the separate server value). Grepped the file for other stray "local epoch" — none remain (line 570 already said "local generation").

commit: efa331dcc26ea00717dc218920d79370396cab4f

Round 3

codex — approved: false

F10 is resolved and F11's nominal self-RIS-first path now works, but F11 remains open. The receipt-time counter does not preserve reset provenance when scroll-lock concatenates buffered frames: an older foreign RIS can consume the credit intended for a later snapshot, after which the snapshot's own RIS revokes its committer and backfill remains paused.

Findings:

  • F1 · major · resolved — A PTY-originated RIS could reset the live terminal after an old history response passed the server gate but before it was spliced, allowing pre-reset history into the post-reset buffer. (packages/client/src/terminal/scrollbackBackfill.ts:591)
  • F2 · major · resolved — The deferred snapshot committer could seed after a resize, reset, newer snapshot, or disposal had invalidated that snapshot. (packages/client/src/terminal/scrollbackBackfill.ts:629)
  • F3 · minor · resolved — The public seed transition allowed callers to seed without first resetting, undermining the fused snapshot reset-and-seed invariant. (packages/client/src/terminal/scrollbackBackfill.ts:357)
  • F4 · minor · resolved — normalLinesOf used the private lines.length value without validating or contract-pinning it, so an upstream shape change could poison mirrorBaseLine with NaN. (packages/kaval/src/ptyHost.ts:101)
  • F5 · minor · resolved — Every RIS replacement appended another already-disposed trim subscription to entry.disposables, causing unbounded lifecycle bookkeeping growth. (packages/kaval/src/ptyHost.ts:749)
  • F6 · minor · resolved — After a permanent prepend invariant failure, the cursor remained active and every later scroll could repeat the same fetch, replay, and error notification. (packages/client/src/terminal/scrollbackBackfill.ts:534)
  • F7 · minor · resolved — The kaval-tui all-blank history-page materialization branch lacked focused coverage, including exact blank-row preservation and page ordering. (packages/kaval-tui/src/historyPage.test.ts:1)
  • F8 · minor · resolved — The Atlas note described superseded resize behavior, the old prepend signature, and defaultScratch as an unverified fallback. (docs/atlas/src/content/atlas/scrollback-backfill.mdx:172)
  • F9 · nit · resolved — An unused onNormalBufferTrim wrapper and duplicate orphaned documentation added needless indirection and confusion. (packages/kaval/src/ptyHost.ts:76)
  • F10 · nit · resolved — The stale-halt comment called the local invalidation counter an epoch even though it is modeled as a generation distinct from seedEpoch/reflowEpoch. (packages/client/src/terminal/scrollbackBackfill.ts:516)
  • F11 · major · open — expectedSelfResets counts future RIS occurrences but does not bind each credit to its snapshot's position in the byte stream. While scroll-locked, a foreign RIS delta can be buffered before an overflow snapshot. The snapshot increments the counter at line 642, then scrollLock joins both chunks in order. The older foreign RIS consumes the credit, the snapshot's own RIS takes the pause branch at line 609, and the guarded committer no-ops. The new test covers self-RIS followed by foreign RIS, but not this foreign-before-self ordering. Backfill therefore still remains paused after a valid overflow snapshot in this case. (packages/client/src/terminal/scrollbackBackfill.ts:592)

codex on the rebuttal: F10: Accepted; line 516 now correctly says "local generation." F11: The nominal fix, shared constant, and self-RIS-then-foreign-RIS test are valid, but I do not accept the admitted foreign-before-snapshot limitation. Under scroll lock, packages/client/src/scrollLock.ts:222 concatenates buffered chunks before parsing, so the counter tracks quantity rather than provenance. An older foreign RIS deterministically steals the later snapshot's credit, and that snapshot then invalidates itself. Calling this safe-halt does not resolve the finding: the fresh snapshot is later in byte order, supplies a valid topLine/reflowEpoch, and is precisely the event that should resume backfill; another snapshot may never arrive. The halt-not-corrupt policy applies when no valid cursor exists, whereas here the valid replacement cursor is discarded solely because reset ownership was lost. The previous requested direction explicitly required a frame/write-boundary signal precise under scroll-lock coalescing, so F11 remains open.

claude — Conceded and fixed F11 for real: replaced the receipt-time credit counter with a byte-position snapshot-seed seam, so a foreign RIS buffered ahead of an overflow snapshot under scroll lock can no longer steal the seed; check gate green, 28/28 backfill tests pass.

  • F11 fixed — Codex is right, and I concede the safe-halt defense. Under scroll lock scrollLock.ts:219 JOINS every buffered chunk into ONE term.write, so the bytes parse in byte order [foreign RIS][own RIS][snapshot] with a SINGLE global esc handler firing for each RIS. The old expectedSelfResets counter tracked quantity, not provenance: the byte-earlier foreign RIS consumed the credit meant for the snapshot's own RIS, the snapshot's RIS then paused and bumped generation past the committer's receipt-captured baseline, and the guarded committer no-op'd — backfill left dead after a valid re-attach whose fresh topLine/reflowEpoch was precisely what should have resumed it. That is corruption-of-liveness, not a halt-with-no-valid-cursor, so halt-not-corrupt did not cover it.

    Fix (client-only, no wire change) — exactly codex's requested direction ("bind reset provenance to the exact snapshot write/parser boundary … mark that boundary so only the snapshot's own leading RIS is absorbed"): I mark the boundary with a zero-width no-op OSC seam, SNAPSHOT_SEED_SEAM (scrollbackBackfill.ts), that Terminal.tsx:888 prepends to EVERY consumed snapshot frame's bytes. It parses in the live xterm immediately before the frame's own content (and its leading RIS), so the controller's new OSC handler (onSeedSeam) captures that frame's committer baseline THERE — after every byte-earlier foreign RIS has already pause()d (so they're excluded), predicting the one extra bump the frame's own leading RIS makes (generation + (carriesReset ? 1 : 0)). The committer seeds iff generation === baseline when it runs: unchanged when no reset landed AFTER the snapshot's own bytes (F11 resumes), but bumped — seed suppressed — by any resize / foreign RIS / newer snapshot / reset / dispose that came after (F2 preserved). The RIS esc handler is now the pure "always pause" F1 handler again — the absorb-counter is gone entirely, simpler than before.

    A FIFO (pendingSeeds) routes the Nth seam to the Nth snapshot's committer (receipt order == byte order), so two snapshots buffered under one flush each get their own baseline. Verified the byte-position math resolves all four orderings; added the two tests codex asked for plus updated the three existing lifecycle tests to the seam ordering:

    • "a foreign RIS buffered AHEAD of a re-attach snapshot does NOT steal its seed" — [foreign RIS][seam][own RIS] → snapshot still seeds (the exact F11 case).
    • "an interleaved foreign RIS between two buffered snapshots — the NEWEST seeds" — [seam1][RIS1][snap1][foreign RIS][seam2][RIS2][snap2] → snap1's committer no-ops (foreign RIS after it), snap2 seeds.
    • Contract-pin: writing SNAPSHOT_SEED_SEAM through a real @xterm/xterm fires its OSC handler exactly once and emits nothing (zero-width) — an xterm OSC-parsing change turns RED here, not into a corrupted snapshot first row.

    Files: scrollbackBackfill.ts (seam constant + OSC handler + FIFO + simplified RIS handler + committer baseline), Terminal.tsx (prepend the seam, gated on commitSeed so seam↔FIFO stay 1:1), scrollbackBackfill.test.ts (fakeTerm registerOscHandler/fireSeedSeam, seam-ordered lifecycle tests, two new byte-order tests, seam no-op pin). Verified: just check (typecheck + biome) green; 28/28 scrollbackBackfill.test.ts pass; just fmt clean. The Atlas note (F8) does not describe the replaced mechanism, so it needs no update.

commit: 74c25674e0b7c2759480f928542cb523041c68d2

Round 4

codex — approved: false

The byte-position seam resolves F11's foreign-RIS-before-snapshot ordering, so F11 is closed. However, this round reopens F2 because lifecycle invalidations before seam parsing are forgotten, and introduces two regressions: PTY output can forge OSC 60697 and corrupt/throw through the pending-seed FIFO, and Terminal.tsx still documents the removed "absorbed RIS" behavior.

Findings:

  • F1 · major · resolved — A PTY-originated RIS could reset the live terminal after an old history response passed the server gate but before it was spliced, allowing pre-reset history into the post-reset buffer. (packages/client/src/terminal/scrollbackBackfill.ts:619)
  • F2 · major · open — The new seam-only baseline reintroduces the original receipt-to-parse race. consumeSnapshotFrame pauses at receipt, but baseline remains null until the asynchronously parsed seam. A resize, explicit reset, or newer snapshot occurring between those points increments generation; the seam then captures that already-incremented generation, forgetting the invalidation and allowing the stale committer to seed. The F2 test was narrowed to fire the seam before resize and no longer covers this ordering. (packages/client/src/terminal/scrollbackBackfill.ts:667)
  • F3 · minor · resolved — The public seed transition allowed callers to seed without first resetting, undermining the fused snapshot reset-and-seed invariant. (packages/client/src/terminal/scrollbackBackfill.ts:378)
  • F4 · minor · resolved — normalLinesOf used the private lines.length value without validating or contract-pinning it, so an upstream shape change could poison mirrorBaseLine with NaN. (packages/kaval/src/ptyHost.ts:101)
  • F5 · minor · resolved — Every RIS replacement appended another already-disposed trim subscription to entry.disposables, causing unbounded lifecycle bookkeeping growth. (packages/kaval/src/ptyHost.ts:749)
  • F6 · minor · resolved — After a permanent prepend invariant failure, the cursor remained active and every later scroll could repeat the same fetch, replay, and error notification. (packages/client/src/terminal/scrollbackBackfill.ts:556)
  • F7 · minor · resolved — The kaval-tui all-blank history-page materialization branch lacked focused coverage, including exact blank-row preservation and page ordering. (packages/kaval-tui/src/historyPage.test.ts:1)
  • F8 · minor · resolved — The Atlas note described superseded resize behavior, the old prepend signature, and defaultScratch as an unverified fallback. (docs/atlas/src/content/atlas/scrollback-backfill.mdx:172)
  • F9 · nit · resolved — An unused onNormalBufferTrim wrapper and duplicate orphaned documentation added needless indirection and confusion. (packages/kaval/src/ptyHost.ts:76)
  • F10 · nit · resolved — The stale-halt comment called the local invalidation counter an epoch even though it is modeled as a generation distinct from seedEpoch/reflowEpoch. (packages/client/src/terminal/scrollbackBackfill.ts:516)
  • F11 · major · resolved — A receipt-time reset credit could be consumed by a byte-earlier foreign RIS when scroll-lock concatenated buffered chunks, causing the later snapshot's own RIS to revoke its committer. (packages/client/src/terminal/scrollbackBackfill.ts:629)
  • F12 · major · open — OSC 60697 is not an out-of-band channel; it is ordinary PTY-controlled output. Any terminal program can emit ESC ] 60697 BEL. The handler ignores payload provenance and unconditionally shifts pendingSeeds, so matching program output can steal a later snapshot's seed. With an empty FIFO it throws from xterm's parser, interrupting terminal parsing. Choosing a high currently-unused identifier does not make collision impossible. (packages/client/src/terminal/scrollbackBackfill.ts:638)
  • F13 · nit · open — The call-site comment still says the snapshot's leading RIS is "absorbed by its esc handler." The new handler always calls pause; the seam instead predicts that generation bump. The comment now describes the removed counter implementation. (packages/client/src/terminal/Terminal.tsx:873)

codex on the rebuttal: F11: Conceded and accepted. The sequence [foreign RIS][seam][own RIS][snapshot] now captures the baseline after the foreign reset and predicts the snapshot reset, so the original reset-credit theft is fixed. The safe-halt defense was also correctly withdrawn. However, the claim that F2 remains preserved is incorrect: moving the entire baseline to seam parsing causes resize/reset/newer-snapshot invalidations between receipt and seam to be incorporated into the baseline instead of superseding the snapshot; the revised test no longer exercises that original race, so F2 is reopened. Separately, F12 is a new regression because OSC identifiers are part of arbitrary PTY output and 60697 has no trustworthy provenance, and F13 records the newly stale call-site comment.

claude — Conceded and fixed both open majors (F2 receipt-to-parse race and F12 OSC forgery/throw) and the F13 stale comment; check gate green, 31/31 backfill tests pass.

  • F2 fixed — Codex is right and I concede. Moving the ENTIRE committer baseline to the seam's byte position (the round-3 F11 fix) also moved OUT-OF-BAND lifecycle invalidations that land in the receipt→seam window: the seam captures generation AFTER a resize/reset/newer-snapshot has already bumped it, so generation === baseline wrongly passes and the stale committer seeds. F11 needs the baseline captured LATE (to exclude a byte-earlier foreign RIS); F2 needs it captured EARLY (to catch an out-of-band bump) — the two pull opposite ways on one counter, exactly codex's dual-token direction. Fix (scrollbackBackfill.ts): added a second counter lifecycleToken bumped ONLY by out-of-band events — width onResize, explicit reset(), a newer consumeSnapshotFrame — via a new pauseLifecycle() (pause() + lifecycleToken++); the RIS esc handler and the internal stale/prepend-fault pauses keep plain pause() (their provenance is byte order, resolved by the seam, or they must not kill a pending recovery snapshot). consumeSnapshotFrame captures myLifecycle = lifecycleToken AFTER its own bump (so it can't invalidate itself), and the committer now no-ops unless BOTH generation === baseline (byte-position, F11/post-seam) AND lifecycleToken === myLifecycle (receipt-time, catches the receipt→seam-window resize/reset/newer-snapshot the seam baseline forgives). Traced all four orderings green. New regression test "a resize BETWEEN a snapshot's receipt and its seam still suppresses the seed (F2 receipt-to-parse)" — fires resize BEFORE the seam parses (the ordering the narrowed test dropped); it fails under the seam-only code and passes now. The existing post-seam F2 test and all F11 byte-order tests still pass unchanged.

  • F12 fixed — Codex is right on both legs: OSC 60697 is ordinary PTY output (any program can emit ESC ] 60697 …), so the handler's unconditional pendingSeeds.shift() let matching program output steal a later snapshot's seed, and its throw on an empty FIFO would blow out of xterm's OSC parser and interrupt the terminal. I evaluated codex's first-choice out-of-band scrollLock barrier and rejected it: scrollLock.ts:222 JOINS every buffered chunk into ONE term.write, so a per-segment barrier fires before the byte-earlier foreign RIS in the same joined write parses — it can't hold the byte-position ordering the seam exists for (un-joining scrollLock is a far larger, riskier change to a load-bearing viewport primitive). Took codex's second option — a validated unpredictable per-frame token. The controller mints crypto.randomUUID() per frame; consumeSnapshotFrame now returns { commit, seam } where seam = seamFor(token) = `\x1b]60697;<token>\x07`, and Terminal.tsx prepends commitSeed.seam (the controller owns the bytes) and calls commitSeed.commit(). Each pending-seed entry stores its token; the OSC handler captures a baseline ONLY when payload === pendingSeeds[0].token — any other payload (foreign output, or an empty FIFO) is consumed silently (return true, emits nothing — it's our private ident) and the FIFO is left INTACT, never thrown. The token is generated client-side and never sent to the PTY, so program output can't forge it. Removed the exported SNAPSHOT_SEED_SEAM literal (replaced by SNAPSHOT_SEED_SEAM_OSC + internal seamFor); fixed the ident's doc comment, which had literally asserted the collision-impossibility claim codex disputed. New tests: "a foreign OSC on the seam ident (unmatched token) neither steals a pending seed nor throws" and "…with an EMPTY FIFO is ignored, not thrown"; updated the contract-pin to write a token-carrying seam through a real @xterm/xterm and assert the handler receives the exact token payload and the buffer stays untouched (pins that xterm round-trips the ;<token> payload the fix depends on).

  • F13 fixedTerminal.tsx:873 call-site comment said the frame's leading RIS is "absorbed by its esc handler" (the removed absorb-counter). The handler always calls pause(); the SEAM predicts that generation bump. Rewrote the comment to say the seam captures the committer's baseline one bump BEFORE the frame's own RIS and PREDICTS it, so the reset doesn't read as an invalidation. Also refreshed the two scrollbackBackfill.ts comments that still named the old SNAPSHOT_SEED_SEAM export (now seamFor(token)) and the byte-position/pending-seed narration to mention the token match.

Verification: just check (pnpm typecheck across all packages + biome lint --error-on-warnings) green; scrollbackBackfill.test.ts 31/31 pass (28 prior + 3 new); just fmt clean. Files: scrollbackBackfill.ts, Terminal.tsx, scrollbackBackfill.test.ts.

commit: e2b87d12d8b2fa4fc0feb3cb8450aa991701245a

Round 5

codex — approved: false

F2, F12, and F13 are correctly fixed: the dual counters cover receipt-time versus byte-order invalidation, token matching prevents foreign OSC output from stealing a seed or throwing, and the comments now describe RIS prediction accurately. However, the new token generator breaks supported plain-HTTP deployments, and the new lifecycle comments misstate disposal behavior. Approval remains blocked on F14 and F15.

Findings:

  • F1 · major · resolved — An in-band RIS could reset xterm while a pre-reset history fetch was still replaying, allowing stale history to splice onto the reset buffer. (packages/client/src/terminal/scrollbackBackfill.ts:670)
  • F2 · major · resolved — A stale snapshot committer could seed after a resize, reset, newer snapshot, or other lifecycle invalidation, including one between frame receipt and seam parsing. (packages/client/src/terminal/scrollbackBackfill.ts:728)
  • F3 · minor · resolved — The public bare seed transition allowed callers to seed without the required reset/snapshot lifecycle. (packages/client/src/terminal/scrollbackBackfill.ts:387)
  • F4 · minor · resolved — The private xterm line-list contract did not validate length, so RIS re-anchoring could poison the absolute cursor with NaN. (packages/kaval/src/ptyHost.ts:101)
  • F5 · minor · resolved — Each RIS accumulated another disposed trim subscription in the teardown array. (packages/kaval/src/ptyHost.ts:749)
  • F6 · minor · resolved — A permanent prepend failure left the cursor active, causing repeated fetch, replay, and error loops. (packages/client/src/terminal/scrollbackBackfill.ts:604)
  • F7 · minor · resolved — Blank history-page materialization lacked focused coverage for its cursor-span behavior. (packages/kaval-tui/src/historyPage.ts:19)
  • F8 · minor · resolved — The Atlas note described superseded resize, prepend, and scratch-terminal behavior. (docs/atlas/src/content/atlas/scrollback-backfill.mdx:252)
  • F9 · nit · resolved — An unused trim wrapper and orphaned duplicate documentation added dead surface area. (packages/kaval/src/ptyHost.ts:79)
  • F10 · nit · resolved — Several comments used stale return-shape, epoch, flush, and reset terminology. (packages/client/src/terminal/scrollbackBackfill.ts:588)
  • F11 · major · resolved — The initial RIS invalidation fix caused an overflow snapshot's own leading RIS—or a foreign RIS consuming its reset credit—to revoke the valid snapshot committer. (packages/client/src/terminal/scrollbackBackfill.ts:675)
  • F12 · major · resolved — The original static OSC seam was forgeable by PTY output and threw from the parser when no seed was pending. (packages/client/src/terminal/scrollbackBackfill.ts:686)
  • F13 · nit · resolved — The call-site comment incorrectly said the snapshot's RIS was absorbed after that mechanism had been removed. (packages/client/src/terminal/Terminal.tsx:870)
  • F14 · major · open — The new unconditional crypto.randomUUID() call is available only in secure contexts, while Kolu explicitly supports plain-HTTP LAN use. On those origins snapshot consumption throws before Terminal.tsx writes the frame, so terminals repeatedly fail to attach. The API restriction is documented by MDN (developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID), while getRandomValues remains available in insecure contexts. (packages/client/src/terminal/scrollbackBackfill.ts:746)
  • F15 · nit · open — The newly added comments at lines 485-487, 528-530, and 729-732 say disposal bumps lifecycleToken or uses pauseLifecycle(). dispose() instead sets disposed, increments generation, and clears the FIFO. Runtime behavior is safe because commit checks disposed, but the documented dual-token invariant is inaccurate. (packages/client/src/terminal/scrollbackBackfill.ts:485)

codex on the rebuttal: F2: accepted—the receipt-captured lifecycleToken now catches out-of-band invalidations before the seam, while generation preserves the byte-order behavior required by F11. F12: accepted—the random per-frame token and FIFO-head match resolve both the forgery and parser-throw defects; F14 is a separate compatibility regression in the chosen token generator. F13: accepted—the revised comment correctly explains that the seam predicts, rather than absorbs, the snapshot's own RIS bump. The new lifecycle commentary introduces the separate F15 documentation inconsistency.

claude — Fixed both open findings: swapped the secure-context-only crypto.randomUUID() seam-token mint for a getRandomValues 128-bit hex token (with an insecure-context regression test), and corrected the three lifecycle comments that wrongly listed disposal under lifecycleToken/pauseLifecycle.

  • F14 fixed — Codex is right, and the codebase already proves the premise: both other browser-side randomUUID uses (comments/useComments.ts:116, surface-app/src/index.ts:282) guard it behind a presence check with a fallback, because kolu is reached over plain HTTP on a LAN where crypto.randomUUID (secure-context-only) is absent and throws. On such an origin consumeSnapshotFrame threw before Terminal.tsx wrote the frame, wedging every attach. Replaced crypto.randomUUID() at scrollbackBackfill.ts:746 with a new mintSeedToken() helper (scrollbackBackfill.ts:337-351) that draws 128 bits from crypto.getRandomValues() — available in insecure contexts — and hex-encodes them (BEL-safe). Chose getRandomValues over Math.random() per codex's steer: the token must stay cryptographically unpredictable to remain unforgeable by PTY output on the same OSC ident (F12), which Math.random would not guarantee. Added a regression test (scrollbackBackfill.test.ts, "mints seam tokens without crypto.randomUUID — works in an insecure context (F14)") that deletes crypto.randomUUID from the global, then drives a full seed: no throw, seam carries a 32-char hex token, back-to-back frames get distinct tokens, and the live frame seeds. Gate green (just check exit 0), all 32 file tests pass.

  • F15 fixed — Codex is right that the comments over-claimed. dispose() (scrollbackBackfill.ts:776) sets the permanent one-way disposed flag, bumps generation, and clears the FIFO — it never calls pauseLifecycle(), so it does NOT bump lifecycleToken. The commit guard checks disposed FIRST (scrollbackBackfill.ts:766), so disposal is genuinely caught by a separate, stronger mechanism, not the token. Took codex's first option (correct the docs, keep the two guards distinct) rather than routing dispose through pauseLifecycle() — a redundant token bump would blur disposal (terminal, one-way) with the resize/reset/newer-snapshot events (recoverable). Fixed all three sites: the lifecycleToken field comment (removed "dispose" from the bump list, added the disposed-flag note), the pauseLifecycle() comment (same), and the consumeSnapshotFrame receipt-capture comment (dropped "dispose" from the "LATER out-of-band event" list, noted disposal no-ops via the disposed guard it checks first).

commit: 4c42e4516a97bcdd5e56a4d8e05e8e6206c5d8e1

Round 6

codex — approved: false

The production changes resolve F14 and F15: seam tokens now use 128 cryptographically random bits from getRandomValues, and the lifecycle comments accurately distinguish lifecycleToken from disposal. One new minor test regression remains: the insecure-context test does not actually remove the inherited randomUUID method and leaves the global crypto object with a changed property shape.

Findings:

  • F1 · major · resolved — An in-band RIS could reset xterm while a pre-reset history fetch was replaying, allowing stale history to splice onto the reset buffer. (packages/client/src/terminal/scrollbackBackfill.ts:689)
  • F2 · major · resolved — A stale snapshot committer could seed after a resize, reset, newer snapshot, or other lifecycle invalidation, including one between frame receipt and seam parsing. (packages/client/src/terminal/scrollbackBackfill.ts:747)
  • F3 · minor · resolved — The public bare seed transition allowed callers to seed without the required reset/snapshot lifecycle. (packages/client/src/terminal/scrollbackBackfill.ts:403)
  • F4 · minor · resolved — The private xterm line-list contract did not validate length, so RIS re-anchoring could poison the absolute cursor with NaN. (packages/kaval/src/ptyHost.ts:101)
  • F5 · minor · resolved — Each RIS accumulated another disposed trim subscription in the teardown array. (packages/kaval/src/ptyHost.ts:749)
  • F6 · minor · resolved — A permanent prepend failure left the cursor active, causing repeated fetch, replay, and error loops. (packages/client/src/terminal/scrollbackBackfill.ts:623)
  • F7 · minor · resolved — Blank history-page materialization lacked focused coverage for its cursor-span behavior. (packages/kaval-tui/src/historyPage.ts:19)
  • F8 · minor · resolved — The Atlas note described superseded resize, prepend, and scratch-terminal behavior. (docs/atlas/src/content/atlas/scrollback-backfill.mdx:252)
  • F9 · nit · resolved — An unused trim wrapper and orphaned duplicate documentation added dead surface area. (packages/kaval/src/ptyHost.ts:79)
  • F10 · nit · resolved — Several comments used stale return-shape, epoch, flush, and reset terminology. (packages/client/src/terminal/scrollbackBackfill.ts:607)
  • F11 · major · resolved — The initial RIS invalidation fix caused an overflow snapshot's own leading RIS—or a foreign RIS consuming its reset credit—to revoke the valid snapshot committer. (packages/client/src/terminal/scrollbackBackfill.ts:694)
  • F12 · major · resolved — The original static OSC seam was forgeable by PTY output and threw from the parser when no seed was pending. (packages/client/src/terminal/scrollbackBackfill.ts:705)
  • F13 · nit · resolved — The call-site comment incorrectly said the snapshot's RIS was absorbed after that mechanism had been removed. (packages/client/src/terminal/Terminal.tsx:870)
  • F14 · major · resolved — The seam-token generator used secure-context-only crypto.randomUUID, breaking terminal attachment on supported plain-HTTP LAN origins. (packages/client/src/terminal/scrollbackBackfill.ts:345)
  • F15 · nit · resolved — The lifecycle documentation incorrectly said disposal bumped lifecycleToken or used pauseLifecycle. (packages/client/src/terminal/scrollbackBackfill.ts:501)
  • F16 · minor · open — The new insecure-context regression test does not make randomUUID unavailable. randomUUID is inherited from Crypto.prototype, so deleting crypto.randomUUID leaves it callable; a production regression back to randomUUID would still pass. The finally assignment also creates an own property that did not exist previously, so it does not restore the original global shape. (packages/client/src/terminal/scrollbackBackfill.test.ts:984)

codex on the rebuttal: F14: accepted—the getRandomValues implementation fixes the supported insecure-origin failure while preserving the unpredictability required by F12. The production finding is resolved; F16 separately tracks the newly introduced ineffective regression test. F15: accepted—the revised comments now correctly state that disposal is enforced by the permanent disposed guard rather than lifecycleToken.

claude — Fixed the one remaining open finding (F16): the F14 insecure-context test now genuinely hides randomUUID and restores the exact original global shape; all other findings F1–F15 were already resolved in prior rounds.

  • F16 fixed — Codex is right, and I verified it directly. In happy-dom (the actual test environment per packages/client/vitest.config.ts:36), crypto is a Crypto instance and randomUUID lives on Crypto.prototype, NOT as an own property. A probe confirmed: ownRandomUUID=false, protoRandomUUIDon=Crypto, and after delete (crypto as any).randomUUID the property is STILL typeof function. So the old sim removed nothing — a production regression back to crypto.randomUUID() would have sailed through the test. The stale finally ((crypto as any).randomUUID = original) also minted an OWN property that never existed, leaking a changed global shape into sibling tests. Fixed in packages/client/src/terminal/scrollbackBackfill.test.ts:984: (1) shadow with an own undefined property via Object.defineProperty(crypto, "randomUUID", { value: undefined, configurable: true, writable: true }) so the whole lookup resolves to undefined; (2) added a guard expect(crypto.randomUUID).toBeUndefined() before exercising the controller, so the test fails loudly if the sim ever stops hiding the API; (3) capture originalDesc = Object.getOwnPropertyDescriptor(crypto, "randomUUID") up front and restore exactly — re-install the descriptor verbatim if it was own, else delete the shadow so the prototype lookup is left intact (no leaked own property). Full file passes (32/32) and just check (typecheck + biome) is green.

commit: dd58670429c32531ba6f428cac26961966188d92

Round 7

codex — approved: true

The F16 fix is correct. The test now genuinely shadows the inherited randomUUID method, asserts that the simulation is active, and restores the exact original property shape in a finally block. No regression was introduced, and all findings F1–F16 are resolved.

Findings:

  • F1 · major · resolved — An in-band RIS could reset xterm while a pre-reset history fetch was replaying, allowing stale history to splice onto the reset buffer. (packages/client/src/terminal/scrollbackBackfill.ts:689)
  • F2 · major · resolved — A stale snapshot committer could seed after a resize, reset, newer snapshot, or other lifecycle invalidation, including one between frame receipt and seam parsing. (packages/client/src/terminal/scrollbackBackfill.ts:747)
  • F3 · minor · resolved — The public bare seed transition allowed callers to seed without the required reset/snapshot lifecycle. (packages/client/src/terminal/scrollbackBackfill.ts:403)
  • F4 · minor · resolved — The private xterm line-list contract did not validate length, so RIS re-anchoring could poison the absolute cursor with NaN. (packages/kaval/src/ptyHost.ts:101)
  • F5 · minor · resolved — Each RIS accumulated another disposed trim subscription in the teardown array. (packages/kaval/src/ptyHost.ts:749)
  • F6 · minor · resolved — A permanent prepend failure left the cursor active, causing repeated fetch, replay, and error loops. (packages/client/src/terminal/scrollbackBackfill.ts:623)
  • F7 · minor · resolved — Blank history-page materialization lacked focused coverage for its cursor-span behavior. (packages/kaval-tui/src/historyPage.ts:19)
  • F8 · minor · resolved — The Atlas note described superseded resize, prepend, and scratch-terminal behavior. (docs/atlas/src/content/atlas/scrollback-backfill.mdx:252)
  • F9 · nit · resolved — An unused trim wrapper and orphaned duplicate documentation added dead surface area. (packages/kaval/src/ptyHost.ts:79)
  • F10 · nit · resolved — Several comments used stale return-shape, epoch, flush, and reset terminology. (packages/client/src/terminal/scrollbackBackfill.ts:607)
  • F11 · major · resolved — The initial RIS invalidation fix caused an overflow snapshot's own leading RIS—or a foreign RIS consuming its reset credit—to revoke the valid snapshot committer. (packages/client/src/terminal/scrollbackBackfill.ts:694)
  • F12 · major · resolved — The original static OSC seam was forgeable by PTY output and threw from the parser when no seed was pending. (packages/client/src/terminal/scrollbackBackfill.ts:705)
  • F13 · nit · resolved — The call-site comment incorrectly said the snapshot's RIS was absorbed after that mechanism had been removed. (packages/client/src/terminal/Terminal.tsx:870)
  • F14 · major · resolved — The seam-token generator used secure-context-only crypto.randomUUID, breaking terminal attachment on supported plain-HTTP LAN origins. (packages/client/src/terminal/scrollbackBackfill.ts:345)
  • F15 · nit · resolved — The lifecycle documentation incorrectly said disposal bumped lifecycleToken or used pauseLifecycle. (packages/client/src/terminal/scrollbackBackfill.ts:501)
  • F16 · minor · resolved — The insecure-context regression test originally deleted an inherited randomUUID method without hiding it and restored it as a new own property. (packages/client/src/terminal/scrollbackBackfill.test.ts:989)

codex on the rebuttal: F16: accepted. The own undefined property now blocks prototype lookup, the assertion proves randomUUID is unavailable before the controller runs, and the finally block either reinstalls the original own descriptor verbatim or deletes the temporary shadow to restore inherited lookup. This fully resolves the finding.

@srid

srid commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

Evidence — Family 3: a height-only resize no longer halts backfill (the positive artifact)

The reported bug was that a routine vertical pane drag (a height-only resize) permanently stalled scroll-up backfill for the rest of the session — kaval bumped the reflow generation on every resize, so the client's next fetch came back stale and the controller paused with no re-seed. The fix gates the generation bump to a real cols change. Here it is, live, against a terminal with 3000 lines of scrollback.

Measured off the live xterm buffer (element.__xterm), a fresh re-attach then scroll-up backfill, with a height-only resize dropped in the middle:

step buffer.length getLine(0) cols rows
bounded re-attach 1070 LINE 1934 … 140 70
backfill #1 1570 LINE 1434 … 140 70
backfill #2 2070 LINE 934 … 140 70
height-only resize 2070 LINE 934 … 140 70 → 44
backfill #3 (post-resize) 2570 LINE 434 … 140 44
backfill #4 (post-resize) 3007 terminal's first line 140 44

The resize changed rows 70 → 44 with cols steady at 140 — a true height-only resize published to kaval — and backfill continued straight through it, from 2070 to the full 3007, getLine(0) walking 934 → 434 → the session's first line. Before the fix, the resize would have staled → paused → halted at 2070. A cols change still (correctly) pauses backfill — that's a real reflow — but a height-only or same-dims resize now renumbers nothing and never halts.

This is pinned in unit tests too: ptyHostHistory.test.ts"a height-only or same-dims resize does NOT stale a stamped cursor" (server gate), and the client controller keeps its cursor on a non-cols resize.

Local artifacts (per-worktree, gitignored — viewable on this box):

  • .dev-server/resize-no-halt.webm — the live capture (bounded attach → backfill → height resize → backfill continues)
  • .dev-server/resize-no-halt-top.png — the resized terminal (shorter, rows 44) holding the full restored scrollback

(Captured on a local dev instance, random ports, isolated from production — production kolu PID verified unchanged before and after. The earlier bounded-attach + cross-host-over-ssh evidence is in the comments above.)

@srid
srid marked this pull request as ready for review July 13, 2026 18:48
@srid
srid merged commit 557e08a into master Jul 13, 2026
8 checks passed
@srid
srid deleted the scrollback-backfill branch July 13, 2026 18:52
srid added a commit that referenced this pull request Jul 13, 2026
…1791)

## What

Retire `lifecycle.restoreSleeping` — a dead client-facing verb — from
the padi surface, its `servePadi` handler, and every client-side
stub/type/mock. Bump `PADI_SURFACE_VERSION` **3.1 → 4.0** (a removed
procedure is a shape-break → MAJOR).

## ⚠️ Version number: ratified as 3.0, shipping as 4.0 (same rule)

srid ratified **MAJOR 3.0** when this branch was off a stale master at
surface **2.0**. Master has since advanced — **#1783** (scrollback
backfill) took `padiSurface` to **3.0** (its `terminalAttach` reshape)
then **3.1** (reflow guard). This branch was rebased onto current master
and the removal re-applied on top of 3.1, so the faithful application of
the *same ratified rule* is **3.1 → 4.0**. Nothing else changed. The
coordinator confirmed this handling before this PR opened.

## Why it's dead

`restoreSleeping` had **no production caller**. Its only writer — the
client respawn loop — was already deleted (W1.R6), and the real
cold-boot restore seeds a sleeping terminal directly via
`seedSleepingTerminal` in `sessionRestore.ts` / `reattach.ts`, never
over the wire. #1784's W12 review dispositioned it as removable.

## Why a MAJOR bump

A removed procedure is a shape-break in **both** skew directions — an
old binder that still called `restoreSleeping` would hit a missing proc
on a 4.0 padi. Only a major flips `isContractVersionCompatible` to
refuse the skew both ways (mirrors 3.0's `terminalAttach` reshape and
2.0's `fs.statFileMtimeMs` removal). The version is an honest statement
of the wire **shape**; encoding "no caller today" as a minor would bake
in exactly the soft assumption the fail-fast rule rejects.

## Deploy consequence (flagged + ratified pre-push)

A major skew is refused, so on the next deploy a newer binder **DRAINS**
any straddling 3.x padi — the graceful `save + exit` every code-change
deploy already pays via the build-digest mismatch — then respawns it at
4.0. **kaval and the PTYs are untouched**: a padi-surface bump does not
touch the kaval contract.

## Disposition history

- **#1784** (W12, "restore survives an unclean kaval death") — its
independent perfection review dispositioned `restoreSleeping` as a dead
verb with no production caller. This PR is that follow-up removal.

## Verification

- Repo-wide grep: nothing outside tests/docs references the verb;
`surface.test.ts` now **pins its absence** from the wire (dropped from
the lifecycle-verb list).
- `just fmt` clean · `just check` (tsc + biome) green · padi **309/309**
· touched client tests green.
- Version pins across `surface.test.ts` / `dial.test.ts` updated to 4.0
(skew-scenario re-pegged to a 5.0 binder vs a 4.x padi).

🤖 Generated with [Claude Code](https://claude.com/claude-code)
srid added a commit that referenced this pull request Jul 13, 2026
**Status: ratified (2026-07-13).** srid delegated ratification to the
coordinator under /perfection-review; the note now carries `status:
accepted` and reads as the current snapshot only — no revision history
in the note body (git and this PR hold the journey).

**This note is now the campaign plan of record for the surface-framework
consolidation.** The original audit (Codex, `gpt-5`) was written
deliberately blind to the existing Atlas plans — that was methodology,
an independent read of the three working trees. This revision is the
reconciliation it deferred: the coordinator's reviewed ruling on the
proposal, ratified by srid, merged in as one plan.

### What merged in

- **The ratified [reactive
bridge](https://kolu.dev/atlas/surface-reactive-bridge.html)** — phase 0
shipped in W5 (#1759); its phases 1–4 are now PRs 7–10 of this plan's
sequence (one numbering scheme; the bridge note carries a
superseded-pointer).
- **The [consolidation
ledger](https://kolu.dev/atlas/padi-cleanup.html)'s campaign 1** — L28
rung 2 → PR 3, L20·L21 → PR 5, L1 → PR 8, L8's hygiene residue as the
tail commit; L22 and L24 dispositioned (L24 already shipped as #1749).
- **A staleness sweep** — the audit's pins predate six merged PRs (W9
#1764, W10 #1772, W11 #1775, W12 #1784, #1783, #1791). Every code claim
was re-grounded against current kolu/drishti/odu tips or cut; e.g. the
"six polled-query state machines" claim died against W9's per-host
ownership shape, and odu already adopted `implementSurface` since the
audit.

### The dispositions

| proposed | ruling |
| --- | --- |
| supervised `SurfaceRuntime` (final router · `done` · idempotent
`close`) | **kernel — PR 1** |
| bound procedures as first-class client members | **kernel — PR 2** |
| opaque `membershipId` per map add (+ typed connection key, `clockNow`
at admit) | **kernel — PR 3** |
| total, schema-valid `failureOf` (no fabricated `"other"`) | **kernel —
PR 4** |
| mirror consumes collection `deltas` (deletes drishti's parallel
stream) | **kernel — PR 5** |
| `firstFrameOrThrow`-style adoptions | **kernel — PR 6** |
| `Feed` member kind | **dropped** — it is the bridge's `scan` wearing a
wire protocol; the append-heavy wire concern (terminal bytes, logs)
becomes a named bridge design question, not a fourth member kind |
| `@kolu/surface-suite` | **deferred** — a thin composition leaf earns
no receptacle; its two real moves (final router, typed key) are in the
kernel; revival condition: the surface-app dependency direction |
| Odu lease stack (`SurfaceLease` … `SurfacePort`) | **Odu-local** —
five concepts for one caller fails prove-then-extract; graduates at a
second consumer; `SurfacePort` dies with it |
| `createLiveQuery` | **obsolete** — W9's `hostCodeTab.ts` ownership
shape supersedes the string-keyed singleton; the createResource instinct
may refine inside it |
| `notificationSurfaceApp` / `bootSurfaceApp` | **needs grounding**
against W5's shipped `createNotify`/SW seam before any PR is cut |

The note is restructured as user-facing / architecture / implementation,
with the sequenced ten-PR list (kernel first, every framework PR
drishti-paired per `.claude/rules/surface.md`) and grep-able
done-criteria. Status stays `proposed` — srid merges and flips.

### Validation

- `just atlas::build` (foreground, exit 0)
- `just atlas::check-sync` (sync + idempotency both green)

🤖 Generated with [Claude Code](https://claude.com/claude-code)
srid added a commit that referenced this pull request Jul 13, 2026
…son (#1719) (#1792)

Fixes #1719 — the intermittent unhandled `AbortError` on stdio-link
teardown (the `padiBinding` "reconnects when padi dies" flake). The
availability follow-up W12's own disposition named; recurred on
**#1712**, **#1764**, **#1783** (`unit@x86_64-linux`, same
`handleTransportClosed` → `peer.close()` `AbortError`).

## Root cause

When the transport dies, `LinkStdioClient.handleTransportClosed()` calls
orpc's `peer.close()`. With **no reason**, orpc's `AsyncIdQueue.close()`
rejects every PENDING async-iterator pull with a **fresh, anonymous
`AbortError`** ("[AsyncIdQueue] Queue[N] … closed or aborted while
waiting for pulling."), delivered asynchronously. The one consumer that
floats it is `mirrorCollection`'s **per-key value pump**
(`mirrorRemoteSurface.ts`): a detached `void (async…)()` IIFE **never
joined into the mirror's settle graph**, whose `ctl` is aborted only in
the teardown `finally` — *after* `peer.close()` — and whose swallow
predicate (`isAbortReason(err, ctl.signal)`) the fresh `AbortError` can
never satisfy. `pumpRemoteSurface` builds the mirror with no signal, so
nothing aborts the pulls first. `mirror.done` resolves and the pump loop
advances while a pull is still parked → the pull rejects with no awaiter
→ float.

## The fix — two mechanism halves (owned/awaited/typed-cancelled, not
swallowed)

**1. `mirrorCollection` OWNS its per-key pumps.** Each pump is tracked;
the collection's `finally` aborts the ctls **first** (a still-parked
pull then rejects with the swallowed `signal.reason`), **then**
`allSettled`s the tracked pumps — so a pump's settle is always observed,
regardless of whether the peer or the finally closed first (per the
ruling's ordering note).

**2. The stdio link passes a TYPED close reason.** `peer.close({ reason:
deadTransportError(SURFACE_STDIO_TRANSPORT_CLOSED, …) })` — so the one
thing that can cross the stdio seam at close is that single owned,
greppable `ORPCError` (the same non-retriable shape `call()` already
throws on a dead link). Makes "an anonymous `AbortError` escapes the
seam" unconstructible (AFP P4).

## Half 2 — the padi backstop (honest divergence, backstop not fix)

padi gains a process-level `unhandledRejection` handler that is **loud**
(a greppable `padi-unhandled-rejection-boundary` marker + an optional
health sink) but **never fatal** — an unidentified future float becomes
a diagnosable log line, not a dead workspace daemon. Installed in
`bin.ts`'s durable-daemon branch only (not the `--stdio` front, whose
stdout is the wire; not `runPadiDaemon`, which tests boot in-process).

**The tension, stated honestly:** this **diverges** from kolu-server's
deliberately-fatal `unhandledRejection` policy. A never-fatal global
boundary *can* mask a genuinely-new bug. Mitigations: the log is loud +
greppable (grep enumerates every float the backstop survived — each a
missing source boundary to hunt), and **identified floats are still
fixed at source** (halves 1–2 above). The boundary is the net under the
fix, not a substitute.

## Evidence — both mechanism pins RED pre-fix (evidence transfers only
within its class)

**Pin (i)** — `packages/surface/src/links/stdio.test.ts`: a pull parked
at transport close rejects with the typed error, not an anonymous
`AbortError`.
- RED on pre-fix: `AssertionError: expected AbortError: [AsyncIdQueue]
Queue[1] was c… to be an instance of ORPCError`
- GREEN post-fix.

**Pin (ii)** — `packages/surface/src/mirrorPumpOwnership.test.ts`:
`mirror.done` does not resolve while a per-key pump is unsettled.
- RED on pre-fix: `expected false to be true` (`pumpSettled` — the
detached pump was abandoned).
- GREEN post-fix.

**Boundary pin** —
`packages/padi/src/unhandledRejectionBoundary.test.ts`: an injected
float logs loudly with the marker, reaches the health sink, and padi
survives (`process.exit` spied, never called). GREEN.

**Suites:** `@kolu/surface` 323/323 · `@kolu/surface-remote` 152/152 ·
`@kolu/padi` 314/314 · `padiBinding.test.ts` 27/27 (×3 local). `just
check` (tsc + biome) clean.

**Flake-class evidence (separate):** flaky-tracker row `reconnects when
padi dies` (#1712/#1764/#1783) flips to *fixed* on merge — see the
tracker update commit + the two-platform CI runs on the final SHA.

**drishti gate (shared surface):** GREEN. drishti's
`packages/app/src/server/router.ts` consumes
`pumpRemoteSurface`/`mirrorRemoteSurface`; my diff changes **zero**
exported signatures/types, and drishti's full `typecheck` passes with my
modified `mirrorRemoteSurface.ts` + `stdio.ts` overlaid onto its
hydrated surface (baseline also green). Per `.claude/rules/surface.md`,
a purely-internal behaviour change needs no drishti PR; drishti inherits
the same float-fix.

## Scope

Framework-touching (`@kolu/surface` links/stdio + mirror) but **not**
structural — no boundary moves, no API/type reshape. Direct PR, no `/be`
(coordinator-agreed). srid merges.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant