fix(terminal): close review round 1 on Stage 1
Five findings, all real. The blocker and both majors are the same
mistake in three places: a claim asserted somewhere cheaper than where
it actually lives.
COHERENCE.md was stale in four places, not the three reported. Step 8
still read "no keybinding" and §11 still read "five settings", but §6's
dispatch table also still cited `is_terminal_escape_chord` — a symbol
this branch deletes. §25 requires that update to ride the PR, so a PR
changing audited ground truth has to re-grep the audit for its own
symbols, not only for its topic.
Acceptance 5 asserted a registry round-trip, which is a test of the
registry: 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.
Acceptance 8a waited for the session count to fall, which the rejected
editor-side cache map satisfies exactly — a map with no purge hook
leaks while sessions drain. Adds `TerminalManager::escape_caches()`, the
lifetime half of Q#TC4c's contract that `escape_parses` cannot cover.
`table.sort` over `pmacs.terminal.profiles` raised "attempt to compare
number with string" on the unknown-profile path whenever the user's
table held both a string and a numeric key, replacing the exact
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.
Also corrects the framing's status line, and a status message whose
embedded whitespace run had survived a rustfmt reflow.
Three new bites, each falsified by revert: deleting the scrollback
consumer fails acc5 and only acc5; restoring the raw-key sort
reproduces the comparison error verbatim; and implementing the rejected
map fails the new acc8a at left: 2, right: 1 while passing the old
session-count version.
Merges githubsucks/main @ ccf29e3.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer
This commit is contained in:
parent
e1db5bb392
commit
8e8f281f0e
32
COHERENCE.md
32
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.
|
||||
|
|
|
|||
|
|
@ -65,27 +65,44 @@ local PROFILE_FIELDS = {
|
|||
env = "table",
|
||||
}
|
||||
|
||||
-- Every diagnostic below renders a caller- or user-supplied value, so
|
||||
-- rendering must never be the thing that fails. `%q` is partial — it
|
||||
-- raises on a table or function — and a profile name arrives straight
|
||||
-- from `open { profile = ... }`.
|
||||
local function describe_name(name)
|
||||
if type(name) == "string" then return string.format("%q", name) end
|
||||
return string.format("<%s %s>", type(name), tostring(name))
|
||||
end
|
||||
|
||||
local function validate_profile(name, profile)
|
||||
local shown = describe_name(name)
|
||||
if type(profile) ~= "table" then
|
||||
error(string.format("terminal profile %q must be a table", name), 0)
|
||||
error(string.format("terminal profile %s must be a table", shown), 0)
|
||||
end
|
||||
for key, value in pairs(profile) do
|
||||
local expected = PROFILE_FIELDS[key]
|
||||
if not expected then
|
||||
error(string.format("terminal profile %q: unknown field %q", name, tostring(key)), 0)
|
||||
error(string.format("terminal profile %s: unknown field %q", shown, tostring(key)), 0)
|
||||
end
|
||||
if type(value) ~= expected then
|
||||
error(string.format(
|
||||
"terminal profile %q: field %q must be a %s, got %s",
|
||||
name, key, expected, type(value)), 0)
|
||||
"terminal profile %s: field %q must be a %s, got %s",
|
||||
shown, key, expected, type(value)), 0)
|
||||
end
|
||||
end
|
||||
return profile
|
||||
end
|
||||
|
||||
-- `terminal.profiles` is a raw user table, so its keys are whatever the
|
||||
-- user wrote. Sorting them directly raises "attempt to compare number
|
||||
-- with string" the moment the table holds both a string and a numeric
|
||||
-- key — and it raises on the UNKNOWN-PROFILE path, replacing the very
|
||||
-- error this list exists to explain with an opaque one. Sorting DISPLAY
|
||||
-- strings is total over every key type, so the diagnostic survives a
|
||||
-- malformed table.
|
||||
local function known_profile_names()
|
||||
local names = {}
|
||||
for name in pairs(terminal.profiles) do names[#names + 1] = name end
|
||||
for name in pairs(terminal.profiles) do names[#names + 1] = tostring(name) end
|
||||
table.sort(names)
|
||||
return names
|
||||
end
|
||||
|
|
@ -106,7 +123,8 @@ local function resolve_profile(requested)
|
|||
local known = known_profile_names()
|
||||
local listed = #known > 0 and table.concat(known, ", ") or "(none defined)"
|
||||
error(string.format(
|
||||
"terminal profile %q is not defined; known profiles: %s", name, listed), 0)
|
||||
"terminal profile %s is not defined; known profiles: %s",
|
||||
describe_name(name), listed), 0)
|
||||
end
|
||||
return validate_profile(name, profile)
|
||||
end
|
||||
|
|
|
|||
|
|
@ -396,8 +396,9 @@ If it does not, stop and repair the remote/fetch configuration.
|
|||
Stage 1's branch. Two stages, two branches, two PRs; **no protocol
|
||||
change**.
|
||||
- **Stage 1 = `githubsucks/terminal-config`**, worktree
|
||||
`../pmacs-terminal-config`, based on `githubsucks/main` @ `d152120`.
|
||||
Profiles, scrollback, escape key, and the `C-c t` opening binding.
|
||||
`../pmacs-terminal-config`, based on `githubsucks/main` @ `d152120`
|
||||
and merged up to `ccf29e3` during review round 1. Profiles,
|
||||
scrollback, escape key, and the `C-c t` opening binding.
|
||||
- **Stage 2 = `terminal-copy-mode`, not started.** Branch it off `main`
|
||||
after Stage 1 merges: no dependency, but both edit
|
||||
`builtin/runtime/terminal.lua`.
|
||||
|
|
@ -424,16 +425,63 @@ If it does not, stop and repair the remote/fetch configuration.
|
|||
`Ctrl-X` is invisible; and the probe **counts occurrences** rather than
|
||||
testing presence, because a single-character probe collides with the
|
||||
child's own banner text.
|
||||
- Verification on this branch (against the committed tree): `cargo fmt
|
||||
--check` clean; strict workspace Clippy clean; 1,832 default + 2,009
|
||||
CRDT library tests; `terminal_config_acceptance` 10/10 in **both**
|
||||
configurations; vterm Stage 1/2/3 9+10 / 6+6 / 5+9; config registry 16;
|
||||
bottom-panel 46; listview 6; compile 67 (isolated config); M4 121;
|
||||
required GPU 202; **isolated-config workspace sweep 3,262 across 94
|
||||
suites**; `git diff --check` clean.
|
||||
- **Review round 1 (2026-07-25) — five findings, all real, all fixed.**
|
||||
One blocker and two majors were the same failure in three places: a
|
||||
claim asserted somewhere cheaper than where it lives.
|
||||
- *Blocker — `COHERENCE.md` was stale in four places, not the three
|
||||
reported.* Step 8 still read "no keybinding"; §11 still read "five
|
||||
settings"; and §6's dispatch table still cited
|
||||
`is_terminal_escape_chord`, **a symbol this PR deletes**. §25 makes
|
||||
that update ride the PR. A PR that changes audited ground truth has
|
||||
to re-grep the audit for its own symbols, not only for its topic.
|
||||
- *Major — acceptance 5 was vacuous.* It asserted a registry
|
||||
round-trip, so it stayed green with the setting's **only** consumer
|
||||
deleted. It now opens a real terminal whose child overflows the
|
||||
24-row screen, scrolls the view to its oldest retained row, and
|
||||
asserts `LINE001` is present at 10,000 and absent at 0. **Asserting
|
||||
a value was stored is not asserting anything reads it.**
|
||||
- *Major — acceptance 8a asserted the session count, not the cache.*
|
||||
An editor-side map with no purge hook — the exact rejected design —
|
||||
leaks *while* sessions drain, so it passed. Fixed with a
|
||||
`TerminalManager::escape_caches()` seam. **A lifecycle claim needs a
|
||||
lifecycle observable.**
|
||||
- *Moderate — `table.sort` over user-controlled profile keys.* A
|
||||
table holding both a string and a numeric key raised `attempt to
|
||||
compare number with string` **on the unknown-profile path**,
|
||||
replacing the diagnostic being asked for; `%q` raised likewise on a
|
||||
non-string `profile` argument. Both are partial functions applied to
|
||||
user input **on a diagnostic path** — the error reporter was the
|
||||
thing that failed.
|
||||
- *Minor — the committed framing still said "not yet approved".*
|
||||
- **Three new bites, each falsified by revert**: deleting the scrollback
|
||||
consumer fails acc5 (and only acc5); restoring the raw-key sort
|
||||
reproduces `attempt to compare string with number` verbatim; and
|
||||
implementing the rejected editor-side map fails the new acc8a at
|
||||
`left: 2, right: 1` **while passing the old session-count version** —
|
||||
which is the review finding demonstrated rather than argued.
|
||||
- Verification after the round-1 fixes, on the tree merged with
|
||||
`githubsucks/main` @ `ccf29e3`: `cargo fmt --check` clean; strict
|
||||
workspace Clippy clean; 1,832 default + 2,009 CRDT library tests;
|
||||
`terminal_config_acceptance` **12/12 in both configurations**; vterm
|
||||
Stage 1/2 9+10 / 6+6; config registry 16+16; bottom-panel Stage 1
|
||||
46+46; M4 121; required GPU 202; `git diff --check` clean.
|
||||
- `compile_mode_acceptance` fails 11/67 against the **real** user
|
||||
config and passes 67/67 with an isolated `XDG_CONFIG_HOME` — the
|
||||
known pre-existing trap, not this branch.
|
||||
- **`vterm_stage3_acceptance::a37` fails on this machine — and fails
|
||||
identically on the PR's own base `d152120`**, so it is not this
|
||||
branch's regression. It is load-sensitive: it passed at `d152120`
|
||||
once and failed at that same commit twenty minutes later, with a
|
||||
second agent saturating the machine with `rustc` in between. Two
|
||||
ways it lies, both worth knowing: it **silently returns `ok` when
|
||||
`pmacs-gpu` is not built** in the same target dir (only
|
||||
`PMACS_REQUIRE_GPU=1` promotes that skip to a failure, and the gate
|
||||
list applies that flag to `-p pmacs-gpu`, a *different* package), and
|
||||
it is **crdt-gated, so CI has never run it at all**. A green a37 in
|
||||
a gate log means nothing unless the binary was built and the flag
|
||||
was set. Needs its own lane; see the CI `crdt`-coverage lane on #168.
|
||||
- `pmacs-gpu` itself failed 201/202 once under the same load and passed
|
||||
202/202 on immediate rerun.
|
||||
|
||||
## Bottom-panel lane (Arc 7) — Stage 1 MERGED; Stage 2 (GPU band) is next
|
||||
|
||||
|
|
|
|||
|
|
@ -1,7 +1,9 @@
|
|||
# Terminal configuration and copy mode
|
||||
|
||||
**Revision 4 — scouted against canonical `main` @ `b889873` (protocol v20),
|
||||
2026-07-25. Not yet approved; no branch, no implementation.**
|
||||
2026-07-25. APPROVED after four review rounds. Stage 1 is implemented on
|
||||
branch `terminal-config` (PR #173); Stage 2 (`terminal-copy-mode`) is
|
||||
framed but not started, and branches off `main` after Stage 1 merges.**
|
||||
|
||||
Revision 4 gives the escape-key cache an owner and a lifecycle (Q#TC4c) —
|
||||
revision 3 named the key but not the storage, and two implementations
|
||||
|
|
@ -503,6 +505,15 @@ additive, on its own binding, and does not replace scroll-and-select.
|
|||
error that **lists the known profile names**, and creates no buffer,
|
||||
session, or process. An explicitly passed unknown `profile` fails the same
|
||||
way **even when `terminal.default-profile` is valid** (Q#TC3a).
|
||||
2a. That diagnostic is **total over a malformed profiles table** (review round
|
||||
1). `pmacs.terminal.profiles` is a raw user table, so listing its names must
|
||||
not assume its keys are comparable and rendering a requested name must not
|
||||
assume it is a string: a table holding both a string and a numeric key made
|
||||
`table.sort` raise `attempt to compare number with string` *on the
|
||||
unknown-profile path*, replacing the exact error being asked for, and `%q`
|
||||
raises on a non-string `profile` argument. Both are partial functions
|
||||
applied to user input on a diagnostic path — the failure class is
|
||||
"the error reporter is the thing that fails".
|
||||
3. Field-by-field resolution follows Q#TC3a: explicit open field beats profile
|
||||
field beats scalar setting beats `$SHELL`. `env` **merges**, with explicit
|
||||
entries overriding profile entries of the same name.
|
||||
|
|
@ -623,6 +634,19 @@ Full gate suite per `CLAUDE.md` for each PR separately, plus:
|
|||
chord unreachable); **10** (its failure mode is a terminal nobody can
|
||||
escape); and **16/17** (a read-only buffer that silently accepts an edit on
|
||||
both sides).
|
||||
- **The observation seams the cache pins need are `escape_parses` (how often)
|
||||
and `escape_caches` (how many are still held).** Neither is inferable from
|
||||
behavior: for a *valid* setting a correct per-session cache and a leaking
|
||||
editor-side map produce identical keystroke results, and both leave the
|
||||
session count draining normally. Review round 1 caught 8a asserting the
|
||||
session count instead — which the unpurged-map bite passes, since a map with
|
||||
no purge hook leaks *while* sessions drain. A lifecycle claim needs a
|
||||
lifecycle observable; the count of live sessions is not one.
|
||||
- **Criterion 5 must open a real terminal and read back retained history.**
|
||||
Round 1 caught it asserting a registry round-trip instead, which is a test of
|
||||
the registry: it stays green with the setting's only consumer deleted. The
|
||||
same shape to watch for anywhere — *asserting that a value was stored is not
|
||||
asserting that anything reads it*.
|
||||
- **Do not gate the new suites on `#[cfg(feature = "crdt")]` unless a test
|
||||
genuinely needs CRDT.** CI never enables that feature, so a suite gated that
|
||||
way is written and then never run — 264 tests are currently dark for exactly
|
||||
|
|
|
|||
|
|
@ -607,7 +607,7 @@ impl TerminalManager {
|
|||
let already = previously_reported.as_deref() == Some(spelling);
|
||||
let message = (!already).then(|| {
|
||||
format!(
|
||||
"terminal.escape-key {spelling:?} is not a valid chord ({error}); using C-c"
|
||||
"terminal.escape-key {spelling:?} is not a valid chord ({error}); using C-c"
|
||||
)
|
||||
});
|
||||
(fallback, Some(spelling.to_owned()), message)
|
||||
|
|
@ -633,6 +633,24 @@ impl TerminalManager {
|
|||
self.escape_parses
|
||||
}
|
||||
|
||||
/// How many terminals currently hold a cached escape chord.
|
||||
///
|
||||
/// The LIFETIME half of Q#TC4c's cache contract, which `escape_parses`
|
||||
/// cannot cover: parse counting says a valid setting is read once, but
|
||||
/// says nothing about whether the cache is ever released. Because the
|
||||
/// cache lives on [`TerminalSession`], this count falls with the
|
||||
/// session set by construction — which is exactly the property worth
|
||||
/// pinning, since the rejected alternative (an editor-side
|
||||
/// `HashMap<BufferId, EscapeCache>`) 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,
|
||||
|
|
|
|||
|
|
@ -32,13 +32,18 @@ fn eval_err(state: &EditorState, src: &str) -> String {
|
|||
}
|
||||
}
|
||||
|
||||
fn screen_text(state: &EditorState, buffer: pmacs::buffer::BufferId) -> String {
|
||||
let manager = state.terminal_manager.borrow();
|
||||
let Some(snapshot) = manager.snapshot(buffer) else {
|
||||
return String::new();
|
||||
};
|
||||
/// The viewport every test projects through. Deliberately SHORTER than
|
||||
/// the 24-row screen a terminal opens with, so "scroll to the oldest
|
||||
/// retained row" has somewhere to go even when nothing is retained —
|
||||
/// which is what makes the two scrollback arms differ by content rather
|
||||
/// than by whether scrolling was possible at all.
|
||||
fn viewport() -> CellSize {
|
||||
CellSize::new(10, 40)
|
||||
}
|
||||
|
||||
fn cells_to_text(cells: &[pmacs::cell::Cell]) -> String {
|
||||
let mut text = String::new();
|
||||
for cell in &snapshot.cells {
|
||||
for cell in cells {
|
||||
match &cell.glyph {
|
||||
Glyph::Char(c) => text.push(*c),
|
||||
Glyph::Cluster(b) => text.push_str(&String::from_utf8_lossy(b)),
|
||||
|
|
@ -48,6 +53,33 @@ fn screen_text(state: &EditorState, buffer: pmacs::buffer::BufferId) -> String {
|
|||
text
|
||||
}
|
||||
|
||||
fn screen_text(state: &EditorState, buffer: pmacs::buffer::BufferId) -> String {
|
||||
let manager = state.terminal_manager.borrow();
|
||||
let Some(snapshot) = manager.snapshot(buffer) else {
|
||||
return String::new();
|
||||
};
|
||||
cells_to_text(&snapshot.cells)
|
||||
}
|
||||
|
||||
/// Text a view actually shows, which is where retained history is
|
||||
/// visible at all — the live `screen_text` above always reads the tail.
|
||||
fn view_text(state: &EditorState, key: TerminalViewKey) -> String {
|
||||
let mut manager = state.terminal_manager.borrow_mut();
|
||||
manager
|
||||
.snapshot_for_view(key, viewport())
|
||||
.map(|snapshot| cells_to_text(&snapshot.cells))
|
||||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
/// Scroll a view to its OLDEST retained row and read it back.
|
||||
fn oldest_view_text(state: &EditorState, key: TerminalViewKey) -> String {
|
||||
state
|
||||
.terminal_manager
|
||||
.borrow_mut()
|
||||
.scroll_view(key, viewport(), i32::MAX);
|
||||
view_text(state, key)
|
||||
}
|
||||
|
||||
fn tick_until(state: &mut EditorState, needle: &str, buffer: pmacs::buffer::BufferId) -> bool {
|
||||
let deadline = Instant::now() + Duration::from_secs(5);
|
||||
loop {
|
||||
|
|
@ -71,7 +103,7 @@ fn focus_terminal(state: &EditorState, buffer: pmacs::buffer::BufferId) -> Windo
|
|||
let mut manager = state.terminal_manager.borrow_mut();
|
||||
manager.register_view(key);
|
||||
manager.claim_controller(key);
|
||||
let _ = manager.snapshot_for_view(key, CellSize::new(10, 40));
|
||||
let _ = manager.snapshot_for_view(key, viewport());
|
||||
window
|
||||
}
|
||||
|
||||
|
|
@ -223,6 +255,44 @@ fn acc2_unknown_profile_lists_known_names_and_creates_nothing() {
|
|||
assert_eq!(state.terminal_manager.borrow().len(), 0);
|
||||
}
|
||||
|
||||
/// Acceptance 2 (malformed table): `pmacs.terminal.profiles` is a raw
|
||||
/// user table, so a diagnostic that walks its keys must be total over
|
||||
/// them. A table holding both a string and a numeric key made
|
||||
/// `table.sort` raise "attempt to compare number with string" — on the
|
||||
/// unknown-profile path, replacing the exact error being asked for.
|
||||
#[test]
|
||||
fn acc2_malformed_profile_keys_do_not_mask_the_unknown_profile_error() {
|
||||
let state = EditorState::new();
|
||||
exec(&state, CAT_PROFILE);
|
||||
exec(
|
||||
&state,
|
||||
r#"pmacs.terminal.profiles[1] = { command = "/bin/sh" }"#,
|
||||
);
|
||||
|
||||
let err = eval_err(
|
||||
&state,
|
||||
r#"return pmacs.terminal.open { profile = "ghost" }"#,
|
||||
);
|
||||
assert!(
|
||||
err.contains("ghost") && err.contains("echo"),
|
||||
"the unknown-profile error must survive a malformed table: {err}"
|
||||
);
|
||||
assert!(
|
||||
!err.contains("attempt to compare"),
|
||||
"listing known profiles must not raise: {err}"
|
||||
);
|
||||
|
||||
// Rendering the REQUESTED name is partial too: `%q` raises on a
|
||||
// table, and the name arrives straight from the caller.
|
||||
let err = eval_err(&state, r"return pmacs.terminal.open { profile = {} }");
|
||||
assert!(
|
||||
err.contains("is not defined") && err.contains("known profiles"),
|
||||
"a non-string profile name must render, not raise: {err}"
|
||||
);
|
||||
|
||||
assert_eq!(state.terminal_manager.borrow().len(), 0);
|
||||
}
|
||||
|
||||
/// Acceptance 3: explicit beats profile beats setting beats `$SHELL`, and
|
||||
/// `env` MERGES rather than replacing.
|
||||
#[test]
|
||||
|
|
@ -282,10 +352,77 @@ fn acc3_acc4_explicit_command_wins_and_empty_default_means_no_profile() {
|
|||
state.process_supervisor.borrow_mut().shutdown();
|
||||
}
|
||||
|
||||
/// Acceptance 5: scrollback resolves from the setting, is overridden by an
|
||||
/// explicit value, and `0` is legal.
|
||||
/// A child that overflows the 24-row screen and then goes quiet, so its
|
||||
/// early output can only still be found in RETAINED HISTORY. Zero-padded
|
||||
/// so `LINE001` is not a substring of `LINE100`.
|
||||
const FILL_PROFILE: &str = r#"
|
||||
pmacs.terminal.profiles.fill = {
|
||||
command = "/bin/sh",
|
||||
args = { "-c",
|
||||
"i=1; while [ $i -le 200 ]; do printf 'LINE%03d\r\n' $i; i=$((i+1)); done; printf 'DONE\r\n'; exec cat" },
|
||||
}
|
||||
"#;
|
||||
|
||||
/// Acceptance 5: the scrollback SETTING reaches the screen's retained
|
||||
/// history, an explicit spec value overrides it, and `0` is legal.
|
||||
///
|
||||
/// Asserted end to end, through a real child and a real view, rather
|
||||
/// than by reading the value back out of the registry: a registry
|
||||
/// round-trip is a test of the registry, and would stay green with the
|
||||
/// setting's only consumer (`terminal.lua`'s `resolved.scrollback_rows`
|
||||
/// fallback) deleted outright.
|
||||
#[test]
|
||||
fn acc5_scrollback_setting_override_and_bounds() {
|
||||
fn acc5_scrollback_setting_reaches_retained_history() {
|
||||
let mut state = EditorState::new();
|
||||
exec(&state, FILL_PROFILE);
|
||||
|
||||
// Arm 1: `0` is legal, and means the early rows are GONE.
|
||||
exec(&state, r#"pmacs.config.set("terminal.scrollback-rows", 0)"#);
|
||||
let none = open_cat_terminal(&state, r#"profile = "fill""#);
|
||||
assert!(tick_until(&mut state, "DONE", none), "child finished");
|
||||
let window = focus_terminal(&state, none);
|
||||
let none_key = TerminalViewKey::new(FrontendId::LOCAL, window, none);
|
||||
let oldest = oldest_view_text(&state, none_key);
|
||||
assert!(
|
||||
!oldest.contains("LINE001"),
|
||||
"with scrollback 0 the oldest retained row must not be the \
|
||||
child's first line: {oldest:?}"
|
||||
);
|
||||
|
||||
// Arm 2: a large setting retains it, reachable by scrolling back.
|
||||
exec(
|
||||
&state,
|
||||
r#"pmacs.config.set("terminal.scrollback-rows", 10000)"#,
|
||||
);
|
||||
let kept = open_cat_terminal(&state, r#"profile = "fill""#);
|
||||
assert!(tick_until(&mut state, "DONE", kept), "child finished");
|
||||
let window = focus_terminal(&state, kept);
|
||||
let kept_key = TerminalViewKey::new(FrontendId::LOCAL, window, kept);
|
||||
let oldest = oldest_view_text(&state, kept_key);
|
||||
assert!(
|
||||
oldest.contains("LINE001"),
|
||||
"with scrollback 10000 the first line must survive in history: \
|
||||
{oldest:?}"
|
||||
);
|
||||
|
||||
// Arm 3: an explicit spec value beats the setting, which is still 10000.
|
||||
let overridden = open_cat_terminal(&state, r#"profile = "fill", scrollback_rows = 0"#);
|
||||
assert!(tick_until(&mut state, "DONE", overridden), "child finished");
|
||||
let window = focus_terminal(&state, overridden);
|
||||
let overridden_key = TerminalViewKey::new(FrontendId::LOCAL, window, overridden);
|
||||
let oldest = oldest_view_text(&state, overridden_key);
|
||||
assert!(
|
||||
!oldest.contains("LINE001"),
|
||||
"an explicit scrollback_rows = 0 must beat the setting: {oldest:?}"
|
||||
);
|
||||
|
||||
state.process_supervisor.borrow_mut().shutdown();
|
||||
}
|
||||
|
||||
/// Acceptance 5 (bounds): the registered range rejects out-of-range
|
||||
/// values, and `0` is inside it rather than a disabled sentinel.
|
||||
#[test]
|
||||
fn acc5_scrollback_bounds() {
|
||||
let state = EditorState::new();
|
||||
exec(&state, r#"pmacs.config.set("terminal.scrollback-rows", 0)"#);
|
||||
assert_eq!(
|
||||
|
|
@ -397,6 +534,11 @@ fn acc7_acc8_acc8a_per_terminal_escape_cache_identity_and_lifecycle() {
|
|||
);
|
||||
assert!(escape_was_armed(&mut state, b, 'N'), "B primes on its C-b");
|
||||
let primed = state.terminal_manager.borrow().escape_parses();
|
||||
assert_eq!(
|
||||
state.terminal_manager.borrow().escape_caches(),
|
||||
2,
|
||||
"each primed terminal holds its own cache"
|
||||
);
|
||||
|
||||
// Acceptance 7 — BOTH directions. Asserting only that A still works
|
||||
// after A->B->A is not enough: an epoch-only cache hands whichever
|
||||
|
|
@ -442,6 +584,13 @@ fn acc7_acc8_acc8a_per_terminal_escape_cache_identity_and_lifecycle() {
|
|||
);
|
||||
|
||||
// Acceptance 8a: the cache dies with its terminal.
|
||||
//
|
||||
// Waiting for the SESSION count to fall is not the assertion — a
|
||||
// session set that drains while an editor-side `HashMap<BufferId,
|
||||
// EscapeCache>` keeps its entry (the rejected implementation named
|
||||
// in Q#TC4c, which has no purge hook) satisfies it exactly. The
|
||||
// discriminating observable is the CACHE count, which such a map
|
||||
// would hold at its high-water mark of 2.
|
||||
let sessions_before = state.terminal_manager.borrow().len();
|
||||
exec(&state, "pmacs.terminal.terminate(TERM_A)");
|
||||
exec(&state, "pmacs.buffer.kill(TERM_A)");
|
||||
|
|
@ -452,10 +601,31 @@ fn acc7_acc8_acc8a_per_terminal_escape_cache_identity_and_lifecycle() {
|
|||
state.tick_processes();
|
||||
assert!(
|
||||
Instant::now() < deadline,
|
||||
"killing the terminal must remove its session, and with it the cache"
|
||||
"killing the terminal must remove its session"
|
||||
);
|
||||
thread::sleep(Duration::from_millis(20));
|
||||
}
|
||||
assert_eq!(
|
||||
state.terminal_manager.borrow().escape_caches(),
|
||||
1,
|
||||
"killing terminal A must drop ITS cache, not merely its session"
|
||||
);
|
||||
|
||||
// ...and the surviving cache is B's, so the right one was dropped.
|
||||
focus_terminal(&state, b);
|
||||
state.dispatch_key(
|
||||
FrontendId::LOCAL,
|
||||
KeyEvent::new(KeyCode::Char('b'), KeyModifiers::CONTROL),
|
||||
);
|
||||
assert!(
|
||||
escape_was_armed(&mut state, b, 'T'),
|
||||
"terminal B must still escape on its own C-b after A was killed"
|
||||
);
|
||||
assert_eq!(
|
||||
state.terminal_manager.borrow().escape_parses(),
|
||||
primed,
|
||||
"B's surviving cache must not have been reparsed"
|
||||
);
|
||||
state.process_supervisor.borrow_mut().shutdown();
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue