From 8c924ebf41a1236779832f261af0d011a4bf513f Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 16:17:24 -0400 Subject: [PATCH 01/18] docs: record #166 as landed and open the CI-coverage lane The handoff snapshot and the active-work ledger both still described the GPU terminal input work as in review. Per their own update protocols this should have ridden #166; it did not, because the review that surfaced the CI-coverage finding came after that PR was already green, and expanding an approved PR to carry a new lane would have been the wrong trade. docs/agent-handoff.md section 1 gains the #166 entry: the split into a frontend-kind-neutral liveness half and a grid-only geometry half, the extracted dispatcher loop body, the trap about the no-placement release that reads like liveness and is not, and why the one-line guard was rejected. docs/active-work.md moves the lane to "Closed since the last snapshot" and opens a new one: the Stage 3 real-path acceptance is dark in CI. The workflow never enables the crdt feature, so every crdt-gated acceptance test is not merely skipped but never compiled -- which covers a37 (real daemon, real PTY, real wgpu) since #135 as well as the two tests #166 added beside it. The fix is one step on the gpu-render job, but it needs its own lane because it would run a37 under lavapipe for the first time, and neither its timing budgets nor its wgpu path have been exercised on that adapter. The lane also asks which other crdt-gated suites are dark for the same reason. Recorded alongside it: #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, so the regression protection is live even though the real-daemon evidence is local-only. No code changes. --- docs/active-work.md | 93 ++++++++++++++++++++----------------------- docs/agent-handoff.md | 43 +++++++++++++++++++- 2 files changed, 85 insertions(+), 51 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 1cff0f2..5624d24 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -227,57 +227,32 @@ If it does not, stop and repair the remote/fetch configuration. buffer a directory should resolve *to*, and `pmacs .` should route into it rather than growing a second directory surface. -## GPU terminal input lane — IN REVIEW +## Stage 3 acceptance is dark in CI — NEEDS A LANE -- Portable branch: `githubsucks/gpu-terminal-input`, worktree - `../pmacs-gui-term-input`, based on `githubsucks/main` @ `46a1b8f`. -- Approved framing: `docs/gpu-terminal-input-framing.md` revision 2, - committed as the branch's first commit (`9a0df21`). Bug fix, not a - feature; **no protocol change (stays v20)**. -- Reported as "text input within the terminal doesn't work on GUI, this is - fine in TUI". Root cause: the dispatcher applied **both** terminal-layout - syncs to **every** attached frontend, and a semantic session satisfies both - conditions (a `term_sizes` entry from `AttachRequest` *and* a terminal - declaration). Its PTY was resized twice per tick forever — grid arm installs - the TUI placement size, semantic arm installs the declared content - rectangle, each arm's idempotence guard seeing only what the other just - wrote — so the child took a `SIGWINCH` storm at tick cadence. -- **The fix is a split, not a guard.** The grid arm is also the only per-tick - controller-liveness release a semantic frontend gets, and - `sync_semantic_terminal_layout` cannot take that over: the buffer-follow - snapshot clears the viewport declaration (`on_buffer_snapshot_sent`), so - that arm stops running in exactly the switch-away case that needs the - release. `sync_terminal_layout` is therefore split into a - frontend-kind-neutral half (panel reconcile + liveness) and a grid-only - geometry half, with the loop body extracted to - `sync_terminal_layouts_for_tick` so the exclusivity is structural and tests - drive the real thing. -- **Trap for anyone touching this again:** the release at the "no - `window_placements` entry" arm reads like liveness and is grid geometry. A - semantic frontend has no placement entry at all, so moving it into the - neutral half releases a GPU controller every tick. -- Bite-verified against **two** pre-images, because the naive guard fixes the - storm and introduces the leak: - - | pin | `main` | naive guard | the split | - |---|---|---|---| - | settle (acc 2+3) | FAIL | pass | pass | - | controller release (acc 6) | pass | FAIL | pass | - | grid still resizes (acc 5) | pass | pass | pass | - -- Real-path evidence: a quiet child trapping `SIGWINCH` reports **144 frames - in 4 s and `WINCH 1..12` on screen** against the pre-fix tree, versus a - settled screen with the fix. -- **Deliberately out of scope, named:** interactive-shell echo on a raw-mode - PTY (Q#GT5 — reproduces in-process too, so it is not the GUI/TUI - asymmetry), and a geometry change appearing to clear the visible screen - (reproduces pre-fix; why acceptance 4 latches its observation across - frames). -- Verification on this branch: `cargo fmt --check` clean; strict workspace - Clippy clean; 1,829 default + 2,006 CRDT library tests; vterm Stage 1/2/3 - 10 / 6 / 9 CRDT; bottom-panel Stage 1 46; M4 121; required GPU 155; - **isolated-config workspace sweep 3,177 across 92 suites, zero failures**; - `git diff --check` clean. Gates were run against the committed tree. +- **No branch, no framing yet.** Found while gating #166; deliberately kept + out of it so a CI change would not arrive after review approval. +- `.github/workflows/ci.yml` **never enables the `crdt` feature** (grep the + workflow directory: zero hits). Every `#[cfg(feature = "crdt")]` acceptance + test is therefore not merely skipped in CI — it is **not compiled**. +- That covers the whole Vterm Stage 3 real-path acceptance, including `a37` + (real daemon + real PTY + real wgpu), which has been dark since #135, and + the two tests #166 added beside it. +- The `gpu-render` job is the only one with lavapipe and + `PMACS_REQUIRE_GPU=1`, and it runs `cargo test -p pmacs-gpu`, which never + reaches the `pmacs` crate's acceptance suites. +- The shape of the fix is one step on the `gpu-render` job: + `cargo test --features crdt --test vterm_stage3_acceptance -- --test-threads=1`. + It needs its own lane rather than a drive-by because it would run `a37` + under lavapipe **for the first time**, and neither its timing budgets nor + its wgpu path have ever been exercised on that adapter or on macOS CI. +- Worth auditing at the same time: which *other* `crdt`-gated acceptance + suites are dark for the same reason. This is a coverage question about the + gate list itself, not about any one suite. +- Mitigating fact, verified rather than assumed: #166's three unit pins are + **not** `crdt`-gated and do run under CI's exact flags + (`--no-default-features --features luajit|lua54`), including the + controller-release pin whose only job is catching the plausible wrong fix. + The regression protection is live; the real-daemon evidence is local-only. ## Bottom-panel lane (window placement + side windows) — Stage 1 IN REVIEW @@ -483,6 +458,24 @@ git worktree add --track \ ## Closed since the last snapshot +- **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** + terminal-layout syncs to **every** attached frontend; a semantic session + satisfies both conditions, so its PTY was resized twice per tick forever and + the child took a `SIGWINCH` storm that made a GPU terminal untypable while + output still flowed. `sync_terminal_layout` is now split into a + frontend-kind-neutral half (panel reconcile + controller liveness) and a + grid-only geometry half, with the loop body extracted to + `sync_terminal_layouts_for_tick` so the exclusivity is structural. No + protocol change (v20). Durable lessons are in `docs/agent-handoff.md` §5; + the framing (`docs/gpu-terminal-input-framing.md` rev 2) carries three + falsified hypotheses, the two-pre-image bite matrix, and two named + out-of-scope items (Q#GT5 interactive-shell echo on a raw PTY, which + 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. + - **GPU initial target — MERGED as #148** (`main` @ `0dd16a5`, 2026-07-24, after two review rounds). `pmacs --gpu [--socket …] FILE` opens a target before the GPU window appears. Protocol bumped 19 → 20: a semantic-session diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index d1f4943..e28911f 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -1,6 +1,9 @@ # Agent handoff — cross-machine continuity -**Last updated: 2026-07-25, after find-file (#162) landed — the dired +**Last updated: 2026-07-25, after the GPU terminal input fix (#166) landed — +the double terminal-layout sync that made a GPU terminal untypable — following +Lean 4 Stage 2 (#161), the dired framing pair (#163/#164), find-file (#162) — +the dired arc's Stage 0 — following COHERENCE.md (#163), Lean 4 Stage 1 (#160), the minimap blank-slab fix (#159), bottom-panel Stage 1 (#155), the inline-math re-scout (#154), the vterm PTY-flake fix (#153), and the @@ -578,6 +581,44 @@ commands, read `docs/active-work.md` immediately after this file. - Remaining ranked arcs: 6 folding Stage 3, 7 DAP, 8 GPU splits, plus the `.ipynb` arc (its JSON-grammar prerequisite shipped in #123). +- **GPU terminal input LANDED — #166** (`main` @ `b889873`; + `docs/gpu-terminal-input-framing.md` rev 2; one review round). The + dispatcher applied **both** terminal-layout syncs to **every** attached + frontend each tick. A semantic session satisfies both conditions — a + `term_sizes` entry from `AttachRequest` *and* a terminal declaration — so + its PTY was resized twice per tick forever: the grid arm installed the TUI + placement size, the semantic arm the declared content rectangle, each arm's + `old_size == size` guard seeing only what the other had just written. The + child took a `SIGWINCH` storm at tick cadence, which made typing into a GPU + terminal impossible while output kept flowing. TUI was structurally + unaffected. + - `EditorInstance::sync_terminal_layout` is split into + `sync_terminal_controller_liveness` (frontend-kind **neutral**: panel + reconcile + release of a controller whose window moved away — reads only + views/windows/controller, never a grid size) and + `sync_terminal_grid_geometry` (**grid only**: TUI placement + resize). + `sync_terminal_layout` survives as the composition, so `editor::run` and + `LOCAL` are byte-identical. + - `daemon::sync_terminal_layouts_for_tick` is the extracted loop body: + liveness for every frontend once per tick, then **exactly one** geometry + arm keyed on `semantic_states` membership — the same fact session + establishment uses, so the arms cannot both fire. + - **The trap, kept in a comment:** the release on a missing + `window_placements` entry reads like liveness and is grid geometry. A + semantic frontend has no placement entry at all, so moving it into the + neutral half would release a GPU controller every tick. + - Why not the one-line guard: the grid arm was also the **only** per-tick + controller-liveness release a semantic frontend got, and + `sync_semantic_terminal_layout` cannot take it over — the buffer-follow + snapshot clears the viewport declaration, so that arm stops running in + exactly the switch-away case that needs the release. + - No protocol change (v20). Gates: 1,829 default + 2,006 CRDT library + tests; vterm Stage 1/2/3 10/6/9 CRDT; bottom-panel 46; M4 121; required + GPU 155; isolated-config workspace sweep 3,177 across 92 suites. + - **Known gap, its own lane:** CI never enables `crdt`, so the Stage 3 + real-path acceptance (including `a37`) is not compiled there. #166's unit + pins are not `crdt`-gated and do run. See `docs/active-work.md`. + ## 2. How we work (the part that must not drift) The user is expert and reviews deeply — they falsify framings and find From 40709fafb93a199ae8a65e264eaad1a45aeebbf0 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 17:17:23 -0400 Subject: [PATCH 02/18] docs: measure the CI coverage gap and record the vterm as-framed audit The lane opened in the previous commit was scoped to the Vterm Stage 3 acceptance. Measuring it properly shows the problem is much larger and not vterm-specific. Comparing cargo test --list under CI's exact flags against the same flags plus crdt: 3,024 versus 3,288. 264 tests are dark in CI, and the single worst line is the library itself at 177 -- cargo test --lib --features crdt is a required local gate that CI has never run. Ten suites run zero or one test, including gpu_initial_target (#148's entire acceptance, 1 of 14), gpu_invocation (#141's, 1 of 14), and a37, the Stage 3 real-daemon/real-PTY/real-wgpu path that #135 built precisely because a decoded-message fixture would prove none of the three fit together. The lane now carries the per-target table, the verified flag combination for the fix, a two-part fix shape (a crdt leg on the test job, plus the GPU-requiring suites onto the existing gpu-render job that already has lavapipe), and an explicit instruction to sort deliberate exclusions from accidental ones first -- some of the 264 are perf suites that are ignored by default and belong to their own jobs, while m10_10_perf has no ignore attribute and no job naming it. docs/vterm-framing.md gains an as-framed audit section. The arc is structurally complete and every test named in the Stage 2 verification map exists, but criterion 22's "without thrash" clause was never pinned anywhere -- the word appears nowhere in src or tests -- and that clause describes exactly the defect #166 fixed. Of the nine Stage 3 tests, only three drive a real daemon, so the six that construct EditorState directly could never see a dispatcher-loop defect; a31 passes on the broken tree for that reason. Four of the nine, including a37 and Stage 3 review round 1's own presence regression guard, do not run in CI at all. The section also records what was not audited: section 11's blanket claim about deferral safety covers roughly twenty items and none were spot-checked. docs/gpu-terminal-input-framing.md scores bet B2 true now that the reporter has confirmed typing works, and retracts Q#GT5. The bash fixture behind it does not reproduce in real use and was almost certainly measuring its own timing rather than a product behaviour; it is marked retracted rather than deleted so nobody re-derives it from an earlier revision. docs/agent-handoff.md section 5 gains the lesson the confirmation cost: a daemon-side fix is not deployed until the daemon is restarted from a tree containing it, and rebuilding a binary does nothing to a running process. No code changes. --- docs/active-work.md | 82 +++++++++++++++++++++--------- docs/agent-handoff.md | 9 ++++ docs/gpu-terminal-input-framing.md | 26 +++++++--- docs/vterm-framing.md | 42 +++++++++++++++ 4 files changed, 129 insertions(+), 30 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 5624d24..5f2f142 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -227,32 +227,68 @@ If it does not, stop and repair the remote/fetch configuration. buffer a directory should resolve *to*, and `pmacs .` should route into it rather than growing a second directory surface. -## Stage 3 acceptance is dark in CI — NEEDS A LANE +## The CRDT half of the test corpus is dark in CI — NEEDS A LANE -- **No branch, no framing yet.** Found while gating #166; deliberately kept - out of it so a CI change would not arrive after review approval. -- `.github/workflows/ci.yml` **never enables the `crdt` feature** (grep the - workflow directory: zero hits). Every `#[cfg(feature = "crdt")]` acceptance - test is therefore not merely skipped in CI — it is **not compiled**. -- That covers the whole Vterm Stage 3 real-path acceptance, including `a37` - (real daemon + real PTY + real wgpu), which has been dark since #135, and - the two tests #166 added beside it. -- The `gpu-render` job is the only one with lavapipe and - `PMACS_REQUIRE_GPU=1`, and it runs `cargo test -p pmacs-gpu`, which never - reaches the `pmacs` crate's acceptance suites. -- The shape of the fix is one step on the `gpu-render` job: - `cargo test --features crdt --test vterm_stage3_acceptance -- --test-threads=1`. - It needs its own lane rather than a drive-by because it would run `a37` - under lavapipe **for the first time**, and neither its timing budgets nor - its wgpu path have ever been exercised on that adapter or on macOS CI. -- Worth auditing at the same time: which *other* `crdt`-gated acceptance - suites are dark for the same reason. This is a coverage question about the - gate list itself, not about any one suite. +- **No branch, no framing yet.** Found while gating #166, then measured + properly during the vterm as-framed audit. Deliberately kept out of #166 so + a CI change would not arrive after review approval. +- **Root cause:** `.github/workflows/ci.yml` never enables the `crdt` feature + anywhere — zero hits across the workflow directory. The `test` job runs + `cargo test --all-targets --no-default-features --features luajit|lua54`. + 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: + + | dark | CI | full | target | + |---:|---:|---:|---| + | 177 | 1,832 | 2,009 | **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` | + | 8 | 0 | 8 | `m10_11_acceptance` | + | 6 | 0 | 6 | `auto_pair_crdt_acceptance` | + | 6 | 0 | 6 | `m10_2_perf` | + | 4 | 5 | 9 | `vterm_stage3_acceptance` | + | 4 | 0 | 4 | `m10_10_perf` | + | 3 | 0 | 3 | `compile_mode_crdt_acceptance` | + | 2 | 22 | 24 | `theme_faces_acceptance` | + | 2 | 0 | 2 | `m11_5_semantic_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 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. +- **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 + built specifically because "a decoded-message fixture would prove none of + the three fit together". +- **Sort deliberate from accidental before proposing a fix.** Some of the 264 + are perf suites that are `#[ignore]`d by default and belong to their own + jobs (`m10_2_perf` 6, `m10_11_perf` 1). `m10_10_perf` has **no** `#[ignore]` + and no CI job naming it, so it looks accidental. This classification is not + finished and is the lane's first task. +- **Fix shape, two parts** (the flag combination is verified to work: + `--no-default-features --features luajit,crdt` lists 10 vterm Stage 1 tests + versus 9 without): + 1. a `crdt` leg on the `test` job for the non-GPU suites and the library; + 2. the GPU-requiring `crdt` suites onto the existing `gpu-render` job, which + already has lavapipe and `PMACS_REQUIRE_GPU=1` — + `vterm_stage3_acceptance`, `gpu_invocation_acceptance`, + `gpu_initial_target_acceptance`, `gpu_font_acceptance`. +- **Expect first-run failures, and budget for them.** These would execute in + CI for the first time ever: real PTY timing on CI runners, wgpu under + lavapipe, and daemon-socket tests at unfamiliar concurrency. Start + ubuntu-only and decide about macOS from evidence. A red first run is the + lane working, not the lane failing. - Mitigating fact, verified rather than assumed: #166's three unit pins are - **not** `crdt`-gated and do run under CI's exact flags - (`--no-default-features --features luajit|lua54`), including the + **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. - The regression protection is live; the real-daemon evidence is local-only. ## Bottom-panel lane (window placement + side windows) — Stage 1 IN REVIEW diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index e28911f..165f06a 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -745,6 +745,15 @@ final variant — its own round-trip cannot detect a discriminant shift. ## 5. Hard-won ops lessons +- **A daemon-side fix is not deployed until the daemon is restarted from a + tree that contains it.** #166's reporter rebuilt and saw no change: the + running daemon had been started from a shared checkout still on a pre-fix + branch, and `pmacs --gpu` attaches to whatever process already owns the + socket. Rebuilding a binary does nothing to a running process. When + validating a daemon-side fix by hand, check the running process's binary + path and start time against the tree you think you fixed — + `ps -eo pid,lstart,args | grep '[p]macs --daemon'` — before concluding the + fix failed. - **Two operations that must be alternatives are not made alternatives by being adjacent.** The dispatcher applied its grid and semantic terminal-layout syncs to every attached frontend; a semantic session diff --git a/docs/gpu-terminal-input-framing.md b/docs/gpu-terminal-input-framing.md index 0bbccef..aa1bea4 100644 --- a/docs/gpu-terminal-input-framing.md +++ b/docs/gpu-terminal-input-framing.md @@ -81,7 +81,11 @@ character: | | frames for a static screen | typed `Z` ever visible at the prompt | |---|---|---| | `main` today | **730** in a 20 s window | **no** | -| with the guard | **2** | (see Q#GT5 — a separate question) | +| with the fix | **2** | yes | + +Bet B2 is **scored TRUE**: with the fix deployed, the reporter confirmed +typing into a GPU terminal works. The earlier caveat here pointed at Q#GT5, +which is now retracted — see "Deferred (named)". The TUI is unaffected: a grid session has no semantic terminal declaration, so only one arm ever runs for it. This is a **frontend-kind** defect, which is @@ -281,11 +285,12 @@ change. Stays v20. snapshot that signals the switch-away). Hence the split in Q#GT1. Recorded rather than deleted: the failure mode is one a reviewer or a future simplification will re-propose. -- **B2.** The user's reported symptom is this defect. *Partially scored: the - storm is proven and GUI-only, and its shape (line editor unusable, output - still flowing) matches the report. Not fully scored until the user, or an - acceptance running the **user's own shell**, confirms typing works after the - fix. Q#GT5 is the reason this bet is stated rather than assumed.* +- **B2 — SCORED TRUE 2026-07-25.** "The user's reported symptom is this + defect." Confirmed in real use after the fix was deployed: typing into a GPU + terminal works. The confirmation needed a daemon **restart** built from a + tree containing the fix — the first attempt reported no change because a + pre-fix daemon still owned the socket, which is worth remembering whenever a + daemon-side fix is being validated by hand. - **B3.** No other pair of per-frontend-kind daemon operations is applied as siblings rather than alternatives. *Scored by an explicit audit of the dispatcher's per-frontend loop during implementation — this defect's shape @@ -294,7 +299,14 @@ change. Stays v20. ## Deferred (named) -- Interactive-shell echo on a raw-mode PTY (Q#GT5) — its own scout. +- ~~Interactive-shell echo on a raw-mode PTY (Q#GT5)~~ — **RETRACTED + 2026-07-25.** The observation behind it (a `bash --norc -i` fixture not + echoing typed characters) does not reproduce in real use: with the fix + deployed, typing into a GPU terminal echoes normally. The fixture was almost + certainly measuring its own timing — polling a published screen snapshot + before readline had finished initialising — not a product behaviour. Recorded + as retracted rather than deleted so nobody re-derives it from the framing's + earlier revision and spends a scout on it. - **A geometry change appears to clear the visible screen.** Observed while building acceptance 4: after the probe's deliberate 25×92 → 20×71 resize, the next frame's visible grid is entirely blank even though the content diff --git a/docs/vterm-framing.md b/docs/vterm-framing.md index b1124c0..b86bcd0 100644 --- a/docs/vterm-framing.md +++ b/docs/vterm-framing.md @@ -1674,6 +1674,48 @@ GPU assertions remain in `pmacs-protocol` and `pmacs-gpu` respectively. - **37:** one real-daemon/real-PTY/headless-wgpu acceptance path; it is not replaced by a decoded-message fixture. +### 0.12 As-framed audit, 2026-07-25 (after #166) + +Prompted by a GPU terminal input defect that shipped in Stage 3 and was fixed +in #166. The arc is structurally complete — all 37 criteria have +implementations, and every test named in the Stage 2 verification map exists — +but the audit found two gaps worth recording against the criteria themselves. + +**Criterion 22's "without thrash" was never pinned.** The criterion reads +"unchanged, zero, passive, and failed resize cases preserve prior geometry +*without thrash*". The word appears nowhere in `src/` or `tests/`. The suite +pinned the four enumerated single-arm cases and never the cross-arm +interaction — which is exactly where the thrash lived: the daemon applied +both the grid and the semantic terminal-layout sync to every attached +frontend, so a semantic session's PTY was resized twice per tick forever. +Criterion 31's "only the exact durable controller changes PTY geometry" was +violated in the same event, in spirit rather than letter: the controller was +the right frontend, but the geometry came from the grid projection. #166 adds +the settle pins; the gap was open from #135 (2026-07-22) until then. + +**Why the Stage 3 suite could not see it.** Of its nine tests, only three +drive a real daemon; the other six construct `EditorState` directly and never +execute the dispatcher loop where the defect lived. `a31`, which is about two +semantic frontends sharing one session, therefore passes on the broken tree. +The same structural blindness explains why `bottom_panel_stage1_acceptance` +was unaffected. A criterion about *dispatcher* behavior needs a test that +runs the dispatcher. + +**Four of the nine Stage 3 tests do not run in CI at all**, because they are +`#[cfg(feature = "crdt")]` and the workflow never enables that feature: +`a37`, the two added by #166, and +`terminal_mode_keeps_reporting_presence_so_peers_drop_the_stale_caret` — which +is Stage 3 review round 1's own regression guard. Stage 1's +`read_only_empty_crdt_bootstrap_is_immutable_against_remote_content`, the CRDT +half of criterion 14, is dark for the same reason. Stage 2 is fully covered +(6/6). This is not a vterm problem: 264 tests workspace-wide are dark, +including 177 in the library. It has its own lane in `docs/active-work.md`. + +**Not audited:** §11's blanket claim that "deferral means graceful ignore or +documented absence, never escape leakage, panic, unbounded allocation, or +child leak". That covers roughly twenty deferred items and none were +spot-checked. It remains an unproven claim rather than a known gap. + ## 10. Gates and bite verification Every PR runs the standing full gates from `AGENTS.md`, sequentially, plus its From ea0632412e07dfd428c2c5a5236da89831109b8e Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 17:39:27 -0400 Subject: [PATCH 03/18] docs: record dired Stage 1 (#165) as landed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #165's own commits could not update the handoff snapshot to name the merge that contains them, so the protocol obligation lands here. - `docs/agent-handoff.md`: absorb the dired lane into §1, replacing the placeholder that promised exactly this. The bullet carries Stage 1's durable substrate facts — why the tolerant `read_dir` had to be Rust, why exposing the core normalizer beat mirroring it in Lua, the fixed-width `_layout` contract Stage 3 reads offsets from, the ambient-action buffer guard, treating a failure as the answer instead of probing, the per-entry error cap, the first mode-scoped keymap and the pre-existing test it broke, and the dedication a descent does not carry. Refresh the head-of-`main` anchor and the last-updated line. - `docs/agent-handoff.md` §5: two ops lessons that cost real time. A fix must be committed before it is bitten, because `scripts/bite` restores by `git checkout --` and reverts to HEAD; a CONFLICTING PR runs no CI at all, because `pull_request` workflows build a merge ref GitHub does not create while the branch conflicts, and nothing reports the absence. - `docs/active-work.md`: remove the merged lane per update-protocol rule 4 and summarize it under "Closed since the last snapshot", keeping the two forward items Stage 2 needs (the rename rebind is first-match-only over a raw path, and Q#DR5's seam is the main-thread drain). Refresh the canonical base. Flag the two lane headers that still call a merged PR "IN REVIEW" — #161 and #166 — rather than editing lanes another thread owns. - `COHERENCE.md`: #165 is no longer a PR. Per §25 the audited claims this work changed were updated when it landed; this corrects their tense in seven places and the two prose lines that still asserted dired was in flight. - `docs/dired-framing.md`: status line to MERGED, and state plainly that Stages 2 and 3 each still need their own framing. Docs only; no code, no gate-relevant change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0126d2sikA6jZpFin3rtLCSK --- COHERENCE.md | 21 +++-- docs/active-work.md | 201 ++++++++++-------------------------------- docs/agent-handoff.md | 113 +++++++++++++++++++++--- docs/dired-framing.md | 4 +- 4 files changed, 164 insertions(+), 175 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index 95761b2..1cfba71 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -120,7 +120,7 @@ asymmetry**, and **per-arc coherence debt**. Coherence-shaped work already in flight at audit time: find-file / dired Stage 0 (`C-x C-f`, merged #162, `docs/dired-framing.md`) and its -Stage 1 directory view (PR #165), bottom panel Stage 1 (merged #155), +Stage 1 directory view (merged #165), bottom panel Stage 1 (merged #155), multi-root LSP affinity (merged #161), the config registry foundation (merged #127). @@ -195,7 +195,7 @@ working, unreachable capability: time; a complete 1,384-line dired existed only as a frozen test fixture (`tests/fixtures/pmacs-dired/init.lua`). **Fixed:** dired Stage 0 opens a path (`C-x C-f`, merged #162) and Stage 1 ships the - browsing view as a builtin (`C-x d` / `C-x C-j`, PR #165). The fixture + browsing view as a builtin (`C-x d` / `C-x C-j`, merged #165). The fixture stays frozen — its `install_local` + `require` routing *is* the M8 package-universality proof (Q#DR1) — and shrinking it is scheduled after Stage 3. @@ -363,11 +363,11 @@ 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 (PR #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 | **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 | | 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 | -| 7 | Find symbol / file | **File: fixed (open by path merged #162; browsing PR #165). Symbol: works but undiscoverable** | No find-file/dired/picker existed at audit. Now `C-x C-f` opens a known path and `C-x d` / `C-x C-j` browse (flat listing, `dired` mode keymap); `M-.`/`M-?`/`C-c o` still bound but advertised nowhere and server-gated; no workspace-symbol command; `pmacs.index.*` has no UI | +| 7 | Find symbol / file | **File: fixed (open by path merged #162; browsing #165). Symbol: works but undiscoverable** | No find-file/dired/picker existed at audit. Now `C-x C-f` opens a known path and `C-x d` / `C-x C-j` browse (flat listing, `dired` mode keymap); `M-.`/`M-?`/`C-c o` still bound but advertised nowhere and server-gated; no workspace-symbol command; `pmacs.index.*` has no UI | | 8 | Open terminal | **Works but undiscoverable** | Full PTY with scrollback + modeline segment — reachable only as `M-x terminal`, no keybinding. *Was broken outright on the GPU frontend until the double terminal-layout sync was fixed: the child took a `SIGWINCH` storm at tick cadence, so typing into it was impossible while output still flowed.* | | 9 | Build / test | **Partial** | `M-x compile.run` works, defaults cwd to detected project root, parses Rust `-->` errors — but no keybinding, an **empty first prompt** (`initial = last and last.cmdline or ""`, `builtin/runtime/compile.lua:1134-1138`), and no `cargo build`/`cargo test` suggestion despite `ProjectKind::Cargo` existing (`src/project.rs:77`) | | 10 | Inspect error | **Partial (good once reached)** | `E:n W:n` modeline counts, underlines, `M-g n/p` + ``C-x ` `` walking a unified compile/grep/diag source, message echo, `RET` visits. Gated entirely on step 6 or 9 succeeding first | @@ -460,7 +460,7 @@ level is the one missing. Audited level-by-level: **Beginner** (should see: files, buffers, search, diagnostics, terminal, build actions, menus, missing-tool guidance): -- files ✓ since #162 / PR #165 (`C-x C-f` opens a path, `C-x d` browses; +- files ✓ since #162 / #165 (`C-x C-f` opens a path, `C-x d` browses; neither is advertised anywhere but the keymap) · buffers ✓ (`C-x b`, `*buffer-list*`) · search ✓ (`C-s`/`C-r`/`C-M-s`; project.search is M-x-only) · diagnostics ✓ once a server runs · terminal ✓ but @@ -1183,7 +1183,7 @@ Primitive-by-primitive against the list above: hierarchy, package dependency graph, worker trees, git status) will each need it; building it once *before* dired's directory view and the workers tree harden their own conventions is exactly this - section's point. Dired Stage 1 (PR #165) landed **without** inventing + section's point. Dired Stage 1 (merged #165) landed **without** inventing one: its listing is flat (Emacs parity), and the recursive in-buffer case — `i` insert-subdirectory — is a named deferral in `docs/dired-framing.md` §13, which is where a shared tree primitive @@ -1340,7 +1340,8 @@ greets a new user says nothing (`EditorCore::new` sets an empty status). Note the dependency: five of the ten onboarding steps above currently -lead somewhere broken or invisible (find a file — in flight; inspect a +lead somewhere broken or invisible (find a file — the mechanism is fixed +since #162/#165 but is advertised nowhere except the keymap; inspect a diagnostic — silent-failure risk; view workers — undiscoverable; setting provenance — unanswerable). Onboarding is correctly sequenced *after* the P1/P4 fixes, but the cheap floor — a welcome buffer in @@ -1404,7 +1405,7 @@ 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, PR #165 browsing); surfacing the +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 @@ -1474,7 +1475,9 @@ 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. Rides alongside the in-flight dired arc. + 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. 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 c934c30..1266e40 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -5,6 +5,14 @@ landed on `main`. Read it after `docs/agent-handoff.md`. Remove completed entries when their PR merges; do not let this become a second permanent backlog. +**Two lane headers below are stale on purpose**, pending the docs updates +their own lanes owe: multi-root LSP affinity **#161 has merged** (the +Lean 4 lane still says IN REVIEW; its continuation is PR #167) and GPU +terminal input **#166 has merged** (its lane still says IN REVIEW; PR +#168 records it). Trust the canonical-base line below over a lane header: +if a PR number appears in `git log --first-parent githubsucks/main`, it +has landed regardless of what its lane says. + ## Repository authority - Canonical development URL: @@ -14,11 +22,14 @@ 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` @ `8c86d34` (the dired framing #164 atop find-file - #162, COHERENCE.md #163, Lean 4 Stage 1 #160, the minimap blank-slab fix - #159, bottom-panel Stage 1 #155, the inline-math re-scout #154, the vterm - PTY-flake fix #153, and the GPU initial-target doc refresh #152; protocol - v20). + `githubsucks/main` @ `c8ec8f3` (dired Stage 1 #165 atop GPU terminal + input #166, multi-root LSP affinity #161, the dired framing #164, + find-file #162, COHERENCE.md #163, Lean 4 Stage 1 #160, the minimap + blank-slab fix #159, bottom-panel Stage 1 #155, the inline-math re-scout + #154, the vterm PTY-flake fix #153, and the GPU initial-target doc + refresh #152; protocol v20). **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 `d3fa632` and lagged badly. On the current destination, `origin` names the canonical URL. This difference is why all recovery begins by @@ -189,154 +200,6 @@ If it does not, stop and repair the remote/fetch configuration. suites**; `git diff --check` clean. The sweep needs an isolated `XDG_CONFIG_HOME` and `-- --skip basedpyright`. -## Dired lane — Stage 0 MERGED; Stage 1 IN REVIEW (PR #165) - -- 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 - implementation notes (S1-1…S1-9). Stages 2 (marks and operations) and 3 - (wdired) each get their own detailed framing after the prior stage lands. -- **Stage 0 (`C-x C-f` find-file) MERGED as #162** (`main` @ `2af1ab3`, - 2026-07-25, one review round, 12/12 CI green). Durable facts moved to - `docs/agent-handoff.md` §1 per rule 3 below. -- **Stage 1 branch: `githubsucks/dired-stage1`**, worktree - `../pmacs-dired-stage1`, based on `githubsucks/main` @ `8c86d34` (the - framing merge #164). **A fresh cut, not a rebase:** the older `dired` - branch (`ffdd642`, worktree `../pmacs-dired-arc`) was based on the - superseded `0827dd1` and carried only the framing content #164 already - put on `main`, so merging it would have reconciled two histories of one - document. It is left untouched and carries nothing unmerged. -- **Stage 1 implemented; no wire change (protocol stays v20).** What - landed on the branch: - - `builtin/runtime/dired.lua`: one buffer per directory named - `*dired:*` with the handle-table ownership check; - read-only intercept + `set_round_trip_input`; the `dired` major mode - and its mode-scoped keymap (`RET`/`f`, `^`, `n`/`p`, `g`, `q`, `s`); - basename cursor re-seating across every wholesale repaint; - `display_file` for file visits and same-window reuse for directory - descent; `C-x d` / `C-x C-j`; the `dired.kill-when-opening` setting. - Loaded after `window.lua`. - - `src/fs.rs`: `ReadDirTolerance`, `FsDirEntryError`, `FsDirListing`, - and one walk that either fails on a per-entry condition or records it - (Q#DR6). `src/async_runtime.rs` carries the listing in - `ReplyKind::ReadDir` / `JobResult::ReadDir`; `src/lua_bindings/mod.rs` - keys the Lua result **shape** on `errors.is_some()`, so the bare array - the frozen M8.2 fixture consumes with `ipairs` is untouched; - `builtin/runtime/fs.lua` validates read-op opts and **rejects unknown - keys** (a typo'd `tolerant` used to degrade silently to fatal). - - `src/editor_core.rs` + `src/lua_bindings/mod.rs`: - `normalize_buffer_path` is `pub` and exposed as - `pmacs.path.canonicalize` — Q#DR2's preferred end state, so no Lua - mirror exists and Stage 2 owes no mirror removal. This makes B2 - ("tolerant `read_dir` is the only Rust change") false by one small - binding, deliberately. - - `tests/dired_acceptance.rs`: 22 tests over framing items 1–16, - dispatch-driven; item 17 is the m8_1/m8_2/m8_3 additivity gate. -- **The framing claim the substrate falsified (S1-2):** R2-3 expected a - dedicated dired panel to carry its dedication across a descent. - `display_buffer` never replaces the buffer in a slot dedicated to - another one — it discards every side-specific parameter and falls back - to the document window (Q#BP3 2.iii), and the exact-window arm errors. - Dired does not unpin the user's panel; both arms are pinned. -- **The vacuity the bites found (S1-3):** acceptance 3c cannot pin the - descent *routing*. Dired holds focus in its own panel, so a raw - `switch_buffer` lands in the same window and every 3c assertion holds - either way. Dedication is the only discriminator, so the - dedicated-panel test is the real pin — and the vacuity is documented at - the assertion rather than relabelled. -- **The pre-existing test dired's first mode-scoped binding broke - (S1-4):** `describe_key_identifies_every_default_binding` asserted every - binding in the stack resolves through `describe.key` context-free, which - held only while the modes table was empty. It now sets the effective - context per binding and explicitly *clears* the mode for global ones, - because a leaked mode legitimately shadows a global chord of the same - name (dired's `RET` shadows `edit.newline-and-indent`). -- Durable substrate facts, independent of this arc: - - `pmacs.buffer.kill` (not `remove`) redirects windows off a doomed - buffer before removal, so `kill-when-opening` kills **after** the - replacement is displayed. - - Interactive origin does **not** survive an await: work resumed in - `tick_async` sees no `InteractiveCommandOrigin`, so `pmacs.window.*` - acts for the *ambient* active frontend (S1-9). - - Kinds are lstat-based in both `read_dir` and `stat`, so nothing in an - entry says whether a symlink points at a directory; `RET` probes by - trying to list it (S1-8). - - A path-backed buffer's *name* is its full path, not its basename — - worth knowing before writing any name assertion. - - `C-x d` takes **no** completion source on purpose (S1-5): with one, - RET on an empty field opens whatever sorts first, and - RET-on-where-you-are is the gesture the binding exists for. The field - is prefilled instead. -- **Bite verification:** 15 claims, each mutated in place and required to - fail the test that names it. `dired.lua` is new, so `scripts/bite`'s - file swap does not apply; every mutation was applied and reverted with - `git checkout --`. One came back VACUOUS and is recorded above. -- **Review round 1 addressed** (framing rev 7, S1-10…S1-12). Three - behavioral fixes, each bite-verified: `dired.revert`'s re-seat is - guarded on the active buffer (an ambient `move_to_line` after an await - moved an unrelated buffer's cursor — the buffer-level instance of - S1-9); `fmt_size` keeps the column width past ten digits, because - `_layout` is a contract Stage 3 is planned against; and the symlink - descent dropped its probe, since `open_directory`'s - changed-nothing-on-failure invariant *is* the probe (it was listing the - target directory twice). Plus a consecutive-`readdir`-error cap, because - **nothing cancels a dired listing** — it carries no supersede key, so - cancellation was never the backstop the tolerant loop implicitly relied - on. Naming/comment findings taken as-is. - - Durable process lesson, hit twice now: a mutation-bite helper restores - with `git checkout --`, which reverts to **HEAD** — so a fix must be - committed *before* it is bitten. Round 1's fixes were briefly wiped by - exactly that. -- **Canonical main integrated twice** — at `46a1b8f` (multi-root LSP - affinity #161) and again at `b889873` (GPU terminal input #166), both - merged rather than rebased per the #135/#137 precedent so the review - anchors stay addressable. Each conflict was a single doc hunk resolved - as the union: this lane owns COHERENCE's journey step 7 file half, #161 - owns the in-flight list, #166 owns step 8's GPU-terminal addendum. - Three things worth carrying: - - **A conflicting PR silently stops running CI.** GitHub builds - `pull_request` runs against the merge ref, which does not exist while - the PR conflicts, so no run is created and nothing reports a - failure — the checks list simply stays as it was. Three pushes to - this branch produced no CI at all before the cause was found. Watch - `mergeable` on a long-lived lane, not just the check list. - - #161's own COHERENCE finding **falsified a claim in this lane's - module doc**: `pmacs.error` is never defined in production, so an - uncaught raise inside a `pmacs.async` coroutine does not reach - `*errors*` as the comment said. It reaches a bare `error()` inside - `pmacs._async.tick()`, whose result `tick_async` discards with - `let _ =` — i.e. nowhere. That makes dired's per-coroutine `pcall` + - `set_status` load-bearing rather than tidy, and the comment now says - so. - - **A lane in review against a fast-moving `main` needs its gates rerun - per integration, not per push.** Main advanced twice inside this - review round, and the second time landed while the first - integration's sweep was still running. The numbers below describe the - twice-merged tree. -- Verification on the twice-merged tree (`main` @ `b889873`): - `cargo fmt --check` clean; strict workspace Clippy clean; **1,832 - default + 2,009 CRDT** library tests; dired acceptance **25 default + - 25 CRDT**; m8_1 10 / m8_2 15 / m8_3 32 unchanged; multi-root 13 and - vterm Stage 3 5 (both suites main added, green under this lane's - `mod.rs` and `editor.rs` changes); M4 121; required GPU 155; - **isolated-`XDG_CONFIG_HOME` workspace sweep 3,205 passed across 93 - suites, zero failures**; `git diff --check` clean. The sweep needs the - isolated config for the reason recorded in the bottom-panel lane - below. -- Coherence (framing §0.5, required since #163): serves `COHERENCE.md` §20 - Priority 1, which names this work explicitly; journey step 7's file half - goes from no surface to a surface; **adds no interaction island** — keys - are a mode-scoped keymap, and wdired will be a mode swap; adopts - `pmacs.config` for `dired.kill-when-opening`; inherits §9's - worker-attribution gap for its `read_dir` jobs without worsening it. The - audited claims this changes are updated in `COHERENCE.md` itself, per its - §25. -- **Boundary with the Journey Stage 1 arc** (`COHERENCE.md` §20 arc-cut - 1): CLI directory-argument handling (`pmacs .` exits 1) belongs there, - not here — Stage 1 does **not** fix it. The two meet at - `resolve_target_buffer`; dired supplies the buffer a directory should - resolve *to*, and `pmacs .` should route into it rather than growing a - second directory surface. - ## GPU terminal input lane — IN REVIEW - Portable branch: `githubsucks/gpu-terminal-input`, worktree @@ -593,6 +456,38 @@ git worktree add --track \ ## Closed since the last snapshot +- **Dired Stage 1 (the directory view) — MERGED as #165** (`main` @ + `c8ec8f3`, 2026-07-25, after one review round). pmacs has a directory + surface: `C-x d` / `C-x C-j`, one read-only buffer per directory named + `*dired:*`, a `dired` major mode carrying + `RET`/`f`, `^`, `n`/`p`, `g`, `q`, `s`. No wire change (v20). The Rust is + two things — a per-entry-tolerant `read_dir` (Q#DR6), which had to be + Rust because `read_dir_blocking` fails a whole listing on any of five + per-entry conditions and a tolerant wrapper cannot be written in Lua at + all, and `normalize_buffer_path` going `pub` as + `pmacs.path.canonicalize` (Q#DR2's preferred end state, so no Lua mirror + exists and Stage 2 owes no mirror removal). The frozen m8_1/m8_2/m8_3 + counts are unchanged, which is the additivity gate. 15 claims + bite-verified; one came back VACUOUS (acceptance 3c cannot pin descent + routing — dired holds focus in its own panel, so dedication is the only + discriminator) and is documented at the assertion rather than + relabelled. Its branch (`dired-stage1`) and worktree + (`../pmacs-dired-stage1`) are done; the abandoned `dired` branch + (`ffdd642`, `../pmacs-dired-arc`) was superseded by a fresh cut and + carries nothing unmerged. **Stage 2 (marks and operations) and Stage 3 + (wdired) each still need their own framing**, and the frozen fixture + shrinks after Stage 3. Durable substrate facts and both new ops lessons + live in `docs/agent-handoff.md` §§1/5; the implementation notes are + `docs/dired-framing.md` §0, S1-1…S1-12. Two named forward items for + Stage 2: `apply_resource_op`'s rename rebind is exact-PathBuf-equality, + first-match-only, looked up with the raw path while stored paths are + normalized — so a directory rename strands every buffer under it, and + `pmacs.fs.rename` has zero production callers, so it can be fixed at + the primitive; and Q#DR5's seam is the main-thread drain + `AsyncRuntime::tick`, not `_take_result`, where rename settles as an + undifferentiated `ReplyKind::FsUnit` and so must be keyed on + `JobKind::FsRename`. + - **GPU initial target — MERGED as #148** (`main` @ `0dd16a5`, 2026-07-24, after two review rounds). `pmacs --gpu [--socket …] FILE` opens a target before the GPU window appears. Protocol bumped 19 → 20: a semantic-session diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index bb5677a..f6900cc 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -1,7 +1,9 @@ # Agent handoff — cross-machine continuity -**Last updated: 2026-07-25, after find-file (#162) landed — the dired -arc's Stage 0 — following COHERENCE.md (#163), Lean 4 Stage 1 (#160), the +**Last updated: 2026-07-25, after dired Stage 1 (#165) landed — the +directory view — following GPU terminal input (#166), multi-root LSP +affinity (#161), the dired framing (#164), find-file (#162) — the dired +arc's Stage 0 — COHERENCE.md (#163), Lean 4 Stage 1 (#160), the minimap blank-slab fix (#159), bottom-panel Stage 1 (#155), the inline-math re-scout (#154), the vterm PTY-flake fix (#153), and the GPU initial-target doc refresh (#152); and before that GPU @@ -26,11 +28,13 @@ commands, read `docs/active-work.md` immediately after this file. ## 1. Where the project stands (2026-07-25) -- `main` @ `2af1ab3` (find-file #162 atop COHERENCE.md #163, Lean 4 Stage 1 - #160, minimap blank-slab #159, bottom-panel Stage 1 #155, inline-math - re-scout #154, vterm PTY-flake #153, and doc refresh #152). Protocol - unchanged at **v20**. The bullets below describe the arcs in their own - terms; this line is the head-of-`main` anchor. +- `main` @ `c8ec8f3` (dired Stage 1 #165 atop GPU terminal input #166, + multi-root LSP affinity #161, the dired framing #164, find-file #162, + COHERENCE.md #163, Lean 4 Stage 1 #160, minimap blank-slab #159, + bottom-panel Stage 1 #155, inline-math re-scout #154, vterm PTY-flake + #153, and doc refresh #152). Protocol unchanged at **v20**. The bullets + below describe the arcs in their own terms; this line is the + head-of-`main` anchor. - **`COHERENCE.md` is now required reading and a required framing input — #163.** It carries the product-coherence thesis, an audited scorecard, per-concern gaps, and §20's priority order, and it is the @@ -70,11 +74,78 @@ 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. +- **dired Stage 1 — the directory view — LANDED — #165** + (`docs/dired-framing.md` §0, S1-1…S1-12; merge `c8ec8f3`; one review + round). pmacs now has a directory surface: `C-x d` / `C-x C-j` open a + read-only listing, one buffer per directory named + `*dired:*`, with a `dired` major mode whose + mode-scoped keymap carries `RET`/`f`, `^`, `n`/`p`, `g`, `q`, `s`. + Protocol unchanged at **v20**. **Stage 2 (marks and operations) and + Stage 3 (wdired) each still need their own framing**; the frozen + fixture shrinks after Stage 3. + - **The Rust is confined to two things**: a per-entry-tolerant + `read_dir` (`ReadDirTolerance {Fatal, PerEntry}` → + `FsDirListing {entries, errors}`), because `read_dir_blocking` fails + a whole listing on any of five per-entry conditions and the tolerant + wrapper its own module doc delegates to package authors **cannot be + written in Lua** (one error value, no partial vec); and + `editor_core::normalize_buffer_path` becoming `pub`, exposed as + `pmacs.path.canonicalize`. Only non-UTF-8 **names** stay fatal — + byte-preserving paths would be needed. The Lua result **shape** keys + on `errors.is_some()`, so the bare array the frozen M8.2 fixture + consumes with `ipairs` is untouched. + - **Exposing a core normalizer beat mirroring it in Lua.** A Lua mirror + would have been a second canonical form — the same class of bug as + the five tab-width constants (#137). Applies to any future Lua-side + path reckoning. + - **A fixed-width column must be fixed-width for every input.** The + exported `pmacs.dired._layout` (MARK 0, KIND 2, PERMS 3–12, SIZE 13, + MTIME 24, NAME 41) is the contract Stage 3 reads offsets from, and + `%10d` overflows at ≥10 GB, silently shifting every column right of + it. Sizes now fall back to a width-clamped magnitude (K/M/G/T/P/E). + - **An ambient action must be gated on the buffer it assumes.** A + revert's cursor re-seat settles a tick or more later, by which time + the user may have switched buffers; the paint names its buffer and is + safe, but seating is ambient. This is the buffer-level instance of + the rule below that interactive origin does not survive an await. + - **A failure IS an answer — don't probe first.** Kinds are lstat-based + in both `read_dir` and `stat`, so nothing in an entry says whether a + symlink points at a directory. `RET` tries to list it and treats the + failure as the answer; an explicit probe was a second full + `read_dir`, so a descent listed twice. + - **Unbounded per-entry error collection needs a cap when nothing + cancels the work.** A dired listing carries no supersede key, so + cancellation was never the backstop the tolerant loop implicitly + relied on (`READDIR_MAX_CONSECUTIVE_ENTRY_ERRORS = 1024`). + - **This is the first builtin with mode-scoped keys** (#129's first + non-detection consumer), which broke the pre-existing + `describe_key_identifies_every_default_binding`: it asserted every + binding resolves through `describe.key` context-free, which held only + while the modes table was empty. It now sets the effective context + per binding and explicitly **clears** the mode for global ones, + because a leaked mode legitimately shadows a global chord of the same + name (dired's `RET` shadows `edit.newline-and-indent`), plus a floor + assertion that at least one mode-scoped binding exists. + - **A dedicated panel does not carry its dedication across a descent** + — the framing expected it to. `display_buffer` never replaces the + buffer in a slot dedicated to another one; it discards every + side-specific parameter and falls back to the document window (Q#BP3 + 2.iii), and the exact-window arm errors. Dired does not unpin the + user's panel; both arms are pinned. + - Smaller facts worth knowing before touching this code: a path-backed + buffer's **name is its full path**, not its basename, which matters + for any name assertion; `pmacs.buffer.kill` (not `remove`) redirects + windows off a doomed buffer first, so `dired.kill-when-opening` kills + **after** the replacement is displayed; ownership is checked against + the handle table only, never the buffer name; and `C-x d` takes **no** + completion source on purpose (with one, `RET` on an empty field opens + whatever sorts first, and RET-where-you-are is the gesture the binding + exists for — the field is prefilled instead). + - Verification at merge: 1,832 default + 2,009 CRDT library tests; + dired acceptance 25 + 25 CRDT; the frozen m8_1 10 / m8_2 15 / m8_3 32 + unchanged, which is the additivity gate for the `read_dir` change; M4 + 121; required GPU 155; isolated-`XDG_CONFIG_HOME` workspace sweep + 3,205 across 93 suites. 15 claims bite-verified. - **GPU initial target LANDED — #148** (`docs/gpu-initial-target-framing.md` rev 3; merge `0dd16a5`; two review rounds). `pmacs --gpu [--socket NAME|PATH] FILE` transports exact Unix path @@ -760,6 +831,24 @@ final variant — its own round-trip cannot detect a discriminant shift. trap-guarded one-file swap over read-only `git show`, with an inverted verdict (exit 0 iff the tests FAIL against the old version), making bite-verification machine-checkable. +- **A fix must be COMMITTED before it is bitten.** `scripts/bite` + restores by `git checkout --`, which reverts the file to **HEAD**, not + to the state it found — so any uncommitted work in a bitten file is + destroyed. A whole review round's fixes were wiped this way during + #165. Corollary for a NEW file: the swap-over-`git show` mode does not + apply at all, so its claims must be bitten by hand-editing, which makes + the commit-first rule load-bearing rather than hygienic. +- **A CONFLICTING PR silently runs no CI at all.** GitHub builds + `pull_request` workflow runs against the PR's **merge ref**, which it + does not create while the branch conflicts with its base. So pushes + land, the branch updates, no run is ever queued, and **nothing reports + the absence** — the checks list simply keeps showing the last + successful run, which reads as current. Three pushes to #165 produced + zero CI before the cause was found, and `gh pr checks` returns nothing + usable here. On any lane that lives through a moving `main`, check + `gh pr view --json mergeable,mergeStateStatus,headRefOid` and + confirm a run exists **for the current head sha**, not merely that a + recent run was green. - **Stacked PRs**: retarget the child to main BEFORE merging the parent — GitHub auto-closes a PR whose base branch is deleted and cannot reopen it (#104 → re-opened as #105). diff --git a/docs/dired-framing.md b/docs/dired-framing.md index ada853e..83e6f1a 100644 --- a/docs/dired-framing.md +++ b/docs/dired-framing.md @@ -1,7 +1,9 @@ # Dired — framing **Revision 7 — 2026-07-25. Status: APPROVED; Stage 0 MERGED as #162; -Stage 1 IN REVIEW as PR #165, review round 1 addressed.** +Stage 1 MERGED as #165 (`main` @ `c8ec8f3`, one review round). Stage 2 +(marks and operations) and Stage 3 (wdired) each still need their own +framing before implementation; the frozen fixture shrinks after Stage 3.** Rev 1 passed a ground-truth review; rev 2 fixed round 1's seven findings; rev 3 fixed round 2's six and was approved; rev 4 recorded what Stage 0's implementation falsified in the approved text (§0); rev 5 adds the From dea1d20e4eaea101020af3997e0016668de159c6 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 17:58:00 -0400 Subject: [PATCH 04/18] docs: refresh the canonical base for #158 The inline-math slice landed while this PR was open. Its own merge removed its ledger lane, so the stale-header note above still names exactly two; only the base anchor needed moving. --- 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 1266e40..d7a8c9d 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -22,8 +22,9 @@ has landed regardless of what its lane says. 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` @ `c8ec8f3` (dired Stage 1 #165 atop GPU terminal - input #166, multi-root LSP affinity #161, the dired framing #164, + `githubsucks/main` @ `5aa9044` (the inline-math slice #158 atop dired + Stage 1 #165, GPU terminal input #166, multi-root LSP affinity #161, + the dired framing #164, find-file #162, COHERENCE.md #163, Lean 4 Stage 1 #160, the minimap blank-slab fix #159, bottom-panel Stage 1 #155, the inline-math re-scout #154, the vterm PTY-flake fix #153, and the GPU initial-target doc From 2b42204693a0df3099e5a4cad0bd43951387abb9 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 21:10:16 -0400 Subject: [PATCH 05/18] docs: integrate #175 and align the recovery threshold with the base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #175 (bottom-panel Stage 2 framing) landed after this branch's last head and touches both shared docs, so the previous green run did not cover the combination. Merged cleanly this time — no conflict. Also fixes an inconsistency this PR introduced: the recovery check still accepted `d152120` while the canonical-base line above declared a newer commit. A threshold looser than the base it guards passes on a tree the rest of the file does not describe, so the two now move together and the text says why. --- docs/active-work.md | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index bfacee3..1ad3bdc 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -23,14 +23,14 @@ here too until #172 removed it — that is the update those two owe.) machine-local: `origin` may name this canonical URL, a release mirror, or something else, and therefore has no authority by name alone. - Canonical base at this snapshot: - `githubsucks/main` @ `ccf29e3` (the CRDT undo repro #157 atop 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). **Lanes below that name an older - base have not been re-based; derive their integration surface from - `git diff ..main`.** + `githubsucks/main` @ `c93f9ee` (the bottom-panel Stage 2 framing #175 + atop 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). + **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 `d3fa632` and lagged badly. On the current destination, `origin` names the canonical URL. This difference is why all recovery begins by @@ -64,7 +64,11 @@ 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 `c93f9ee` — the base named above — or a +newer intentional main. Keep this threshold and the canonical-base line in +step: a recovery check that accepts an older commit than the base it +declares canonical will pass on a tree the rest of this file does not +describe. If it does not, stop and repair the remote/fetch configuration. ## Lean 4 lane (Arc 8) — Stage 1 MERGED; Stage 2 IN REVIEW (PR #161) From dd581cd90ce1cc72c5d9a5f09caa3ca852b853b9 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 21:12:51 -0400 Subject: [PATCH 06/18] docs: frame the PTY terminate diagnostic (revision 4) A docs-only PR (#172) failed Test (macos-latest / luajit) on acc28_child_input_and_the_c_c_escape_work_unchanged_in_a_panel with "kill: EPERM: Operation not permitted" raised out of terminate. A docs diff cannot cause that, main was green at the PR's exact base, and three other PRs passed the same job. This framing reaches revision 4 after three review rounds, and what it proposes is much smaller than what it started with. Revisions 1 to 3 each proposed a tolerance rule -- treat some errno as success -- and each was unsound in the same way: they concluded something about a process from something that was not about that process. Revision 1 concluded from an errno alone, which says only that a syscall failed. Revision 2 concluded from the spawned leader while a PTY signal targets the tty's foreground process group, which diverges from the leader exactly when job control is in use. Revision 3 corrected EPERM but kept group-directed ESRCH, which proves only that the selected foreground group vanished, not that the leader exited. So no tolerance rule lands. The disposition is preserved exactly: every failing call still fails, with no state transition and no ledger arming. What lands is that the failure explains itself, recording the target source and value, the spawn-time pgid or leader pid, the errno, and the leader's real try_wait state as five separate facts. Every candidate fix is decidable from those together and none is decidable from the errno alone. Two claims are stated more narrowly than earlier revisions had them. Consulting try_wait reaps an exited child and caches its status, so this is not "strictly additive" -- it is "no disposition change", with an event-count test pinning that poll_one still emits exactly one exit event. And the test seam injects the kill attempt's result only, never the observation, so the real ChildHandle::try_wait runs against the real child; a stubbed observation would bypass the path under test. Parked with their reasons: all tolerance rules, terminate becoming idempotent for an already-reaped process (an independent fix answering a different failure), and signal_target's read-then-kill of tcgetpgrp, which is the most likely real fix site. The lane closes when this lands rather than waiting for the flake to recur; the next occurrence carries its own evidence under whoever's PR. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- docs/process-signal-tolerance-framing.md | 258 +++++++++++++++++++++++ 1 file changed, 258 insertions(+) create mode 100644 docs/process-signal-tolerance-framing.md diff --git a/docs/process-signal-tolerance-framing.md b/docs/process-signal-tolerance-framing.md new file mode 100644 index 0000000..1d3c45d --- /dev/null +++ b/docs/process-signal-tolerance-framing.md @@ -0,0 +1,258 @@ +# Framing — make the PTY terminate failure self-describing (diagnostic only) + +**Revision 4.** Status: awaiting review round 4. Lane: +`pty-terminate-eperm`, worktree `../pmacs-math-slice`, based on +`githubsucks/main` @ `ccf29e3`. + +**Diagnostic only. No disposition changes, no tolerance rules, no +behavioural fix.** Every rule this document proposed across revisions 1 +to 3 is parked (§5). The lane's entire deliverable is that the next +occurrence of the failure explains itself. + +## Revision history + +**Revision 3 → 4**, after review round 3 (two blocking, one major) and +its scope call. All accepted. + +- **Group-directed ESRCH was also unsafe**, for the same reason EPERM + was: it proves the selected *foreground group* vanished, not that the + leader exited. A job-control race — foreground job exits after + `tcgetpgrp` and before `kill`, shell alive and not yet reclaiming the + terminal — would have been reported as success with the leader never + signalled. Rev 3's acceptance 7 pinned that unsafe behaviour. **All + tolerance is parked** (§5). +- **Rev 3's Stage A implemented Stage B.** It declared itself + diagnostic-only, then listed tolerance and bookkeeping acceptances. + Removed. +- **Q#PS6 (already-reaped `terminate` is `Ok`) is parked separately.** + It is an independent behavioural fix answering a different failure; + under one-feature/one-PR it does not ride with instrumentation. +- **"Strictly additive / cannot regress behaviour" was overstated** and + is narrowed (Q#PD3). +- The injected-kill seam is restored as an explicit decision (Q#PD4). + +**Rounds 1–3, for the record.** Rev 1 classified on errno alone and +claimed a live owned child cannot yield EPERM — false. Rev 2 gated on +`try_wait`, which observes the leader while a PTY signal targets the +foreground group — unsound whenever those diverge, and it could not be +shown to fix the observed failure at all. Rev 3 corrected EPERM but left +ESRCH unsafe and mixed the stages. **Three consecutive designs were +wrong in the same direction: each tried to conclude something about a +process from something that was not about that process.** + + +## 0. Coherence impact (COHERENCE §20) + +- **Journey step 8, "Open a terminal"** (§2), teardown half. **No grade + change and no behavioural change** — this lane only improves what a + failure reports. +- **Serves §9 (worker model), failure attribution**, in its most literal + sense: an error that names only an errno cannot be attributed. +- **Interaction islands: none. Config registry: not adopted. + Background-work attribution: unchanged.** +- **No audited claim in COHERENCE.md changes**, so under §25 no + COHERENCE edit rides this PR. + + +## 1. Ground truth (scouted @ `ccf29e3`, re-verified each revision) + +### 1.1 The failure reports an errno and nothing else + +`ProcessSupervisor::signal` (`src/process.rs:921`) maps the `kill` +failure to `format!("kill: {e}")` (`:931`). That string is everything a +reader gets. + +### 1.2 The signal target is not the observation target + +- **Signal target** — `signal_target` (`:687`) returns `-pgrp` for a + PTY, where `pgrp = master.process_group_leader()`: the tty's + **current foreground process group**, read at signal time. +- **Observation target** — `ChildHandle::try_wait` (`:668`) observes the + **spawned leader**. + +They coincide only while the leader owns the terminal. Job control is +precisely the mechanism that makes them diverge, and the PTY path is +**always group-directed by design** — spawn rejects `group = true` for +PTY mode with the rationale that "PTY children already lead their own +session and are signaled group-wide" (`:1428-1429`). + +**This is why every tolerance rule across rev 1–3 failed review**, and +why the diagnostic must record the target and the leader state as +*separate* facts. + +### 1.3 The reap ledger is disjoint from this path + +`tick_reap_ledger` (`:1075`) treats any probe error as "nothing left we +can reach" for **bounded growth**, asserting EPERM "cannot happen for our +own children". It is armed only for `proc.spec.group`, which PTY mode +cannot set. Rev 1's "asymmetry" argument was a misreading; withdrawn. + +### 1.4 The observed failure, and the limits of the evidence + +macOS CI, PR #172 (**docs-only** diff), `Test (macos-latest / luajit)`, +`acc28_child_input_and_the_c_c_escape_work_unchanged_in_a_panel` +([attempt 1](https://github.com/levineuwirth/pmacs/actions/runs/30177276839/attempts/1)): + +``` +in function 'terminate' +cause: ExternalError(Process("kill: EPERM: Operation not permitted")) +``` + +**Established:** the errno, and the call path +(`pmacs.terminal.terminate` → `session.rs:566` → `signal`). + +**Not established:** that the child had exited (the probe's last source +statement is a file write at +`tests/bottom_panel_stage1_acceptance.rs:2239`; CPython teardown follows +and does not synchronise with it); that any pgid was recycled; or what +the signal target actually was. + +**This is the whole reason the lane is diagnostic.** Every candidate fix +needs at least one of those three facts, and none is available. + +### 1.5 Caller inventory + +| Caller | Disposition | +|---|---| +| `src/lsp.rs:1364`, `:2427` | discards (`let _ =`) | +| `src/mcp.rs:1229`, `:1239`, `:1915` | discards (`let _ =`) | +| `src/terminal/session.rs:319`, `:607`, `:635` | discards (`let _ =`) | +| **`src/terminal/session.rs:566`** (propagating at `:577`) | **propagates** as `TerminalError::Process` | +| supervisor-internal `shutdown` path | discards | +| `src/lua_bindings/mod.rs:8150`, `:8164` | propagates to Lua | +| `src/lua_bindings/mod.rs:8717` | propagates (via `session.rs:566`) | +| `src/daemon.rs:4162` | **test-only** `.expect`, not production | + +No test in the repository asserts either error string, so widening the +message breaks nothing. + +### 1.6 `portable-pty` caches the exit status on Unix + +Pinned `portable-pty 0.9.0`: `spawn_command` returns +`std::process::Child` (`unix.rs:228`), and `impl Child for +std::process::Child::try_wait` delegates to +`std::process::Child::try_wait` (`lib.rs:271-277`), which caches into +`self.status`. Both `ChildHandle` variants therefore cache. + + +## 2. Decisions + +### Q#PD1 — what the widened error records + +On a `kill` failure in `signal`, the error carries: + +| Field | Why | +|---|---| +| **target source** — `tcgetpgrp` vs `group` vs `leader-pid` fallback | which branch of `signal_target` (`:687`) ran | +| **target kind and value** — `-pgid` or `pid`, with the number | the entity actually signalled | +| **spawn-time pgid / leader pid** | a divergence from the target is the job-control hypothesis, visible only by comparison | +| **errno** | as today | +| **leader `try_wait` state** — `exited(status)` / `live` / `unobservable(e)` | separates "the leader is gone" from "the group we signalled is gone" — the distinction all three failed designs collapsed | + +Every candidate Stage B rule is decidable from these five together, and +none is decidable from the errno alone. + +### Q#PD2 — the disposition is preserved exactly + +The call still fails, with the same `Err`, in every case. No state +transition changes, no ledger arming changes, no tolerance. A reader +diffing behaviour should find none. + +### Q#PD3 — the honest claim is "no disposition change", not "strictly additive" + +Rev 3 said the diagnostic was only an error-string change and could not +regress behaviour. **That overstated it.** `try_wait` on an exited child +**reaps it and caches the status**, so consulting it in the failure path +is an internal state change: the child may be reaped earlier than it +otherwise would be. + +Observably safe, because both variants cache (§1.6) and `poll_one` +(`:1133`) will still see `Ok(Some(_))` and emit its event. But safe by +argument is not safe by assertion, so the terminate-failure-then-tick +event pin is retained (acceptance 5). + +### Q#PD4 — the injected-kill seam injects the KILL, never the observation + +Acceptance 5 needs a forced `kill` failure while the **real** +`ChildHandle::try_wait` runs against the **real** child. A stubbed +observation would bypass exactly the code path in question. + +So the seam is a test-only override of the *kill attempt's result*, +consumed once by the signal path; everything downstream — target +selection, the observation, the error construction — runs for real. This +also makes the diagnostic's own fields testable without racing the +kernel. + +### Q#PD5 — nothing else lands here + +No tolerance rule, no idempotence change, no `signal_target` change. See +§5. + + +## 3. Bets (falsifiable) + +- **B1 — The five fields are sufficient to discriminate the §1.4 + hypotheses.** Falsified if a recurrence carries all five and still + leaves the cause ambiguous — which would itself be a finding worth + having. +- **B2 — Widening the message breaks no caller.** Evidence: §1.5, and no + test asserts the string. + +*Retracted across revisions and not reinstated:* rev 1's "a live owned +child cannot yield EPERM"; rev 2's "exit observation suffices"; rev 2's +"this removes the failure class"; rev 3's "group ESRCH is safe to +tolerate". + + +## 4. Acceptance + +1. A group-directed `kill` failure produces an error carrying all five + Q#PD1 fields, with the target rendered as `-pgid` and the leader + state distinct from it. +2. A leader-directed `kill` failure does the same, with the target + rendered as `pid` and the target source recorded as the fallback + branch. +3. The leader state renders each of `exited(status)`, `live`, and + `unobservable(e)` correctly. +4. **The disposition is unchanged**: every injected failure still + returns `Err`, with no state transition and no ledger arming + (Q#PD2). Falsified by revert — flipping any arm to `Ok` fails this. +5. **Forced injected kill failure against the real PTY child + observation**, then tick: exactly one exit event, with the correct + status (Q#PD3/Q#PD4). A fully stubbed observation does not satisfy + this and is rejected as vacuous. +6. The existing suites stay green, pinning "no behavioural change" from + the outside. + + +## 5. Parked (not deferred-and-forgotten — each needs its own evidence) + +- **All tolerance rules.** Group-directed EPERM *and* ESRCH both fail on + the §1.2 entity split; leader-directed tolerance is plausible but + unmotivated until evidence shows the fallback branch is ever taken. + Needs Stage A evidence first. +- **Q#PS6, `terminate` on an already-reaped process returning `Ok`.** + Independent behavioural fix, different failure (§1.6 of rev 3), its + own lane under one-feature/one-PR. +- **`signal_target`'s read-then-kill of `tcgetpgrp`** — still the most + likely real fix site, still unframed. +- `terminate` cancelling pending restarts; PTYs in + `pmacs.process.list`; any change to `C-c` delivery. + + +## 6. Gates + +Full suite per `CLAUDE.md`. Touched suites: +`bottom_panel_stage1_acceptance`, the vterm stages, and +`compile_mode_acceptance`. Sweep with `-- --skip basedpyright`. + + +## 7. Branch plan + +`pty-terminate-eperm`, one PR, diagnostic only. This framing is its first +commit; the instrumentation and its tests are the second. + +**The lane then closes.** It does not wait for the flake to recur: the +next occurrence — whenever it happens, under whoever's PR — carries its +own evidence, and Stage B is framed then. Math work proceeds immediately +after this lands. From 62316a9ced3e8e36d9771fb3bd365640cc757052 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 21:25:58 -0400 Subject: [PATCH 07/18] fix(process): make a failing kill describe itself (Q#PD1) A failing kill in ProcessSupervisor::signal reported an errno and nothing else, which is not enough to diagnose the macOS CI failure that prompted this lane: three different hypotheses about that EPERM produce the same message, and the fix each one implies is different. The error now carries five facts as separate fields: the target source (which branch of signal_target ran), the target kind and value, the spawn-time group for a group-directed signal, the errno, and the spawned leader's real try_wait state. Keeping the target and the leader apart is the whole point. For a PTY the signal goes to the terminal's foreground process group, read from the tty at signal time, while the leader is the child that was spawned. Those are different entities whenever job control has moved the terminal, and three rejected designs for this code were unsound precisely because they concluded something about one from the other. The report states both and concludes nothing. The disposition is unchanged. Every call that failed before still fails, with no state transition and no reap-ledger arming. That is asserted directly rather than assumed, because it is what separates this from the tolerance rules review rejected. Q#PD3, stated narrowly: this is not a pure message change. Consulting try_wait reaps an exited child and caches its status, so the child may be reaped earlier than it otherwise would be. That is observably safe because portable-pty 0.9.0 returns a std::process::Child on Unix and delegates try_wait straight to it, so the status is cached and poll_one still sees it -- but safe by argument is not safe by assertion, so a test forces a kill failure against the real PTY child and then checks that exactly one terminal event survives. Q#PD4: the test seam injects the kill attempt's result only, never the observation. Target selection, the real ChildHandle::try_wait against the real child, and the error construction all run unmodified; a stubbed observation would bypass the code path under test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- src/process.rs | 385 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 378 insertions(+), 7 deletions(-) diff --git a/src/process.rs b/src/process.rs index 2b53bf3..88fb42e 100644 --- a/src/process.rs +++ b/src/process.rs @@ -470,6 +470,13 @@ pub struct ProcessSupervisor { /// TERM→KILL window used when arming the ledger. Constant /// [`GROUP_TERM_GRACE`] in production; overridable in tests. group_term_grace: Duration, + /// Q#PD4 test seam: forces the next `kill(2)` attempt in + /// [`Self::signal`] to fail with this errno, consumed once. + /// Always `None` in production — there is no way to set it outside + /// `cfg(test)`. It replaces the *kill result only*, so the leader + /// observation still runs against the real child handle; a stubbed + /// observation would bypass the code path under test. + forced_kill_errno: Option, } /// One armed group in the reap ledger. @@ -684,7 +691,50 @@ impl ChildHandle { } } -fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { +/// Which branch of [`signal_target`] chose the target (Q#PD1). +/// +/// Recorded on failure because the branches differ in what a failing +/// `kill` can possibly mean: only [`Self::LeaderPid`] aims at the +/// spawned child itself. The other two aim at a *group*, which for a +/// PTY is read from the terminal and can belong to something the +/// supervisor never spawned. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum TargetSource { + /// The tty's current foreground process group, read at signal + /// time. Diverges from the leader exactly when job control has + /// moved the terminal. + ForegroundGroup, + /// A `group = true` pipe child leading its own process group. + SpawnGroup, + /// The child's own pid. + LeaderPid, +} + +impl TargetSource { + fn as_str(self) -> &'static str { + match self { + Self::ForegroundGroup => "tcgetpgrp", + Self::SpawnGroup => "group", + Self::LeaderPid => "leader-pid", + } + } + + /// Whether the target is a process group rather than one process. + fn is_group(self) -> bool { + matches!(self, Self::ForegroundGroup | Self::SpawnGroup) + } +} + +/// The entity a signal was actually aimed at, plus the branch that +/// chose it. Carried so a failure can report the target as a fact +/// separate from the leader's state (Q#PD1). +#[derive(Debug, Clone, Copy)] +struct SignalTarget { + pid: Pid, + source: TargetSource, +} + +fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { if let Some(runtime) = proc.runtime.as_ref() && let ChildHandle::Pty { _master: master, .. @@ -692,7 +742,10 @@ fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { && let Some(pgrp) = master.process_group_leader() && pgrp > 0 { - return Ok(Pid::from_raw(-pgrp)); + return Ok(SignalTarget { + pid: Pid::from_raw(-pgrp), + source: TargetSource::ForegroundGroup, + }); } // `group = true` pipe children lead a fresh process group // (`process_group(0)` at spawn ⇒ pgid == pid), so fatal signals @@ -700,11 +753,80 @@ fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { // (Q#CM3). if proc.spec.group { let pgid = i32::try_from(pid).map_err(|e| e.to_string())?; - return Ok(Pid::from_raw(-pgid)); + return Ok(SignalTarget { + pid: Pid::from_raw(-pgid), + source: TargetSource::SpawnGroup, + }); } - Ok(Pid::from_raw( - i32::try_from(pid).map_err(|e| e.to_string())?, - )) + Ok(SignalTarget { + pid: Pid::from_raw(i32::try_from(pid).map_err(|e| e.to_string())?), + source: TargetSource::LeaderPid, + }) +} + +/// The spawned leader's state at the moment a `kill` failed (Q#PD1). +/// +/// Deliberately reported *beside* the target rather than folded into a +/// verdict: for a PTY the two are different entities whenever job +/// control has moved the terminal, and three successive designs for +/// this code were unsound precisely because they collapsed them. +enum LeaderObservation { + Exited(TermStatus), + Live, + Unobservable(String), + NoRuntime, +} + +impl LeaderObservation { + fn render(&self) -> String { + match self { + Self::Exited(TermStatus::Exited(code)) => format!("exited(code {code})"), + Self::Exited(TermStatus::Signaled(sig)) => format!("exited(signal {sig})"), + Self::Live => "live".to_owned(), + Self::Unobservable(e) => format!("unobservable({e})"), + Self::NoRuntime => "no-runtime".to_owned(), + } + } +} + +/// Observe the spawned leader. Note this *reaps* an exited child and +/// caches its status; that is why Q#PD3 claims "no disposition change" +/// rather than "strictly additive", and why an event-count test pins +/// that `poll_one` still emits exactly one exit event afterwards. +fn observe_leader(proc: &mut ManagedProcess) -> LeaderObservation { + let Some(runtime) = proc.runtime.as_mut() else { + return LeaderObservation::NoRuntime; + }; + match runtime.child.try_wait() { + Ok(Some(status)) => LeaderObservation::Exited(status), + Ok(None) => LeaderObservation::Live, + Err(e) => LeaderObservation::Unobservable(e), + } +} + +/// Render a failing `kill` as the five facts of Q#PD1. The disposition +/// is unchanged (Q#PD2) — this only replaces a message that said +/// nothing but the errno. +fn signal_failure_report( + target: SignalTarget, + leader_pid: u32, + errno: &nix::errno::Errno, + leader: &LeaderObservation, +) -> String { + let expected = if target.source.is_group() { + match i32::try_from(leader_pid) { + Ok(p) => format!(", expected_group=-{p}"), + Err(_) => String::new(), + } + } else { + String::new() + }; + format!( + "kill: {errno} (target={} via {}, leader_pid={leader_pid}{expected}, leader={})", + target.pid.as_raw(), + target.source.as_str(), + leader.render(), + ) } /// Termination status of one generation. Internal --- the supervisor @@ -807,9 +929,20 @@ impl ProcessSupervisor { shut_down: false, reap_ledger: HashMap::new(), group_term_grace: GROUP_TERM_GRACE, + forced_kill_errno: None, } } + /// Q#PD4 test seam: make the next `kill(2)` attempt in + /// [`Self::signal`] report `errno` instead of calling the kernel. + /// Consumed by that one attempt. Everything downstream — target + /// selection, the leader observation against the real child, and + /// the error construction — runs unmodified. + #[cfg(test)] + fn force_next_kill_errno(&mut self, errno: nix::errno::Errno) { + self.forced_kill_errno = Some(errno); + } + /// Override the SIGTERM-to-SIGKILL grace window. Test helper. pub fn set_grace_period(&mut self, d: Duration) { self.grace_period = d; @@ -928,7 +1061,21 @@ impl ProcessSupervisor { return Err(format!("process {id} is not running")); }; let target = signal_target(proc, pid)?; - nix::sys::signal::kill(target, Some(signal)).map_err(|e| format!("kill: {e}"))?; + // Q#PD4: the seam injects the KILL attempt's result only — + // never the observation below — so target selection, the real + // `ChildHandle::try_wait` against the real child, and the error + // construction all run for real. Consumed once. + let kill_result = match self.forced_kill_errno.take() { + Some(errno) => Err(errno), + None => nix::sys::signal::kill(target.pid, Some(signal)), + }; + if let Err(errno) = kill_result { + // Q#PD1/Q#PD2: the failure describes itself; the + // disposition is unchanged — this still returns `Err`, + // with no state transition and no ledger arming. + let leader = observe_leader(proc); + return Err(signal_failure_report(target, pid, &errno, &leader)); + } if matches!(signal, Signal::SIGTERM | Signal::SIGKILL | Signal::SIGHUP) { proc.state = ProcessState::Exiting { pid, @@ -2131,6 +2278,230 @@ mod tests { ); } + /// Spawn a PTY child that stays alive until terminated, and wait + /// for its `Started` event so a pid and a foreground group exist. + fn spawn_live_pty(sup: &mut ProcessSupervisor, name: &str) -> ProcessId { + let mut spec = ProcessSpec::new(name, "/bin/sh"); + spec.args = vec!["-c".into(), "sleep 30".into()]; + spec.mode = ProcessMode::Pty { + rows: 24, + cols: 80, + mode: TerminalMode::Canonical, + }; + let id = sup.spawn(spec).expect("spawn"); + let _ = drain_until(sup, id, Duration::from_secs(5), |evs| { + evs.iter() + .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) + }); + id + } + + /// Q#PD1 acceptance 1 — a group-directed failure names the target, + /// the branch that chose it, the expected group, the errno, and the + /// leader's own state, as five separate facts. + /// + /// The leader field is the one that matters: for a PTY the signal + /// goes to the terminal's foreground group, which is a different + /// entity from the spawned child whenever job control has moved + /// the terminal. Three rejected designs for this code collapsed + /// the two; the report keeps them apart. + #[test] + fn a_group_directed_kill_failure_reports_target_and_leader_separately() { + let mut sup = ProcessSupervisor::new(); + let id = spawn_live_pty(&mut sup, "diag-group"); + + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + assert!(err.contains("EPERM"), "errno is reported: {err}"); + assert!( + err.contains("via tcgetpgrp"), + "the target SOURCE distinguishes a tty-read group from a spawn group: {err}" + ); + assert!( + err.contains("target=-"), + "a group target renders negative: {err}" + ); + assert!( + err.contains("expected_group=-"), + "the spawn-time group is shown so a divergence is visible: {err}" + ); + assert!( + err.contains("leader=live"), + "the leader is observed independently of the group: {err}" + ); + // Non-vacuity: the two numbers are actually rendered, not empty. + assert!( + err.contains("leader_pid=") && !err.contains("leader_pid=0,"), + "a real leader pid is reported: {err}" + ); + } + + /// Q#PD1 acceptance 2 — a leader-directed failure records the + /// fallback branch and a positive target, and omits the group + /// field that would be meaningless for it. + #[test] + fn a_leader_directed_kill_failure_reports_the_fallback_branch() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-leader", "/bin/sh"); + spec.args = vec!["-c".into(), "sleep 30".into()]; + let id = sup.spawn(spec).expect("spawn"); + let _ = drain_until(&mut sup, id, Duration::from_secs(5), |evs| { + evs.iter() + .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) + }); + + sup.force_next_kill_errno(nix::errno::Errno::ESRCH); + let err = sup.terminate(id).expect_err("injected ESRCH must fail"); + + assert!(err.contains("ESRCH"), "errno is reported: {err}"); + assert!( + err.contains("via leader-pid"), + "a non-group pipe child targets its own pid: {err}" + ); + assert!( + !err.contains("target=-"), + "a leader target renders positive: {err}" + ); + assert!( + !err.contains("expected_group="), + "the group field is omitted where it has no meaning: {err}" + ); + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#PD1 acceptance 3 — every leader state renders distinctly. The + /// `Unobservable` and `NoRuntime` arms cannot be produced by a + /// real child on demand, so they are pinned directly; `live` and + /// `exited` are pinned through the real path by the tests around + /// this one. + #[test] + fn every_leader_observation_renders_distinctly() { + assert_eq!( + LeaderObservation::Exited(TermStatus::Exited(0)).render(), + "exited(code 0)" + ); + assert_eq!( + LeaderObservation::Exited(TermStatus::Signaled("SIGTERM".into())).render(), + "exited(signal SIGTERM)" + ); + assert_eq!(LeaderObservation::Live.render(), "live"); + assert_eq!( + LeaderObservation::Unobservable("try_wait: boom".into()).render(), + "unobservable(try_wait: boom)" + ); + assert_eq!(LeaderObservation::NoRuntime.render(), "no-runtime"); + } + + /// Q#PD1 acceptance 3, exited arm through the REAL path — the + /// leader has genuinely exited and the report says so. + #[test] + fn a_failure_after_the_child_exits_reports_the_leader_as_exited() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-exited", "/bin/sh"); + spec.args = vec!["-c".into(), "exit 3".into()]; + let id = sup.spawn(spec).expect("spawn"); + // Wait for the child to actually be gone, but do NOT tick past + // the point where the record leaves Running — `signal` needs a + // live record to reach the kill at all. + std::thread::sleep(Duration::from_millis(300)); + + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + assert!( + err.contains("leader=exited("), + "an exited leader is observed as exited, not guessed from the errno: {err}" + ); + } + + /// Q#PD2 acceptance 4 — **the disposition is unchanged.** An + /// injected failure still fails, and neither the state transition + /// nor the reap-ledger arming runs. This is the assertion that + /// separates a diagnostic from the tolerance rules three review + /// rounds rejected; flipping any arm to `Ok` fails it. + #[test] + fn an_injected_failure_changes_no_state_and_arms_no_ledger() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-disposition", "/bin/sh"); + spec.args = vec!["-c".into(), "sleep 30".into()]; + spec.group = true; + let id = sup.spawn(spec).expect("spawn"); + let _ = drain_until(&mut sup, id, Duration::from_secs(5), |evs| { + evs.iter() + .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) + }); + assert!( + sup.reap_ledger.is_empty(), + "precondition: nothing armed before the attempt" + ); + + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + assert!(err.contains("via group"), "a group=true pipe child: {err}"); + + assert!( + matches!( + sup.processes.get(&id).expect("record").state, + ProcessState::Running { .. } + ), + "a failed kill must not transition the record to Exiting" + ); + assert!( + sup.reap_ledger.is_empty(), + "a failed kill must not arm the reap ledger" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#PD3/Q#PD4 acceptance 5 — the diagnostic consults the REAL + /// `ChildHandle::try_wait` on the REAL child, which reaps it and + /// caches the status. `poll_one` must still emit exactly one exit + /// event afterwards. + /// + /// A stubbed observation would bypass the double-`try_wait` path + /// entirely and pin nothing, so the injection replaces the kill + /// result only. + #[test] + fn observing_the_leader_does_not_consume_the_exit_event() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-one-event", "/bin/sh"); + spec.args = vec!["-c".into(), "exit 7".into()]; + spec.mode = ProcessMode::Pty { + rows: 24, + cols: 80, + mode: TerminalMode::Canonical, + }; + let id = sup.spawn(spec).expect("spawn"); + std::thread::sleep(Duration::from_millis(300)); + + // The forced failure drives `observe_leader`, which try_waits + // the real PTY child for the first time. + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + assert!( + err.contains("leader=exited("), + "the real handle was consulted: {err}" + ); + + // Now the supervisor's own try_wait must still see the status. + let evs = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); + let terminal = evs + .iter() + .filter(|e| { + matches!( + e.kind, + ProcessEventKind::Exited { .. } | ProcessEventKind::Signaled { .. } + ) + }) + .count(); + assert_eq!( + terminal, 1, + "exactly one terminal event survives the diagnostic's try_wait" + ); + } + #[test] fn signal_terminates_a_running_child() { let mut sup = ProcessSupervisor::new(); From 52731ba1216055ab1c1380ee4f3cc63b42bfae26 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 21:30:23 -0400 Subject: [PATCH 08/18] style: pass Errno by value (clippy pedantic) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- src/process.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/process.rs b/src/process.rs index 88fb42e..1453e10 100644 --- a/src/process.rs +++ b/src/process.rs @@ -810,7 +810,7 @@ fn observe_leader(proc: &mut ManagedProcess) -> LeaderObservation { fn signal_failure_report( target: SignalTarget, leader_pid: u32, - errno: &nix::errno::Errno, + errno: nix::errno::Errno, leader: &LeaderObservation, ) -> String { let expected = if target.source.is_group() { @@ -1074,7 +1074,7 @@ impl ProcessSupervisor { // disposition is unchanged — this still returns `Err`, // with no state transition and no ledger arming. let leader = observe_leader(proc); - return Err(signal_failure_report(target, pid, &errno, &leader)); + return Err(signal_failure_report(target, pid, errno, &leader)); } if matches!(signal, Signal::SIGTERM | Signal::SIGKILL | Signal::SIGHUP) { proc.state = ProcessState::Exiting { From 40f7f8169019204ec2df2a85366f2673326d604b Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 22:16:56 -0400 Subject: [PATCH 09/18] test(process): pin exact diagnostic values and drop the timing-dependent sleeps Round-1 review found both test weaknesses. The exited-child tests used a fixed 300 ms sleep as proof the child had exited, which on a loaded runner can be false and would turn them into spurious failures. nix's waitid is unavailable on macOS and libc::waitid would need unsafe, which the crate forbids, so the tests now synchronise on the observation under test: a bounded loop that drives the production diagnostic until it reports the leader as exited. Each failing attempt leaves the record untouched because the failure path returns before any bookkeeping, so the loop is side-effect free, and it is strictly stronger than a sleep because it observes the actual state rather than assuming it. The assertions were substring checks -- target=-, expected_group=-, leader=exited( -- which a hardcoded target or a wrong exit code would satisfy. They are now exact message equality built from the pid the kernel actually assigned and the errno's own Display, and the one-event test asserts the surviving event carries exit code 7 rather than any terminal event. The group test also spawns /bin/sleep directly rather than through a shell, since a shell may place the command in a different foreground process group than the one being asserted. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- src/process.rs | 213 ++++++++++++++++++++++++++++--------------------- 1 file changed, 122 insertions(+), 91 deletions(-) diff --git a/src/process.rs b/src/process.rs index 1453e10..a944de4 100644 --- a/src/process.rs +++ b/src/process.rs @@ -2278,103 +2278,131 @@ mod tests { ); } - /// Spawn a PTY child that stays alive until terminated, and wait - /// for its `Started` event so a pid and a foreground group exist. - fn spawn_live_pty(sup: &mut ProcessSupervisor, name: &str) -> ProcessId { - let mut spec = ProcessSpec::new(name, "/bin/sh"); - spec.args = vec!["-c".into(), "sleep 30".into()]; + /// Spawn a PTY child that leads its own session and stays alive + /// until terminated, returning its id and OS pid. + /// + /// `/bin/sleep` directly rather than through a shell: a shell may + /// place the command in a different foreground process group, and + /// these tests assert the exact target the tty reports. + fn spawn_live_pty(sup: &mut ProcessSupervisor, name: &str) -> (ProcessId, u32) { + let mut spec = ProcessSpec::new(name, "/bin/sleep"); + spec.args = vec!["30".into()]; spec.mode = ProcessMode::Pty { rows: 24, cols: 80, mode: TerminalMode::Canonical, }; let id = sup.spawn(spec).expect("spawn"); - let _ = drain_until(sup, id, Duration::from_secs(5), |evs| { + (id, spawn_started_pid(sup, id)) + } + + /// Drain until `Started` and return the OS pid it carries. + fn spawn_started_pid(sup: &mut ProcessSupervisor, id: ProcessId) -> u32 { + let evs = drain_until(sup, id, Duration::from_secs(5), |evs| { evs.iter() .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) }); - id + evs.iter() + .find_map(|e| match e.kind { + ProcessEventKind::Started { pid } => Some(pid), + _ => None, + }) + .expect("Started carries a pid") + } + + /// Drive the production diagnostic until it observes the leader as + /// exited, bounded by `timeout`. + /// + /// A fixed sleep is NOT proof of exit — on a loaded runner the child + /// can still be live, which would turn these tests into false + /// failures. This synchronises on the very observation under test. + /// Each failing attempt leaves the record untouched, because the + /// failure path returns before any bookkeeping (Q#PD2), so looping + /// is side-effect free. + fn terminate_until_leader_exited( + sup: &mut ProcessSupervisor, + id: ProcessId, + timeout: Duration, + ) -> String { + let deadline = Instant::now() + timeout; + loop { + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + if err.contains("leader=exited(") { + return err; + } + assert!( + Instant::now() < deadline, + "leader never observed as exited within {timeout:?}: {err}" + ); + std::thread::sleep(Duration::from_millis(10)); + } } /// Q#PD1 acceptance 1 — a group-directed failure names the target, /// the branch that chose it, the expected group, the errno, and the /// leader's own state, as five separate facts. /// - /// The leader field is the one that matters: for a PTY the signal - /// goes to the terminal's foreground group, which is a different - /// entity from the spawned child whenever job control has moved - /// the terminal. Three rejected designs for this code collapsed - /// the two; the report keeps them apart. + /// Asserted as an exact message against the pid the kernel actually + /// assigned, so a hardcoded target could not satisfy it. The leader + /// field is the one that matters: for a PTY the signal goes to the + /// terminal's foreground group, a different entity from the spawned + /// child whenever job control has moved the terminal. Three rejected + /// designs for this code collapsed the two; the report keeps them + /// apart, and here they are asserted to agree only because nothing + /// has moved the terminal. #[test] fn a_group_directed_kill_failure_reports_target_and_leader_separately() { let mut sup = ProcessSupervisor::new(); - let id = spawn_live_pty(&mut sup, "diag-group"); + let (id, pid) = spawn_live_pty(&mut sup, "diag-group"); sup.force_next_kill_errno(nix::errno::Errno::EPERM); let err = sup.terminate(id).expect_err("injected EPERM must fail"); - assert!(err.contains("EPERM"), "errno is reported: {err}"); - assert!( - err.contains("via tcgetpgrp"), - "the target SOURCE distinguishes a tty-read group from a spawn group: {err}" + let expected = format!( + "kill: {} (target=-{pid} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=live)", + nix::errno::Errno::EPERM ); - assert!( - err.contains("target=-"), - "a group target renders negative: {err}" - ); - assert!( - err.contains("expected_group=-"), - "the spawn-time group is shown so a divergence is visible: {err}" - ); - assert!( - err.contains("leader=live"), - "the leader is observed independently of the group: {err}" - ); - // Non-vacuity: the two numbers are actually rendered, not empty. - assert!( - err.contains("leader_pid=") && !err.contains("leader_pid=0,"), - "a real leader pid is reported: {err}" + assert_eq!( + err, expected, + "the report names the exact target the tty reported, the exact \ + leader pid, and observes the leader as live" ); + + let _ = sup.signal(id, Signal::SIGKILL); } /// Q#PD1 acceptance 2 — a leader-directed failure records the - /// fallback branch and a positive target, and omits the group - /// field that would be meaningless for it. + /// fallback branch and a positive target, and omits the group field + /// that would be meaningless for it. Exact message again. #[test] fn a_leader_directed_kill_failure_reports_the_fallback_branch() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("diag-leader", "/bin/sh"); - spec.args = vec!["-c".into(), "sleep 30".into()]; + let mut spec = ProcessSpec::new("diag-leader", "/bin/sleep"); + spec.args = vec!["30".into()]; let id = sup.spawn(spec).expect("spawn"); - let _ = drain_until(&mut sup, id, Duration::from_secs(5), |evs| { - evs.iter() - .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) - }); + let pid = spawn_started_pid(&mut sup, id); sup.force_next_kill_errno(nix::errno::Errno::ESRCH); let err = sup.terminate(id).expect_err("injected ESRCH must fail"); - assert!(err.contains("ESRCH"), "errno is reported: {err}"); - assert!( - err.contains("via leader-pid"), - "a non-group pipe child targets its own pid: {err}" + let expected = format!( + "kill: {} (target={pid} via leader-pid, leader_pid={pid}, leader=live)", + nix::errno::Errno::ESRCH ); - assert!( - !err.contains("target=-"), - "a leader target renders positive: {err}" - ); - assert!( - !err.contains("expected_group="), - "the group field is omitted where it has no meaning: {err}" + assert_eq!( + err, expected, + "a non-group pipe child targets its own pid, and the group \ + field is omitted where it has no meaning" ); + let _ = sup.signal(id, Signal::SIGKILL); } /// Q#PD1 acceptance 3 — every leader state renders distinctly. The - /// `Unobservable` and `NoRuntime` arms cannot be produced by a - /// real child on demand, so they are pinned directly; `live` and - /// `exited` are pinned through the real path by the tests around - /// this one. + /// `Unobservable` and `NoRuntime` arms cannot be produced by a real + /// child on demand, so they are pinned directly; `live` and `exited` + /// are pinned through the real path by the tests around this one. #[test] fn every_leader_observation_renders_distinctly() { assert_eq!( @@ -2393,25 +2421,27 @@ mod tests { assert_eq!(LeaderObservation::NoRuntime.render(), "no-runtime"); } - /// Q#PD1 acceptance 3, exited arm through the REAL path — the - /// leader has genuinely exited and the report says so. + /// Q#PD1 acceptance 3, exited arm through the REAL path — the leader + /// has genuinely exited and the report carries its exact code, not + /// merely "some exit". #[test] fn a_failure_after_the_child_exits_reports_the_leader_as_exited() { let mut sup = ProcessSupervisor::new(); let mut spec = ProcessSpec::new("diag-exited", "/bin/sh"); spec.args = vec!["-c".into(), "exit 3".into()]; let id = sup.spawn(spec).expect("spawn"); - // Wait for the child to actually be gone, but do NOT tick past - // the point where the record leaves Running — `signal` needs a - // live record to reach the kill at all. - std::thread::sleep(Duration::from_millis(300)); + let pid = spawn_started_pid(&mut sup, id); - sup.force_next_kill_errno(nix::errno::Errno::EPERM); - let err = sup.terminate(id).expect_err("injected EPERM must fail"); + let err = terminate_until_leader_exited(&mut sup, id, Duration::from_secs(10)); - assert!( - err.contains("leader=exited("), - "an exited leader is observed as exited, not guessed from the errno: {err}" + let expected = format!( + "kill: {} (target={pid} via leader-pid, leader_pid={pid}, leader=exited(code 3))", + nix::errno::Errno::EPERM + ); + assert_eq!( + err, expected, + "the exact exit code is observed from the real child, not \ + inferred from the errno" ); } @@ -2427,10 +2457,7 @@ mod tests { spec.args = vec!["-c".into(), "sleep 30".into()]; spec.group = true; let id = sup.spawn(spec).expect("spawn"); - let _ = drain_until(&mut sup, id, Duration::from_secs(5), |evs| { - evs.iter() - .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) - }); + let pid = spawn_started_pid(&mut sup, id); assert!( sup.reap_ledger.is_empty(), "precondition: nothing armed before the attempt" @@ -2438,7 +2465,12 @@ mod tests { sup.force_next_kill_errno(nix::errno::Errno::EPERM); let err = sup.terminate(id).expect_err("injected EPERM must fail"); - assert!(err.contains("via group"), "a group=true pipe child: {err}"); + + let expected = format!( + "kill: {} (target=-{pid} via group, leader_pid={pid}, expected_group=-{pid}, leader=live)", + nix::errno::Errno::EPERM + ); + assert_eq!(err, expected, "a group=true pipe child reports via group"); assert!( matches!( @@ -2458,7 +2490,7 @@ mod tests { /// Q#PD3/Q#PD4 acceptance 5 — the diagnostic consults the REAL /// `ChildHandle::try_wait` on the REAL child, which reaps it and /// caches the status. `poll_one` must still emit exactly one exit - /// event afterwards. + /// event, carrying the exact code. /// /// A stubbed observation would bypass the double-`try_wait` path /// entirely and pin nothing, so the injection replaces the kill @@ -2474,34 +2506,33 @@ mod tests { mode: TerminalMode::Canonical, }; let id = sup.spawn(spec).expect("spawn"); - std::thread::sleep(Duration::from_millis(300)); + let _ = spawn_started_pid(&mut sup, id); - // The forced failure drives `observe_leader`, which try_waits - // the real PTY child for the first time. - sup.force_next_kill_errno(nix::errno::Errno::EPERM); - let err = sup.terminate(id).expect_err("injected EPERM must fail"); + // Drives `observe_leader`, which try_waits the real PTY child + // for the first time and reaps it. + let err = terminate_until_leader_exited(&mut sup, id, Duration::from_secs(10)); assert!( - err.contains("leader=exited("), - "the real handle was consulted: {err}" + err.contains("leader=exited(code 7)"), + "the real handle was consulted and carries the exact code: {err}" ); - // Now the supervisor's own try_wait must still see the status. + // The supervisor's own try_wait must still see that status. let evs = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); - let terminal = evs + let terminal: Vec = evs .iter() - .filter(|e| { - matches!( - e.kind, - ProcessEventKind::Exited { .. } | ProcessEventKind::Signaled { .. } - ) + .filter_map(|e| match e.kind { + ProcessEventKind::Exited { code, .. } => Some(code), + ProcessEventKind::Signaled { .. } => Some(-1), + _ => None, }) - .count(); + .collect(); assert_eq!( - terminal, 1, - "exactly one terminal event survives the diagnostic's try_wait" + terminal, + vec![7], + "exactly one terminal event survives the diagnostic's try_wait, \ + carrying the child's real exit code" ); } - #[test] fn signal_terminates_a_running_child() { let mut sup = ProcessSupervisor::new(); From 18d481b046a5237a5b955dea45e9b13043cf20df Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 22:19:19 -0400 Subject: [PATCH 10/18] docs: record the PTY terminate diagnostic lane in the ledger The ledger's own update protocol requires a lane for volatile work, and PR #176 had none: branch, worktree, review state, and verification were all missing. Records why the lane ships a diagnostic rather than a fix -- three rejected tolerance designs, the two facts that killed the original argument (group=true is rejected for PTY mode so the reap ledger never applies to that path, and the ledger comment asserts EPERM cannot happen rather than ruling that it means dead), and that the CI evidence never established the child had exited. Also records the round-1 test fixes and the four verified bites, so a reader can tell which assertions are load-bearing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- docs/active-work.md | 69 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/docs/active-work.md b/docs/active-work.md index b60d62c..ffd9b1e 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -55,6 +55,75 @@ git status --short --branch The `git log` command must expose `d152120` or a newer intentional main. If it does not, stop and repair the remote/fetch configuration. +## PTY terminate diagnostic lane — IN REVIEW (PR #176) + +- Portable branch: `githubsucks/pty-terminate-eperm`; worktree + `../pmacs-math-slice`. **PR #176**, base `main`, based on `ccf29e3` + with `c93f9ee` (#175) merged in. +- Approved framing: `docs/process-signal-tolerance-framing.md` + **revision 4**, after three review rounds. +- **Diagnostic only. No disposition change.** Every call that failed + before still fails, with no state transition and no reap-ledger + arming. `src/process.rs` is the only source file touched. +- **Why nothing is fixed:** revisions 1–3 each proposed a *tolerance* + rule and all three were rejected as unsound in the same way — each + concluded something about a process from something that was not about + that process. Rev 1 from an errno alone (EPERM means the caller lacks + permission, not that the id was recycled); rev 2 from `try_wait`, + which observes the spawned **leader** while a PTY signal targets + `-tcgetpgrp(...)`, entities that diverge exactly when job control has + moved the terminal; rev 3 from group-directed **ESRCH**, which proves + only that the selected foreground group vanished. +- **Two facts that killed the original argument.** `group = true` is + *rejected* for PTY mode at spawn (`src/process.rs:1428-1429`), so the + reap ledger never applies to the PTY path at all; and the ledger + comment (`:1075`) says EPERM "cannot happen for our own children" and + drops the entry for **bounded growth** — not a ruling that EPERM means + dead. +- **The CI evidence never established the child had exited.** The probe's + last source statement is a file write and CPython teardown does not + synchronise with it, so no tolerance rule could even be shown to fix + the symptom. That is the whole reason the lane is diagnostic. +- What ships: a failing `kill` now reports five separate facts — target + source, target kind/value, spawn-time group, errno, and the leader's + real `try_wait` state. The test seam injects the **kill result only**, + never the observation, so the real `ChildHandle::try_wait` runs against + the real child. +- **Not "strictly additive".** `try_wait` reaps and caches, so an exited + child may be reaped earlier than otherwise. Safe because + `portable-pty` 0.9.0 returns a `std::process::Child` on Unix and + delegates `try_wait` to it, so `poll_one` still sees the cached + status — pinned by an exactly-one-terminal-event test rather than + assumed. +- Round-1 review fixes: the exited-child tests no longer use a fixed + sleep as proof of exit (nix's `waitid` is unavailable on macOS and + `libc::waitid` needs `unsafe`, which the crate forbids), instead + driving the production diagnostic in a bounded loop until it observes + the exit; and every assertion is now exact message equality built from + the kernel-assigned pid, since the substring forms would have accepted + a hardcoded target or a wrong exit code. +- Bites, all verified rather than assumed: tolerating the failure fails + the disposition test; stubbing the leader observation fails three + tests including the one-event pin; a hardcoded target fails four; a + wrong exit code fails two. +- Verification: fmt, `git diff --check`, strict workspace clippy clean; + lib 1,838 + CRDT 2,015 (both +6, exactly the new tests); GPU 202; M4 + 121; bottom-panel 46; compile-mode 67; vterm 9/6/5; sweep 3,256 across + 93 suites with two load-contention flakes that pass 3/3 isolated + (`read_dir_supersede_cancels_in_flight_predecessor`, known + pre-existing, and + `headless_snapshot_round_trip_summary_restores_the_minimap`). The + second is structurally unreachable from this diff: `pmacs-gpu` depends + on `pmacs-protocol`, never on `pmacs`. +- **Parked, each with its reason:** all tolerance rules (need the + evidence this PR produces); `terminate` idempotence for an + already-reaped process (independent fix, different failure, one + feature per PR); and `signal_target`'s read-then-kill of `tcgetpgrp` + — still the most likely real fix site. +- **The lane closes when this merges.** It does not wait for the flake + 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) — Stage 1 MERGED; Stage 2 IN REVIEW (PR #161) - Stage 1 **merged as #160** (`main` @ `0827dd1`, 2026-07-25, one review From 00cc615db557d8f8cf67a574df3e4f0e5a65aef8 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 22:25:05 -0400 Subject: [PATCH 11/18] test(process): read the pid without ticking in the fast-exit tests The parallel workspace sweep failed observing_the_leader_does_not_consume_the_exit_event with "process ProcessId(26) is not running". A real defect in the test, not a flake. The helper that fetched the pid drained for the Started event, and draining ticks. A tick can observe an immediately-exiting child and transition the record out of Running, after which signal returns "is not running" and never reaches the diagnostic -- so the loop spun to its 10 s bound and panicked. It passed standalone because the drain returned on Started before poll_one saw the exit; only the sweep's load shifted the timing enough to lose that race. Fast-exiting children now read the pid straight from the supervisor record, which does not tick. The bounded loop also fails fast when the record has left Running, so a future recurrence is diagnosed in one line rather than surfacing as a timeout. Verified under matched load: 15/15 green with all 16 cores saturated. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- src/process.rs | 35 ++++++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/src/process.rs b/src/process.rs index a944de4..9e02629 100644 --- a/src/process.rs +++ b/src/process.rs @@ -2296,7 +2296,27 @@ mod tests { (id, spawn_started_pid(sup, id)) } - /// Drain until `Started` and return the OS pid it carries. + /// The OS pid straight from the supervisor's own record, WITHOUT + /// ticking. + /// + /// `drain_until` ticks, and a tick can observe a fast child's exit + /// and transition the record out of `Running` — after which + /// `signal` returns "is not running" and never reaches the + /// diagnostic at all. Any test whose child exits promptly must read + /// the pid this way. (Found by the parallel workspace sweep: the + /// drain-based helper raced only under load.) + fn record_pid(sup: &ProcessSupervisor, id: ProcessId) -> u32 { + match sup.processes.get(&id).expect("record").state { + ProcessState::Running { pid, .. } | ProcessState::Exiting { pid, .. } => pid, + ProcessState::Starting => panic!("spawn has not reported a pid yet"), + ProcessState::Terminated(_) => { + panic!("the record already left Running; the pid is unavailable") + } + } + } + + /// Drain until `Started` and return the OS pid it carries. Safe + /// only for children that outlive the drain; see [`record_pid`]. fn spawn_started_pid(sup: &mut ProcessSupervisor, id: ProcessId) -> u32 { let evs = drain_until(sup, id, Duration::from_secs(5), |evs| { evs.iter() @@ -2331,6 +2351,11 @@ mod tests { if err.contains("leader=exited(") { return err; } + assert!( + !err.contains("is not running"), + "the record left Running before the diagnostic could run, so \ + this test never exercised it: {err}" + ); assert!( Instant::now() < deadline, "leader never observed as exited within {timeout:?}: {err}" @@ -2430,7 +2455,9 @@ mod tests { let mut spec = ProcessSpec::new("diag-exited", "/bin/sh"); spec.args = vec!["-c".into(), "exit 3".into()]; let id = sup.spawn(spec).expect("spawn"); - let pid = spawn_started_pid(&mut sup, id); + // NOT `spawn_started_pid`: draining ticks, and this child exits + // immediately. + let pid = record_pid(&sup, id); let err = terminate_until_leader_exited(&mut sup, id, Duration::from_secs(10)); @@ -2506,7 +2533,9 @@ mod tests { mode: TerminalMode::Canonical, }; let id = sup.spawn(spec).expect("spawn"); - let _ = spawn_started_pid(&mut sup, id); + // NOT `spawn_started_pid`: draining ticks, and a tick can reap + // this immediately-exiting child before the diagnostic runs. + let _ = record_pid(&sup, id); // Drives `observe_leader`, which try_waits the real PTY child // for the first time and reaps it. From 3b7cc67197924474544d9d73e6539c9446f1ac75 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 22:33:22 -0400 Subject: [PATCH 12/18] docs: record the sweep-found test race in the lane The parallel sweep failed one of the new tests for a real reason, not a flake: drain_until ticks, and a tick can reap an immediately-exiting child before the diagnostic runs. Recorded with the matched-load measurement that shows the fix is load-bearing (0/15 fixed vs 1/10 unfixed under full saturation), and the final sweep numbers. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HZjWMjwPXhPbt9upku9mCk --- docs/active-work.md | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index ffd9b1e..497dd63 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -106,15 +106,32 @@ If it does not, stop and repair the remote/fetch configuration. the disposition test; stubbing the leader observation fails three tests including the one-event pin; a hardcoded target fails four; a wrong exit code fails two. +- **The sweep found a real defect in these tests, not a flake.** + `observing_the_leader_does_not_consume_the_exit_event` failed with + "process ProcessId(26) is not running": the pid helper drained for + `Started`, and **`drain_until` ticks**. A tick can observe an + immediately-exiting child and move the record out of `Running`, after + which `signal` never reaches the diagnostic at all, so the bounded + loop spun to its limit. It passed standalone because the drain + returned on `Started` before `poll_one` saw the exit; only load lost + the race. Fast-exiting children now read the pid straight from the + supervisor record (no tick), and the loop fails fast if the record + left `Running`. **Verified under matched load: 0/15 with all 16 cores + saturated, while the old ticking helper fails 1/10 — the fix is + load-bearing.** - Verification: fmt, `git diff --check`, strict workspace clippy clean; lib 1,838 + CRDT 2,015 (both +6, exactly the new tests); GPU 202; M4 - 121; bottom-panel 46; compile-mode 67; vterm 9/6/5; sweep 3,256 across - 93 suites with two load-contention flakes that pass 3/3 isolated - (`read_dir_supersede_cancels_in_flight_predecessor`, known - pre-existing, and - `headless_snapshot_round_trip_summary_restores_the_minimap`). The - second is structurally unreachable from this diff: `pmacs-gpu` depends - on `pmacs-protocol`, never on `pmacs`. + 121; bottom-panel 46; compile-mode 67; vterm 9/6/5; **isolated-config + `--no-fail-fast` sweep 3,258 across 93 suites, zero failures**. + Earlier sweeps on this branch showed two failures and then one; the + totals reconcile (3,256/2 → 3,257/1 → 3,258/0, same test count). The + two that were genuinely unrelated — + `read_dir_supersede_cancels_in_flight_predecessor` (known + pre-existing) and + `headless_snapshot_round_trip_summary_restores_the_minimap` — are + load-contention flakes; the second is structurally unreachable from + this diff, since `pmacs-gpu` depends on `pmacs-protocol` and never on + `pmacs`. - **Parked, each with its reason:** all tolerance rules (need the evidence this PR produces); `terminate` idempotence for an already-reaped process (independent fix, different failure, one From 1b1e5990709842706d7b1b70054dd08d0095ff47 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 10:10:27 -0400 Subject: [PATCH 13/18] feat(terminal): copy mode over retained scrollback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage 2 of docs/terminal-config-and-copy-mode-framing.md (rev 4, approved). `M-x terminal.copy-mode`, or `C-t` in a terminal buffer — physically `C-c C-t`, since every unescaped key goes to the child — materializes the retained scrollback into an ordinary read-only, path-less buffer, with `g` to re-snapshot and `q` to return. No protocol change. Materializing is the whole design. isearch, motion, selection and the kill ring work with no new substrate because the snapshot is a rope, so SearchStore and the existing match painting apply unchanged. And "keys must not reach the child" dissolves structurally rather than being guarded: the transport arm keys on is_terminal(buffer_id), and a snapshot is not a terminal, so the arm never fires. The dispatch-shadow count stays at six and describe-key keeps telling the truth — asserted directly, since that is the observable difference between the buffer-local idiom and a shadow. One serializer, not two (Q#TC7). `copy_retained` builds a whole-range selection and hands it to `copy_selection_bytes`; a second walk would re-derive soft-wrap joining, wide-glyph continuation, cluster bytes and per-row trailing-blank trimming, and the two would drift. Four unit pins in view.rs assert exact bytes against the same projection fixtures that pin the serializer itself. Q#TC6a is implemented as two calls, and the second is the load-bearing one: an intercept guards dispatch only, and no Lua binding sets Buffer::read_only, so set_round_trip_input is what keeps a replica frontend from applying optimistically and emitting an op that would pass ensure_writable and mutate both sides. Acceptance 16 pins that UNGATED, because CI never compiles the crdt feature. Eight of nine criteria. Criterion 17's semantic-frontend end-to-end pin is deliberately absent: the optimistic apply lives only in pmacs-gpu/src/main.rs and the headless SemanticClient has no optimistic path, so a faithful test needs the real GPU binary — the a37 foundation, which CI never compiles, silently returns ok when the binary is unbuilt, and is load-sensitive. Both halves of the mechanism are pinned ungated instead (16, and 16b for the hazard); the wire-level half stays an explicit obligation of the CI crdt-coverage lane. Substrate fact found while wiring lifecycle: TerminalManager::prune REACTS to a buffer already gone from the registry rather than removing one, so a child exiting leaves both the terminal and its snapshot alive. 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, each failing exactly one test: removing set_round_trip_input fails acceptance 16 in the DEFAULT configuration; a naive independent serializer fails all four unit pins, with the diffs naming each drift mode; making re-invoke create a fresh buffer fails 18; dropping the kill-with-terminal teardown fails 18; removing the intercept fails 16b. COHERENCE.md: §6 gains this as the worked example that a modal-looking feature need not become a shadow; §11 records the scope="global" deferral's second live case, making the argument for both registry deferrals cumulative; §2 step 8 gains copy mode and keeps the still-missing close command named. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer --- COHERENCE.md | 34 +- builtin/runtime/terminal.lua | 186 +++++++ docs/active-work.md | 49 +- docs/terminal-config-and-copy-mode-framing.md | 35 +- src/lua_bindings/mod.rs | 19 + src/terminal/view.rs | 143 +++++ tests/terminal_copy_mode_acceptance.rs | 516 ++++++++++++++++++ 7 files changed, 970 insertions(+), 12 deletions(-) create mode 100644 tests/terminal_copy_mode_acceptance.rs diff --git a/COHERENCE.md b/COHERENCE.md index 4e7361c..f707234 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -368,7 +368,7 @@ Full verdict table: | 5 | Edit | **Works** | Full CUA + Emacs keymap in 161 lines (`builtin/keymaps/default.lua`); isearch, query-replace, kill ring, undo/redo, auto-indent/pair/comment, atomic save. Genuinely excellent zero-config | | 6 | Language intelligence | **Partial** | Rust grammar bundled and auto-attaches; rust-analyzer preconfigured (`builtin/runtime/lsp.lua:44-52`) — but a missing binary fails silently (§1.2) and highlighting masks it. No LSP status command exists to diagnose | | 7 | Find symbol / file | **File: fixed (open by path merged #162; browsing PR #165). Symbol: works but undiscoverable** | No find-file/dired/picker existed at audit. Now `C-x C-f` opens a known path and `C-x d` / `C-x C-j` browse (flat listing, `dired` mode keymap); `M-.`/`M-?`/`C-c o` still bound but advertised nowhere and server-gated; no workspace-symbol command; `pmacs.index.*` has no UI | -| 8 | Open terminal | **Works** | Full PTY with scrollback + modeline segment, bound to `C-c t` and configurable through three registered settings (`terminal.default-profile`, `terminal.scrollback-rows`, `terminal.escape-key`) plus named `pmacs.terminal.profiles` (PR #173). Named limitation: `C-c t` is unreachable from *inside* a terminal window, where `C-c` is consumed as the escape — `M-x terminal` still works there. *Was broken outright on the GPU frontend until the double terminal-layout sync was fixed: the child took a `SIGWINCH` storm at tick cadence, so typing into it was impossible while output still flowed.* | +| 8 | Open terminal | **Works** | Full PTY with scrollback + modeline segment, bound to `C-c t` and configurable through three registered settings (`terminal.default-profile`, `terminal.scrollback-rows`, `terminal.escape-key`) plus named `pmacs.terminal.profiles` (PR #173), and searchable through `M-x terminal.copy-mode` / `C-c C-t`, which materializes the retained scrollback into an ordinary read-only buffer (Stage 2). Named limitations: `C-c t` is unreachable from *inside* a terminal window, where `C-c` is consumed as the escape — `M-x terminal` still works there; and there is still **no close/kill command**, which is the remaining half of this step's discoverability gap. *Was broken outright on the GPU frontend until the double terminal-layout sync was fixed: the child took a `SIGWINCH` storm at tick cadence, so typing into it was impossible while output still flowed.* | | 9 | Build / test | **Partial** | `M-x compile.run` works, defaults cwd to detected project root, parses Rust `-->` errors — but no keybinding, an **empty first prompt** (`initial = last and last.cmdline or ""`, `builtin/runtime/compile.lua:1134-1138`), and no `cargo build`/`cargo test` suggestion despite `ProjectKind::Cargo` existing (`src/project.rs:77`) | | 10 | Inspect error | **Partial (good once reached)** | `E:n W:n` modeline counts, underlines, `M-g n/p` + ``C-x ` `` walking a unified compile/grep/diag source, message echo, `RET` visits. Gated entirely on step 6 or 9 succeeding first | | 11 | See background work | **Works but undiscoverable** | `*workers*` view via `M-x editor.list-workers`; `C-c C-k` cancel-at-point. No keybinding, no statusline spinner/progress indicator anywhere (§9) | @@ -658,6 +658,25 @@ Facts that define the gap: a terminal buffer. Since #173 that chord is `terminal.escape-key` rather than a hardcoded `C-c`, so a user can *move* which prefix is eaten; they cannot make the shadow stop eating one. +- **A worked example that a modal-*looking* feature need not become a + shadow.** Terminal copy mode (Stage 2 of the terminal-config arc) is + the case that most invited a seventh rung: it wants motion, search and + its own `g`/`q` inside a surface where every unescaped key otherwise + goes to a child process. It resolves to the buffer-local keymap idiom + instead, by **materializing** the retained scrollback into an ordinary + read-only document buffer. The keys-must-not-reach-the-child problem + then dissolves structurally rather than being guarded: the transport + arm keys on `is_terminal(buffer_id)`, and a snapshot buffer is not a + terminal, so the arm never fires. No new precedence rung, no new + hand-synced guard-list entry, and `describe-key` keeps reporting the + truth — pinned by asserting exactly that for the snapshot's `g` and + `q`, which is the observable difference between the idiom and a + shadow. **The count stays at six.** + + The transferable rule: when a feature wants a keymap over *content*, + ask whether the content can become a buffer. The shadows that exist + are the cases where it genuinely cannot (a minibuffer prompt, a + live search prompt) — not the cases where nobody tried. - **No transient-keymap mechanism exists to migrate to.** `KeymapStack` has exactly three fixed scopes — `Buffer(BufferId)`, `Mode(String)`, `Global` (`src/keymap_stack.rs:37-44`); resolution order buffer → @@ -1041,6 +1060,19 @@ layering, provenance, and adoption have not followed.** scalars. It is the clearest evidence yet that table-valued settings are the blocking prerequisite: the terminal is now half-registered, and no settings UI can render the half that matters most. +- **The missing `scope = "global"` flag has its second live case.** After + `autosave.interval-ms`, the terminal's two *open-time* settings — + `terminal.default-profile` and `terminal.scrollback-rows` — are read + before their terminal's identity buffer exists, so a buffer-local + override can never be consulted. The registry accepts `set_local` on + them anyway, because `Live` mutability is all it can express. Nothing + breaks; the setting simply has no effect, which is the worst shape a + configuration surface can take. `terminal.escape-key` is the contrast + that shows this is a real distinction rather than a blanket wish: it + *deliberately* supports buffer-locals, and per-terminal escapes are a + feature. So the argument for both deferrals is now **cumulative and + concrete** rather than hypothetical — two adopters, two distinct + missing primitives, one feature. - **No persistence**: settings changed at runtime do not survive restart (the `custom-file` split-brain question is a named deferral). - The three-level separation holds in principle today (registry / diff --git a/builtin/runtime/terminal.lua b/builtin/runtime/terminal.lua index 143a663..b44e824 100644 --- a/builtin/runtime/terminal.lua +++ b/builtin/runtime/terminal.lua @@ -49,6 +49,18 @@ local function bind_terminal_keys(buffer) bind("C-v", "terminal.page-down") bind("M-<", "terminal.scroll-oldest") bind("M->", "terminal.scroll-bottom") + -- Q#TC8a/Q#TC9: copy mode is ADDITIVE. The live keys above are + -- unchanged; this is one more leaf beside them. `C-t` is globally + -- `edit.transpose-chars`, which is meaningless in a read-only + -- terminal buffer, and binding it buffer-locally is the scoped + -- idiom rather than a shadow — `keymap.bind`'s strictness rejects + -- binding a PREFIX of an existing sequence within a scope, not + -- cross-scope shadowing. + -- + -- Physically typed as `C-c C-t`: in a terminal every unescaped key + -- goes to the child, so terminal-local bindings are reached through + -- the escape. That also matches emacs-libvterm's own chord. + bind("C-t", "terminal.copy-mode") end -- Q#TC1: profiles are a raw Lua table, not a config setting. The @@ -190,6 +202,180 @@ pmacs.command.define { -- `C-c` is consumed as the escape. `M-x terminal` still works there. pmacs.keymap.bind { scope = "global", sequence = "C-c t", command = "terminal" } +-- === Copy mode (Stage 2, Q#TC6) ========================================= +-- +-- `terminal.copy-mode` MATERIALIZES the retained rows into an ordinary +-- read-only document buffer instead of adding a modal state to the +-- terminal. That choice is the whole design: +-- +-- * isearch, motion, selection, `M-w` and the kill ring all work with no +-- new substrate — the snapshot is a rope, so `SearchStore` and the +-- existing match painting apply unchanged; +-- * "keys must not reach the child" dissolves structurally rather than +-- being guarded: the transport arm keys on `is_terminal(buffer)`, and +-- a snapshot buffer is not a terminal, so it never fires; +-- * the dispatch-shadow count stays at SIX (`COHERENCE.md` §6) and +-- `describe-key` keeps telling the truth, because the bindings are +-- buffer-local and inspectable. + +local raw_copy_retained = assert(terminal._copy_retained, + "pmacs.terminal._copy_retained is required") + +-- snapshot buffer name -> { terminal = , buffer = } +-- +-- Keyed by NAME, not by buffer handle: handles are not stable table keys, +-- and a name survives the user killing the snapshot (listview precedent). +local snapshots = {} + +local function buffer_name(buf) + local ok, described = pcall(pmacs.describe.buffer, buf) + if ok and described then return described.name end + return nil +end + +local function find_buffer_by_name(name) + for _, id in ipairs(pmacs.buffer.list()) do + local ok, described = pcall(pmacs.describe.buffer, id) + if ok and described and described.name == name then return id end + end + return nil +end + +-- `*terminal:bash*` -> `*terminal-copy: terminal:bash*`. The surrounding +-- asterisks are stripped before nesting so the result reads as one +-- generated-buffer name rather than two. +local function snapshot_name_for(term_buf) + local name = buffer_name(term_buf) or "terminal" + return string.format("*terminal-copy: %s*", (name:gsub("^%*", ""):gsub("%*$", ""))) +end + +-- Q#TC7: the snapshot text comes from the SAME serializer selection-copy +-- uses, so soft wraps, wide glyphs, clusters and trailing blanks cannot +-- drift between the two. +local function render_snapshot(record) + local text = raw_copy_retained(record.terminal) or "" + local buf = record.buffer + local len = buf:len() + -- Snapshot writes bypass the read-only intercept; everything else is + -- rejected by it. + if len > 0 then buf:delete(0, len, { bypass_intercept = true }) end + if #text > 0 then buf:insert(0, text, { bypass_intercept = true }) end +end + +local function ensure_snapshot(term_buf) + local name = snapshot_name_for(term_buf) + local record = snapshots[name] + if record and record.buffer:is_valid() then + -- Q#TC8: re-invoking refreshes IN PLACE. Retarget the terminal too, + -- in case a terminal buffer was recreated under the same name. + record.terminal = term_buf + return record + end + + local buf = find_buffer_by_name(name) or pmacs.buffer.create(name) + record = { terminal = term_buf, buffer = buf } + snapshots[name] = record + + -- Q#TC6a — BOTH calls, and the second is the load-bearing one. + -- + -- An intercept guards the dispatch/edit path only. It does NOT set + -- `Buffer::read_only` (deliberately independent), and no Lua binding + -- sets that flag at all, so an optimistic CRDT op from a semantic + -- frontend bypasses the intercept AND passes `ensure_writable()` — + -- mutating the daemon buffer in lockstep with the mirror, with no + -- divergence to notice. `set_round_trip_input` prevents that at the + -- only point it can be prevented: `dispatch_idle_for` reports false + -- while this buffer is focused, so the frontend never applies + -- optimistically and never emits the op. It is the guard, not + -- hardening. + pmacs.buffer.add_intercept(buf, function() + error(name .. " is read-only") + end) + pmacs.buffer.set_round_trip_input(buf, true) + + pmacs.keymap.bind { scope = "buffer", buffer = buf, + sequence = "g", command = "terminal.copy-refresh" } + pmacs.keymap.bind { scope = "buffer", buffer = buf, + sequence = "q", command = "terminal.copy-quit" } + + -- Q#TC8 lifecycle, both directions. Killing the terminal takes its + -- snapshot with it; killing the snapshot alone leaves the terminal + -- running and merely forgets the record, so a later invoke rebuilds. + -- + -- `on_removed` is sound here because every user-facing kill path + -- routes through `pmacs.buffer.kill`, which fires the callbacks. The + -- terminal manager's own `prune` does not — but it never removes a + -- buffer either; it REACTS to one already gone from the registry. A + -- child exiting therefore leaves both the terminal and its snapshot + -- alive, which is what makes reading back a finished command's output + -- work at all. + pcall(pmacs.buffer.on_removed, term_buf, function() + local current = snapshots[name] + if current and current.buffer:is_valid() then + pcall(pmacs.buffer.kill, current.buffer) + end + snapshots[name] = nil + end) + pcall(pmacs.buffer.on_removed, buf, function() + snapshots[name] = nil + end) + + return record +end + +-- The snapshot record whose buffer the active window shows, or nil. +local function snapshot_for_current_buffer() + local buf = pmacs.window.buffer() + if not buf then return nil end + local name = buffer_name(buf) + if not name then return nil end + return snapshots[name] +end + +function terminal.copy_mode(term_buf) + term_buf = term_buf or pmacs.window.buffer() + assert(term_buf, "terminal.copy-mode: no active buffer") + if not terminal.is_terminal(term_buf) then + error("terminal.copy-mode: the current buffer is not a terminal", 0) + end + local record = ensure_snapshot(term_buf) + render_snapshot(record) + pmacs.window.switch_buffer(record.buffer) + return record.buffer +end + +pmacs.command.define { + name = "terminal.copy-mode", + description = "Open a searchable read-only snapshot of this terminal's scrollback.", + fn = function() return terminal.copy_mode() end, +} + +pmacs.command.define { + name = "terminal.copy-refresh", + description = "Re-snapshot the source terminal into this copy buffer.", + fn = function() + local record = snapshot_for_current_buffer() + if not record then return end + if not record.terminal:is_valid() then + pmacs.editor.set_status("terminal.copy-refresh: the source terminal is gone") + return + end + render_snapshot(record) + end, +} + +pmacs.command.define { + name = "terminal.copy-quit", + description = "Return to the terminal this copy buffer was taken from.", + fn = function() + local record = snapshot_for_current_buffer() + if not record then return end + if record.terminal:is_valid() then + pmacs.window.switch_buffer(record.terminal) + end + end, +} + pmacs.command.define { name = "terminal.copy-selection", description = "Copy the active terminal selection.", diff --git a/docs/active-work.md b/docs/active-work.md index 88e70df..bfdb7ff 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -389,19 +389,52 @@ If it does not, stop and repair the remote/fetch configuration. **isolated-config workspace sweep 3,177 across 92 suites, zero failures**; `git diff --check` clean. Gates were run against the committed tree. -## Terminal config + copy mode arc — Stage 1 IN REVIEW +## Terminal config + copy mode arc — Stage 1 MERGED; Stage 2 IN REVIEW - Approved framing: `docs/terminal-config-and-copy-mode-framing.md` **revision 4** (four review rounds), committed as the first commit of Stage 1's branch. Two stages, two branches, two PRs; **no protocol change**. -- **Stage 1 = `githubsucks/terminal-config`**, worktree - `../pmacs-terminal-config`, based on `githubsucks/main` @ `d152120` - and merged up to `c93f9ee` during review round 1. Profiles, - scrollback, escape key, and the `C-c t` opening binding. -- **Stage 2 = `terminal-copy-mode`, not started.** Branch it off `main` - after Stage 1 merges: no dependency, but both edit - `builtin/runtime/terminal.lua`. +- **Stage 1 MERGED as #173** (`main` @ `cf54270`, 2026-07-26, one review + round, twelve checks green). Branch `githubsucks/terminal-config` and + worktree `../pmacs-terminal-config` retained. +- **Stage 2 = `githubsucks/terminal-copy-mode`**, worktree + `../pmacs-terminal-copy-mode`, based on `githubsucks/main` @ + `cf54270`. Copy mode: `M-x terminal.copy-mode` / `C-c C-t`. +- **Stage 2 ships eight of nine criteria, and the missing one is named.** + Criterion 17 (a real semantic frontend proving neither daemon buffer + nor mirror mutates) is **not pinned**: the optimistic apply exists only + in `pmacs-gpu/src/main.rs`, and the headless `SemanticClient` every + other semantic test uses has no optimistic path, so a faithful test + must drive the real GPU binary — the `a37` foundation, which CI never + compiles, silently skips without the binary, and is load-sensitive. A + second test on that footing buys the appearance of coverage. Both + halves of the mechanism are pinned **ungated** instead: acceptance 16 + (the guard is armed — `dispatch_idle` false while the snapshot is + focused) and 16b (the hazard is real — the snapshot's `is_read_only()` + is **false** despite the intercept, so nothing at the rope/CRDT + boundary would stop an op that did arrive). The wire-level half is 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. - 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`; diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index 48b75d8..68a88dd 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -1,9 +1,16 @@ # Terminal configuration and copy mode **Revision 4 — scouted against canonical `main` @ `b889873` (protocol v20), -2026-07-25. APPROVED after four review rounds. Stage 1 is implemented on -branch `terminal-config` (PR #173); Stage 2 (`terminal-copy-mode`) is -framed but not started, and branches off `main` after Stage 1 merges.** +2026-07-25. APPROVED after four review rounds. Stage 1 MERGED as #173 +(`main` @ `cf54270`, 2026-07-26). Stage 2 implemented on branch +`terminal-copy-mode` off `main` @ `cf54270`; no protocol change.** + +**Stage 2 ships eight of its nine criteria.** Criterion 17's semantic-frontend +end-to-end pin is deliberately absent — see the note under it — because a +faithful version requires the real `pmacs-gpu` optimistic path, and therefore +the `a37` foundation, which CI never compiles and which skips silently. Both +halves of the *mechanism* it guards are pinned ungated instead (16, 16b). No +other criterion is partial. Revision 4 gives the escape-key cache an owner and a lifecycle (Q#TC4c) — revision 3 named the key but not the storage, and two implementations @@ -576,6 +583,28 @@ additive, on its own binding, and does not replace scroll-and-select. emitted, bypasses the Lua intercept, passes `ensure_writable()`, and mutates **both sides** — a buffer the editor calls read-only silently accepts an edit. + + **NOT PINNED as specified, deliberately, and this is the one gap in + Stage 2.** A faithful test has to drive the *real* `pmacs-gpu` binary: + the optimistic apply lives only in `pmacs-gpu/src/main.rs` + (`optimistic_crdt_insert` / `optimistic_insert_text`), and the headless + `SemanticClient` the other semantic tests use has no optimistic path at + all, so it cannot produce the op whose absence is the claim. That means + building on the `a37` foundation — which is `crdt`-gated so CI never + compiles it, **returns `ok` without running** when `pmacs-gpu` is absent + from the target directory, and is load-sensitive enough to pass and fail + at the same commit twenty minutes apart. A second test on that footing + would add the appearance of coverage without the substance. + + What IS pinned instead, ungated and in CI: acceptance 16 asserts the + guard is armed (`dispatch_idle` false while the snapshot is focused, so + no replica can apply optimistically or emit), and acceptance 16b asserts + the hazard is real by showing the snapshot buffer's `is_read_only()` is + **false** despite the intercept — i.e. nothing at the rope/CRDT boundary + would stop such an op if one arrived. Together those cover both halves of + Q#TC6a's *mechanism*. What remains unproven is only the end-to-end wire + behaviour of a real GPU frontend, and it stays an explicit obligation of + the CI `crdt`-coverage lane rather than being quietly dropped. 18. Re-invoking against the same terminal refreshes in place; the buffer count does not grow (Q#TC8). Killing the snapshot leaves the terminal running; killing the terminal removes the snapshot. diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 1e7ac04..37a1307 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -8836,6 +8836,25 @@ fn install_terminal( )?; } + { + let manager = manager.clone(); + terminal.set( + "_copy_retained", + // Q#TC7: returns the whole retained range as a string, through + // the same serializer selection-copy uses. Takes an explicit + // buffer rather than resolving the active view, because copy + // mode reads a terminal that may not be displayed — and + // because the caller already holds the handle it keyed its + // snapshot on. + lua.create_function(move |lua, buffer: BufferIdLua| { + let Some(bytes) = manager.borrow().copy_retained(buffer.0) else { + return Ok(None); + }; + Ok(Some(lua.create_string(&bytes)?)) + })?, + )?; + } + pmacs.set("terminal", terminal) } diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 1c0957d..b787dee 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -329,6 +329,29 @@ impl TerminalManager { copy_selection_bytes(&rows, selection) } + /// Serialize a session's ENTIRE retained range — scrollback plus the + /// visible screen — through the same path [`copy_selection`] uses. + /// + /// Q#TC7. This deliberately builds a whole-range *selection* and hands + /// it to the existing serializer rather than walking the rows itself. + /// Soft-wrap joining, wide-glyph continuation, cluster bytes, and + /// per-row trailing-blank trimming are Vterm Stage 2 criterion 21's + /// pinned behavior; a second walk would re-derive all four and the two + /// would drift. That inheritance is what acceptance 13 asserts, by + /// comparing this against a full-range `copy_selection` rather than + /// against a literal. + /// + /// Returns `None` for a non-terminal buffer and for a session whose + /// retained rows are all empty — there is no cell to anchor to. + /// Unlike `copy_selection` this needs no registered view, so copy mode + /// does not depend on the terminal being currently displayed. + #[must_use] + pub fn copy_retained(&self, buffer_id: BufferId) -> Option> { + let session = self.sessions.get(&buffer_id)?; + let projection = session.screen.projection_ref(); + retained_bytes(&retained_rows(projection)) + } + /// Start an editor-owned primary selection at a viewport coordinate. pub fn begin_selection( &mut self, @@ -540,6 +563,40 @@ fn retained_rows(projection: BorrowedScreenProjection<'_>) -> RetainedRows<'_> { RetainedRows { projection } } +/// Serialize every retained cell, through the selection-copy serializer. +/// +/// Split out from [`TerminalManager::copy_retained`] so the fidelity +/// claims — soft-wrap joining, per-row trailing-blank trimming, wide-glyph +/// continuation, cluster bytes — are testable against the same projection +/// fixtures that pin `copy_selection_bytes` itself. Those four are exactly +/// what a second, independently written walk would get wrong. +fn retained_bytes(rows: &RetainedRows<'_>) -> Option> { + copy_selection_bytes(rows, full_retained_selection(rows)?) +} + +/// The selection spanning every retained cell. +/// +/// Rows with no cells are skipped at both ends rather than clamped: an +/// anchor into a zero-width row cannot resolve (`resolve_anchor` requires +/// `cell_offset` to fall inside `cell_offset .. cell_offset + len`), so +/// including one would make the whole range unresolvable and silently +/// yield nothing. Interior empty rows are untouched, because trailing- and +/// interior-blank handling belongs to the serializer. +fn full_retained_selection(rows: &RetainedRows<'_>) -> Option { + let mut occupied = rows.iter().filter(|row| !row.cells.is_empty()); + let first = occupied.next()?; + // `RetainedRows::iter` is a chain of slice iterators exposed as + // `impl Iterator`, so it is not double-ended; scan forward. + let last = occupied.last().unwrap_or(first); + Some(TerminalSelection { + anchor: row_lead(first), + head: LogicalCellAnchor { + logical_line_id: last.logical_line_id, + cell_offset: last.cell_offset.saturating_add(last.cells.len() as u32 - 1), + }, + }) +} + fn row_lead(row: &TerminalRow) -> LogicalCellAnchor { LogicalCellAnchor { logical_line_id: row.logical_line_id, @@ -1001,6 +1058,92 @@ mod tests { assert_eq!(bytes, b"abcd\ne"); } + /// Stage 2 criteria 13 and 14. Every property here is one a second, + /// independently written whole-range walk would get wrong: a naive + /// walk emits a newline per physical row (breaking the soft wrap), + /// keeps trailing default blanks, and has to rediscover that history + /// precedes the visible screen. Asserting exact bytes is what makes + /// "it reuses the serializer" falsifiable. + #[test] + fn retained_copy_spans_history_joins_soft_wraps_and_trims_blanks() { + let source = projection( + vec![row(1, 0, "ab ", true), row(1, 3, "cd ", false)], + vec![row(2, 0, "e ", false), row(3, 0, " ", false)], + ); + let retained = retained_rows(source.as_borrowed()); + let bytes = retained_bytes(&retained).expect("whole range resolves"); + // `ab`+`cd` joined across the soft wrap; `e` on its own hard row; + // the all-blank final row trimmed to nothing but still separated. + assert_eq!(bytes, b"abcd\ne\n"); + } + + /// The whole-range selection must not depend on a view existing, and + /// must agree with an explicit full-span selection through the public + /// serializer — the anti-drift half of criterion 13. + #[test] + fn retained_copy_agrees_with_an_explicit_full_span_selection() { + let source = projection( + vec![row(1, 0, "aaa", false)], + vec![row(2, 0, "bbb", false), row(3, 0, "ccc", false)], + ); + let retained = retained_rows(source.as_borrowed()); + let explicit = copy_selection_bytes( + &retained, + TerminalSelection { + anchor: LogicalCellAnchor { + logical_line_id: 1, + cell_offset: 0, + }, + head: LogicalCellAnchor { + logical_line_id: 3, + cell_offset: 2, + }, + }, + ) + .expect("explicit selection resolves"); + assert_eq!(retained_bytes(&retained).expect("whole range"), explicit); + assert_eq!(explicit, b"aaa\nbbb\nccc"); + } + + /// A wide glyph must be copied once across the whole range too, not + /// once per cell it occupies. + #[test] + fn retained_copy_emits_a_wide_glyph_once() { + let wide = TerminalRow { + cells: vec![ + Cell { + glyph: Glyph::Char('界'), + style: Style::default(), + attachment: None, + }, + Cell { + glyph: Glyph::Continuation, + style: Style::default(), + attachment: None, + }, + Cell::default(), + ], + logical_line_id: 9, + cell_offset: 0, + soft_wrapped: false, + }; + let source = projection(Vec::new(), vec![wide]); + let retained = retained_rows(source.as_borrowed()); + assert_eq!( + retained_bytes(&retained).expect("whole range"), + "界".as_bytes() + ); + } + + /// A session with nothing retained yields `None` rather than an empty + /// string, so the caller can tell "no terminal" from "empty terminal". + #[test] + fn retained_copy_of_zero_width_rows_is_none() { + let source = projection(Vec::new(), vec![row(1, 0, "", false)]); + let retained = retained_rows(source.as_borrowed()); + assert!(retained_bytes(&retained).is_none()); + } + #[test] fn wide_continuation_canonicalizes_to_lead_and_copies_once() { let wide = TerminalRow { diff --git a/tests/terminal_copy_mode_acceptance.rs b/tests/terminal_copy_mode_acceptance.rs new file mode 100644 index 0000000..5b62ad8 --- /dev/null +++ b/tests/terminal_copy_mode_acceptance.rs @@ -0,0 +1,516 @@ +//! Terminal copy-mode acceptance (Stage 2 of +//! `docs/terminal-config-and-copy-mode-framing.md`, criteria 13-21). +//! +//! **Deliberately NOT `#[cfg(feature = "crdt")]`.** CI never enables that +//! feature, so a gated suite is written and then never run — 264 tests are +//! dark workspace-wide for exactly that reason. Criterion 16, the +//! round-trip gate Q#TC6a's entire safety argument rests on, needs no CRDT +//! and must be caught by the default configuration. + +use std::thread; +use std::time::{Duration, Instant}; + +use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; +use mlua::Value; +use pmacs::cell::{CellSize, Glyph}; +use pmacs::editor::EditorState; +use pmacs::protocol::FrontendId; +use pmacs::terminal::TerminalViewKey; + +const SNAPSHOT_NAME: &str = "*terminal-copy: terminal:sh*"; + +fn exec(state: &EditorState, src: &str) { + state + .lua_host + .lua() + .load(src) + .exec() + .unwrap_or_else(|e| panic!("lua failed: {src}\n{e}")); +} + +fn eval(state: &EditorState, src: &str) -> T { + state + .lua_host + .lua() + .load(src) + .eval() + .unwrap_or_else(|e| panic!("lua eval failed: {src}\n{e}")) +} + +fn eval_err(state: &EditorState, src: &str) -> String { + let result: mlua::Result = state.lua_host.lua().load(src).eval(); + match result { + Ok(_) => panic!("expected an error from: {src}"), + Err(e) => e.to_string(), + } +} + +fn press(state: &mut EditorState, code: KeyCode, mods: KeyModifiers) { + state.dispatch_key(FrontendId::LOCAL, KeyEvent::new(code, mods)); +} + +/// The live terminal screen's text, used only to wait for the child. +fn screen_text(state: &EditorState, buffer: pmacs::buffer::BufferId) -> String { + let manager = state.terminal_manager.borrow(); + let Some(snapshot) = manager.snapshot(buffer) else { + return String::new(); + }; + let mut text = String::new(); + for cell in &snapshot.cells { + match &cell.glyph { + Glyph::Char(c) => text.push(*c), + Glyph::Cluster(b) => text.push_str(&String::from_utf8_lossy(b)), + Glyph::Continuation => {} + } + } + text +} + +fn tick_until(state: &mut EditorState, needle: &str, buffer: pmacs::buffer::BufferId) -> bool { + let deadline = Instant::now() + Duration::from_secs(5); + loop { + state.tick_processes(); + if screen_text(state, buffer).contains(needle) { + return true; + } + if Instant::now() >= deadline { + return false; + } + thread::sleep(Duration::from_millis(20)); + } +} + +fn terminal_buffers(state: &EditorState) -> Vec { + let manager = state.terminal_manager.borrow(); + state + .core + .borrow() + .registry + .borrow() + .ids() + .iter() + .copied() + .filter(|id| manager.is_terminal(*id)) + .collect() +} + +/// A child that overflows the 24-row screen and then goes quiet, so its +/// early lines exist ONLY in scrollback — which is what makes criterion +/// 15's "content only in scrollback" claim meaningful. +const FILL_PROFILE: &str = r#" +pmacs.terminal.profiles.fill = { + command = "/bin/sh", + args = { "-c", + "printf 'NEEDLE-IN-SCROLLBACK\r\n'; i=1; while [ $i -le 200 ]; do printf 'LINE%03d\r\n' $i; i=$((i+1)); done; printf 'DONE\r\n'; exec cat" }, +} +"#; + +/// Open the fill terminal, wait for the child to finish, and return its id. +fn open_fill_terminal(state: &mut EditorState) -> pmacs::buffer::BufferId { + exec(state, FILL_PROFILE); + let before = terminal_buffers(state); + exec( + state, + r#"TERM_BUF = pmacs.terminal.open { profile = "fill" }"#, + ); + let fresh: Vec<_> = terminal_buffers(state) + .into_iter() + .filter(|id| !before.contains(id)) + .collect(); + assert_eq!(fresh.len(), 1, "exactly one terminal must have opened"); + let buffer = fresh[0]; + assert!(tick_until(state, "DONE", buffer), "the child must finish"); + buffer +} + +/// Give LOCAL a window on the terminal and register/claim its view, which +/// is what makes `dispatch_key`'s terminal transport arm reachable. +fn focus_terminal(state: &EditorState, buffer: pmacs::buffer::BufferId) { + state.core.borrow_mut().switch_active_buffer(buffer).ok(); + let window = state.core.borrow().active_window_id(); + let key = TerminalViewKey::new(FrontendId::LOCAL, window, buffer); + let mut manager = state.terminal_manager.borrow_mut(); + manager.register_view(key); + manager.claim_controller(key); + let _ = manager.snapshot_for_view(key, CellSize::new(10, 40)); +} + +fn buffer_text_by_name(state: &EditorState, name: &str) -> Option { + eval( + state, + &format!( + r" + for _, id in ipairs(pmacs.buffer.list()) do + local ok, d = pcall(pmacs.describe.buffer, id) + if ok and d and d.name == {name:?} then + return id:slice(0, id:len()) + end + end + return nil + " + ), + ) +} + +fn active_buffer_name(state: &EditorState) -> String { + eval( + state, + r"local b = pmacs.window.buffer(); return (pmacs.describe.buffer(b)).name", + ) +} + +fn buffer_count(state: &EditorState) -> usize { + state.core.borrow().registry.borrow().ids().len() +} + +/// Acceptance 13: the snapshot's text is exactly the whole retained range +/// as the existing copy path serializes it. +/// +/// Compared against `_copy_retained` rather than a literal, so this cannot +/// pass by both sides drifting the same way; the exact-bytes fidelity +/// claims (criterion 14) are pinned at the unit level in +/// `src/terminal/view.rs`, against the same projection fixtures that pin +/// `copy_selection_bytes` itself. +#[test] +fn acc13_snapshot_is_the_whole_retained_range_through_the_shared_serializer() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + + exec(&state, "SNAP = pmacs.terminal.copy_mode(TERM_BUF)"); + let snapshot_text = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot buffer exists"); + let serialized: String = eval( + &state, + r"return pmacs.terminal._copy_retained(TERM_BUF) or ''", + ); + + assert_eq!( + snapshot_text, serialized, + "the snapshot must be byte-identical to the shared serializer's output" + ); + assert!( + snapshot_text.contains("NEEDLE-IN-SCROLLBACK") && snapshot_text.contains("LINE200"), + "the range must span scrollback AND the visible screen" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 14 (end-to-end half): the snapshot really is a rope-backed +/// document buffer and not a terminal, which is what makes every +/// buffer-shaped consumer work and what removes the transport arm. +#[test] +fn acc14_the_snapshot_is_an_ordinary_non_terminal_buffer() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + let is_terminal: bool = eval( + &state, + r"local b = pmacs.window.buffer(); return pmacs.terminal.is_terminal(b)", + ); + assert!( + !is_terminal, + "the snapshot must NOT be a terminal — that is what structurally \ + removes the transport arm rather than guarding it" + ); + assert_eq!(active_buffer_name(&state), SNAPSHOT_NAME); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 15: isearch finds content that exists ONLY in scrollback, +/// with no change to `src/search.rs` (B1). +#[test] +fn acc15_isearch_finds_content_only_in_scrollback() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + + // The needle is off the visible screen: the live terminal cannot see it. + assert!( + !screen_text(&state, terminal).contains("NEEDLE-IN-SCROLLBACK"), + "precondition: the needle must have scrolled off the live screen" + ); + + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + state.core.borrow_mut().set_cursor_byte(0); + + // Drive real isearch: C-s then the needle. + press(&mut state, KeyCode::Char('s'), KeyModifiers::CONTROL); + for ch in "NEEDLE-IN-SCROLLBACK".chars() { + press(&mut state, KeyCode::Char(ch), KeyModifiers::NONE); + } + let cursor = state.core.borrow().cursor(); + press(&mut state, KeyCode::Enter, KeyModifiers::NONE); + + let text = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); + let expected = text + .find("NEEDLE-IN-SCROLLBACK") + .expect("the needle is in the snapshot") as u64; + assert_eq!( + cursor, + expected, + "isearch must land on the scrollback-only match; text was {:?}", + &text[..text.len().min(80)] + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 16 — the load-bearing pin, and the reason this suite is +/// ungated. `set_round_trip_input` is the ONLY thing standing between a +/// replica frontend and unauthorized mutation (Q#TC6a), so its regression +/// must be caught in the configuration CI actually compiles. +#[test] +fn acc16_dispatch_idle_is_false_while_the_snapshot_is_focused() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + assert!( + !state.dispatch_idle(), + "a focused snapshot must round-trip keys, so no replica applies \ + optimistically and none emits a CRDT op" + ); + // ...and it is the SNAPSHOT that does it, not merely "some terminal + // buffer is around": switching to an ordinary buffer restores idle. + exec( + &state, + r#"pmacs.window.switch_buffer(pmacs.buffer.create("*plain*"))"#, + ); + assert!(state.dispatch_idle(), "an ordinary buffer is idle again"); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 16 (the other half): the intercept rejects ordinary edits, +/// and — the fact that makes round-trip load-bearing rather than defence +/// in depth — the buffer is **not** `read_only` at the rope boundary. +#[test] +fn acc16b_the_intercept_rejects_edits_but_is_not_rope_level_protection() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + let before = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); + press(&mut state, KeyCode::Char('z'), KeyModifiers::NONE); + let after = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); + assert_eq!(before, after, "the read-only intercept rejects self-insert"); + + // Q#TC6a, stated as a test so the next reader does not mistake the + // intercept for real immutability: no Lua binding sets + // `Buffer::read_only`, so this buffer accepts rope/CRDT mutation and + // only the round-trip mark above keeps a replica from producing one. + let core = state.core.borrow(); + let registry = core.registry.borrow(); + let ids = registry.ids(); + let snapshot = ids + .iter() + .copied() + .find(|id| { + registry + .get(*id) + .is_ok_and(|buf| buf.name() == SNAPSHOT_NAME) + }) + .expect("snapshot buffer id"); + assert!( + !registry + .get(snapshot) + .expect("snapshot buffer") + .is_read_only(), + "the Lua intercept does NOT set Buffer::read_only — this is why \ + set_round_trip_input is the guard and not hardening" + ); + drop(registry); + drop(core); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 18: re-invoking refreshes in place, and the lifecycle runs +/// both directions. +#[test] +fn acc18_reinvoke_refreshes_in_place_and_lifecycle_runs_both_ways() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + let count_after_first = buffer_count(&state); + + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + assert_eq!( + buffer_count(&state), + count_after_first, + "re-invoking must refresh in place, not accumulate buffers" + ); + + // Killing the snapshot alone leaves the terminal running. + exec( + &state, + &format!( + r" + for _, id in ipairs(pmacs.buffer.list()) do + local ok, d = pcall(pmacs.describe.buffer, id) + if ok and d and d.name == {SNAPSHOT_NAME:?} then pmacs.buffer.kill(id) end + end + " + ), + ); + assert!( + state.terminal_manager.borrow().is_terminal(terminal), + "killing the snapshot must leave the terminal untouched" + ); + assert!( + buffer_text_by_name(&state, SNAPSHOT_NAME).is_none(), + "the snapshot buffer is gone" + ); + + // ...and it can be rebuilt afterwards. + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + assert!( + buffer_text_by_name(&state, SNAPSHOT_NAME).is_some(), + "a later invoke rebuilds the snapshot" + ); + + // Killing the terminal takes its snapshot with it. + exec(&state, "pmacs.terminal.terminate(TERM_BUF)"); + exec(&state, "pmacs.buffer.kill(TERM_BUF)"); + assert!( + buffer_text_by_name(&state, SNAPSHOT_NAME).is_none(), + "killing the terminal must remove its snapshot" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 19: `C-t` in a terminal — physically `C-c C-t`, because every +/// unescaped key goes to the child — enters copy mode; `g` refreshes and +/// `q` returns to the source terminal. +#[test] +fn acc19_escape_c_t_enters_copy_mode_and_g_and_q_work() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + let terminal_name = active_buffer_name(&state); + + // The escape, then the terminal-local binding. + press(&mut state, KeyCode::Char('c'), KeyModifiers::CONTROL); + press(&mut state, KeyCode::Char('t'), KeyModifiers::CONTROL); + assert_eq!( + active_buffer_name(&state), + SNAPSHOT_NAME, + "C-c C-t must enter copy mode" + ); + + // `g` re-snapshots in place. + let before = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); + press(&mut state, KeyCode::Char('g'), KeyModifiers::NONE); + let after = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); + assert_eq!(before, after, "a quiet terminal re-snapshots identically"); + assert_eq!( + active_buffer_name(&state), + SNAPSHOT_NAME, + "g must not move us" + ); + + // `q` returns to the source terminal. + press(&mut state, KeyCode::Char('q'), KeyModifiers::NONE); + assert_eq!( + active_buffer_name(&state), + terminal_name, + "q must return to the terminal the snapshot was taken from" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 20: copy mode is additive — the live terminal's own keys are +/// unchanged while a snapshot exists, and the terminal still follows its +/// tail. +#[test] +fn acc20_live_terminal_keys_are_unchanged_while_a_snapshot_exists() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + // Back to the terminal; its five live bindings must still resolve. + exec(&state, "pmacs.window.switch_buffer(TERM_BUF)"); + for (sequence, command) in [ + ("M-w", "terminal.copy-selection"), + ("M-v", "terminal.page-up"), + ("C-v", "terminal.page-down"), + ("M-<", "terminal.scroll-oldest"), + ("M->", "terminal.scroll-bottom"), + ] { + let resolved: Option = eval( + &state, + &format!(r"local d = pmacs.describe.key({sequence:?}); return d and d.command"), + ); + assert_eq!( + resolved.as_deref(), + Some(command), + "{sequence} must still be the live terminal binding" + ); + } + + // The terminal is still following its tail: the child's last output is + // visible without scrolling. + assert!( + screen_text(&state, terminal).contains("DONE"), + "the live terminal keeps following its tail" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 21: the dispatch-shadow count is unchanged at six, pinned by +/// the observable difference between a buffer-local keymap and a shadow — +/// `describe-key` telling the truth about `g` and `q` in the snapshot. +/// +/// A seventh shadow would decode these keys before `KeymapStack::resolve` +/// ever ran, so introspection would report whatever the global binding is +/// (or nothing) while the keys behaved differently. +#[test] +fn acc21_describe_key_reports_the_truth_for_the_snapshot_bindings() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + for (sequence, command) in [("g", "terminal.copy-refresh"), ("q", "terminal.copy-quit")] { + let resolved: Option = eval( + &state, + &format!(r"local d = pmacs.describe.key({sequence:?}); return d and d.command"), + ); + assert_eq!( + resolved.as_deref(), + Some(command), + "describe-key must report the buffer-local {sequence} binding" + ); + } + + // And the binding really is scoped: back in the terminal, `q` is not + // the copy-mode command. + exec(&state, "pmacs.window.switch_buffer(TERM_BUF)"); + let resolved: Option = eval( + &state, + r#"local d = pmacs.describe.key("q"); return d and d.command"#, + ); + assert_ne!( + resolved.as_deref(), + Some("terminal.copy-quit"), + "the snapshot's q must not leak into the terminal buffer" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Copy mode refuses a non-terminal buffer rather than producing an empty +/// snapshot of nothing. +#[test] +fn copy_mode_refuses_a_non_terminal_buffer() { + let state = EditorState::new(); + let err = eval_err(&state, "return pmacs.terminal.copy_mode()"); + assert!( + err.contains("not a terminal"), + "the refusal must say why: {err}" + ); +} From 2eb6218ccd3400e3aca640898b29c00b8055f3a6 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 10:45:32 -0400 Subject: [PATCH 14/18] fix(terminal): close review round 1 on copy mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, all real, and they rhyme in pairs. Two implementation defects and two vacuous pins, all four tracing to one root: a name is not an identity, and a context-free readout is not a state observation. A foreign buffer carrying the snapshot's name was adopted and then overwritten. `pmacs.buffer.create` accepts any caller-chosen name and snapshot writes use bypass_intercept, so found-by-name adoption clobbered user data — the reviewer reproduced "do not clobber" becoming 23 newlines. Now follows dired's F7 rule: ownership means "in copy mode's own handle table", never "found by name", and a taken name yields a `<2>` variant. Snapshot identity was keyed by terminal NAME. `TerminalManager::open` uniquifies only the derived name — an explicit `name = ...` is inserted verbatim — so two valid terminals can share one, and a name-keyed table handed them a single snapshot: the second invocation retargeted it, `q` returned to the wrong terminal, and killing either removed the shared buffer. Identity is now the terminal buffer, compared in an array, because BufferIdLua implements `__eq` but each wrapper is a distinct table key: comparison works, hashing does not. The kill-with-terminal callback now closes over its own record rather than looking the name up again. The refresh pins were vacuous. Acceptance 19 compared a quiet terminal's snapshot against itself and 18 counted buffers, so both passed with render_snapshot replaced by a no-op. The child is `exec cat`, so the tests now type a marker into the focused terminal, require it ABSENT from the existing snapshot, and only then refresh — via `g` and via re-invocation respectively. The tail-follow pin could not observe view state. `TerminalManager::snapshot(buffer_id)` is context-free and always returns the live screen, so it reported "at the tail" even for a view forced to the oldest retained row. Now read through `snapshot_for_view`'s at_bottom and its projected cells. Adds acceptance 18a (a foreign same-named buffer is never adopted or clobbered) and 18b (two same-named terminals get two independent snapshots, each `q` returns to its own source, and killing one leaves the other's snapshot alive). Four new 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, which is the vacuity demonstrated rather than argued; and forcing the view off the tail fails 20. Criterion 17 stays a named follow-up, per review agreement, until the real GPU probe is non-skipping and CI-executed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer --- builtin/runtime/terminal.lua | 125 +++++--- docs/active-work.md | 29 ++ docs/terminal-config-and-copy-mode-framing.md | 48 ++- tests/terminal_copy_mode_acceptance.rs | 290 ++++++++++++++++-- 4 files changed, 432 insertions(+), 60 deletions(-) diff --git a/builtin/runtime/terminal.lua b/builtin/runtime/terminal.lua index b44e824..f2eca85 100644 --- a/builtin/runtime/terminal.lua +++ b/builtin/runtime/terminal.lua @@ -221,11 +221,56 @@ pmacs.keymap.bind { scope = "global", sequence = "C-c t", command = "terminal" } local raw_copy_retained = assert(terminal._copy_retained, "pmacs.terminal._copy_retained is required") --- snapshot buffer name -> { terminal = , buffer = } +-- An ARRAY of `{ terminal = , buffer = }`, scanned linearly and +-- compared with `==`, following dired's handle table (F7). -- --- Keyed by NAME, not by buffer handle: handles are not stable table keys, --- and a name survives the user killing the snapshot (listview precedent). -local snapshots = {} +-- Not `snapshots[name]`, and not `snapshots[buf]`, for two separate +-- reasons — both of which were live defects in review round 1: +-- +-- * **A terminal name is not a unique key.** `TerminalManager::open` +-- uniquifies only the DERIVED name; an explicitly passed +-- `name = "*same*"` is inserted verbatim +-- (`src/terminal/session.rs`, `if spec.name.is_some()`). Two valid +-- terminals can therefore share a name, and a name-keyed table gives +-- them one snapshot between them: the second invocation silently +-- retargets it, `q` returns to the wrong terminal, and killing either +-- one removes the shared buffer. +-- * **A buffer handle is not a stable table key.** `BufferIdLua` +-- implements `__eq` but each wrapper is a distinct table key, so +-- `snapshots[buf]` would miss on a freshly minted handle for the same +-- buffer. Comparison works; hashing does not. Hence the scan. +local handles = {} + +-- Compact dead entries first, so a command in a killed snapshot sees +-- "not in copy mode" rather than operating on dead state. +local function live_handles() + local live = {} + for _, h in ipairs(handles) do + local term_ok, term_valid = pcall(h.terminal.is_valid, h.terminal) + local snap_ok, snap_valid = pcall(h.buffer.is_valid, h.buffer) + if term_ok and term_valid and snap_ok and snap_valid then + live[#live + 1] = h + end + end + handles = live + return live +end + +local function handle_for_terminal(term_buf) + if term_buf == nil then return nil end + for _, h in ipairs(live_handles()) do + if h.terminal == term_buf then return h end + end + return nil +end + +local function handle_for_snapshot(buf) + if buf == nil then return nil end + for _, h in ipairs(live_handles()) do + if h.buffer == buf then return h end + end + return nil +end local function buffer_name(buf) local ok, described = pcall(pmacs.describe.buffer, buf) @@ -233,7 +278,7 @@ local function buffer_name(buf) return nil end -local function find_buffer_by_name(name) +local function buffer_named(name) for _, id in ipairs(pmacs.buffer.list()) do local ok, described = pcall(pmacs.describe.buffer, id) if ok and described and described.name == name then return id end @@ -244,11 +289,31 @@ end -- `*terminal:bash*` -> `*terminal-copy: terminal:bash*`. The surrounding -- asterisks are stripped before nesting so the result reads as one -- generated-buffer name rather than two. -local function snapshot_name_for(term_buf) +local function snapshot_base_name(term_buf) local name = buffer_name(term_buf) or "terminal" return string.format("*terminal-copy: %s*", (name:gsub("^%*", ""):gsub("%*$", ""))) end +-- How far the `<2>`, `<3>`, ... disambiguation walks before giving up. +local NAME_VARIANT_LIMIT = 99 + +-- `pmacs.buffer.create` takes any caller-chosen name, so a foreign buffer +-- may already be called `*terminal-copy: sh*` — and two same-named +-- terminals legitimately produce the same base name. Painting into a +-- buffer we did not create would clobber a user's data through +-- `bypass_intercept`, so **found-by-name is NOT adoption**: ownership +-- means "this buffer is in the handle table above", exactly as in dired. +local function unique_snapshot_name(term_buf) + local name = snapshot_base_name(term_buf) + if buffer_named(name) == nil then return name end + for i = 2, NAME_VARIANT_LIMIT do + local candidate = string.format("%s<%d>", name, i) + if buffer_named(candidate) == nil then return candidate end + end + error(string.format( + "terminal.copy-mode: %s is taken and no free variant remains", name), 0) +end + -- Q#TC7: the snapshot text comes from the SAME serializer selection-copy -- uses, so soft wraps, wide glyphs, clusters and trailing blanks cannot -- drift between the two. @@ -262,19 +327,17 @@ local function render_snapshot(record) if #text > 0 then buf:insert(0, text, { bypass_intercept = true }) end end -local function ensure_snapshot(term_buf) - local name = snapshot_name_for(term_buf) - local record = snapshots[name] - if record and record.buffer:is_valid() then - -- Q#TC8: re-invoking refreshes IN PLACE. Retarget the terminal too, - -- in case a terminal buffer was recreated under the same name. - record.terminal = term_buf - return record - end +local function claim_snapshot(term_buf) + -- Q#TC8: re-invoking against the same terminal refreshes IN PLACE. + -- Identity is the terminal BUFFER, so two same-named terminals get two + -- snapshots and neither can retarget the other's. + local existing = handle_for_terminal(term_buf) + if existing then return existing end - local buf = find_buffer_by_name(name) or pmacs.buffer.create(name) - record = { terminal = term_buf, buffer = buf } - snapshots[name] = record + local name = unique_snapshot_name(term_buf) + local buf = pmacs.buffer.create(name) + local record = { terminal = term_buf, buffer = buf } + handles[#handles + 1] = record -- Q#TC6a — BOTH calls, and the second is the load-bearing one. -- @@ -298,9 +361,11 @@ local function ensure_snapshot(term_buf) pmacs.keymap.bind { scope = "buffer", buffer = buf, sequence = "q", command = "terminal.copy-quit" } - -- Q#TC8 lifecycle, both directions. Killing the terminal takes its - -- snapshot with it; killing the snapshot alone leaves the terminal - -- running and merely forgets the record, so a later invoke rebuilds. + -- Q#TC8 lifecycle, both directions. Killing the terminal takes ITS + -- snapshot with it — `record`, captured here, not "whatever is + -- currently filed under this name"; killing the snapshot alone leaves + -- the terminal running, and `live_handles` compacts the entry out so a + -- later invoke rebuilds. -- -- `on_removed` is sound here because every user-facing kill path -- routes through `pmacs.buffer.kill`, which fires the callbacks. The @@ -310,14 +375,8 @@ local function ensure_snapshot(term_buf) -- alive, which is what makes reading back a finished command's output -- work at all. pcall(pmacs.buffer.on_removed, term_buf, function() - local current = snapshots[name] - if current and current.buffer:is_valid() then - pcall(pmacs.buffer.kill, current.buffer) - end - snapshots[name] = nil - end) - pcall(pmacs.buffer.on_removed, buf, function() - snapshots[name] = nil + local ok, valid = pcall(record.buffer.is_valid, record.buffer) + if ok and valid then pcall(pmacs.buffer.kill, record.buffer) end end) return record @@ -325,11 +384,7 @@ end -- The snapshot record whose buffer the active window shows, or nil. local function snapshot_for_current_buffer() - local buf = pmacs.window.buffer() - if not buf then return nil end - local name = buffer_name(buf) - if not name then return nil end - return snapshots[name] + return handle_for_snapshot(pmacs.window.buffer()) end function terminal.copy_mode(term_buf) @@ -338,7 +393,7 @@ function terminal.copy_mode(term_buf) if not terminal.is_terminal(term_buf) then error("terminal.copy-mode: the current buffer is not a terminal", 0) end - local record = ensure_snapshot(term_buf) + local record = claim_snapshot(term_buf) render_snapshot(record) pmacs.window.switch_buffer(record.buffer) return record.buffer diff --git a/docs/active-work.md b/docs/active-work.md index bfdb7ff..9ba9efa 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -435,6 +435,35 @@ If it does not, stop and repair the remote/fetch configuration. 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. - 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`; diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index 68a88dd..b7ced8e 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -5,12 +5,19 @@ (`main` @ `cf54270`, 2026-07-26). Stage 2 implemented on branch `terminal-copy-mode` off `main` @ `cf54270`; no protocol change.** -**Stage 2 ships eight of its nine criteria.** Criterion 17's semantic-frontend -end-to-end pin is deliberately absent — see the note under it — because a -faithful version requires the real `pmacs-gpu` optimistic path, and therefore -the `a37` foundation, which CI never compiles and which skips silently. Both -halves of the *mechanism* it guards are pinned ungated instead (16, 16b). No -other criterion is partial. +**Stage 2 ships eight of its nine criteria, plus 18a and 18b added in review +round 1.** Criterion 17's semantic-frontend end-to-end pin is deliberately +absent — see the note under it — because a faithful version requires the real +`pmacs-gpu` optimistic path, and therefore the `a37` foundation, which CI never +compiles and which skips silently. Both halves of the *mechanism* it guards are +pinned ungated instead (16, 16b). No other criterion is partial. + +**Review round 1 found four defects, and the pair of them rhymes.** Two were +implementation (18a's foreign-buffer clobber, 18b's name-keyed identity) and +two were vacuous pins (18/19's refresh, 20's tail-follow) — and all four trace +to the same root: **a name is not an identity, and a context-free readout is +not a state observation.** The name mistake produced both P1s; the readout +mistake produced both P2s. Revision 4 gives the escape-key cache an owner and a lifecycle (Q#TC4c) — revision 3 named the key but not the storage, and two implementations @@ -608,11 +615,40 @@ additive, on its own binding, and does not replace scroll-and-select. 18. Re-invoking against the same terminal refreshes in place; the buffer count does not grow (Q#TC8). Killing the snapshot leaves the terminal running; killing the terminal removes the snapshot. + + **The refresh half must be observed by CONTENT, not by buffer count** + (review round 1). Counting buffers, or comparing a quiet terminal's + snapshot against itself, passes with `render_snapshot` replaced by a + no-op. The child is `exec cat`, so the test types a marker into the + focused terminal, requires it **absent** from the existing snapshot, and + only then re-invokes — the "advance the world" discipline. +18a. **A foreign buffer carrying the snapshot's name is never adopted.** + `pmacs.buffer.create` accepts any caller-chosen name, and snapshot writes + use `bypass_intercept`, so found-by-name adoption silently overwrites a + user's data — reproduced in review round 1 as "do not clobber" becoming + 23 newlines. Ownership means **"in copy mode's own handle table"**, which + is dired's F7 rule; a taken name yields a `<2>` variant. +18b. **Snapshot identity is the terminal BUFFER, not its name.** + `TerminalManager::open` uniquifies only the *derived* name — an explicit + `name = ...` is inserted verbatim — so two valid terminals can share one. + A name-keyed table hands them a single snapshot: the second invocation + retargets it, `q` returns to the wrong terminal, and killing either one + removes the shared buffer. Keyed instead by comparing buffer handles in + an array, because `BufferIdLua` implements `__eq` but each wrapper is a + distinct table key — comparison works, hashing does not. 19. `C-t` in a terminal buffer (physically `C-c C-t`) enters copy mode; `g` refreshes the snapshot from the live terminal and `q` returns to the source terminal (Q#TC8a). 20. The live terminal's own keys are unchanged while a snapshot exists (Q#TC9), and the terminal keeps following its tail. + + **Tail-following must be read through the registered VIEW.** Review + round 1: `TerminalManager::snapshot(buffer_id)` is context-free and + always returns the live screen, so it reports "at the tail" even for a + view forced to the oldest retained row — falsified by doing exactly + that and watching the assertion still pass. `snapshot_for_view`'s + `at_bottom` plus its projected cells are the only observables that can + tell the two apart. 21. The dispatch-shadow count is **unchanged at six** — pinned by asserting `describe-key` reports the truth for the snapshot buffer's `g` and `q`, which is the observable difference between the buffer-local idiom and a diff --git a/tests/terminal_copy_mode_acceptance.rs b/tests/terminal_copy_mode_acceptance.rs index 5b62ad8..39c2753 100644 --- a/tests/terminal_copy_mode_acceptance.rs +++ b/tests/terminal_copy_mode_acceptance.rs @@ -123,16 +123,66 @@ fn open_fill_terminal(state: &mut EditorState) -> pmacs::buffer::BufferId { buffer } +fn viewport() -> CellSize { + CellSize::new(10, 40) +} + /// Give LOCAL a window on the terminal and register/claim its view, which /// is what makes `dispatch_key`'s terminal transport arm reachable. -fn focus_terminal(state: &EditorState, buffer: pmacs::buffer::BufferId) { +/// Returns the view key, so assertions can read the *projected* view +/// rather than the context-free live screen. +fn focus_terminal(state: &EditorState, buffer: pmacs::buffer::BufferId) -> TerminalViewKey { state.core.borrow_mut().switch_active_buffer(buffer).ok(); let window = state.core.borrow().active_window_id(); let key = TerminalViewKey::new(FrontendId::LOCAL, window, buffer); let mut manager = state.terminal_manager.borrow_mut(); manager.register_view(key); manager.claim_controller(key); - let _ = manager.snapshot_for_view(key, CellSize::new(10, 40)); + let _ = manager.snapshot_for_view(key, viewport()); + key +} + +/// Make the child produce NEW output, so a refresh has something to find. +/// +/// The child is `exec cat`, so typing into the focused terminal echoes +/// back. Without this, "refresh" tests compare a quiet terminal against +/// itself and pass with the render replaced by a no-op — the defect review +/// round 1 found in acceptance 18 and 19. +fn emit_into_child(state: &mut EditorState, terminal: pmacs::buffer::BufferId, marker: &str) { + focus_terminal(state, terminal); + for ch in marker.chars() { + press(state, KeyCode::Char(ch), KeyModifiers::NONE); + } + assert!( + tick_until(state, marker, terminal), + "the child must echo {marker:?} back onto the live screen" + ); +} + +/// What the registered VIEW currently projects — which, unlike +/// `manager.snapshot(buffer)`, depends on where the view is anchored. +fn view_text(state: &EditorState, key: TerminalViewKey) -> String { + let mut manager = state.terminal_manager.borrow_mut(); + let Some(snapshot) = manager.snapshot_for_view(key, viewport()) else { + return String::new(); + }; + let mut text = String::new(); + for cell in &snapshot.cells { + match &cell.glyph { + Glyph::Char(c) => text.push(*c), + Glyph::Cluster(b) => text.push_str(&String::from_utf8_lossy(b)), + Glyph::Continuation => {} + } + } + text +} + +fn view_at_bottom(state: &EditorState, key: TerminalViewKey) -> bool { + state + .terminal_manager + .borrow_mut() + .snapshot_for_view(key, viewport()) + .is_some_and(|snapshot| snapshot.at_bottom) } fn buffer_text_by_name(state: &EditorState, name: &str) -> Option { @@ -336,13 +386,30 @@ fn acc18_reinvoke_refreshes_in_place_and_lifecycle_runs_both_ways() { exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); let count_after_first = buffer_count(&state); + assert!( + !buffer_text_by_name(&state, SNAPSHOT_NAME) + .expect("snapshot") + .contains("REINVOKE"), + "precondition: the marker has not been emitted yet" + ); + // Advance the world, then re-invoke. Counting buffers alone is + // vacuous: it passes with the render replaced by a no-op, so the + // refresh must be observed by CONTENT that only exists after the + // first snapshot was taken. + emit_into_child(&mut state, terminal, "REINVOKE"); exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + assert!( + buffer_text_by_name(&state, SNAPSHOT_NAME) + .expect("snapshot") + .contains("REINVOKE"), + "re-invoking must actually re-serialize, not just reuse the buffer" + ); exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); assert_eq!( buffer_count(&state), count_after_first, - "re-invoking must refresh in place, not accumulate buffers" + "...and it must refresh IN PLACE, not accumulate buffers" ); // Killing the snapshot alone leaves the terminal running. @@ -402,17 +469,6 @@ fn acc19_escape_c_t_enters_copy_mode_and_g_and_q_work() { "C-c C-t must enter copy mode" ); - // `g` re-snapshots in place. - let before = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); - press(&mut state, KeyCode::Char('g'), KeyModifiers::NONE); - let after = buffer_text_by_name(&state, SNAPSHOT_NAME).expect("snapshot"); - assert_eq!(before, after, "a quiet terminal re-snapshots identically"); - assert_eq!( - active_buffer_name(&state), - SNAPSHOT_NAME, - "g must not move us" - ); - // `q` returns to the source terminal. press(&mut state, KeyCode::Char('q'), KeyModifiers::NONE); assert_eq!( @@ -420,6 +476,45 @@ fn acc19_escape_c_t_enters_copy_mode_and_g_and_q_work() { terminal_name, "q must return to the terminal the snapshot was taken from" ); + + // Now advance the world and come back WITHOUT re-invoking copy mode, + // so the snapshot is genuinely stale. Comparing a quiet terminal's + // snapshot against itself is vacuous — it passes with `render_snapshot` + // replaced by a no-op. + emit_into_child(&mut state, terminal, "AFTER-G"); + exec( + &state, + &format!( + r" + for _, id in ipairs(pmacs.buffer.list()) do + local ok, d = pcall(pmacs.describe.buffer, id) + if ok and d and d.name == {SNAPSHOT_NAME:?} then + pmacs.window.switch_buffer(id) + end + end + " + ), + ); + assert!( + !buffer_text_by_name(&state, SNAPSHOT_NAME) + .expect("snapshot") + .contains("AFTER-G"), + "the snapshot must still be stale before `g` — otherwise the next \ + assertion proves nothing" + ); + + press(&mut state, KeyCode::Char('g'), KeyModifiers::NONE); + assert!( + buffer_text_by_name(&state, SNAPSHOT_NAME) + .expect("snapshot") + .contains("AFTER-G"), + "`g` must re-snapshot from the live terminal" + ); + assert_eq!( + active_buffer_name(&state), + SNAPSHOT_NAME, + "g must not move us" + ); state.process_supervisor.borrow_mut().shutdown(); } @@ -430,7 +525,7 @@ fn acc19_escape_c_t_enters_copy_mode_and_g_and_q_work() { fn acc20_live_terminal_keys_are_unchanged_while_a_snapshot_exists() { let mut state = EditorState::new(); let terminal = open_fill_terminal(&mut state); - focus_terminal(&state, terminal); + let key = focus_terminal(&state, terminal); exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); // Back to the terminal; its five live bindings must still resolve. @@ -453,11 +548,25 @@ fn acc20_live_terminal_keys_are_unchanged_while_a_snapshot_exists() { ); } - // The terminal is still following its tail: the child's last output is - // visible without scrolling. + // The terminal still FOLLOWS ITS TAIL while a snapshot exists. + // + // Read through the registered view, not `manager.snapshot(buffer)`: + // that call is context-free and always returns the live screen, so it + // reports "at the tail" even for a view forced to the oldest retained + // row. The projected view is the only thing that can distinguish them. assert!( - screen_text(&state, terminal).contains("DONE"), - "the live terminal keeps following its tail" + view_at_bottom(&state, key), + "precondition: the view starts at the tail" + ); + emit_into_child(&mut state, terminal, "TAILMARK"); + assert!( + view_at_bottom(&state, key), + "new child output must not knock the view off the tail" + ); + assert!( + view_text(&state, key).contains("TAILMARK"), + "the freshest output must be visible in the PROJECTED view: {:?}", + view_text(&state, key) ); state.process_supervisor.borrow_mut().shutdown(); } @@ -503,6 +612,149 @@ fn acc21_describe_key_reports_the_truth_for_the_snapshot_bindings() { state.process_supervisor.borrow_mut().shutdown(); } +/// Acceptance 18a (review round 1, P1): a foreign buffer that happens to +/// carry the snapshot's name is **never adopted**. +/// +/// `pmacs.buffer.create` takes any caller-chosen name, and snapshot writes +/// use `bypass_intercept`, so found-by-name adoption clobbers a user's +/// data outright. Ownership means "in copy mode's own handle table" +/// (dired's F7 rule); a taken name gets a `<2>` variant instead. +#[test] +fn acc18a_a_foreign_same_named_buffer_is_never_adopted_or_clobbered() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + + // A user's buffer, sitting exactly where the snapshot wants to go. + exec( + &state, + &format!( + r" + FOREIGN = pmacs.buffer.create({SNAPSHOT_NAME:?}) + FOREIGN:insert(0, 'do not clobber') + " + ), + ); + + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + let foreign_text: String = eval(&state, r"return FOREIGN:slice(0, FOREIGN:len())"); + assert_eq!( + foreign_text, "do not clobber", + "the foreign buffer must be untouched" + ); + assert_ne!( + active_buffer_name(&state), + SNAPSHOT_NAME, + "copy mode must not display the foreign buffer" + ); + assert_eq!( + active_buffer_name(&state), + format!("{SNAPSHOT_NAME}<2>"), + "a taken name must yield a unique variant" + ); + assert!( + buffer_text_by_name(&state, &format!("{SNAPSHOT_NAME}<2>")) + .expect("variant snapshot") + .contains("LINE200"), + "the variant is the real snapshot" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + +/// Acceptance 18b (review round 1, P1): snapshot identity is the terminal +/// BUFFER, not its name. +/// +/// `TerminalManager::open` uniquifies only the *derived* name — an +/// explicit `name = ...` is inserted verbatim — so two valid terminals can +/// share a name. Keying snapshots by name gives them one buffer between +/// them: the second invocation retargets it, `q` returns to the wrong +/// terminal, and killing either one removes the shared snapshot. +#[test] +fn acc18b_two_same_named_terminals_get_two_independent_snapshots() { + let mut state = EditorState::new(); + exec(&state, FILL_PROFILE); + + let before = terminal_buffers(&state); + exec( + &state, + r#"TERM_A = pmacs.terminal.open { profile = "fill", name = "*same*" }"#, + ); + exec( + &state, + r#"TERM_B = pmacs.terminal.open { profile = "fill", name = "*same*" }"#, + ); + let fresh: Vec<_> = terminal_buffers(&state) + .into_iter() + .filter(|id| !before.contains(id)) + .collect(); + assert_eq!(fresh.len(), 2, "two terminals opened under one name"); + + // Distinguish them by content, since their names are identical. + emit_into_child(&mut state, fresh[0], "AAAA"); + emit_into_child(&mut state, fresh[1], "BBBB"); + + focus_terminal(&state, fresh[0]); + let snap_a: String = eval( + &state, + r"local b = pmacs.terminal.copy_mode(TERM_A); return (pmacs.describe.buffer(b)).name", + ); + focus_terminal(&state, fresh[1]); + let snap_b: String = eval( + &state, + r"local b = pmacs.terminal.copy_mode(TERM_B); return (pmacs.describe.buffer(b)).name", + ); + + assert_ne!( + snap_a, snap_b, + "two terminals must not share one snapshot buffer" + ); + let text_a = buffer_text_by_name(&state, &snap_a).expect("snapshot A"); + let text_b = buffer_text_by_name(&state, &snap_b).expect("snapshot B"); + assert!( + text_a.contains("AAAA") && !text_a.contains("BBBB"), + "snapshot A must hold only A's output: {:?}", + &text_a[text_a.len().saturating_sub(60)..] + ); + assert!( + text_b.contains("BBBB") && !text_b.contains("AAAA"), + "snapshot B must hold only B's output" + ); + + // `q` from each snapshot returns to ITS OWN terminal, which is only + // observable through the buffer id — the two names are the same. + exec( + &state, + &format!( + r" + for _, id in ipairs(pmacs.buffer.list()) do + local ok, d = pcall(pmacs.describe.buffer, id) + if ok and d and d.name == {snap_b:?} then pmacs.window.switch_buffer(id) end + end + " + ), + ); + press(&mut state, KeyCode::Char('q'), KeyModifiers::NONE); + let returned_is_b: bool = eval(&state, r"return pmacs.window.buffer() == TERM_B"); + assert!( + returned_is_b, + "q from B's snapshot must return to terminal B" + ); + + // Killing terminal A removes only A's snapshot. + exec(&state, "pmacs.terminal.terminate(TERM_A)"); + exec(&state, "pmacs.buffer.kill(TERM_A)"); + assert!( + buffer_text_by_name(&state, &snap_a).is_none(), + "A's snapshot dies with A" + ); + assert!( + buffer_text_by_name(&state, &snap_b).is_some(), + "B's snapshot must SURVIVE — a shared buffer would have gone too" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + /// Copy mode refuses a non-terminal buffer rather than producing an empty /// snapshot of nothing. #[test] From 8c5b39ef32afc05970b8a7f63c39bcdc8f064f26 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 13:12:08 -0400 Subject: [PATCH 15/18] fix(buffer): make generated buffers survive undo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2, P1. Undo could empty the "read-only" snapshot. `render_snapshot` wrote with bypass_intercept, which leaves ordinary undo history behind, and `Buffer::undo` reaches the rope through `ensure_writable` without ever consulting the intercept chain. So a single `C-/` — or `M-x buffer.undo`, which needs no keymap at all — 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 exactly where undo runs. Rebinding the undo chords buffer-locally would not have closed this, 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. Adds `Buffer::set_generated_contents` (Lua: `pmacs.buffer.set_generated_contents`): lift `read_only`, replace the contents skipping intercepts, discard the resulting history, re-assert `read_only`. This ships the framing's deferred immutability lane as ONE primitive rather than exposing the setter — a bare `set_read_only` would let a caller lock a buffer it can no longer refresh, which is precisely why that lane was deferred. Discarding history is load-bearing twice: it removes what undo would replay, and it stops a periodically refreshed buffer accumulating rope clones that `read_only` guarantees nothing can ever pop. New acceptance 16c drives the real M-x path (`command.invoke_interactive`), the chord, and redo, and asserts the owner's own refresh still works — the operation plain `read_only` would have broken. Acceptance 16b flips from asserting `is_read_only()` is false to true, because the property it documented is the one that was wrong. Three `buffer.rs` unit tests cover the primitive directly, including that ten refreshes leave an empty undo stack. Bite: restoring the delete+insert render reproduces the report exactly — `left: Some("")` against the full snapshot — failing 16c and 16b. Still open, and now named in the framing, COHERENCE.md §14 and the ledger: `*compilation*` and listview have not adopted the primitive and remain emptiable by `M-x buffer.undo`; a streaming-friendly variant is needed for the append case. In CRDT mode `read_only` is what refuses undo, since loro's UndoManager exposes no clear through `CrdtState`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer --- COHERENCE.md | 11 +- builtin/runtime/terminal.lua | 18 ++- docs/active-work.md | 33 +++++ docs/terminal-config-and-copy-mode-framing.md | 38 ++++++ src/buffer.rs | 115 ++++++++++++++++++ src/lua_bindings/mod.rs | 23 ++++ tests/terminal_copy_mode_acceptance.rs | 85 +++++++++++-- 7 files changed, 306 insertions(+), 17 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index f707234..93298fc 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -1214,7 +1214,16 @@ Primitive-by-primitive against the list above: rebindable (§6's counter-example). - **Output channel** ✓ — the compile-mode `*compilation*` model (streamed, intercept-read-only, error-rule parsing), reused by grep - and shell-command. + and shell-command. **Caveat found in terminal copy mode's review + (Stage 2): "intercept-read-only" is not read-only.** `Buffer::undo` + reaches the rope through `ensure_writable` without consulting the + intercept chain, so `M-x buffer.undo` empties such a buffer — and + rebinding the undo *chords* buffer-locally does not close it, as + `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. - **Diagnostics collection** ✓ — `DiagnosticStore` + signs + unified `error.next` source. - **Transient selector** ✓ — the minibuffer (though its `source` diff --git a/builtin/runtime/terminal.lua b/builtin/runtime/terminal.lua index f2eca85..5a912da 100644 --- a/builtin/runtime/terminal.lua +++ b/builtin/runtime/terminal.lua @@ -319,12 +319,18 @@ end -- drift between the two. local function render_snapshot(record) local text = raw_copy_retained(record.terminal) or "" - local buf = record.buffer - local len = buf:len() - -- Snapshot writes bypass the read-only intercept; everything else is - -- rejected by it. - if len > 0 then buf:delete(0, len, { bypass_intercept = true }) end - if #text > 0 then buf:insert(0, text, { bypass_intercept = true }) end + -- The owner-authorized write, and the ONLY one this buffer accepts. + -- + -- Not `delete`+`insert` with `bypass_intercept` (review round 2): that + -- leaves the buffer writable at the rope, and it leaves undo history + -- behind. `Buffer::undo` reaches the rope through `ensure_writable` + -- without consulting the intercept chain, so a single `C-/` — or + -- `M-x buffer.undo`, which no buffer-local rebinding can take away — + -- replaced a freshly rendered snapshot with an empty buffer. + -- `set_generated_contents` writes, discards the history, and leaves + -- `read_only` asserted, so undo/redo and remote CRDT imports are all + -- refused at the rope. + pmacs.buffer.set_generated_contents(record.buffer, text) end local function claim_snapshot(term_buf) diff --git a/docs/active-work.md b/docs/active-work.md index 362dd1c..1ac337d 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -738,6 +738,39 @@ If it does not, stop and repair the remote/fetch configuration. 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. In + CRDT mode `read_only` is what refuses undo, since loro's `UndoManager` + exposes no clear through `CrdtState`. - 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`; diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index b7ced8e..9d2a494 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -508,6 +508,35 @@ additive, on its own binding, and does not replace scroll-and-select. "skip the intercepts". Naming only the setter would have made it look like a one-line follow-up. + **PARTIALLY RETIRED in Stage 2, because review round 2 turned it from a + nice-to-have into a defect.** An intercept guards the dispatch path only, + and `Buffer::undo` reaches the rope through `ensure_writable` without ever + consulting the intercept chain — so a single `C-/` replaced a freshly + rendered snapshot with an empty buffer. Rebinding the undo chords + buffer-locally, which is `*compilation*`'s existing idiom, does **not** + close it: `compile.lua` says so itself ("command/menu undo stays + dispatchable"), and `M-x buffer.undo` needs no keymap. + + The fix ships the deferral's two halves together as **one** primitive + rather than exposing the setter: `Buffer::set_generated_contents` (Lua: + `pmacs.buffer.set_generated_contents`) lifts `read_only`, replaces the + contents skipping intercepts, **discards the history**, and re-asserts + `read_only`. Pairing the lock with the write is precisely what makes it + safe — a bare `set_read_only` would let a caller lock a buffer it can no + longer refresh, which is why the lane was deferred in the first place. + Discarding history is load-bearing twice: it removes the entries undo + 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). The CRDT half + is also still open: `set_generated_contents` clears the v0.1 stacks, and + in CRDT mode `read_only` is what refuses undo, since loro's + `UndoManager` has no clear exposed through `CrdtState`. + ## Acceptance ### Stage 1 — `terminal-config` @@ -577,6 +606,15 @@ additive, on its own binding, and does not replace scroll-and-select. them for selection copy. 15. isearch over the snapshot finds content that is **only in scrollback** (scrolled off the visible screen), with no change to `src/search.rs` (B1). +16c. **Undo cannot empty the snapshot, by chord OR by command** (review + round 2). `Buffer::undo` bypasses the intercept chain entirely, so the + snapshot must be `read_only` at the rope. Pinning only the chords would + be a false pass: `M-x buffer.undo` and the menu reach the command with + no keymap involved, which is why `*compilation*`'s chord-rebinding idiom + does not close this. Pinned through **`invoke_interactive`**, the real + M-x path, plus the chord, plus redo — and paired with an assertion that + the owner's own refresh still works, since that is what plain + `read_only` would have broken. 16. **Ungated, runs in CI:** focusing the snapshot buffer makes `dispatch_idle_for` report **false**. This is the whole mechanism Q#TC6a depends on, it needs no CRDT, and it fails the moment diff --git a/src/buffer.rs b/src/buffer.rs index 7f048d5..27a496a 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -504,6 +504,55 @@ impl Buffer { self.read_only = read_only; } + /// Replace a generated buffer's entire contents on behalf of its owner, + /// and leave it genuinely immutable. + /// + /// This is the **owner-authorized update path** that genuine + /// immutability for generated buffers requires. A snapshot, panel or + /// `*compilation*` buffer must reject ordinary edits, **undo, redo**, + /// and remote CRDT imports alike — and only [`read_only`] does that. + /// An edit intercept is not enough: it guards the dispatch/edit path + /// only, while [`Buffer::undo`] reaches the rope through + /// `ensure_writable` without ever consulting the intercept chain. A + /// buffer protected by an intercept alone can therefore be emptied by + /// `C-/`, by `M-x buffer.undo`, or by the menu — the command is + /// reachable even where the chords are rebound to no-ops. + /// + /// But `read_only` also blocks the owner's own refresh, which is the + /// operation such buffers exist for. So the owner needs exactly one + /// door, and this is it: lift the flag, replace the contents skipping + /// intercepts, **discard the resulting history**, re-assert the flag. + /// + /// Discarding history is not tidiness. Without it every refresh pushes + /// undo entries holding full rope clones that nothing can ever pop — + /// `read_only` guarantees they are unreachable — so a periodically + /// refreshed buffer would grow without bound. + /// + /// [`read_only`]: Self::set_read_only + pub fn set_generated_contents(&mut self, bytes: &[u8]) -> Result<(), BufferError> { + self.read_only = false; + let result = self.replace_whole_buffer(bytes); + // Cleared even on failure: a partial replace must not leave a + // half-applied edit reachable through an undo the owner cannot see. + self.undo.clear(); + self.redo.clear(); + self.read_only = true; + result + } + + fn replace_whole_buffer(&mut self, bytes: &[u8]) -> Result<(), BufferError> { + let len = self.len(); + if len > 0 { + self.apply_edit_skip_intercepts(EditOp::Delete { + range: Range::new(0, len), + })?; + } + if !bytes.is_empty() { + self.apply_edit_skip_intercepts(EditOp::Insert { pos: 0, bytes })?; + } + Ok(()) + } + fn ensure_writable(&self) -> Result<(), BufferError> { if self.read_only { Err(BufferError::ReadOnly { @@ -1955,6 +2004,72 @@ mod tests { } ); + /// The whole point of the primitive: after an owner write the buffer + /// is immutable, and `undo` — which never consults the intercept + /// chain — cannot reach back past it. + #[test] + fn set_generated_contents_writes_then_locks_and_leaves_nothing_to_undo() { + let mut buf = Buffer::new(BufferId::next(), "*generated*"); + buf.set_generated_contents(b"first render").expect("write"); + + assert_eq!(buf.len(), 12); + assert!(buf.is_read_only(), "the buffer ends immutable"); + assert!( + matches!(buf.undo(), Err(BufferError::ReadOnly { .. })), + "undo must be refused at the rope, not merely at dispatch" + ); + assert!(matches!(buf.redo(), Err(BufferError::ReadOnly { .. }))); + + // Even with the lock lifted there is no history to replay — the + // protection does not depend on the flag alone. + buf.set_read_only(false); + assert!(matches!(buf.undo(), Err(BufferError::NothingToUndo))); + assert!(matches!(buf.redo(), Err(BufferError::NothingToRedo))); + } + + /// Refreshing repeatedly must not accumulate unreachable history. + /// Each render would otherwise push entries holding full rope clones + /// that `read_only` guarantees nothing can ever pop. + #[test] + fn repeated_generated_writes_do_not_accumulate_history() { + let mut buf = Buffer::new(BufferId::next(), "*generated*"); + for i in 0..10 { + buf.set_generated_contents(format!("render {i}").as_bytes()) + .expect("write"); + } + let mut bytes = vec![0u8; buf.len() as usize]; + buf.snapshot_rope().slice(0, buf.len(), &mut bytes); + assert_eq!(String::from_utf8(bytes).expect("utf8"), "render 9"); + + buf.set_read_only(false); + assert!( + matches!(buf.undo(), Err(BufferError::NothingToUndo)), + "ten renders must leave an empty undo stack, not ten entries" + ); + } + + /// An ordinary edit is still refused after a generated write, so the + /// primitive does not quietly leave the buffer writable. + #[test] + fn set_generated_contents_still_refuses_ordinary_edits() { + let mut buf = Buffer::new(BufferId::next(), "*generated*"); + buf.set_generated_contents(b"content").expect("write"); + assert!(matches!( + buf.apply_edit(EditOp::Insert { + pos: 0, + bytes: b"x" + }), + Err(BufferError::ReadOnly { .. }) + )); + assert!(matches!( + buf.apply_edit_skip_intercepts(EditOp::Insert { + pos: 0, + bytes: b"x" + }), + Err(BufferError::ReadOnly { .. }) + )); + } + #[cfg(feature = "crdt")] #[test] fn read_only_rejects_remote_crdt_before_import_and_allows_empty_bootstrap() { diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index e27d9dd..cff5bda 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -3065,6 +3065,29 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result Date: Sun, 26 Jul 2026 13:43:22 -0400 Subject: [PATCH 16/18] fix(buffer): fan out generated writes, and clear the history that exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 3, on the round-2 primitive itself. One lesson covers all three findings: 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 reached `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 imported the owner's write and their optimistic edits were generated against content already replaced. The `delete`+`insert` pair this replaced had done that fan-out for free. Now applies ONE whole-buffer `Replace`, returns its `Edit`, and notifies from the binding. The doc comment states the obligation, because the next owner to adopt the primitive inherits it. 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` exposes no clear, but needs none: it records only what happens after it is constructed, the same property `CrdtState::from_bytes` already uses to keep the seed insert out of undo. `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 keeps its analysis under a superseded-in-part box rather than being silently rewritten — its conclusion survives, two of its premises do not. New pins. acc16d paints the window after a SHRINKING generated write: stale offsets then point past the buffer end, so the failure is the reported crash rather than merely stale pixels. acc16e asserts the refresh is queued for mirrors, through the real copy-mode path; `crdt`-gated and therefore dark in CI, which is why 16d drives the binding rather than the terminal. Plus a CRDT 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, and recorded in COHERENCE.md §14: the fan-out obligation makes `*compilation*`/listview adoption more than a one-line swap. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer --- COHERENCE.md | 8 +- builtin/runtime/terminal.lua | 31 ++-- docs/active-work.md | 49 +++++- docs/terminal-config-and-copy-mode-framing.md | 78 ++++++++- src/buffer.rs | 69 ++++++-- src/crdt.rs | 20 +++ src/lua_bindings/mod.rs | 19 ++- tests/terminal_copy_mode_acceptance.rs | 154 ++++++++++++++++++ 8 files changed, 382 insertions(+), 46 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index 93298fc..0ba2ac8 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -1223,7 +1223,13 @@ Primitive-by-primitive against the list above: 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. + not yet adopted it and remain emptiable. **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. - **Diagnostics collection** ✓ — `DiagnosticStore` + signs + unified `error.next` source. - **Transient selector** ✓ — the minibuffer (though its `source` diff --git a/builtin/runtime/terminal.lua b/builtin/runtime/terminal.lua index 5a912da..ef2fea4 100644 --- a/builtin/runtime/terminal.lua +++ b/builtin/runtime/terminal.lua @@ -329,7 +329,10 @@ local function render_snapshot(record) -- replaced a freshly rendered snapshot with an empty buffer. -- `set_generated_contents` writes, discards the history, and leaves -- `read_only` asserted, so undo/redo and remote CRDT imports are all - -- refused at the rope. + -- refused at the rope. Its binding also fans the resulting edit out to + -- the windows showing this buffer and to replica mirrors (review round + -- 3) — a rope write alone leaves a displaying window indexing the new + -- contents with stale line offsets. pmacs.buffer.set_generated_contents(record.buffer, text) end @@ -345,18 +348,22 @@ local function claim_snapshot(term_buf) local record = { terminal = term_buf, buffer = buf } handles[#handles + 1] = record - -- Q#TC6a — BOTH calls, and the second is the load-bearing one. + -- Q#TC6a — BOTH calls, and the protection is now LAYERED. Review + -- round 2 changed what each one is for. -- - -- An intercept guards the dispatch/edit path only. It does NOT set - -- `Buffer::read_only` (deliberately independent), and no Lua binding - -- sets that flag at all, so an optimistic CRDT op from a semantic - -- frontend bypasses the intercept AND passes `ensure_writable()` — - -- mutating the daemon buffer in lockstep with the mirror, with no - -- divergence to notice. `set_round_trip_input` prevents that at the - -- only point it can be prevented: `dispatch_idle_for` reports false - -- while this buffer is focused, so the frontend never applies - -- optimistically and never emits the op. It is the guard, not - -- hardening. + -- `set_generated_contents` leaves `read_only` asserted at the rope, so + -- on the DAEMON side undo, redo, ordinary edits and imported CRDT ops + -- are all refused by `ensure_writable()`. The intercept below is no + -- longer the daemon's guard; it survives to give a dispatching edit a + -- named error instead of a bare refusal. + -- + -- `set_round_trip_input` still guards the half `read_only` cannot + -- reach: a semantic frontend applies optimistically in its own MIRROR + -- before the daemon ever sees the op. `dispatch_idle_for` reports + -- false while this buffer is focused, so the mirror never mutates and + -- no op is emitted to be refused. That is the layering — rope-level + -- read-only protects the daemon copy, round-trip input protects the + -- replica copy — and neither substitutes for the other. pmacs.buffer.add_intercept(buf, function() error(name .. " is read-only") end) diff --git a/docs/active-work.md b/docs/active-work.md index 1ac337d..dc8c306 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -768,9 +768,52 @@ If it does not, stop and repair the remote/fetch configuration. 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. In - CRDT mode `read_only` is what refuses undo, since loro's `UndoManager` - exposes no clear through `CrdtState`. + 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. - 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`; diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index 9d2a494..3d9891d 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -6,7 +6,10 @@ `terminal-copy-mode` off `main` @ `cf54270`; no protocol change.** **Stage 2 ships eight of its nine criteria, plus 18a and 18b added in review -round 1.** Criterion 17's semantic-frontend end-to-end pin is deliberately +round 1 and 16c-16e in rounds 2-3.** Rounds 2 and 3 changed the design, not +just the code: the snapshot is now genuinely `read_only` at the rope, so +**Q#TC6a's analysis below is superseded in part** — read the box at its head +before the analysis. Q#TC6a's conclusion survives; two of its premises do not. Criterion 17's semantic-frontend end-to-end pin is deliberately absent — see the note under it — because a faithful version requires the real `pmacs-gpu` optimistic path, and therefore the `a37` foundation, which CI never compiles and which skips silently. Both halves of the *mechanism* it guards are @@ -348,6 +351,32 @@ ordinary document buffer, so: and `set_round_trip_input` is the ONLY thing standing between a replica frontend and unauthorized mutation.** +> **SUPERSEDED IN PART BY IMPLEMENTATION (review rounds 2-3). Read this +> box before the analysis below it.** The reasoning is still the correct +> account of the substrate *as it stood when this was written*, and its +> conclusion about round-trip input still holds. Two of its premises no +> longer do: +> +> - "**No Lua binding sets `read_only` at all**" — one does now. +> `pmacs.buffer.set_generated_contents` leaves it asserted, so on the +> daemon side undo, redo, ordinary edits and imported CRDT ops are all +> refused by `ensure_writable()`. That closed a real defect: undo +> bypasses the intercept chain, so `M-x buffer.undo` emptied the +> snapshot. +> - "**`set_round_trip_input` is the ONLY thing**" — it is now the only +> thing standing between a replica and *mirror* mutation, which is the +> half `read_only` cannot reach. A semantic frontend applies +> optimistically in its own mirror before the daemon sees the op; a +> daemon-side refusal cannot prevent that, it can only make the two +> copies disagree. +> +> The protection is therefore **layered, not singular**: rope-level +> read-only protects the daemon copy, round-trip input protects the +> replica copy, and neither substitutes for the other. The intercept +> survives only to give a dispatching edit a named error. The Deferred +> lane below records what this leaves open for `*compilation*` and +> listview, which have **not** adopted the primitive. + The established idiom is two calls: `listview.lua:106` and `compile.lua:272` each pair `pmacs.buffer.add_intercept` with `pmacs.buffer.set_round_trip_input(buf, true)`. Revision 2 described the @@ -382,7 +411,8 @@ Two things follow, and both are recorded rather than fixed here: genuinely immutable at the rope/CRDT boundary the way terminal identity buffers are, turning round-trip back into real defence in depth. That is a substrate change affecting listview and compile as much as this snapshot, so - it is named in Deferred with its own lane. + it is named in Deferred with its own lane. **Done for this snapshot only**, + and not by exposing the setter — see the Deferred lane and the box above. **Q#TC7 — the materializer reuses the existing serializer.** A whole-range variant of `copy_selection_bytes` over `retained_rows` inherits the criterion @@ -532,10 +562,19 @@ additive, on its own binding, and does not replace scroll-and-select. 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). The CRDT half - is also still open: `set_generated_contents` clears the v0.1 stacks, and - in CRDT mode `read_only` is what refuses undo, since loro's - `UndoManager` has no clear exposed through `CrdtState`. + (`*compilation*` appends rather than replacing wholesale). + + **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 + the history lives in loro's `UndoManager`. `read_only` would stop that + history being *replayed* but not *retained* — a panel refreshed on a + timer still grows without bound, which is the condition the contract + says it eliminates. `UndoManager` exposes no `clear`, but it needs none: + a manager records only what happens after it is constructed, which + `CrdtState::from_bytes` already relies on to keep the seed insert out of + undo. `CrdtState::clear_undo_history` rebinds a fresh manager to the same + doc, and `set_generated_contents` clears whichever history the buffer + actually has. ## Acceptance @@ -615,6 +654,23 @@ additive, on its own binding, and does not replace scroll-and-select. M-x path, plus the chord, plus redo — and paired with an assertion that the owner's own refresh still works, since that is what plain `read_only` would have broken. +16d. **A generated write reaches the window, not just the rope** (review + round 3). `set_generated_contents` returns one whole-buffer `Replace` + and its binding fans it out; swallowing it leaves a displaying + window's `TextView` line index describing the *previous* contents. + Pinned by **painting** — a shrinking write, so the stale offsets point + past the buffer end and the next render trips + `assertion failed: end <= self.len()` in `src/rope.rs`, which is the + reported crash rather than merely stale pixels. Driven through the Lua + binding copy mode itself calls, so it covers every future owner of the + primitive. +16e. **The same write is queued for replica mirrors** (review round 3, + CRDT half). The dropped fan-out also skipped + `queue_daemon_origin_crdt_op`, so a replica's mirror never imports the + owner's write and its optimistic edits are generated against content + already replaced. Pinned through the real copy-mode refresh on an + upgraded snapshot. `crdt`-gated, therefore dark in CI — 16d is the half + that actually runs there. 16. **Ungated, runs in CI:** focusing the snapshot buffer makes `dispatch_idle_for` report **false**. This is the whole mechanism Q#TC6a depends on, it needs no CRDT, and it fails the moment @@ -644,9 +700,13 @@ additive, on its own binding, and does not replace scroll-and-select. What IS pinned instead, ungated and in CI: acceptance 16 asserts the guard is armed (`dispatch_idle` false while the snapshot is focused, so no replica can apply optimistically or emit), and acceptance 16b asserts - the hazard is real by showing the snapshot buffer's `is_read_only()` is - **false** despite the intercept — i.e. nothing at the rope/CRDT boundary - would stop such an op if one arrived. Together those cover both halves of + the buffer is `is_read_only()` **true** at the rope, so an op that did + arrive at the daemon would be refused by `ensure_writable()` rather + than applied. (Rounds 1-2 asserted **false** here, documenting the + hazard; round 2 closed it, and the assertion was flipped with it. + That does not make 17 redundant — a daemon-side refusal cannot stop a + replica mutating its own mirror, which is precisely what + `set_round_trip_input` is for.) Together those cover both halves of Q#TC6a's *mechanism*. What remains unproven is only the end-to-end wire behaviour of a real GPU frontend, and it stays an explicit obligation of the CI `crdt`-coverage lane rather than being quietly dropped. diff --git a/src/buffer.rs b/src/buffer.rs index 27a496a..a9c01e9 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -526,31 +526,43 @@ impl Buffer { /// Discarding history is not tidiness. Without it every refresh pushes /// undo entries holding full rope clones that nothing can ever pop — /// `read_only` guarantees they are unreachable — so a periodically - /// refreshed buffer would grow without bound. + /// refreshed buffer would grow without bound. In CRDT mode the same + /// retention lives in loro's `UndoManager`, so both are cleared. + /// + /// # The returned edit must be fanned out + /// + /// One whole-buffer [`EditOp::Replace`] is applied, and its [`Edit`] + /// is returned rather than swallowed, because a rope write is only + /// half of an edit. Callers **must** route the result through their + /// normal edit-notification path (for the Lua surface, + /// `notify_buffer_edit_to_windows`). A window already displaying the + /// buffer keeps a stale `TextView` line cache otherwise, and the next + /// paint indexes the new rope with old ranges; and in CRDT mode the + /// op never reaches replica mirrors, so their optimistic edits are + /// generated against content the owner has already replaced. /// /// [`read_only`]: Self::set_read_only - pub fn set_generated_contents(&mut self, bytes: &[u8]) -> Result<(), BufferError> { + pub fn set_generated_contents(&mut self, bytes: &[u8]) -> Result { self.read_only = false; - let result = self.replace_whole_buffer(bytes); + let result = self.apply_edit_skip_intercepts(EditOp::Replace { + range: Range::new(0, self.len()), + bytes, + }); // Cleared even on failure: a partial replace must not leave a // half-applied edit reachable through an undo the owner cannot see. - self.undo.clear(); - self.redo.clear(); + self.clear_history(); self.read_only = true; result } - fn replace_whole_buffer(&mut self, bytes: &[u8]) -> Result<(), BufferError> { - let len = self.len(); - if len > 0 { - self.apply_edit_skip_intercepts(EditOp::Delete { - range: Range::new(0, len), - })?; + /// Drop undo and redo history in whichever mode this buffer is in. + fn clear_history(&mut self) { + self.undo.clear(); + self.redo.clear(); + #[cfg(feature = "crdt")] + if let Some(crdt) = self.crdt.as_ref() { + crdt.clear_undo_history(); } - if !bytes.is_empty() { - self.apply_edit_skip_intercepts(EditOp::Insert { pos: 0, bytes })?; - } - Ok(()) } fn ensure_writable(&self) -> Result<(), BufferError> { @@ -2048,6 +2060,33 @@ mod tests { ); } + /// Review round 3, P2. In CRDT mode the v0.1 stacks are bypassed + /// entirely, so clearing them proves nothing: the history the + /// primitive promises to discard lives in loro's `UndoManager`. + /// The lock is lifted deliberately — `read_only` stops the replay, + /// but the contract is that there is nothing left to replay. + #[cfg(feature = "crdt")] + #[test] + fn generated_writes_accumulate_no_crdt_history_either() { + let mut buf = + Buffer::new_with_crdt(BufferId::next(), "*generated*", 1).expect("crdt construction"); + for i in 0..10 { + buf.set_generated_contents(format!("render {i}").as_bytes()) + .expect("write"); + } + assert_eq!(rope_string(&buf), "render 9"); + assert!( + !buf.crdt_state().expect("crdt-backed").can_undo(), + "the UndoManager must have nothing recorded" + ); + + buf.set_read_only(false); + assert!( + matches!(buf.undo(), Err(BufferError::NothingToUndo)), + "CRDT-mode undo must find no history either" + ); + } + /// An ordinary edit is still refused after a generated write, so the /// primitive does not quietly leave the buffer writable. #[test] diff --git a/src/crdt.rs b/src/crdt.rs index 8cef9e3..ca68d1a 100644 --- a/src/crdt.rs +++ b/src/crdt.rs @@ -486,6 +486,26 @@ impl CrdtState { pub fn record_checkpoint(&self) -> LoroResult<()> { self.undo.borrow_mut().record_new_checkpoint() } + + /// Discard the bound peer's undo and redo history, keeping the + /// document itself untouched. + /// + /// Loro's `UndoManager` exposes no `clear`, but it does not need + /// one: a manager records only what happens **after** it is + /// constructed. [`Self::from_bytes`] already relies on exactly + /// that property to keep the seed insert out of undo. Replacing + /// the manager with a fresh one bound to the same doc therefore + /// leaves nothing to undo, and drops the old manager's retained + /// stacks with it. + /// + /// Used by [`crate::buffer::Buffer::set_generated_contents`], whose + /// contract is that a generated buffer accumulates no history + /// across refreshes. Marking the buffer read-only would stop the + /// history being *replayed*, but not being *retained* — a panel + /// refreshed on a timer would grow without bound. + pub fn clear_undo_history(&self) { + *self.undo.borrow_mut() = Self::create_undo_manager(&self.doc); + } } /// T M10.3: map a [`crate::protocol::FrontendId`] to the loro `PeerID` diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index cff5bda..aa5de88 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -3077,12 +3077,19 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result pmacs::buffer::BufferId { + let core = state.core.borrow(); + let reg = core.registry.borrow(); + reg.ids() + .iter() + .copied() + .find(|id| reg.get(*id).is_ok_and(|b| b.name() == SNAPSHOT_NAME)) + .expect("snapshot buffer exists") +} + +/// Rendered cells of the active window (the `m4_acceptance` grid helper; +/// cross-crate test code can't import it). +fn render_active_window_to_grid( + state: &mut EditorState, + rows: u32, + cols: u32, +) -> Vec { + use pmacs::cell::{Cell, CellGrid}; + use pmacs::view::{View, Viewport}; + use pmacs::window::Rect; + + let mut core = state.core.borrow_mut(); + let active = core.active_window_id(); + let registry = core.registry.clone(); + let win = core.windows.get_mut(&active).expect("active window"); + let rect = Rect::new(0, 0, rows, cols); + let mut backing = vec![Cell::default(); (rows * cols) as usize]; + let reg = registry.borrow(); + let buf = reg.get(win.buffer_id).expect("buffer in registry"); + let viewport = Viewport { + buffer_start: 0, + buffer_end: buf.len(), + cell_origin: rect.origin, + cell_size: CellSize::new(rows, cols), + gutter_w: 0, + folds: None, + }; + let mut grid = CellGrid { + cells: &mut backing, + stride: cols, + size: CellSize::new(rows, cols), + }; + win.text_view.render(buf, viewport, &mut grid); + backing +} + +fn grid_row(cells: &[pmacs::cell::Cell], row: u32, cols: u32) -> String { + (0..cols) + .map(|c| match cells[(row * cols + c) as usize].glyph { + Glyph::Char(ch) => ch, + _ => ' ', + }) + .collect::() + .trim_end() + .to_owned() +} + +/// Review round 3, P1. A rope write is only half of an edit: the window +/// showing the buffer holds a `TextView` line index that only `on_edit` +/// maintains, so a write that reaches the rope without the notification +/// leaves the two disagreeing. +/// +/// Pinned by PAINTING, because that is where the disagreement bites: with +/// the fan-out dropped, the next render indexes the new rope with the old +/// line offsets. A shrinking write is used deliberately — stale offsets +/// then point past the buffer end, which is the reported crash rather than +/// merely stale pixels. +/// +/// Driven through `pmacs.buffer.set_generated_contents`, the seam copy +/// mode's refresh actually calls, so it also covers `*compilation*` and +/// any other owner that adopts the primitive later. +#[test] +fn acc16d_a_generated_write_notifies_the_window_that_displays_it() { + let mut state = EditorState::new(); + exec( + &state, + r" + GEN = pmacs.buffer.create('*generated-probe*') + pmacs.buffer.set_generated_contents(GEN, 'alpha\nbeta\ngamma\ndelta\nepsilon\n') + pmacs.window.switch_buffer(GEN) + ", + ); + let painted = render_active_window_to_grid(&mut state, 6, 20); + assert_eq!( + grid_row(&painted, 0, 20), + "alpha", + "precondition: the window paints the generated buffer" + ); + + exec( + &state, + r"pmacs.buffer.set_generated_contents(GEN, 'CHANGED\n')", + ); + let painted = render_active_window_to_grid(&mut state, 6, 20); + assert_eq!( + grid_row(&painted, 0, 20), + "CHANGED", + "the window must paint the refreshed contents" + ); + assert_eq!( + grid_row(&painted, 1, 20), + "", + "and nothing of the longer contents it replaced" + ); +} + +/// Review round 3, P1, CRDT half. The same dropped fan-out also skips +/// `queue_daemon_origin_crdt_op`, so replica mirrors never import the +/// owner's write and their optimistic edits are generated against content +/// the owner has already replaced. +/// +/// Gated because `upgrade_to_crdt` is — and therefore dark in CI, which +/// never enables the feature. The default-configuration half above is the +/// one that actually runs there. +#[cfg(feature = "crdt")] +#[test] +fn acc16e_a_refresh_queues_the_owners_write_for_replica_mirrors() { + let mut state = EditorState::new(); + let terminal = open_fill_terminal(&mut state); + focus_terminal(&state, terminal); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + let snapshot = snapshot_buffer_id(&state); + { + let core = state.core.borrow(); + let mut reg = core.registry.borrow_mut(); + let buffer = reg.get_mut(snapshot).expect("snapshot buffer"); + // `read_only` refuses the upgrade's own bookkeeping path the same + // way it refuses everything else, so lift it around the upgrade. + buffer.set_read_only(false); + buffer.upgrade_to_crdt(2).expect("upgrade"); + buffer.set_read_only(true); + } + state.core.borrow_mut().pending_crdt_ops.clear(); + + emit_into_child(&mut state, terminal, "MIRRORME"); + exec(&state, "pmacs.terminal.copy_mode(TERM_BUF)"); + + let queued: Vec<_> = state + .core + .borrow() + .pending_crdt_ops + .iter() + .map(|(_, id, _)| *id) + .collect(); + assert!( + queued.contains(&snapshot), + "the owner's refresh must be queued for broadcast; queued: {queued:?}" + ); + state.process_supervisor.borrow_mut().shutdown(); +} + /// Acceptance 18: re-invoking refreshes in place, and the lifecycle runs /// both directions. #[test] From 23c966cc31142ad6ce2e4ba4cc32749b8eaff9be Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 14:38:22 -0400 Subject: [PATCH 17/18] docs(terminal): restate criterion 17's bite for the fixed substrate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 4, P2. A fix can invalidate a test that was never written. Criterion 17 still specified the pre-round-2 world: remove `set_round_trip_input` and the optimistic op "passes `ensure_writable()` and mutates BOTH sides, silently, with no divergence to notice". That was true while no Lua binding set `read_only`. Since `set_generated_contents` does, the daemon refuses the op — so only the frontend's own mirror mutates, and the copies diverge. The gap matters precisely because 17 is unpinned. A real-GPU test written to the old spec would hunt for a daemon-side edit that can no longer occur and pass for the wrong reason, quietly readmitting the round-2 regression through a test not yet built. The specification is the artifact under review here, not the code. Restated around unauthorized MIRROR mutation plus daemon refusal — divergence — in all four places carrying the obsolete claim: the criterion itself, 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. `docs/active-work.md` also still described acceptance 16b as asserting `is_read_only()` is false, which round 2 flipped. Why round-trip input stays load-bearing rather than redundant, now stated wherever the daemon guard is mentioned: a 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 is looking at. Also recorded, after capturing it properly this time: the gate-run flake in `cargo test --lib --features crdt` is `process::tests::setsid_escapee_is_not_reaped_and_teardown_reclaims_readers` (`active_reader_probe` -> None, "live runtime probe"), ~1 run in 5. Pre-existing and unrelated — this branch does not touch `src/process.rs`, the test passes 10/10 standalone and 2017/2017 at `--test-threads=1`, and it is another instance of the known `drain_until` trap: draining for `Started` also ticks, and a tick reaps the leader. That also explains the unattributed "2 failed" run noted in round 2. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gGQC6eqHJVbZJ5Hg7aLer --- docs/active-work.md | 45 +++++++++++++++++-- docs/terminal-config-and-copy-mode-framing.md | 39 +++++++++++----- tests/terminal_copy_mode_acceptance.rs | 11 ++++- 3 files changed, 79 insertions(+), 16 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index dc8c306..62a9b6d 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -685,10 +685,15 @@ If it does not, stop and repair the remote/fetch configuration. 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 hazard is real — the snapshot's `is_read_only()` - is **false** despite the intercept, so nothing at the rope/CRDT - boundary would stop an op that did arrive). The wire-level half is an - explicit obligation of the CI `crdt`-coverage lane. + 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 @@ -814,6 +819,38 @@ If it does not, stop and repair the remote/fetch configuration. - **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 identified and attributed, not waved off.** + `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 passes 10/10 standalone, failing only + under full-suite parallelism. It is **another instance of the known + `drain_until` trap** — draining for `Started` to learn the pid also + ticks, and a tick reaps the leader, so the probe that follows finds + nothing live. Same module and same signature as the earlier + `signal`-says-"is not running" case. This also explains the + unattributed "2 failed" CRDT run recorded in round 2. Belongs to the + CI `crdt`-coverage lane, which is where the whole class lives. - 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`; diff --git a/docs/terminal-config-and-copy-mode-framing.md b/docs/terminal-config-and-copy-mode-framing.md index 3d9891d..15878cf 100644 --- a/docs/terminal-config-and-copy-mode-framing.md +++ b/docs/terminal-config-and-copy-mode-framing.md @@ -9,7 +9,12 @@ round 1 and 16c-16e in rounds 2-3.** Rounds 2 and 3 changed the design, not just the code: the snapshot is now genuinely `read_only` at the rope, so **Q#TC6a's analysis below is superseded in part** — read the box at its head -before the analysis. Q#TC6a's conclusion survives; two of its premises do not. Criterion 17's semantic-frontend end-to-end pin is deliberately +before the analysis. Q#TC6a's conclusion survives; two of its premises do +not, and **criterion 17's bite was restated with them** — the daemon now +refuses the op, so the failure it must look for is mirror mutation plus +divergence, not silent agreement. + +Criterion 17's semantic-frontend end-to-end pin is deliberately absent — see the note under it — because a faithful version requires the real `pmacs-gpu` optimistic path, and therefore the `a37` foundation, which CI never compiles and which skips silently. Both halves of the *mechanism* it guards are @@ -347,9 +352,10 @@ ordinary document buffer, so: inspectable — the idiom `COHERENCE.md` §6 identifies as the right side of the line. -**Q#TC6a — the snapshot is BOTH intercept-read-only AND round-trip-marked, -and `set_round_trip_input` is the ONLY thing standing between a replica -frontend and unauthorized mutation.** +**Q#TC6a — the snapshot is read-only at the rope AND round-trip-marked, and +each guard covers a copy the other cannot reach: `read_only` refuses the op +at the daemon, `set_round_trip_input` is the ONLY thing standing between a +replica frontend and unauthorized mutation of its own mirror.** > **SUPERSEDED IN PART BY IMPLEMENTATION (review rounds 2-3). Read this > box before the analysis below it.** The reasoning is still the correct @@ -680,10 +686,22 @@ additive, on its own binding, and does not replace scroll-and-select. 17. **Through a semantic frontend** (this one does need CRDT): keys typed in the snapshot buffer reach ordinary dispatch and never the child, and **neither the daemon buffer nor the frontend's mirror is mutated** - (Q#TC6a). Bite: with `set_round_trip_input` removed, the optimistic op is - emitted, bypasses the Lua intercept, passes `ensure_writable()`, and - mutates **both sides** — a buffer the editor calls read-only silently - accepts an edit. + (Q#TC6a). Bite: with `set_round_trip_input` removed, the frontend + applies the edit **optimistically to its own mirror** and emits the op; + the mirror now shows text the user was told is read-only. The daemon + refuses the op at `ensure_writable()` — `set_generated_contents` leaves + `read_only` asserted — so the two copies **diverge**, and the local + mirror is the one the user is looking at. + + **This bite changed in review round 3, and the direction matters.** + Rounds 1-2 specified it as "mutates *both sides*, silently, with no + divergence to notice" — true when nothing set `read_only` from Lua, + and false now. The eventual real-GPU test must assert **mirror + mutation plus daemon refusal**, not silent agreement; written the old + way it would look for a daemon-side edit that can no longer happen and + pass for the wrong reason. That the daemon now holds is exactly why + round-trip input is still load-bearing rather than redundant: a + refusal protects the daemon's copy and does nothing for the replica's. **NOT PINNED as specified, deliberately, and this is the one gap in Stage 2.** A faithful test has to drive the *real* `pmacs-gpu` binary: @@ -795,8 +813,9 @@ Full gate suite per `CLAUDE.md` for each PR separately, plus: implementations (epoch-only key, single last-entry, unpurged map), which is why one pin was not enough; **9** (a hardcoded `0x03` makes the configured chord unreachable); **10** (its failure mode is a terminal nobody can - escape); and **16/17** (a read-only buffer that silently accepts an edit on - both sides). + escape); and **16** (a read-only buffer whose replica mirror accepts an + edit the user is then looking at — 17's daemon half was closed in review + round 2, and its bite restated in round 3). - **The observation seams the cache pins need are `escape_parses` (how often) and `escape_caches` (how many are still held).** Neither is inferable from behavior: for a *valid* setting a correct per-session cache and a leaking diff --git a/tests/terminal_copy_mode_acceptance.rs b/tests/terminal_copy_mode_acceptance.rs index 9572cbc..ed3f6d7 100644 --- a/tests/terminal_copy_mode_acceptance.rs +++ b/tests/terminal_copy_mode_acceptance.rs @@ -308,8 +308,15 @@ fn acc15_isearch_finds_content_only_in_scrollback() { /// Acceptance 16 — the load-bearing pin, and the reason this suite is /// ungated. `set_round_trip_input` is the ONLY thing standing between a -/// replica frontend and unauthorized mutation (Q#TC6a), so its regression -/// must be caught in the configuration CI actually compiles. +/// replica frontend and unauthorized mutation **of its own mirror** +/// (Q#TC6a), so its regression must be caught in the configuration CI +/// actually compiles. +/// +/// Rope-level `read_only` does not substitute for it. Since review round 2 +/// the daemon refuses such an op at `ensure_writable()` — but a refusal +/// arrives after the frontend has already applied optimistically and +/// painted the result. What that buys is divergence instead of silent +/// agreement; what stops the mutation is this. #[test] fn acc16_dispatch_idle_is_false_while_the_snapshot_is_focused() { let mut state = EditorState::new(); From a58bb110bc871e95126381d97486e9388bdd50f5 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 14:54:37 -0400 Subject: [PATCH 18/18] docs(terminal): qualify process flake attribution Separate the observed suite behavior from the suspected drain_until mechanism, and leave round-2 failures unattributed without test names. --- docs/active-work.md | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 62a9b6d..3a5bb88 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -837,20 +837,23 @@ If it does not, stop and repair the remote/fetch configuration. 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 identified and attributed, not waved off.** + - **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 passes 10/10 standalone, failing only - under full-suite parallelism. It is **another instance of the known - `drain_until` trap** — draining for `Started` to learn the pid also - ticks, and a tick reaps the leader, so the probe that follows finds - nothing live. Same module and same signature as the earlier - `signal`-says-"is not running" case. This also explains the - unattributed "2 failed" CRDT run recorded in round 2. Belongs to the - CI `crdt`-coverage lane, which is where the whole class lives. + 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`;