feat: persist and restore scrollback as a marked record - #52
Merged
Conversation
…keys Adds the storage half of scrollback persistence (docs/specs/scrollback-persistence.md), with no behavior change by default: `scrollback-persist` defaults to `never`, matching Ghostty, which restores window topology but never terminal contents. noa-config gains four noa-only keys (`scrollback-persist`, `-limit`, `-total-limit`, `-max-age-days`) plus `scrollback_dir()`. Days and bytes are plain integers, following the crate's existing convention — there is no unit-suffix parser anywhere in noa's config, so `7d` diagnoses rather than silently parsing as 7. noa-grid gains `snapshot`, a self-contained NOASB byte format, and two `Terminal` methods to capture and restore a tail. The format works at the materialized Row/Cell level rather than serializing the paged scrollback, because two of the paged form's ids are process-scoped: `GraphemeId` indexes a global interner and `HyperlinkId` indexes `Terminal::hyperlinks`. Writing those to disk would decode to different text in the next process, so both are resolved to their content and rebuilt on load. Styles are re-interned per snapshot and the body is deflated. Capture reads the primary screen even while the alternate screen is active: restoring a dead frame of a full-screen app into a fresh shell would show a program that is no longer running, when what the pane wants is the shell history underneath. Restore rewraps rows to the new grid width so a snapshot taken in a narrow window does not come back with its soft-wraps frozen, and never separates a wide glyph from its spacer.
Closes the gap where restoring a session's layout implied its contents came back too. Two independent defects, fixed independently: Honesty (ships even with persistence off): a pane restored with no record now says so, in a row naming the key that would change it. Restoring the tabs and splits is itself a promise the empty pane breaks, and the previous behavior left the user to guess whether they had lost the output or merely not configured keeping it. Persistence (opt-in via `scrollback-persist = tail`): each pane's scrollback tail is captured on clean quit and on idle checkpoints, and restored on launch above the live shell, separated by a labeled boundary row and marked with a gutter so recovered history cannot be read as this session's output. Restored rows are ordinary scrollback, so selection, copy and search work on them with no extra code. Capture deliberately does not share `persist_session`'s trigger: that runs on every structural change, and re-encoding a megabyte per pane each time a split moves is not viable. Checkpoints fire 5s after output settles, with a 60s ceiling so a sustained flood — the long build whose tail matters most — still gets captured. A checkpoint also rewrites the topology document, because a key minted but not recorded there would be deleted by the next launch's collector: precisely the crash the checkpoint exists to survive. Storage posture, given this writes material that previously lived only in RAM (exported credentials, echoed tokens, URLs with embedded PATs): default off, `0700` directory, `0600` files, excluded from Time Machine, nothing created at all while disabled. Snapshot keys are validated as 16 hex digits — they come from a user-writable file and are interpolated into a path. Scratch terminals, the quick terminal, remote panes and the alternate screen are never captured. Four independent caps (per-pane, total, age, orphan collection) bound the directory. Restore is inert by construction: rows are inserted as history rather than replayed as VT input, so a snapshot cannot move the cursor, set a title, or write to the pty.
Found by running it: the separator inserted at restore was being captured back into the next snapshot, so every relaunch buried one more in the history. Three launches left two stale separators — one of them rewrapped into three rows by a width change — plus the previous session's prompt, sitting in the middle of what should read as one continuous record. Capture now skips the pane's synthetic annotation row (the separator, or the Stage 0 notice, whichever was inserted). The exclusion is a session-absolute row so eviction cannot make it point at an unrelated live row, and it is cleared once eviction passes it. Three generations now leave zero separators in the record and keep only the real prompts. Also widens the rule fallback: at 46 columns the separator lost both rules and rendered as a bare sentence, which reads as output rather than as a boundary. Adds `noa-grid --example dump-snapshot`, which decodes a `.nsb` and prints each row's text and pen. The format is opaque, so without it a wrong-looking restored pane gives no way to tell whether the capture, the file, or the restore is at fault — this whole bug was found with it.
Tri-engine review (codex + claude; agy failed to run) surfaced one
dominant defect class and several storage-posture gaps.
Stale absolute row indices. `record_rows`/`annotation_row` are
session-absolute, and both a column-count reflow and a scrollback clear
renumber that space wholesale — the gutter would paint over live rows,
capture's `skip_row` would delete a real row from the record (reviving
the separator-accumulation bug), and `Discard restored history` would
drop live history off the front. `Terminal` already bumps a coordinate
generation on exactly those operations and deliberately leaves it alone
for ordinary eviction, so comparing against it is the complete
staleness test. On mismatch the marking is dropped rather than acted
on; the boundary itself is a row, so it survives reflow and the record
stays legible. Measured on a real launch: the marking stays live across
every frame, so the guard costs nothing in the normal path.
The byte budget counted only row bodies. Style, link and grapheme
tables are written alongside them and are not bounded by cell count —
one OSC 8 URI can reach the parser's 12 MiB ceiling on its own and blow
a 1 MiB budget with a handful of rows. The budget is now checked
against the real encoded size, and a single link is capped at 4 KiB
(the cell's text is kept either way).
`emit_logical_line` did not terminate at width 1 with a wide glyph:
backing off to keep the lead with its spacer left the split at the row
start, consuming nothing. Confirmed by a test that had to be killed.
Storage posture. No more `PathBuf::from(".")` fallback — an unresolved
data directory now means persistence is unavailable, rather than
writing terminal output into the process's working directory and
chmodding a directory noa did not create (which also stops spawning the
worker thread when the feature is off). Capture is gated on
`window-save-state` too, since the key that makes a snapshot reachable
is written by `persist_session`, which is a no-op while session state
is disabled — writing unreachable records is cost with no benefit.
Turning the feature off, or setting the budget to zero, now purges what
is already on disk instead of waiting for the next launch's collector.
The manual checkpoint command now pairs with `persist_session` like the
timer does, or the next launch's collector deletes the file the user
explicitly asked for. The checkpoint ceiling is anchored on the start
of the dirty streak rather than the last checkpoint, so a pane that
never goes quiet is still captured before its first one, and is clamped
from below so a burst after a long idle is not captured mid-stream.
The record gutter is no longer drawn while the alternate screen is
active — its rows are a different coordinate space and would collide.
Encoding moves off the terminal lock. Capture now clones rows under the lock (a memcpy) and hands them to the persist worker, which does the style interning and deflate. The io thread blocks on that same mutex to drain the pty, so the previous arrangement — deflate inside the locked section, once per pane, at every checkpoint and at quit — stalled the one path this repo is measured on. The comment claiming the lock was held "only for the encode" is now true. Startup no longer serializes per-pane I/O. Referenced snapshots are read and inflated in parallel before the restore loop, so the winit thread pays one pane's latency instead of every pane's; only the prepend, which touches the terminal, stays on it. Storage lifecycle. Interrupted writes leave a `.tmp` holding the same plaintext the snapshot does, and the collector only looked at `.nsb` — so turning the feature off did not actually drain the directory. Backup exclusion is applied on every `ensure_dir` rather than only at creation, since its result is discarded and one transient failure would otherwise put every later snapshot into Time Machine forever. Closing a pane, tab or window now deletes that pane's snapshot instead of leaving it for the next launch's sweep — closing a tab is the most direct way someone says they are done with that output. Restore no longer adopts the saved key. Two instances restoring the same session would claim the same file and overwrite each other; each pane now mints its own and drops the file it was restored from once its first capture lands, which keeps crash-resilience through the gap and leaves one file per pane in steady state (verified across four relaunches). Enabling the feature says so. The config file is the documented way to turn it on and had no acknowledgement at all; it now logs what is being written and where, once per enable (spec §9). Also: the inflate ceiling derives from the configured budget and rejects rather than truncating; decoded hyperlink ids are bounded against the table that came with them and the remap runs unconditionally, so a corrupt file cannot adopt a live URI; and a record boundary clears the preceding row's wrap flag so rewrap cannot glue the separator onto real output. `a_skipped_row_below_the_eviction_point_is_ignored` passed `usize::MAX`, which is above every valid row — it tested the wrong branch. It now evicts for real, and two companion tests cover the above-range and lands-on-a-live-row cases.
Stage 2 of docs/specs/scrollback-persistence.md, less the badge. `scrollback-persist-encrypt` seals each snapshot with AES-256-GCM under a 256-bit key generated on first use and kept in the login keychain, marked non-syncable so a record of one machine's terminal output never appears on another. The container wraps noa-grid's format from outside, so the grid crate stays free of any notion of a keychain and a snapshot decodes identically whether or not it was ever sealed. Reading branches on the file rather than on the config, so toggling the setting does not strand what is already stored. When encryption is on and no key can be obtained, the write is declined rather than silently falling back to plaintext — the user asked for encryption specifically. Off by default, because it is a real trade rather than a free win: lose the keychain entry and the records are unreadable. `security-framework` 3.7 does not expose `kSecAttrAccessible`, so the key gets the login keychain's default (readable only while unlocked) plus an explicit no-sync; recorded as a deviation. Kitty-graphics placeholder cells are replaced with an `[image]` marker at capture. The image itself lives in a store that is not persisted and would not exist in the next process, so serializing the placeholder scalars restores a run of tofu, while dropping them silently erases the fact that anything was there. The crypt tests deliberately go through a key-supplied split rather than the public entry points: an earlier version called `seal()` directly and minted a real keychain item on the developer's machine as a side effect of `cargo test`.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3b0bb7ba4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+436
to
+439
| if let Some(key) = surface.scrollback_key.clone() | ||
| && let Some(persister) = self.scrollback_persister.as_ref() | ||
| { | ||
| persister.discard(key); |
There was a problem hiding this comment.
When this command is used immediately after restoration, surface.scrollback_key is still None; the on-disk file that supplied the displayed record is stored in surface.superseded_scrollback_key. Consequently, the command removes the rows visually but leaves the sensitive snapshot and its session reference intact, so a crash followed by relaunch restores the supposedly discarded history. Delete the superseded key and persist the updated session as part of this operation.
Useful? React with 👍 / 👎.
Comment on lines
+170
to
+172
| // This pane's own file now holds everything the restored one did. | ||
| if let Some(superseded) = superseded { | ||
| persister.discard(superseded); |
There was a problem hiding this comment.
For the first capture after restoring a pane, persister.save only queues an asynchronous write, yet the valid source snapshot is immediately queued for deletion. If encoding, encryption/keychain access, or the filesystem write fails—or the process crashes before the worker finishes—the source can be deleted while the session has already switched to the new key, leaving no restorable record. The old key should only be removed after the worker confirms that the replacement was durably written.
Useful? React with 👍 / 👎.
Comment on lines
+217
to
+218
| let scrollback_persist_disabled = previous.scrollback_persist.persists() | ||
| && !(applied.scrollback_persist.persists() && applied.window_save_state.restores()); |
There was a problem hiding this comment.
When a config reload changes only scrollback-persist-limit from a positive value to 0, persistence mode and window-save-state remain enabled, so this condition is false and existing snapshots are not deleted. They can remain on disk indefinitely while panes are quiet, despite zero being documented and handled by capture as an instruction to retain nothing. Include the positive-to-zero limit transition in the immediate purge condition.
Useful? React with 👍 / 👎.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restoring a session brought back the tabs and splits but not a single line of output, and said nothing about it. The layout coming back is itself a promise that the contents did — this closes the gap from both ends.
Honesty (ships even with persistence off). A pane restored with no record now says so, in one line that names the key which would change that. This is the part that works by default; the rest is opt-in.
Persistence (
scrollback-persist = tail, defaultnever). Each pane's scrollback tail is captured on clean quit and on idle checkpoints, and restored on launch above the live shell behind a labelled boundary row, with a gutter marking the restored rows. Restored rows are ordinary scrollback, so selection, copy and search work on them with no extra code.Default
neveris deliberate: Ghostty restores no contents, so enabling by default would be an observable deviation, and persisting terminal output changes what a stolen disk yields. Spec:docs/specs/scrollback-persistence.md(§12 lists every deliberate deviation and what was withdrawn).Design notes worth reviewing
The wire format works at the
Row/Celllevel, not the paged scrollback. SerialisingPagedScrollbackdirectly looks obvious and is wrong:GraphemeIdindexes a process-global interner andHyperlinkIdindexesTerminal::hyperlinks, so a file written with those ids decodes to different text in the next process. Both are resolved to their content and rebuilt on load.Capture does not share
persist_session's trigger. That runs on every structural change; re-encoding a megabyte per pane each time a split moves is not viable. Checkpoints fire 5s after output settles, with a 60s ceiling anchored on the start of the dirty streak so a sustained flood — the long build whose tail matters most — is still captured. A checkpoint also rewrites the topology document: a key minted but not recorded there is deleted by the next launch's collector, which is precisely the crash the checkpoint exists to survive.Cached absolute row indices are invalidated by coordinate generation.
record_rows/annotation_roware session-absolute, and both a column reflow and a scrollback clear renumber that space.Terminalalready bumps a generation on exactly those and deliberately leaves it alone for ordinary eviction, so comparing against it is the complete staleness test. On mismatch the marking is dropped rather than acted on — the boundary itself is a row, so it survives reflow either way.Storage posture.
0700directory,0600files, excluded from Time Machine, nothing created at all while disabled, and no data-dir fallback (an unresolved data directory means persistence is unavailable, not "write into the working directory"). Snapshot keys are validated as 16 hex digits — they come from a user-writable file and are interpolated into a path. Scratch terminals, the quick terminal, remote panes and the alternate screen are never captured. Capture also requireswindow-save-state, since the reference that makes a snapshot restorable lives in the session file.Restore is inert by construction. Rows are inserted as history rather than replayed as VT input, so a snapshot cannot move the cursor, set a title, or write to the pty.
Changes
a947268): four noa-only keys plusscrollback-persist-encrypt;scrollback_dir(). Days and bytes are plain integers, following the crate's existing convention — there is no unit-suffix parser anywhere in noa's config, so7ddiagnoses rather than silently parsing as 7a947268): theNOASBformat,Terminalcapture/restore, rewrap on width change (never separating a wide glyph from its spacer),discard_history_prefix4783050): the store and its worker, capture triggers, launch collector, the record view, session schema v3, two palette commands, a Settings row, and the renderer's column-0 gutter440f72c,6fe1933,d1dfb59): see belowa3b0bb7): AES-256-GCM under a login-keychain key, and[image]markers for kitty-graphics placeholder cellsReview
Ran
/judge(Codex + Claude; agy failed to run — its sandbox blockedgit diffand it exited 0 without an artifact). 28 findings after dedup, all grounded. Every HIGH and MEDIUM is fixed in6fe1933/d1dfb59; two were resolved as documentation with the reasoning recorded, and one was rejected outright:Row::occupied()instead of value equality. Restored rows come fromRow::from_cellswith a conservative full-length watermark, so this would encode every column on re-capture and bloat snapshots — a regression dressed as a fixscrollback-persist-max-age-daysbounds file age, not content age. Bounding content age needs per-row timestamps and a format changeThree defects were found by running the thing rather than by reading it: the record separator being re-captured and accumulating one per relaunch, a non-terminating loop in
rewrapat width 1 with a wide glyph (a test had to be killed to confirm it), and a unit test that minted a real generic password in the developer's login keychain as a side effect ofcargo test.cargo run -p noa-grid --example dump-snapshot -- <file>.nsbdecodes a snapshot and prints each row's text and pen. The format is opaque, so a wrong-looking restored pane otherwise gives no way to tell whether the capture, the file, or the restore is at fault — the accumulation bug was found with it.Test plan
cargo test --workspace --offline: green, 0 failures (run with the sandbox disabled —noa-ipcneeds loopback bind)cargo clippy --workspace --all-targets --offline: cleanCellfield, malformed/truncated/hostile input, the byte budget against a link that lives in the side table, path-traversal rejection of snapshot keys, collector orphan/expiry/budget/temp-sweep, and rewrap terminationHOME: the idle checkpoint fires unprompted and writes0600in a0700directory; the record survives four relaunch generations with zero separators baked in and one file per pane;window-save-state = nevercreates no directory at all; turning the setting off purges immediately; and the coordinate-generation guard was measured (not assumed) to leave the marking live across every frame of a normal restoreNot done
draw_toast_cardtakes no placement, so a persistent badge needs new drawing and positioning code — and screen-recording permission is currently revoked on this machine, so it could not be looked at. Adding an unverifiable visual element to a layer whose GPU traps are documented in CLAUDE.md is not a good trade. The boundary is already carried by the separator row, which labels itselfscreencaptureandCGEvent.postToPidare both refused by TCChttps://claude.ai/code/session_011rXCoq61BVttWaYHCtJzvC