From 23c966cc31142ad6ce2e4ba4cc32749b8eaff9be Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 14:38:22 -0400 Subject: [PATCH] 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();