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]