diff --git a/COHERENCE.md b/COHERENCE.md index 9fe85f0..4e7361c 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -368,7 +368,7 @@ Full verdict table: | 5 | Edit | **Works** | Full CUA + Emacs keymap in 161 lines (`builtin/keymaps/default.lua`); isearch, query-replace, kill ring, undo/redo, auto-indent/pair/comment, atomic save. Genuinely excellent zero-config | | 6 | Language intelligence | **Partial** | Rust grammar bundled and auto-attaches; rust-analyzer preconfigured (`builtin/runtime/lsp.lua:44-52`) — but a missing binary fails silently (§1.2) and highlighting masks it. No LSP status command exists to diagnose | | 7 | Find symbol / file | **File: fixed (open by path merged #162; browsing PR #165). Symbol: works but undiscoverable** | No find-file/dired/picker existed at audit. Now `C-x C-f` opens a known path and `C-x d` / `C-x C-j` browse (flat listing, `dired` mode keymap); `M-.`/`M-?`/`C-c o` still bound but advertised nowhere and server-gated; no workspace-symbol command; `pmacs.index.*` has no UI | -| 8 | Open terminal | **Works but undiscoverable** | Full PTY with scrollback + modeline segment — reachable only as `M-x terminal`, no keybinding. *Was broken outright on the GPU frontend until the double terminal-layout sync was fixed: the child took a `SIGWINCH` storm at tick cadence, so typing into it was impossible while output still flowed.* | +| 8 | Open terminal | **Works** | Full PTY with scrollback + modeline segment, bound to `C-c t` and configurable through three registered settings (`terminal.default-profile`, `terminal.scrollback-rows`, `terminal.escape-key`) plus named `pmacs.terminal.profiles` (PR #173). Named limitation: `C-c t` is unreachable from *inside* a terminal window, where `C-c` is consumed as the escape — `M-x terminal` still works there. *Was broken outright on the GPU frontend until the double terminal-layout sync was fixed: the child took a `SIGWINCH` storm at tick cadence, so typing into it was impossible while output still flowed.* | | 9 | Build / test | **Partial** | `M-x compile.run` works, defaults cwd to detected project root, parses Rust `-->` errors — but no keybinding, an **empty first prompt** (`initial = last and last.cmdline or ""`, `builtin/runtime/compile.lua:1134-1138`), and no `cargo build`/`cargo test` suggestion despite `ProjectKind::Cargo` existing (`src/project.rs:77`) | | 10 | Inspect error | **Partial (good once reached)** | `E:n W:n` modeline counts, underlines, `M-g n/p` + ``C-x ` `` walking a unified compile/grep/diag source, message echo, `RET` visits. Gated entirely on step 6 or 9 succeeding first | | 11 | See background work | **Works but undiscoverable** | `*workers*` view via `M-x editor.list-workers`; `C-c C-k` cancel-at-point. No keybinding, no statusline spinner/progress indicator anywhere (§9) | @@ -379,6 +379,13 @@ A journey observation worth keeping verbatim from the audit: C-M-s` opens all folds, while opening a file, opening a terminal, and running a build have no bindings at all. +Two of that observation's three examples have since been answered — +opening a file by `C-x C-f` (#162) and opening a terminal by `C-c t` +(#173). **Running a build still has no binding**, and the underlying +inversion is a standing bias in how new work gets bound, not three +isolated omissions: the quote stays as written because it names the +pattern, and the pattern is not retired until step 9 is. + --- ## 3. A Strong Zero-Configuration State @@ -639,7 +646,7 @@ Everything funnels through one function: `EditorInstance::dispatch_key` | 3 | query-replace | `editor.rs:945` | `QueryReplaceKey::from_chord` (`editor.rs:2967`) | **full shadow** | | 4 | Minibuffer | `editor.rs:951` | `MinibufferAction::from_chord` (`src/minibuffer.rs:468`) | **full shadow** | | 5 | Completion popup | `editor.rs:958-971` | `CompletionPopupKey::from_chord` (`editor.rs:3056`) | **partial shadow** (control chords only; skipped while a multi-key prefix is pending) | -| 6 | Terminal transport + `C-c` escape | `editor.rs:973-1010` | `is_terminal_escape_chord` (`editor.rs:4355`) | **partial, transport-level** | +| 6 | Terminal transport + configurable escape | `editor.rs:973-1010` | `EditorState::terminal_escape_chord` → `TerminalManager::escape_chord` (`src/terminal/session.rs`) | **partial, transport-level** | | 7 | Ordinary dispatch | `editor.rs:1018-1032` | `KeymapStack::resolve` | the only inspectable layer | Facts that define the gap: @@ -647,8 +654,10 @@ Facts that define the gap: - **Full shadows eat every key**, including unrecognized ones (each decoder has an `Ignore`/`Dismiss` fallback arm). While a terminal buffer is focused and unescaped, *all* keys encode to the child — - `C-c`-leading user bindings are **structurally unreachable** in a - terminal buffer. + bindings led by the escape chord are **structurally unreachable** in + a terminal buffer. Since #173 that chord is `terminal.escape-key` + rather than a hardcoded `C-c`, so a user can *move* which prefix is + eaten; they cannot make the shadow stop eating one. - **No transient-keymap mechanism exists to migrate to.** `KeymapStack` has exactly three fixed scopes — `Buffer(BufferId)`, `Mode(String)`, `Global` (`src/keymap_stack.rs:37-44`); resolution order buffer → @@ -1013,20 +1022,29 @@ layering, provenance, and adoption have not followed.** `ConfigValue`s; `describe-setting`'s "Source:" names where `define()` ran. The inspection view sketched above is currently impossible to render. -- **Adoption is five settings**: `editing.auto-pair` (pair.lua), +- **Adoption is eight settings**: `editing.auto-pair` (pair.lua), `editing.trim-on-save` (editops.lua), `autosave.interval-ms` (autosave.lua), `window.panel-height` + `window.min-height` - (window.lua). Everything else a user might set — theme, fonts, LSP + (window.lua), and `terminal.default-profile` + + `terminal.scrollback-rows` + `terminal.escape-key` (terminal.lua, + #173). Everything else a user might set — theme, fonts, LSP server config, killring size, recentf/saveplace/desktop enables, pair sets, comment strings, `pmacs.parse.*` — lives in raw Lua outside the registry and is therefore invisible to `describe-setting` and any future settings UI. The migration list is already written: `docs/config-registry-framing.md` "named deferrals" (table-valued settings are the hard prerequisite for LSP/pair/comment tables). +- **The table-valued gap now has a named, shipped instance.** + `pmacs.terminal.profiles` (#173) is a raw Lua table sitting beside + three registered scalars *for the same feature*, because a profile is + inherently `{ command, args, cwd, env }` and the registry stores four + scalars. It is the clearest evidence yet that table-valued settings + are the blocking prerequisite: the terminal is now half-registered, + and no settings UI can render the half that matters most. - **No persistence**: settings changed at runtime do not survive restart (the `custom-file` split-brain question is a named deferral). - The three-level separation holds in principle today (registry / - hooks+keymaps / packages), but with five settings registered, level 1 + hooks+keymaps / packages), but with eight settings registered, level 1 is effectively empty — users need executable Lua for nearly every ordinary preference, which is the exact failure the section warns about. diff --git a/builtin/runtime/terminal.lua b/builtin/runtime/terminal.lua index a6573fe..143a663 100644 --- a/builtin/runtime/terminal.lua +++ b/builtin/runtime/terminal.lua @@ -65,27 +65,44 @@ local PROFILE_FIELDS = { env = "table", } +-- Every diagnostic below renders a caller- or user-supplied value, so +-- rendering must never be the thing that fails. `%q` is partial — it +-- raises on a table or function — and a profile name arrives straight +-- from `open { profile = ... }`. +local function describe_name(name) + if type(name) == "string" then return string.format("%q", name) end + return string.format("<%s %s>", type(name), tostring(name)) +end + local function validate_profile(name, profile) + local shown = describe_name(name) if type(profile) ~= "table" then - error(string.format("terminal profile %q must be a table", name), 0) + error(string.format("terminal profile %s must be a table", shown), 0) end for key, value in pairs(profile) do local expected = PROFILE_FIELDS[key] if not expected then - error(string.format("terminal profile %q: unknown field %q", name, tostring(key)), 0) + error(string.format("terminal profile %s: unknown field %q", shown, tostring(key)), 0) end if type(value) ~= expected then error(string.format( - "terminal profile %q: field %q must be a %s, got %s", - name, key, expected, type(value)), 0) + "terminal profile %s: field %q must be a %s, got %s", + shown, key, expected, type(value)), 0) end end return profile end +-- `terminal.profiles` is a raw user table, so its keys are whatever the +-- user wrote. Sorting them directly raises "attempt to compare number +-- with string" the moment the table holds both a string and a numeric +-- key — and it raises on the UNKNOWN-PROFILE path, replacing the very +-- error this list exists to explain with an opaque one. Sorting DISPLAY +-- strings is total over every key type, so the diagnostic survives a +-- malformed table. local function known_profile_names() local names = {} - for name in pairs(terminal.profiles) do names[#names + 1] = name end + for name in pairs(terminal.profiles) do names[#names + 1] = tostring(name) end table.sort(names) return names end @@ -106,7 +123,8 @@ local function resolve_profile(requested) local known = known_profile_names() local listed = #known > 0 and table.concat(known, ", ") or "(none defined)" error(string.format( - "terminal profile %q is not defined; known profiles: %s", name, listed), 0) + "terminal profile %s is not defined; known profiles: %s", + describe_name(name), listed), 0) end return validate_profile(name, profile) end diff --git a/docs/active-work.md b/docs/active-work.md index aeac490..972c40b 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -396,8 +396,9 @@ If it does not, stop and repair the remote/fetch configuration. Stage 1's branch. Two stages, two branches, two PRs; **no protocol change**. - **Stage 1 = `githubsucks/terminal-config`**, worktree - `../pmacs-terminal-config`, based on `githubsucks/main` @ `d152120`. - Profiles, scrollback, escape key, and the `C-c t` opening binding. + `../pmacs-terminal-config`, based on `githubsucks/main` @ `d152120` + and merged up to `ccf29e3` during review round 1. Profiles, + scrollback, escape key, and the `C-c t` opening binding. - **Stage 2 = `terminal-copy-mode`, not started.** Branch it off `main` after Stage 1 merges: no dependency, but both edit `builtin/runtime/terminal.lua`. @@ -424,16 +425,63 @@ If it does not, stop and repair the remote/fetch configuration. `Ctrl-X` is invisible; and the probe **counts occurrences** rather than testing presence, because a single-character probe collides with the child's own banner text. -- Verification on this branch (against the committed tree): `cargo fmt - --check` clean; strict workspace Clippy clean; 1,832 default + 2,009 - CRDT library tests; `terminal_config_acceptance` 10/10 in **both** - configurations; vterm Stage 1/2/3 9+10 / 6+6 / 5+9; config registry 16; - bottom-panel 46; listview 6; compile 67 (isolated config); M4 121; - required GPU 202; **isolated-config workspace sweep 3,262 across 94 - suites**; `git diff --check` clean. +- **Review round 1 (2026-07-25) — five findings, all real, all fixed.** + One blocker and two majors were the same failure in three places: a + claim asserted somewhere cheaper than where it lives. + - *Blocker — `COHERENCE.md` was stale in four places, not the three + reported.* Step 8 still read "no keybinding"; §11 still read "five + settings"; and §6's dispatch table still cited + `is_terminal_escape_chord`, **a symbol this PR deletes**. §25 makes + that update ride the PR. A PR that changes audited ground truth has + to re-grep the audit for its own symbols, not only for its topic. + - *Major — acceptance 5 was vacuous.* It asserted a registry + round-trip, so it stayed green with the setting's **only** consumer + deleted. It now opens a real terminal whose child overflows the + 24-row screen, scrolls the view to its oldest retained row, and + asserts `LINE001` is present at 10,000 and absent at 0. **Asserting + a value was stored is not asserting anything reads it.** + - *Major — acceptance 8a asserted the session count, not the cache.* + An editor-side map with no purge hook — the exact rejected design — + leaks *while* sessions drain, so it passed. Fixed with a + `TerminalManager::escape_caches()` seam. **A lifecycle claim needs a + lifecycle observable.** + - *Moderate — `table.sort` over user-controlled profile keys.* A + table holding both a string and a numeric key raised `attempt to + compare number with string` **on the unknown-profile path**, + replacing the diagnostic being asked for; `%q` raised likewise on a + non-string `profile` argument. Both are partial functions applied to + user input **on a diagnostic path** — the error reporter was the + thing that failed. + - *Minor — the committed framing still said "not yet approved".* +- **Three new bites, each falsified by revert**: deleting the scrollback + consumer fails acc5 (and only acc5); restoring the raw-key sort + reproduces `attempt to compare string with number` verbatim; and + implementing the rejected editor-side map fails the new acc8a at + `left: 2, right: 1` **while passing the old session-count version** — + which is the review finding demonstrated rather than argued. +- Verification after the round-1 fixes, on the tree merged with + `githubsucks/main` @ `ccf29e3`: `cargo fmt --check` clean; strict + workspace Clippy clean; 1,832 default + 2,009 CRDT library tests; + `terminal_config_acceptance` **12/12 in both configurations**; vterm + Stage 1/2 9+10 / 6+6; config registry 16+16; bottom-panel Stage 1 + 46+46; M4 121; required GPU 202; `git diff --check` clean. - `compile_mode_acceptance` fails 11/67 against the **real** user config and passes 67/67 with an isolated `XDG_CONFIG_HOME` — the known pre-existing trap, not this branch. + - **`vterm_stage3_acceptance::a37` fails on this machine — and fails + identically on the PR's own base `d152120`**, so it is not this + branch's regression. It is load-sensitive: it passed at `d152120` + once and failed at that same commit twenty minutes later, with a + second agent saturating the machine with `rustc` in between. Two + ways it lies, both worth knowing: it **silently returns `ok` when + `pmacs-gpu` is not built** in the same target dir (only + `PMACS_REQUIRE_GPU=1` promotes that skip to a failure, and the gate + list applies that flag to `-p pmacs-gpu`, a *different* package), and + it is **crdt-gated, so CI has never run it at all**. A green a37 in + a gate log means nothing unless the binary was built and the flag + was set. Needs its own lane; see the CI `crdt`-coverage lane on #168. + - `pmacs-gpu` itself failed 201/202 once under the same load and passed + 202/202 on immediate rerun. ## Bottom-panel lane (Arc 7) — Stage 1 MERGED; Stage 2 (GPU band) is next diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index 3f13987..48b75d8 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -1,7 +1,9 @@ # Terminal configuration and copy mode **Revision 4 — scouted against canonical `main` @ `b889873` (protocol v20), -2026-07-25. Not yet approved; no branch, no implementation.** +2026-07-25. APPROVED after four review rounds. Stage 1 is implemented on +branch `terminal-config` (PR #173); Stage 2 (`terminal-copy-mode`) is +framed but not started, and branches off `main` after Stage 1 merges.** Revision 4 gives the escape-key cache an owner and a lifecycle (Q#TC4c) — revision 3 named the key but not the storage, and two implementations @@ -503,6 +505,15 @@ additive, on its own binding, and does not replace scroll-and-select. error that **lists the known profile names**, and creates no buffer, session, or process. An explicitly passed unknown `profile` fails the same way **even when `terminal.default-profile` is valid** (Q#TC3a). +2a. That diagnostic is **total over a malformed profiles table** (review round + 1). `pmacs.terminal.profiles` is a raw user table, so listing its names must + not assume its keys are comparable and rendering a requested name must not + assume it is a string: a table holding both a string and a numeric key made + `table.sort` raise `attempt to compare number with string` *on the + unknown-profile path*, replacing the exact error being asked for, and `%q` + raises on a non-string `profile` argument. Both are partial functions + applied to user input on a diagnostic path — the failure class is + "the error reporter is the thing that fails". 3. Field-by-field resolution follows Q#TC3a: explicit open field beats profile field beats scalar setting beats `$SHELL`. `env` **merges**, with explicit entries overriding profile entries of the same name. @@ -623,6 +634,19 @@ Full gate suite per `CLAUDE.md` for each PR separately, plus: chord unreachable); **10** (its failure mode is a terminal nobody can escape); and **16/17** (a read-only buffer that silently accepts an edit on both sides). +- **The observation seams the cache pins need are `escape_parses` (how often) + and `escape_caches` (how many are still held).** Neither is inferable from + behavior: for a *valid* setting a correct per-session cache and a leaking + editor-side map produce identical keystroke results, and both leave the + session count draining normally. Review round 1 caught 8a asserting the + session count instead — which the unpurged-map bite passes, since a map with + no purge hook leaks *while* sessions drain. A lifecycle claim needs a + lifecycle observable; the count of live sessions is not one. +- **Criterion 5 must open a real terminal and read back retained history.** + Round 1 caught it asserting a registry round-trip instead, which is a test of + the registry: it stays green with the setting's only consumer deleted. The + same shape to watch for anywhere — *asserting that a value was stored is not + asserting that anything reads it*. - **Do not gate the new suites on `#[cfg(feature = "crdt")]` unless a test genuinely needs CRDT.** CI never enables that feature, so a suite gated that way is written and then never run — 264 tests are currently dark for exactly diff --git a/src/terminal/session.rs b/src/terminal/session.rs index 6e71ea3..c731fb0 100644 --- a/src/terminal/session.rs +++ b/src/terminal/session.rs @@ -607,7 +607,7 @@ impl TerminalManager { let already = previously_reported.as_deref() == Some(spelling); let message = (!already).then(|| { format!( - "terminal.escape-key {spelling:?} is not a valid chord ({error}); using C-c" + "terminal.escape-key {spelling:?} is not a valid chord ({error}); using C-c" ) }); (fallback, Some(spelling.to_owned()), message) @@ -633,6 +633,24 @@ impl TerminalManager { self.escape_parses } + /// How many terminals currently hold a cached escape chord. + /// + /// The LIFETIME half of Q#TC4c's cache contract, which `escape_parses` + /// cannot cover: parse counting says a valid setting is read once, but + /// says nothing about whether the cache is ever released. Because the + /// cache lives on [`TerminalSession`], this count falls with the + /// session set by construction — which is exactly the property worth + /// pinning, since the rejected alternative (an editor-side + /// `HashMap`) has no purge hook and would hold + /// this at its high-water mark while sessions drained. + #[must_use] + pub fn escape_caches(&self) -> usize { + self.sessions + .values() + .filter(|session| session.escape.is_some()) + .count() + } + /// Resize a terminal screen and its PTY after validating shared limits. pub fn resize( &mut self, diff --git a/tests/terminal_config_acceptance.rs b/tests/terminal_config_acceptance.rs index a613eb2..ceeb8fe 100644 --- a/tests/terminal_config_acceptance.rs +++ b/tests/terminal_config_acceptance.rs @@ -32,13 +32,18 @@ fn eval_err(state: &EditorState, src: &str) -> String { } } -fn screen_text(state: &EditorState, buffer: pmacs::buffer::BufferId) -> String { - let manager = state.terminal_manager.borrow(); - let Some(snapshot) = manager.snapshot(buffer) else { - return String::new(); - }; +/// The viewport every test projects through. Deliberately SHORTER than +/// the 24-row screen a terminal opens with, so "scroll to the oldest +/// retained row" has somewhere to go even when nothing is retained — +/// which is what makes the two scrollback arms differ by content rather +/// than by whether scrolling was possible at all. +fn viewport() -> CellSize { + CellSize::new(10, 40) +} + +fn cells_to_text(cells: &[pmacs::cell::Cell]) -> String { let mut text = String::new(); - for cell in &snapshot.cells { + for cell in cells { match &cell.glyph { Glyph::Char(c) => text.push(*c), Glyph::Cluster(b) => text.push_str(&String::from_utf8_lossy(b)), @@ -48,6 +53,33 @@ fn screen_text(state: &EditorState, buffer: pmacs::buffer::BufferId) -> String { text } +fn screen_text(state: &EditorState, buffer: pmacs::buffer::BufferId) -> String { + let manager = state.terminal_manager.borrow(); + let Some(snapshot) = manager.snapshot(buffer) else { + return String::new(); + }; + cells_to_text(&snapshot.cells) +} + +/// Text a view actually shows, which is where retained history is +/// visible at all — the live `screen_text` above always reads the tail. +fn view_text(state: &EditorState, key: TerminalViewKey) -> String { + let mut manager = state.terminal_manager.borrow_mut(); + manager + .snapshot_for_view(key, viewport()) + .map(|snapshot| cells_to_text(&snapshot.cells)) + .unwrap_or_default() +} + +/// Scroll a view to its OLDEST retained row and read it back. +fn oldest_view_text(state: &EditorState, key: TerminalViewKey) -> String { + state + .terminal_manager + .borrow_mut() + .scroll_view(key, viewport(), i32::MAX); + view_text(state, key) +} + fn tick_until(state: &mut EditorState, needle: &str, buffer: pmacs::buffer::BufferId) -> bool { let deadline = Instant::now() + Duration::from_secs(5); loop { @@ -71,7 +103,7 @@ fn focus_terminal(state: &EditorState, buffer: pmacs::buffer::BufferId) -> Windo let mut manager = state.terminal_manager.borrow_mut(); manager.register_view(key); manager.claim_controller(key); - let _ = manager.snapshot_for_view(key, CellSize::new(10, 40)); + let _ = manager.snapshot_for_view(key, viewport()); window } @@ -223,6 +255,44 @@ fn acc2_unknown_profile_lists_known_names_and_creates_nothing() { assert_eq!(state.terminal_manager.borrow().len(), 0); } +/// Acceptance 2 (malformed table): `pmacs.terminal.profiles` is a raw +/// user table, so a diagnostic that walks its keys must be total over +/// them. A table holding both a string and a numeric key made +/// `table.sort` raise "attempt to compare number with string" — on the +/// unknown-profile path, replacing the exact error being asked for. +#[test] +fn acc2_malformed_profile_keys_do_not_mask_the_unknown_profile_error() { + let state = EditorState::new(); + exec(&state, CAT_PROFILE); + exec( + &state, + r#"pmacs.terminal.profiles[1] = { command = "/bin/sh" }"#, + ); + + let err = eval_err( + &state, + r#"return pmacs.terminal.open { profile = "ghost" }"#, + ); + assert!( + err.contains("ghost") && err.contains("echo"), + "the unknown-profile error must survive a malformed table: {err}" + ); + assert!( + !err.contains("attempt to compare"), + "listing known profiles must not raise: {err}" + ); + + // Rendering the REQUESTED name is partial too: `%q` raises on a + // table, and the name arrives straight from the caller. + let err = eval_err(&state, r"return pmacs.terminal.open { profile = {} }"); + assert!( + err.contains("is not defined") && err.contains("known profiles"), + "a non-string profile name must render, not raise: {err}" + ); + + assert_eq!(state.terminal_manager.borrow().len(), 0); +} + /// Acceptance 3: explicit beats profile beats setting beats `$SHELL`, and /// `env` MERGES rather than replacing. #[test] @@ -282,10 +352,77 @@ fn acc3_acc4_explicit_command_wins_and_empty_default_means_no_profile() { state.process_supervisor.borrow_mut().shutdown(); } -/// Acceptance 5: scrollback resolves from the setting, is overridden by an -/// explicit value, and `0` is legal. +/// A child that overflows the 24-row screen and then goes quiet, so its +/// early output can only still be found in RETAINED HISTORY. Zero-padded +/// so `LINE001` is not a substring of `LINE100`. +const FILL_PROFILE: &str = r#" +pmacs.terminal.profiles.fill = { + command = "/bin/sh", + args = { "-c", + "i=1; while [ $i -le 200 ]; do printf 'LINE%03d\r\n' $i; i=$((i+1)); done; printf 'DONE\r\n'; exec cat" }, +} +"#; + +/// Acceptance 5: the scrollback SETTING reaches the screen's retained +/// history, an explicit spec value overrides it, and `0` is legal. +/// +/// Asserted end to end, through a real child and a real view, rather +/// than by reading the value back out of the registry: a registry +/// round-trip is a test of the registry, and would stay green with the +/// setting's only consumer (`terminal.lua`'s `resolved.scrollback_rows` +/// fallback) deleted outright. #[test] -fn acc5_scrollback_setting_override_and_bounds() { +fn acc5_scrollback_setting_reaches_retained_history() { + let mut state = EditorState::new(); + exec(&state, FILL_PROFILE); + + // Arm 1: `0` is legal, and means the early rows are GONE. + exec(&state, r#"pmacs.config.set("terminal.scrollback-rows", 0)"#); + let none = open_cat_terminal(&state, r#"profile = "fill""#); + assert!(tick_until(&mut state, "DONE", none), "child finished"); + let window = focus_terminal(&state, none); + let none_key = TerminalViewKey::new(FrontendId::LOCAL, window, none); + let oldest = oldest_view_text(&state, none_key); + assert!( + !oldest.contains("LINE001"), + "with scrollback 0 the oldest retained row must not be the \ + child's first line: {oldest:?}" + ); + + // Arm 2: a large setting retains it, reachable by scrolling back. + exec( + &state, + r#"pmacs.config.set("terminal.scrollback-rows", 10000)"#, + ); + let kept = open_cat_terminal(&state, r#"profile = "fill""#); + assert!(tick_until(&mut state, "DONE", kept), "child finished"); + let window = focus_terminal(&state, kept); + let kept_key = TerminalViewKey::new(FrontendId::LOCAL, window, kept); + let oldest = oldest_view_text(&state, kept_key); + assert!( + oldest.contains("LINE001"), + "with scrollback 10000 the first line must survive in history: \ + {oldest:?}" + ); + + // Arm 3: an explicit spec value beats the setting, which is still 10000. + let overridden = open_cat_terminal(&state, r#"profile = "fill", scrollback_rows = 0"#); + assert!(tick_until(&mut state, "DONE", overridden), "child finished"); + let window = focus_terminal(&state, overridden); + let overridden_key = TerminalViewKey::new(FrontendId::LOCAL, window, overridden); + let oldest = oldest_view_text(&state, overridden_key); + assert!( + !oldest.contains("LINE001"), + "an explicit scrollback_rows = 0 must beat the setting: {oldest:?}" + ); + + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 5 (bounds): the registered range rejects out-of-range +/// values, and `0` is inside it rather than a disabled sentinel. +#[test] +fn acc5_scrollback_bounds() { let state = EditorState::new(); exec(&state, r#"pmacs.config.set("terminal.scrollback-rows", 0)"#); assert_eq!( @@ -397,6 +534,11 @@ fn acc7_acc8_acc8a_per_terminal_escape_cache_identity_and_lifecycle() { ); assert!(escape_was_armed(&mut state, b, 'N'), "B primes on its C-b"); let primed = state.terminal_manager.borrow().escape_parses(); + assert_eq!( + state.terminal_manager.borrow().escape_caches(), + 2, + "each primed terminal holds its own cache" + ); // Acceptance 7 — BOTH directions. Asserting only that A still works // after A->B->A is not enough: an epoch-only cache hands whichever @@ -442,6 +584,13 @@ fn acc7_acc8_acc8a_per_terminal_escape_cache_identity_and_lifecycle() { ); // Acceptance 8a: the cache dies with its terminal. + // + // Waiting for the SESSION count to fall is not the assertion — a + // session set that drains while an editor-side `HashMap` keeps its entry (the rejected implementation named + // in Q#TC4c, which has no purge hook) satisfies it exactly. The + // discriminating observable is the CACHE count, which such a map + // would hold at its high-water mark of 2. let sessions_before = state.terminal_manager.borrow().len(); exec(&state, "pmacs.terminal.terminate(TERM_A)"); exec(&state, "pmacs.buffer.kill(TERM_A)"); @@ -452,10 +601,31 @@ fn acc7_acc8_acc8a_per_terminal_escape_cache_identity_and_lifecycle() { state.tick_processes(); assert!( Instant::now() < deadline, - "killing the terminal must remove its session, and with it the cache" + "killing the terminal must remove its session" ); thread::sleep(Duration::from_millis(20)); } + assert_eq!( + state.terminal_manager.borrow().escape_caches(), + 1, + "killing terminal A must drop ITS cache, not merely its session" + ); + + // ...and the surviving cache is B's, so the right one was dropped. + focus_terminal(&state, b); + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('b'), KeyModifiers::CONTROL), + ); + assert!( + escape_was_armed(&mut state, b, 'T'), + "terminal B must still escape on its own C-b after A was killed" + ); + assert_eq!( + state.terminal_manager.borrow().escape_parses(), + primed, + "B's surviving cache must not have been reparsed" + ); state.process_supervisor.borrow_mut().shutdown(); }