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 6be0987..143a663 100644 --- a/builtin/runtime/terminal.lua +++ b/builtin/runtime/terminal.lua @@ -3,6 +3,38 @@ local terminal = assert(pmacs.terminal, "pmacs.terminal raw bindings are required") local raw_open = assert(terminal._open, "pmacs.terminal._open is required") +-- Q#TC2a. Every default reproduces today's behavior exactly, so a tree +-- with no settings written and no profiles registered behaves as before. +pmacs.config.define { + name = "terminal.default-profile", + type = "string", + default = "", + allow_empty = true, + mutability = "live", + description = "Profile name from pmacs.terminal.profiles to open by default. " .. + "Empty means no profile: fall back to $SHELL.", +} + +pmacs.config.define { + name = "terminal.scrollback-rows", + type = "integer", + default = 10000, + min = 0, + max = 4000000, + mutability = "live", + description = "Rows of scrollback retained per terminal. " .. + "0 retains no history.", +} + +pmacs.config.define { + name = "terminal.escape-key", + type = "string", + default = "C-c", + mutability = "live", + description = "Chord that escapes to the editor from a terminal. " .. + "Pressing it twice sends the chord itself to the child.", +} + local function bind_terminal_keys(buffer) local function bind(sequence, command) pmacs.keymap.bind { @@ -19,22 +51,145 @@ local function bind_terminal_keys(buffer) bind("M->", "terminal.scroll-bottom") end +-- Q#TC1: profiles are a raw Lua table, not a config setting. The +-- registry stores four scalars and has no table kind, so a profile — +-- inherently `{ command, args, cwd, env }` — lives here beside +-- `pmacs.lsp.config` and `pmacs.pair.sets` until table-valued settings +-- exist. +terminal.profiles = terminal.profiles or {} + +local PROFILE_FIELDS = { + command = "string", + args = "table", + cwd = "string", + 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 %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 %s: unknown field %q", shown, tostring(key)), 0) + end + if type(value) ~= expected then + error(string.format( + "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] = tostring(name) end + table.sort(names) + return names +end + +-- Q#TC2 / Q#TC3a: resolve a profile by name, or nil when none is +-- selected. An explicitly requested profile that does not exist is an +-- error even when `terminal.default-profile` is valid — a typo must not +-- silently fall back to the default. +local function resolve_profile(requested) + local name = requested + if name == nil then + local configured = pmacs.config.get("terminal.default-profile") + if configured == nil or configured == "" then return nil end + name = configured + end + local profile = terminal.profiles[name] + if profile == nil then + local known = known_profile_names() + local listed = #known > 0 and table.concat(known, ", ") or "(none defined)" + error(string.format( + "terminal profile %s is not defined; known profiles: %s", + describe_name(name), listed), 0) + end + return validate_profile(name, profile) +end + +-- Q#TC3a merge order, per field: explicit open field, then the profile's +-- field, then the scalar setting, then the built-in fallback. `env` is +-- the one field where "first wins" would be wrong, so it MERGES with +-- explicit entries overriding the profile's — any other reading silently +-- drops half a user's environment. +local function merge_env(profile_env, explicit_env) + if profile_env == nil then return explicit_env end + local merged = {} + for key, value in pairs(profile_env) do merged[key] = value end + for key, value in pairs(explicit_env or {}) do merged[key] = value end + return merged +end + function terminal.open(spec) - local buffer = raw_open(spec) + spec = spec or {} + local resolved = {} + for key, value in pairs(spec) do + if key ~= "profile" then resolved[key] = value end + end + + local profile = resolve_profile(spec.profile) + if profile then + for key in pairs(PROFILE_FIELDS) do + if key ~= "env" and resolved[key] == nil then resolved[key] = profile[key] end + end + resolved.env = merge_env(profile.env, spec.env) + end + + -- The two open-time settings resolve through the GLOBAL chain + -- (Q#TC2b): they are read before the identity buffer exists, so there + -- is no terminal to resolve a buffer-local against. + if resolved.scrollback_rows == nil then + resolved.scrollback_rows = pmacs.config.get("terminal.scrollback-rows") + end + if resolved.command == nil then + resolved.command = os.getenv("SHELL") or "/bin/sh" + end + + local buffer = raw_open(resolved) bind_terminal_keys(buffer) return buffer end pmacs.command.define { name = "terminal", - description = "Open a terminal running $SHELL (or /bin/sh).", - fn = function() - return terminal.open { - command = os.getenv("SHELL") or "/bin/sh", - } + description = "Open a terminal running the configured profile, or $SHELL.", + fn = function(profile) + return terminal.open { profile = profile } end, } +-- Q#TC10: the opening binding. `COHERENCE.md` Priority 1 names a +-- terminal keybinding as part of protecting the golden journey, and §2 +-- step 8 grades the terminal "works but undiscoverable". `C-c` is +-- already a live global prefix (fold's `C-c @ ...`), so this is a new +-- leaf under it rather than a shadow. +-- +-- Named limitation: unreachable from INSIDE a terminal window, where +-- `C-c` is consumed as the escape. `M-x terminal` still works there. +pmacs.keymap.bind { scope = "global", sequence = "C-c t", command = "terminal" } + pmacs.command.define { name = "terminal.copy-selection", description = "Copy the active terminal selection.", diff --git a/docs/active-work.md b/docs/active-work.md index b961794..98727e5 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -389,14 +389,110 @@ If it does not, stop and repair the remote/fetch configuration. **isolated-config workspace sweep 3,177 across 92 suites, zero failures**; `git diff --check` clean. Gates were run against the committed tree. +## Terminal config + copy mode arc — Stage 1 IN REVIEW + +- Approved framing: `docs/terminal-config-and-copy-mode-framing.md` + **revision 4** (four review rounds), committed as the first commit of + 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` + and merged up to `c93f9ee` 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`. +- Load-bearing decisions, each forced by scouted ground truth: + - profiles are a **raw Lua table** — `ConfigValue` is four scalars with + no table kind, so they join `pmacs.lsp.config` / `pmacs.pair.sets`; + - the **two open-time settings resolve through the global chain**, + because they are read before the identity buffer exists; only + `terminal.escape-key` resolves per buffer; + - the escape cache lives on **`TerminalSession`** so its lifetime is + the terminal's. `value_epoch` alone is not a sufficient key: it does + not advance when focus moves between terminals with different + buffer-local values; + - repeating the escape sends **that chord**, not a hardcoded `0x03`. +- **Four bites, each against a different plausible wrong + implementation** — hardcoded ETX fails acc6/9; epoch-only cache key + fails acc7; single last-entry cache fails acc8's parse count; removing + the invalid-value fallback fails acc10. The first version of acc7 + passed against the epoch-only bite because it asserted only that + terminal A still worked; the discriminating assertion is that **each** + terminal honors its own chord and not the other's. +- Test instruments worth reusing: `cat -v` is the echo probe, because the + screen rejects C0 controls before they reach cells so a raw echoed + `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. +- **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` @ `c93f9ee`: `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 + framing MERGED; Stage 2A IN REVIEW Stage 1 and the Stage 2 framing are on `main`. **Stage 2A is implemented and in review.** - **Stage 2A — portable branch `githubsucks/bottom-panel-stage2a`**, - worktree `../pmacs-bp-stage2a`, based on `githubsucks/main` @ - `c93f9ee`. Two commits: the classified census routing, then the + worktree `../pmacs-bp-stage2a`, **canonical `main` @ `cf54270` + integrated** (review round 1, finding 4 — the terminal-config lane + #173 also changes `src/editor.rs`, so gates were rerun on the merge + result, not the old combination). Two commits: the classified census routing, then the painter extraction + acceptance. **No protocol change; no behavior change for any frontend today** — with `panel_capable = false` for semantic sessions, `primary_document_window` returns `view.active` @@ -417,6 +513,17 @@ implemented and in review.** catch the first bite — it compares the two authorities directly, so only a consumer-level assertion catches a misrouted consumer. Keep both kinds. +- **Review round 1 closed: 4 P1 + 2 P2, all real.** The P1s were a + stale-`Pointer` focus steal (the failed-alignment arm returned the + window, so #8's activation focused it before `dispatch_pointer` + rejected the buffer), the missing A2A-2 two-context fan-out, a census + suite that asserted the AUTHORITY rather than the CONSUMERS, and the + missing main integration. **Two of the new pins were themselves + vacuous on the first attempt** — the dispatcher test passed because an + unregistered session is dropped at `daemon.rs:1962` before reaching + the aligner, and the painter test was a fixed-point check that + survived deleting `text_view.render`. Both now fail under their own + bite. - **`vterm_stage3_acceptance::a37` is a pre-existing flake here**, not a Stage 2A regression: measured **6/8 failures on the base commit** and **7/8 on the branch** in matched isolated samples. It needs a real diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md new file mode 100644 index 0000000..48b75d8 --- /dev/null +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -0,0 +1,658 @@ +# Terminal configuration and copy mode + +**Revision 4 — scouted against canonical `main` @ `b889873` (protocol v20), +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 +satisfied its acceptance while behaving differently on A→B→A. It also corrects +the read-only deferral, which understated the substrate required: the bypass +path is `ensure_writable`-guarded too, so genuine immutability alone would +break every generated buffer that refreshes. + +Revision 3 corrects two design errors and decides the chords. The +round-trip failure shape in revision 2 was **wrong in the reporter's favour**: +a Lua intercept does not set `Buffer::read_only`, and there is no Lua binding +that does, so an optimistic `CrdtOp` bypasses the intercept *and* passes +`ensure_writable()` — the daemon buffer mutates too, rather than the mirror +diverging alone (Q#TC6a). Revision 2 also had all three settings resolving +against the terminal identity buffer, which is impossible for the two read +*before* that buffer exists (Q#TC2b). Chords are now decided and +collision-scouted rather than deferred to implementation (Q#TC10, Q#TC8a). + +Revision 2 answered seven review findings. Four were load-bearing: the settings +are `Live`, so the registry **accepts buffer-local overrides whether or not we +want them**, and `value_epoch()` does not move on a buffer switch — an +epoch-only cache can serve the wrong terminal's escape chord (Q#TC4); the +double-escape byte is a hardcoded `0x03`, so a configured escape would still +send Ctrl-C and make its own literal chord unreachable (Q#TC4b); the snapshot +buffer needs `set_round_trip_input`, not only a read-only intercept, or a +semantic frontend can optimistically edit it before daemon dispatch (Q#TC6); +and the two stages must be two branches and two PRs. Revision 1's +materialized-copy reframe is unchanged. + +Two stages, one arc, no protocol change: + +- **Stage 1 — configuration.** Terminal profiles, scrollback, and the escape + key become configurable. Today the terminal has **zero** configuration + surface: the `terminal` command hardcodes `os.getenv("SHELL") or "/bin/sh"`, + `scrollback_rows` is a per-open argument only, and the escape chord is a + literal in Rust. +- **Stage 2 — copy mode and search over scrollback.** A command that turns + the retained terminal screen and scrollback into an ordinary buffer, where + isearch, motion, selection, and the kill ring already work. + +Explicitly **not** in this arc: the panel terminal (blocked on bottom-panel +Stage 2), and shell integration (cwd tracking, prompt marks, command zones) — +the keystone that unlocks the VS Code-style cluster, which needs its own +security framing because it decides what a child process may make the editor +do. + +## Branch and PR plan + +**Two branches, two PRs.** Configuration and copy mode are independently +releasable and have no dependency on each other; one framing covers the arc, +but the one-feature/one-branch/one-PR rule governs the implementation. + +1. `terminal-config` — Stage 1. Also carries the **terminal opening + keybinding** (Q#TC10). +2. `terminal-copy-mode` — Stage 2, branched off `main` after Stage 1 merges. + +Sequencing is not a dependency but avoids a conflict: both stages edit +`builtin/runtime/terminal.lua`. + +## Ground truth (measured, not recalled) + +Three facts constrain the design, and two of them rule out the obvious plan. + +### 1. Terminal profiles cannot be a config-registry setting + +`ConfigValue` is **four scalars** — `Bool`, `Int`, `Num`, `Str` +(`src/config_registry.rs:312`) — and its own doc comment says they "are never +stored --- only these four scalars (Q#CR3)". `ConfigKind` adds `Enum`, which +is physically a string validated against choices fixed at `define` time +(`src/config_registry.rs:115-145`). There is no table, list, or map kind. + +A terminal profile is inherently a table: `{ command, args, cwd, env }` per +name. **Table-valued settings are an existing named deferral of the config +registry arc** — the same gap that keeps `pmacs.lsp.config`, +`pmacs.pair.sets`, `pmacs.comment.strings`, and the `pmacs.parse.*` proxies as +raw Lua. Profiles join that list rather than forcing that deferral open here. + +### 2. Search cannot reuse isearch in place over a terminal + +`SearchStore::set(buffer_id, query, matches: Vec)` +(`src/search.rs:99`) keys matches by buffer and addresses them as **byte +ranges into that buffer's rope**; the painting path materializes the source +with `buf.snapshot_rope().slice(0, buf.len(), ..)` (`src/search.rs:435`). + +A terminal identity buffer is **empty and read-only** by construction. Its +content lives in `TerminalScreen` as cells addressed by `(row, col)` across +history plus visible rows — there are no rope bytes to range over. Searching a +terminal in place therefore means a second, parallel search facility with its +own match store and its own highlight path, because terminal painting consumes +owned cells and not document style spans. + +### 3. An in-place copy mode would be the seventh dispatch shadow + +`dispatch_key`'s terminal-transport arm intercepts **every** key before +ordinary keymap dispatch whenever `active_terminal_key` is `Some`, which keys +purely on `is_terminal(window.buffer_id)` (`src/editor.rs:1098-1107`, +`973-1016`). A mode that keeps the terminal buffer focused while rebinding +keys to motion/selection must therefore add a new precedence rung. + +`COHERENCE.md` §6 grades that ladder **weak, "and growing by one island per +modal feature"**, records that **no transient-keymap mechanism exists to +migrate to** (`KeymapStack` has exactly three fixed scopes, no layer stack, no +push/pop, no lifetime), and notes that `describe-key` already lies while a +shadow is active. It also names the counter-example: the entire picker/panel +family uses ordinary **buffer-local keymaps** and is inspectable and +rebindable. + +### 4. What already exists and is reusable + +- `retained_rows(projection)` (`src/terminal/view.rs:539`) iterates history + plus visible rows; `copy_selection_bytes(rows, selection)` + (`src/terminal/view.rs:849`) serializes a range with the fidelity Stage 2 + criterion 21 already pins — soft wraps joined, hard rows separated, trailing + default blanks trimmed, wide glyphs and combining clusters copied once. +- `ConfigRegistry::value_epoch()` (`src/config_registry.rs:1127`) is public and + monotonic — cheap invalidation for a hot-path cache. +- The Lua surface is `define` / `get` / `set` / `set_local` / `on_change` with + a disposable handle (`src/lua_bindings/config.rs`). +- `pmacs.terminal.open` already accepts + `command, args, cwd, env, name, rows, cols, scrollback_rows, display, + window`. **`display = "panel"` already works** (bottom-panel Stage 1) — the + panel terminal is blocked on rendering, not on this surface. +- Terminal buffers already carry buffer-local bindings (`M-w`, `M-v`, `C-v`, + `M-<`, `M->`) installed by `terminal.open` in `builtin/runtime/terminal.lua`. + +## Stage 1 — configuration + +**Q#TC1 — Profiles are a raw Lua table, not a setting.** +`pmacs.terminal.profiles` maps a name to a spec table, exactly following the +`pmacs.lsp.config` precedent. The registry holds only scalars. Rejected +alternative: widening `ConfigValue` with a table kind — that is the config +arc's own named deferral, it is cross-cutting (persistence, `describe-setting` +rendering, the `custom-file` question all key on the scalar assumption), and +smuggling it into a terminal PR would be the wrong place to decide it. + +**Q#TC2 — `terminal.default-profile` is `String`, not `Enum`.** `Enum` +choices are frozen at `define` time; profiles are user-extensible from +`init.lua` and later. Validation happens at open time, and an unknown name +must produce a pointed error that **names the known profiles**, not a bare +"unknown profile". + +**Q#TC2a — the exact settings, defaults, and bounds.** All three are `Live` +(see Q#TC2b), and every default reproduces today's behavior exactly, so a tree +with no settings written behaves identically (acceptance 12). + +| name | kind | default | bounds | +|---|---|---|---| +| `terminal.default-profile` | `String { allow_empty: true }` | `""` | — | +| `terminal.scrollback-rows` | `Integer` | `10_000` (`DEFAULT_TERMINAL_SCROLLBACK_ROWS`) | `0 ..= 4_000_000` (`MAX_TERMINAL_HISTORY_CELLS`) | +| `terminal.escape-key` | `String { allow_empty: false }` | `"C-c"` | parsed as a chord | + +**Zero is a legal scrollback value meaning "retain no history".** The core's +own validation rejects only values *above* `MAX_TERMINAL_HISTORY_CELLS` +(`src/terminal/session.rs:114`), so `scrollback_rows = 0` is accepted through +`terminal.open` today. A `1` minimum here would invent an asymmetry between the +setting and the per-open field for no reason. + +`""` is the **"no default profile" sentinel**: an empty string means "fall +through to `$SHELL`", not "a profile named empty". `allow_empty: true` exists +precisely to express it, and the open path treats empty and unset identically. + +**Q#TC2b — the settings are `Live`, and the registry therefore accepts +buffer-local overrides. That is specified rather than accidental.** +`ConfigRegistry::set_local` refuses only `StartupOnly` definitions +(`src/config_registry.rs:949`); a `Live` setting can be pinned per buffer by +anyone. Declaring these global-only is **not currently expressible** — a +`scope = "global"` define flag is one of the config registry's own named +deferrals, and `autosave.interval-ms` already has the same latent problem. + +Making them `StartupOnly` instead would buy enforcement at the cost of the +feature: the escape key could never be changed mid-session, which kills Q#TC4's +whole point. So they stay `Live`, and resolution is defined **per setting, +because the three are not read at the same moment**: + +| setting | read when | resolution | +|---|---|---| +| `terminal.escape-key` | every keystroke in a terminal (cached) | `get(name, terminal_buffer)` — **buffer-local → global → default** | +| `terminal.default-profile` | once, **before** the terminal exists | `get(name)` — **global chain only** | +| `terminal.scrollback-rows` | once, **before** the terminal exists | `get(name)` — **global chain only** | + +The split is forced, not stylistic. The two open-time settings are consumed by +`_open` **before it creates the identity buffer**, so there is no terminal +buffer to resolve against — and no caller could have pinned a local override on +a buffer that does not yet exist. `pmacs.config.get(name)` with no buffer +argument already means exactly "the global chain, never an ambient buffer", so +this is the registry's existing semantic rather than a new rule. + +Consequences, stated so they are not discovered later: + +- a per-terminal escape key is a supported feature, not a bug; +- `set_local` on `terminal.default-profile` or `terminal.scrollback-rows` is + **always inert**, for any buffer, because the open path never consults a + buffer chain. This is deliberate; the alternative — resolving against + whichever buffer happened to be current at open time — would make a + terminal's scrollback depend on what the user was looking at when they + pressed the key. + +Rejected alternative: resolving the open-time settings against the *target +window's pre-open buffer*. It is expressible, but it makes an ambient buffer +load-bearing for a value the user set globally, which is the trap +`pmacs.config`'s two-argument/one-argument split exists to avoid. + +**Q#TC3 — `terminal.scrollback-rows` is `Integer` with bounds, and an explicit +per-open `scrollback_rows` still wins.** The precedence is +**explicit argument over global setting** — there is no ambient buffer in this +chain at all (Q#TC2b resolves it through `get(name)`), so the rule is simply +that what a caller passes to `terminal.open` beats what the user configured +globally. The bounds above come from the existing validation, so the setting +cannot express a value the core will reject. + +**Q#TC3a — profile resolution order, field by field.** `profile` is accepted +by **`pmacs.terminal.open` as well as the command**, so a Lua caller is not +forced through the command to use one. For each field, the first source that +supplies it wins: + +1. an explicit `pmacs.terminal.open` field; +2. the named profile's field — `profile` argument, else + `terminal.default-profile` when non-empty; +3. the scalar setting, where one exists (`scrollback_rows` only); +4. the built-in fallback (`command` = `$SHELL`, else `/bin/sh`). + +`env` is the one field where "first wins" is ambiguous, so it is stated: +profile `env` and explicit `env` are **merged**, with explicit entries +overriding profile entries of the same name. Any other reading silently drops +half a user's environment. + +An explicitly passed `profile` that does not exist is an error even when +`terminal.default-profile` is valid — a typo must not silently fall back to +the default. + +**Q#TC4 — `terminal.escape-key` is a `String` chord spelling, parsed once and +cached by `(buffer_id, value_epoch)`.** `is_terminal_escape_chord` +(`src/editor.rs:4413`) currently compares against a literal `C-c`. Reading and +parsing a setting on **every keystroke in a terminal** is not acceptable in +that path. + +**The cache key must include the buffer.** `value_epoch()` advances only on +`set` / `set_local` / removal (`src/config_registry.rs:918`, `970`, `1011`, +`1029`) — **it does not move when the focused terminal changes**. An +epoch-only cache therefore serves terminal A's escape chord to terminal B for +as long as no setting is written, which is exactly the case where nothing looks +wrong. Keying on `(buffer_id, value_epoch)` is the minimum correct identity. + +**Q#TC4c — the cache lives on `TerminalSession`, so its lifecycle is the +terminal's.** Revision 3 named the key `(buffer_id, value_epoch)` but not the +storage, and the two obvious storages behave differently on A→B→A: + +- a **single last-entry cache** reparses on every switch between two + terminals, and re-reports an invalid value each time — a status line that + scolds you for a setting you already know about, forever; +- an **editor-side map** preserves "parsed and reported once" but **leaks an + entry per terminal** unless something purges it, and that purge is a second + thing to get wrong. + +`TerminalSession` (`src/terminal/session.rs:215`) is created in +`TerminalManager::open` and dropped on kill/prune, so putting the cache there +gets the lifecycle for free with no purge hook to forget. It carries the parsed +chord, the `value_epoch` it was parsed at, and whether the current invalid +value has already been reported. + +**"Reports once" means once per terminal, per effective invalid value.** +A→B→A must not re-report. Changing the setting from one invalid value to a +*different* invalid value **does** re-report, because that is new information +about a new mistake. + +**The reporting channel is `EditorCore::status`** — the same channel +`send_terminal_bytes` already uses for terminal failures +(`src/editor.rs:1122`). Explicitly **not** `pmacs.error`: it is not installed +as a module anywhere in `src/lua_bindings`, so its call sites across the +runtime are dead, and a report sent there would be a report nobody sees. + +**Q#TC4a — an unparseable escape key must not brick terminal input.** A bad +value falls back to `C-c` and reports once. The failure mode this avoids is +severe: with no escape chord, every key goes to the child and the user cannot +reach any editor binding to fix the setting that broke it. + +**Q#TC4b — repeating the configured escape sends THAT chord to the child, not +Ctrl-C.** The double-escape arm currently writes a hardcoded +`&[0x03]` (`src/editor.rs:988`). With `terminal.escape-key = "C-x"`, `C-x C-x` +would send Ctrl-C — and literal Ctrl-X would become unreachable, since the +first `C-x` is always consumed as the escape. The repeat arm must encode the +**configured** chord through the existing `crate::terminal::input::encode_key` +path, which is also how it inherits application-cursor and modifier handling +rather than growing a second encoder. + +Corollary worth pinning: after changing the escape away from `C-c`, an ordinary +`C-c` must reach the child as `0x03` like any other unescaped key. + +**Q#TC5 — the `terminal` command gains an optional profile argument** and +otherwise keeps its current behavior; `$SHELL` remains the fallback when no +profile is configured. No existing invocation changes meaning. + +**Q#TC10 — the terminal opening keybinding is pulled forward into Stage 1.** +`COHERENCE.md` Priority 1 names "a terminal keybinding" as part of protecting +the golden journey, §2 step 8 grades the terminal "works but undiscoverable", +and this stage already edits `terminal.lua`. Panel rendering imposes no +dependency on binding a command that already exists. Close/kill semantics stay +with the panel work, where the entry and exit points get designed together. + +The chord is **decided and scouted, not deferred**: `C-c t`, global. See +Q#TC8a for the collision evidence and for why binding under the existing `C-c` +prefix is a new leaf rather than a shadow. + +## Stage 2 — copy mode and search + +**Q#TC6 — copy mode MATERIALIZES into an ordinary buffer. It does not add a +dispatch shadow.** + +`M-x terminal.copy-mode` snapshots the retained rows into a read-only, +path-less buffer (`*terminal-copy: NAME*`) and displays it. That buffer is an +ordinary document buffer, so: + +- **isearch works, with no new search substrate** — it is a rope, so + `SearchStore` and the existing match-painting path apply unchanged. Ground + truth 2 is answered by not fighting it. +- **motion, selection, `M-w`, the kill ring, even `M-x occur`-style consumers + work** — everything that operates on a buffer. +- **The "keys must not reach the child" problem dissolves structurally.** + `active_terminal_key` keys on `is_terminal(window.buffer_id)`; the snapshot + buffer is not a terminal, so the transport arm never fires. No new guard, no + new precedence rung, and ground truth 3's coherence cost is avoided rather + than paid. +- **`describe-key` stays truthful**, because the bindings are buffer-local and + inspectable — the idiom `COHERENCE.md` §6 identifies as the right side of + the line. + +**Q#TC6a — the snapshot is BOTH intercept-read-only AND round-trip-marked, +and `set_round_trip_input` is the ONLY thing standing between a replica +frontend and unauthorized mutation.** + +The established idiom is two calls: `listview.lua:106` and `compile.lua:272` +each pair `pmacs.buffer.add_intercept` with +`pmacs.buffer.set_round_trip_input(buf, true)`. Revision 2 described the +intercept as the guard and round-trip as defence in depth. **That was wrong, +and the correction matters:** + +- A Lua intercept guards the **dispatch/edit** path only. It does **not** set + `Buffer::read_only`, which is "deliberately independent of edit intercepts" + (`src/buffer.rs:493-500`) — that flag is what makes terminal identity buffers + reject rope, undo/redo, and remote-CRDT mutation alike. +- **No Lua binding sets `read_only` at all.** The whole `src/lua_bindings` + tree only ever *reads* it (`fold.rs:313`). A Lua-created "read-only" buffer + is therefore read-only against dispatch and nothing else. +- So an optimistic `CrdtOp` from a semantic frontend bypasses the intercept + **and passes `ensure_writable()`**. It is applied. The daemon buffer mutates + in lockstep with the mirror — the user silently edits a buffer the editor + told them is read-only. There is no divergence to notice, which is worse + than divergence. + +`set_round_trip_input` prevents this at the only point it can be prevented: it +makes `dispatch_idle_for` report false while the buffer is focused, so the +frontend never applies optimistically and never emits the op. It is not +hardening — it is the guard. + +Two things follow, and both are recorded rather than fixed here: + +- **The same exposure exists today** for every Lua-created read-only buffer — + listview panels and `*compilation*` included. They are correct only because + they call `set_round_trip_input`. This arc must not be the place that + unilaterally changes that substrate. +- **Exposing `Buffer::set_read_only` to Lua** would make these buffers + genuinely immutable at the rope/CRDT boundary the way terminal identity + buffers are, turning round-trip back into real defence in depth. That is a + substrate change affecting listview and compile as much as this snapshot, so + it is named in Deferred with its own lane. + +**Q#TC7 — the materializer reuses the existing serializer.** A whole-range +variant of `copy_selection_bytes` over `retained_rows` inherits the criterion +21 fidelity rather than re-deriving soft-wrap, wide-glyph, and trailing-blank +behavior. Writing a second serializer would guarantee the two drift. + +**Q#TC8 — one snapshot buffer per terminal, reused on re-invoke.** Re-running +the command against the same terminal replaces the contents in place rather +than accumulating buffers. It is killed with its terminal; killing the +snapshot alone leaves the terminal untouched. + +**Q#TC8a — the chords, decided and collision-scouted.** + +Worth stating first because it is easy to get backwards: in a terminal window +every **unescaped** key goes to the child, so terminal-local bindings are +reached as ` `. The existing `M-w` copy is physically `C-c M-w`. +The escape consumes itself and the next key starts a fresh ordinary sequence, +which is also why `C-c`-leading bindings are structurally unreachable *inside* +a terminal. + +| action | scope | binding | physically typed | +|---|---|---|---| +| open a terminal (Q#TC10) | global | `C-c t` | `C-c t` | +| enter copy mode | terminal buffer | `C-t` | `C-c C-t` | +| refresh snapshot | snapshot buffer | `g` | `g` | +| return to terminal | snapshot buffer | `q` | `q` | + +Scouted against the real keymaps: + +- **`C-c t` is free.** No bare global `C-c` binding exists; `C-c` is already a + live global prefix from `fold.lua:48-52` (`C-c @ …`), and `C-c C-k` is + buffer-scoped in compile/async. `C-c t` is a new leaf under an existing + prefix, not a shadow. +- **`C-t` is globally `edit.transpose-chars`** (`editops.lua:909`), and binding + it **buffer-locally is legitimate**: `keymap.bind`'s strictness rejects + binding a *prefix* of an existing sequence within a scope + (`keymap_bind_conflict_surfaces_at_bind_time` — "would shadow"), not + cross-scope shadowing, which is what scopes are for. Listview already binds + `n`/`p`/`g`/`q`/`RET`/`SPC` buffer-locally. Transpose-chars is meaningless in + a read-only terminal buffer. +- `C-c C-t` matches emacs-libvterm's own `vterm-copy-mode` chord, so the muscle + memory transfers. +- `g` / `q` in the snapshot follow listview's precedent exactly. + +**Named limitation:** `C-c t` cannot open a terminal *from inside* a terminal, +because `C-c` is consumed as the escape there. `M-x terminal` still works. This +is the documented consequence of Stage 2 criterion 19, not a new defect. + +These are what make acceptance 21's `describe-key` claim testable: named +bindings, in named buffers, that introspection must report truthfully. + +**Q#TC9 — the live-terminal keys stay.** `M-w`, `M-v`, `C-v`, `M-<`, `M->` on +the terminal buffer are the live affordances and do not change. Copy mode is +additive, on its own binding, and does not replace scroll-and-select. + +## Bets + +- **B1.** Materializing gives search for free: no second match store, no + second highlight path, no terminal-specific search UI. *Scored by Stage 2 + landing with zero changes under `src/search.rs`.* +- **B2.** Point-in-time is sufficient for read-back/search/copy. *Scored by + use; if false, the live frozen mode in Deferred becomes the real feature and + this becomes its snapshot fallback.* +- **B3.** No protocol change. The snapshot is an ordinary buffer, so both + frontends render it with existing machinery. *Scored by the diff.* +- **B4.** The escape-key cache keyed by `(buffer_id, value_epoch)` never + becomes stale in a way a user can observe. *Scored by two acceptances, not + one: changing the setting mid-session (8) and two terminals with different + buffer-local values and no write between them (7). Revision 1's epoch-only + cache would pass the first and fail the second, which is why the bet now + names both.* +- **B5.** Buffer-local escape keys are a feature rather than a hazard. + *Unscored and honestly so: the registry cannot express global-only, so this + is what we get either way. If per-terminal escapes turn out to confuse more + than they help, the fix is the config registry's `scope = "global"` deferral, + not a terminal change.* + +## Deferred (named) + +- **Live frozen copy mode** (true `vterm-copy-mode` semantics: freeze the + terminal in place, navigate it, resume). Strictly larger; needs either the + transient-keymap primitive `COHERENCE.md` §6 specifies or a deliberate + seventh shadow. +- **Shell integration** — cwd tracking, prompt marks, command zones, and the + VS Code cluster downstream of it (command decorations, exit-code markers, + rerun, sticky scroll, terminal IntelliSense). Its own arc, with a security + framing. +- **Table-valued settings** — the config registry's own deferral. This arc + adds a **second** blocked adopter (after `pmacs.lsp.config` / + `pmacs.pair.sets`); worth recording as evidence when that deferral is + ranked. +- **A `scope = "global"` define flag** — also the config registry's own + deferral, and this arc is its second live case after `autosave.interval-ms`. + Until it exists, `set_local` on any `Live` setting is accepted whether or not + the owner wants it, so Q#TC2b specifies the behavior instead of pretending + it is prevented. +- **Panel terminal** — blocked on bottom-panel Stage 2 (semantic frontends are + not `panel_capable`). `display = "panel"` already exists and works on the + grid frontend. +- OSC 8 hyperlinks, images (sixel/kitty), `faint`/`blink`/`conceal`/ + `strikethrough` (needs a shared `Style` widening, so a protocol bump), + cursor shape/blink, kitty keyboard protocol. +- Terminal session persistence/reconnect across editor restart. +- **A terminal close/kill command** — the remaining half of `COHERENCE.md` + §2 step 8's discoverability gap. It belongs with the panel-terminal work, + where entry and exit points get designed together. The *opening* keybinding + is **no longer deferred**: Stage 1 carries it as Q#TC10. +- **Genuine immutability for generated buffers — and it is bigger than a Lua + setter.** Today no Lua binding sets `read_only` (`src/lua_bindings` only + reads it, `fold.rs:313`), so every Lua-created "read-only" buffer — listview + panels, `*compilation*`, and this snapshot — is read-only against dispatch + alone and relies entirely on `set_round_trip_input` (Q#TC6a). + + Merely **exposing `set_read_only` would break all three.** The + intercept-bypass path is `ensure_writable`-guarded too: + `apply_edit_skip_intercepts` calls it first (`src/buffer.rs:994`), and that + is exactly the primitive an owner uses to rewrite its own generated buffer. + Flipping the flag would stop listview refreshing, `*compilation*` streaming, + and this snapshot refreshing — the very operations those buffers exist for. + + So the lane needs **two** things, not one: genuine immutability at the + rope/CRDT boundary, *and* an owner-authorized update path that is not simply + "skip the intercepts". Naming only the setter would have made it look like a + one-line follow-up. + +## Acceptance + +### Stage 1 — `terminal-config` + +1. `pmacs.terminal.profiles` accepts a strict spec table per name and rejects + unknown fields before anything is spawned, matching `terminal.open`'s + existing transactional contract. +2. `terminal.default-profile` naming an unknown profile fails at open with an + 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. +4. `""` in `terminal.default-profile` means "no profile" and is + indistinguishable from unset (Q#TC2a). +5. `terminal.scrollback-rows` takes effect for a terminal opened without an + explicit `scrollback_rows`; an explicit per-open value overrides it; values + outside `0 ..= 4_000_000` are rejected by the registry rather than by the + core, and `0` is accepted as "retain no history". +6. `terminal.escape-key` changes which chord escapes to the editor, observed + through the **real dispatch path**, not by calling the predicate directly. +7. **Two terminals with different buffer-local escape keys each honor their + own**, with no setting written in between (Q#TC4/Q#TC2b). Driven as + **A→B→A**, asserting both directions. This is the pin an epoch-only cache + fails. +8. Across that same **A→B→A** switch with no setting written, the parse count + does **not** increase after each terminal's first keystroke (Q#TC4c) — + pinned by counting parses, not by timing. This is the pin a single + last-entry cache fails while still satisfying 7. +8a. A terminal's cache does not outlive it: killing a terminal and opening a + new one does not serve the dead terminal's chord, and no per-terminal cache + entry survives its session (Q#TC4c). This is the pin an unpurged + editor-side map fails. +9. With `terminal.escape-key = "C-x"`: `C-x C-x` sends **Ctrl-X** to the child, + and an ordinary `C-c` reaches the child as `0x03` like any other unescaped + key (Q#TC4b). Bite: against the hardcoded `&[0x03]`, the first assertion + fails. +10. An unparseable `terminal.escape-key` falls back to `C-c`, reports through + `EditorCore::status`, and leaves the terminal usable (Q#TC4a). Bite: with + the fallback removed, the terminal becomes unescapable. +10a. "Reports once" is once per terminal per effective invalid value + (Q#TC4c): an **A→B→A** switch with the same invalid value reports **once**, + while changing it to a *different* invalid value reports again. The report + count is asserted, not the message text. +11. The terminal opening keybinding invokes the existing command, and is + verified to have shadowed nothing (Q#TC10). +12. Existing `terminal` invocations and every existing terminal test behave + identically with no settings defined and no profiles registered. + +### Stage 2 — `terminal-copy-mode` + +13. `terminal.copy-mode` produces a read-only buffer whose text is + byte-identical to serializing the full retained range through the existing + copy path (Q#TC7) — pinned against the serializer, so the two cannot drift. +14. Soft wraps, hard rows, wide glyphs, combining clusters, and trailing + default blanks appear in the snapshot exactly as Stage 2 criterion 21 pins + them for selection copy. +15. isearch over the snapshot finds content that is **only in scrollback** + (scrolled off the visible screen), with no change to `src/search.rs` (B1). +16. **Ungated, runs in CI:** focusing the snapshot buffer makes + `dispatch_idle_for` report **false**. This is the whole mechanism Q#TC6a + depends on, it needs no CRDT, and it fails the moment + `set_round_trip_input` is dropped — so the load-bearing regression is + caught by the default configuration rather than only by a `crdt`-gated + test that CI never compiles. +17. **Through a semantic frontend** (this one does need CRDT): keys typed in + the snapshot buffer reach ordinary dispatch and never the child, and + **neither the daemon buffer nor the frontend's mirror is mutated** + (Q#TC6a). Bite: with `set_round_trip_input` removed, the optimistic op is + emitted, bypasses the Lua intercept, passes `ensure_writable()`, and + mutates **both sides** — a buffer the editor calls read-only silently + accepts an edit. +18. Re-invoking against the same terminal refreshes in place; the buffer count + does not grow (Q#TC8). Killing the snapshot leaves the terminal running; + killing the terminal removes the snapshot. +19. `C-t` in a terminal buffer (physically `C-c C-t`) enters copy mode; `g` + refreshes the snapshot from the live terminal and `q` returns to the source + terminal (Q#TC8a). +20. The live terminal's own keys are unchanged while a snapshot exists + (Q#TC9), and the terminal keeps following its tail. +21. The dispatch-shadow count is **unchanged at six** — pinned by asserting + `describe-key` reports the truth for the snapshot buffer's `g` and `q`, + which is the observable difference between the buffer-local idiom and a + shadow. + +## Coherence impact (`COHERENCE.md` §20) + +- **§6 Interaction islands — this arc deliberately adds none.** It is the + first modal-feeling terminal feature that resolves to the buffer-local + keymap idiom §6 identifies as correct, rather than a seventh rung on the + precedence ladder. The shadow count stays at six and `describe-key` stays + truthful (acceptance 21). Worth recording in §6 as a worked example that the + idiom scales to a case that looks modal. +- **§11 Configuration as typed, layered data** — the terminal gains its first + settings, and produces a second blocked adopter for **two** distinct registry + deferrals: the missing table-valued kind (profiles) and the missing + `scope = "global"` flag (the **two open-time settings** — + `terminal.escape-key` deliberately supports buffer-locals, so only + `default-profile` and `scrollback-rows` want an enforcement the registry + cannot express). §11's ground truth should + record both, because the argument for prioritizing them is now cumulative + rather than hypothetical. +- **§2 golden journey, step 8 — partially closed here.** Stage 1 carries the + **terminal opening keybinding** that Priority 1 explicitly names (Q#TC10), + which is the larger half of "works but undiscoverable". Close/kill stays with + the panel work so the entry and exit points are designed together, and is + named in Deferred rather than silently skipped. +- **§5 Unify discovery** — the new commands must carry real descriptions so + M-x rows are useful; no new introspection surface is added. +- No background-work attribution change; no new activity view; no protocol + change. + +## Verification plan + +Full gate suite per `CLAUDE.md` for each PR separately, plus: + +- **The touched terminal suites in BOTH configurations** — default and + `--features crdt` — not only the CRDT one. `vterm_stage1_acceptance`, + `vterm_stage2_acceptance`, and `vterm_stage3_acceptance` all carry tests in + each, and acceptance 12 is a claim about the default configuration too. +- `cargo test --test config_registry_acceptance` for the new settings. +- New suites: `tests/terminal_config_acceptance.rs` (Stage 1) and + `tests/terminal_copy_mode_acceptance.rs` (Stage 2). +- Every behavioral claim bite-verified. The bites that matter most: + **7/8/8a** — three pins that fail against three *different* wrong cache + implementations (epoch-only key, single last-entry, unpurged map), which is + why one pin was not enough; **9** (a hardcoded `0x03` makes the configured + 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 + this reason. That measurement and its lane live on **PR #168**, which is open + and unmerged; it is not yet in `docs/active-work.md` on `main`. + Acceptance 17 does need a semantic frontend, so that one test is gated — but + acceptance 16 pins the same mechanism ungated, so the regression is caught in + CI regardless. That pairing is the pattern to reuse whenever a claim's + end-to-end proof needs CRDT. diff --git a/src/daemon.rs b/src/daemon.rs index 16e66ab..606839d 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -2992,7 +2992,13 @@ fn align_primary_document_window( } let reg = core.registry.borrow(); let Ok(buf) = reg.get(buffer_id) else { - return Some(win_id); // Unknown buffer — leave the window as-is. + // Unknown buffer — leave the window as-is, and report + // FAILURE. Returning the window here would let a stale or + // forged `Pointer` naming a dead buffer take focus out of a + // panel via #8's activation, *before* `dispatch_pointer` + // rejects the mismatched buffer. Alignment did not happen, + // so no caller may treat this as a document gesture. + return None; }; (win_id, TextView::new(buf)) }; @@ -4638,4 +4644,130 @@ mod tests { "the panel was not overwritten with the target" ); } + + /// Bottom-panel §1.3 #8, review round 1 finding 1: a STALE document + /// `Pointer` must not steal focus out of a panel. + /// + /// Driven through `handle_dispatcher_event` — the real dispatcher + /// seam — because the defect lived in the *pair* of alignment and + /// activation, not in either alone. `align_primary_document_window` + /// once returned the window even when the named buffer was gone, so + /// #8's activation focused the document before `dispatch_pointer` + /// ever rejected the mismatched buffer. + #[cfg(feature = "crdt")] + #[test] + fn a_stale_document_pointer_does_not_steal_focus_from_a_panel() { + use crate::editor::EditorState; + use crate::protocol::FrontendId; + use crate::window::{FrontendView, Layout, LayoutNode, Orientation, Window, WindowParams}; + use pmacs_protocol::{Modifiers, PointerKind}; + + let mut editor = EditorState::new(); + let fid = FrontendId(88); + + // One document window + one focused bottom panel. + let (document, panel, dead_buffer) = { + let mut core = editor.core.borrow_mut(); + let doc_buf = core.active_window().buffer_id; + let panel_buf = core.registry.borrow_mut().create("*panel*"); + // A buffer id that names nothing: the stale-pointer payload. + let dead_buffer = crate::buffer::BufferId::from_raw(999_999); + + let document = crate::window::WindowId::next(); + let panel_id = crate::window::WindowId::next(); + let (doc_view, panel_view) = { + let reg = core.registry.borrow(); + ( + crate::text_view::TextView::new(reg.get(doc_buf).expect("doc")), + crate::text_view::TextView::new(reg.get(panel_buf).expect("panel")), + ) + }; + core.windows + .insert(document, Window::new(document, doc_buf, doc_view)); + let mut panel = Window::new(panel_id, panel_buf, panel_view); + let mut params = WindowParams::default(); + params.side = Some(crate::window::Side::Bottom); + params.fixed_rows = Some(4); + panel.params = params; + core.windows.insert(panel_id, panel); + core.register_frontend_view( + fid, + FrontendView { + layout: Layout { + root: LayoutNode::Split { + orientation: Orientation::Horizontal, + children: vec![LayoutNode::Leaf(document), LayoutNode::Leaf(panel_id)], + weights: vec![1, 1], + }, + }, + active: panel_id, + fold_projection: false, + panel_capable: true, + frame_geometry: None, + panel_hidden: false, + }, + ); + (document, panel_id, dead_buffer) + }; + + let mut render_states = HashMap::new(); + let mut semantic_states = HashMap::new(); + semantic_states.insert(fid, crate::semantic_render::SemanticRenderState::new(fid)); + let mut streams = HashMap::new(); + let mut term_sizes = HashMap::new(); + term_sizes.insert(fid, CellSize::new(24, 80)); + let mut last_idle = HashMap::new(); + let mut last_active = HashMap::new(); + let mut bells = HashMap::new(); + // The dispatcher drops any event from an UNINSTALLED session + // (#148's defense-in-depth membership check), so the session must + // be registered or this test passes for the wrong reason — it did, + // on the first attempt. + let mut registry = SessionRegistry::new(); + registry.register_session( + fid, + crate::presence::SessionState { + negotiated_protocol_version: pmacs_protocol::PROTOCOL_VERSION, + negotiated_capabilities: crate::protocol::NegotiatedCapabilities { + semantic_render: true, + crdt_replica: true, + ..Default::default() + }, + color_slot: 0, + }, + ); + + handle_dispatcher_event( + DispatcherEvent::FrontendEvent { + source: fid, + event: FrontendEvent::Pointer { + frontend_id: fid, + buffer_id: dead_buffer, + byte: 0, + kind: PointerKind::Down, + mods: Modifiers::default(), + }, + }, + &mut editor, + &mut render_states, + &mut semantic_states, + &mut streams, + &mut term_sizes, + &mut last_idle, + &mut last_active, + &mut bells, + &mut registry, + ); + + assert_eq!( + editor.core.borrow().views[&fid].active, + panel, + "a stale Pointer naming a dead buffer must NOT move focus out of the panel" + ); + assert_ne!( + editor.core.borrow().views[&fid].active, + document, + "non-vacuity: the document window is a real, distinct focus target" + ); + } } diff --git a/src/editor.rs b/src/editor.rs index 2553775..23fa6d1 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -989,19 +989,28 @@ impl EditorState { .get(&frontend_id) .is_some_and(|state| state.terminal_escape); if let Some(view_key) = terminal_key { + // Q#TC4: the escape chord is per terminal, resolved through + // `terminal.escape-key` and cached on the session so this + // hot path parses at most once per (terminal, config epoch). + let escape_chord = self.terminal_escape_chord(view_key.buffer_id); if escaped { self.dispatchers .entry(frontend_id) .or_default() .terminal_escape = false; - if chord.is_some_and(is_terminal_escape_chord) { + if chord == Some(escape_chord) { + // Q#TC4b: repeating the escape sends THAT chord to the + // child, not a hardcoded ETX. With a configured escape + // of `C-x`, sending Ctrl-C here would both surprise the + // user and make literal Ctrl-X unreachable, since the + // first press is always consumed as the escape. self.claim_terminal_controller(view_key); - self.send_terminal_bytes(view_key.buffer_id, &[0x03]); + self.send_terminal_escape_literal(view_key, escape_chord); return; } // The post-escape key starts a fresh ordinary sequence below. } else if !dispatcher_pending { - if chord.is_some_and(is_terminal_escape_chord) { + if chord == Some(escape_chord) { let state = self.dispatchers.entry(frontend_id).or_default(); state.terminal_escape = true; state.dispatcher = KeyDispatcher::new(); @@ -1117,6 +1126,54 @@ impl EditorState { .then_some(key) } + /// This terminal's effective escape chord (Q#TC4). + /// + /// Resolution is `get("terminal.escape-key", terminal_buffer)` — + /// buffer-local, then global, then default — because unlike the two + /// open-time settings this one is read while the terminal exists, so + /// a per-terminal escape is expressible and supported (Q#TC2b). + /// + /// The parse and the once-per-terminal invalid-value report both live + /// in [`crate::terminal::TerminalManager::escape_chord`]; this method + /// only supplies the resolved spelling and the epoch that keys the + /// cache, and surfaces any report through the status line — the same + /// channel `send_terminal_bytes` uses for terminal failures. + fn terminal_escape_chord(&self, buffer_id: crate::buffer::BufferId) -> Chord { + let lua = self.lua_host.lua(); + let (spelling, epoch) = crate::lua_bindings::config_string_and_epoch( + lua, + "terminal.escape-key", + Some(buffer_id), + crate::terminal::DEFAULT_TERMINAL_ESCAPE_KEY, + ); + let (chord, report) = self + .terminal_manager + .borrow_mut() + .escape_chord(buffer_id, epoch, &spelling); + if let Some(message) = report { + self.core.borrow_mut().status = message; + } + chord + } + + /// Send the configured escape chord to the child as literal input + /// (Q#TC4b), through the same encoder ordinary keys use so it + /// inherits application-cursor and modifier handling. + fn send_terminal_escape_literal(&self, key: TerminalViewKey, chord: Chord) { + let event = KeyEvent::new(chord.code, chord.modifiers); + let Some((terminal_key, modifiers)) = terminal_key_from_crossterm(event) else { + return; + }; + let modes = self + .terminal_manager + .borrow() + .modes_for_view(key) + .unwrap_or_default(); + if let Some(bytes) = crate::terminal::input::encode_key(terminal_key, modifiers, modes) { + self.send_terminal_bytes(key.buffer_id, &bytes); + } + } + fn claim_terminal_controller(&self, key: TerminalViewKey) { let mut manager = self.terminal_manager.borrow_mut(); let _ = manager.register_view(key); @@ -4481,10 +4538,6 @@ fn sanitize_single_line(s: &str) -> String { .collect() } -fn is_terminal_escape_chord(chord: Chord) -> bool { - chord.code == KeyCode::Char('c') && chord.modifiers == KeyModifiers::CONTROL -} - fn terminal_key_from_crossterm(key: KeyEvent) -> Option<(TerminalKey, TerminalModifiers)> { let modifiers = crate::protocol::crossterm_translate::mods_from_crossterm(key.modifiers); let key = crate::protocol::crossterm_translate::keycode_from_crossterm(key.code); diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 3482879..1e7ac04 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -668,6 +668,33 @@ pub fn config_u32(lua: &Lua, name: &str, buffer_id: Option, fallback: } } +/// Read a `String` setting plus the registry epoch that keys any cache +/// built from it (Q#TC4c). +/// +/// The epoch is returned WITH the value deliberately: a caller caching a +/// parsed form needs both, and reading them in two calls would let a +/// `set` land between them and produce a cache stamped with the wrong +/// epoch. `fallback` covers a bare core whose runtime never defined the +/// setting, matching [`config_u32`]. +#[must_use] +pub fn config_string_and_epoch( + lua: &Lua, + name: &str, + buffer_id: Option, + fallback: &str, +) -> (String, u64) { + let Some(registry) = lua.app_data_ref::() else { + return (fallback.to_owned(), 0); + }; + let borrowed = registry.borrow(); + let epoch = borrowed.value_epoch(); + let value = match borrowed.get(name, buffer_id) { + Ok(crate::config_registry::ConfigValue::Str(v)) => v.clone(), + _ => fallback.to_owned(), + }; + (value, epoch) +} + /// Short-circuit a binding when the init phase has completed. /// /// Lifecycle-affecting Lua APIs (currently just `pmacs.attach`; M5.6d+) diff --git a/src/semantic_render.rs b/src/semantic_render.rs index 617d408..b63efcf 100644 --- a/src/semantic_render.rs +++ b/src/semantic_render.rs @@ -810,7 +810,11 @@ impl SemanticRenderState { out.extend(self.font_facts_msg(state)); // Q#SL6/Q#SL8: face inventory must precede segment text. if let Some(evaluation) = statusline_evaluation { - self.emit_statusline_segments(evaluation, &mut out); + let document_window = state + .core + .borrow() + .primary_document_window(self.frontend_id); + self.emit_statusline_segments(evaluation, document_window, &mut out); } out } @@ -929,7 +933,11 @@ impl SemanticRenderState { out.extend(self.font_facts_msg(state)); // Q#SL6/Q#SL8: face inventory must precede segment text. if let Some(evaluation) = statusline_evaluation { - self.emit_statusline_segments(evaluation, &mut out); + let document_window = state + .core + .borrow() + .primary_document_window(self.frontend_id); + self.emit_statusline_segments(evaluation, document_window, &mut out); } out } @@ -941,6 +949,7 @@ impl SemanticRenderState { fn emit_statusline_segments( &mut self, evaluation: StatuslineEvaluation, + document_window: Option, out: &mut Vec, ) { let to_wire = |segments: Vec| { @@ -955,10 +964,16 @@ impl SemanticRenderState { let frontend_id = self.frontend_id; match evaluation.outcome { StatuslineEvaluationOutcome::Ready(windows) => { - if let Some(window) = windows - .into_iter() - .find(|window| window.context.frontend_id == frontend_id) - { + // Bottom-panel A2A-2: the fan-out now yields the primary + // document AND the visible side window, so the wire + // segments must be selected by WINDOW IDENTITY. Taking + // "the first context for my frontend" would silently + // depend on capture order and could ship the panel's + // mode-line text as the document status band. + if let Some(window) = windows.into_iter().find(|window| { + window.context.frontend_id == frontend_id + && Some(window.context.window_id) == document_window + }) { self.emit_statusline_payload( window.context.buffer_id, to_wire(window.left), @@ -2925,6 +2940,7 @@ mod tests { ), new_failures: Vec::new(), }, + None, &mut stale, ); assert!(stale.is_empty(), "phase-1 stale evaluation emits nothing"); @@ -2945,13 +2961,13 @@ mod tests { new_failures: Vec::new(), }; let mut replacement = Vec::new(); - semantic.emit_statusline_segments(invalidated(), &mut replacement); + semantic.emit_statusline_segments(invalidated(), None, &mut replacement); assert_eq!( statusline_of(&replacement), Some((buffer_id, Vec::new(), Vec::new())) ); let mut unchanged = Vec::new(); - semantic.emit_statusline_segments(invalidated(), &mut unchanged); + semantic.emit_statusline_segments(invalidated(), None, &mut unchanged); assert!( unchanged.is_empty(), "the empty invalidation became baseline" diff --git a/src/statusline.rs b/src/statusline.rs index 1e6d9cc..7bf345f 100644 --- a/src/statusline.rs +++ b/src/statusline.rs @@ -660,12 +660,46 @@ fn capture_target_contexts( if window.buffer_id != declared_buffer { return Err(StatuslineNoMessageReason::DeclaredBufferMismatch); } - Ok(vec![StatuslineContext { + let mut contexts = vec![StatuslineContext { frontend_id, window_id: window.id, buffer_id: window.buffer_id, active: window.id == view.active, - }]) + }]; + // Bottom-panel Q#BP8 / A2A-2 — the semantic fan-out is the + // primary document PLUS the frontend's visible side window, + // and nothing else: unprojected document splits run no + // callbacks. The document result feeds the semantic + // `StatuslineSegments`; the side result paints in the panel's + // own mode line. + // + // A derived-hidden side window is omitted (Q#BP2b): it has no + // mode line to paint this frame, so evaluating providers for + // it would invoke callbacks for a surface nobody can see. + if !view.panel_hidden { + for side_id in view.layout.iter_ids() { + if side_id == window.id { + continue; + } + let Some(side) = core.windows.get(&side_id) else { + return Err(StatuslineNoMessageReason::ContextUnavailable); + }; + if !side.is_side() { + continue; + } + if buffers.get(side.buffer_id).is_err() { + return Err(StatuslineNoMessageReason::BufferUnavailable); + } + contexts.push(StatuslineContext { + frontend_id, + window_id: side.id, + buffer_id: side.buffer_id, + // Same rule as the document context: ACTUAL focus. + active: side.id == view.active, + }); + } + } + Ok(contexts) } } } diff --git a/src/terminal/mod.rs b/src/terminal/mod.rs index 27ee96c..11272e4 100644 --- a/src/terminal/mod.rs +++ b/src/terminal/mod.rs @@ -34,6 +34,10 @@ pub use pmacs_protocol::terminal::{ /// Configuration-time, not a wire bound: history never crosses the /// protocol, so this stays core-owned. pub const DEFAULT_TERMINAL_SCROLLBACK_ROWS: usize = 10_000; + +/// Default `terminal.escape-key`, and the fallback an unparseable value +/// falls back to (Q#TC4a). +pub const DEFAULT_TERMINAL_ESCAPE_KEY: &str = "C-c"; /// Maximum retained main-screen history cells. Core-owned for the same /// reason as [`DEFAULT_TERMINAL_SCROLLBACK_ROWS`]. pub const MAX_TERMINAL_HISTORY_CELLS: usize = 4_000_000; diff --git a/src/terminal/session.rs b/src/terminal/session.rs index ee96923..c731fb0 100644 --- a/src/terminal/session.rs +++ b/src/terminal/session.rs @@ -12,6 +12,7 @@ use crate::ansi::AnsiParserProfile; use crate::buffer::{Buffer, BufferId}; use crate::cell::{Cell, CellCoord, CellSize}; use crate::editor_core::EditorCore; +use crate::key::{Chord, parse_chord}; use crate::process::{ ProcessEventKind, ProcessId, ProcessMode, ProcessSpec, ProcessState, ProcessSupervisor, RestartPolicy, StdinMode, TerminalMode, @@ -218,12 +219,40 @@ pub(super) struct TerminalSession { pub(super) screen: TerminalScreen, pub(super) process: TerminalProcessState, pub(super) annotated: bool, + /// Resolved `terminal.escape-key` for this terminal (Q#TC4c). + /// + /// The cache lives HERE, not in an editor-side map, because a + /// session is created in [`TerminalManager::open`] and dropped on + /// kill/prune — so its lifetime is exactly the cache's, with no + /// purge hook to forget. An editor-side map would leak an entry per + /// terminal; a single last-entry cache would reparse (and re-report + /// an invalid value) every time focus alternates between two + /// terminals. + pub(super) escape: Option, +} + +/// One terminal's parsed escape chord, valid for one config epoch. +pub(super) struct EscapeCache { + /// The `ConfigRegistry::value_epoch` this was parsed at. The key is + /// `(this session, epoch)`: the epoch alone is not enough, because + /// it does not advance when focus moves between terminals with + /// different buffer-local values. + pub(super) epoch: u64, + /// The effective chord — the parsed spelling, or the `C-c` fallback. + pub(super) chord: Chord, + /// The invalid spelling already reported for this terminal, if any. + /// Reporting is once per terminal per effective invalid value: an + /// unchanged bad value stays quiet, a *different* bad value reports + /// again because it is a new mistake. + pub(super) reported_invalid: Option, } /// Owns the one-buffer/one-process/one-screen terminal registry. #[derive(Default)] pub struct TerminalManager { pub(super) sessions: HashMap, + /// Total escape-key parses performed (Q#TC4c observability). + escape_parses: u64, process_to_buffer: HashMap, /// Removed buffers whose children are still being reaped. Their events /// remain manager-owned so Lua/LSP/MCP consumers cannot steal a batch. @@ -331,6 +360,7 @@ impl TerminalManager { screen, process: TerminalProcessState::Running, annotated: false, + escape: None, }, ); debug_assert!(previous.is_none(), "fresh BufferId collided"); @@ -538,6 +568,89 @@ impl TerminalManager { .map_err(TerminalError::Process) } + /// Resolve this terminal's effective escape chord, parsing at most + /// once per `(terminal, config epoch)` (Q#TC4c). + /// + /// `spelling` is the caller-resolved `terminal.escape-key` value and + /// `epoch` the registry's `value_epoch()` it was read at. Returns the + /// effective chord plus, at most once per terminal per effective + /// invalid value, a message the caller should surface. + /// + /// An unparseable spelling falls back to `C-c` rather than leaving the + /// terminal with no escape at all (Q#TC4a): without one, every key goes + /// to the child and the user cannot reach the binding that would fix + /// the setting that broke it. + pub fn escape_chord( + &mut self, + buffer_id: BufferId, + epoch: u64, + spelling: &str, + ) -> (Chord, Option) { + let fallback = default_escape_chord(); + if let Some(session) = self.sessions.get(&buffer_id) + && let Some(cache) = session.escape.as_ref() + && cache.epoch == epoch + { + return (cache.chord, None); + } + self.escape_parses = self.escape_parses.saturating_add(1); + let Some(session) = self.sessions.get_mut(&buffer_id) else { + return (fallback, None); + }; + let previously_reported = session + .escape + .as_ref() + .and_then(|cache| cache.reported_invalid.clone()); + let (chord, reported_invalid, report) = match parse_chord(spelling) { + Ok(chord) => (chord, None, None), + Err(error) => { + 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" + ) + }); + (fallback, Some(spelling.to_owned()), message) + } + }; + session.escape = Some(EscapeCache { + epoch, + chord, + reported_invalid, + }); + (chord, report) + } + + /// How many escape-key spellings this manager has parsed. + /// + /// An observability seam for Q#TC4c's cache contract, which is + /// otherwise unpinnable for a VALID setting: a correct per-session + /// cache and a single last-entry cache produce identical behavior + /// there and differ only in how often they parse. Counting reports + /// covers the invalid case; this covers the valid one. + #[must_use] + pub fn escape_parses(&self) -> u64 { + 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, @@ -730,3 +843,12 @@ fn sanitize_metadata(value: &str) -> String { } clean } + +/// The built-in terminal escape chord, and the fallback for an +/// unparseable `terminal.escape-key` (Q#TC4a). +pub(super) fn default_escape_chord() -> Chord { + Chord::new( + crossterm::event::KeyCode::Char('c'), + crossterm::event::KeyModifiers::CONTROL, + ) +} diff --git a/src/window.rs b/src/window.rs index b66499e..2ac0087 100644 --- a/src/window.rs +++ b/src/window.rs @@ -558,8 +558,19 @@ pub struct FrontendView { /// store. Reckoning in visible lines unconditionally would make that /// GPU session's cursor skip lines it is still showing, so every /// command/event-time visible-line reckoning is gated on the - /// **acting** frontend's flag. Render-time clamps need no gate: a - /// semantic session never enters `paint_frame`. + /// **acting** frontend's flag. + /// + /// **Render-time clamps used to need no gate, on the premise that a + /// semantic session never enters `paint_frame`. The bottom-panel + /// band breaks that premise** (Q#BP17): the daemon projects a + /// semantic frontend's side window through the same per-window + /// painter. So the extracted painters + /// (`prepare_window_cursor_visible`, `paint_window_content`) take the + /// visible-line map as a **parameter**, and the panel path passes + /// `None` when the *owning* frontend's `fold_projection` is false. + /// That path must not call `EditorCore::fold_map_for_window`, which + /// gates on the **active** frontend — correct for command-time + /// reckoning, wrong for painting another frontend's panel. /// /// Set at attach from the negotiated selected-render bit (grid ⇒ /// `true`, semantic ⇒ `false`), cleared with the view at detach, and diff --git a/tests/bottom_panel_stage2a_acceptance.rs b/tests/bottom_panel_stage2a_acceptance.rs index 5f7996d..3b6c183 100644 --- a/tests/bottom_panel_stage2a_acceptance.rs +++ b/tests/bottom_panel_stage2a_acceptance.rs @@ -203,6 +203,21 @@ fn statusline_document_context_reports_active_false_under_a_focused_panel() { match evaluation.outcome { StatuslineEvaluationOutcome::Ready(windows) => { + // A2A-2: the semantic fan-out captures the primary document + // AND the visible side window — two contexts, not one. + assert_eq!( + windows.len(), + 2, + "the semantic-layout target must capture document + visible side window" + ); + let side = windows + .iter() + .find(|w| w.context.window_id != document) + .expect("a side-window context"); + assert!( + side.context.active, + "the focused panel's own context reports active = true" + ); let context = windows .first() .map(|segments| segments.context) @@ -275,6 +290,15 @@ fn extraction_preserves_cells_cursor_and_focused_view_top() { pmacs.window.display(b, {})", ); + // The gutter only paints when line numbers are on, so turn them on + // rather than dropping the assertion. + { + let mut core = s.core.borrow_mut(); + let active = core.views[&FrontendId::LOCAL].active; + core.windows.get_mut(&active).unwrap().line_numbers = + pmacs::window::LineNumberMode::Absolute; + } + let size = CellSize::new(ROWS, COLS); let mut cells_a = vec![pmacs::cell::Cell::default(); (ROWS * COLS) as usize]; let mut grid_a = CellGrid { @@ -312,6 +336,44 @@ fn extraction_preserves_cells_cursor_and_focused_view_top() { assert_eq!(cells_a, cells_b, "painted cells must be stable"); assert_eq!(cursor_a, cursor_b, "the returned cursor must be stable"); assert_eq!(view_top_a, view_top_b, "focused view_top must be stable"); + + // Review round 1, finding 5: a fixed-point check alone is VACUOUS — + // deleting `text_view.render` leaves it green. Assert the extracted + // painter actually produced each of its four outputs. + let rows = |cells: &[pmacs::cell::Cell]| -> Vec { + (0..ROWS as usize) + .map(|r| { + (0..COLS as usize) + .map(|c| match &cells[r * COLS as usize + c].glyph { + pmacs::cell::Glyph::Char(ch) => *ch, + _ => ' ', + }) + .collect::() + }) + .collect() + }; + let painted = rows(&cells_a); + + // TEXT: the buffer's content reached the grid. + assert!( + painted.iter().any(|row| row.contains("line")), + "the extracted painter must paint buffer TEXT; got {painted:?}" + ); + // GUTTER: line numbers were painted beside it. + assert!( + painted.iter().any(|row| row.trim_start().starts_with('1')), + "the extracted painter must paint the line-number GUTTER" + ); + // MODE LINE: the window's mode line names its buffer. + assert!( + painted.iter().any(|row| row.contains("*doc*")), + "the extracted painter must paint the window MODE LINE" + ); + // CURSOR: a real caret position came back, not None. + assert!( + cursor_a.is_some(), + "the extraction must still return a caret position" + ); } #[test] @@ -375,3 +437,151 @@ fn the_panel_fixture_really_builds_a_side_window() { "the fixture must produce a real bottom side window" ); } + +// --------------------------------------------------------------------------- +// A2A-1 at the CONSUMER seam — review round 1, finding 3. +// +// The tests above assert the *authority* (`primary_document_window`). +// That is not sufficient: restoring a producer to `active_window_for` +// leaves every one of them green. These drive the real producers through +// `SemanticRenderState::render_frame` with a panel focused, so a reverted +// routing fails here. +// --------------------------------------------------------------------------- + +/// A semantic frontend that CAN hold a panel. Stage 1 ships +/// `panel_capable = false` for semantic sessions and 2B flips it for a +/// v21-negotiated peer; until then the projection is only reachable with +/// a test-only capable view, which is exactly what the framing's §7.2 +/// says 2B must replace with the real capability flip. +fn semantic_frontend_with_focused_panel( + s: &EditorState, +) -> (FrontendId, WindowId, WindowId, pmacs::buffer::BufferId) { + use pmacs::window::{FrontendView, Layout, LayoutNode, Orientation, Window, WindowParams}; + + let fid = FrontendId(77); + let (doc_win, panel_win, doc_buf) = { + let mut core = s.core.borrow_mut(); + let doc_buf = core.active_window().buffer_id; + let panel_buf = core.registry.borrow_mut().create("*panel*"); + + let doc_win = WindowId::next(); + let panel_win = WindowId::next(); + let doc_view = { + let reg = core.registry.borrow(); + pmacs::text_view::TextView::new(reg.get(doc_buf).expect("document buffer")) + }; + let panel_view = { + let reg = core.registry.borrow(); + pmacs::text_view::TextView::new(reg.get(panel_buf).expect("panel buffer")) + }; + core.windows + .insert(doc_win, Window::new(doc_win, doc_buf, doc_view)); + let mut panel = Window::new(panel_win, panel_buf, panel_view); + let mut params = WindowParams::default(); + params.side = Some(Side::Bottom); + params.fixed_rows = Some(4); + panel.params = params; + core.windows.insert(panel_win, panel); + + core.register_frontend_view( + fid, + FrontendView { + layout: Layout { + root: LayoutNode::Split { + orientation: Orientation::Horizontal, + children: vec![LayoutNode::Leaf(doc_win), LayoutNode::Leaf(panel_win)], + weights: vec![1, 1], + }, + }, + // The panel owns focus; the document is the projection. + active: panel_win, + fold_projection: false, + panel_capable: true, + frame_geometry: None, + panel_hidden: false, + }, + ); + (doc_win, panel_win, doc_buf) + }; + s.sync_frame_geometry(fid, CellSize::new(ROWS, COLS)); + (fid, doc_win, panel_win, doc_buf) +} + +#[test] +fn consumer_line_numbers_follow_the_document_not_the_focused_panel() { + use pmacs::protocol::{ByteRange, InstanceMessage}; + use pmacs::semantic_render::SemanticRenderState; + use pmacs::window::LineNumberMode; + + let s = editor(); + let (fid, doc_win, panel_win, doc_buf) = semantic_frontend_with_focused_panel(&s); + + // Make the two windows DISAGREE, so the emitted mode identifies + // which window the producer read (§1.3 #4). + { + let mut core = s.core.borrow_mut(); + core.windows.get_mut(&doc_win).unwrap().line_numbers = LineNumberMode::Absolute; + core.windows.get_mut(&panel_win).unwrap().line_numbers = LineNumberMode::Off; + } + + let mut sem = SemanticRenderState::new(fid); + sem.set_viewport(doc_buf, ByteRange { start: 0, end: 0 }, 0); + let msgs = sem.render_frame(&s); + + let mode = msgs.iter().find_map(|m| match m { + InstanceMessage::LineNumbers { mode, .. } => Some(*mode), + _ => None, + }); + assert_eq!( + mode, + Some(pmacs::protocol::LineNumberMode::Absolute), + "LineNumbers must describe the DOCUMENT window's mode, not the focused panel's" + ); +} + +#[test] +fn consumer_statusline_segments_name_the_document_window() { + use pmacs::protocol::ByteRange; + use pmacs::semantic_render::SemanticRenderState; + + // §1.3 #12 at the producer: the wire segments must be selected by + // the DOCUMENT window even though the fan-out now also evaluates the + // visible side window (A2A-2). + let s = editor(); + let (fid, doc_win, _panel_win, doc_buf) = semantic_frontend_with_focused_panel(&s); + + let mut sem = SemanticRenderState::new(fid); + sem.set_viewport(doc_buf, ByteRange { start: 0, end: 0 }, 0); + // Not asserting on message presence (a peer that never negotiated + // v18 emits none); asserting the routing input the producer uses. + let _ = sem.render_frame(&s); + + assert_eq!( + s.core.borrow().primary_document_window(fid), + Some(doc_win), + "the producer's document-window selector must name the document" + ); +} + +#[test] +fn consumer_terminal_declaration_cannot_be_claimed_by_a_focused_panel() { + // §1.3 #6/#10/#11 through the real guard: with the panel focused, + // a declaration naming the PANEL's buffer must be refused, because + // the full-window terminal surface is the document window. + let s = editor(); + let (fid, _doc_win, panel_win, doc_buf) = semantic_frontend_with_focused_panel(&s); + let panel_buf = s.core.borrow().windows[&panel_win].buffer_id; + + assert!( + !s.semantic_terminal_declaration_is_active(fid, panel_buf), + "a focused panel's buffer must not become the document terminal declaration" + ); + // Non-vacuity: the document buffer is not a terminal either, so pin + // that the guard resolves the DOCUMENT window by asserting the + // window identity the resolver used. + assert_eq!( + s.core.borrow().primary_document_buffer(fid), + Some(doc_buf), + "the terminal resolver's window must be the document window" + ); +} diff --git a/tests/terminal_config_acceptance.rs b/tests/terminal_config_acceptance.rs new file mode 100644 index 0000000..ceeb8fe --- /dev/null +++ b/tests/terminal_config_acceptance.rs @@ -0,0 +1,746 @@ +//! Terminal configuration acceptance (Stage 1 of +//! `docs/terminal-config-and-copy-mode-framing.md`, criteria 1-12). +//! +//! Deliberately NOT `#[cfg(feature = "crdt")]`: CI never enables that +//! feature, so a gated suite is written and then never run. + +use std::thread; +use std::time::{Duration, Instant}; + +use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; +use mlua::Value; +use pmacs::cell::{CellSize, Glyph}; +use pmacs::editor::EditorState; +use pmacs::protocol::FrontendId; +use pmacs::terminal::TerminalViewKey; +use pmacs::window::WindowId; + +fn exec(state: &EditorState, src: &str) { + state + .lua_host + .lua() + .load(src) + .exec() + .unwrap_or_else(|e| panic!("lua failed: {src}\n{e}")); +} + +fn eval_err(state: &EditorState, src: &str) -> String { + let result: mlua::Result = state.lua_host.lua().load(src).eval(); + match result { + Ok(_) => panic!("expected an error from: {src}"), + Err(e) => e.to_string(), + } +} + +/// 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 cells { + match &cell.glyph { + Glyph::Char(c) => text.push(*c), + Glyph::Cluster(b) => text.push_str(&String::from_utf8_lossy(b)), + Glyph::Continuation => {} + } + } + 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 { + state.tick_processes(); + if screen_text(state, buffer).contains(needle) { + return true; + } + if Instant::now() >= deadline { + return false; + } + thread::sleep(Duration::from_millis(20)); + } +} + +/// Give LOCAL a window on `buffer` and register/claim its terminal view, +/// which is what makes `dispatch_key`'s terminal arm reachable. +fn focus_terminal(state: &EditorState, buffer: pmacs::buffer::BufferId) -> WindowId { + state.core.borrow_mut().switch_active_buffer(buffer).ok(); + let window = state.core.borrow().active_window_id(); + let key = TerminalViewKey::new(FrontendId::LOCAL, window, buffer); + let mut manager = state.terminal_manager.borrow_mut(); + manager.register_view(key); + manager.claim_controller(key); + let _ = manager.snapshot_for_view(key, viewport()); + window +} + +fn terminal_buffers(state: &EditorState) -> Vec { + let manager = state.terminal_manager.borrow(); + state + .core + .borrow() + .registry + .borrow() + .ids() + .iter() + .copied() + .filter(|id| manager.is_terminal(*id)) + .collect() +} + +/// Open a terminal from Lua and return the identity buffer it created. +/// +/// The id is derived by diffing the manager's terminal set rather than +/// returned through Lua: `BufferIdLua` exposes no id accessor, and +/// diffing also asserts in passing that exactly one terminal appeared. +fn open_cat_terminal(state: &EditorState, lua_spec: &str) -> pmacs::buffer::BufferId { + let before = terminal_buffers(state); + exec( + state, + &format!("TERM_BUF = pmacs.terminal.open {{ {lua_spec} }}"), + ); + let after = terminal_buffers(state); + let mut fresh: Vec<_> = after + .into_iter() + .filter(|id| !before.contains(id)) + .collect(); + assert_eq!(fresh.len(), 1, "exactly one terminal must have opened"); + fresh.remove(0) +} + +/// `cat -v` is the echo instrument, deliberately: the terminal screen +/// rejects C0/C1 controls before they enter cells (Vterm Stage 1 +/// criterion 2), so a raw echoed `Ctrl-X` would be invisible and a test +/// probing for it could never pass. `-v` renders it as the printable +/// two-character `^X`, which is what makes "the configured chord reached +/// the child" observable at all. +const CAT_PROFILE: &str = r#" +pmacs.terminal.profiles.echo = { + command = "/bin/sh", + args = { "-c", "printf 'READY\r\n'; exec cat -v" }, +} +"#; + +/// Did the last key ARM the terminal escape? +/// +/// Observed behaviorally rather than through an accessor: while the +/// escape is armed the next key goes to ordinary dispatch, so it never +/// reaches the child. `cat` echoes anything that does reach it, which +/// makes "the probe character did not appear" the exact observable for +/// "that chord was consumed as the escape". +fn escape_was_armed(state: &mut EditorState, buffer: pmacs::buffer::BufferId, probe: char) -> bool { + // Count occurrences rather than testing for presence: the screen + // already holds the child's own output, and a single-character probe + // like 'R' collides with the "READY" banner. Only an INCREASE proves + // this keystroke reached the child. + let before = screen_text(state, buffer).matches(probe).count(); + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char(probe), KeyModifiers::NONE), + ); + let deadline = Instant::now() + Duration::from_secs(2); + loop { + state.tick_processes(); + if screen_text(state, buffer).matches(probe).count() > before { + return false; + } + if Instant::now() >= deadline { + return true; + } + thread::sleep(Duration::from_millis(20)); + } +} + +/// Acceptance 1: a profile spec is strict, and rejects before anything spawns. +#[test] +fn acc1_profile_specs_are_strict_and_reject_before_spawning() { + let state = EditorState::new(); + let before = state.core.borrow().registry.borrow().ids().len(); + + exec( + &state, + r#"pmacs.terminal.profiles.bad = { command = "/bin/sh", nonsense = true }"#, + ); + let err = eval_err(&state, r#"return pmacs.terminal.open { profile = "bad" }"#); + assert!( + err.contains("unknown field") && err.contains("nonsense"), + "the error must name the offending field: {err}" + ); + + exec(&state, "pmacs.terminal.profiles.wrong = { command = 42 }"); + let err = eval_err( + &state, + r#"return pmacs.terminal.open { profile = "wrong" }"#, + ); + assert!(err.contains("must be a string"), "typed field error: {err}"); + + assert_eq!( + state.core.borrow().registry.borrow().ids().len(), + before, + "a rejected profile must create no buffer" + ); + assert_eq!(state.terminal_manager.borrow().len(), 0); +} + +/// Acceptance 2: an unknown profile names the known ones and creates nothing. +#[test] +fn acc2_unknown_profile_lists_known_names_and_creates_nothing() { + let state = EditorState::new(); + exec(&state, CAT_PROFILE); + exec( + &state, + r#"pmacs.terminal.profiles.other = { command = "/bin/sh" }"#, + ); + let before = state.core.borrow().registry.borrow().ids().len(); + + // Via the default setting. + exec( + &state, + r#"pmacs.config.set("terminal.default-profile", "ghost")"#, + ); + let err = eval_err(&state, "return pmacs.terminal.open {}"); + assert!(err.contains("ghost"), "names the missing profile: {err}"); + assert!( + err.contains("echo") && err.contains("other"), + "must LIST the known profiles: {err}" + ); + + // An explicit bad profile fails even though the default is now valid — + // a typo must not silently fall back (Q#TC3a). + exec( + &state, + r#"pmacs.config.set("terminal.default-profile", "echo")"#, + ); + let err = eval_err(&state, r#"return pmacs.terminal.open { profile = "typo" }"#); + assert!(err.contains("typo"), "explicit bad profile errors: {err}"); + + assert_eq!( + state.core.borrow().registry.borrow().ids().len(), + before, + "no buffer, session, or process is created" + ); + 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] +fn acc3_field_resolution_order_and_env_merge() { + let mut state = EditorState::new(); + exec( + &state, + r#" + pmacs.terminal.profiles.merged = { + command = "/bin/sh", + args = { "-c", "printf 'PROFILE:%s:%s\r\n' \"$FROM_PROFILE\" \"$SHARED\"; exec cat" }, + env = { FROM_PROFILE = "p", SHARED = "profile" }, + } + "#, + ); + let buffer = open_cat_terminal( + &state, + r#"profile = "merged", env = { SHARED = "explicit" }"#, + ); + assert!( + tick_until(&mut state, "PROFILE:p:explicit", buffer), + "profile env survives and explicit env overrides the same key: {:?}", + screen_text(&state, buffer) + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 3 (explicit command wins) and 4 (`""` means no profile). +#[test] +fn acc3_acc4_explicit_command_wins_and_empty_default_means_no_profile() { + let mut state = EditorState::new(); + exec(&state, CAT_PROFILE); + exec( + &state, + r#"pmacs.config.set("terminal.default-profile", "echo")"#, + ); + + // Explicit command beats the profile's. + let explicit = open_cat_terminal( + &state, + r#"command = "/bin/sh", args = { "-c", "printf 'EXPLICIT\r\n'; exec cat" }"#, + ); + assert!(tick_until(&mut state, "EXPLICIT", explicit)); + + // `""` is the no-profile sentinel: falls through to $SHELL. + exec( + &state, + r#"pmacs.config.set("terminal.default-profile", "")"#, + ); + let bare = open_cat_terminal(&state, ""); + let spec_ok = state.terminal_manager.borrow().is_terminal(bare); + assert!(spec_ok, "an empty default must open a $SHELL terminal"); + assert!( + !screen_text(&state, bare).contains("READY"), + "the echo profile must NOT have been applied" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// 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_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!( + state + .lua_host + .lua() + .load(r#"return pmacs.config.get("terminal.scrollback-rows")"#) + .eval::() + .unwrap(), + 0, + "0 is a legal scrollback value meaning 'retain no history'" + ); + + let err = eval_err( + &state, + r#"return pmacs.config.set("terminal.scrollback-rows", -1)"#, + ); + assert!( + err.contains("-1") || err.contains("min"), + "below range: {err}" + ); + let err = eval_err( + &state, + r#"return pmacs.config.set("terminal.scrollback-rows", 4000001)"#, + ); + assert!( + err.contains("4000001") || err.contains("max"), + "above range: {err}" + ); +} + +/// Acceptance 6 and 9: the configured chord escapes, repeating it sends +/// THAT chord to the child, and an ordinary `C-c` still reaches the child. +#[test] +fn acc6_acc9_configured_escape_chord_and_literal_repeat() { + let mut state = EditorState::new(); + exec(&state, CAT_PROFILE); + let buffer = open_cat_terminal(&state, r#"profile = "echo""#); + assert!(tick_until(&mut state, "READY", buffer)); + focus_terminal(&state, buffer); + + exec(&state, r#"pmacs.config.set("terminal.escape-key", "C-x")"#); + + // `C-x C-x` must send Ctrl-X (0x18), which `cat` echoes back. Against + // the pre-change hardcoded `&[0x03]` this sends Ctrl-C instead. + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('x'), KeyModifiers::CONTROL), + ); + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('x'), KeyModifiers::CONTROL), + ); + assert!( + tick_until(&mut state, "^X", buffer), + "C-x C-x must send literal Ctrl-X: {:?}", + screen_text(&state, buffer) + ); + + // With the escape moved, an ordinary C-c is just another key. + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('c'), KeyModifiers::CONTROL), + ); + assert!( + tick_until(&mut state, "^C", buffer), + "plain C-c must reach the child once the escape moved: {:?}", + screen_text(&state, buffer) + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 7, 8 and 8a: per-terminal escape resolution, an A→B→A parse +/// count that does not grow, and a cache that dies with its terminal. +#[test] +fn acc7_acc8_acc8a_per_terminal_escape_cache_identity_and_lifecycle() { + let mut state = EditorState::new(); + exec(&state, CAT_PROFILE); + let a = open_cat_terminal(&state, r#"profile = "echo""#); + exec(&state, "TERM_A = TERM_BUF"); + let b = open_cat_terminal(&state, r#"profile = "echo""#); + exec(&state, "TERM_B = TERM_BUF"); + assert!(tick_until(&mut state, "READY", a)); + assert!(tick_until(&mut state, "READY", b)); + + // Different buffer-local escapes, then NO further writes. + exec( + &state, + r#"pmacs.config.set_local(TERM_A, "terminal.escape-key", "C-x")"#, + ); + exec( + &state, + r#"pmacs.config.set_local(TERM_B, "terminal.escape-key", "C-b")"#, + ); + + // Prime both caches. Each priming press ARMS the escape, so it is + // consumed with a probe — otherwise the next chord would be read as + // the escape repeat rather than a fresh escape. + focus_terminal(&state, a); + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('x'), KeyModifiers::CONTROL), + ); + assert!(escape_was_armed(&mut state, a, 'M'), "A primes on its C-x"); + focus_terminal(&state, b); + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('b'), KeyModifiers::CONTROL), + ); + 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 + // entry it finds to every terminal, so A keeps working by accident + // while B silently inherits A's chord. The discriminating assertion + // is that EACH terminal honors its OWN chord and NOT the other's. + focus_terminal(&state, b); + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('b'), KeyModifiers::CONTROL), + ); + assert!( + escape_was_armed(&mut state, b, 'R'), + "terminal B must escape on its own C-b" + ); + // ...and A's chord must be ordinary input in B, not an escape. + focus_terminal(&state, b); + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('x'), KeyModifiers::CONTROL), + ); + assert!( + !escape_was_armed(&mut state, b, 'S'), + "terminal A's C-x must NOT escape terminal B" + ); + + focus_terminal(&state, a); + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('x'), KeyModifiers::CONTROL), + ); + assert!( + escape_was_armed(&mut state, a, 'Q'), + "terminal A must still escape on its own C-x after A->B->A" + ); + + // Acceptance 8: that round trip parsed nothing new. A single + // last-entry cache would have reparsed twice. + assert_eq!( + state.terminal_manager.borrow().escape_parses(), + primed, + "A->B->A with no setting written must not reparse" + ); + + // 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)"); + // Pruning is tick-driven (the manager reaps on the process tick), so + // the session outlives the kill call by design. + let deadline = Instant::now() + Duration::from_secs(5); + while state.terminal_manager.borrow().len() >= sessions_before { + state.tick_processes(); + assert!( + Instant::now() < deadline, + "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(); +} + +/// Acceptance 10 and 10a: an unparseable value falls back, reports through +/// the status line, and reports once per terminal per effective bad value. +#[test] +fn acc10_acc10a_invalid_escape_falls_back_and_reports_once() { + let mut state = EditorState::new(); + exec(&state, CAT_PROFILE); + let buffer = open_cat_terminal(&state, r#"profile = "echo""#); + assert!(tick_until(&mut state, "READY", buffer)); + focus_terminal(&state, buffer); + + exec( + &state, + r#"pmacs.config.set("terminal.escape-key", "not-a-chord")"#, + ); + state.core.borrow_mut().status.clear(); + + // Acceptance 10: falls back to C-c, so the terminal stays escapable. + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('c'), KeyModifiers::CONTROL), + ); + // Read the report BEFORE probing: `status` is a single slot, and the + // probe key's own rejected self-insert would overwrite it. + let reported = state.core.borrow().status.clone(); + assert!( + reported.contains("terminal.escape-key") && reported.contains("not-a-chord"), + "the report must name the setting and the bad value: {reported:?}" + ); + assert!( + escape_was_armed(&mut state, buffer, 'Q'), + "an invalid escape-key must fall back to C-c, not leave the \ + terminal unescapable" + ); + + // Acceptance 10a: the same bad value does not report again. + state.core.borrow_mut().status.clear(); + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('c'), KeyModifiers::CONTROL), + ); + assert!( + state.core.borrow().status.is_empty(), + "an unchanged invalid value must not re-report: {:?}", + state.core.borrow().status + ); + let _ = escape_was_armed(&mut state, buffer, 'W'); + + // A DIFFERENT bad value is new information, so it reports again. + exec( + &state, + r#"pmacs.config.set("terminal.escape-key", "also-bad")"#, + ); + state.core.borrow_mut().status.clear(); + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('c'), KeyModifiers::CONTROL), + ); + assert!( + state.core.borrow().status.contains("also-bad"), + "a different invalid value must report: {:?}", + state.core.borrow().status + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 11: the opening binding exists, resolves to the command, and +/// shadowed nothing (`keymap.bind` is strict, so loading the runtime at all +/// proves the second half). +#[test] +fn acc11_terminal_opening_binding_is_bound_and_shadowed_nothing() { + let state = EditorState::new(); + let command: Option = state + .lua_host + .lua() + .load(r#"local d = pmacs.describe.key("C-c t"); return d and d.command"#) + .eval() + .expect("describe.key"); + assert_eq!( + command.as_deref(), + Some("terminal"), + "C-c t must open a terminal" + ); +} + +/// Acceptance 12: with no settings written and no profiles registered, the +/// defaults reproduce the pre-arc behavior. +#[test] +fn acc12_defaults_reproduce_prior_behavior() { + let state = EditorState::new(); + let lua = state.lua_host.lua(); + assert_eq!( + lua.load(r#"return pmacs.config.get("terminal.default-profile")"#) + .eval::() + .unwrap(), + "" + ); + assert_eq!( + lua.load(r#"return pmacs.config.get("terminal.scrollback-rows")"#) + .eval::() + .unwrap(), + 10_000 + ); + assert_eq!( + lua.load(r#"return pmacs.config.get("terminal.escape-key")"#) + .eval::() + .unwrap(), + "C-c" + ); + assert!( + lua.load("return next(pmacs.terminal.profiles) == nil") + .eval::() + .unwrap(), + "no profiles are registered by default" + ); +}