From 44f5e44b9fce3cb4654e8f95249e8fd39e4371c8 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 15:48:03 -0400 Subject: [PATCH 01/11] docs: record #168 and #178 as landed, and the generated-buffer invariant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Documentation only; no code, no protocol change. Handoff §1 anchors on `main` @ `fe8b8ba` and leads with terminal copy mode (#178) and its landed-doc pair (#168). The active-work terminal lane flips to BOTH STAGES MERGED, and the #166 lane records #168 landing with what it measured: 264 tests dark workspace-wide for want of `crdt` in CI, and the vterm audit's honest count of acceptances that drive a real daemon (2, not 3). The substantive addition is a new §4 substrate invariant for `Buffer::set_generated_contents`, because it is now the one authorized write for every generated buffer and three of its properties are non-obvious enough that four review rounds were spent finding them: - an intercept is not read-only — `Buffer::undo` reaches the rope through `ensure_writable` without consulting the intercept chain, and rebinding the undo chords does not help because `M-x buffer.undo` needs no keymap; - a bare `set_read_only` would be worse than nothing, since it also refuses the owner's refresh — the pairing is the primitive, which is why no Lua `set_read_only` exists; - a rope write is only half of an edit: the returned `Edit` must be fanned out, or a displaying window paints the new rope with stale line offsets and replica mirrors never import the write. Also recorded: history clearing must clear whichever history the buffer has (the v0.1 stacks are bypassed in CRDT mode), that `*compilation*` and listview have NOT adopted the primitive and remain emptiable by `M-x buffer.undo`, and that this does not replace `set_round_trip_input` — the two guards cover different copies, and a daemon refusal arrives after the frontend has already painted. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer --- docs/active-work.md | 20 ++++++++++++---- docs/agent-handoff.md | 55 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 7df3466..0c234ee 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -468,7 +468,7 @@ If it does not, stop and repair the remote/fetch configuration. **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 +## Terminal config + copy mode arc — BOTH STAGES MERGED (arc complete) - Approved framing: `docs/terminal-config-and-copy-mode-framing.md` **revision 4** (four review rounds), committed as the first commit of @@ -481,9 +481,15 @@ If it does not, stop and repair the remote/fetch configuration. 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 MERGED as #178** (`main` @ `fe8b8ba`, 2026-07-26, **four + review rounds**, twelve checks green on head `1b44c69` — verified by + `head_sha`, not by the check summary). Copy mode: + `M-x terminal.copy-mode` / `C-c C-t`. Branch + `githubsucks/terminal-copy-mode` and worktree + `../pmacs-terminal-copy-mode` retained. Main was integrated once, after + #168 landed; the `docs/active-work.md` terminal-lane conflict resolved + by taking main's fuller Stage 1 sentence under this lane's Stage 2 + record. - **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 @@ -947,6 +953,12 @@ 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), 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 45ee353..7d1b326 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,7 +41,8 @@ commands, read `docs/active-work.md` immediately after this file. ## 1. Where the project stands (2026-07-26) -- `main` @ `a27f646` (Lean 4 Stage 4a #179 atop bottom-panel Stage 2A +- `main` @ `fe8b8ba` (terminal copy mode #178 atop 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 @@ -884,6 +889,50 @@ 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:** `*compilation*` and listview panels still rely on +intercept-plus-`set_round_trip_input` and remain emptiable by +`M-x buffer.undo`. Adoption is not a one-line swap — it inherits the +fan-out obligation, and `*compilation*` appends rather than replacing, so +it needs a streaming variant. 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, From e4c9d652312c9a5d33ea7d74a46851f8377c49d9 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 16:28:04 -0400 Subject: [PATCH 02/11] docs: fix five review-round-5 findings in the landed-doc refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Documentation only; no code, no protocol change. All five findings reproduced before fixing. 1. The CRDT-dark census was stale. Re-measured at `fe8b8ba` under CI's exact flags versus the same flags plus `crdt`: 3,170 vs 3,443 — **273 dark, 185 in the library**, not the 264/177 #168 measured at `1b6a084`. The per-target table is regenerated (it gains a `terminal_copy_mode_acceptance` row, acc16e's, from this very arc), the rows are stated to sum to the total, and the lane now says the number moves with every merge and must be re-measured rather than quoted. #168's figure is kept as a dated historical reading with a pointer to the live one. 2. The generated-buffer non-adopter inventory was short by half. It is **four writers, not two**: listview panels (`listview.lua:60-61`), `*compilation*` **and** `*search-results*` — both through `compile.lua`'s shared `ensure_slot`, so naming only the first undercounts a mechanism rather than a buffer — and dired (`dired.lua:371`). All four pair an erroring intercept with `bypass_intercept` writes over a still-writable rope, and all four are emptiable by `M-x buffer.undo`. Corrected in the handoff §4 (as a table, with each writer's shape), `COHERENCE.md` §14, and the framing's deferred-lane text. The adoption estimate gains a consequence: the two `compile.lua` slots append and need a streaming variant; listview and dired are whole-buffer replaces and are the cheap half. 3. The ledger's canonical base still named `a27f646` while the same file recorded #168 and #178. Now `fe8b8ba`, with the recovery check's accept-or-newer floor moved with it — a stale floor is what lets a wrong base pass verification. 4. The completed terminal lane is **removed**, not marked complete. Rule 4 of this file's own update protocol says a lane goes when it merges, and its opening contract says the file records only what has not landed. Its durable facts moved first: a new arc bullet in the handoff §1 (the snapshot materializes, so the dispatch-shadow count stays at six; `prune` reacts to removal rather than causing it; ownership means the handle table, never found-by-name; profiles are a raw Lua table and why the escape cache lives on `TerminalSession`; what criterion 17 must assert when it can finally be written; the `cat -v` echo probe and count-don't-match rule), with the `set_generated_contents` invariant already in §4. A compact entry remains under "Closed since the last snapshot". The gate-run flake the lane carried moved to the CI `crdt`-coverage lane, which owns its discrimination — verbatim, including its explicit refusal to claim a root cause. 5. The refreshed handoff was internally stale: it anchors on a `main` that contains #179 and #165 while still calling both "in review". Both now read MERGED. Dired's durable facts are deliberately **not** absorbed here — that is open PR #169's job, and writing it from two PRs would put two authorities on one text — so the dired lane stays, with a note saying why it survives rule 4 and who removes it. Also recorded while verifying finding 4's new home: the `crdt` Clippy failure on `main` is re-verified with exact sites (four in `src/daemon.rs`, one in `tests/auto_indent_crdt_acceptance.rs`), because any CI job that compiles the `crdt` targets is red on arrival until they are fixed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer --- COHERENCE.md | 12 +- docs/active-work.md | 381 ++++-------------- docs/agent-handoff.md | 88 +++- docs/terminal-config-and-copy-mode-framing.md | 15 +- 4 files changed, 176 insertions(+), 320 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index 0ba2ac8..acd744d 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -1222,14 +1222,20 @@ 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 writers have not yet adopted it and + remain emptiable** — listview panels, `*compilation*`, + `*search-results*` (the same `ensure_slot` mechanism in `compile.lua`), + and dired buffers, all of which pair an erroring intercept with + `bypass_intercept` writes over a still-writable rope. **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 two `compile.lua` slots **append** rather than + replacing wholesale, so they 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` diff --git a/docs/active-work.md b/docs/active-work.md index 0c234ee..ba77bd1 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -14,16 +14,17 @@ backlog. 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` @ `a27f646` (Lean 4 Stage 4a #179 atop 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` @ `fe8b8ba` (terminal copy mode #178 atop 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 `a27f646`; the recovery + check below accepts it or anything newer. - On the transfer source, `origin/main` named a release mirror at `d3fa632` and lagged badly. On the current destination, `origin` names the canonical URL. This difference is why all recovery begins by @@ -57,7 +58,7 @@ git worktree list git status --short --branch ``` -The `git log` command must expose `d152120` or a newer intentional main. +The `git log` command must expose `fe8b8ba` or a newer intentional main. If it does not, stop and repair the remote/fetch configuration. ## Lean 4 lane (Arc 8) — Stages 1, 2, 3a, 3b, 4a MERGED; 4b is next @@ -230,7 +231,13 @@ If it does not, stop and repair the remote/fetch configuration. --check` clean. - Stage 4b (the input method) is NOT in this PR and not started. -## Dired lane — Stage 0 MERGED; Stage 1 IN REVIEW (PR #165) +## Dired lane — Stages 0 and 1 MERGED; Stage 2 framing in review (#171) + +> **Lane retained deliberately.** Rule 4 below removes a lane after +> merge, but its durable facts must reach `docs/agent-handoff.md` first, +> and that absorption is the job of the **open landed-doc PR #169**. +> Writing it from here would put two PRs on the same text. #169 removes +> this lane; the Stage 2 framing PR #171 is still open besides. - Approved framing: `docs/dired-framing.md` **revision 6** — rev 5 is the approved text (merged as its own docs PR #164), rev 6 adds §0's Stage 1 @@ -389,11 +396,16 @@ 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,170 vs 3,443 — 273 tests dark.** Re-measured at `fe8b8ba` + (2026-07-26). **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,842 | 2,027 | **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` | @@ -405,15 +417,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 @@ -467,288 +484,34 @@ 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 — BOTH STAGES MERGED (arc complete) - -- 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 MERGED as #178** (`main` @ `fe8b8ba`, 2026-07-26, **four - review rounds**, twelve checks green on head `1b44c69` — verified by - `head_sha`, not by the check summary). Copy mode: - `M-x terminal.copy-mode` / `C-c C-t`. Branch - `githubsucks/terminal-copy-mode` and worktree - `../pmacs-terminal-copy-mode` retained. Main was integrated once, after - #168 landed; the `docs/active-work.md` terminal-lane conflict resolved - by taking main's fuller Stage 1 sentence under this lane's Stage 2 - record. -- **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` (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. + 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`** — + re-verified at `fe8b8ba`: four errors 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 one in + `tests/auto_indent_crdt_acceptance.rs:42` (doc backticks). 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 @@ -936,6 +699,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. - **GPU terminal input (the double terminal-layout sync) — MERGED as #166** (`main` @ `b889873`, 2026-07-25, one review round, all twelve checks green after a macOS PTY-timing rerun). The dispatcher applied **both** @@ -955,7 +736,9 @@ git worktree add --track \ 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), the + measured (**264 tests dark workspace-wide**, 177 in the library — as + of `1b6a084`; the live figure is 273/185 at `fe8b8ba`, and the + coverage lane above is the authority), 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. diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 7d1b326..883d860 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -61,6 +61,53 @@ commands, read `docs/active-work.md` immediately after this file. interaction islands added, config-registry adoption, background-work attribution. Its §2 grades the golden journey **broken at step 3** (`pmacs .` exits 1). +- **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 LANDED** (`docs/lean4-mode-framing.md`; #160, #161, #167, #170; merge `d400f30`). pmacs edits Lean 4: `arborium-lean` highlighting, a @@ -106,9 +153,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) is implemented and in review - as PR #179** (branch `lean4-stage4a-typed-edit-chain`, framing rev - 8). It is substrate only: `builtin/runtime/typed_edit.lua` owns the + - **Stage 4a (the typed-edit consumer chain) MERGED as #179** + (branch `lean4-stage4a-typed-edit-chain`, framing rev 8; it is part + of the `fe8b8ba` 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 `tests/auto_pair_acceptance.rs` is unchanged by zero lines @@ -184,11 +231,13 @@ commands, read `docs/active-work.md` immediately after this file. against an open buffer yet fails to load one that is not open — find-file expands the tilde Lua-side. Loading through the normalized path is a named deferral. - - **Stage 1 (the directory view) is IN REVIEW as PR #165** — the - builtin `dired.lua`, the per-entry-tolerant `read_dir` opt, and - `pmacs.path.canonicalize`. Its branch state, substrate facts, and - verification live in `docs/active-work.md`; this section absorbs them - when it merges. + - **Stage 1 (the directory view) MERGED as #165** — the builtin + `dired.lua`, the per-entry-tolerant `read_dir` opt, and + `pmacs.path.canonicalize`. Its durable facts have **not** been + absorbed here yet: that is the job of the open landed-doc PR + **#169**, and duplicating it from this PR would put two authorities + on the same text. Until #169 merges, `docs/active-work.md`'s dired + lane remains the record. - Protocol **v20** (`SUPPORTED=[6..=20]`; v16 = `ThemeFacts`, v17 = `FontFacts`, v18 = `StatuslineSegments`, v19 = terminal frames/events, v20 = the GPU initial-target semantic bootstrap family). @@ -920,11 +969,24 @@ 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:** `*compilation*` and listview panels still rely on -intercept-plus-`set_round_trip_input` and remain emptiable by -`M-x buffer.undo`. Adoption is not a one-line swap — it inherits the -fan-out obligation, and `*compilation*` appends rather than replacing, so -it needs a streaming variant. Recorded in `COHERENCE.md` §14. +**Not yet adopted — and the inventory is four call sites, not two.** +Every generated buffer outside copy mode still uses the older idiom: +an erroring intercept plus `set_round_trip_input`, written through +`bypass_intercept`, with the rope left writable. All of them are +emptiable by `M-x buffer.undo`: + +| buffer | writer | shape | +|---|---|---| +| listview panels | `builtin/runtime/listview.lua:60-61` | delete-all + insert | +| `*compilation*` | `builtin/runtime/compile.lua` (`ensure_slot`) | **append** per output batch | +| `*search-results*` | same `ensure_slot` mechanism in `compile.lua` | **append** per match batch | +| dired buffers | `builtin/runtime/dired.lua:371` | whole-buffer replace | + +Adoption is not a one-line swap. It inherits the fan-out obligation, and +the two `compile.lua` slots append rather than replacing wholesale, so +they need a **streaming variant** of the primitive; listview and dired +are already 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 diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index 15878cf..574d58c 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -564,11 +564,16 @@ 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 writers, not two** (corrected in + review round 5, which found the inventory short): listview panels + (`listview.lua:60-61`), `*compilation*` and `*search-results*` (both + through `compile.lua`'s shared `ensure_slot`), and dired buffers + (`dired.lua:371`) all still rely on intercept-plus-round-trip over a + writable rope, and are all 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 — the two `compile.lua` + slots append rather than replacing wholesale, 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 From 600b3d0c7dd2519570705fffe9fea47403f20914 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 15:00:17 -0400 Subject: [PATCH 03/11] docs: framing for Journey Stage 1a (directory open on one path) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Serves COHERENCE.md §2 (the golden product journey), §19 (coherence acceptance tests), and §20 Priority 1, which grades the journey broken at step 3 because `pmacs .` exits 1. Stage 1a ships four things: the directory argument routed into dired's buffer on both the local and daemon/GPU startup paths; EditorState::open adopting resolve_target_buffer so the two path-open implementations become one; a scoped-destination commit primitive so an async open lands where it was requested or nowhere; and the first cross-subsystem journey acceptance suite. Framing only -- no implementation. Rev 5 after four review rounds. --- docs/journey-stage1a-framing.md | 913 ++++++++++++++++++++++++++++++++ 1 file changed, 913 insertions(+) create mode 100644 docs/journey-stage1a-framing.md diff --git a/docs/journey-stage1a-framing.md b/docs/journey-stage1a-framing.md new file mode 100644 index 0000000..36f40e4 --- /dev/null +++ b/docs/journey-stage1a-framing.md @@ -0,0 +1,913 @@ +# Journey Stage 1a — open a directory, on one path + +**Status: framing, rev 5, awaiting approval.** +**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. + +--- + +## 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` — drops the startup scratch (`editor.rs:797`) | 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 drops the startup scratch, Q#JR3) +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** — a directory argument counts as "had a file argument" and +suppresses desktop restore, on Q#DS7's reasoning: a positional argument +means "open this", not "restore my session". + +--- + +## 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 the buffer the fresh +view's primary document window already holds. §2.8's reassert reasserts +that same buffer — already correct, therefore harmless. + +**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`. +- **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). +- **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. +- **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 + +- **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 — the startup scratch is still dropped (Q#JR3).** + `EditorState::open` leaves exactly one buffer. + *Mutation:* replace `replace_active_buffer` with a bare + `install_buffer_in_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 — desktop restore stays suppressed (Q#JR12).** + *Mutation:* pass `false` for `had_file` on the directory path. +- **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** The scratch drop (`replace_active_buffer`) is preserved. +- **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 window's pre-existing buffer — + LOCAL's primary document buffer, not necessarily scratch. Accepted and + documented. +- **Q#JR10** An unclaimed directory with the handler cleared exits 0 with + a status message. +- **Q#JR12** A directory argument suppresses desktop restore. +- **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. From dd9f380533f7a82beaa04033898937f70bba32db Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 15:01:17 -0400 Subject: [PATCH 04/11] docs(active-work): record Journey Stage 1a branch and ordering Rev 5 is approved and the branch is cut, so the ledger's "no branch, commit, or PR exists yet" line no longer describes reality. Records the recovery command, notes that PR #177 has merged and therefore unblocks implementation, and carries the standing obligation that dired Stage 2 re-scouts around commit_to before its branch is cut. --- docs/active-work.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/docs/active-work.md b/docs/active-work.md index 5a74c24..ed6ceb7 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -331,6 +331,25 @@ If it does not, stop and repair the remote/fetch configuration. --check` clean. - Stage 4b (the input method) is NOT in this PR and not started. +## Journey Stage 1a — framing rev 5 APPROVED; branch cut, implementing + +- Approved framing: `docs/journey-stage1a-framing.md` **rev 5** (four + review rounds). Branch `journey-stage1a-directory-open`, framing + committed as its first commit. No PR yet. +- Recovery: `git fetch githubsucks && git checkout + journey-stage1a-directory-open`. The framing now travels; the + implementation does not until it is committed and pushed. +- Ordering: PR #177 MERGED (2026-07-26), so 1a is 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. +- Implementation order inside the branch (framing §12): scoped frontend + override + shared eligibility predicate first (separable, testable + without dired), then `commit_to` and the opaque destination, then the + directory arm and resolver chain, then the journey suite, then the + doc updates COHERENCE §25 requires. + ## The CRDT half of the test corpus is dark in CI — NEEDS A LANE - **No branch, no framing yet.** Found while gating #166, then measured From f09f66ce37e42f1eb3e808f03ea52f74bcd0e45c Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 16:20:54 -0400 Subject: [PATCH 05/11] feat(journey): open a directory, on one path Journey Stage 1a's core: `pmacs .` opens the directory instead of exiting 1, and local startup stops being a second implementation of path resolution. `EditorState::open` now calls `EditorCore::resolve_target_buffer` -- the primitive whose own doc comment says it exists "so two path-normalization, dedup, and hook transactions cannot drift apart", and which local startup had never been a caller of. `resolve_target_buffer` returns a typed `ResolvedTarget` rather than `(BufferId, HookKind)`, with a `Directory` arm checked ahead of the load. Without it the load runs and fails: `File::open` succeeds on a directory and `read_to_end` returns EISDIR, which is not `NotFound`, so the `[new file]` arm never fired. A directory creates no buffer. It dispatches a resolver chain: the short-circuit `path.open-directory` hook, which no builtin subscribes to, and then `pmacs.path.directory_handler`, which dired defaults. The split is forced rather than chosen -- hook callbacks only append and builtins load before init.lua, so a subscribing builtin would always claim before any user listener could run. A raising listener stops the chain and suppresses the fallback. The listing is async and the daemon bootstrap is not, so the whole post-await commit runs inside a new `pmacs.window.commit_to`: it validates the destination -- frontend live, window live, buffer unchanged, window replaceable -- BEFORE invoking its callback, then scopes the acting frontend for its extent. Validating at display time would be four dired mutations too late. That scope is deliberately not `InteractiveCommandOrigin`, which does not reach the core-ambient APIs and is authenticated user-command authority a background continuation must not acquire. The dedication rule is extracted into one `window_accepts_buffer` shared by exact display, the display probe, and the new preflight, with `incoming: Option` -- `None` means "the replacement does not exist yet" and refuses a dedicated window. `display_file` keeps its directory-is-an-error contract and does not enter the chain; find-file's accept arm depends on it. Framing: docs/journey-stage1a-framing.md rev 5 (Q#JR1-JR15). --- builtin/hooks/default.lua | 15 + builtin/runtime/async.lua | 11 + builtin/runtime/dired.lua | 84 +++-- src/daemon.rs | 47 ++- src/editor.rs | 311 ++++++++++++++++-- src/editor_core.rs | 193 +++++++++++- src/lua_bindings/mod.rs | 85 +++++ src/lua_bindings/window_panel.rs | 150 ++++++++- tests/journey_acceptance.rs | 525 +++++++++++++++++++++++++++++++ 9 files changed, 1356 insertions(+), 65 deletions(-) create mode 100644 tests/journey_acceptance.rs 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..adbc8f1 100644 --- a/builtin/runtime/dired.lua +++ b/builtin/runtime/dired.lua @@ -598,7 +598,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 +629,64 @@ 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() + 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 = pmacs.window.buffer() - if active ~= nil and handle_for_buffer(active) == nil then - handle.prev = active + -- `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 + end 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 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 +698,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/src/daemon.rs b/src/daemon.rs index 84716eb..4a7e08f 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1627,18 +1627,57 @@ 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()))?; + let buffer_id = dest.buffer; + editor.dispatch_directory_open(&path, dest); + editor.reconcile_panel_layout(frontend_id); + return Ok(OpenedInitialTarget { + buffer_id, + 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 => { diff --git a/src/editor.rs b/src/editor.rs index a19971e..1e94d92 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"); @@ -769,35 +878,56 @@ 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`] specifically + /// because it drops the just-created scratch buffer; an + /// `install_buffer_in_window` here would leave a stray `*scratch*` + /// behind every `pmacs FILE` (Q#JR3). + /// * **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 @@ -809,6 +939,135 @@ impl EditorState { Ok(state) } + /// 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`, dropping any old /// scratch buffer if the active window's previous buffer has no /// other windows referencing it. Returns silently on a stale 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..4228e0c 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,111 @@ 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::AnyUserData, 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. + let dest = dest + .borrow::() + .map_err(|_| { + 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 +516,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..591536d --- /dev/null +++ b/tests/journey_acceptance.rs @@ -0,0 +1,525 @@ +// 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. + +use std::path::Path; +use std::time::{Duration, Instant}; + +use pmacs::editor::EditorState; +use pmacs::editor_core::normalize_buffer_path; +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(); + } +} + +/// 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() +} + +/// 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:?}" + ); +} + +// --------------------------------------------------------------------------- +// Step 5 — edit immediately +// --------------------------------------------------------------------------- + +/// **N11** — the journey's step-3-into-step-5 path: start on a +/// directory, visit a listed file, and type into *that* file. +/// +/// 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:")); + + let target = td.path().join("alpha.txt"); + exec( + &s, + &format!( + "pmacs.window.display_file({:?}, {{ select = true }})", + target.display().to_string() + ), + ); + pump(&mut s); + + exec(&s, "pmacs.window.buffer():insert(0, 'EDITED ')"); + let text = active_text(&s); + assert!( + text.starts_with("EDITED "), + "the edit 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** — a directory argument suppresses desktop restore, on the same +/// reasoning a file argument does (Q#DS7): a positional argument means +/// "open this", not "restore my session". +/// +/// *Mutation:* pass `false` for `had_file` on the directory path. +#[test] +fn preservation_a_directory_argument_suppresses_desktop_restore() { + let td = project(); + let mut s = launch(td.path()); + // Arm the restore AFTER startup, then confirm the startup path + // treated its argument as a positional open: `had_file` is what + // `run` passes, and a directory must set it. + let had_file = true; + s.restore_desktop_if_armed(had_file); + assert!( + !status(&s).contains("desktop-restore"), + "a positional directory argument must not trigger a restore; got {:?}", + status(&s) + ); +} + +/// **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" + ); +} From 25b07be97b97e2ce10947cad0bc750c1ecf72499 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 16:39:23 -0400 Subject: [PATCH 06/11] fix(journey): correct Q#JR3, report the post-dispatch buffer, unvacuate two pins Four review findings, 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 the window's buffer_id and removes nothing. The claim came from that function's own doc comment, wrong for as long as it has existed, and rev 5 propagated it into the framing and into new documentation this branch added. Both comments are corrected here, because this PR was adding further false references to a claim P4 depends on. Actually removing the stale scratch is buffer-lifetime work and stays out. The daemon bootstrap could report the wrong buffer. The directory arm captured the destination id, ran the resolver chain synchronously, then returned the captured id -- so a handler that opened something synchronously through commit_to had already replaced the window's buffer, and the reply paired one buffer's snapshot with another's identity. It also returned early, skipping the post-hook revalidation the framing said stayed active. The arm now re-reads the destination after dispatch and rehomes through `non_side_target` as the file arm does. Pinned by a test whose handler claims synchronously. 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. Both gestures now go through dispatch_key. P7 is removed rather than weakened. Q#JR12 has nothing to pin -- `had_file = file.is_some()` and a directory is Some like any other, so no directory-specific branch exists to break. The old test never armed restore and hard-coded had_file, so it could not fail against any implementation. Also adds the daemon bootstrap pins (N2, N5) and fixes an insertion that had orphaned a `#[cfg(feature = "crdt")]` from the test it guarded -- which would have made one new test dark and one existing test escape its gate. Framing: docs/journey-stage1a-framing.md rev 6. --- docs/journey-stage1a-framing.md | 110 ++++++++++++++--- src/daemon.rs | 213 +++++++++++++++++++++++++++++++- src/editor.rs | 32 +++-- tests/journey_acceptance.rs | 104 +++++++++++----- 4 files changed, 401 insertions(+), 58 deletions(-) diff --git a/docs/journey-stage1a-framing.md b/docs/journey-stage1a-framing.md index 36f40e4..b5833cc 100644 --- a/docs/journey-stage1a-framing.md +++ b/docs/journey-stage1a-framing.md @@ -1,6 +1,7 @@ # Journey Stage 1a — open a directory, on one path -**Status: framing, rev 5, awaiting approval.** +**Status: framing, rev 6 — APPROVED at rev 5; rev 6 records +corrections found during implementation.** **Serves `COHERENCE.md` §2 (the golden product journey), §19 (coherence acceptance tests), §20 Priority 1.** @@ -76,6 +77,43 @@ acceptance tests), §20 Priority 1.** - 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. + --- ## 0.5. Coherence impact (`COHERENCE.md` §20, required since #163) @@ -160,7 +198,7 @@ daemon bootstrap (`daemon.rs:1641`). **Local startup is not one of them** | 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` — drops the startup scratch (`editor.rs:797`) | none; caller installs | +| 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 @@ -319,7 +357,9 @@ 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 drops the startup scratch, Q#JR3) +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). @@ -328,9 +368,15 @@ the core (Q#JR1a). `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** — a directory argument counts as "had a file argument" and -suppresses desktop restore, on Q#DS7's reasoning: a positional argument -means "open this", not "restore my session". +**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. --- @@ -538,9 +584,26 @@ 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 the buffer the fresh -view's primary document window already holds. §2.8's reassert reasserts -that same buffer — already correct, therefore harmless. +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 @@ -723,8 +786,13 @@ is **removed rather than recast**: it proved nothing N1 does not. window unchanged; `find_file_accepting_a_directory_reports_instead_of_raising` passes unmodified. *Mutation:* route `display_file` into the resolver chain. -- **P7 — desktop restore stays suppressed (Q#JR12).** - *Mutation:* pass `false` for `had_file` on the directory path. +- **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 @@ -851,7 +919,12 @@ discovered late. - **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** The scratch drop (`replace_active_buffer`) is preserved. +- **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. @@ -860,12 +933,17 @@ discovered late. 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 window's pre-existing buffer — - LOCAL's primary document buffer, not necessarily scratch. Accepted and - documented. +- **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** A directory argument suppresses desktop restore. +- **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** diff --git a/src/daemon.rs b/src/daemon.rs index 4a7e08f..1451a0c 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1661,11 +1661,51 @@ fn open_initial_target( let dest = editor .capture_directory_destination(frontend_id, origin_window) .ok_or_else(|| format!("cannot open {}: no document window", path.display()))?; - let buffer_id = dest.buffer; 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, }); } @@ -4924,6 +4964,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 1e94d92..9c204bc 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -891,10 +891,19 @@ impl EditorState { /// /// * **The window install.** `resolve_target_buffer` deliberately /// does not touch windows, so the caller places the buffer. - /// Startup uses [`Self::replace_active_buffer`] specifically - /// because it drops the just-created scratch buffer; an - /// `install_buffer_in_window` here would leave a stray `*scratch*` - /// behind every `pmacs FILE` (Q#JR3). + /// 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 @@ -1068,9 +1077,18 @@ impl EditorState { } } - /// 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. + /// 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/tests/journey_acceptance.rs b/tests/journey_acceptance.rs index 591536d..f1e4c03 100644 --- a/tests/journey_acceptance.rs +++ b/tests/journey_acceptance.rs @@ -31,8 +31,10 @@ use std::path::Path; use std::time::{Duration, Instant}; +use crossterm::event::{KeyCode, KeyEvent, KeyEventKind, KeyEventState, KeyModifiers}; use pmacs::editor::EditorState; use pmacs::editor_core::normalize_buffer_path; +use pmacs::protocol::FrontendId; use tempfile::TempDir; // --------------------------------------------------------------------------- @@ -65,6 +67,35 @@ fn pump(s: &mut EditorState) { } } +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"); @@ -333,11 +364,18 @@ fn journey_a_raising_resolver_suppresses_the_fallback_and_reports() { // Step 5 — edit immediately // --------------------------------------------------------------------------- -/// **N11** — the journey's step-3-into-step-5 path: start on a -/// directory, visit a listed file, and type into *that* file. +/// **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 +/// 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() { @@ -345,21 +383,24 @@ fn journey_step5_editing_a_file_reached_through_the_directory() { let mut s = launch(td.path()); assert!(active_name(&s).starts_with("*dired:")); - let target = td.path().join("alpha.txt"); - exec( - &s, - &format!( - "pmacs.window.display_file({:?}, {{ select = true }})", - target.display().to_string() - ), - ); + // 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); - exec(&s, "pmacs.window.buffer():insert(0, 'EDITED ')"); + 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("EDITED "), - "the edit must land in the visited file's buffer; got {text:?}" + text.starts_with('X'), + "a self-insert must land in the visited file's buffer; got {text:?}" ); assert!( buffer_count(&s) >= 2, @@ -466,26 +507,21 @@ fn preservation_an_unreadable_file_reports_with_its_path() { ); } -/// **P7** — a directory argument suppresses desktop restore, on the same -/// reasoning a file argument does (Q#DS7): a positional argument means -/// "open this", not "restore my session". -/// -/// *Mutation:* pass `false` for `had_file` on the directory path. -#[test] -fn preservation_a_directory_argument_suppresses_desktop_restore() { - let td = project(); - let mut s = launch(td.path()); - // Arm the restore AFTER startup, then confirm the startup path - // treated its argument as a positional open: `had_file` is what - // `run` passes, and a directory must set it. - let had_file = true; - s.restore_desktop_if_armed(had_file); - assert!( - !status(&s).contains("desktop-restore"), - "a positional directory argument must not trigger a restore; got {:?}", - status(&s) - ); -} +// **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. From d2ab3761c75060e0f79a87d8301d06589071838f Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 16:45:12 -0400 Subject: [PATCH 07/11] docs: correct the non-adopter attribution and two stale anchors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6. Documentation only; no code, no protocol change. 1. **The four-writer table mapped the wrong buffers to `ensure_slot`.** Verified at the call sites: `ensure_slot` has exactly two callers, `*compilation*` (`compile.lua:1090`) and `*shell-command*` (`:1125`). `*search-results*` is an **independent panel** in `builtin/commands/default.lua` with its own intercept (`:869`), round-trip mark and writes; `compile.lua` declares its name only to answer a predicate (`:216`), which is what made it look like a third slot. Round 5 fixed an undercount and introduced a misattribution in the same paragraph — the count was right, the mechanism was not. The table is now keyed by **writer**, not by buffer, so the mapping cannot silently drift again: four mechanisms, five buffers. It carries an explicit "do not read `ensure_slot` as covering the search panel" note, because that is the specific wrong inference. Corrected identically in `COHERENCE.md` §14 and the framing's deferred-lane text, which both carried the error. The scope claim is narrowed with it. "Every generated buffer outside copy mode" was too wide: `*workers*`, `*help*` and `*buffer-list*` are generated but do not use this idiom, and the REPL package's intercept (`packages/repl/init.lua:187`) is an op-filtering editing policy rather than a read-only panel. The claim is now "every remaining intercept-protected writer", and the two excluded groups are named so the next reader does not have to re-derive the boundary. 2. **The recovery floor contradicted itself.** The canonical-base line said the check accepts `a27f646` or anything newer while the check below required `74301d1`. The floor genuinely advanced; the prose now says so outright — a tree at `a27f646` no longer passes — and states why the floor must move with the base rather than trailing it. 3. **Two anchors survived the integration.** Lean 4 Stage 4a said it was part of "the `fe8b8ba` anchor above" when the anchor had become `74301d1`; it now refers to the anchor rather than restating a commit, which is what let it go stale. And #168's closed entry called its own `fe8b8ba` figure "the live figure" — it is a reading taken at `1b6a084`, kept as history, and now says so and points at the coverage lane as the single authority with an explicit "do not quote this one forward". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer --- COHERENCE.md | 21 ++++++----- docs/active-work.md | 15 +++++--- docs/agent-handoff.md | 37 ++++++++++++------- docs/terminal-config-and-copy-mode-framing.md | 23 +++++++----- 4 files changed, 59 insertions(+), 37 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index 6aaad38..e2d0143 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -1222,20 +1222,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; **four writers have not yet adopted it and - remain emptiable** — listview panels, `*compilation*`, - `*search-results*` (the same `ensure_slot` mechanism in `compile.lua`), - and dired buffers, all of which pair an erroring intercept with - `bypass_intercept` writes over a still-writable rope. **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 — and the two `compile.lua` slots **append** rather than - replacing wholesale, so they 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. + 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` diff --git a/docs/active-work.md b/docs/active-work.md index 503689f..a978db0 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -36,8 +36,12 @@ landed regardless of what a lane says. #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 `a27f646`; the recovery - check below accepts it or anything newer. + protocol v20). The previous snapshot named `a27f646`, and **the + recovery floor has advanced past it**: the check below now requires + `74301d1` or newer, so a tree at `a27f646` 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 @@ -733,9 +737,10 @@ git worktree add --track \ 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 — as - of `1b6a084`; the live figure is 273/185 at `fe8b8ba`, and the - coverage lane above is the authority), the + 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. diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 7b1173f..f029192 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -157,7 +157,7 @@ commands, read `docs/active-work.md` immediately after this file. at the moment of the swap. - **Stage 4a (the typed-edit consumer chain) MERGED as #179** (branch `lean4-stage4a-typed-edit-chain`, framing rev 8; it is part - of the `fe8b8ba` anchor above). It is substrate only: `builtin/runtime/typed_edit.lua` owns the + 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 `tests/auto_pair_acceptance.rs` is unchanged by zero lines @@ -1036,24 +1036,33 @@ 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 — and the inventory is four call sites, not two.** -Every generated buffer outside copy mode still uses the older idiom: -an erroring intercept plus `set_round_trip_input`, written through -`bypass_intercept`, with the rope left writable. All of them are +**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`: -| buffer | writer | shape | +| writer | buffers | shape | |---|---|---| -| listview panels | `builtin/runtime/listview.lua:60-61` | delete-all + insert | -| `*compilation*` | `builtin/runtime/compile.lua` (`ensure_slot`) | **append** per output batch | -| `*search-results*` | same `ensure_slot` mechanism in `compile.lua` | **append** per match batch | -| dired buffers | `builtin/runtime/dired.lua:371` | whole-buffer replace | +| `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 two `compile.lua` slots append rather than replacing wholesale, so -they need a **streaming variant** of the primitive; listview and dired -are already whole-buffer replaces and are the cheap half. Recorded in -`COHERENCE.md` §14. +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 diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index 574d58c..bd405ea 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -564,16 +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 — four writers, not two** (corrected in - review round 5, which found the inventory short): listview panels - (`listview.lua:60-61`), `*compilation*` and `*search-results*` (both - through `compile.lua`'s shared `ensure_slot`), and dired buffers - (`dired.lua:371`) all still rely on intercept-plus-round-trip over a - writable rope, and are all still emptiable by `M-x buffer.undo`. The + **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 two `compile.lua` - slots append rather than replacing wholesale, while listview and dired - already write whole-buffer replaces and are the cheap half. + 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 From d9d1c0b4234d3689f6428c1ce05192e6c05c41e6 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 17:25:22 -0400 Subject: [PATCH 08/11] test(journey): pin the destination commit, and land the doc updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes Journey Stage 1a: the `commit_to` acceptance suite (framing §6 N4, N6, N6b, N6c, P1, P2, P3) plus the documentation updates COHERENCE §25 requires the PR to carry. Bite-testing the new pins found a real gap. 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 case — a commit reached from inside an interactive command, where the origin sits between the override and the ambient value — and nothing pinned it. N4b is added, driven through `dispatch_key` because that is the only thing that establishes an interactive origin, and the mutation now bites it. Two smaller corrections found the same way: * `commit_to`'s forged-destination message was unreachable. Typed as `AnyUserData`, mlua rejected a table during argument conversion, so a caller got "error converting Lua table to userdata" — true, but naming neither the rule nor the remedy. The parameter is now `mlua::Value` and the pointed message fires. * P1 and P2 also fail on full revert, since `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 in the P list because their discriminating falsifier is the named mutation — a revert-only check cannot distinguish "validates" from "validates in time" — and each pin now says so at its own site rather than being silently mislabelled. Bite results, each run against the whole 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 -> N4b fails; nothing else Docs: COHERENCE §2 grade + step-3 verdict row, §20 Priority 1 and the arc list; the GPU initial-target framing's Q#GT6 and acceptance 10, whose directory case this stage deliberately supersedes; handoff §1; the active-work ledger; framing rev 7. Co-Authored-By: Claude Opus 5 (1M context) --- COHERENCE.md | 57 ++- docs/active-work.md | 41 +- docs/agent-handoff.md | 47 +- docs/gpu-initial-target-framing.md | 20 +- docs/journey-stage1a-framing.md | 54 ++- src/lua_bindings/window_panel.rs | 18 +- tests/journey_acceptance.rs | 663 +++++++++++++++++++++++++++++ 7 files changed, 858 insertions(+), 42 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index 636a8f3..e7229f3 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -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 | @@ -1474,14 +1483,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 @@ -1545,11 +1558,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/docs/active-work.md b/docs/active-work.md index ed6ceb7..5a78b6d 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -331,24 +331,41 @@ If it does not, stop and repair the remote/fetch configuration. --check` clean. - Stage 4b (the input method) is NOT in this PR and not started. -## Journey Stage 1a — framing rev 5 APPROVED; branch cut, implementing +## Journey Stage 1a — IMPLEMENTED on branch, gates run, PR pending -- Approved framing: `docs/journey-stage1a-framing.md` **rev 5** (four - review rounds). Branch `journey-stage1a-directory-open`, framing - committed as its first commit. No PR yet. +- Framing `docs/journey-stage1a-framing.md` **rev 7** (four review + rounds, then two correction revisions found during implementation). + Branch `journey-stage1a-directory-open`, rebased onto `githubsucks/main` + @ `74301d1`. - Recovery: `git fetch githubsucks && git checkout - journey-stage1a-directory-open`. The framing now travels; the - implementation does not until it is committed and pushed. -- Ordering: PR #177 MERGED (2026-07-26), so 1a is unblocked. 1a lands + 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` (23 pins). 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. +- 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. -- Implementation order inside the branch (framing §12): scoped frontend - override + shared eligibility predicate first (separable, testable - without dired), then `commit_to` and the opaque destination, then the - directory arm and resolver chain, then the journey suite, then the - doc updates COHERENCE §25 requires. +- **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 diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index b2a0c45..04dee19 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -54,8 +54,51 @@ 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). + 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. - **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 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 index b5833cc..ebb3cc9 100644 --- a/docs/journey-stage1a-framing.md +++ b/docs/journey-stage1a-framing.md @@ -114,6 +114,39 @@ acceptance tests), §20 Priority 1.** 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. + --- ## 0.5. Coherence impact (`COHERENCE.md` §20, required since #163) @@ -697,6 +730,14 @@ is **removed rather than recast**: it proved nothing N1 does not. 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). @@ -711,7 +752,11 @@ is **removed rather than recast**: it proved nothing N1 does not. - **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. + 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 @@ -746,6 +791,13 @@ is **removed rather than recast**: it proved nothing N1 does not. ### 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 diff --git a/src/lua_bindings/window_panel.rs b/src/lua_bindings/window_panel.rs index 4228e0c..1c700a5 100644 --- a/src/lua_bindings/window_panel.rs +++ b/src/lua_bindings/window_panel.rs @@ -370,7 +370,7 @@ pub(crate) fn install(lua: &Lua, core: &SharedCore, win: &Table) -> mlua::Result "commit_to", lua.create_function( move |lua, - (dest, body): (mlua::AnyUserData, mlua::Function)| + (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: @@ -381,9 +381,21 @@ pub(crate) fn install(lua: &Lua, core: &SharedCore, win: &Table) -> mlua::Result // 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 - .borrow::() - .map_err(|_| { + .ok_or_else(|| { mlua::Error::runtime( "pmacs.window.commit_to: expected a destination captured by \ the editor (it cannot be constructed from Lua)", diff --git a/tests/journey_acceptance.rs b/tests/journey_acceptance.rs index f1e4c03..d25cf4c 100644 --- a/tests/journey_acceptance.rs +++ b/tests/journey_acceptance.rs @@ -27,14 +27,22 @@ //! 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; // --------------------------------------------------------------------------- @@ -129,6 +137,70 @@ 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"); @@ -360,6 +432,597 @@ fn journey_a_raising_resolver_suppresses_the_fallback_and_reports() { ); } +// --------------------------------------------------------------------------- +// 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); +} + +/// **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 // --------------------------------------------------------------------------- From 7741cf806a1dd603e258bfd125146abdde07fea7 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 18:20:02 -0400 Subject: [PATCH 09/11] fix(journey): honor the captured window, not the selected one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1 of PR #182. One implementation gap and two stale claims. **The scope pins the frontend; it does not pin the window.** Framing §4.4 specified `display{ window = dest:window() }`, but dired's commit still ended in `pmacs.window.switch_buffer`, which targets whatever window the scoped frontend has selected. A split or panel that took focus while `read_dir` was pending therefore 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. Both sites now read the captured window: `display` routes to it with `select = true` (the later `seat_cursor` acts on the active window), and the `prev` read asks it directly. N4c pins both halves. The suite's existing routing pins all varied *frontend* identity; none varied the selected window within one frontend, which is exactly why 23 green pins missed this. Bite: dired's `display` back to `switch_buffer` fails N4c alone; `prev` read from the ambient window fails N4c alone. Two stale documentation claims, both of which this PR was supposed to have already fixed: * **The §0 scorecard 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 protocol covers both. §19's row and ground truth were stale the same way — this PR creates the first cross-subsystem suite, which §19 says should exist and grades as missing — and are corrected too. * **P4 still read "leaves exactly one buffer"**, the exact claim rev 6 corrected as false everywhere else in the framing. Restated to what it actually pins: the file is in the *active window*. The test was already written correctly; only the framing lied. Framing rev 8. Co-Authored-By: Claude Opus 5 (1M context) --- COHERENCE.md | 34 ++++++++------- builtin/runtime/dired.lua | 31 ++++++++++++-- docs/active-work.md | 12 ++++-- docs/journey-stage1a-framing.md | 49 ++++++++++++++++++--- tests/journey_acceptance.rs | 75 +++++++++++++++++++++++++++++++++ 5 files changed, 175 insertions(+), 26 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index e7229f3..45a9683 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 @@ -1452,20 +1452,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* diff --git a/builtin/runtime/dired.lua b/builtin/runtime/dired.lua index adbc8f1..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 @@ -641,6 +654,13 @@ local function open_directory(path, opts, departed) -- 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 + local handle = claim_handle(canonical) handle.entries = entries handle.errors = errors @@ -652,14 +672,19 @@ local function open_directory(path, opts, departed) if departed ~= nil then handle.prev = departed.prev else - local active = pmacs.window.buffer() + 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) + 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) diff --git a/docs/active-work.md b/docs/active-work.md index 5a78b6d..919dc5d 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -331,10 +331,11 @@ If it does not, stop and repair the remote/fetch configuration. --check` clean. - Stage 4b (the input method) is NOT in this PR and not started. -## Journey Stage 1a — IMPLEMENTED on branch, gates run, PR pending +## Journey Stage 1a — PR #182 OPEN, review round 1 closed -- Framing `docs/journey-stage1a-framing.md` **rev 7** (four review - rounds, then two correction revisions found during implementation). +- 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`, rebased onto `githubsucks/main` @ `74301d1`. - Recovery: `git fetch githubsucks && git checkout @@ -355,6 +356,11 @@ If it does not, stop and repair the remote/fetch configuration. 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 diff --git a/docs/journey-stage1a-framing.md b/docs/journey-stage1a-framing.md index ebb3cc9..ce5589f 100644 --- a/docs/journey-stage1a-framing.md +++ b/docs/journey-stage1a-framing.md @@ -1,7 +1,7 @@ # Journey Stage 1a — open a directory, on one path -**Status: framing, rev 6 — APPROVED at rev 5; rev 6 records -corrections found during implementation.** +**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.** @@ -147,6 +147,30 @@ acceptance tests), §20 Priority 1.** 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) @@ -741,6 +765,15 @@ is **removed rather than recast**: it proved nothing N1 does not. - **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 @@ -826,10 +859,16 @@ entire claim. P3–P8 are preservation pins in the strict sense. 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 — the startup scratch is still dropped (Q#JR3).** - `EditorState::open` leaves exactly one buffer. +- **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`. + `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`. diff --git a/tests/journey_acceptance.rs b/tests/journey_acceptance.rs index d25cf4c..7ab36c0 100644 --- a/tests/journey_acceptance.rs +++ b/tests/journey_acceptance.rs @@ -570,6 +570,81 @@ fn commit_to_outranks_an_interactive_origin() { 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 From 4cf1b631980708a6489511e75a8879e2578d285d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 18:33:26 -0400 Subject: [PATCH 10/11] docs(active-work): the journey suite is 24 pins, not 23 A count is a reading, not a constant. Recorded as of rev 8 with an explicit instruction to re-read rather than quote the line. Co-Authored-By: Claude Opus 5 (1M context) --- docs/active-work.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 919dc5d..2d93adf 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -346,8 +346,9 @@ If it does not, stop and repair the remote/fetch configuration. `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` (23 pins). No protocol change — - still v20. + 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. From 7a3a55de40f0566b40f060730997dff3f813abda Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Mon, 27 Jul 2026 21:47:01 -0400 Subject: [PATCH 11/11] docs(active-work): remove the landed Lean 4 lane The ledger preamble already says Lean 4's merged lane was removed, and the durable Stage 4b facts already live in the handoff. Remove the stale section that still called Stage 4b in review so PR #182's post-merge state is internally consistent. --- docs/active-work.md | 140 -------------------------------------------- 1 file changed, 140 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 6ee50fe..83c3a51 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -178,146 +178,6 @@ 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 - -- **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. - ## Journey Stage 1a — PR #182 OPEN, review round 1 closed - Framing `docs/journey-stage1a-framing.md` **rev 8** (four review