Skip to content

fix(terminal-core): cache-lifetime protocol state (backlog 003) + ED3 resize-wipe auto-repair - #27

Merged
rockyway merged 9 commits into
developfrom
feature/protocol-state-and-ed3-repair
Jul 25, 2026
Merged

fix(terminal-core): cache-lifetime protocol state (backlog 003) + ED3 resize-wipe auto-repair#27
rockyway merged 9 commits into
developfrom
feature/protocol-state-and-ed3-repair

Conversation

@rockyway

Copy link
Copy Markdown
Contributor

Summary

Two independent bugs, both found on the same live pane (pn-15kpdw3ls) in one debugging session, and both rooted in the same class of problem: state TermFlow observes from the PTY byte stream gets lost or corrupted specifically when a pane spends time as a cached-but-unmounted background tab/pane.

Fix 1 — backlog 003 (Shift+Enter/Ctrl+J permanently broken): ConPTY's one-shot CSI ?9001h (Win32-Input-Mode) and Kitty's keyboard-flag handshakes were only observed while a pane was mounted (CSI parser handlers registered in mount(), torn down in unmount()). A handshake arriving while backgrounded was missed forever. Moved the state-mutating handlers (Kitty >u/<u/=u/>m, Win32 9001 on ?h/?l, DECSTR) to cache-entry lifetime — registered once at Terminal creation, disposed only when the tab is actually closed — mirroring the existing dataDisposable/exitDisposable pattern.

