diff --git a/COHERENCE.md b/COHERENCE.md index 7f8f456..758b3e2 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -95,7 +95,7 @@ remain open to them. | § | Concern | Grade | One-line state | |---|---|---|---| -| 2 | Golden product journey | **Broken at entry** | `pmacs .` exits 1; only "launch" and "edit" pass cleanly zero-config | +| 2 | Golden product journey | **Runs to step 5** | `pmacs .` opens the directory (Journey Stage 1a); thin from step 6 on | | 3 | Zero-configuration state | **Partial** | Defaults genuinely strong; missing-tool failure is silent, not graceful | | 4 | Progressive disclosure | **Inverted** | The advanced level is real; the beginner level is the missing one | | 5 | Unified discoverability | **Substrate without surface** | Best-in-class registration metadata; almost no way for a user to reach it | @@ -112,7 +112,7 @@ remain open to them. | 16 | Semantic frontend | **Strong** | v6..=v20 negotiated protocol; degradation practiced; TUI/GPU share the model | | 17 | Distribution | **Missing** | CI is test-only; no binaries, channels, checksums, or update path | | 18 | Onboarding | **Missing** | No welcome, no tutorial; `C-h` deletes a word; `M-x` is the only door in | -| 19 | Coherence acceptance tests | **Missing (culture ready)** | Superb per-arc acceptance discipline; zero cross-subsystem journey tests | +| 19 | Coherence acceptance tests | **Started** | `tests/journey_acceptance.rs` exists (steps 2, 3, 5); the other five scenarios are still unwritten | Three cross-cutting patterns explain most of the table; they are detailed in §1.1–§1.3: **substrate without surface**, **the silence @@ -339,7 +339,8 @@ the journey. ### Ground truth: the journey today -**Grade: broken at step 3.** Verified empirically at audit time: +**Grade: reaches step 5; thin from step 6 on.** Was **broken at step 3** +at audit time: ``` $ ./target/release/pmacs . @@ -347,15 +348,23 @@ pmacs: Is a directory (os error 21) EXIT=1 ``` -The literal first arrow of the diagram above fails. `load_file` +The literal first arrow of the diagram above failed. `load_file` (`src/file_io.rs:81-87`) does `File::open` (succeeds on a directory) then `read_to_end` → EISDIR, which is not `NotFound`, so -`EditorState::open` returns `Err` and `main` prints and exits -(`src/main.rs:411-414`). Multiple file arguments are also rejected -(`"multiple files not yet supported"`, `src/main.rs:227`). Everything -from step 6 onward is gated on a file being open, and the only -zero-config way to open one is naming it on the command line — which -requires already knowing the path. +`EditorState::open` returned `Err` and `main` printed and exited. + +**Journey Stage 1a fixed that arrow** (`docs/journey-stage1a-framing.md`). +`resolve_target_buffer` now answers `ResolvedTarget::Directory` *ahead* +of the load, `pmacs .` lists the directory in dired, `RET` visits a +file, and a self-insert lands in it — steps 3 and 5 run end to end, +pinned by `tests/journey_acceptance.rs`. Which surface opens a directory +is a `path.open-directory` chain with dired as a replaceable fallback, +so this did not grow a second directory surface. + +Still true: multiple file arguments are rejected (`"multiple files not +yet supported"`, `src/main.rs:227`), and everything from step 6 onward +is gated on a file being open — but the zero-config way to open one is +no longer "already know the path". Full verdict table: @@ -363,7 +372,7 @@ Full verdict table: |---|---|---|---| | 1 | Install | **Partial** | Source build only: `cargo build --release --workspace --features pmacs/crdt` (`README.md`). No binaries, no packaging. Runtime deps (`/bin/sh`, git, tar, coreutils) documented, never checked at runtime | | 2 | Launch unconfigured | **Works** | `EditorState::new()` → empty `*scratch*`; missing config is not an error (`src/config.rs:7-9`); recentf/saveplace/autosave default-on | -| 3 | Open real project | **Missing at the CLI** | `pmacs .` still exits 1 (above): `load_file` does `File::open` (which succeeds on a directory) then `read_to_end` → EISDIR, which is not `NotFound`, so `resolve_target_buffer`'s create-a-`[new file]` arm never fires. Dired Stage 1 (merged #165) supplies the buffer a directory should resolve *to*; routing `pmacs .` into it is Journey Stage 1's work, which must not invent a second directory surface | +| 3 | Open real project | **Works at the CLI** | Journey Stage 1a: `resolve_target_buffer` answers `ResolvedTarget::Directory` before the EISDIR-producing load, and `EditorState::open` / the daemon bootstrap dispatch the `path.open-directory` chain, whose fallback is dired (#165's buffer, reached rather than duplicated). Startup no longer fails: an unreadable directory, a crashed resolver, and a cleared handler all report on the status line and leave the session running. Because the listing is async and the bootstrap is synchronous, the commit runs against a destination captured at request time (`pmacs.window.commit_to`) rather than against the ambient frontend | | 4 | Understand interface | **Partial** | Mode line gives name/modified/L:C/scroll + mode/LSP/terminal segments; but no welcome text (`EditorCore::new` sets `status: String::new()`), no cheat sheet, and `C-h` deletes a word (§18) | | 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 | @@ -1225,14 +1234,23 @@ Primitive-by-primitive against the list above: `compile.lua`'s own comment admits ("command/menu undo stays dispatchable"). `Buffer::set_generated_contents` (write + discard history + assert `read_only`, in one authorized call) now fixes this - for the terminal snapshot; `*compilation*` and listview panels have - not yet adopted it and remain emptiable. **A second half of the same + for the terminal snapshot; **four writer mechanisms have not yet adopted + it and remain emptiable** — listview panels; `compile.lua`'s + `ensure_slot`, which serves `*compilation*` **and** `*shell-command*`; + the independent `*search-results*` panel in + `builtin/commands/default.lua`; and dired buffers. All pair an erroring + intercept with `bypass_intercept` writes over a still-writable rope. + (`*workers*`, `*help*` and `*buffer-list*` are generated but do not use + this idiom.) **A second half of the same caveat, found in round 3: a rope write is only half of an edit.** The owner-authorized write must be fanned out to the windows showing the buffer and queued for replica mirrors, or the displaying window keeps a line index describing the previous contents and the next paint indexes the new rope with stale ranges. Adoption is therefore not a - one-line swap. + one-line swap — and the three appending buffers (`*compilation*`, + `*shell-command*`, `*search-results*`) need a streaming variant of the + primitive that does not exist yet. Listview and dired already write + whole-buffer replaces and are the cheap half. - **Diagnostics collection** ✓ — `DiagnosticStore` + signs + unified `error.next` source. - **Transient selector** ✓ — the minibuffer (though its `source` @@ -1446,20 +1464,24 @@ subsystems, complementing (not replacing) subsystem tests: ### Ground truth -**Grade: missing — but the culture that would make them excellent is the -project's strongest process asset.** +**Grade: started — the first suite exists; five of the six scenarios +above do not.** -Zero cross-subsystem journey tests exist. Every acceptance suite in the -tree pins one subsystem's contract (superbly — bite-verified, -falsified-by-revert, vacuity-checked). Several of the scenarios above -are currently *untestable* because the behavior doesn't exist (install -in-session, disable, open a directory); the ones that are testable -(first launch, command discovery, worker cancellation, remote -attach/reconnect) could be written today and would immediately pin the -journey against regression. The first coherence acceptance suite should -be the §2 journey itself, growing a step at a time as steps become -real — that is how "the journey is a release gate" stops being -aspirational. +At audit time zero cross-subsystem journey tests existed. **Journey +Stage 1a created `tests/journey_acceptance.rs`**, the §2 journey itself, +seeded with steps 2 (launch unconfigured), 3 (open a real project), and +5 (edit immediately), and declared a ratchet: stages add rows, none +removes them. That is the "first launch" scenario, partially — missing +tools still have no actionable guidance to assert. + +The rest is unchanged. Every other acceptance suite in the tree pins one +subsystem's contract (superbly — bite-verified, falsified-by-revert, +vacuity-checked). Command discovery, workspace lifecycle, worker +ownership, package lifecycle, and remote execution have no +cross-subsystem suite; several remain *untestable* because the behavior +doesn't exist (install in-session, disable). Steps 6–12 join +`journey_acceptance.rs` as later stages make them real — that is how +"the journey is a release gate" stops being aspirational. (Related lesson already in the handoff: `compile_mode_acceptance` accidentally reads the real user config — an *unintentional* @@ -1477,14 +1499,18 @@ missing runtime entity — a real arc). ### Priority 1: Protect the golden product journey Establish the end-to-end workflow; treat regressions as release -blockers. **State: broken at step 3 (§2). Mostly wiring, and unusually -cheap:** directory-argument handling (the remaining half of step 3 — -dired Stage 1 landed the buffer it should resolve to); a find-file -surface (**done**: #162 open-by-path, #165 browsing); surfacing the -LSP spawn failure with guidance (§1.2); a +blockers. **State: runs to step 5; thin from step 6 (§2). Mostly wiring, +and unusually cheap:** directory-argument handling (**done**: Journey +Stage 1a); a find-file surface (**done**: #162 open-by-path, #165 +browsing); surfacing the LSP spawn failure with guidance (§1.2); a compile keybinding + `cargo build`/`test` default from the existing -`ProjectKind::Cargo`; a terminal keybinding; a welcome buffer. The -journey acceptance suite (§19) is the ratchet that keeps it fixed. +`ProjectKind::Cargo`; a terminal keybinding (**done**: `C-c t`, #173); a +welcome buffer. The journey acceptance suite (§19) is the ratchet that +keeps it fixed — it **exists now** (`tests/journey_acceptance.rs`, +Stage 1a), seeded with steps 2, 3, and 5. + +Journey Stage 1b is the named remainder: the compile binding + Cargo +defaults, LSP spawn guidance, and the welcome buffer. ### Priority 2: Make workspace and location explicit @@ -1548,11 +1574,13 @@ Candidate arc cuts, honoring one-feature-one-branch-one-PR and the framing workflow (each needs its own scout + framing before any implementation — this list is direction, not commitment): -1. **Journey Stage 1** (P1): directory open + compile defaults + - LSP-failure surfacing + bindings + welcome buffer + the first - journey acceptance suite. Dired Stage 1 has landed (#165), so the - buffer a directory resolves *to* already exists; this arc routes - `pmacs .` into it rather than growing a second directory surface. +1. **Journey Stage 1** (P1): split at the new-Rust-primitive line. + **Stage 1a — landed**: directory open, the `EditorState::open` → + `resolve_target_buffer` unification, the destination-scope substrate, + and the first journey acceptance suite. It routes `pmacs .` into + #165's dired buffer rather than growing a second directory surface. + **Stage 1b — remaining**: compile defaults, LSP-failure surfacing, + bindings, welcome buffer. 2. **Discovery surface** (P4): the describe/list/where-is command family, M-x rich rows, help unification, help prefix. 3. **Transient keymap layer** (§6): the overlay scope + lifetime diff --git a/builtin/hooks/default.lua b/builtin/hooks/default.lua index 7fe3a0a..4fabfe9 100644 --- a/builtin/hooks/default.lua +++ b/builtin/hooks/default.lua @@ -61,6 +61,21 @@ define { kind = "all-must-succeed", } +define { + name = "path.open-directory", + description = "Fired when a directory path is opened (Journey Stage 1a). " .. + "Receives the canonical absolute path and an opaque " .. + "destination. Return false to CLAIM the directory and stop " .. + "the fan-out; return nothing to decline. No builtin " .. + "subscribes -- because hook callbacks only ever append, a " .. + "subscribing builtin would always claim before any user " .. + "listener could run, so this hook is the user's chain and " .. + "pmacs.path.directory_handler is the default surface it " .. + "falls back to. A callback that RAISES stops the chain and " .. + "suppresses that fallback.", + kind = "short-circuit", +} + define { name = "editor.before-quit", description = "Fired before the editor exits. Return false to veto.", diff --git a/builtin/runtime/async.lua b/builtin/runtime/async.lua index 94555c7..af74cc1 100644 --- a/builtin/runtime/async.lua +++ b/builtin/runtime/async.lua @@ -77,6 +77,17 @@ end -- inside a coroutine spawned by pmacs.async --- a bare call from main -- thread will raise on the first yield. function Handle:await() + -- Journey Stage 1a (Q#JR14b): `pmacs.window.commit_to` scopes the + -- acting frontend for the dynamic extent of its callback, using an + -- RAII guard on the Rust stack. Yielding out of that extent would + -- restore the scope while this coroutine is still parked, so the rest + -- of the commit would resume ambient -- silently reintroducing the + -- misrouting the scope exists to prevent. Do the awaiting BEFORE + -- entering the commit, which is what dired does with its listing. + if async_mod._in_commit_scope() then + error("await: cannot await inside pmacs.window.commit_to; " .. + "await first, then commit") + end if not async_mod._is_complete(self._id) then -- Yield self so pmacs.async's step() can park us. R46 carve-out: -- this `coroutine.yield` is runtime code; package code uses diff --git a/builtin/runtime/dired.lua b/builtin/runtime/dired.lua index 9c6bc92..c8054fe 100644 --- a/builtin/runtime/dired.lua +++ b/builtin/runtime/dired.lua @@ -571,7 +571,15 @@ end -- deliberately so (Q#DR10): the next directory is the same kind of -- thing as the current one and belongs in the same slot, while a file -- is not a dired buffer and belongs in the document area. -local function display(handle, opts, departed) +-- +-- `captured` (Journey Stage 1a, Q#JR14) is the destination window a +-- background open must land in. It is NOT the same as "wherever the +-- scoped frontend is looking now": the scope fixes the *frontend*, and +-- within one frontend the selected window can still have moved to +-- another split while the listing was in flight. The preflight cannot +-- catch that -- the captured window is still live and still holds its +-- captured buffer -- so honoring it is this function's job. +local function display(handle, opts, departed, captured) local side = nil if departed ~= nil then -- Dired's own window, not the request's: walking a tree in a side @@ -587,6 +595,11 @@ local function display(handle, opts, departed) -- both the substrate's documented policy and Emacs's, so dired does -- not try to unpin the user's panel. pmacs.window.display(handle.buf, { side = side, select = true }) + elseif captured ~= nil then + -- `select = true` because the rest of the commit -- seat_cursor via + -- `pmacs.editor.move_to_line` -- acts on the frontend's ACTIVE + -- window, so the seat would land in the wrong window otherwise. + pmacs.window.display(handle.buf, { window = captured, select = true }) else pmacs.window.switch_buffer(handle.buf) end @@ -598,7 +611,7 @@ end pmacs.dired = pmacs.dired or {} -local OPEN_OPTS = { display = true, select_name = true } +local OPEN_OPTS = { display = true, select_name = true, dest = true } -- Open `path`'s dired buffer, replacing `departed` (a handle) in the -- window it occupies when this is a navigation rather than a fresh @@ -629,36 +642,76 @@ local function open_directory(path, opts, departed) local sort_mode = (handle_for_path(canonical) or {}).sort_mode or SORT_MODES[1] local entries, errors = read_listing(canonical, sort_mode) - local handle = claim_handle(canonical) - handle.entries = entries - handle.errors = errors - handle.sort_mode = sort_mode + -- Everything from here down MUTATES: it claims or finds a handle, + -- creates a buffer, reads the ambient buffer for `prev`, and paints. + -- None of it is undoable, and none of it may run against a + -- destination that has gone away -- so when the caller captured one + -- (Journey Stage 1a, Q#JR14), the whole commit runs inside + -- `pmacs.window.commit_to`, which validates the destination BEFORE + -- invoking this and scopes the acting frontend for its extent. + -- + -- Note the await above is deliberately OUTSIDE the commit: awaiting + -- inside it is refused (Q#JR14b), because a yield would restore the + -- scope while this coroutine is still parked. + local function commit() + -- The captured window, read once. Everything below that would + -- otherwise consult "the active window" must consult THIS instead: + -- the scope pins the frontend, not the selected window, and a split + -- or panel can take focus within that frontend while the listing is + -- in flight (Q#JR14). + local captured = opts.dest ~= nil and opts.dest:window() or nil - -- `q` returns to the buffer you came from, never to another dired - -- buffer (which would trap `q` walking back down the tree); on a - -- descent the arriving buffer inherits the departing one's origin. - if departed ~= nil then - handle.prev = departed.prev - else - local active = pmacs.window.buffer() - if active ~= nil and handle_for_buffer(active) == nil then - handle.prev = active + local handle = claim_handle(canonical) + handle.entries = entries + handle.errors = errors + handle.sort_mode = sort_mode + + -- `q` returns to the buffer you came from, never to another dired + -- buffer (which would trap `q` walking back down the tree); on a + -- descent the arriving buffer inherits the departing one's origin. + if departed ~= nil then + handle.prev = departed.prev + else + local active + if captured ~= nil then + active = pmacs.window.buffer(captured) + else + active = pmacs.window.buffer() + end + if active ~= nil and handle_for_buffer(active) == nil then + handle.prev = active + end end + + paint(handle) + display(handle, opts, departed, captured) + -- Seating happens after the display: `switch_buffer` zeroes the + -- window cursor, so an earlier seat would be discarded. + seat_cursor(handle, opts.select_name, 1) + kill_departed(departed, handle) + return handle.buf end - paint(handle) - display(handle, opts, departed) - -- Seating happens after the display: `switch_buffer` zeroes the - -- window cursor, so an earlier seat would be discarded. - seat_cursor(handle, opts.select_name, 1) - kill_departed(departed, handle) - return handle.buf + if opts.dest == nil then + -- Interactive path (`C-x d`, tree descent, refresh): the acting + -- frontend is still ambient a tick later, which is what dired has + -- always relied on. Migrating these onto a captured destination too + -- is a named deferral, not this stage's work. + return commit() + end + + local ok, result = pmacs.window.commit_to(opts.dest, commit) + if not ok then + error(string.format("destination is gone (%s)", tostring(result))) + end + return result end function pmacs.dired.open(path, opts) return open_directory(path, opts, nil) end + -- Every interactive entry point funnels through here: spawn the -- coroutine the await needs, and turn a failure into a status message -- rather than an uncaught raise inside `pmacs.async` (which would land @@ -670,6 +723,20 @@ local function open_async(path, opts, departed, where) end) end +-- Journey Stage 1a (Q#JR7): dired is the DEFAULT directory surface, not +-- a `path.open-directory` subscriber. +-- +-- It cannot be a subscriber and still be replaceable. `HookRegistry.add` +-- only appends, and builtins load before `init.lua`, so a dired +-- subscription would always run first and always claim -- no user +-- listener could ever win. The hook is therefore the user's chain and +-- this slot is the fallback the editor consults when that chain +-- declines. Replace it to change what opens a directory; set it to nil +-- to disable directory opening entirely. +pmacs.path.set_directory_handler(function(path, dest) + open_async(path, { dest = dest }, nil, "dired") +end) + -- --------------------------------------------------------------------------- -- Commands -- --------------------------------------------------------------------------- diff --git a/docs/active-work.md b/docs/active-work.md index ff1b894..83c3a51 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -5,14 +5,18 @@ landed on `main`. Read it after `docs/agent-handoff.md`. Remove completed entries when their PR merges; do not let this become a second permanent backlog. -**Two lane headers below are stale on purpose**, pending the docs updates -their own lanes owe: multi-root LSP affinity **#161 has merged** (the -Lean 4 lane still says IN REVIEW; its continuation is PR #167) and GPU -terminal input **#166 has merged** (its lane still says IN REVIEW; PR -#168 records it). Trust the canonical-base line below over a lane header: -if a PR number appears in `git log --first-parent githubsucks/main`, it -has landed regardless of what its lane says. (The inline-math lane was -here too until #172 removed it — that is the update those two owe.) +**One lane below is retained past its merge, and says so at its own +head**: the PTY terminate diagnostic (#176), because no landed-doc PR +owns moving its facts to `docs/agent-handoff.md` yet, and rule 4 removes +a lane only *after* that move. Every other merged lane has been removed — +the Lean 4 and GPU-terminal-input headers this paragraph used to +disclaim are gone, as are the inline-math (#172), dired (#169), and +terminal config + copy mode lanes — the last of these was #180's work, +folded into #182 so two open PRs would stop re-conflicting in this file. + +**Trust the canonical-base line below over any lane header**: if a PR +number appears in `git log --first-parent githubsucks/main`, it has +landed regardless of what a lane says. ## Repository authority @@ -23,17 +27,22 @@ here too until #172 removed it — that is the update those two owe.) machine-local: `origin` may name this canonical URL, a release mirror, or something else, and therefore has no authority by name alone. - Canonical base at this snapshot: - `githubsucks/main` @ `74301d1` (the dired Stage 1 landed-doc refresh - #169 atop Lean 4 Stage 4a #179, bottom-panel - Stage 2A #177, the bottom-panel Stage 2 framing #175, terminal - configuration Stage 1 #173, Lean 4 Stage 3b #170, Stage 3a #167, the - CRDT undo repro #157, the inline-math landed-doc refresh #172, the - bottom-panel landed-doc refresh #156, the inline-math slice #158, - dired Stage 1 #165, the GPU terminal input fix #166, Lean 4 Stage 2 - #161, the dired framing #164, COHERENCE.md #163, find-file #162, Lean 4 - Stage 1 #160, and the minimap blank-slab fix #159; protocol v20). The - previous snapshot named `d152120`; the recovery check below accepts it - or anything newer. + `githubsucks/main` @ `42025e4` (Lean 4 Stage 4b #181, atop the dired + Stage 1 landed docs #169 and the PTY-terminate diagnostic #176, + terminal copy mode #178, the GPU-terminal-input landed docs #168, Lean + 4 Stage 4a #179, bottom-panel Stage 2A #177, the bottom-panel Stage 2 + framing #175, terminal configuration Stage 1 #173, Lean 4 Stage 3b + #170, Stage 3a #167, the CRDT undo repro #157, the inline-math + landed-doc refresh #172, the bottom-panel landed-doc refresh #156, the + inline-math slice #158, dired Stage 1 #165, the GPU terminal input fix + #166, Lean 4 Stage 2 #161, the dired framing #164, COHERENCE.md #163, + find-file #162, Lean 4 Stage 1 #160, and the minimap blank-slab fix + #159; protocol v20). The previous snapshot named `74301d1`, and **the + recovery floor advances with it**: the check below now requires + `42025e4` or newer, so a tree at `74301d1` no longer passes. That is + deliberate — the floor moves with the base, because a check that + accepts an older commit than the declared base passes on a tree the + rest of this file does not describe. **Lanes below that name an older base have not been re-based; derive their integration surface from `git diff ..main`.** - On the transfer source, `origin/main` named a release mirror at @@ -69,14 +78,21 @@ git worktree list git status --short --branch ``` -The `git log` command must expose `c93f9ee` — the base named above — or a +The `git log` command must expose `42025e4` — the base named above — or a newer intentional main. Keep this threshold and the canonical-base line in step: a recovery check that accepts an older commit than the base it declares canonical will pass on a tree the rest of this file does not describe. If it does not, stop and repair the remote/fetch configuration. -## PTY terminate diagnostic lane — IN REVIEW (PR #176) +## PTY terminate diagnostic lane — MERGED (PR #176) + +> **Lane retained deliberately, and it is the next one to close.** #176 +> merged into `main` @ `bf8878f` (2026-07-26); rule 4 below removes a +> merged lane, but only after its durable facts reach +> `docs/agent-handoff.md`. **That absorption is unowned** — no landed-doc +> PR exists for #176 — so removing the lane now would delete the record +> instead of moving it. Whoever opens that PR removes this section. - Portable branch: `githubsucks/pty-terminate-eperm`; worktree `../pmacs-math-slice`. **PR #176**, base `main`, based on `ccf29e3` @@ -162,145 +178,50 @@ If it does not, stop and repair the remote/fetch configuration. to recur; the next occurrence carries its own evidence under whoever's PR, and a Stage B framing follows then. -## Lean 4 lane (Arc 8) — Stages 1–4a MERGED; Stage 4b IN REVIEW +## Journey Stage 1a — PR #182 OPEN, review round 1 closed -- **Stages 1, 2, 3a, 3b and 4a are MERGED** — #160 (`main` @ `0827dd1`), - #161 (`46a1b8f`), #167 (`6f348c9`), #170 (`d400f30`), #179 - (`a27f646`). Their full - histories were pruned from this ledger in round 6, per this file's own - instruction to remove entries when their PR merges; the durable facts - now live in `docs/agent-handoff.md` §1's Lean 4 bullet, which is where - a fresh machine should read them. `docs/lean4-mode-framing.md` rev 9 - carries the decisions. - -### Stage 4b — the Unicode input method (branch `lean4-stage4b-input-method`) - -- Framing `docs/lean4-mode-framing.md` **revision 12** (rounds 10, 11 - and 12 = review of the implementation). Stage - 4a (the typed-edit consumer chain) MERGED as #179; this branch is 4b, - the Lean content that registers on it. -- Footprint: `scripts/regen-lean-abbrev` (new, the generator), - `builtin/runtime/lean_abbrev.lua` (new, VENDORED DATA — 1,855 entries - from `leanprover/vscode-lean4@17d1d08`, Apache-2.0), - `builtin/runtime/lean_input.lua` (new, the consumer at priority 50), - `src/editor.rs` (two `include_str!` blocks), - `tests/lean_input_acceptance.rs` (new, 31 tests), and one - `#[cfg(feature = "crdt")]` `--lib` test in `src/daemon.rs` - (acceptance 45f). No protocol change (Q#LN14). Entirely Lua apart - from the load sites and that one test. -- **Round 9 corrected three acceptance criteria that the real table - contradicts** — found by simulating the state machine over all 1,855 - entries and re-reading upstream at the pinned commit, not by reading - the prose again. `\to` is NOT eager (`top`, `to0`, `toa` extend it); - `\zzzz` expands to `ζzzz ` because `ze`/`zeta`/`zsqrtd` exist, and - only `$ % , ; @ W` open no key at all; and `\alpha`'s undo does not - restore `\alpha ` because `alpha` IS eager, so the terminator is a - separate edit. Criteria 38, 41 and 42 now state both paths. -- **Two generator bugs, both caught by its own round-trip check - failing closed:** `str.splitlines()` also splits on U+2028/U+2029, - and 53 symbols contain one literally, so the check reported a count - mismatch that was its own bug; then escaping via `chr(byte)` produced - a latin-1-shaped string that `write_text(encoding="utf-8")` - re-encoded, and every non-ASCII symbol landed double-encoded. The - first version of the check compared IN-MEMORY strings and agreed with - itself. **It now stages the file, re-reads the bytes from disk, and - renames into place only on a match.** -- **The point must be placed explicitly after the replace.** The - expansion SHRINKS the buffer (`\alpha` 6 bytes → `α` 2), so a point - left at the pre-edit offset is past the new end and every later - self-insert is silently rejected — the editor looks dead after the - first expansion. Pairing's "no cursor motion on the clean path" does - not generalize: that holds only for an insert AT the cursor. -- **Three tests were vacuous when first written and were found by - biting, not by review:** the abandonment test asserted text that a - wrongly-surviving record would also produce (claiming makes no edit — - it needed the follow-up keystroke that completes an eager key); the - re-arm test used the framing's own `\alpha\to`, which never reaches - the re-arm branch because `alpha` is eager and closes the record - first (`\al\to` does); and both buffer-switch tests passed through - `find_or_open`'s fresh-load path, which fires `buffer.after-load` and - a record-less edit rather than `buffer.after-switch` — deleting the - subscriber left them green. All three now bite. -- **Bite table** (each mutation, and the tests it fails): - - | Mutation | Tests it fails | - |---|---| - | register at priority 150 (after pairing) | 2 | - | claim only completed expansions | 2 | - | longest match instead of shortest | 9 | - | equal-length tie keeps the LATER key | 3 | - | remove the eager branch | 8 | - | expand without the terminator in the span | 2 | - | remove the re-arm branch | 1 | - | remove the point-still-at-span-end check | 1 | - | remove the exact-revision check | 1 | - | leave the point where the replace found it | 5 | - | remove the `lean4` language gate | 1 | - | remove the `lean.abbrev` gate | 2 | - | `buffer.after-switch` clears every frontend | 1 | - | delete the `buffer.after-switch` subscriber | 1 | - | `frontend.detached` purges every frontend | 1 | - | claim the terminator | 1 | - | expand inside the chain, then decline | 2 | - | drop the `cursor() == post_cursor` check | 1 | - | place the point without the context guard | 1 | - | load lean_input.lua after lsp.lua | 1 | - | let a nested fan-out consume the deferred slot | 1 | - | stop counting chain invocations | 1 | - | count fan-outs in the expander instead of the sentinel | 1 | - - Acceptance 45f bit by construction: without a registered window for - the source frontend it ran six fan-outs with a nil record and proved - nothing, because `handle_remote_crdt_op` arms nothing unless the - source's active window displays the buffer. -- **Round 10 (review) found three defects, all about what happens - AROUND the expansion rather than about resolving an abbreviation.** A - pair character that TERMINATES an abbreviation never reached pairing - (`\alp(` gave `α(`): the first revision claimed the terminator, and - merely declining is not enough either, because the chain hands each - consumer a copy of the record made before any consumer ran — so - expanding inside the chain invalidates pairing's copy and the closer - is lost anyway (verified by mutation, not assumed). The expansion now - runs on **its own `buffer.after-edit` subscriber** after the chain, - with a span that stops before the terminator. That is a new instance - of Q#AP7, so it is now pinned with the sighelp fake server. - Post-insert point motion was also mistaken for a valid span (the - relevance check needs `cursor() == post_cursor`, as pairing's has - since #110), and cursor placement could move a buffer an intercept - had switched to. -- **Round 11 found the round-10 fix incomplete in one place: - `buffer.after-edit` fan-outs NEST.** A consumer between the expander - (50) and pairing (100) that calls `pmacs.hook.run("buffer.after-edit")` - re-enters the expander's subscriber while the OUTER chain is still - mid-list; the nested pass expanded and outer pairing then resumed with - an invalidated record — `α(` again, through the chain's documented - re-entrancy seam instead of through claiming. **Deferring work past a - fan-out means owning which fan-out it belongs to.** The chain's - subscriber and the expander's each run exactly once per fan-out, so - counting the first and matching it off in the second identifies the - nesting level with no new seam in merged Stage 4a substrate. -- **Round 12 found round 11's counter in the wrong place.** It counted - invocations of the EXPANDER, which is optional: a lower-priority - consumer can claim and stop the chain before the expander runs, while - that fan-out's deferred subscriber still runs — so the nested pass - went uncounted, looked outermost, expanded early, and outer pairing - resumed with an invalidated record. The count now comes from a no-op - consumer at the MINIMUM priority, which runs first in every chain - invocation that reaches any consumer, and degrades safely: the only - thing that can skip it is a claim ahead of it, which skips the - expander too. A subscriber registered beside `run_deferred` cannot - serve — the whole nested fan-out completes inside the outer chain's - subscriber, before it would run. -- **Rounds 10–12 share a shape worth naming.** Each fix was correct - about the failure it was shown and wrong about the boundary of the - mechanism it leaned on — first the chain's copy semantics, then its - re-entrancy, then its short-circuit. **A queue that outlives the - thing that filled it has to name that thing, not approximate it.** -- Undo is cross-peer-degraded on CRDT frontends and that is ACCEPTED, - named in the module header (Q#LN21): six source-peer optimistic - inserts replaced by one daemon-peer op. `set_round_trip_input` would - fix it and also disables `dispatch_idle`, so RET would stop inserting - a newline. +- Framing `docs/journey-stage1a-framing.md` **rev 8** (four review + rounds, two correction revisions found during implementation, one from + review round 1 of PR #182). + Branch `journey-stage1a-directory-open`, based on `githubsucks/main` + @ `42025e4` (rebased onto `74301d1`, then integrated `42025e4` and the + landed-docs work below by merge — the branch is under review, so its + history is no longer rewritten). +- Recovery: `git fetch githubsucks && git checkout + journey-stage1a-directory-open`. Everything below is committed and + pushed; nothing depends on a worktree or `/tmp`. +- **Ships:** the directory arm on `resolve_target_buffer`, + `EditorState::open` rewritten as a caller of it (the unification), the + `path.open-directory` chain + `pmacs.path.directory_handler` fallback + slot, `pmacs.window.commit_to` with its scoped frontend and preflight, + the nonconstructible destination userdata, the daemon bootstrap arm, + and `tests/journey_acceptance.rs` (**24 pins** as of rev 8 — a count, + not a constant; re-read it rather than quoting this line). No protocol + change — still v20. +- **Doc updates ride the PR** per COHERENCE §25: §2 grade + step-3 + verdict row, §20 Priority 1 + the arc list, the GPU initial-target + framing's Q#GT6 / acceptance 10 supersession, handoff §1. +- **Bite results** (each mutation run against the full suite): scope + stops swapping `core.active_frontend` → N6a + P3 fail, nothing else; + preflight moved after the callback → P1 + P2 fail, nothing else; drop + the `ScopedFrontend` arm from `acting_frontend` → N4b fails, nothing + else. That last mutation is why N4b exists — it left N4 green. + Round 1 of PR #182 added two more: dired's `display` back to + `switch_buffer`, and `prev` read from the ambient window → each fails + **N4c** alone. **The scope pins the frontend, not the window** — every + routing pin before N4c varied frontend identity and none varied the + selected window within one frontend, so 23 green pins missed it. +- Ordering: PR #177 MERGED (2026-07-26), so 1a was unblocked. 1a lands + before dired Stage 2. When 1a lands, Stage 2 must re-scout and revise + its framing around the scoped `pmacs.window.commit_to` boundary before + its implementation branch is cut. That revision is a prerequisite, not + a review-time discovery. +- **Named deferrals carried out of this stage:** dired's *interactive* + paths (`C-x d`, tree descent, refresh) still rely on the ambient + frontend a tick later and are not migrated onto captured destinations; + the stale startup scratch buffer is still not removed (only the false + doc comment is corrected); `resolve_target_buffer`'s directory arm has + no picker, only the chain that leaves room for one. ## The CRDT half of the test corpus is dark in CI — NEEDS A LANE @@ -313,11 +234,18 @@ If it does not, stop and repair the remote/fetch configuration. Every `#[cfg(feature = "crdt")]` test is therefore **not compiled** in CI, not merely skipped. - **Measured, `--list` under CI's exact flags versus the same flags plus - `crdt`: 3,024 vs 3,288 — 264 tests dark.** Per target: + `crdt`: 3,176 vs 3,449 — 273 tests dark.** Re-measured at `74301d1` + (2026-07-26; at `fe8b8ba` it read 3,170 vs 3,443, the same 273 dark — + #176 added six tests, none of them `crdt`-gated). **The number moves + with every merge and must be + re-measured, not quoted.** #168 reported 3,024 vs 3,288 — 264 dark, + 177 in the library — at `1b6a084`; #178 then added CRDT-only + generated-buffer coverage, and other lanes landed CRDT tests in + between. Per target: | dark | CI | full | target | |---:|---:|---:|---| - | 177 | 1,832 | 2,009 | **the library itself** (`src/lib.rs`) | + | 185 | 1,848 | 2,033 | **the library itself** (`src/lib.rs`) | | 21 | 15 | 36 | `m5_5_acceptance` | | 13 | 1 | 14 | `gpu_invocation_acceptance` | | 13 | 1 | 14 | `gpu_initial_target_acceptance` | @@ -329,15 +257,20 @@ If it does not, stop and repair the remote/fetch configuration. | 3 | 0 | 3 | `compile_mode_crdt_acceptance` | | 2 | 22 | 24 | `theme_faces_acceptance` | | 2 | 0 | 2 | `m11_5_semantic_acceptance` | + | 1 | 14 | 15 | `terminal_copy_mode_acceptance` | | 1 | 9 | 10 | `vterm_stage1_acceptance` | | 1 | 7 | 8 | `statusline_segments_acceptance` | | 1 | 10 | 11 | `gpu_font_acceptance` | | 1 | 0 | 1 | `auto_indent_crdt_acceptance` | | 1 | 0 | 1 | `m10_11_perf` | + The rows sum to 273; the table is the whole census, not its head. + - **The single worst line is the library.** `cargo test --lib --features crdt` - is a REQUIRED local gate in `CLAUDE.md`, and CI has never run it. 177 - library tests — the whole CRDT half — are developer-machine-only. + is a REQUIRED local gate in `CLAUDE.md`, and CI has never run it. 185 + library tests — the whole CRDT half — are developer-machine-only, and + that count grows with every merged branch that adds a `crdt`-gated + unit test. - **Ten suites run zero or one test in CI**, including `gpu_initial_target` (#148's entire acceptance, 1/14), `gpu_invocation` (#141's, 1/14), and `a37`, the Vterm Stage 3 real-daemon/real-PTY/real-wgpu path that #135 @@ -391,282 +324,39 @@ If it does not, stop and repair the remote/fetch configuration. - Mitigating fact, verified rather than assumed: #166's three unit pins are **not** `crdt`-gated and do run under CI's exact flags, including the controller-release pin whose only job is catching the plausible wrong fix. - -## Terminal config + copy mode arc — Stage 1 MERGED; Stage 2 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 MERGED as #173** (`main` @ `cf54270`, 2026-07-26, one review - round, all twelve checks green). Branch `githubsucks/terminal-config` - and worktree `../pmacs-terminal-config` retained. Profiles, scrollback, - a per-terminal configurable escape key, and the `C-c t` opening - binding; no protocol change. Main was integrated **twice** during the - single review round (`ccf29e3`, then `c93f9ee` after the first merge - left the PR conflicting) — see the no-CI-while-conflicting fact below. -- **Stage 2 = `githubsucks/terminal-copy-mode`**, worktree - `../pmacs-terminal-copy-mode`, based on `githubsucks/main` @ - `cf54270`. Copy mode: `M-x terminal.copy-mode` / `C-c C-t`. -- **Stage 2 ships eight of nine criteria, and the missing one is named.** - Criterion 17 (a real semantic frontend proving neither daemon buffer - nor mirror mutates) is **not pinned**: the optimistic apply exists only - in `pmacs-gpu/src/main.rs`, and the headless `SemanticClient` every - other semantic test uses has no optimistic path, so a faithful test - must drive the real GPU binary — the `a37` foundation, which CI never - compiles, silently skips without the binary, and is load-sensitive. A - second test on that footing buys the appearance of coverage. Both - halves of the mechanism are pinned **ungated** instead: acceptance 16 - (the guard is armed — `dispatch_idle` false while the snapshot is - focused) and 16b (the daemon holds — `is_read_only()` is **true** at - the rope, so an op that did arrive is refused by `ensure_writable()`). - **Rounds 2-3 changed what 17 must show.** 16b asserted `false` through - round 1, documenting the hazard; round 2 closed it. So the eventual - real-GPU test must look for **mirror mutation plus daemon refusal — - divergence** — not the "mutates both sides, silently" the criterion - originally specified, which after the fix cannot happen and would pass - for the wrong reason. The wire-level half stays an explicit obligation - of the CI `crdt`-coverage lane. -- Load-bearing Stage 2 decisions: - - **The snapshot MATERIALIZES into an ordinary buffer**, so isearch, - motion, selection and the kill ring work with no new substrate, and - "keys must not reach the child" dissolves structurally — the - transport arm keys on `is_terminal(buffer_id)` and a snapshot is not - a terminal. **The dispatch-shadow count stays at six.** - - **One serializer, not two** (Q#TC7): `copy_retained` builds a - whole-range *selection* and hands it to `copy_selection_bytes`. - - **`prune` reacts to removal rather than causing it** — it filters on - `!registry.contains(buffer_id)`, so a child exiting does NOT remove - the terminal buffer. That is why `on_removed` is a sound teardown - hook, and why a finished command's output stays readable. -- **Five bites, five different wrong implementations.** Removing - `set_round_trip_input` fails acceptance 16 **in the default - configuration** (the whole reason that pin is ungated); a naive - independently-written serializer fails all four unit pins, with the - diffs naming each drift mode (broken soft wrap, untrimmed blanks, - trailing newline); making re-invoke create a fresh buffer fails 18; - dropping the kill-with-terminal teardown fails 18; removing the - intercept fails 16b. Each failed exactly one test. -- **Review round 1 — four findings, all real, and they rhyme in pairs.** - Two P1 implementation defects and two P2 vacuous pins, all four tracing - to one root: **a name is not an identity, and a context-free readout is - not a state observation.** - - *P1 — a foreign same-named buffer was adopted and clobbered.* Snapshot - writes use `bypass_intercept`, so found-by-name adoption overwrote a - user's buffer; the reviewer reproduced "do not clobber" becoming 23 - newlines. Fixed by dired's F7 rule: **ownership means "in our own - handle table"**, and a taken name yields a `<2>` variant. - - *P1 — snapshot identity was keyed by terminal NAME.* - `TerminalManager::open` uniquifies only the *derived* name, so an - explicit `name = "*same*"` lets two valid terminals share one; they - then shared a snapshot, `q` returned to the wrong terminal, and - killing either removed it. Now keyed by comparing buffer handles in an - array — `BufferIdLua` implements `__eq` but each wrapper is a distinct - table key, so **comparison works and hashing does not**. - - *P2 — the refresh pins were vacuous.* 19 compared a quiet terminal's - snapshot against itself and 18 counted buffers, so both passed with - `render_snapshot` replaced by a no-op. Now the test types a marker - into the `cat` child, requires it **absent** first, then refreshes. - - *P2 — the tail-follow pin could not observe view state.* - `manager.snapshot(buffer_id)` is context-free and always reads the - live screen, so it reported "at the tail" for a view forced to the - oldest retained row. Now read through `snapshot_for_view`'s - `at_bottom` and projected cells. -- **Four more bites, all discriminating.** Restoring adopt-by-name fails - 18a *and* 18b; restoring name-keyed identity fails 18b; making - `render_snapshot` a no-op fails **both** 18 and 19 (the vacuity, - demonstrated); and forcing the view off the tail fails 20. -- **Review round 2 — one P1, and its fix retires half a named deferral.** - **Undo emptied the "read-only" snapshot.** `render_snapshot` wrote with - `bypass_intercept`, leaving ordinary undo history, and **`Buffer::undo` - reaches the rope through `ensure_writable` without ever consulting the - intercept chain** — so `C-/` *or* `M-x buffer.undo` replaced a freshly - rendered snapshot with an empty buffer. `set_round_trip_input` does not - help: it routes the key into the daemon command path, which is where - undo runs. - - **Rebinding the undo chords would NOT have fixed it**, and - `compile.lua` already says so in a comment — "command/menu undo stays - dispatchable". `*compilation*` and listview panels therefore carry the - same latent defect today. - - Fixed with `Buffer::set_generated_contents` (Lua - `pmacs.buffer.set_generated_contents`): lift `read_only`, replace - skipping intercepts, **discard history**, re-assert `read_only`. This - ships the deferred lane's two halves *as one primitive* — a bare - `set_read_only` would let a caller lock a buffer it can no longer - refresh, which is exactly why that lane was deferred. Clearing history - also stops a periodically refreshed buffer accumulating rope clones - nothing can ever pop. - - New pins: **acc16c** drives the real M-x path - (`command.invoke_interactive`), the chord, and redo, and asserts the - owner's refresh still works; **acc16b** flipped from asserting - `is_read_only()` is *false* to *true*, because the property it - described is the one that was fixed; plus three `buffer.rs` unit tests. - - Bite: restoring the `delete`+`insert` render reproduces the report - exactly — `left: Some("")` against the full snapshot — failing acc16c - and acc16b. - - **Still open:** `*compilation*` and listview remain emptiable by - `M-x buffer.undo`; the primitive they need now exists and is proven, - so the remainder is adoption plus a streaming-friendly variant. -- **Review round 3 — one P1 and two P2s, all on the round-2 primitive.** - The lesson: **a rope write is only half of an edit, and "discard - history" means whichever history the buffer actually has.** - - **P1 — the binding swallowed the edit.** `set_generated_contents` - returned `()`, so nothing called `notify_buffer_edit_to_windows`. - Two consequences, both reproduced by the reviewer: in the default - build a window showing the buffer kept a `TextView` line index - describing the *previous* contents, and the next paint indexed the - new rope with stale ranges — `assertion failed: end <= self.len()` - in `src/rope.rs`; in the CRDT build `pending_crdt_ops` stayed empty, - so replica mirrors never received the owner's write. The prior - `buf:delete`/`buf:insert` pair had done this fan-out for free. - Fixed by applying **one whole-buffer `Replace`**, returning its - `Edit`, and notifying from the binding. - - **P2 — "discard history" was false in CRDT mode.** The v0.1 stacks - are bypassed entirely there; the history lives in loro's - `UndoManager`. `read_only` stops the replay but not the retention, - which is the memory cost the contract claims to eliminate. - `UndoManager` has no `clear`, but needs none — it records only what - happens after construction, the property `CrdtState::from_bytes` - already uses to keep the seed insert out of undo. New - `CrdtState::clear_undo_history` rebinds a fresh manager to the - same doc. - - **P2 — the docs described the pre-fix architecture.** Q#TC6a said no - Lua binding sets `read_only` and round-trip input is the only guard; - the acceptance text still said `is_read_only() == false` while 16b - had been flipped to `true`; `terminal.lua`'s comment repeated the - obsolete claim. The architecture is **layered** and now says so: - rope-level read-only protects the daemon copy, round-trip input - protects the replica's optimistic mirror, and neither substitutes - for the other. Q#TC6a carries a superseded-in-part box rather than - being silently rewritten. - - New pins: **acc16d** paints the window after a *shrinking* generated - write (the stale offsets then point past the end, which is the - reported crash rather than stale pixels); **acc16e** asserts the - refresh is queued for mirrors through the real copy-mode path - (`crdt`-gated, therefore dark in CI — 16d is the half that runs); - plus a CRDT `buffer.rs` unit test that ten renders leave the - `UndoManager` with nothing recorded. - - Bites: dropping the notify panics acc16d at `rope.rs:145` and fails - acc16e with `queued: []`; dropping the `UndoManager` rebind fails - the new unit test on `can_undo`. - - **Still open:** the fan-out obligation makes `*compilation*`/listview - adoption more than a one-line swap — recorded in `COHERENCE.md` §14 - alongside the undo half. -- **Review round 4 — one P2, docs only, and it is the interesting kind.** - **A fix can invalidate a test that was never written.** Criterion 17's - *bite* still described the pre-round-2 world: remove - `set_round_trip_input` and the op "mutates both sides, silently, with - no divergence to notice". True while nothing set `read_only` from Lua; - false once `set_generated_contents` did. A real-GPU test written to - that spec would hunt for a daemon-side edit that can no longer occur - and pass for the wrong reason — the specification would have leaked - the round-2 regression back in, through a test not yet built. - - Restated around **unauthorized mirror mutation plus daemon refusal = - divergence**, in all four places that carried the old claim: the - criterion, the Q#TC6a heading, the acceptance-16 doc comment, and the - bite roster. The heading's "ONLY thing" now says what it is the only - thing *for* — the replica's own mirror. - - Why round-trip input is still load-bearing rather than redundant: a - daemon refusal arrives after the frontend has already applied - optimistically and painted. It buys divergence instead of silent - agreement; it does not prevent the mutation the user sees. - - **Gate-run flake observed and scoped without overclaiming its cause.** - `cargo test --lib --features crdt` failed ~1 run in 5 on - `process::tests::setsid_escapee_is_not_reaped_and_teardown_reclaims_readers` - — `active_reader_probe` returning `None` at `process.rs:3179` - ("live runtime probe"). **Pre-existing and unrelated:** this branch - does not touch `src/process.rs` (last changed by the Darwin PTY - signal-name fix), and the test passed 10/10 standalone; the observed - failures were during parallel full-suite runs. That localizes the - trigger to suite load or interaction, but does **not** distinguish - parallelism from another full-suite effect — no serial full-suite bite - was run. The leading code-path explanation is the known `drain_until` - trap: draining for `Started` also ticks, and a tick can reap the leader - before the following `active_reader_probe`. That is an inference from - the failure site and control flow, not yet a falsified root cause. - It belongs to the CI `crdt`-coverage lane for discrimination. The two - round-2 CRDT failures had no captured test names; this flake is a - plausible candidate for them, but they remain **unattributed**. -- 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. +- **This lane also owns a `--lib --features crdt` flake, observed and + scoped without overclaiming its cause** (inherited from #178's gating, + where the terminal lane recorded it). `cargo test --lib --features + crdt` failed ~1 run in 5 on + `process::tests::setsid_escapee_is_not_reaped_and_teardown_reclaims_readers` + — `active_reader_probe` returning `None` at `process.rs:3179` ("live + runtime probe"). **Pre-existing and unrelated to #178:** that branch + did not touch `src/process.rs` at all, and the test passed 10/10 + standalone; the observed + failures were during parallel full-suite runs. That localizes the + trigger to suite load or interaction, but does **not** distinguish + parallelism from another full-suite effect — no serial full-suite bite + was run. The leading code-path explanation is the known `drain_until` + trap: draining for `Started` also ticks, and a tick can reap the leader + before the following `active_reader_probe`. That is an inference from + the failure site and control flow, not yet a falsified root cause. + Discriminating it belongs here. Two unnamed CRDT failures in #178's + round-2 gating are a plausible match but remain **unattributed** — no + test names were captured. +- **A second standing obstacle for this lane:** `cargo clippy --workspace + --all-targets --features crdt -- -D warnings` **fails on `main`** — + measured at `74301d1`: seven errors before the build aborts, four in + `src/daemon.rs` (`useless_conversion` at 3996, missing doc backticks at + 4076, `too_many_lines` 112/100 at 4083, an unneeded `mut` at 4965) and + three in `tests/vterm_stage3_acceptance.rs` (`too_many_lines` at 637 + and 793, a redundant `continue` at 843). **Treat that as a lower + bound, not an inventory:** Clippy abandons the remaining targets once + one fails, and a run on an older tree surfaced a further doc-backticks + error in `tests/auto_indent_crdt_acceptance.rs:42` that this run never + reached. The + standing gate list runs Clippy without `crdt`, so these lints have + never been enforced. Any CI job that compiles the `crdt` targets has to + fix them first or it will be red on arrival. ## Bottom-panel lane (Arc 7) — Stages 1, 2A + framing MERGED; 2B is next @@ -854,6 +544,24 @@ git worktree add --track \ ## Closed since the last snapshot +- **Terminal configuration + copy mode arc — BOTH STAGES MERGED, lane + removed.** Stage 1 **#173** (`main` @ `cf54270`, one review round) and + Stage 2 **#178** (`main` @ `fe8b8ba`, **four review rounds**, twelve + checks green on head `1b44c69` — verified by `head_sha`, not by the + check summary), both 2026-07-26, both with no protocol change. + Approved framing: `docs/terminal-config-and-copy-mode-framing.md` rev + 4, committed as the first commit of Stage 1's branch; its Q#TC6a + carries a superseded-in-part box rather than a silent rewrite. Durable + facts moved to `docs/agent-handoff.md` §1 (the arc bullet) and §4 (the + `set_generated_contents` invariant) per rule 3 below, and to + `COHERENCE.md` §14. **Stage 2 ships eight of nine criteria and the + missing one is named** — criterion 17 needs a real GPU frontend, so it + waits on the `a37` footing; the handoff records what it must assert. + Branches `githubsucks/terminal-config` and + `githubsucks/terminal-copy-mode` with worktrees + `../pmacs-terminal-config` and `../pmacs-terminal-copy-mode` are + retained. The gate-run flake found while gating #178 moved to the CI + `crdt`-coverage lane above, which owns its discrimination. - **Dired Stage 1 (the directory view) — MERGED as #165** (`main` @ `c8ec8f3`, 2026-07-25, after one review round). pmacs has a directory surface: `C-x d` / `C-x C-j`, one read-only buffer per directory named @@ -903,6 +611,15 @@ git worktree add --track \ reproduces in-process and so is not the GUI/TUI asymmetry; and a geometry change appearing to clear the visible screen, which reproduces pre-fix). Branch `gpu-terminal-input` and worktree `../pmacs-gui-term-input` retained. + **Its landed-doc pair MERGED as #168** (`main` @ `1b6a084`, + 2026-07-26): #166 recorded as landed, the CI `crdt`-coverage gap + measured (**264 tests dark workspace-wide**, 177 in the library — a + reading taken at `1b6a084` and kept here only as history. **The CI + `crdt`-coverage lane above is the authority for the live figure**; + do not quote this one forward), the + vterm audit corrected — "only 3 of 9 acceptances drive a real daemon" + was optimistic; without the frontend binary the honest number is + **2** — and the a37 findings folded into the coverage lane. - **Inline-math slice — MERGED as #158** (`main` @ `5aa9044`, 2026-07-25). Detect → parse → layout → draw for `$…$`, entirely inside `pmacs-gpu`, no protocol change. Verified by the user's manual pass on diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 67e7b43..6607df6 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -1,9 +1,13 @@ # Agent handoff — cross-machine continuity -**Last updated: 2026-07-26, after Lean 4 Stage 4a (#179) — the typed-edit +**Last updated: 2026-07-26, after terminal copy mode (#178) — `C-c C-t` +materializes a terminal's whole retained range into an ordinary buffer, +plus `Buffer::set_generated_contents`, the first genuinely immutable +generated-buffer write path — and its landed-doc pair (#168); following +Lean 4 Stage 4a (#179) — the typed-edit consumer chain — and bottom-panel Stage 2A (#177), the classified census routing that makes every Projection-class consumer ask -`primary_document_window`; following the bottom-panel Stage 2 framing +`primary_document_window`; the bottom-panel Stage 2 framing (#175), terminal configuration Stage 1 (#173) — profiles, scrollback, a per-terminal configurable escape key, and the `C-c t` opening binding — Lean 4 stages 3a and 3b (#167, #170), pmacs' first Lean language server; @@ -37,8 +41,10 @@ commands, read `docs/active-work.md` immediately after this file. ## 1. Where the project stands (2026-07-26) -- `main` @ `74301d1` (the dired Stage 1 landed-doc refresh #169 atop - Lean 4 Stage 4a #179, bottom-panel Stage 2A +- `main` @ `42025e4` (Lean 4 Stage 4b #181, atop the dired Stage 1 + landed docs #169 and the PTY-terminate diagnostic #176, terminal copy + mode #178, the GPU-terminal-input landed docs #168, Lean 4 Stage 4a + #179, bottom-panel Stage 2A #177, the bottom-panel Stage 2 framing #175, terminal configuration Stage 1 #173, Lean 4 Stage 3b #170, Stage 3a #167, the CRDT undo repro #157, the inline-math landed-doc refresh #172, the bottom-panel @@ -55,11 +61,100 @@ commands, read `docs/active-work.md` immediately after this file. standard new work is evaluated against. Per `CLAUDE.md`, **every new framing doc must state its coherence impact** — journey steps touched, interaction islands added, config-registry adoption, background-work - attribution. Its §2 grades the golden journey **broken at step 3** - (`pmacs .` exits 1). -- **Lean 4 arc (Arc 8) — stages 1, 2, 3a, 3b LANDED** - (`docs/lean4-mode-framing.md`; #160, #161, #167, #170; merge - `d400f30`). pmacs edits Lean 4: `arborium-lean` highlighting, a + attribution. Its §2 grades the golden journey; **Journey Stage 1a + moved that grade off "broken at step 3"** — see the arc bullet below. +- **Journey arc (P1) — Stage 1a LANDED** + (`docs/journey-stage1a-framing.md`). `pmacs .` opens a directory + instead of exiting 1, on **one** path: `resolve_target_buffer` gained a + `ResolvedTarget::Directory` arm *ahead* of the load, `EditorState::open` + became a caller of it rather than a parallel implementation, and the + daemon/GPU bootstrap shares the same arm. Which surface handles a + directory is the `path.open-directory` chain with dired as a + replaceable fallback slot. `tests/journey_acceptance.rs` is the new + cross-subsystem ratchet (steps 2, 3, 5 seeded; **stages add rows, none + removes them**). No protocol change. + - **A hook a builtin subscribes to can never be first-claimant-wins + for users.** `HookRegistry::add` only appends and builtins load + before `init.lua`, so a dired subscription would always claim before + any user listener. That is why dired is a *slot* + (`pmacs.path.directory_handler`) and not a subscriber — and why + clearing the slot has to leave startup succeeding with a status, + not exiting 1. + - **A raise and a `false` are indistinguishable in `proceed`.** + `run_short_circuit` returns `proceed = false` for both; only + `HookOutcome.errors` separates them, and it decides whether to + *report*, not whether to fall back. Getting this backwards produces a + fallback that runs after a user's resolver crashed mid-handling. + - **The listing is async; the bootstrap is synchronous.** The whole + post-await commit therefore runs against a destination captured at + request time (`pmacs.window.commit_to`), which preflights every + precondition *before* invoking the callback — dired mutates handle + state, `prev`, and paint long before it reaches anything that could + refuse, so validating at display time is four mutations too late. + Awaiting inside a commit is refused: a yield would restore the scope + while the coroutine is still parked. + - **The scope swaps `core.active_frontend`, not just an override** — + `pmacs.window.buffer()`'s no-arg arm reads the ambient active buffer + directly, so dired's `prev` capture would otherwise follow whatever + frontend happened to be dispatching. The override *also* exists, and + is load-bearing in exactly one case: a commit reached from inside an + interactive command, where the origin would otherwise outrank the + ambient value. Bite-testing found N4 green without it. + - **`replace_active_buffer` does not drop the startup scratch buffer**, + despite its doc comment having claimed so for as long as it has + existed. Its body is one `switch_active_buffer` call. The comment is + corrected here; changing the lifetime is separate work. + - Stage 1b is the named remainder: compile binding + Cargo defaults, + LSP spawn guidance, welcome buffer. +- **Terminal configuration + copy mode arc — COMPLETE** + (`docs/terminal-config-and-copy-mode-framing.md` rev 4; Stage 1 #173, + Stage 2 #178; no protocol change in either, still v20). Stage 1 ships + profiles, scrollback, a per-terminal configurable escape key and the + `C-c t` opener; Stage 2 ships copy mode — `M-x terminal.copy-mode` / + `C-c C-t`. + - **The snapshot MATERIALIZES into an ordinary buffer.** That is the + arc's organizing decision: isearch, motion, selection and the kill + ring work with no new substrate, and "keys must not reach the child" + dissolves structurally, because the transport arm keys on + `is_terminal(buffer_id)` and a snapshot is not a terminal. **The + dispatch-shadow count therefore stays at six.** + - **`prune` reacts to buffer removal rather than causing it** — it + filters on `!registry.contains(buffer_id)`, so a child exiting does + **not** remove the terminal buffer. That is what makes `on_removed` a + sound teardown hook, and why a finished command's output stays + readable. + - **Ownership means "in our own handle table", never found-by-name** + (dired's F7 rule, re-learned here): snapshot writes use + `bypass_intercept`, so adopting a same-named foreign buffer clobbers + user data. Snapshot identity is keyed by **comparing buffer handles + in an array** — `BufferIdLua` implements `__eq` but each wrapper is a + distinct table key, so comparison works and hashing does not. + - **Profiles are a raw Lua table**, joining `pmacs.lsp.config` and + `pmacs.pair.sets`, because `ConfigValue` is four scalars with no + table kind. The two open-time settings resolve through the **global** + chain (they are read before the identity buffer exists); only + `terminal.escape-key` resolves per buffer, and its cache lives on + **`TerminalSession`** so its lifetime is the terminal's — + `value_epoch` alone is not a sufficient key, because it does not + advance when focus moves between terminals holding different + buffer-local values. + - **Criterion 17 is deliberately unpinned, and its bite is now stated + correctly.** A real semantic frontend proving neither copy is mutated + needs the actual GPU binary (the optimistic apply exists only in + `pmacs-gpu/src/main.rs`; the headless `SemanticClient` has no + optimistic path), i.e. the `a37` footing §5 warns about. After + `set_generated_contents` the eventual test must look for + **unauthorized mirror mutation plus daemon refusal — divergence**, + not the "mutates both sides silently" the criterion originally + specified, which can no longer happen and would pass for the wrong + reason. *A fix can invalidate a test that was never written.* + - 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 such probes must **count + occurrences rather than test presence**, because a single-character + probe collides with the child's own banner text. +- **Lean 4 arc (Arc 8) — stages 1, 2, 3a, 3b, 4a, 4b ALL LANDED** + (`docs/lean4-mode-framing.md`; #160, #161, #167, #170, #179, #181). pmacs edits Lean 4: `arborium-lean` highlighting, a `lean4` major mode, `⟨⟩ ⦃⦄ ⟮⟯` pairs, and a `lake serve` language server with a Lake-aware outermost root, a lazy toolchain probe, a one-shot `lean --server` fallback, and `waitForDiagnostics`. **No @@ -102,8 +197,9 @@ commands, read `docs/active-work.md` immediately after this file. config swap invalidates. The durable lesson is to heal at **consumption** — the point where a stale record is handed out — not at the moment of the swap. - - **Stage 4a (typed-edit consumer chain) MERGED as #179** (`main` @ - `a27f646`, two review rounds). It is substrate only: + - **Stage 4a (the typed-edit consumer chain) MERGED as #179** + (branch `lean4-stage4a-typed-edit-chain`, framing rev 8; it is part + of the main anchor above). It is substrate only: `builtin/runtime/typed_edit.lua` owns the single `buffer.after-edit` subscriber and the single one-shot read, `pair.lua` becomes its first registered consumer, and @@ -128,8 +224,8 @@ commands, read `docs/active-work.md` immediately after this file. reason had been copied into a module comment, an acceptance criterion, a test comment, and the ledger. **Correct the source a rationale derives from, not only the sites that quote it.** - - **Stage 4b (the Unicode input method) is implemented and in review** - (branch `lean4-stage4b-input-method`, framing rev 9): a vendored + - **Stage 4b (the Unicode input method) MERGED as #181** + (framing rev 9): a vendored 1,855-entry table generated from `leanprover/vscode-lean4@17d1d08` by `scripts/regen-lean-abbrev`, plus a consumer registered on the Stage 4a chain at priority 50, ahead of pairing. **A consumer @@ -993,6 +1089,72 @@ before trusting them: ## 4. Substrate invariants (do not undo; tests enforce most of these) +**Generated buffers: `Buffer::set_generated_contents` is the ONE +authorized write** (terminal copy mode #178) — lift `read_only`, replace +via a single whole-buffer `Replace` skipping intercepts, discard history, +re-assert `read_only`, and **return the `Edit`**. Three things make it a +unit rather than a convenience: + +- **An intercept is not read-only.** `Buffer::undo` reaches the rope + through `ensure_writable` and never consults the intercept chain, so an + intercept-only "read-only" buffer is emptied by `M-x buffer.undo`. + Rebinding the undo *chords* buffer-locally does **not** close it — + `compile.lua`'s own comment says so ("command/menu undo stays + dispatchable"). Only rope-level `read_only` does. +- **A bare `set_read_only` would be worse than nothing**, because it also + refuses the owner's refresh — the operation such buffers exist for. + That is why the pairing, not the setter, is the primitive. There is + deliberately no Lua `set_read_only`. +- **A rope write is only half of an edit.** The returned `Edit` must be + fanned out (`notify_buffer_edit_to_windows`, which also queues the + daemon-origin CRDT op). Skip it and a displaying window keeps a + `TextView` line index describing the previous contents — the next paint + indexes the new rope with stale ranges and trips + `assertion failed: end <= self.len()` — while replica mirrors never + import the write at all. + +History clearing is load-bearing twice (nothing can pop entries +`read_only` makes unreachable, so they leak), and must clear **whichever +history the buffer has**: the v0.1 stacks are bypassed in CRDT mode, where +it lives in loro's `UndoManager`. That has no `clear`, and needs none — a +manager records only what happens after construction, so +`CrdtState::clear_undo_history` rebinds a fresh one to the same doc. + +**Not yet adopted — the inventory is four writer mechanisms covering +five buffers.** *Every remaining intercept-protected writer* uses the +older idiom: an erroring intercept plus `set_round_trip_input`, written +through `bypass_intercept`, with the rope left writable. All are +emptiable by `M-x buffer.undo`: + +| writer | buffers | shape | +|---|---|---| +| `builtin/runtime/listview.lua:60-61` | every listview panel | delete-all + insert | +| `builtin/runtime/compile.lua` (`ensure_slot`) | `*compilation*`, `*shell-command*` | **append** per output batch | +| `builtin/commands/default.lua:869` | `*search-results*` | reset per query, then **append** per match batch | +| `builtin/runtime/dired.lua:371` | every dired buffer | whole-buffer replace | + +**Do not read `ensure_slot` as covering the search panel** — it serves +`*compilation*` and `*shell-command*` only (`compile.lua:1090,1125`). +`*search-results*` is an independent panel with its own intercept, +round-trip mark and writes, and `compile.lua` names it only in a +predicate. Nor is the scope "every generated buffer": `*workers*`, +`*help*` and `*buffer-list*` are generated too but do not use this +idiom, and the REPL package's intercept +(`builtin/packages/repl/init.lua:187`) is an op-filtering editing +policy, not a read-only panel — neither group belongs to this lane. + +Adoption is not a one-line swap. It inherits the fan-out obligation, and +the three appending buffers need a **streaming variant** of the +primitive; listview and dired already write whole-buffer replaces and +are the cheap half. Recorded in `COHERENCE.md` §14. + +**And it does not replace `set_round_trip_input`.** The protection is +layered across two copies: rope-level `read_only` refuses the op at the +daemon; round-trip input stops a semantic frontend applying +optimistically to its **own mirror**, which a daemon-side refusal cannot +reach — the refusal arrives after the frontend has already painted, so it +buys divergence, not prevention. + **Command boundaries (Arc 2 kill-ring substrate)** — `EditorCore.command_history: HashMap`, per frontend. Rotate on: keybound command, self-insert, menu invoke, diff --git a/docs/gpu-initial-target-framing.md b/docs/gpu-initial-target-framing.md index c3b372c..41ea7d2 100644 --- a/docs/gpu-initial-target-framing.md +++ b/docs/gpu-initial-target-framing.md @@ -282,8 +282,17 @@ observers; they are not the transport implementation. - Any `NotFound` from the initial load creates an empty path-backed buffer, including when a parent is currently absent; save-time errors remain save-time errors, matching local `pmacs FILE`. -- `PermissionDenied`, `IsADirectory`, invalid path bytes at the OS boundary, - and other non-`NotFound` errors fail startup. +- `PermissionDenied`, invalid path bytes at the OS boundary, and other + non-`NotFound` errors fail startup. +- **`IsADirectory` is superseded by Journey Stage 1a** + (`docs/journey-stage1a-framing.md`). A directory no longer reaches the + load at all: `resolve_target_buffer` answers `ResolvedTarget::Directory` + ahead of it, so a directory target now *succeeds*, dispatching the + `path.open-directory` chain and replying `Opened`. Deliberate + supersession, not drift — the whole point of that stage is that + `pmacs .` must not exit 1, and a daemon/GPU bootstrap that still failed + would leave the two entry points disagreeing about the same argument. + Non-directory failures are unchanged. - The buffer display name may use `Path::display()` and therefore replacement characters; this must never replace the raw backing path used for dedup, load, or save. @@ -550,12 +559,17 @@ process behavior. 9. **New file:** a nonexistent target produces an empty snapshot, `[new file]` status/path identity, accepts an edit/save through the real session, and creates the requested file under the launcher cwd—not the daemon cwd. -10. **Open error:** a directory/permission-denied target returns a specific +10. **Open error:** a permission-denied target returns a specific failure before ready/window creation and makes root fail. The daemon shuts down that failed session's socket; a client that lingers or sends another event cannot reach uninstalled session state. An existing daemon remains connectable; a pre-existing frontend's active buffer and contents remain unchanged. + **Amended by Journey Stage 1a:** the *directory* case is deliberately + superseded and moved to the success path — see Q#GT6. A directory + target now reaches ready and the document window shows dired, pinned + by `initial_target_directory_reaches_ready` and its two siblings in + `src/daemon.rs`. Permission-denied is unchanged and still fails. 11. **Dedup preserves unsaved edits:** frontend A opens and modifies a file without saving; target-launch frontend B opens the same normalized path and receives A's authoritative unsaved text with the same `BufferId`, not disk diff --git a/docs/journey-stage1a-framing.md b/docs/journey-stage1a-framing.md new file mode 100644 index 0000000..ce5589f --- /dev/null +++ b/docs/journey-stage1a-framing.md @@ -0,0 +1,1082 @@ +# Journey Stage 1a — open a directory, on one path + +**Status: framing, rev 8 — APPROVED at rev 5; revs 6–8 record +corrections found during implementation and review of PR #182.** +**Serves `COHERENCE.md` §2 (the golden product journey), §19 (coherence +acceptance tests), §20 Priority 1.** + +## 0. Revision history + +- rev 1 (2026-07-26) — first framing. Scouted against `main` @ `d400f30`. +- rev 2 (2026-07-26) — review round 1. Q#JR2 withdrawn (its ground truth + was false); destination pinning added; the resolver chain restructured + around append-only hook registration; `ResolvedTarget` typed; + `display_file`'s contract specified; the GPU-framing supersession named. +- rev 3 (2026-07-26) — review round 2. Two blockers, two contract gaps: + - **Fail-closed was not failure-atomic** (§4.4). dired mutates handle + state — claim, listing, `prev`, paint — *before* it ever attempts + `display`, so rev 2's "does nothing" left a hidden buffer and could + corrupt an existing handle's `prev`. **Per the review's decision, 1a + now carries the destination-scope substrate**: a `commit_to` + primitive that revalidates and enters the captured frontend's scope + *before* any dired mutation, so the whole post-await commit — `prev` + capture, claim, paint, display, seat — executes against the captured + destination or not at all. + - **Expected-buffer validation** (Q#JR14): the destination carries the + buffer it was requested against, so a user who replaces the bootstrap + buffer mid-listing is not overwritten by stale launch intent. Rev 2's + window-only pin said launch intent wins; it should not, and B2's + "before the user can act" was false (§8). + - **Acceptance 6 was still vacuous**, and the blanket "each fails with + the change reverted" rule cannot hold for preservation guards. §6 is + split into new-behavior acceptances and preservation pins, each pin + naming the targeted mutation that falsifies it (§6.0). + - **Hook error policy specified** (Q#JR15): in a short-circuit hook a + raise and a `false` both yield `proceed = false` (`hook.rs:299-323`); + only `HookOutcome.errors` distinguishes them. An error now stops the + chain *and* suppresses the fallback. + - The fallback slot is described honestly as an **unowned singleton** + (§0.5), not an "ownership-carrying registration". +- rev 4 (2026-07-26) — review round 3. Three substrate details and one + inverted bite mutation: + - **`InteractiveCommandOrigin` was the wrong mechanism, twice over** + (§2.11). It does not scope the APIs rev 3 claimed — no-arg + `pmacs.window.buffer()` reads `core.active_buffer_id()` directly + (`mod.rs:12547`) and `move_to_line` mutates the core's ambient active + window (`mod.rs:12703`) — so `prev` capture and cursor seating stayed + ambient. And it is *authenticated interactive-command authority*: + entering it would make dired's `paint` satisfy the pre-edit unfold + guard (`mod.rs:1391`), `invoke_interactive`'s rotation (`:5400`), and + terminal command context (`:8515`). Rev 4 uses a **separate scoped + frontend override** that also swaps `core.active_frontend`, and + `commit_to` does not touch the interactive origin (Q#JR14e). + - **`dest` becomes nonconstructible userdata** (Q#JR14d). As a table it + is shared across hook listeners, so an earlier listener could mutate + the destination and decline — redirecting later listeners or dired — + and any Lua could fabricate a valid triple. + - **Preflight was missing replaceability** (Q#JR14f). Exact display + also refuses a window dedicated to another buffer + (`editor_core.rs:3566`), so a live destination holding its expected + buffer could still refuse *after* dired claimed and painted — rev 2's + hidden-buffer failure through another door. + - **N8's falsifier was inverted** (§6.1). Both a claim and a raise give + `proceed == false`, so keying the fallback on `proceed` alone is + *correct*; `errors` decides the extra report, not the fallback. +- rev 5 (2026-07-26) — review round 4. The remaining predicate input and + acceptance details: + - **Replaceability now names the incoming buffer** (Q#JR14f). The + shared predicate takes `Option` and serves all three existing + consumers: exact display passes its requested buffer, + `probe_display_target` passes its existing-buffer result, and + `commit_to` passes `None` because dired's replacement does not exist + yet. Thus a destination dedicated to its still-current bootstrap + buffer is refused before dired mutates anything. + - **N6c is executable:** the first listener catches the userdata + mutation rejection and declines, the second verifies the token stayed + unchanged and declines, and only the fallback commits. + - The dired accessor spelling, revision heading, and Stage 2 ledger + claim are corrected. + +- rev 6 (2026-07-26) — **corrections found while implementing**, not a + new design round. Four, all confirmed against the tree: + - **Q#JR3 was false.** `replace_active_buffer` does *not* drop the + startup scratch buffer; its body is one `switch_active_buffer` call, + which reassigns `aw.buffer_id` and removes nothing. The claim came + from that function's own doc comment (`editor.rs:1071`), which has + been wrong for as long as it has existed, and rev 5 propagated it + into §2.2, §3, P4, and the decision list without checking the body. + Corrected in all four places; the stale comment is corrected in this + PR too, since this PR would otherwise add *more* false references to + it. **Actually removing the stale scratch is separate work** — + buffer-lifetime changes have their own consequences (what else holds + the id, what `C-x b` lists) and are not smuggled into a directory-open + stage. + - **The daemon bootstrap could report the wrong buffer** (§4.5). The + directory arm captured `dest.buffer`, ran the resolver chain + *synchronously*, then returned the captured id — so a handler that + opened something synchronously (through `commit_to`, the supported + way) had already replaced the window's buffer, and the reply would + pair one buffer's snapshot with another's identity. The early return + also skipped the post-hook revalidation this framing claimed stayed + active. Rev 6 decides: **report what the window actually holds after + the dispatch**, and rehome through `non_side_target` exactly as the + file arm does. + - **N11 tested neither `RET` nor self-insert.** It called + `display_file` and `buf:insert` directly, so it stayed green with + dired's `RET` binding, its entry dispatch, and the editor's + self-insert path all broken — most of what "the journey works" means. + Both gestures are now dispatched as real keys. + - **P7 was vacuous and is removed, not weakened.** Q#JR12 has nothing + to pin: `run` computes `had_file = file.is_some()` and a directory + path is `Some` like any other, so suppression is structural and the + named mutation would require inventing the branch first. The rev 5 + test additionally never armed restore and hard-coded `had_file`, so + it asserted nothing about `run`. Q#JR12 is downgraded to an + observation. + +- rev 7 (2026-07-26) — **found while writing the `commit_to` suite and + bite-testing it.** Three, all confirmed: + - **N4 did not pin what its comment claimed.** Deleting the + `ScopedFrontend` arm from `acting_frontend` left N4 green, because + `ScopedFrontend::enter` *also* swaps `core.active_frontend` and the + ambient fallback then answers correctly on its own. The arm is + load-bearing in exactly one situation — a commit reached from inside + an interactive command, where the origin sits between the override + and the ambient value and would otherwise win. **N4b** is added, + driven through `dispatch_key` (the only thing that establishes an + interactive origin), and the mutation now bites it. The general + lesson is the §6.0 one again from a new angle: two mechanisms that + agree on the common path make either one look load-bearing. + - **`commit_to`'s forged-destination message was unreachable.** With + the parameter typed `mlua::AnyUserData`, mlua rejected a table during + argument conversion, so a caller who fabricated one got "error + converting Lua table to userdata" — true, but naming neither the rule + nor how to obtain a real destination. The parameter is now + `mlua::Value` and the pointed message actually fires. The refusal is + unchanged; only its legibility is. + - **P1 and P2 also fail on full revert**, since `commit_to` does not + exist on the pre-image. §6.0's "legitimately green on the pre-image" + does not describe them. They stay in the P list because their + *discriminating* falsifier is the named mutation, not the revert: a + revert-only check cannot distinguish "validates" from "validates in + time", which is the entire claim. Noted at each pin rather than + silently mislabelled. + - Bite results recorded: mutation A (scope stops swapping + `core.active_frontend`) fails N6a and P3 and nothing else; mutation B + (preflight moved after the callback) fails P1 and P2 and nothing + else; mutation C (drop the `ScopedFrontend` arm) fails N4b and + nothing else. + +- rev 8 (2026-07-26) — **review of PR #182.** One implementation gap and + two stale claims: + - **dired did not honor the captured window.** §4.4 specified + `display{ window = dest:window() }`; the implementation still ended + in `pmacs.window.switch_buffer`, which targets whatever window the + *scoped frontend* has selected. The scope pins the frontend; it does + not pin the window. So a split or panel that took focus while + `read_dir` was pending received the listing, and `prev` was captured + from it too — with every preflight check passing, because the + captured window was still live and still held its captured buffer. + Fixed in both places (`display` and the `prev` read), and **N4c** + added. The suite's routing pins all varied *frontend* identity; + none varied the selected window within one frontend, which is why + 23 green pins missed it. + - **The §0 scorecard row still graded §2 "Broken at entry"** while §2's + own ground truth had been rewritten — the scorecard is a second copy + of the same claim and §25's update protocol covers both. §19's row + and ground truth were stale in the same way (this PR creates the + first cross-subsystem suite) and are corrected too. + - **P4 still said "leaves exactly one buffer"**, the exact claim rev 6 + corrected as false everywhere else. Restated to what it actually + pins — the file is in the *active window* — matching the test that + was already written correctly. + +--- + +## 0.5. Coherence impact (`COHERENCE.md` §20, required since #163) + +- **Journey steps.** §20's first-named arc; 1a takes the broken half of + **step 3**. After 1a, `pmacs .` opens the directory. Steps 4 and 6–12 + do not change grade. §2's verdict table and §20 Priority 1's "State: + broken at step 3" line are rewritten in this PR per §25. +- **Interaction islands: adds none, removes one.** No new keymap, mode, + or modal surface; the directory arm routes into #165's dired buffer — + the "must not invent a second directory surface" constraint. The + unification (§3) removes an island: startup and the daemon bootstrap + resolve paths through two independently-written implementations today. +- **Config registry.** Adds no keys. The directory fallback is a function + slot, not a setting — `ConfigValue` is four scalars and a handler is + none of them (the reason terminal profiles could not be settings, #173). +- **Ownership, stated honestly (rev 3).** That slot is an **unowned + singleton**: last writer wins, no owning package, no `SourceLocation`, + no removal lifecycle, and it does not appear in any inspection surface. + That is a real §13 gap and this framing does not dress it up — §20 + Priority 3 is deliberately deferred, and 1a is not the place to invent + ownership machinery for one slot. **Named migration:** when Priority 3 + lands registration ownership and `pmacs.hook.remove`, the slot becomes + an ordinary lowest-priority hook subscription carrying its owner, and + this primitive is deleted rather than extended. +- **Background-work attribution (§9).** No new `JobKind` variant, no new + `PendingJob` field; the listing uses `pmacs.fs.read_dir`, whose kind + #165 added. Neutral. +- **Frontend parity (§16).** Both frontends get the behavior from the + same primitive. One asymmetry ships knowingly: the GPU path displays + its pre-existing bootstrap buffer until the listing settles (§8 B2). +- **New substrate (rev 3, revised rev 4–5).** `commit_to` (§4.4) is a + general fix for a general problem — *every* post-await + `pmacs.window.*` call in the tree acts on the ambient frontend by + documented design (`dired.lua:68-73`). 1a introduces it for one caller + and does not migrate the others; that migration is named as deferred + rather than smuggled in. It adds a **scoped frontend override** + distinct from `InteractiveCommandOrigin` (§2.11), deliberately: a + background continuation gets destination scope **without** acquiring + interactive-command authority, which keeps the "programmatic vs + interactive" distinction the unfold guard, command boundaries, and the + terminal surface all depend on. + +--- + +## 1. What Stage 1a ships + +1. **`pmacs .` opens the directory**, on the local TUI path and the + daemon/GPU bootstrap path, routed into #165's dired buffer. +2. **One path-resolution primitive** — `EditorState::open` adopts + `EditorCore::resolve_target_buffer` wholesale. +3. **A scoped-destination commit primitive** (§4.4) so an async open + lands where it was requested, or nowhere. +4. **The first cross-subsystem journey acceptance suite** (§19). + +Not in 1a — Stage 1b: a compile keybinding and `cargo build`/`test` +defaults from the existing `ProjectKind::Cargo`, LSP spawn-failure +guidance (§1.2), a welcome buffer. + +--- + +## 2. Ground truth (scouted 2026-07-26, `main` @ `d400f30`; re-verified rev 4) + +### 2.1 `pmacs .` still exits 1, and why + +`load_file` (`src/file_io.rs:81`) does `File::open` — which succeeds on a +directory — then `read_to_end`, returning `EISDIR`. Not +`ErrorKind::NotFound`, so every `NotFound` arm is skipped and the error +propagates; `main` prints and exits (`src/main.rs:400-403`). + +### 2.2 There are two path-open implementations, not one + +`resolve_target_buffer` (`editor_core.rs:885`) documents itself as *"One +primitive, so two path-normalization, dedup, and hook transactions cannot +drift apart."* Callers: `display_file` (`window_panel.rs:402`) and the +daemon bootstrap (`daemon.rs:1641`). **Local startup is not one of them** +— `EditorState::open` (`editor.rs:757`) hand-writes the same shape. + +| | `EditorState::open` | `resolve_target_buffer` | +|---|---|---| +| Stored buffer path | **normalized** — `set_buffer_path` normalizes internally (`editor_core.rs:810-822`) | **normalized** — same setter | +| Displayed name | `path.display()` raw (`editor.rs:772`) | `path.display()` raw | +| `NotFound` arm | empty path-backed buffer, `[new file]` | identical | +| Dedup | none | `find_buffer_for_path` | +| Window install | `replace_active_buffer` — switches the ACTIVE window (`editor.rs:797`). **It does not drop the startup scratch** (rev 6): its body is one `switch_active_buffer` call, which reassigns `aw.buffer_id` and removes nothing. The doc comment claiming otherwise was wrong before this stage and is corrected in this PR | none; caller installs | +| Error type | `io::Error`, bare | `String`, prefixed `cannot open {path}: ` | + +**The two agree on every observable except the error prefix and the +window install.** Rev 1 claimed a raw-vs-normalized split and built a +decision, a bet, and an acceptance on it; all three were withdrawn in rev +2. Rev 3 draws the further consequence the review identified: because the +implementations already agree, **no equivalence assertion can prove the +unification happened** — such a test passes on the pre-image. §6.0 +restructures the acceptance list around that. + +The unification's value is therefore (a) the directory arm reaching +startup once rather than being written twice, and (b) closing drift the +primitive was created to prevent and did not. Not a behavior fix. + +### 2.3 dired creates its own buffer and refuses adoption + +`claim_handle` (`dired.lua:486`) creates the buffer, applies the +read-only intercept, `set_round_trip_input`, and the `dired` major mode. +Its comment is explicit that finding a buffer by name is **not** +adoption. Handles are pathless. No Lua `buffer.set_name` / +`set_file_path` exists; dired Stage 2 (PR #171 §5) is scoped to add one. + +### 2.4 The listing is async; the bootstrap reply is not + +`read_listing` (`dired.lua:462`) awaits `pmacs.fs.read_dir` and its +comment says *"Must run inside `pmacs.async`"*. The daemon bootstrap is +one synchronous block: `open_initial_target` (`daemon.rs:1624`) → +`initial_target_snapshot` (`:1823`) → `InitialTargetResult::Opened` +(`:1888`), with the GPU frontend blocking on the reply before creating +its window (`pmacs-gpu/src/attach.rs:551`). + +`tick_async` resuming a coroutine in the frame its result arrives does +**not** bound the listing to one frame — the worker must still finish. +`tests/dired_acceptance.rs:103`'s `pump` drives until parked-coroutine +*and* pending-job counts both reach zero: *"nothing dired does is +observable until this returns."* + +### 2.5 Post-await, dired acts on the ambient frontend — by design + +`dired.lua:68-73`: *"`pmacs.window.*` calls made after the await act for +the **ambient** active frontend, since interactive origin does not +survive the tick boundary; and `pmacs.editor.move_to_line` acts on the +ambient **buffer**, which is why every post-await re-seat is guarded."* + +Correct for an interactive `C-x d`. Wrong for a startup open that must +land in a specific frontend's specific window. + +**And the ambient reach is wider than `display`.** `open_directory` +(`dired.lua:607-655`) after the await, in order: + +1. `read_listing` — the await; +2. `handle_for_path(canonical)` / `claim_handle(canonical)` — **creates a + buffer**, applies intercept/mode, registers a handle; +3. assigns `entries`, `errors`, `sort_mode`; +4. `handle.prev = pmacs.window.buffer()` — **reads the ambient buffer**; +5. `paint(handle)` — mutates the buffer; +6. `display(handle, opts, departed)` — the first call that could refuse; +7. `seat_cursor` — `move_to_line` on the ambient buffer; +8. `kill_departed`. + +`lookup_window` refuses a foreign window id (`window_panel.rs:202-212`), +but only at step 6. **Rev 2's "fails closed, does nothing" was false**: +steps 2–5 have already run. A refusal leaves a hidden dired buffer and a +registered handle, and step 4 can capture an unrelated frontend's buffer +as `prev`. §4.4 fixes this by revalidating and scoping *before* step 2. + +### 2.6 Subscribers exist before the hook fires — but ordering is fixed + +`EditorState::new()` loads the builtin runtime (dired at `editor.rs:539`) +then user `init.lua` (`:609`, `cfg(not(test))`). `HookRegistry::add` +**appends** (`hook.rs:240`); no prepend, no priority, no removal +(`COHERENCE.md` §13 names `pmacs.hook.remove`'s absence as a Priority 3 +prerequisite). **A builtin subscriber always runs before any user +subscriber, forever.** + +### 2.7 Short-circuit cannot distinguish a claim from a crash + +`run_short_circuit` (`hook.rs:299-323`) returns `proceed: false` for a +literal `false` return **and** for a raising callback; only +`HookOutcome.errors` (non-empty in the second case) tells them apart. +A resolver chain that keys only on `proceed` treats a broken user +callback as a successful claim. Q#JR15 decides the policy. + +### 2.8 `open_initial_target` reasserts after hooks + +It re-checks the buffer exists (`daemon.rs:1665-1670`) then reinstalls it +into the origin document window, rehoming if a hook closed it +(`:1673-1690`). §4.5's design does not fight this. + +### 2.9 This deliberately supersedes part of the GPU initial-target framing + +`docs/gpu-initial-target-framing.md` Q#GT6 (`:278`) lists `IsADirectory` +among initial-target failures; its acceptance 10 (`:550`) requires *"a +directory/permission-denied target returns a specific failure before +ready/window creation"*. **1a supersedes the directory half only.** +Permission-denied, invalid path bytes, session teardown, and the +"existing daemon remains connectable" clause keep their contract. The +superseded assertions are amended in that framing in this PR, per §25. + +### 2.10 `display_file`'s directory failure is load-bearing today + +`builtin/commands/default.lua:724` wraps `display_file` in a `pcall` +whose comment says *"only a real failure (a directory, a permission +error) reaches here"*, pinned by +`find_file_accepting_a_directory_reports_instead_of_raising` +(`tests/find_file_acceptance.rs:235`). §4.6 answers to it. + +### 2.11 There is a scope mechanism, and it is the wrong one + +`acting_frontend` (`window_panel.rs:46-50`) reads +`InteractiveCommandOrigin` app data, falling back to +`core.active_frontend_key()`; `InteractiveCommandOrigin::enter(fid)` +(`editor.rs:63-69`) returns an RAII guard. Rev 3 proposed reusing it. +Two independent reasons it cannot be: + +**(a) It does not scope what rev 3 claimed.** Only the window-panel +bindings consult `acting_frontend`. Two of dired's post-await steps do +not go through it at all: + +- **no-arg `pmacs.window.buffer()`** — step 4's `prev` capture — reads + `core.active_buffer_id()` directly (`mod.rs:12547`). Its comment is + explicit that this is deliberate and infallible, and states the + assumption it rests on: *"dispatch sets `active_frontend` to the acting + frontend before running a command, so the two agree on every real + path."* +- **`pmacs.editor.move_to_line`** — step 7's cursor seating — is + `cc.borrow_mut().move_to_line(line)` on the core's ambient active + window (`mod.rs:12703`). + +So entering the interactive origin would scope `display` and leave `prev` +capture and seating ambient — precisely the two steps §2.5 identifies as +corrupting. + +**(b) It is authenticated user-command authority, and a startup +continuation must not impersonate one.** `InteractiveCommandOrigin` is +what distinguishes a user command's edit from a plugin's or the data +API's. Three consumers would be misled: + +- the **pre-edit unfold** guard (`mod.rs:1385-1400`), whose doc calls it + *"the scoped authority that distinguishes a user command's edit from a + plugin's or the data API's programmatic one"* — dired's `paint` would + satisfy it and unfold at the edit site; +- `invoke_interactive`'s command-boundary rotation (`:5400`), which + raises without it and would silently succeed with it; +- `terminal_command_frontend` / `active_terminal_view_key` (`:8515`, + `:8527`), which treat its presence as "an interactive frontend context". + +Q#JR14e therefore introduces a **separate** override. Note that the +`window.buffer()` comment above is not an obstacle but a specification: +swapping `core.active_frontend` for the scope's extent is exactly what +makes its stated assumption true for a continuation, restoring the +invariant rather than working around it. + +--- + +## 3. The unification (Q#JR1) + +`EditorState::open` becomes a thin caller of `resolve_target_buffer`, +keeping `replace_active_buffer` (which switches the **active** window, +Q#JR3 as corrected in rev 6 — it does not destroy the old scratch, and +never did) +and keeping its "fire the hook after the core borrow ends" structure +(`editor.rs:786-795`) — listeners re-enter `pmacs.editor.*` and re-borrow +the core (Q#JR1a). + +**Q#JR4** — startup errors gain the `cannot open {path}: ` prefix. +`pmacs /root/secret` names the file, which today's bare message does not. +This is the *only* user-visible change from the unification (§2.2). + +**Q#JR12 (downgraded to an observation, rev 6)** — a directory argument +suppresses desktop restore, on Q#DS7's reasoning that a positional +argument means "open this" rather than "restore my session". This needs +no work and cannot be pinned: `run` computes `had_file = file.is_some()` +(`editor.rs:3152`), and a directory path is `Some` like any other, so +there is no directory-specific branch that could get it wrong. Rev 5 +carried an acceptance for it; that test never armed restore and +hard-coded `had_file`, asserting nothing, and is removed rather than +repaired. + +--- + +## 4. The directory arm, the resolver, and the destination + +### 4.1 Q#JR5 — a typed result + +```rust +pub enum ResolvedTarget { + Buffer { id: BufferId, fire: HookKind }, + Directory { path: PathBuf }, // normalized: absolute, ~-expanded, lexically clean +} +``` + +Rev 1's `(Option, HookKind)` admitted states that cannot occur. +`resolve_target_buffer` checks `path.is_dir()` ahead of the load. + +**Q#JR8** — the `Directory` variant carries an explicitly normalized +path. It is *not* free: normalization lives inside `set_buffer_path`, and +this arm creates no buffer, so nothing would normalize anything and the +local caller would still hold `"."`. Same lesson as the Lean 4 arc's URI +affinity — a handler keying state by path must never receive `"."`. + +**Q#JR5b** — `editor_core::HookKind` and `hook::HookKind` are unrelated +types sharing a name; both are written path-qualified in every file this +PR touches, and `window_panel.rs:37`'s bare import is changed to match. + +**Q#JR6** — Rust creates no buffer for a directory. A placeholder needs +reaping, is reinstalled by §2.8's reassert, and — if dired adopted it — +would drag in dired Stage 2's rename prerequisite (§2.3). + +### 4.2 Where the directory arm is consumed + +`EditorState::open` and `open_initial_target` dispatch the resolver +chain. `display_file` does not (§4.6). + +### 4.3 Q#JR7 — a user-only hook, then a replaceable fallback + +Given §2.6, "a package subscribes ahead of dired" is unreachable. So the +two roles are split: + +**The chain.** `path.open-directory`, `kind = "short-circuit"`, fired +first. Returning `false` claims the directory and stops the fan-out. **No +builtin subscribes** — the rule that makes "user code runs first" true +under append-only registration, stated in the hook's own description. + +**The fallback.** If unclaimed, the arm calls the directory handler — a +function slot defaulted by `dired.lua`: + +```lua +pmacs.path.set_directory_handler(function(path, dest) + open_async(path, { dest = dest }, nil, "dired") +end) +``` + +Users replace it, chain it (capture the previous value first), or +**disable** it (`set_directory_handler(nil)`), which is what makes +acceptance 10's unclaimed path reachable. It is an unowned singleton +slot, with the honest accounting and named migration in §0.5. + +**Q#JR15 (new) — a raising callback stops the chain *and* suppresses the +fallback.** §2.7 shows `proceed` alone cannot distinguish a raise from a +claim. Policy: inspect `HookOutcome.errors`; when non-empty, report +through `*errors*` **and** `pmacs.editor.set_status`, and do **not** run +the fallback. Rationale: this preserves the existing short-circuit +contract (a raising `buffer.before-save` callback already vetoes the +save), and running the fallback after a user's resolver crashed would +open dired on a directory the user's code may have been mid-way through +handling. The cost — a broken user callback disables directory opening +until fixed — is visible, reported through two surfaces, and preferable +to silently ignoring the user's resolver. + +*Deferred, named:* hook priority/prepend is the general fix for §2.6 and +belongs with `pmacs.hook.remove` in Priority 3. When it lands, the +fallback becomes an ordinary lowest-priority subscription. + +### 4.4 Q#JR14 (rev 5) — the scoped-destination commit + +**The blocker rev 2 missed:** §2.5 shows dired mutates handle state at +steps 2–5 and only reaches a refusable call at step 6. "Fails closed, +does nothing" was false — a refusal left a hidden buffer, a registered +handle, and a `prev` captured from whichever frontend happened to be +ambient. Per the review's decision, **1a carries the substrate fix.** + +**Q#JR14d — the destination is an opaque capability, not a table.** +`dest` is **nonconstructible userdata**, created only by Rust, holding +three private ids: + +| field (private) | source | purpose | +|---|---|---| +| frontend | local: `FrontendId::LOCAL`; bootstrap: the attaching `frontend_id` | the scope to commit in | +| window | local: the active window; bootstrap: `origin_window` (`daemon.rs:1637`) | where the listing goes | +| buffer | the buffer that window holds at capture time | **stale-intent detection** | + +A table would be wrong in two ways, both reachable: the *same* `dest` is +passed to every hook listener in turn, so an earlier listener could +mutate it and then decline — redirecting later listeners or the fallback +— and any Lua could fabricate a plausible triple and call `commit_to` +directly. Userdata makes both unrepresentable rather than merely +discouraged. + +The only accessor is read-only `dest:window()`, which dired needs for its +exact `display{window = …}` target. `commit_to` accepts **only** this +userdata and revalidates its private contents itself; it never trusts a +caller-supplied id. + +**Q#JR14e — a separate scoped frontend override, not the interactive +origin.** §2.11 gives both reasons. Rev 4 adds a distinct app-data +override with resolution order: + +``` +acting_frontend = scoped override → interactive origin → ambient +``` + +Its RAII guard **also** swaps `core.active_frontend` and restores it on +drop, which is what covers the core-ambient APIs `acting_frontend` never +sees (`window.buffer()` no-arg, `move_to_line`). `commit_to` does **not** +enter `InteractiveCommandOrigin`, so a startup continuation never +acquires interactive-command authority. + +**The primitive.** `pmacs.window.commit_to(dest, fn)`: + +1. **Preflight, before running anything** — the destination's frontend + has a registered view; its window is live in that view's layout; the + window still holds the captured buffer (Q#JR14c); and the window is + **replaceable** (Q#JR14f). +2. On any failure, returns `false, reason` **without calling `fn`** — so + nothing is claimed, painted, or captured. +3. On success, enters the scoped override for the dynamic extent of `fn` + and calls it. Inside, `display{window = …}`, no-arg + `window.buffer()`, `move_to_line`, and every other ambient primitive + resolve against the captured destination — which is why a `frontend` + option on `display` alone would have been insufficient. + +**Q#JR14f — preflight must establish replaceability, through the same +predicate every exact-target probe and display uses.** Exact display +refuses a window that is `dedicated` unless it already shows the +*incoming* buffer (`editor_core.rs:3566`). The distinction is +load-bearing here: `dest.buffer` is the captured bootstrap buffer, not +dired's future buffer. Passing it as the incoming buffer would approve a +window dedicated to that bootstrap buffer; dired would then claim and +paint its different buffer, and exact display would refuse afterward — +rev 2's hidden-buffer failure through another door. + +The eligibility test is therefore extracted once, with the semantic +input `incoming: Option`: + +| caller | input | dedicated-window result | +|---|---|---| +| `display_buffer` exact-target arm | `Some(request.buffer_id)` | eligible only when already showing that buffer | +| `probe_display_target` | its existing `Option` | preserves today's load-before-placement probe contract | +| `commit_to` preflight | `None` | always ineligible — the replacement does not exist yet | + +`probe_display_target` already carries the correct `Option` +shape (`editor_core.rs:3470-3483`), so leaving it on a private copy while +sharing only the other two would preserve the same drift this extraction +exists to remove. Core unit coverage pins the three decisive rows: +dedicated + `Some(current)` is eligible; dedicated + `Some(other)` is +refused; dedicated + `None` is refused. + +**Q#JR14b — `fn` must not await.** The scope is an RAII guard on the +Rust stack; a yield inside it would let the guard's extent and the +coroutine's suspension diverge, restoring the override while the +continuation is still parked. `commit_to` sets a flag that `Handle:await` +checks and raises on, naming the rule. Enforced, not documented — pinned +by N6. + +**Atomicity, stated precisely.** `commit_to` is atomic **against +destination-precondition failure**: if any preflight check fails, no +callback runs and nothing is mutated. It is **not** a transaction over +the callback — if `fn` raises halfway through, `commit_to` restores the +scope and propagates, but whatever `fn` already mutated stays mutated. +Rolling that back would require dired to make its claim/paint sequence +undoable, which is a dired change well beyond 1a. What 1a guarantees is +that the *destination* checks happen before the first mutation, which is +the failure the review identified. + +**dired's change.** `open_directory` keeps `read_listing` (the await) +outside, then performs steps 2–8 inside a single `commit_to` callback, +displaying with `{ window = dest:window() }` rather than the ambient +`switch_buffer`. On a `false` return it reports through +`pmacs.editor.set_status` and returns, having mutated nothing. + +**Q#JR14c — stale intent loses to the user.** If the destination window +now holds a different buffer than at capture, the request is stale and +**fails closed**. Rev 2's window-only pin said launch intent overwrites +whatever the user did meanwhile; that was wrong, and it rested on B2's +"before the user can act", which §2.4 disproves — a large directory takes +many frames and the user can act in every one of them. The user's action +is newer information than the launch argument. + +**What this buys, stated as the review framed it:** competing frontend +activity no longer turns a valid startup request into a nondeterministic +no-op. A live, unchanged destination receives its listing regardless of +what other frontends did meanwhile. Fail-closed is reserved for a +destination that is genuinely dead or stale. + +*Deferred, named:* migrating dired's other post-await paths (`C-x d`, +tree descent/ascent, refresh) and every other ambient post-await +`pmacs.window.*` call in the tree onto `commit_to`. 1a introduces the +primitive for the startup path and does not sweep; the sweep is its own +PR with its own acceptance, and this framing does not pretend the general +problem is solved. + +### 4.5 Q#JR9 — what the bootstrap reply names, and what it shows + +`open_initial_target` on a `Directory` installs nothing: it dispatches the +resolver, then replies `Opened { buffer_id }` naming **whatever the +destination window holds once that dispatch returns** — re-read, not the +id captured beforehand (Q#JR9b, rev 6). + +The distinction is not academic. The chain runs **synchronously**. +dired's handler defers, because its listing must await; a user's resolver +is under no such obligation, and one that opens something synchronously +through `commit_to` — the supported way to do it — has already replaced +the window's buffer by the time the reply is built. Reporting the +captured id would pair one buffer's snapshot with another's identity, and +the frontend would render a document nobody asked for. + +Re-reading also subsumes the case where a hook closed the window, so this +arm rehomes through `non_side_target` exactly as the file arm's reassert +does, rather than returning early and skipping that check — which rev 5's +implementation did while this section claimed the revalidation stayed +active. + +Absent a synchronous claimant the re-read yields the buffer the window +already held, which is the ordinary case. + +**That buffer is not necessarily `*scratch*`.** `build_fresh_frontend_view` +clones **LOCAL's primary document buffer** (`daemon.rs:2997`) — M10.9 made +attaching frontends share LOCAL's buffer so overlays fire; the +bottom-panel arc narrowed it to the *primary document* buffer so a TUI +panel could not become a new frontend's document. If LOCAL holds a real +document, `pmacs --gpu .` briefly displays and snapshots that unrelated +document. + +**Decision: accept and document.** A bootstrap placeholder re-creates +everything Q#JR6 rejected to fix a transient, and the session genuinely +*is* showing LOCAL's document — the same thing a no-argument `--gpu` +attach shows. Acceptance N5 pins it with a deliberately non-scratch LOCAL +primary so it is observed rather than assumed. + +### 4.6 Q#JR13 — `display_file` keeps its directory error + +`display_file` does **not** dispatch the resolver. On +`ResolvedTarget::Directory` it raises: + +- the message names the path and the directory reason (an improvement on + the raw `EISDIR` text, and the only user-visible change here); +- the active buffer, window layout, and selected window are unchanged — + nothing created, nothing switched; +- `find_file_accepting_a_directory_reports_instead_of_raising` passes + **unmodified**. + +`display_file` is "put this file in a window", not a CLI router. Routing +it into dired would silently change `C-x C-f` on a directory, in a PR +about the CLI, through a `pcall` arm whose comment guarantees the +opposite. + +*Deferred, named:* Emacs's `find-file` does open dired on a directory, +and that is reasonable eventual behavior. It is a find-file UX decision +with its own acceptance, belonging to the dired arc or 1b. When taken it +is a small change at `default.lua:724`, and the pinned test above is what +gets deliberately rewritten. + +--- + +## 5. The journey acceptance suite (§19) + +New: `tests/journey_acceptance.rs`, seeded with steps 2 (launch +unconfigured), 3 (open a real project), and 5 (edit immediately), +driving the **real startup entry point** — a directory arm with no +production caller passes every direct-call test. Steps 6–12 enter as +later stages make them real; the file is a ratchet. + +Every dired-dependent assertion pumps to quiescence using +`dired_acceptance.rs:103`'s idiom (parked coroutines *and* pending jobs +at zero), never a fixed frame count (§2.4). + +--- + +## 6. Acceptance + +### 6.0 Two kinds of pin, and why the distinction matters + +Rev 2 asserted that every acceptance "fails with the change reverted". +The review is right that this cannot hold for preservation guards — and +rev 2's acceptance 6 was the proof: because both implementations already +agree on every observable (§2.2), an equivalence assertion passes on the +pre-image. **Behavioral equivalence cannot demonstrate structural reuse.** +The list is therefore split, and each preservation pin names the +*targeted mutation* it is bite-tested against: + +- **(N) New-behavior acceptances** — must fail on full revert. +- **(P) Preservation pins** — legitimately green on the pre-image; + falsified by a named targeted mutation, not by revert. + +That local startup reaches the new directory behavior is proven by N1, +not by any equivalence assertion — which is also why rev 2's acceptance 6 +is **removed rather than recast**: it proved nothing N1 does not. + +### 6.1 New-behavior acceptances (N) + +- **N1** `pmacs .` in a project directory exits 0 and, after pumping to + quiescence, the active buffer is dired's, listing that directory. + Today: exit 1. +- **N2** Daemon/GPU bootstrap with a directory initial target receives + `InitialTargetResult::Opened`, not `Failed`, and after quiescence the + document window shows the dired buffer. Supersedes the GPU framing's + acceptance 10 for directories (§2.9). +- **N3** `pmacs .` on an unreadable directory reports through dired's + status path and leaves the session running — no exit 1, no half-built + buffer. +- **N4 — delivery despite competing frontends (the blocker's positive + half).** Two registered frontends; a directory bootstrap for frontend + A; frontend B dispatches unrelated activity (buffer switch, window + focus) while the listing is in flight. After quiescence the listing is + in **A's** captured window, and B's active buffer and window are + unchanged. Falsified by reverting `commit_to` to the ambient + `switch_buffer`. +- **N4b — the scope outranks an *interactive origin*, added rev 7.** N4 + alone does not pin `acting_frontend`'s ordering claim: with the + `ScopedFrontend` arm deleted, N4 still passes, because `enter` also + swaps `core.active_frontend`. The arm matters only when an interactive + origin is set, which sits between the override and the ambient value. + A command dispatched by frontend B calls `commit_to` with A's + destination; the commit must still land in A's window. Falsified by + deleting the arm, or by ordering it after the interactive origin. +- **N5** Bootstrap with a deliberately **non-scratch** LOCAL primary + document buffer: the reply's `buffer_id` is that buffer, and after + quiescence the window shows dired (Q#JR9, §4.5). +- **N4c — the captured *window*, not the captured frontend's selected + one (added rev 8).** One frontend, two windows: capture a destination, + then split and move focus to the other window and give it a buffer of + its own, then run dired's handler path with the captured destination. + The listing lands in the captured window, the focused window is + untouched, and `q` returns to the buffer the *captured* window showed. + Falsified independently by restoring `switch_buffer` in dired's + `display` and by reading `prev` from the ambient window — both were + verified to fail only this pin. +- **N6 — `commit_to` scopes and restores, on every exit path.** Three + cases, each asserting that **both** the scoped override and + `core.active_frontend` return to their prior values: (a) `fn` returns + normally; (b) `fn` raises; (c) `fn` awaits and is refused (Q#JR14b). + Case (c) additionally asserts the raise names the rule. Rev 3 checked + only the interactive origin's restoration on the success path, which + §2.11 shows is neither the right value nor enough paths. Falsified by + dropping the flag, or by restoring on success only. +- **N6b — `commit_to` refuses a forged destination.** A Lua-constructed + table with plausible `frontend`/`window`/`buffer` fields is rejected as + a type error, and userdata cannot be constructed from Lua (Q#JR14d). + Falsified by accepting a table. *Rev 7:* the parameter is typed + `mlua::Value` and `commit_to` performs the check itself, so the refusal + names the rule — typed as `AnyUserData`, mlua rejected the table during + argument conversion with a message naming neither the rule nor the + remedy, leaving the pointed one unreachable. +- **N6c — a declining listener cannot redirect the destination.** Two + listeners: the first receives `dest`, attempts mutation inside `pcall`, + observes the read-only rejection, and declines; the second verifies + `dest:window()` still names the original window and also declines; then + the fallback commits there (Q#JR14d). Falsified by passing a shared, + mutable table. +- **N7 — the resolver chain.** `path.open-directory` is short-circuit and + first-claimant-wins, exercised through an **ordinary user-registered + listener** (no builtin subscribes, §4.3): two listeners, the first + returns `false`, the second must not run, and the fallback must not + run. Falsified by `all-must-succeed` or `accumulate`. +- **N8 — a raising callback suppresses the fallback *and* is reported + (Q#JR15).** A listener that raises: the fallback does not run, the + directory does not open, and the failure reaches both `*errors*` and + the status line. + *Falsifier, corrected in rev 4:* keying the fallback on `proceed` alone + is **already correct** for suppression — §2.7 shows a raise gives + `proceed == false` just as a claim does. `errors` decides the *report*, + not the fallback. So N8 is falsified by either (a) running the fallback + when `errors` is non-empty — i.e. treating a raise as a decline — or + (b) mutating the short-circuit outcome so a raise yields + `proceed = true`. Rev 3 named the inverse mutation, which does not + falsify anything. +- **N9** The hook and the handler receive a **canonical absolute path** — + firing on `.` from a known cwd delivers that cwd, not `"."` (Q#JR8). +- **N10** With the handler slot cleared and no listener claiming, + `pmacs .` exits **0**, leaves the bootstrap buffer in place, and sets a + status naming the path (Q#JR10). +- **N11** `pmacs .` → dired lists → `RET` on a listed file visits it → a + self-insert lands in **that file's** buffer. (Rev 1 self-inserted into + the dired buffer, whose intercept rejects every edit, `dired.lua:506`.) + +### 6.2 Preservation pins (P), each with its falsifying mutation + +*Rev 7 correction:* **P1 and P2 also fail on full revert** — `commit_to` +does not exist on the pre-image, so §6.0's "legitimately green on the +pre-image" does not describe them. They stay here because their +*discriminating* falsifier is the named mutation: a revert-only check +cannot distinguish "validates" from "validates in time", which is their +entire claim. P3–P8 are preservation pins in the strict sense. + +- **P1 — precondition failure is atomic (the blocker's negative half).** + **Three** destination failures, each asserted the same way — after + quiescence the buffer count is unchanged, **no dired buffer or handle + exists for that path**, no window's buffer changed, and a status names + the failure: + 1. **dead** — the destination window was closed; + 2. **stale** — its buffer was replaced (Q#JR14c); + 3. **ineligible** — it is `dedicated` to its still-current captured + buffer, but dired's incoming replacement does not exist yet + (Q#JR14f, completed rev 5). This is the case a preflight that + mistakenly passes `dest.buffer` as the incoming buffer approves and + `display` then refuses *after* dired has claimed and painted. + *Mutation:* move the preflight from before `claim_handle` to after + `paint` — rev 2's design. P1 fails on all three; rev 2's acceptance 3b + passes. *Second mutation, for case 3 specifically:* pass + `Some(dest.buffer)` instead of `None` to the shared eligibility + predicate while keeping liveness and stale-buffer validation. Only case + 3 fails — which is the point of separating it. +- **P2 — stale intent loses (Q#JR14c).** The user replaces the + destination window's buffer while the listing is in flight; their + buffer survives and dired does not overwrite it. + *Mutation:* drop `dest.buffer` from revalidation (rev 2's window-only + pin). P2 fails. +- **P3 — dired's existing handles are not corrupted.** With a dired + buffer already open in another frontend, a failed startup open leaves + that handle's `prev`, entries, and cursor untouched. + *Mutation:* restore the ambient `handle.prev = pmacs.window.buffer()` + outside the scope (§2.5 step 4). +- **P4 — startup shows the file in the *active window* (Q#JR3, corrected + rev 6, restated rev 8).** `EditorState::open` displays the loaded + buffer in the active window and no window is left showing the startup + scratch. It does **not** assert a buffer count: `replace_active_buffer` + does not drop the scratch buffer, and rev 5's "leaves exactly one + buffer" wording — which survived rev 6's correction here by oversight, + caught in review of PR #182 — asserted a guarantee the editor does not + make. + *Mutation:* replace `replace_active_buffer` with a bare + `install_buffer_in_window` into some other window. +- **P5 — the `NotFound` arm survives the refactor.** A nonexistent path + yields an empty path-backed buffer with `[new file]` and fires no hook. + *Mutation:* delete the `NotFound` arm from `resolve_target_buffer`. +- **P6 — `display_file` keeps its contract (Q#JR13).** It raises on a + directory naming path and reason; active buffer, layout, and selected + window unchanged; `find_file_accepting_a_directory_reports_instead_of_raising` + passes unmodified. + *Mutation:* route `display_file` into the resolver chain. +- **P7 — REMOVED in rev 6.** Q#JR12 is structural: `run` computes + `had_file = file.is_some()` and a directory path is `Some` like any + other, so there is no directory-specific branch to break and the named + mutation would have to invent one first. Rev 5's test never armed + restore and hard-coded `had_file`, so it could not fail against any + implementation. Removed rather than repaired — a green test that cannot + fail reads as coverage. +- **P8 — startup errors name the file (Q#JR4).** A non-`NotFound`, + non-directory failure produces a message containing `cannot open` and + the path. *(Legitimately N-shaped for the prefix, P-shaped for the + failure itself; listed here because the failure behavior is preserved + and only the message changes.)* + +`scripts/bite` runs over the new suite. A VACUOUS report on any N is a +blocker; each P's named mutation is run as its bite check, since revert +cannot falsify it. + +--- + +## 7. Deferred (named) + +- **Migrating the rest of the tree onto `commit_to`** (§4.4) — dired's + other post-await paths and every other ambient post-await + `pmacs.window.*` call. Its own PR, its own acceptance. +- **Hook priority / prepend**, with `pmacs.hook.remove`, in §20 Priority + 3 — at which point the fallback slot becomes an ordinary lowest-priority + subscription and §0.5's unowned-singleton gap closes. +- **True adoption (option B).** Rust creates the buffer, dired adopts — + one buffer, no transient — but it needs dired Stage 2's rename / + clear-path capability (§2.3). Dired Stage 3; Q#JR6 does not block it. +- **The bootstrap transient** (§4.5, §8 B2). +- **`C-x C-f` on a directory opening dired** (§4.6). +- **Multiple path arguments** (`main.rs:227`, `:232`, `:240`). +- **`pmacs .` opening a panel** rather than the document window. +- Stage 1b and the rest of §20 Priority 1. + +--- + +## 8. Bets + +- **B1 — "one thing opens a directory" holds.** If a picker and dired + should both run, short-circuit is wrong and the hook must become a + resolver returning a target. +- **B2 (corrected twice) — the bootstrap transient is acceptable.** The + window shows its pre-existing buffer **until the listing settles** — + not "one frame" (rev 1), and **not** "before the user can act" (rev 2): + §2.4 disproves the bound and Q#JR14c is the consequence — the user + *can* act, so stale intent must lose. The bet is only that the + transient is visually acceptable at process start. +- **B3 — withdrawn** (rev 2). There was no path-normalization change. +- **B4 — failing closed on a genuinely dead or stale destination is + better than guessing.** Narrowed in rev 3: it applies only after + revalidation says the destination is gone, not to any competing + activity (N4). +- **B5 — `commit_to`'s no-await rule is livable.** Every commit step + dired performs after the listing is synchronous today, so the rule + costs nothing here. If a future handler genuinely needs to await + mid-commit, the primitive needs a re-entrant design and this bet is + what will have failed. +- **B6 (rev 5) — extracting the eligibility predicate is + behavior-preserving.** Q#JR14f shares one predicate between + `commit_to`'s preflight, `probe_display_target`, and `display_buffer`'s + exact-target arm rather than writing a third copy. The bet is that the + two existing callers' behavior survives the extraction unchanged — + core unit tests pin the `Option` matrix, and + `bottom_panel_stage1_acceptance` catches placement-level drift, which + is why it is in the gate list. The alternative has no extraction risk + and a certain cost: a future eligibility rule added to one copy reopens + Q#JR14f's exact hole. Taking the risk tests can catch over drift they + cannot. + +--- + +## 9. Gates + +``` +cargo fmt --check +cargo clippy --workspace --all-targets -- -D warnings # own step +cargo test --lib +cargo test --lib --features crdt +cargo test --test journey_acceptance +cargo test --test dired_acceptance +cargo test --test find_file_acceptance # P6, unmodified +cargo test --test gpu_initial_target_acceptance # §2.9 supersession +cargo test --test theme_faces_acceptance # EditorState::open caller +cargo test --test m4_acceptance -- --skip basedpyright # 4 open() callers +cargo test --test bottom_panel_stage1_acceptance # commit_to touches display +PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu +cargo test --workspace -- --skip basedpyright +git diff --check +``` + +`m4_acceptance` and `theme_faces_acceptance` call `EditorState::open` +directly (§2.2) — the unification's blast radius. `find_file_acceptance` +and `gpu_initial_target_acceptance` encode contracts this PR preserves +(§4.6) and supersedes (§2.9). `bottom_panel_stage1_acceptance` is +included because `commit_to` scopes the frontend that `display`'s +placement policy resolves against and Q#JR14f extracts the exact-target +eligibility rule that suite already pins. + +--- + +## 10. Sequencing + +**1a implements after PR #177 merges.** #177 touches `src/daemon.rs` and +`src/editor.rs`; §2.8's reassert logic sits next to its census work. +#179 also touches `src/editor.rs`. + +No dired code is in flight — #169 and #171 are docs-only and no open PR +touches `builtin/runtime/dired.lua` (verified 2026-07-26). 1a's dired +change (a handler registration, plus wrapping `open_directory`'s +post-await commit in `commit_to`) does not collide with dired Stage 2, +which is unapproved for implementation. The `commit_to` wrap is a larger +dired change than rev 2's, touching the body Stage 2's rename work also +touches. + +**Rev 5 — decided: 1a stays ahead of dired Stage 2.** Stage 2 is not in +implementation, and the scoped commit boundary gives it a better shape to +build on than it would have had — a rename transaction across five path +owners is exactly the kind of multi-step commit that wants a validated, +scoped destination rather than ambient state. **Obligation this creates:** +when 1a lands, dired Stage 2 re-scouts and revises its framing around +`commit_to` before implementation; that revision is a prerequisite of +Stage 2's branch, recorded here and in `docs/active-work.md` so it is not +discovered late. + +--- + +## 11. Numbered decisions + +- **Q#JR1** `EditorState::open` adopts `resolve_target_buffer` wholesale. +- **Q#JR1a** The hook fires outside the core borrow. +- **Q#JR2** *Withdrawn (rev 2)* — its premise was false. +- **Q#JR3 (corrected rev 6)** Startup keeps using + `replace_active_buffer`, which switches the **active** window — not + because it drops the old scratch (it does not, and never did) but + because an `install_buffer_in_window` elsewhere would load the file + while leaving the user looking at scratch. Removing the stale scratch + buffer is separate work. +- **Q#JR4** Startup errors gain the `cannot open {path}: ` prefix. +- **Q#JR5** `resolve_target_buffer` returns a typed `ResolvedTarget`. +- **Q#JR5b** Both `HookKind` types are written path-qualified. +- **Q#JR6** Rust creates no buffer for a directory. +- **Q#JR7** `path.open-directory` is a short-circuit **user-only** chain; + builtins do not subscribe; dired is a replaceable fallback slot. +- **Q#JR8** `ResolvedTarget::Directory` carries an explicitly normalized + path. +- **Q#JR9** The bootstrap reply names the destination window's buffer — + absent a synchronous claimant, LOCAL's primary document buffer, not + necessarily scratch. Accepted and documented. +- **Q#JR9b (rev 6)** That id is **re-read after the dispatch**, and the + arm rehomes through `non_side_target` rather than returning early: a + synchronous resolver may already have replaced the buffer. +- **Q#JR10** An unclaimed directory with the handler cleared exits 0 with + a status message. +- **Q#JR12 (observation, rev 6)** A directory argument suppresses + desktop restore structurally, via `had_file = file.is_some()`. No work, + no pin. +- **Q#JR13** `display_file` keeps its directory-is-an-error contract. +- **Q#JR14** The destination `{frontend, window, buffer}` is captured at + resolve time; `commit_to` preflights and scopes the **entire** + post-await commit. +- **Q#JR14b** A `commit_to` callback must not await; enforced, not + documented. +- **Q#JR14c** Stale intent loses to the user: a replaced destination + buffer fails closed. +- **Q#JR14d** `dest` is nonconstructible userdata with a read-only + `window()` accessor — not a table a listener can mutate or Lua can + forge. +- **Q#JR14e** A **separate** scoped frontend override, resolved ahead of + the interactive origin and also swapping `core.active_frontend`. + `commit_to` never enters `InteractiveCommandOrigin`. +- **Q#JR14f** Preflight establishes **replaceability** via the same + `Option` eligibility predicate used by + `probe_display_target` and `display_buffer`; `commit_to` passes `None` + because its replacement does not exist yet. +- **Q#JR15** A raising resolver callback stops the chain **and** + suppresses the fallback, reported through `*errors*` and the status + line. + +--- + +## 12. Branch and PR plan + +One feature, one branch, one PR: `journey-stage1a-directory-open`. + +1. Commit this framing. +2. Unification (§3) + P4, P5, P7, P8. +3. `ResolvedTarget` + the directory arm + the resolver chain, fallback + slot, and error policy (§4.1–4.3) + N7, N8, N9, N10; `display_file`'s + preserved contract (§4.6) + P6. +4. The scoped frontend override + the shared eligibility predicate + (Q#JR14e, Q#JR14f), then `commit_to` and the opaque destination + (§4.4) + N4, N6, N6b, N6c, P1, P2, P3. The override and the predicate + extraction land first as separable core changes: both are testable + without dired, and the predicate's `Some(current)` / `Some(other)` / + `None` unit matrix plus `bottom_panel_stage1_acceptance` must prove the + extraction behavior-preserving before anything depends on it. +5. `tests/journey_acceptance.rs` (§5) + N1, N2, N3, N5, N11. +6. `COHERENCE.md` §2 verdict table and §20 Priority 1 rewritten per §25; + `docs/gpu-initial-target-framing.md` Q#GT6 + acceptance 10 amended for + the superseded directory case (§2.9); `docs/agent-handoff.md` §1 and + `docs/active-work.md` updated. diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index 15878cf..bd405ea 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -564,11 +564,21 @@ additive, on its own binding, and does not replace scroll-and-select. would replay, and it stops a periodically refreshed buffer accumulating rope clones that `read_only` guarantees nothing can ever pop. - **What remains of the lane:** `*compilation*` and listview panels still - rely on intercept-plus-round-trip and are still emptiable by - `M-x buffer.undo`. The primitive they need now exists and is proven, so - the remaining work is adoption plus a streaming-friendly variant - (`*compilation*` appends rather than replacing wholesale). + **What remains of the lane — four writer mechanisms over five + buffers**, not the two this section first named (round 5 found the + inventory short; round 6 found the corrected version misattributing + the search panel). Every remaining intercept-protected writer still + relies on intercept-plus-round-trip over a writable rope, and every one + is still emptiable by `M-x buffer.undo`: listview panels + (`listview.lua:60-61`); `compile.lua`'s `ensure_slot`, which serves + `*compilation*` and `*shell-command*` — **not** `*search-results*`, + which `compile.lua` names only in a predicate; the independent + `*search-results*` panel in `builtin/commands/default.lua:869`, with + its own intercept and writes; and dired buffers (`dired.lua:371`). The + primitive they need now exists and is proven, so the remaining work is + adoption plus a streaming-friendly variant — the three appending + buffers need it, while listview and dired already write whole-buffer + replaces and are the cheap half. **The CRDT half is closed too** (review round 3). Clearing the v0.1 stacks proves nothing in CRDT mode, where they are bypassed entirely and diff --git a/src/daemon.rs b/src/daemon.rs index e34a6d6..40e9fe2 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1627,18 +1627,97 @@ fn open_initial_target( // create and select a side window, and bootstrap must reassert the // requested buffer in a document window rather than overwriting a // panel merely because it became `view.active`. - let (origin_window, buffer_id, fire) = { + let (origin_window, resolved) = { let mut core = editor.core.borrow_mut(); core.active_frontend = frontend_id; let origin_window = core .primary_document_window(frontend_id) .ok_or_else(|| "attaching frontend has no document window".to_string())?; - let (buffer_id, fire) = core.resolve_target_buffer(&path)?; + let resolved = core.resolve_target_buffer(&path)?; + (origin_window, resolved) + }; + + // Journey Stage 1a (Q#JR6/Q#JR9): a DIRECTORY installs nothing. + // + // Nothing can be installed, because the listing that satisfies a + // directory open is asynchronous and this block is synchronous — the + // frontend is blocked on `InitialTargetResult` and will not create + // its window until it arrives, so there is no tick in which a + // listing could settle. The reply therefore names the buffer the + // fresh view's document window ALREADY holds, which is a valid, + // ready session; the listing replaces it a tick or more later. + // + // That buffer is NOT necessarily `*scratch*`: `build_fresh_frontend_view` + // clones LOCAL's primary document buffer. If LOCAL holds a real + // document, this session briefly displays and snapshots it. Accepted + // and documented rather than papered over with a placeholder buffer, + // which would need reaping and would be fought by the reassert below. + // + // `publish_to_replicas` is false for the same reason an `AfterSwitch` + // dedup sets it false: this buffer is pre-existing and already + // published, not freshly loaded here. + let (buffer_id, fire) = match resolved { + crate::editor_core::ResolvedTarget::Directory { path } => { + let dest = editor + .capture_directory_destination(frontend_id, origin_window) + .ok_or_else(|| format!("cannot open {}: no document window", path.display()))?; + editor.dispatch_directory_open(&path, dest); + editor.reconcile_panel_layout(frontend_id); + + // The reply must name what the window ACTUALLY holds now, not + // what it held before the dispatch. + // + // The chain runs synchronously. dired's handler defers (it + // spawns a coroutine for the listing), but a user's resolver + // is under no such obligation: a handler that opens something + // synchronously -- through `commit_to`, which is exactly the + // supported way to do it -- has already replaced this + // window's buffer by the time we get here. Reporting the + // captured id would then send the snapshot of one buffer and + // the identity of another, and the frontend would render a + // document nobody asked for. + // + // Re-reading also covers the case a hook closed the window, + // which is why this rehomes through `non_side_target` exactly + // as the file arm's reassert does rather than returning early + // and skipping that check. + let mut core = editor.core.borrow_mut(); + core.active_frontend = frontend_id; + let destination = if core + .views + .get(&frontend_id) + .is_some_and(|view| view.layout.iter_ids().contains(&origin_window)) + { + origin_window + } else { + core.non_side_target(frontend_id) + .map_err(|error| format!("cannot reselect {}: {error}", path.display()))? + }; + core.focus_window(frontend_id, destination); + let buffer_id = core + .windows + .get(&destination) + .map(|window| window.buffer_id) + .ok_or_else(|| format!("cannot reselect {}: window died", path.display()))?; + return Ok(OpenedInitialTarget { + buffer_id, + // False whether or not the chain replaced the buffer: an + // untouched destination is pre-existing and already + // published, and a buffer a synchronous handler installed + // went through the ordinary display path, which publishes + // on its own terms. + publish_to_replicas: false, + }); + } + crate::editor_core::ResolvedTarget::Buffer { id, fire } => (id, fire), + }; + + { + let mut core = editor.core.borrow_mut(); core.install_buffer_in_window(origin_window, buffer_id) .map_err(|error| format!("cannot select {}: {error}", path.display()))?; core.focus_window(frontend_id, origin_window); - (origin_window, buffer_id, fire) - }; + } match fire { crate::editor_core::HookKind::AfterLoad => { @@ -5022,6 +5101,177 @@ mod tests { ); } + /// **N2** (Journey Stage 1a) — a DIRECTORY initial target reaches + /// readiness instead of failing. + /// + /// This deliberately supersedes the directory half of the GPU + /// initial-target framing's Q#GT6 and its acceptance 10, which + /// required `IsADirectory` to fail before window creation. + /// Permission-denied and every other pre-readiness failure keep that + /// contract. + #[test] + fn initial_target_directory_reaches_ready() { + use crate::editor::EditorState; + use crate::protocol::FrontendId; + + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join("alpha.txt"), b"alpha\n").expect("write"); + + let mut editor = EditorState::new(); + editor + .lua_host + .lua() + .load("pmacs.lsp.config = {}") + .exec() + .expect("wipe lsp config"); + let fid = FrontendId(131); + let view = build_fresh_frontend_view(&mut editor, false, false); + editor.core.borrow_mut().register_frontend_view(fid, view); + + let opened = open_initial_target( + &mut editor, + fid, + InitialTarget { + path: dir.path().as_os_str().as_bytes().to_vec(), + cwd: dir.path().as_os_str().as_bytes().to_vec(), + }, + ) + .expect("a directory target must reach readiness, not fail"); + + // The reply names a live buffer in a live document window: a + // valid, ready session. The listing arrives later, asynchronously. + let core = editor.core.borrow(); + assert!( + core.registry.borrow().contains(opened.buffer_id), + "the reported buffer must exist so its snapshot can be sent" + ); + let active = core.views[&fid].active; + assert_eq!( + core.windows[&active].buffer_id, opened.buffer_id, + "the reported buffer is the one the document window shows" + ); + } + + /// **N5** — the bootstrap buffer is not necessarily `*scratch*`. + /// + /// `build_fresh_frontend_view` clones LOCAL's PRIMARY DOCUMENT + /// buffer, so when LOCAL holds a real document the fresh session + /// briefly displays and snapshots it. Q#JR9 accepts that rather than + /// introducing a placeholder; this observes it instead of assuming. + #[test] + fn initial_target_directory_reports_a_non_scratch_primary() { + use crate::editor::EditorState; + use crate::protocol::FrontendId; + + let dir = tempfile::tempdir().expect("tempdir"); + let doc = dir.path().join("already-open.txt"); + std::fs::write(&doc, b"local document\n").expect("write"); + + // LOCAL holds a real document, not scratch. + let mut editor = EditorState::open(doc.clone()).expect("open"); + editor + .lua_host + .lua() + .load("pmacs.lsp.config = {}") + .exec() + .expect("wipe lsp config"); + let local_primary = editor + .core + .borrow() + .primary_document_buffer(FrontendId::LOCAL) + .expect("LOCAL always has a document window"); + + let fid = FrontendId(132); + let view = build_fresh_frontend_view(&mut editor, false, false); + editor.core.borrow_mut().register_frontend_view(fid, view); + + let opened = open_initial_target( + &mut editor, + fid, + InitialTarget { + path: dir.path().as_os_str().as_bytes().to_vec(), + cwd: dir.path().as_os_str().as_bytes().to_vec(), + }, + ) + .expect("a directory target must reach readiness"); + + assert_eq!( + opened.buffer_id, local_primary, + "the bootstrap reply names LOCAL's primary document buffer, \ + which is a real document here rather than *scratch*" + ); + } + + /// **N2b (rev 6)** — a resolver that claims SYNCHRONOUSLY is reported + /// correctly. + /// + /// The bug this pins: the arm captured the destination buffer id + /// *before* dispatching the chain and reported that. The chain runs + /// synchronously, so a handler that opens something immediately — + /// through `commit_to`, the supported way — had already replaced the + /// window's buffer, and the reply paired one buffer's snapshot with + /// another's identity. + /// + /// Falsified by reporting the captured id instead of re-reading. + #[test] + fn initial_target_directory_reports_what_a_synchronous_handler_installed() { + use crate::editor::EditorState; + use crate::protocol::FrontendId; + + let dir = tempfile::tempdir().expect("tempdir"); + + let mut editor = EditorState::new(); + editor + .lua_host + .lua() + .load( + "pmacs.lsp.config = {} + claimed = pmacs.buffer.create('*claimed*') + pmacs.path.set_directory_handler(function(path, dest) + pmacs.window.commit_to(dest, function() + pmacs.window.display(claimed, { select = true }) + end) + end)", + ) + .exec() + .expect("install a synchronous handler"); + + let fid = FrontendId(133); + let view = build_fresh_frontend_view(&mut editor, false, false); + editor.core.borrow_mut().register_frontend_view(fid, view); + + let opened = open_initial_target( + &mut editor, + fid, + InitialTarget { + path: dir.path().as_os_str().as_bytes().to_vec(), + cwd: dir.path().as_os_str().as_bytes().to_vec(), + }, + ) + .expect("a claimed directory target must reach readiness"); + + // Compare by NAME: the reported id must be the handler's buffer, + // and naming it is what makes the failure legible when it is not. + let core = editor.core.borrow(); + let reported_name = core + .registry + .borrow() + .get(opened.buffer_id) + .expect("the reported buffer exists") + .name() + .to_string(); + assert_eq!( + reported_name, "*claimed*", + "the reply must name what the handler installed, not the \ + buffer captured before the dispatch" + ); + let active = core.views[&fid].active; + assert_eq!( + core.windows[&active].buffer_id, opened.buffer_id, + "…and that buffer is what the window shows" + ); + } + /// Bottom-panel §1.3 #1/#3/#21 — the three Projection producers whose /// only production caller is `dispatcher_loop`, pinned at the named /// seams that loop calls. Round 2 finding: reverting any of them to diff --git a/src/editor.rs b/src/editor.rs index 3b3b274..935ee4c 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -26,7 +26,6 @@ use unicode_width::UnicodeWidthStr; use crate::async_runtime::SharedAsyncRuntime; use crate::cell::{CellCoord, CellSize}; use crate::editor_core::EditorCore; -use crate::file_io::load_file; use crate::frontend::{Event, Frontend, KeyEvent, KeyEventKind, MouseEvent, install_panic_hook}; use crate::key::{Chord, display_sequence}; use crate::keymap_stack::{Action, KeyDispatcher}; @@ -80,6 +79,109 @@ impl Drop for InteractiveCommandOriginGuard { } } +/// A frontend scope for **background** work — deliberately NOT +/// [`InteractiveCommandOrigin`] (Journey Stage 1a, Q#JR14e). +/// +/// An async continuation (a settled directory listing, and eventually +/// any other post-await window work) needs to act for the frontend that +/// *requested* it rather than whichever one happens to be ambient when +/// the worker finishes. Reusing the interactive origin for that would be +/// wrong twice over: +/// +/// 1. **It does not scope enough.** Only `acting_frontend` consults it, +/// so `pmacs.window.display` would be scoped while no-arg +/// `pmacs.window.buffer()` (which reads `active_buffer_id()` +/// directly) and `pmacs.editor.move_to_line` (which mutates the +/// core's ambient active window) stayed ambient — and those are +/// precisely the calls that capture and seat. +/// 2. **It is authenticated user-command authority.** It is what +/// distinguishes a user command's edit from a plugin's or the data +/// API's: the pre-edit unfold guard, `invoke_interactive`'s +/// command-boundary rotation, and the terminal surface's "requires an +/// interactive frontend context" checks all key off it. A background +/// listing must not acquire any of that. +/// +/// So this is a separate slot, resolved *ahead* of the interactive +/// origin, whose guard **also** swaps `EditorCore::active_frontend` — +/// which is what covers the core-ambient APIs `acting_frontend` never +/// sees. That swap is not a workaround: `pmacs.window.buffer()`'s no-arg +/// arm documents its own correctness as resting on "dispatch sets +/// `active_frontend` to the acting frontend before running a command", +/// and this restores that invariant for a continuation. +#[derive(Clone, Default)] +pub(crate) struct ScopedFrontend(Rc>>); + +impl ScopedFrontend { + /// The override in force, if any. + #[must_use] + pub(crate) fn current(&self) -> Option { + self.0.get() + } + + /// Enter a background frontend scope, also swapping the core's + /// ambient `active_frontend`. Both are restored on drop, on every + /// exit path including a raising callback. + pub(crate) fn enter( + &self, + core: &SharedCore, + commit_scope: &CommitScopeActive, + frontend_id: FrontendId, + ) -> ScopedFrontendGuard { + let previous = self.0.replace(Some(frontend_id)); + let previous_active = { + let mut core = core.borrow_mut(); + let was = core.active_frontend; + core.active_frontend = frontend_id; + was + }; + let previous_commit = commit_scope.0.replace(true); + ScopedFrontendGuard { + scope: self.clone(), + core: core.clone(), + previous, + previous_active, + commit_scope: commit_scope.clone(), + previous_commit, + } + } +} + +pub(crate) struct ScopedFrontendGuard { + scope: ScopedFrontend, + core: SharedCore, + previous: Option, + previous_active: FrontendId, + /// Cleared together with the scope, so an awaiting callback cannot + /// leave `await` refused after the commit ends (Q#JR14b). + commit_scope: CommitScopeActive, + previous_commit: bool, +} + +impl Drop for ScopedFrontendGuard { + fn drop(&mut self) { + self.scope.0.set(self.previous); + self.core.borrow_mut().active_frontend = self.previous_active; + self.commit_scope.0.set(self.previous_commit); + } +} + +/// Whether a `pmacs.window.commit_to` callback is currently running +/// (Journey Stage 1a, Q#JR14b). +/// +/// Read from Lua as `pmacs._async._in_commit_scope()`; `Handle:await` +/// refuses while it is set. Lives beside the scope guard so the two can +/// never disagree. +#[derive(Clone, Default)] +pub struct CommitScopeActive(Rc>); + +impl CommitScopeActive { + /// Whether a commit callback is on the stack. + #[must_use] + pub fn active(&self) -> bool { + self.0.get() + } +} + // --------------------------------------------------------------------------- // EditorState // --------------------------------------------------------------------------- @@ -261,6 +363,13 @@ impl EditorState { let mut lua_host = LuaHost::with_registry(registry).expect("Lua runtime initialization"); let interactive_origin = InteractiveCommandOrigin::default(); lua_host.lua().set_app_data(interactive_origin.clone()); + // Q#JR14e/Q#JR14b: the background frontend scope and the + // commit-scope flag live only as Lua app data -- `commit_to` and + // `Handle:await` are the only readers, and both reach them that + // way. No `EditorState` field, so there is no second handle that + // could disagree with the one the guard restores. + lua_host.lua().set_app_data(ScopedFrontend::default()); + lua_host.lua().set_app_data(CommitScopeActive::default()); lua_host .attach_editor(&core) .expect("editor bindings + builtin chunks"); @@ -790,35 +899,65 @@ impl EditorState { /// Construct an editor for a path. Empty buffer with `[new file]` /// status if the path does not exist; loaded contents otherwise. + /// + /// Journey Stage 1a (Q#JR1): this is a thin caller of + /// [`EditorCore::resolve_target_buffer`], not a second + /// implementation of it. That primitive documents itself as "one + /// primitive, so two path-normalization, dedup, and hook + /// transactions cannot drift apart" — and local startup, which had + /// hand-written the same three-arm shape, was not one of its callers + /// until now. + /// + /// Two things this caller still owns, and must keep owning: + /// + /// * **The window install.** `resolve_target_buffer` deliberately + /// does not touch windows, so the caller places the buffer. + /// Startup uses [`Self::replace_active_buffer`], which switches + /// the ACTIVE window — an `install_buffer_in_window` into some + /// other window would load the file and leave the user looking at + /// scratch (Q#JR3). + /// + /// It does **not** destroy the scratch buffer, despite what + /// `replace_active_buffer`'s own doc comment has long claimed: + /// that function only calls `switch_active_buffer`, which + /// reassigns the window's `buffer_id` and removes nothing. The + /// startup scratch survives in the registry, and did before this + /// stage too. Changing that is buffer-lifetime work with its own + /// consequences (what else may hold the id, what `C-x b` should + /// list) and is deliberately not smuggled in here. + /// * **Firing the hook outside the core borrow.** Listeners + /// re-enter `pmacs.editor.*`, which re-borrows the core + /// (Q#JR1a) — the same reason the daemon bootstrap and + /// `display_file` both fire theirs after their borrow blocks end. + /// + /// A directory resolves to [`ResolvedTarget::Directory`] and is + /// dispatched to the directory resolver chain rather than opened as + /// a buffer (Q#JR6); see [`Self::open_directory_target`]. + #[allow( + clippy::needless_pass_by_value, + reason = "stable public entry point mirroring `pmacs PATH` and \ + `run(Option)`; the body stopped consuming the \ + PathBuf when this became a `resolve_target_buffer` caller, \ + and churning the signature would touch every caller for no \ + behavioral gain" + )] pub fn open(path: PathBuf) -> io::Result { - let display_name = path.display().to_string(); - let state = Self::new(); + let mut state = Self::new(); + let resolved = state + .core + .borrow_mut() + .resolve_target_buffer(&path) + .map_err(io::Error::other)?; let mut fire_after_load = false; - match load_file(&path) { - Ok((bytes, meta)) => { - let new_id = state - .lua_host - .registry() - .borrow_mut() - .create_from_bytes(display_name, &bytes); - state.replace_active_buffer(new_id); - let mut core = state.core.borrow_mut(); - core.set_buffer_path(new_id, Some(path)); - core.set_buffer_meta(new_id, Some(meta)); - fire_after_load = true; - Ok(()) + match resolved { + crate::editor_core::ResolvedTarget::Buffer { id, fire } => { + state.replace_active_buffer(id); + fire_after_load = matches!(fire, crate::editor_core::HookKind::AfterLoad); } - Err(e) if e.kind() == io::ErrorKind::NotFound => { - let new_id = state.lua_host.registry().borrow_mut().create(display_name); - state.replace_active_buffer(new_id); - let mut core = state.core.borrow_mut(); - core.set_buffer_path(new_id, Some(path)); - core.status = "[new file]".into(); - Ok(()) + crate::editor_core::ResolvedTarget::Directory { path } => { + state.open_directory_target(&path); } - Err(e) => Err(e), - }?; - let mut state = state; + } if fire_after_load { // Fire the hook *after* the borrow on `core` is released // (block above ends). Listeners may legitimately re-enter @@ -830,9 +969,147 @@ impl EditorState { Ok(state) } - /// Switch the active window to `buffer_id`, dropping any old - /// scratch buffer if the active window's previous buffer has no - /// other windows referencing it. Returns silently on a stale id. + /// Capture the destination a directory open must commit to + /// (Q#JR14), or `None` when `frontend` has no document window. + /// + /// Synchronous by necessity: the listing settles a tick or more + /// later, and by then the ambient frontend, selected window, and + /// active buffer may all name something else. + pub(crate) fn capture_directory_destination( + &self, + frontend: crate::protocol::FrontendId, + window: crate::window::WindowId, + ) -> Option { + let core = self.core.borrow(); + let buffer = core.windows.get(&window)?.buffer_id; + Some(crate::editor_core::DirectoryDestination { + frontend, + window, + buffer, + }) + } + + /// Local-startup directory open (Q#JR6): resolve the destination + /// from `LOCAL`'s document window and dispatch the resolver chain. + /// + /// Public because it is the whole of what `pmacs DIRECTORY` does + /// after resolution — acceptance drives this rather than + /// `resolve_target_buffer`, so a directory arm with no production + /// caller cannot pass. + pub fn open_directory_target(&mut self, path: &std::path::Path) { + // Canonicalize here as well as in the resolver arm. The two are + // not redundant: this is a public "open this directory" seam, so + // a caller that did not come through `resolve_target_buffer` + // must still hand the chain a canonical path (Q#JR8) --- and + // normalization is idempotent, so the startup path pays nothing. + let path = crate::editor_core::normalize_buffer_path(path.to_path_buf()); + let path = path.as_path(); + let window = self + .core + .borrow() + .primary_document_window(crate::protocol::FrontendId::LOCAL); + let dest = window.and_then(|window| { + self.capture_directory_destination(crate::protocol::FrontendId::LOCAL, window) + }); + let Some(dest) = dest else { + self.core.borrow_mut().status = + format!("cannot open {}: no document window", path.display()); + return; + }; + self.dispatch_directory_open(path, dest); + } + + /// Run the directory resolver chain for `path`, then its fallback + /// (Journey Stage 1a, Q#JR7/Q#JR15). + /// + /// Order is user chain first, builtin default second — see + /// `install_path_module` for why that cannot be expressed as two + /// hook subscriptions. + /// + /// **A raising listener stops the chain AND suppresses the + /// fallback.** `run_short_circuit` returns `proceed = false` both + /// for a literal `false` (a claim) and for a raise, so `proceed` + /// alone already suppresses correctly; `errors` is what distinguishes + /// them, and it decides only whether to *report*. Running the + /// fallback after a user's resolver crashed would open dired on a + /// directory that resolver may have been part-way through handling, + /// so a crash is treated as a claim that failed — reported through + /// the `*errors*` buffer (which `run_hook` already does) and the + /// status line (which it does not), and visible in both. + pub(crate) fn dispatch_directory_open( + &mut self, + path: &std::path::Path, + dest: crate::editor_core::DirectoryDestination, + ) { + let display = path.display().to_string(); + let args = { + let lua = self.lua_host.lua(); + let destination = + match lua.create_userdata(crate::lua_bindings::DirectoryDestinationLua(dest)) { + Ok(userdata) => mlua::Value::UserData(userdata), + Err(error) => { + self.core.borrow_mut().status = format!("cannot open {display}: {error}"); + return; + } + }; + let path_value = match lua.create_string(display.as_bytes()) { + Ok(string) => mlua::Value::String(string), + Err(error) => { + self.core.borrow_mut().status = format!("cannot open {display}: {error}"); + return; + } + }; + mlua::MultiValue::from_vec(vec![path_value, destination]) + }; + + match self.lua_host.run_hook("path.open-directory", args.clone()) { + // A listener raised. `run_hook` has already appended the + // record to *errors*; add the status line, and do NOT fall + // back (Q#JR15). + Some(outcome) if !outcome.errors.is_empty() => { + self.core.borrow_mut().status = + format!("cannot open {display}: a path.open-directory listener failed"); + return; + } + // Claimed: a listener returned false. + Some(outcome) if !outcome.proceed => return, + // Declined, or no listeners at all. + _ => {} + } + + let handler = { + let lua = self.lua_host.lua(); + lua.globals() + .get::("pmacs") + .and_then(|pmacs| pmacs.get::("path")) + .and_then(|path| path.get::("directory_handler")) + .unwrap_or(mlua::Value::Nil) + }; + let mlua::Value::Function(handler) = handler else { + // The slot is clear: nothing surfaces directories. The + // session started fine and simply has nothing to show for + // the argument, so this is a status message and NOT a + // startup failure (Q#JR10). + self.core.borrow_mut().status = format!("no handler for directory {display}"); + return; + }; + if let Err(error) = handler.call::<()>(args) { + self.core.borrow_mut().status = format!("cannot open {display}: {error}"); + } + } + + /// Switch the active window to `buffer_id`. Returns silently on a + /// stale id. + /// + /// **Corrected (Journey Stage 1a).** This comment previously claimed + /// it dropped "any old scratch buffer if the active window's + /// previous buffer has no other windows referencing it". It never + /// did: the body is one `switch_active_buffer` call, which reassigns + /// `aw.buffer_id` and removes nothing from the registry. The claim + /// was load-bearing enough that a framing decision (Q#JR3) and an + /// acceptance pin were written against it before anyone checked the + /// body. Removing the stale scratch may well be worth doing; it is + /// separate work, and this comment no longer promises it. fn replace_active_buffer(&self, buffer_id: crate::buffer::BufferId) { let mut core = self.core.borrow_mut(); let _ = core.switch_active_buffer(buffer_id); diff --git a/src/editor_core.rs b/src/editor_core.rs index 89432cc..7fd90c6 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -94,6 +94,77 @@ pub enum HookKind { None, } +/// What a path resolved to (Journey Stage 1a, Q#JR5). +/// +/// A sum type rather than `(Option, HookKind)`: that pair +/// admits three states that cannot occur (`None` with `AfterLoad`, +/// `Some` with a directory, …), and every caller would have to +/// re-establish by hand which combinations are real. +/// +/// **Do not confuse [`HookKind`] here with [`crate::hook::HookKind`]** — +/// unrelated types sharing a name. This one says *which* lifecycle hook +/// to fire; that one says how a hook's callbacks fan out. Every site +/// touching both writes them path-qualified (Q#JR5b). +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum ResolvedTarget { + /// A file buffer, plus the hook the caller must fire with the + /// destination window active. + Buffer { + /// The resolved buffer. + id: BufferId, + /// Which lifecycle hook this resolution owes. + fire: HookKind, + }, + /// A directory. No buffer is created (Q#JR6) — the directory + /// resolver chain decides what surfaces it, and dired builds its own + /// buffer through `claim_handle` rather than adopting one. + /// + /// `path` is **normalized** — absolute, tilde-expanded, lexically + /// clean. This is not free and must not be assumed: normalization + /// otherwise happens inside [`Self::set_buffer_path`], which never + /// runs on this arm, so a caller resolving `"."` would keep `"."` + /// (Q#JR8). A handler keying state by path needs the canonical form. + Directory { + /// The normalized directory path. + path: PathBuf, + }, +} + +/// Where a directory open was requested, captured **synchronously** at +/// resolve time (Journey Stage 1a, Q#JR14). +/// +/// The listing that satisfies a directory open is asynchronous +/// (`pmacs.fs.read_dir` is worker-dispatched and must be awaited), so the +/// code that finally builds and displays the listing runs a tick or more +/// later — outside interactive dispatch, where `pmacs.window.*` acts on +/// the *ambient* frontend by documented design (`builtin/runtime/dired.lua`). +/// Without a captured destination, a second frontend dispatching in the +/// meantime silently redirects the listing. +/// +/// All three fields are load-bearing: +/// +/// * `frontend` — the scope the commit must run in. +/// * `window` — the exact destination; the ambient selected window is +/// not it. +/// * `buffer` — what that window held at capture time, so **stale +/// intent loses to the user** (Q#JR14c). A user who replaced the +/// buffer while the listing was in flight is newer information than +/// the launch argument, and must not be overwritten. +/// +/// Exposed to Lua only as nonconstructible userdata (Q#JR14d): as a +/// table, the *same* value is handed to every resolver listener in turn, +/// so one could mutate it and then decline — redirecting later listeners +/// — and any Lua could fabricate a plausible triple. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct DirectoryDestination { + /// Frontend that requested the directory. + pub frontend: FrontendId, + /// Window the listing must land in. + pub window: WindowId, + /// Buffer that window held at capture time (stale-intent check). + pub buffer: BufferId, +} + /// A `display_buffer` request (Q#BP3). /// /// `height` and `dedicated` are deliberately option-valued at the policy @@ -880,18 +951,41 @@ impl EditorCore { /// One primitive, so two path-normalization, dedup, and hook /// transactions cannot drift apart. /// + /// A **directory** resolves to [`ResolvedTarget::Directory`] before + /// any load is attempted (Journey Stage 1a, Q#JR5/Q#JR6). Without + /// that arm the load runs and fails: `File::open` succeeds on a + /// directory and `read_to_end` then returns `EISDIR`, which is not + /// `NotFound`, so the `[new file]` arm never fires and every caller + /// saw a hard error — the reason `pmacs .` exited 1 and the golden + /// journey was graded broken at step 3 (`COHERENCE.md` §2). + /// /// # Errors /// Any load failure other than `NotFound`. - pub fn resolve_target_buffer(&mut self, path: &Path) -> Result<(BufferId, HookKind), String> { + pub fn resolve_target_buffer(&mut self, path: &Path) -> Result { + // Ahead of the load, deliberately: see the EISDIR note above. + if path.is_dir() { + return Ok(ResolvedTarget::Directory { + path: normalize_buffer_path(path.to_path_buf()), + }); + } match self.get_or_load_buffer(path) { - Ok((buffer_id, true)) => Ok((buffer_id, HookKind::AfterLoad)), - Ok((buffer_id, false)) => Ok((buffer_id, HookKind::AfterSwitch)), + Ok((id, true)) => Ok(ResolvedTarget::Buffer { + id, + fire: HookKind::AfterLoad, + }), + Ok((id, false)) => Ok(ResolvedTarget::Buffer { + id, + fire: HookKind::AfterSwitch, + }), Err(error) if error.kind() == std::io::ErrorKind::NotFound => { let display_path = path.display().to_string(); let buffer_id = self.registry.borrow_mut().create(display_path); self.set_buffer_path(buffer_id, Some(path.to_path_buf())); "[new file]".clone_into(&mut self.status); - Ok((buffer_id, HookKind::None)) + Ok(ResolvedTarget::Buffer { + id: buffer_id, + fire: HookKind::None, + }) } Err(error) => Err(format!("cannot open {}: {error}", path.display())), } @@ -3472,16 +3566,56 @@ impl EditorCore { fid: FrontendId, existing: Option, window: Option, + ) -> Result { + self.probe_display_target_inner(fid, existing, window) + } + + /// Whether `window` will accept `incoming` as its buffer — the one + /// dedication rule, shared by every consumer (Journey Stage 1a, + /// Q#JR14f). + /// + /// A dedicated window refuses anything other than what it already + /// shows; an undedicated one accepts anything. `incoming` is + /// deliberately optional, and the `None` case is not a degenerate + /// spelling of "don't care" — it means **the replacement buffer does + /// not exist yet**, and a dedicated window must therefore be treated + /// as ineligible: + /// + /// | caller | `incoming` | dedicated window | + /// |---|---|---| + /// | [`Self::display_buffer`] exact-target arm | `Some(request.buffer_id)` | eligible only when already showing it | + /// | [`Self::probe_display_target`] | its existing-buffer result | preserves the load-before-placement probe | + /// | `commit_to` preflight | `None` | always ineligible | + /// + /// `commit_to` passes `None` because a directory open's destination + /// is validated *before* the handler builds its buffer. Passing the + /// captured bootstrap buffer instead would approve a window + /// dedicated to *that* buffer, the handler would then claim and paint + /// a different one, and the exact display would refuse afterwards — + /// after the mutations the preflight exists to prevent. + /// + /// Extracted rather than reimplemented per caller: two copies of a + /// rule that must agree is exactly the drift this stage's + /// path-resolution unification exists to close, and a future + /// eligibility rule added to only one copy would reopen it. + #[must_use] + pub fn window_accepts_buffer(&self, window: WindowId, incoming: Option) -> bool { + self.windows.get(&window).is_some_and(|w| { + !w.params.dedicated || incoming.is_some_and(|buffer_id| w.buffer_id == buffer_id) + }) + } + + fn probe_display_target_inner( + &self, + fid: FrontendId, + existing: Option, + window: Option, ) -> Result { let view = self .views .get(&fid) .ok_or_else(|| format!("frontend {fid:?} has no window layout"))?; - let eligible = |id: WindowId| { - self.windows.get(&id).is_some_and(|w| { - !w.params.dedicated || existing.is_some_and(|buffer_id| w.buffer_id == buffer_id) - }) - }; + let eligible = |id: WindowId| self.window_accepts_buffer(id, existing); if let Some(target) = window { if !view.layout.iter_ids().contains(&target) { return Err(format!( @@ -3563,7 +3697,7 @@ impl EditorCore { .windows .get(&target) .ok_or_else(|| format!("display: window {} is not live", target.raw()))?; - if window.params.dedicated && window.buffer_id != request.buffer_id { + if !self.window_accepts_buffer(target, Some(request.buffer_id)) { return Err(format!( "display: window {} is dedicated to another buffer", target.raw() @@ -5300,6 +5434,45 @@ mod tests { assert!(s.active_window_for(FrontendId::LOCAL).is_some()); } + /// Journey Stage 1a (Q#JR14f): the three decisive rows of the shared + /// eligibility predicate. + /// + /// The `None` row is the one that exists for `commit_to`, and it is + /// not a "don't care": a directory open validates its destination + /// *before* the handler creates the buffer that will land there, so + /// there is no incoming id to compare and a dedicated window must be + /// refused. Approving it would let the handler claim and paint, and + /// the display would refuse afterwards — after the mutations the + /// preflight exists to prevent. + #[test] + fn window_accepts_buffer_matrix() { + let mut s = fresh(); + let window = s.views[&FrontendId::LOCAL].active; + let current = s.windows[&window].buffer_id; + let other = s.registry.borrow_mut().create(String::from("other")); + + // Undedicated: accepts anything, including "not decided yet". + assert!(s.window_accepts_buffer(window, Some(current))); + assert!(s.window_accepts_buffer(window, Some(other))); + assert!(s.window_accepts_buffer(window, None)); + + s.windows.get_mut(&window).expect("live").params.dedicated = true; + + // Dedicated: only what it already shows. + assert!( + s.window_accepts_buffer(window, Some(current)), + "a dedicated window still accepts the buffer it displays" + ); + assert!( + !s.window_accepts_buffer(window, Some(other)), + "a dedicated window refuses a different buffer" + ); + assert!( + !s.window_accepts_buffer(window, None), + "a dedicated window refuses an as-yet-unbuilt replacement" + ); + } + #[test] fn register_and_unregister_frontend_view() { // T M10.8 — the lifecycle API the dispatcher uses on attach diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index aa5de88..b624a00 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -3650,9 +3650,74 @@ fn install_path_module(lua: &Lua) -> mlua::Result { ) })?, )?; + // Journey Stage 1a (Q#JR7): the directory fallback. + // + // The resolver for a directory open is a two-tier arrangement, and + // the split is forced by how registration works rather than chosen + // for elegance. `path.open-directory` is a short-circuit hook that + // **no builtin subscribes to** — because `HookRegistry::add` only + // appends and builtins load before `init.lua`, a subscribing builtin + // would always claim first and no user listener could ever run. So + // the hook is the user's chain, and the default surface is this + // slot, consulted only when the chain declines. + // + // A slot, not a `pmacs.config` setting: `ConfigValue` is four + // scalars and a handler is none of them (the same reason terminal + // profiles could not be settings). It is an UNOWNED singleton — + // last writer wins, no owning package, no `SourceLocation`, no + // removal lifecycle, absent from every inspection surface. That is a + // real `COHERENCE.md` §13 gap, recorded rather than dressed up: when + // §20 Priority 3 lands registration ownership and `hook.remove`, + // this becomes an ordinary lowest-priority subscription carrying its + // owner and this slot is deleted rather than extended. + // + // Readable as `pmacs.path.directory_handler` so a replacement can + // capture and chain to the previous one; `nil` disables directory + // opening entirely, which is what makes that path testable. + path.set("directory_handler", mlua::Value::Nil)?; + path.set( + "set_directory_handler", + lua.create_function(|lua, handler: mlua::Value| { + match &handler { + mlua::Value::Nil | mlua::Value::Function(_) => {} + other => { + return Err(mlua::Error::runtime(format!( + "pmacs.path.set_directory_handler: expected a function or nil, got {}", + other.type_name() + ))); + } + } + let pmacs: Table = lua.globals().get("pmacs")?; + let path: Table = pmacs.get("path")?; + path.set("directory_handler", handler)?; + Ok(()) + })?, + )?; Ok(path) } +/// Lua handle for a captured directory destination (Q#JR14d). +/// +/// Deliberately **nonconstructible from Lua** and read-only. The same +/// value is passed to every `path.open-directory` listener in turn: as a +/// table, an earlier listener could mutate it and then decline, +/// redirecting later listeners or the fallback to a window the user +/// never asked for — and any Lua could fabricate a plausible +/// frontend/window/buffer triple and hand it to `commit_to`. Userdata +/// with no constructor and no setters makes both unrepresentable rather +/// than merely discouraged. +/// +/// The single accessor exists because dired needs the exact window for +/// its `display{window = …}` target; nothing needs the frontend or the +/// captured buffer, which stay private to the preflight. +pub(crate) struct DirectoryDestinationLua(pub(crate) crate::editor_core::DirectoryDestination); + +impl mlua::UserData for DirectoryDestinationLua { + fn add_methods>(methods: &mut M) { + methods.add_method("window", |_, this, ()| Ok(this.0.window.raw())); + } +} + /// Build the `pmacs.ansi.*` table. The only entry today is /// `parser()`; future additions (e.g. an event-table-validator /// helper) live alongside it. @@ -6938,6 +7003,26 @@ pub fn install_async( )?; } + // Journey Stage 1a (Q#JR14b): `pmacs.window.commit_to` runs its + // callback inside a Rust-stack RAII scope. Yielding out of that + // scope would let the guard's dynamic extent and the coroutine's + // suspension diverge — the guard would restore the frontend override + // while the continuation is still parked, so the rest of the commit + // would silently run ambient again, which is the exact bug the scope + // exists to prevent. `Handle:await` therefore refuses inside it. + // + // Enforced here rather than documented in the framing, because a + // rule that only exists in prose is one a future caller breaks + // without noticing. + async_mod.set( + "_in_commit_scope", + lua.create_function(|lua, ()| { + Ok(lua + .app_data_ref::() + .is_some_and(|scope| scope.active())) + })?, + )?; + { let rt = runtime.clone(); async_mod.set( diff --git a/src/lua_bindings/window_panel.rs b/src/lua_bindings/window_panel.rs index f4833ef..1c700a5 100644 --- a/src/lua_bindings/window_panel.rs +++ b/src/lua_bindings/window_panel.rs @@ -44,8 +44,22 @@ use crate::window::{DEFAULT_PANEL_ROWS, MIN_WINDOW_OUTER_ROWS, Side, WindowId}; /// call falls back to the ambient active frontend, exactly as the /// terminal surface does. pub(crate) fn acting_frontend(lua: &Lua, core: &SharedCore) -> FrontendId { - lua.app_data_ref::() - .and_then(|origin| origin.current()) + // Journey Stage 1a (Q#JR14e): the background scope wins. + // + // Order is deliberate — scoped override, then interactive origin, + // then ambient. A `commit_to` callback runs for the frontend that + // *requested* the work, and it must win over whatever happens to be + // dispatching when the worker settles. It is a separate slot rather + // than a reuse of the interactive origin because that origin is + // authenticated user-command authority (the pre-edit unfold guard, + // command-boundary rotation, and the terminal surface all key off + // it), and a background continuation must not acquire it. + lua.app_data_ref::() + .and_then(|scope| scope.current()) + .or_else(|| { + lua.app_data_ref::() + .and_then(|origin| origin.current()) + }) .unwrap_or_else(|| core.borrow().active_frontend_key()) } @@ -350,6 +364,123 @@ pub(crate) fn finish_adopter_placement( a coherent surface" )] pub(crate) fn install(lua: &Lua, core: &SharedCore, win: &Table) -> mlua::Result<()> { + { + let cc = core.clone(); + win.set( + "commit_to", + lua.create_function( + move |lua, + (dest, body): (mlua::Value, mlua::Function)| + -> mlua::Result { + // Journey Stage 1a (Q#JR14). Preflight FIRST, then + // scope, then run. The ordering is the whole point: + // an async handler mutates real state (dired claims + // a buffer, registers a handle, captures `prev`, and + // paints) long before it reaches any call that could + // refuse. Validating at display time is four + // mutations too late and leaves a hidden buffer + // behind, so every destination precondition is + // checked before the callback is invoked at all. + // + // Typed as `Value` rather than `AnyUserData` so this + // message is REACHABLE: with the narrower type mlua + // rejects a table during argument conversion, and a + // caller who fabricated one got "error converting Lua + // table to userdata" — true, but it names neither the + // rule nor how to get a real destination. + let dest = match &dest { + mlua::Value::UserData(userdata) => { + userdata.borrow::().ok() + } + _ => None, + }; + let dest = dest + .ok_or_else(|| { + mlua::Error::runtime( + "pmacs.window.commit_to: expected a destination captured by \ + the editor (it cannot be constructed from Lua)", + ) + })? + .0; + + // 1. The requesting frontend still has a layout. + let refusal = { + let core = cc.borrow(); + if !core.views.contains_key(&dest.frontend) { + Some("requesting frontend is gone".to_string()) + } else if !core + .views + .get(&dest.frontend) + .is_some_and(|view| view.layout.iter_ids().contains(&dest.window)) + { + // 2. The destination window is still live in it. + Some(format!("window {} is gone", dest.window.raw())) + } else if core + .windows + .get(&dest.window) + .is_some_and(|w| w.buffer_id != dest.buffer) + { + // 3. Stale intent (Q#JR14c): the user + // replaced the buffer while the work was + // in flight. Their action is newer + // information than the request, so the + // request loses. + Some(format!( + "window {} now shows another buffer", + dest.window.raw() + )) + } else if !core.window_accepts_buffer(dest.window, None) { + // 4. Replaceability (Q#JR14f). `None` + // because the replacement does not exist + // yet — passing the captured buffer would + // approve a window dedicated to *it*, and + // the handler's different buffer would be + // refused later, after mutating. + Some(format!("window {} is dedicated", dest.window.raw())) + } else { + None + } + }; + if let Some(reason) = refusal { + let mut out = mlua::MultiValue::new(); + out.push_back(mlua::Value::String(lua.create_string(reason.as_bytes())?)); + out.push_front(mlua::Value::Boolean(false)); + return Ok(out); + } + + let scope = lua + .app_data_ref::() + .ok_or_else(|| { + mlua::Error::runtime( + "pmacs.window.commit_to: no frontend scope installed", + ) + })? + .clone(); + let commit = lua + .app_data_ref::() + .ok_or_else(|| { + mlua::Error::runtime( + "pmacs.window.commit_to: no commit scope installed", + ) + })? + .clone(); + // Both the override and the core's ambient + // `active_frontend` are restored when this guard + // drops -- on the normal return AND on a raising + // callback, which is why the result is captured + // rather than `?`-propagated through the drop. + let result = { + let _guard = scope.enter(&cc, &commit, dest.frontend); + body.call::(()) + }; + let mut out = result?; + out.push_front(mlua::Value::Boolean(true)); + Ok(out) + }, + )?, + )?; + } + { let cc = core.clone(); win.set( @@ -397,10 +528,33 @@ pub(crate) fn install(lua: &Lua, core: &SharedCore, win: &Table) -> mlua::Result .probe_display_target(fid, existing, explicit_window) .map_err(mlua::Error::runtime)?; // 3. Load, dedup, or create the path-backed buffer. - let (buffer_id, fire) = cc + // + // Journey Stage 1a (Q#JR13): a DIRECTORY raises here + // and does NOT enter the directory resolver chain. + // `display_file` is "put this file in a window", not + // a CLI router — and `find-file`'s accept arm + // (`builtin/commands/default.lua`) wraps this call in + // a `pcall` whose comment guarantees that "only a + // real failure (a directory, a permission error) + // reaches here", pinned by + // `find_file_accepting_a_directory_reports_instead_of_raising`. + // Routing it into dired would silently change what + // `C-x C-f` on a directory does. Opening dired from + // find-file is a named deferral, not a side effect of + // the CLI work. + let (buffer_id, fire) = match cc .borrow_mut() .resolve_target_buffer(&path_buf) - .map_err(mlua::Error::runtime)?; + .map_err(mlua::Error::runtime)? + { + crate::editor_core::ResolvedTarget::Buffer { id, fire } => (id, fire), + crate::editor_core::ResolvedTarget::Directory { path } => { + return Err(mlua::Error::runtime(format!( + "pmacs.window.display_file: {} is a directory", + path.display() + ))); + } + }; // 4. Enter Q#BP4's transaction, so any hook observes // the DOCUMENT TARGET as active. let mut request = DisplayRequest::new(buffer_id); diff --git a/tests/journey_acceptance.rs b/tests/journey_acceptance.rs new file mode 100644 index 0000000..7ab36c0 --- /dev/null +++ b/tests/journey_acceptance.rs @@ -0,0 +1,1299 @@ +// tests/journey_acceptance.rs --- the golden product journey. + +//! The first cross-subsystem acceptance suite (`COHERENCE.md` §19, +//! `docs/journey-stage1a-framing.md` §5). +//! +//! Every other suite in the tree pins one subsystem's contract. This one +//! pins that the subsystems form a usable whole, walking `COHERENCE.md` +//! §2's twelve-step journey. Stage 1a seeds it with the steps that are +//! real today — 2 (launch unconfigured), 3 (open a real project), and 5 +//! (edit immediately). Steps 6–12 join as later stages make them real. +//! +//! **This file is a ratchet: stages add rows, none removes them.** +//! +//! Two disciplines it must keep: +//! +//! * **Drive the real entry point.** A directory arm with no production +//! caller passes every direct-call test, so step 3 goes through +//! `EditorState::open` — the same function `pmacs FILE` calls — and +//! not through `resolve_target_buffer`. +//! * **Pump to quiescence, never to a frame count.** Every listing is +//! worker-dispatched; `tick_async` resuming a coroutine in the frame +//! its result arrives does not bound when the worker finishes. +//! +//! Pins are labelled **N** (new behavior — must fail on full revert) or +//! **P** (preservation — legitimately green on the pre-image, falsified +//! by the named targeted mutation). See framing §6.0 for why the +//! distinction is load-bearing: an equivalence assertion between two +//! implementations that already agree proves nothing about structural +//! reuse. +//! +//! Two P pins here — P1 and P2 — *also* fail on full revert, since +//! `commit_to` does not exist on the pre-image. They are labelled P +//! because their discriminating falsifier is the named mutation: a +//! revert-only check cannot distinguish "validates" from "validates in +//! time", which is their entire claim. Each says so at its own site. + +use std::path::Path; +use std::time::{Duration, Instant}; + +use crossterm::event::{KeyCode, KeyEvent, KeyEventKind, KeyEventState, KeyModifiers}; +use pmacs::buffer::BufferId; +use pmacs::editor::EditorState; +use pmacs::editor_core::normalize_buffer_path; +use pmacs::protocol::FrontendId; +use pmacs::window::{FrontendView, Layout, Window, WindowId}; +use tempfile::TempDir; + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +fn exec(s: &EditorState, src: &str) { + s.lua_host.lua().load(src.to_string()).exec().unwrap(); +} + +fn eval(s: &EditorState, src: &str) -> T { + s.lua_host.lua().load(src.to_string()).eval().unwrap() +} + +/// Drive the async runtime to quiescence — no parked coroutine, no +/// pending worker job. The directory listing is invisible until this +/// returns, and how many frames it takes is not knowable in advance. +fn pump(s: &mut EditorState) { + let deadline = Instant::now() + Duration::from_secs(10); + loop { + let idle: bool = eval( + s, + "return pmacs._async.parked_count() == 0 and pmacs._async.pending_count() == 0", + ); + if idle { + return; + } + assert!(Instant::now() < deadline, "async pump deadline exceeded"); + s.tick_async(); + } +} + +fn key(code: KeyCode, mods: KeyModifiers) -> KeyEvent { + KeyEvent { + code, + modifiers: mods, + kind: KeyEventKind::Press, + state: KeyEventState::NONE, + } +} + +fn press(s: &mut EditorState, code: KeyCode) { + s.dispatch_key(FrontendId::LOCAL, key(code, KeyModifiers::NONE)); +} + +fn type_char(s: &mut EditorState, c: char) { + s.dispatch_key(FrontendId::LOCAL, key(KeyCode::Char(c), KeyModifiers::NONE)); +} + +/// The 0-based line an entry renders on, found by its trailing name +/// column -- the same shape `dired_acceptance` uses. +fn line_of(s: &EditorState, name: &str) -> usize { + let text = active_text(s); + for (index, line) in text.lines().enumerate() { + if line.trim_end().ends_with(name) { + return index; + } + } + panic!("no listing line for {name:?} in:\n{text}"); +} + +/// A project a journey can plausibly be run against. +fn project() -> TempDir { + let td = tempfile::tempdir().expect("tempdir"); + std::fs::write(td.path().join("alpha.txt"), b"alpha\n").expect("write alpha"); + std::fs::write(td.path().join("beta.txt"), b"beta\n").expect("write beta"); + td +} + +fn canon(path: &Path) -> String { + normalize_buffer_path(path.to_path_buf()) + .to_string_lossy() + .into_owned() +} + +fn active_name(s: &EditorState) -> String { + eval(s, "return pmacs.window.buffer():name()") +} + +fn active_text(s: &EditorState) -> String { + eval( + s, + "local b = pmacs.window.buffer()\nreturn b:slice(0, b:len())", + ) +} + +fn status(s: &EditorState) -> String { + s.core.borrow().status.clone() +} + +fn buffer_count(s: &EditorState) -> usize { + s.core.borrow().registry.borrow().ids().len() +} + +/// The buffer a window currently shows, or `None` if it is not live. +fn buffer_in(s: &EditorState, window: WindowId) -> Option { + s.core.borrow().windows.get(&window).map(|w| w.buffer_id) +} + +/// The window `LOCAL` currently has selected. +fn local_window(s: &EditorState) -> WindowId { + s.core + .borrow() + .views + .get(&FrontendId::LOCAL) + .expect("LOCAL view") + .active +} + +/// Register a second frontend with its own single-window layout, +/// mirroring `build_fresh_frontend_view` (the same helper shape +/// `bottom_panel_stage1_acceptance` uses). +fn attach_frontend(s: &EditorState, fid: FrontendId) -> WindowId { + let mut core = s.core.borrow_mut(); + let buffer_id = core.active_buffer_id(); + let text_view = { + let reg = core.registry.borrow(); + pmacs::text_view::TextView::new(reg.get(buffer_id).expect("buffer")) + }; + let win = WindowId::next(); + core.windows + .insert(win, Window::new(win, buffer_id, text_view)); + core.register_frontend_view( + fid, + FrontendView { + layout: Layout::single(win), + active: win, + fold_projection: true, + panel_capable: true, + frame_geometry: None, + panel_hidden: false, + }, + ); + win +} + +/// Drive the **real** chain far enough to obtain a genuine destination +/// and leave it in the Lua global `dest`. +/// +/// The listener claims (returns `false`), so nothing is committed and no +/// fallback runs: what lands in `dest` is exactly the userdata dired +/// would have received, produced by the production capture rather than +/// fabricated. Nothing in the test suite can construct one — that is +/// N6b's whole subject. +fn capture_dest(s: &mut EditorState, dir: &Path) { + exec( + s, + "dest = nil + pmacs.hook.add('path.open-directory', function(_, d) dest = d return false end)", + ); + s.open_directory_target(dir); + pump(s); + assert!( + eval::(s, "return dest ~= nil"), + "the chain must hand listeners a destination" + ); +} + +/// Open through the **real** startup entry point, as `pmacs PATH` does. +fn launch(path: &Path) -> EditorState { + let mut s = EditorState::open(path.to_path_buf()).expect("startup must not fail"); + exec(&s, "pmacs.lsp.config = {}"); + pump(&mut s); + s +} + +// --------------------------------------------------------------------------- +// Step 2 — launch unconfigured +// --------------------------------------------------------------------------- + +/// **N** — the editor starts with no configuration and no arguments. +#[test] +fn journey_step2_launches_unconfigured_into_scratch() { + let s = EditorState::new(); + assert_eq!(active_name(&s), "*scratch*"); + assert!( + status(&s).is_empty(), + "a clean launch reports no error; got {:?}", + status(&s) + ); +} + +// --------------------------------------------------------------------------- +// Step 3 — open a real project +// --------------------------------------------------------------------------- + +/// **N1** — `pmacs .` opens the directory. +/// +/// The headline of Stage 1a and of `COHERENCE.md` §2's "broken at step +/// 3" grade. Before the directory arm this construction returned +/// `Err(EISDIR)` and `main` exited 1. +#[test] +fn journey_step3_opening_a_directory_lists_it() { + let td = project(); + let s = launch(td.path()); + + let name = active_name(&s); + assert_eq!( + name, + format!("*dired:{}*", canon(td.path())), + "the active buffer must be the directory's dired buffer" + ); + let text = active_text(&s); + assert!( + text.contains("alpha.txt") && text.contains("beta.txt"), + "the listing must show the directory's entries; got {text:?}" + ); +} + +/// **N1b** — and it is a *successful* startup, not a rescued failure. +/// +/// Guards the specific regression shape: an implementation that opened +/// dired but still left an error on the status line would look right in +/// the assertion above while `pmacs .` still printed a diagnostic. +#[test] +fn journey_step3_directory_startup_reports_no_error() { + let td = project(); + let s = launch(td.path()); + assert!( + !status(&s).contains("cannot open"), + "a successful directory open must not leave an error status; got {:?}", + status(&s) + ); +} + +/// **N3** — an unreadable directory reports and leaves the session +/// running, rather than failing startup. +#[cfg(target_os = "linux")] +#[test] +fn journey_step3_unreadable_directory_reports_without_failing_startup() { + use std::os::unix::fs::PermissionsExt; + let td = tempfile::tempdir().expect("tempdir"); + let locked = td.path().join("locked"); + std::fs::create_dir(&locked).expect("mkdir"); + std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o000)).expect("chmod"); + + // Startup itself must succeed: the failure is the *listing*, which + // happens a tick later and belongs on the status line. + let s = launch(&locked); + assert!( + !status(&s).is_empty(), + "a failed listing must report through the status line" + ); + assert!( + !active_name(&s).starts_with("*dired:"), + "a failed listing must leave no dired buffer behind" + ); + + std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o700)).expect("restore"); +} + +/// **N9** — the resolver receives a canonical absolute path. +/// +/// Falsified by dropping the normalization in +/// `ResolvedTarget::Directory`: nothing else normalizes on that arm, +/// because no buffer is created and `set_buffer_path` never runs. +#[test] +fn journey_directory_resolver_receives_a_canonical_path() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + exec( + &s, + "seen = nil + pmacs.hook.add('path.open-directory', function(path) seen = path return false end)", + ); + + // A path with a redundant component, which only canonicalization removes. + let noisy = td.path().join("subdir").join(".."); + std::fs::create_dir_all(td.path().join("subdir")).expect("mkdir"); + s.open_directory_target(&noisy); + pump(&mut s); + + let seen: String = eval(&s, "return seen"); + assert_eq!( + seen, + canon(td.path()), + "the resolver must receive the canonical path, not the literal argument" + ); +} + +/// **N10** — with the handler cleared and nothing claiming, a directory +/// argument still starts successfully. +/// +/// The regression path back to exit 1. Reachable only because the +/// fallback is a clearable slot rather than a builtin hook subscription. +#[test] +fn journey_unclaimed_directory_starts_successfully_with_a_status() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + exec(&s, "pmacs.path.set_directory_handler(nil)"); + + let before = active_name(&s); + s.open_directory_target(td.path()); + pump(&mut s); + + assert_eq!( + active_name(&s), + before, + "with no handler the window keeps the buffer it had" + ); + assert!( + status(&s).contains(&canon(td.path())), + "the status must name the directory nothing surfaced; got {:?}", + status(&s) + ); +} + +// --------------------------------------------------------------------------- +// The resolver chain +// --------------------------------------------------------------------------- + +/// **N7** — first claimant wins, through an ordinary user listener, and +/// a claim suppresses the fallback. +#[test] +fn journey_resolver_chain_is_first_claimant_wins() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + exec( + &s, + "first, second, fallback_ran = false, false, false + pmacs.path.set_directory_handler(function() fallback_ran = true end) + pmacs.hook.add('path.open-directory', function() first = true return false end) + pmacs.hook.add('path.open-directory', function() second = true return false end)", + ); + + s.open_directory_target(td.path()); + pump(&mut s); + + assert!(eval::(&s, "return first"), "the first listener runs"); + assert!( + !eval::(&s, "return second"), + "a claim stops the fan-out before the second listener" + ); + assert!( + !eval::(&s, "return fallback_ran"), + "a claim suppresses the fallback" + ); +} + +/// **N8** — a raising listener suppresses the fallback *and* is +/// reported. +/// +/// Falsified by running the fallback when `errors` is non-empty (i.e. +/// treating a raise as a decline), or by making a raise yield +/// `proceed = true`. NOT falsified by keying suppression on `proceed` +/// alone — that is already correct, since a raise and a claim both give +/// `proceed == false`. +#[test] +fn journey_a_raising_resolver_suppresses_the_fallback_and_reports() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + exec( + &s, + "fallback_ran = false + pmacs.path.set_directory_handler(function() fallback_ran = true end) + pmacs.hook.add('path.open-directory', function() error('resolver exploded') end)", + ); + + s.open_directory_target(td.path()); + pump(&mut s); + + assert!( + !eval::(&s, "return fallback_ran"), + "a crashed resolver must not fall through to the default surface" + ); + assert!( + !status(&s).is_empty(), + "the failure must reach the status line, not only *errors*" + ); + let errors: String = eval( + &s, + "for _, id in ipairs(pmacs.buffer.list()) do + local ok, d = pcall(pmacs.describe.buffer, id) + if ok and d and d.name == '*errors*' then + return id:slice(0, id:len()) + end + end + return ''", + ); + assert!( + errors.contains("resolver exploded"), + "the failure must also reach the *errors* buffer; got {errors:?}" + ); +} + +// --------------------------------------------------------------------------- +// The destination commit (`pmacs.window.commit_to`) +// --------------------------------------------------------------------------- +// +// The substrate half of Stage 1a. A directory listing settles a tick or +// more after the request, by which time the ambient frontend, selected +// window, and active buffer may all name something else — so the whole +// post-await commit runs against a destination captured at request time. +// +// `LOCAL` is the requesting frontend throughout, because +// `open_directory_target` is the local-startup seam; the daemon's +// non-`LOCAL` capture is pinned in `src/daemon.rs`, where the production +// caller lives. What varies here is what the *ambient* frontend is doing +// while the commit runs, which is exactly the misrouting the scope +// exists to prevent. + +/// The frontend that competes for ambient authority in these tests. +const COMPETITOR: FrontendId = FrontendId(7); + +/// **N4** — the commit lands in the *requesting* frontend's window even +/// though another frontend is the one dispatching. +/// +/// The blocker's positive half. Falsified by reverting `commit_to` to an +/// ambient display: the file then appears in the competitor's window. +#[test] +fn commit_to_delivers_to_the_requesting_frontend_not_the_ambient_one() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + capture_dest(&mut s, td.path()); + + let local_win = local_window(&s); + let other_win = attach_frontend(&s, COMPETITOR); + let other_before = buffer_in(&s, other_win); + + // The competitor becomes the dispatching frontend while the work is + // "in flight" — the state a worker completion actually returns to. + s.core.borrow_mut().active_frontend = COMPETITOR; + + let alpha = td.path().join("alpha.txt").display().to_string(); + exec( + &s, + &format!( + "assert(pmacs.window.commit_to(dest, function() + pmacs.window.display_file({alpha:?}) + end))" + ), + ); + + assert_eq!( + buffer_in(&s, other_win), + other_before, + "the competing frontend's window must be untouched" + ); + s.core.borrow_mut().active_frontend = FrontendId::LOCAL; + assert_eq!( + active_name(&s), + alpha, + "the commit must land in the requesting frontend's captured window" + ); + assert_eq!( + local_window(&s), + local_win, + "and in that window, not a new one" + ); +} + +/// **N4b** — the scope beats an *interactive origin*, not merely the +/// ambient frontend. +/// +/// Found by bite-testing N4: with the `ScopedFrontend` arm deleted from +/// `acting_frontend`, N4 still passed, because `ScopedFrontend::enter` +/// also swaps `core.active_frontend` and the ambient fallback then +/// answers correctly on its own. The arm is load-bearing in exactly one +/// situation — a commit reached from inside an interactive command, +/// where the origin sits *between* the override and the ambient value +/// and would otherwise win. `acting_frontend`'s comment claims that +/// ordering; nothing pinned it. +/// +/// Driven through `dispatch_key`, because the interactive origin is +/// established by dispatch and by nothing else — `invoke_interactive` +/// requires a context rather than creating one. +/// +/// Falsified by deleting the `ScopedFrontend` arm from +/// `acting_frontend`, or by reordering it after the interactive origin. +#[test] +fn commit_to_outranks_an_interactive_origin() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + capture_dest(&mut s, td.path()); + + let local_win = local_window(&s); + let other_win = attach_frontend(&s, COMPETITOR); + let other_before = buffer_in(&s, other_win); + + let alpha = td.path().join("alpha.txt").display().to_string(); + exec( + &s, + &format!( + "pmacs.command.define {{ + name = 'test.journey-commit', + description = 'commit to a captured destination from inside a command', + fn = function() + committed = pmacs.window.commit_to(dest, function() + pmacs.window.display_file({alpha:?}) + end) + end, + }} + pmacs.keymap.bind {{ scope = 'global', sequence = 'C-c j', + command = 'test.journey-commit' }}" + ), + ); + + // The COMPETITOR runs the command, so ITS id is the interactive + // origin for the whole invocation. + s.dispatch_key(COMPETITOR, key(KeyCode::Char('c'), KeyModifiers::CONTROL)); + s.dispatch_key(COMPETITOR, key(KeyCode::Char('j'), KeyModifiers::NONE)); + + assert!( + eval::(&s, "return committed"), + "the commit must be accepted" + ); + assert_eq!( + buffer_in(&s, other_win), + other_before, + "the invoking frontend's own window must be untouched" + ); + s.core.borrow_mut().active_frontend = FrontendId::LOCAL; + assert_eq!( + active_name(&s), + alpha, + "the commit must land in the captured destination, not the \ + interactive origin's window" + ); + assert_eq!(local_window(&s), local_win); +} + +/// **N4c** — the commit lands in the *captured window*, not merely in +/// the captured frontend's currently selected one. +/// +/// Review finding on PR #182. Every other routing pin here varies +/// frontend identity; none varied the selected window *within* one +/// frontend, and dired's commit still ended in `switch_buffer`, which +/// targets whatever window the scoped frontend has active. The preflight +/// cannot catch this — the captured window is still live and still holds +/// its captured buffer — so a split that took focus while `read_dir` was +/// pending got the listing, and `prev` was captured from it too. +/// +/// Both halves are asserted: where the listing lands, and where `q` +/// goes. Falsified by restoring `pmacs.window.switch_buffer` in dired's +/// `display`, or by reading `prev` from the ambient window. +#[test] +fn a_background_open_uses_the_captured_window_not_the_selected_one() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + capture_dest(&mut s, td.path()); + + let target = local_window(&s); + let origin = buffer_in(&s, target).expect("the captured window's buffer"); + + // Split, move focus to the OTHER window, and give it a buffer of its + // own. The captured window is untouched, so every preflight check + // still passes -- which is exactly why this needs its own pin. + exec( + &s, + "local captured = dest:window() + pmacs.window.split_horizontal() + while pmacs.window.current() == captured do pmacs.window.focus_next() end + pmacs.window.switch_buffer(pmacs.buffer.create('*elsewhere*'))", + ); + let elsewhere = local_window(&s); + assert_ne!(elsewhere, target, "focus must have moved to another window"); + let elsewhere_buffer = buffer_in(&s, elsewhere); + + // dired's real handler path, with the captured destination. + exec( + &s, + &format!( + "pmacs.async(function() + pmacs.dired.open({:?}, {{ dest = dest }}) + end)", + canon(td.path()) + ), + ); + pump(&mut s); + + assert_eq!( + buffer_in(&s, elsewhere), + elsewhere_buffer, + "the window that took focus mid-listing must be untouched" + ); + assert_eq!( + local_window(&s), + target, + "the commit must select the captured window" + ); + assert!( + active_name(&s).starts_with("*dired:"), + "and the listing must be in it; got {:?}", + active_name(&s) + ); + + // `prev` came from the captured window too, not from `*elsewhere*`. + type_char(&mut s, 'q'); + assert_eq!( + buffer_in(&s, target), + Some(origin), + "`q` must return to the buffer the CAPTURED window showed" + ); +} + +/// **N6a** — the scope is restored when the callback returns normally. +/// +/// Falsified by dropping the guard's restore, or by never swapping +/// `core.active_frontend` in the first place (then `inside` reads the +/// competitor and the assertion fails from the other direction). +#[test] +fn commit_to_scopes_and_restores_on_a_normal_return() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + capture_dest(&mut s, td.path()); + attach_frontend(&s, COMPETITOR); + s.core.borrow_mut().active_frontend = COMPETITOR; + + exec( + &s, + "inside, scoped = nil, nil + assert(pmacs.window.commit_to(dest, function() + inside = pmacs.frontend.id() + scoped = pmacs._async._in_commit_scope() + end))", + ); + + assert_eq!( + eval::(&s, "return inside"), + i64::try_from(FrontendId::LOCAL.0).expect("frontend id"), + "inside the commit the acting frontend is the requesting one" + ); + assert!( + eval::(&s, "return scoped"), + "and the commit-scope flag is set while the callback runs" + ); + assert_eq!( + s.core.borrow().active_frontend, + COMPETITOR, + "the ambient frontend must be restored on return" + ); + assert!( + !eval::(&s, "return pmacs._async._in_commit_scope()"), + "and the commit-scope flag cleared" + ); + assert_eq!( + eval::(&s, "return pmacs.frontend.id()"), + i64::try_from(COMPETITOR.0).expect("frontend id"), + "the Lua-visible frontend must be restored too" + ); +} + +/// **N6b (part of N6)** — a raising callback still restores. +/// +/// The path that makes the guard RAII rather than a pair of statements: +/// `commit_to` captures the call's result and lets the guard drop before +/// propagating it. Falsified by `?`-propagating the callback's error +/// through the scope, or by restoring on the success path only. +#[test] +fn commit_to_restores_when_the_callback_raises() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + capture_dest(&mut s, td.path()); + attach_frontend(&s, COMPETITOR); + s.core.borrow_mut().active_frontend = COMPETITOR; + + exec( + &s, + "local ok, err = pcall(pmacs.window.commit_to, dest, function() + error('commit exploded') + end) + raised = (not ok) and tostring(err) or ''", + ); + + assert!( + eval::(&s, "return raised").contains("commit exploded"), + "the callback's error must propagate" + ); + assert_eq!( + s.core.borrow().active_frontend, + COMPETITOR, + "a raising callback must still restore the ambient frontend" + ); + assert!( + !eval::(&s, "return pmacs._async._in_commit_scope()"), + "and must still clear the commit-scope flag" + ); +} + +/// **N6c (part of N6)** — awaiting inside a commit is refused, the +/// refusal names the rule, and the scope is restored anyway. +/// +/// A yield would restore the scope while the coroutine is still parked, +/// so the rest of the commit would resume ambient — silently +/// reintroducing exactly the misrouting N4 pins against. Driven inside +/// `pmacs.async`, which is where a real await lives. +/// +/// Falsified by dropping the `_in_commit_scope` check from +/// `Handle:await`: the await then succeeds and `refusal` reads +/// ``. +#[test] +fn commit_to_refuses_an_await_and_restores() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + capture_dest(&mut s, td.path()); + attach_frontend(&s, COMPETITOR); + s.core.borrow_mut().active_frontend = COMPETITOR; + + exec( + &s, + &format!( + "refusal = nil + pmacs.async(function() + local handle = pmacs.fs.read_dir({:?}) + local ok, err = pcall(pmacs.window.commit_to, dest, function() + return handle:await() + end) + refusal = (not ok) and tostring(err) or '' + -- Drain it OUTSIDE the commit, which is where the refusal + -- says the await belongs -- and which also settles the job + -- so the pump can reach quiescence. + handle:await() + end)", + td.path().display().to_string() + ), + ); + pump(&mut s); + + let refusal: String = eval(&s, "return refusal"); + assert!( + refusal.contains("cannot await inside") && refusal.contains("commit_to"), + "the refusal must name the rule it enforces; got {refusal:?}" + ); + assert_eq!( + s.core.borrow().active_frontend, + COMPETITOR, + "a refused await must still restore the ambient frontend" + ); + assert!( + !eval::(&s, "return pmacs._async._in_commit_scope()"), + "and must still clear the commit-scope flag" + ); +} + +/// **N6b** — a forged destination is rejected, and the callback never +/// runs. +/// +/// A plausible `{frontend, window, buffer}` table is what any Lua could +/// fabricate. Falsified by accepting a table, or by borrowing the +/// userdata after invoking the callback. +#[test] +fn commit_to_refuses_a_forged_destination() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + capture_dest(&mut s, td.path()); + + let win = eval::(&s, "return dest:window()"); + exec( + &s, + &format!( + "ran = false + local ok, err = pcall(pmacs.window.commit_to, + {{ frontend = 0, window = {win}, buffer = 0 }}, + function() ran = true end) + rejected = (not ok) and tostring(err) or ''" + ), + ); + + let rejected: String = eval(&s, "return rejected"); + assert!( + rejected.contains("cannot be constructed from Lua"), + "a forged table must be rejected by type, not merely fail later; got {rejected:?}" + ); + assert!( + !eval::(&s, "return ran"), + "a rejected destination must not reach the callback" + ); +} + +/// **N6c** — a declining listener cannot redirect the destination. +/// +/// The same userdata is handed to every listener in turn. As a table, an +/// earlier listener could rewrite the window and then decline, sending +/// the fallback somewhere the user never asked for. Falsified by passing +/// a shared mutable table. +#[test] +fn a_declining_listener_cannot_redirect_the_destination() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + let target = local_window(&s); + + exec( + &s, + "seen_first, seen_second, mutation = nil, nil, nil + pmacs.hook.add('path.open-directory', function(_, d) + seen_first = d:window() + -- Try to redirect, then decline. Both halves matter: a + -- successful mutation with a decline is the attack. + local ok, err = pcall(function() d.window = 999 end) + mutation = (not ok) and tostring(err) or '' + end) + pmacs.hook.add('path.open-directory', function(_, d) + seen_second = d:window() + end)", + ); + + s.open_directory_target(td.path()); + pump(&mut s); + + let mutation: String = eval(&s, "return mutation"); + assert!( + !mutation.contains(""), + "the destination must be read-only; got {mutation:?}" + ); + let first = eval::(&s, "return seen_first"); + let second = eval::(&s, "return seen_second"); + assert_eq!( + first, second, + "every listener must see the same, unaltered destination" + ); + assert_eq!( + u64::try_from(second).expect("window id"), + target.raw(), + "and it must still name the window the editor captured" + ); + // And the fallback commits THERE, not to whatever the first listener + // wanted -- the observable the attack was aiming at. + assert!( + active_name(&s).starts_with("*dired:"), + "the declined chain must still fall back to dired" + ); + assert_eq!( + buffer_in(&s, target), + Some(eval::(&s, "return pmacs.window.buffer()").0), + "in the captured window" + ); +} + +// --- the commit's preservation pins --------------------------------------- + +/// **P1** — every destination precondition is checked *before* the +/// callback runs, so a failure mutates nothing. +/// +/// Four refusals, each asserted the same way: `commit_to` returns +/// `(false, reason)`, the callback never ran, and no buffer was created. +/// Table-driven deliberately — the failure message names which +/// precondition regressed, which four separate near-identical tests +/// would give up in exchange for nothing. +/// +/// *Mutation:* move the preflight from before the callback to after it +/// (rev 2's design, which validated at display time). All four fail. +/// *Second mutation, for the dedicated case:* pass `Some(dest.buffer)` +/// instead of `None` to `window_accepts_buffer`. Only that case fails — +/// which is why it is listed separately from the stale-buffer case it +/// otherwise resembles. +/// +/// **Also fails on full revert**, since `commit_to` does not exist on the +/// pre-image. It is listed as a P because the discriminating falsifier is +/// the named mutation, not the revert: a revert-only check would not +/// distinguish "validates" from "validates in time". +#[test] +fn preservation_a_failed_precondition_never_reaches_the_callback() { + // (label, Lua that breaks the precondition, expected reason fragment) + let cases: [(&str, &str, &str); 4] = [ + ( + "frontend gone", + // Handled in Rust below: unregistering a view has no Lua surface. + "", + "requesting frontend is gone", + ), + ( + "window gone", + "local doomed = dest:window() + pmacs.window.split_horizontal() + while pmacs.window.current() == doomed do pmacs.window.focus_next() end + pmacs.window.close_others()", + "is gone", + ), + ( + "stale buffer", + "pmacs.window.switch_buffer(pmacs.buffer.create('*usurper*'))", + "now shows another buffer", + ), + ( + "dedicated", + "pmacs.window.set_params(dest:window(), { dedicated = true })", + "is dedicated", + ), + ]; + + for (label, break_it, expected) in cases { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + capture_dest(&mut s, td.path()); + + if label == "frontend gone" { + s.core + .borrow_mut() + .unregister_frontend_view(FrontendId::LOCAL); + } else { + exec(&s, break_it); + } + let before = buffer_count(&s); + + exec( + &s, + "ran = false + ok, reason = pmacs.window.commit_to(dest, function() ran = true end)", + ); + + assert!( + !eval::(&s, "return ok"), + "{label}: commit_to must refuse" + ); + let reason: String = eval(&s, "return tostring(reason)"); + assert!( + reason.contains(expected), + "{label}: reason must say why; wanted {expected:?}, got {reason:?}" + ); + assert!( + !eval::(&s, "return ran"), + "{label}: the callback must not run at all -- validating after it \ + is four mutations too late" + ); + assert_eq!( + buffer_count(&s), + before, + "{label}: a refused commit must create no buffer" + ); + } +} + +/// **P2 — stale intent loses**, through dired's real commit path. +/// +/// The user replaced the destination window's buffer while the listing +/// was in flight. Their action is newer information than the request, so +/// the request loses: dired refuses, their buffer survives, and no dired +/// buffer or handle is left behind for that path. +/// +/// P1 pins the preflight in isolation; this drives `pmacs.dired.open` +/// with a captured destination — the same call the handler makes — so +/// the atomicity claim is asserted where the four mutations actually +/// live. +/// +/// *Mutation:* drop the `dest.buffer` comparison from the preflight +/// (window-only validation). The dired buffer then replaces the user's. +#[test] +fn preservation_a_stale_destination_loses_to_the_users_newer_buffer() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + capture_dest(&mut s, td.path()); + let target = local_window(&s); + + // The user switches the destination window while the work is in flight. + exec( + &s, + "usurper = pmacs.buffer.create('*usurper*') + pmacs.window.switch_buffer(usurper)", + ); + let usurper = buffer_in(&s, target); + let before = buffer_count(&s); + + exec( + &s, + &format!( + "failure = nil + pmacs.async(function() + local ok, err = pcall(pmacs.dired.open, {:?}, {{ dest = dest }}) + failure = (not ok) and tostring(err) or '' + end)", + canon(td.path()) + ), + ); + pump(&mut s); + + let failure: String = eval(&s, "return failure"); + assert!( + failure.contains("destination is gone"), + "dired must report the refusal rather than commit; got {failure:?}" + ); + assert_eq!( + buffer_in(&s, target), + usurper, + "the user's newer buffer must survive" + ); + assert_eq!( + buffer_count(&s), + before, + "and no dired buffer may be left behind" + ); + assert_eq!( + active_name(&s), + "*usurper*", + "nor may the refusal change what is displayed" + ); +} + +/// **P3** — dired reads its `prev` inside the scope, so `q` returns to +/// the *destination* window's buffer, not the ambient frontend's. +/// +/// `handle.prev` is captured with `pmacs.window.buffer()`, whose no-arg +/// arm reads the core's ambient `active_buffer_id()`. That is precisely +/// why the scope swaps `core.active_frontend` and not only the override: +/// a scope that swapped the override alone would leave this one line +/// reading the competitor's buffer, and `q` would drop the user into a +/// buffer from another frontend's window. +/// +/// Asserted through `q` rather than by reaching into dired's handle +/// table — `prev`'s entire meaning is where `q` lands. +/// +/// *Mutation:* stop swapping `core.active_frontend` in +/// `ScopedFrontend::enter` (keep the override). `q` then lands in +/// `*competitor*`. +#[test] +fn preservation_dired_captures_prev_from_the_destination_not_the_ambient_frontend() { + let td = project(); + let mut s = EditorState::new(); + exec(&s, "pmacs.lsp.config = {}"); + + let target = local_window(&s); + let origin = buffer_in(&s, target).expect("the startup buffer"); + + // A competitor whose window shows a buffer of its own, ambient while + // the listing settles. + let other_win = attach_frontend(&s, COMPETITOR); + let competitor_buffer = + eval::(&s, "return pmacs.buffer.create('*competitor*')") + .0; + s.core + .borrow_mut() + .install_buffer_in_window(other_win, competitor_buffer) + .expect("install"); + s.core.borrow_mut().active_frontend = COMPETITOR; + + s.open_directory_target(td.path()); + pump(&mut s); + s.core.borrow_mut().active_frontend = FrontendId::LOCAL; + assert!( + active_name(&s).starts_with("*dired:"), + "the listing must have committed" + ); + + type_char(&mut s, 'q'); + assert_eq!( + buffer_in(&s, target), + Some(origin), + "`q` must return to the buffer the DESTINATION window showed, not \ + the ambient frontend's" + ); +} + +// --------------------------------------------------------------------------- +// Step 5 — edit immediately +// --------------------------------------------------------------------------- + +/// **N11** — the journey's step-3-into-step-5 path, through the real +/// input path at every step: start on a directory, press `RET` on a +/// listed file, then type a character into it. +/// +/// Rev 6 correction: this previously called `display_file` and +/// `buf:insert` directly, so it stayed green with dired's `RET` binding, +/// its entry dispatch, or the editor's self-insert path all broken — +/// which is most of what "the journey works" is supposed to mean. Both +/// gestures are now dispatched as keys. +/// +/// Deliberately not a self-insert into the dired buffer, whose intercept +/// rejects every edit: asserting an edit lands there would contradict +/// the read-only contract rather than pin the journey. +#[test] +fn journey_step5_editing_a_file_reached_through_the_directory() { + let td = project(); + let mut s = launch(td.path()); + assert!(active_name(&s).starts_with("*dired:")); + + // Seat on the entry, then VISIT it with the real key. + let line = line_of(&s, "alpha.txt"); + exec(&s, &format!("pmacs.editor.move_to_line({line})")); + press(&mut s, KeyCode::Enter); + pump(&mut s); + + assert_eq!( + active_name(&s), + td.path().join("alpha.txt").display().to_string(), + "RET on a listed file must visit it" + ); + + // And type into it with the real key. + type_char(&mut s, 'X'); + let text = active_text(&s); + assert!( + text.starts_with('X'), + "a self-insert must land in the visited file's buffer; got {text:?}" + ); + assert!( + buffer_count(&s) >= 2, + "the dired buffer and the visited file both exist" + ); +} + +// --------------------------------------------------------------------------- +// Preservation pins (P) — green on the pre-image; see the named mutation +// --------------------------------------------------------------------------- + +/// **P4** — startup shows the file in the *active* window. +/// +/// *Mutation:* replace `replace_active_buffer` with a bare +/// `install_buffer_in_window` into some other window in +/// `EditorState::open`. +/// +/// **Note, found during implementation:** this does NOT assert that the +/// initial scratch buffer is destroyed, because it is not. +/// `replace_active_buffer`'s doc comment claims it drops "any old +/// scratch buffer if the active window's previous buffer has no other +/// windows referencing it", but all it does is call +/// `switch_active_buffer`, which reassigns the window's `buffer_id` and +/// never removes anything. The stale scratch survives in the registry +/// today, on `main`, unrelated to this stage — so asserting otherwise +/// would have pinned a guarantee the editor does not make and failed on +/// the pre-image for the wrong reason. What the unification must +/// preserve is which window shows the file, and that is what this pins. +#[test] +fn preservation_opening_a_file_shows_it_in_the_active_window() { + let td = project(); + let target = td.path().join("alpha.txt"); + let s = EditorState::open(target.clone()).expect("open"); + + // The displayed name is the argument as given (`path.display()`), + // which both implementations have always produced -- the *stored* + // path is what gets normalized, inside `set_buffer_path`. + assert_eq!( + active_name(&s), + target.display().to_string(), + "the file must be in the active window, not merely loaded" + ); + let scratch_displayed: bool = eval( + &s, + "for _, id in ipairs(pmacs.buffer.list()) do + local ok, d = pcall(pmacs.describe.buffer, id) + if ok and d and d.name == '*scratch*' and pmacs.window.buffer() == id then + return true + end + end + return false", + ); + assert!( + !scratch_displayed, + "no window may still be showing the startup scratch buffer" + ); +} + +/// **P5** — the `NotFound` arm survives the unification. +/// +/// *Mutation:* delete the `NotFound` arm from `resolve_target_buffer`. +/// The arm most likely to be lost in a wholesale refactor, because its +/// failure mode is a hard error on a perfectly ordinary gesture. +#[test] +fn preservation_a_missing_path_becomes_a_new_file_buffer() { + let td = project(); + let fresh = td.path().join("not-yet.txt"); + let s = EditorState::open(fresh.clone()).expect("a missing path is not an error"); + + assert_eq!(status(&s), "[new file]"); + let len: usize = eval(&s, "return pmacs.window.buffer():len()"); + assert_eq!(len, 0, "a new-file buffer starts empty"); + assert!(!fresh.exists(), "nothing is written until save"); +} + +/// **P8** — a startup failure names the file. +/// +/// The message gained a `cannot open {path}: ` prefix in Stage 1a; the +/// *failure* is preserved, only its wording improved. Before, the bare +/// `io::Error` never named the path. +#[cfg(target_os = "linux")] +#[test] +fn preservation_an_unreadable_file_reports_with_its_path() { + use std::os::unix::fs::PermissionsExt; + let td = project(); + let locked = td.path().join("locked.txt"); + std::fs::write(&locked, b"secret\n").expect("write"); + std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o000)).expect("chmod"); + + let rendered = match EditorState::open(locked.clone()) { + Ok(_) => panic!("an unreadable file must fail"), + Err(error) => error.to_string(), + }; + + std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o600)).expect("restore"); + + assert!( + rendered.contains("cannot open"), + "the message must say what failed; got {rendered:?}" + ); + assert!( + rendered.contains(&locked.display().to_string()), + "the message must name the file; got {rendered:?}" + ); +} + +// **P7 — removed in rev 6, not weakened.** +// +// Q#JR12 said a directory argument must suppress desktop restore, and +// rev 5 carried a pin for it. There is nothing to pin. `run` computes +// `had_file = file.is_some()` (`editor.rs:3152`) and a directory path is +// `Some` like any other, so the suppression is structural: no +// directory-specific branch exists that could get it wrong, and the +// named mutation ("pass false for `had_file` on the directory path") +// would require inventing the branch first. +// +// The rev 5 test also never armed desktop restore and hard-coded +// `had_file = true` after startup, so it asserted nothing about `run`'s +// decision and would have passed against any implementation. Keeping a +// green test that cannot fail is worse than having none: it reads as +// coverage. Q#JR12 is downgraded to an observation in the framing. + +/// **P6** — `display_file` keeps its directory-is-an-error contract and +/// does not enter the resolver chain. +/// +/// *Mutation:* route `display_file` into the directory resolver. +/// `find_file_accepting_a_directory_reports_instead_of_raising` in +/// `find_file_acceptance.rs` is the companion pin through find-file's +/// real accept path; this one pins the primitive and the window state. +#[test] +fn preservation_display_file_still_refuses_a_directory() { + let td = project(); + let mut s = EditorState::open(td.path().join("alpha.txt")).expect("open"); + exec(&s, "pmacs.lsp.config = {}"); + let before_name = active_name(&s); + let before_count = buffer_count(&s); + + let raised: bool = eval( + &s, + &format!( + "local ok = pcall(pmacs.window.display_file, {:?}) return not ok", + td.path().display().to_string() + ), + ); + pump(&mut s); + + assert!(raised, "display_file on a directory must raise"); + assert_eq!( + active_name(&s), + before_name, + "a refused display_file must not change the active buffer" + ); + assert_eq!( + buffer_count(&s), + before_count, + "a refused display_file must not create a buffer" + ); +}