diff --git a/docs/active-work.md b/docs/active-work.md index dfed143..dc8852d 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -465,10 +465,10 @@ has **no branch and no framing yet**. `../pmacs-generated-immutability`. **PR #188**, base `main`, forked from `githubsucks/main` @ `ad41cf1`, **integrated to `7586905`** (#189, `COHERENCE.md` only; clean merge, no conflict). Framing only — - `docs/generated-buffer-immutability-framing.md`, revision 4, plus this + `docs/generated-buffer-immutability-framing.md`, revision 5, plus this lane. **No runtime code, no protocol change.** -- **PROPOSED — three review rounds closed (fifteen findings, nine P1, six - P2). Not approved. Do not implement, do not merge.** +- **PROPOSED — four review rounds closed (twenty findings, thirteen P1, + seven P2). Not approved. Do not implement, do not merge.** - **What it frames.** The class-wide half of the `set_generated_contents` invariant that `docs/agent-handoff.md` §4 and `COHERENCE.md` §14 both record as unfinished: `Buffer::undo` gates on `ensure_writable()` @@ -495,15 +495,19 @@ has **no branch and no framing yet**. (`:1140-1163`), so it was neither cleaned nor detected; and the unconditional relock **locked a fresh buffer that was never successfully written**. `NoOp` clears, `Rejected` restores the entry - lock state, `Diverged` clears nothing and surfaces. + lock state, `Diverged` clears nothing and surfaces. **Revision 5 keeps + the five outcomes but preserves the `Edit` in + `AppliedThenFailed { edit, error }`: the borrow-free Lua finisher fans + it out to window caches and replica mirrors before returning the + error.** Collapsing to `Result` inside `Buffer` was too early. - **Two stages, two PRs.** Stage 1 — listview ownership fix **plus its identity-routing fix in the same PR**, dired and listview adopting the shipped primitive, the window-coordinate clamp, and the fold decision. Stage 2 — the new primitive, compile's nine write sites, the search panel's four, compile/search ownership + routing, the path-backed - refusal plus `mark_clean`, the `identity_protected` field, and - the bounded `unlock_generated`. -- **Six facts from this lane that other lanes need before it merges:** + refusal plus `mark_clean`, and the terminal-only + `identity_protected` guard. **No Lua unlock ships.** +- **Nine facts from this lane that other lanes need before it merges:** - **`bypass_intercept` is the wrong inventory key.** It misses `*buffer-list*`, `*help*` and `*workers*`, which are generated with plain writes and no intercept at all. `docs/agent-handoff.md` §4's @@ -545,10 +549,11 @@ has **no branch and no framing yet**. identity buffer.** It does `self.read_only = false` unconditionally (`src/buffer.rs:546`), so it lifts a lock it did not install, writes, and re-locks. Present on `main`, untested, unframed anywhere before - this revision. Bounded in Stage 2 by the `identity_protected` field — - an **intrinsic** flag set once by `TerminalSession::open`, never - written by `set_read_only`. Revision 3 tried to infer this from the - lock's provenance instead; that broke the lift-and-restore idiom at + revision 4. Refused in Stage 2 by the `identity_protected` field — + an **intrinsic** flag marked once by a crate-private monotonic + `mark_identity_protected()` in `TerminalSession::open`, never written + by `set_read_only`. Revision 3 tried to infer this from the lock's + provenance instead; that broke the lift-and-restore idiom at `tests/terminal_copy_mode_acceptance.rs:578-584`, and the general lesson is that a **derived** fact must be maintained by every mutation of what it derives from — and `set_read_only` is `pub`. @@ -558,6 +563,13 @@ has **no branch and no framing yet**. compiles it, so a green run of that suite proves nothing about the seam. Any lane touching `read_only` semantics must run it with the feature and confirm `acc16e` is in the count. + - **`identity_protected` is not generated-lock provenance.** Revision + 4 tried to use “not a terminal identity buffer” as proof that the + generated primitive installed the lock; it is not. Revision 5 + therefore removes `pmacs.buffer.unlock_generated` from the arc + entirely. Wdired's future generated→editable transition remains + dired Stage 3 work and must be owner-specific or use the eventual + lock-policy enum. - **The CRDT `Replace` mid-transaction divergence is real and unowned.** `crdt.delete` then `crdt.insert` (`src/buffer.rs:1140-1163`); if the first succeeds and the second fails, the code's own comment @@ -565,7 +577,11 @@ has **no branch and no framing yet**. violation." It reaches `apply_edit` and `apply_edit_skip_intercepts` today and is reported as an ordinary `CrdtRejected`, so nothing distinguishes it. This lane names and contains it; **repair is - deferred and unowned.** + deferred and unowned.** Revision 5 makes the classifier mandatory: + a private delete→insert helper is fault-injected under + `cargo test --lib --features crdt`; there is no four-variant fallback + that maps divergence to `Rejected` and leaves a fresh buffer + writable. - **Overlap warning.** Stage 2 touches `src/lua_bindings/mod.rs`'s buffer mutator bindings and `src/buffer.rs`. Do not run it concurrently with the `apply_resource_op` lane or the bottom-panel 2B work without diff --git a/docs/generated-buffer-immutability-framing.md b/docs/generated-buffer-immutability-framing.md index c4461bb..2004bfa 100644 --- a/docs/generated-buffer-immutability-framing.md +++ b/docs/generated-buffer-immutability-framing.md @@ -3,9 +3,41 @@ **PROPOSED — needs explicit user approval before implementation. DO NOT implement, DO NOT merge.** -**Revision 4 — scouted against canonical `githubsucks/main` @ `7586905`, +**Revision 5 — scouted against canonical `githubsucks/main` @ `7586905`, 2026-07-28.** Every count in this document was re-measured at this -revision; the command output is in the revision-4 block below. +base in revision 4; revision 5 changes only the framed design and +acceptance, not a counted tree surface. + +## Revision 5 + +**Answers review round 4 on `d98d0b3` — four P1, one P2. All five +findings are confirmed. This revision removes one proposed public +capability, makes one transaction outcome larger, and makes the CRDT +fault seam mandatory rather than leaving a fallback for implementation +time.** + +| finding | the decision | +|---|---| +| **P1-1** — `AppliedThenFailed` loses the `Edit`, so the binding cannot fan out a mutation before returning `Err` | Confirmed. `AppliedThenFailed` becomes `{ edit: Edit, error: BufferError }`; `apply_generated_edit` returns the outcome rather than collapsing it to `Result` inside `Buffer`. After the registry borrow drops, the binding fans out `Applied`, `NoOp`, **and `AppliedThenFailed`**. The last arm then surfaces its error. That preserves the window-cache and replica-mirror invariant even when a `View::on_edit` failure occurs after the rope swap. The Rust return type of `set_generated_contents` changes with the internal transaction; its Lua name and call signature do not. New criterion 15a pins both the window and CRDT directions. | +| **P1-2** — criterion 15 probes the edit flag while `read_only` masks it | Confirmed. `begin_edit` calls `ensure_writable` before it checks `editing_in_progress`, while `AppliedThenFailed` deliberately relocks. The old follow-up therefore returned `ReadOnly` for both the correct and broken implementations. Criterion 15 now performs the same Rust-side lift already used by criteria 4, 16b and 17 before issuing the ordinary edit; only then can its outcome distinguish the cleared flag from `ConcurrentEdit`. | +| **P1-3** — the four-variant fallback maps divergence to `Rejected` and restores a fresh buffer to writable | Confirmed. The fallback is withdrawn. A CRDT delete-success/insert-failure must be `Diverged`, must leave the buffer locked, and must surface distinctly. Criterion 16c is reclassified as a `crdt`-only fault-injection unit test: Stage 2 extracts a private delete→insert classifier whose production closures call loro and whose test closures force delete `Ok` / insert `Err`. There is no implementation-time choice to weaken containment because staging is inconvenient. | +| **P1-4** — `identity_protected` is not generated-lock provenance | Confirmed. Q#GB7 now chooses the fallback it previously named: **this arc ships no `unlock_generated` at all.** `identity_protected` remains, but only as a monotonic terminal-identity guard in the write direction; it is never described as proof that the generated primitive installed a lock. The proposed setter becomes a crate-private, one-way `mark_identity_protected()` so “set once” is enforced rather than documentary. A future wdired door stays with dired Stage 3 and must frame the provenance/ownership transition it actually needs. | +| **P2-5** — Stage 2 still instructs implementation to build revision 3's rejected shared registry | Confirmed. The staging checklist now matches Q#GB18 exactly: `compile.lua` answers from `slot_for_buffer`, `default.lua` answers from `search_panel_owns`, and each capture site consults the other through a guarded optional predicate. No owner registers buffers with the other, and no teardown work is introduced. | + +**Reachability recheck for the changed criteria.** Criterion 15 now +crosses the post-failure lock only by a Rust-side lift, so the next edit +can actually reach the flag it asserts. Criterion 15a drives the same +valid `FailingView` mutation through the production Lua finisher and +observes its window and replica consumers. Criterion 16c drives the +otherwise-unconstructible second-op failure through the private +delete→insert classifier. Criterion 13 is deliberately structural +because revision 5's decision is the absence of a capability. None +passes merely by failing before the mechanism under test. + +Revision 4's measurements and its answers to review round 3 remain +below as history. Where its live design said “bounded unlock” or offered +the four-variant CRDT fallback, revision 5 supersedes it explicitly +rather than silently smoothing the reversal over. ## Revision 4 @@ -245,10 +277,11 @@ Two things the arc turns out NOT to be, both discovered by measurement: says it prevents. Q#GB18. - **Added in revision 3:** `read_only` is **not** this arc's flag. It carries three unrelated authorities (§2.11), so a capability defined - over it reaches all three — which is why the unlock needed provenance - (Q#GB15), why the lock silently disables fold creation (Q#GB16), and - why the *shipped* primitive can already overwrite a live terminal's - identity buffer. + over it reaches all three — which is why revision 5 removes the unlock + rather than pretending `identity_protected` is provenance (Q#GB7), + why the lock silently disables fold creation (Q#GB16), and why the + *shipped* primitive can already overwrite a live terminal's identity + buffer (Q#GB15). --- @@ -947,7 +980,7 @@ are the two consequences. ### 3.1 Recommendation -**`Buffer::apply_generated_edit(op: EditOp) -> Result` +**`Buffer::apply_generated_edit(op: EditOp) -> GeneratedOutcome` — one authorized op at a time — exposed to Lua as a new option key on the mutators that already exist:** @@ -960,15 +993,23 @@ buf:replace(s, e, text, { generated = true }) Semantics, per call, entirely inside one `with_registry_mut` and one `&mut Buffer` method — the exact ordering, including every error path, is §3.4, which revision 3 adds because review P1-1 showed revision 2 had no -workable one. The binding then fans the `Edit` out through the -`notify_buffer_edit_to_windows` call it **already makes** -(`src/lua_bindings/mod.rs:1291`, `:1302`, `:1322`), after the borrow has -dropped. +workable one. The binding then fans every outcome that carries an +`Edit` out through `notify_buffer_edit_to_windows` after the borrow has +dropped: `Applied` and `NoOp` before returning success, and +`AppliedThenFailed` **before returning its error**. Revision 5 makes that +third arm explicit because collapsing the outcome to `Result` inside +`Buffer` discarded the only value capable of updating window caches and +replica mirrors. `Buffer::set_generated_contents(bytes)` is reimplemented as -`apply_generated_edit(Replace { range: 0..len, bytes })`. **It keeps its -name, its signature, its doc comment and its tests** — it becomes the -whole-buffer spelling of one primitive rather than a second primitive. +`apply_generated_edit(Replace { range: 0..len, bytes })`. **Its Lua name +and call signature stay fixed; its Rust return changes from +`Result` to `GeneratedOutcome`**, and its direct Rust +tests match on the outcome. The change is required: a wrapper that +collapses `AppliedThenFailed` to `Err` before the binding sees it loses +the applied `Edit` and cannot satisfy the fan-out contract. It becomes +the whole-buffer spelling of one primitive rather than a second +primitive. **One sentence for why it wins: it is the only candidate in which the buffer is never observably unlocked, because the lift and the re-assert @@ -1036,12 +1077,16 @@ in Q#GB8's deferral, not adopted here. ### 3.3 The four questions the recommendation must answer -**How many `Edit`s are fanned out, and when?** One per generated op, -immediately, by the binding that already does it. `compile.lua`'s +**How many `Edit`s are fanned out, and when?** One per generated op that +reaches an `Edit`, immediately, by the binding. That includes +`AppliedThenFailed`: the rope and CRDT have already changed, so the +binding fans its carried `Edit` out and only then returns the carried +error. `compile.lua`'s `emit_text` fast path emits one insert for a whole output batch, so a typical `feed_bytes` produces one to three ops; a CR-heavy progress bar -produces more. This is exactly today's fan-out count — the conversion -changes authority, not cardinality. +produces more. Successful calls retain today's fan-out cardinality; the +new failure arm adds the notification that today's early `Err` loses, +because that error can follow a real mutation. **Per-op or per-scope history clearing?** Per op, and it is cheap by construction: because `read_only` is re-asserted immediately, **at most @@ -1056,19 +1101,46 @@ against the existing compile-mode timings in both configurations. If it does, the escape hatch is to suppress recording rather than clear it — recorded as a named deferral rather than designed speculatively. -**CRDT-mode behaviour?** Identical to `set_generated_contents` today. The -`Edit` carries `crdt_op` when the buffer is CRDT-backed, and +**CRDT-mode behaviour?** Identical to `set_generated_contents` today on +success, and corrected on the post-apply error path. The `Edit` carries +`crdt_op` when the buffer is CRDT-backed, and `notify_buffer_edit_to_windows` queues it via `queue_daemon_origin_crdt_op` (`src/lua_bindings/mod.rs:1582`) so replica -mirrors import the owner's write. History clearing goes to loro's -`UndoManager`. Nothing new. +mirrors import the owner's write. `AppliedThenFailed` must carry that +same `Edit`; returning its error before notification would leave the +authoritative CRDT changed while every replica missed the op. History +clearing goes to loro's `UndoManager`. **How do the returned edits reach the fan-out without a live registry -borrow?** By construction, unchanged since #178: `run_bypass_edit` +borrow?** By construction, extending #178's shape: `run_bypass_edit` (`src/lua_bindings/mod.rs:1445`) closes its `with_registry_mut` before returning, and the mutator bindings call `notify_buffer_edit_to_windows` afterwards. `run_generated_edit` (§3.4) -occupies the same position and closes its borrow the same way. +occupies the same position and closes its borrow the same way, but +returns the whole `GeneratedOutcome`. A binding-level finisher handles +it: + +```rust +match run_generated_edit(lua, id, op) { + Applied(edit) | NoOp(edit) => { + notify_buffer_edit_to_windows(lua, id, &edit); + Ok(edit) + } + AppliedThenFailed { edit, error } => { + notify_buffer_edit_to_windows(lua, id, &edit); + Err(error.into()) + } + Rejected(error) => Err(error.into()), + Diverged(error) => { + surface_divergence(lua, &error); + Err(error.into()) + } +} +``` + +The helper is shared by the three mutators and the +`set_generated_contents` binding so one of the four cannot forget the +failure fan-out. ### 3.4 The transaction, and why revision 2 had none (review P1-1) @@ -1159,7 +1231,7 @@ for the v0.1 undo stack and does not extend past it.** Three failures: the no-op arm (`src/buffer.rs:1245-1253`), returns `Ok`, never bumps `revision`, so revision 3 skipped **both** the clear and `mark_clean`: the buffer ends locked, **modified**, and carrying - poppable history that `unlock_generated` re-exposes. Shipped + poppable history that a later Rust-side lift can re-expose. Shipped `set_generated_contents` clears unconditionally (`:551-553`), so revision 3's predicate was a **regression of an existing contract**, not a refinement of one. And it lands on a path this document @@ -1192,34 +1264,53 @@ signature change and not a free one — see the cost note below. ```rust /// What a generated write did, reported by the apply rather than /// inferred by its caller. The variant, not `revision`, selects cleanup. -enum GeneratedOutcome { +pub enum GeneratedOutcome { /// The rope changed and an undo entry exists. Applied(Edit), /// A semantic no-op: rope unchanged, no undo entry pushed. The call /// SUCCEEDED, so the buffer is now a generated buffer. NoOp(Edit), - /// Refused before anything mutated: rope, CRDT and history are all - /// exactly as they were. + /// Failed with rope, CRDT and history exactly as they were. This also + /// includes a deliberate no-op whose `on_edit` notification failed: + /// view side effects may have run, but no buffer/CRDT mutation exists + /// to fan out. Rejected(BufferError), - /// The rope changed and a later stage failed. An undo entry exists - /// over contents no caller can see. - AppliedThenFailed(BufferError), + /// The rope changed and a later stage failed. The Edit must survive so + /// windows and replica mirrors observe the mutation before the error. + AppliedThenFailed { edit: Edit, error: BufferError }, /// CRDT only: the CRDT was partially mutated and the rope was not. /// `rope ≡ CRDT projection` no longer holds. Diverged(BufferError), } ``` +`run_rope_edit_and_broadcast` returns this richer outcome. +`apply_edit` and `apply_edit_skip_intercepts` map it back to their +existing `Result` API, preserving ordinary callers' +surface; the generated path retains it through cleanup and the +borrow-free binding finisher. The enum is public because the existing +public `set_generated_contents` method and the new +`apply_generated_edit` method return it; their Rust docs state that a +higher layer that owns window or replica state must fan out every +edit-bearing variant before handling its success/error. + +That mapping deliberately does **not** claim to repair the same +post-apply notification loss for every ordinary Rust edit API. Their +public `Result` surface cannot carry both the edit and +the later view error, and changing all of those callers is broader than +generated-buffer immutability. The pre-existing ordinary path is named +in §8 rather than hidden by the helper refactor. + **The cleanup each variant triggers.** `entry` is the `read_only` value observed on entry. -| outcome | history | `mark_clean` | `read_only` after | returns | +| outcome | history | `mark_clean` | `read_only` after | binding action | |---|---|---|---|---| -| `Applied` | **cleared** | yes | `true` | `Ok(Edit)` | -| `NoOp` | **cleared** | yes | `true` | `Ok(Edit)` | -| `Rejected` | untouched | no | **restored to `entry`** | `Err` | -| `AppliedThenFailed` | **cleared** | **no** | `true` | `Err` | -| `Diverged` | **untouched** | no | `true` | `Err`, and it must **surface** | +| `Applied` | **cleared** | yes | `true` | fan out `Edit`, then `Ok` | +| `NoOp` | **cleared** | yes | `true` | fan out `Edit`, then `Ok` | +| `Rejected` | untouched | no | **restored to `entry`** | `Err`, no fan-out | +| `AppliedThenFailed` | **cleared** | **no** | `true` | **fan out carried `Edit`, then `Err`** | +| `Diverged` | **untouched** | no | `true` | distinct surfaced `Err`, no rope `Edit` | `editing_in_progress` is cleared on **all five**, unconditionally. @@ -1247,9 +1338,11 @@ that **this arc cannot fix it**: - The rope is intact and the CRDT is not. Nothing local reconstructs the deleted range — loro exposes no rollback at this seam, and the `export_updates_since` that would name the delta runs after both ops. -- **Clearing history would destroy the last local record of the - pre-edit rope**, which is the only material anything could later - reconcile from. So `Diverged` clears nothing. +- The rope still contains the pre-edit bytes, but the CRDT UndoManager is + the only existing **CRDT-native inverse record** for the successful + delete. Clearing it would discard the one operation a later repair + lane might use to reconcile the document. So `Diverged` clears + nothing; this arc does not invoke that undo automatically. - Locking is the strongest available *containment*: it stops further ops compounding a divergence that already exists. So `read_only = true`, and this is the one place where locking a buffer on an error path is @@ -1270,18 +1363,24 @@ lane.** Splitting a CRDT `Replace` into a single transactional op, or reconciling the two, is loro-level work with no bearing on generated buffers specifically. -**The ordering, with every exit path named.** +**The ordering, with every exit path named.** The method returns the +outcome intact; only the borrow-free binding finisher above converts it +to Lua success/error after performing any required fan-out. ```rust -pub fn apply_generated_edit(&mut self, op: EditOp<'_>) -> Result { +pub fn apply_generated_edit(&mut self, op: EditOp<'_>) -> GeneratedOutcome { // (1) Q#GB10: path-backed refusal. Before any state change. - if self.file_path.is_some() { return Err(GeneratedWriteOnFileBuffer { .. }); } + if self.file_path.is_some() { + return Rejected(GeneratedWriteOnFileBuffer { .. }); + } // (2) Q#GB15: this buffer's read_only is an identity protection. - if self.identity_protected { return Err(ReadOnly { .. }); } + if self.identity_protected { return Rejected(ReadOnly { .. }); } // (3) re-entrancy gate — begin_edit's SECOND check, not its first. - if self.editing_in_progress { return Err(ConcurrentEdit { .. }); } + if self.editing_in_progress { return Rejected(ConcurrentEdit { .. }); } // (4) bounds pre-validation, so an invalid range costs nothing. - self.validate_op_bounds(&op)?; + if let Err(error) = self.validate_op_bounds(&op) { + return Rejected(error); + } let entry_read_only = self.read_only; self.editing_in_progress = true; @@ -1293,12 +1392,15 @@ pub fn apply_generated_edit(&mut self, op: EditOp<'_>) -> Result { self.read_only = true; self.clear_history(); } + AppliedThenFailed { .. } => { + self.read_only = true; + self.clear_history(); + } Diverged(_) => { self.read_only = true; } Rejected(_) => { self.read_only = entry_read_only; } } self.editing_in_progress = false; // (6) unconditional, all paths - outcome.into_result() + outcome // binding owns fan-out + conversion } ``` @@ -1309,7 +1411,7 @@ pub fn apply_generated_edit(&mut self, op: EditOp<'_>) -> Result` surface in this arc. Generated bindings + retain and fan out the edit; generalizing that contract across every + Rust and Lua edit caller is separate work. - **The five `*scratch*` find-or-create copies** (§2.10 Class 5) — `default.lua:581`, `:1145`, `listview.lua:190`, `compile.lua:1052`, `dired.lua:914`. Correct only while `*scratch*` stays unowned and @@ -2675,10 +2811,10 @@ test.** Every criterion below names its exit. to ship an unlock. It is a **pre-existing** exposure: `pmacs.buffer.set_generated_contents` is already public on `main` and already locks any buffer id it is handed. This arc neither creates it - nor closes it — Q#GB15's provenance bounds who may *unlock*, not who - may *lock*. Recorded as a standing hazard rather than as this arc's - work, which is the correction revision 3 makes to sweep B's - conclusion. + nor closes it, and revision 5 deliberately adds no unlock because + `identity_protected` cannot prove who installed the lock. Recorded as + a standing hazard rather than as this arc's work, which is the + correction revision 3 began and revision 5 completes. - **`docs/agent-handoff.md` §4's inventory is keyed by `bypass_intercept`** and therefore misses Class C, and its headline "four writer mechanisms" is **five** once `src/help.rs:354` is counted @@ -2687,10 +2823,12 @@ test.** Every criterion below names its exit. - **Removed from this list in revision 3: `COHERENCE.md` §14's listview consumer list.** PR #189 landed the correction (§1.5). A merged correction is removed, not relabelled. -- **Returned to this list in revision 3: wdired's unlock.** Revision 1 - deferred it, revision 2 made it Stage 1 work, and revision 3 lands the - *capability* in Stage 2 (Q#GB7 + Q#GB15) while the **wdired consumer** - itself stays deferred to dired Stage 3, which is not framed. +- **Wdired's generated→editable transition.** Revision 1 deferred it, + revision 2 made a general unlock Stage 1 work, revision 3 moved that + capability to Stage 2, and revision 5 removes it. The **wdired + consumer** stays deferred to dired Stage 3, which must frame an + owner-specific transition or the eventual policy enum rather than + inheriting a Lua clear of `read_only`. ## 9. Coherence impact (`COHERENCE.md` §20) @@ -2835,8 +2973,11 @@ Plus, per stage: nothing about the seam P1-3 is about. Judge it by whether the test count includes `acc16e`, not by the verdict alone. - **Stage 2 additionally needs a `crdt` run of whatever suite hosts the - §3.4 transaction criteria**, for criterion 16's second half and 16c. - Same reasoning, same failure mode. + §3.4 transaction criteria**, for criterion 15a's replica half and + criterion 16's second half. Criterion 16c lives in + `cargo test --lib --features crdt` because its private + delete-success/insert-failure classifier is a unit-test fault seam, + not a public acceptance input. Same reasoning, same failure mode. - **Run `scripts/bite` on every criterion expressible as a test today.** Stage 1 criteria 1–3, 8, 9 and Stage 2 criteria 1, 7, 8, 9, 14 have `main` pre-images and can be falsified by revert; the rest are