Fix 2 — ED3 resize-wipe auto-repair: codex (ratatui) answers a resize with ESC[2J ESC[3J (erasing scrollback) then re-emits its own retained transcript, capped at ~1000 lines. The backend runs an independent vt100 parser per terminal whose scrollback survives 2J/3J for already-scrolled history (proven by a new regression test) — the data was never actually lost, only the live browser-side view was clipped. Added a new GET /terminals/:id/full-scrollback endpoint exposing that authoritative buffer, detect a CSI 3J arriving shortly after our own background-reactivation convergence resize, and silently repair the live view via term.reset()+write() — the same mechanism hydrate() already uses for reattach.

Full design doc + implementation plan in termflow-fabric:

  • docs/superpowers/specs/2026-07-24-protocol-state-and-resize-wipe-fixes-design.md
  • docs/superpowers/plans/2026-07-24-protocol-state-and-resize-wipe-fixes.md

Process

Followed brainstorm → design → plan → TDD implement → internal dual-agent review (spec-vs-implementation audit + architecture/lifecycle/security review, run in parallel). The architecture review caught 3 real issues in the ED3 repair path (independently corroborated in part by the spec audit), all fixed in a follow-up commit:

  • runEd3Repair could clobber actively-streaming live output with a stale fetch (missing the idle-settle re-check its sibling reconcileSnapshot already has) — fixed with a proper settle-gate + re-arm.
  • ed3RepairTimer wasn't cancelled in unmount(), so a scheduled repair from an abandoned engine instance could fire against a since-remounted pane — fixed to match fitTimer's teardown.
  • convergenceResizeAt was stamped unconditionally on every activation (narrow false-positive risk on an unrelated clear shortly after switching tabs) — narrowed to stamp only when a real backend resize is actually pending.

Test plan

  • packages/terminal-core full suite: 470/470 passing (was 461 baseline)
  • Root bun/jest suite: 633/633 passing
  • Rust: cargo build, cargo test (191/191), cargo clippy (no NEW warnings — pre-existing baseline findings in unrelated files confirmed unchanged)
  • tsc --noEmit: 0 errors
  • Production build (bun run build): clean
  • Manual smoke (real ConPTY timing, can't be substituted by unit tests): background a tab before codex finishes starting, switch back, confirm Shift+Enter now inserts a newline instead of submitting — flagged for the user to verify live, not run by this PR

🤖 Generated with Claude Code

…e race, leaked timer, false-positive stamp)

- runEd3Repair now re-checks output idle-settle before AND after the getFullScrollback
  fetch, deferring (re-arming on the same generation) instead of clobbering live output —
  mirrors the existing reconcileSnapshot settle-gate pattern.
- unmount() now cancels ed3RepairTimer, matching fitTimer's teardown — a scheduled repair
  from an abandoned engine instance can no longer fire against a since-remounted pane.
- convergenceResizeAt is now stamped only when flushDeferredResizeOnActivation determines
  a real backend resize is actually pending/being sent, not unconditionally on every
  activation — narrows the false-positive window on an unrelated clear/tput reset run
  shortly after switching to a tab whose size didn't change.
- Corrected a comment overstating what the entry-swap carry-forward guarantees.

Found by an internal architecture/lifecycle/security review agent; independently
corroborated by a parallel spec-vs-implementation audit agent flagging the same
idle-settle gap. 3 new regression tests added (9/9 in engine.ed3-repair.test.ts).
… in-flight fetch

unmount() only clears ed3RepairTimer if it's still armed at that moment; if
getFullScrollback is already in flight, the timer is null and unmount() has
nothing to cancel. If the fetch then resolves into the "output still
unsettled" branch, it re-arms a brand-new timer that unmount() — having
already run — can never see or cancel again, leaking a retry loop against an
abandoned engine instance. Added an explicit `!this.container` bail (nulled
by unmount()) at both checkpoints in runEd3Repair, matching the same
defensive pattern reconcileSnapshot already uses.

Found by agy (external PR review on #27). Verified the new regression test
actually catches the bug by temporarily reverting the guard and confirming
the test fails (getFullScrollback called twice) before restoring it.
…arget and fetch-race gaps

- full_scrollback_snapshot (state.rs) now appends cursor_state_formatted() +
  attributes_formatted() to the rendered blob: render_full_scrollback replays
  plain rows with no position tracking, so without this the repaired pane's
  cursor landed wherever the last line's newline happened to fall, not the
  program's actual cursor position.
- get_terminal_full_scrollback (api_server.rs) now appends input_modes_snapshot(),
  mirroring get_terminal_snapshot exactly: without it, a repair's reset()+write()
  silently dropped mouse tracking/bracketed paste/app cursor-keypad/focus
  reporting the still-running program already asserted (it won't re-send them
  mid-session).
- runEd3Repair now captures attachedProcessId and the exact lastDataAt value
  before starting the fetch, and re-validates both are UNCHANGED (not just
  "recent enough") after the await: attach() retargeting to a new process
  mid-fetch no longer lets a stale blob overwrite the new session's terminal,
  and output landing during a slow fetch no longer slips past a recency-only
  settle check purely because enough wall-clock time passed by response time.

Found by codex (external PR review on #27), independent from agy's review.
Verified all 4 new/changed checks are load-bearing: the new Rust cursor test
confirmed via cargo check (integration-tests feature — this class of
AppState-based test crashes the Windows test binary at runtime per this
repo's documented constraint, so it's compile-verified locally and executes
on Linux/macOS CI only, matching existing precedent for this test module);
the two new frontend regression tests confirmed by temporarily reverting each
guard and observing the expected failure before restoring.
@rockyway

Copy link
Copy Markdown
Contributor Author

External review round (agy + codex, via agent-research)

Both reviewed independently in parallel, focused on TerminalEngine.ts's mount()/unmount() changes, cache.ts's new fields, and the new api_server.rs endpoint.

agy — 1 finding: runEd3Repair's re-arm branch could schedule a new ed3RepairTimer after unmount() already ran (if getFullScrollback was in flight when the pane was backgrounded), leaking a retry loop against an abandoned engine instance. Fixed with a !this.container guard at both checkpoints, matching reconcileSnapshot's existing pattern.

codex — 3 findings:

  • High: the repair's reset()+write() never restored the live program's input modes (mouse tracking, bracketed paste, app cursor/keypad, focus reporting) or cursor position — the running TUI still believes those modes are active while xterm silently dropped them. Fixed by appending input_modes_snapshot() (mirroring the existing /snapshot endpoint) and the vt100 crate's own cursor_state_formatted()/attributes_formatted() to the new endpoint's response.
  • High: attach() retargeting to a new process mid-fetch wasn't guarded — a stale blob for the old process could commit over a brand-new session. Fixed by capturing attachedProcessId before the fetch and re-validating it's unchanged after.
  • Medium: the post-fetch settle check only tested recency, not whether output had changed at all during the fetch window — a slow fetch could let output that arrived mid-request look "settled" purely by elapsed time. Fixed by capturing the exact lastDataAt value before the fetch and requiring it to be unchanged after, in addition to the recency check.

All 4 findings verified as real before fixing (read the actual code/vt100 source, didn't blindly patch), and all fixes verified load-bearing by temporarily reverting each guard and confirming the corresponding new regression test fails before restoring. Full reviews saved to termflow-fabric/docs/review/065-review-pr27-agy.md and 066-review-pr27-codex.md.

Updated test counts: terminal-core 473/473, root suite 633/633, Rust cargo test 191/191 (+1 gated behind integration-tests, compile-verified locally, runs on Linux/macOS CI), tsc --noEmit clean.

🤖 Generated with Claude Code

…anel

Adds a "Build: <sha> on <branch> - uncommitted changes - built <time>" line
beneath the existing current-version line so a running install can be traced
back to the exact commit it was built from.

New services/buildInfo.ts reads the webpack DefinePlugin git constants through
typeof guards, so it degrades to null outside a bundle (jest/ts-node) instead
of throwing ReferenceError.
@rockyway
rockyway merged commit 5863caf into develop Jul 25, 2026
@rockyway
rockyway deleted the feature/protocol-state-and-ed3-repair branch August 19, 2026 20:17
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.

2 participants