From a118f0a3a2c43f0546401f7c2e4b3dcf7f0ac9b1 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 8 Jul 2026 17:07:08 -0400 Subject: [PATCH] fix(query-replace): pin the session to its origin buffer (wrong-buffer guard) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review High (merge blocker): query-replace searched the origin buffer's bytes but applied edits and moved the cursor through apply_active_edit / search_place_cursor, which target whatever is ACTIVE. Focus can drift mid-session — a click into another split, a key from another frontend, both changing the active buffer outside the shadow — so a match found in the origin buffer could be applied to an unrelated one. Buffer corruption. Fix: query_replace_on_origin() verifies the active buffer still equals the session's origin buffer before every edit; on mismatch it ABORTS without editing (clears the highlight, drops the session, status 'query-replace aborted: active buffer changed'), so an origin match can never land in a foreign buffer. Guards replace/skip/all/replace-and-quit. The dispatcher's after-edit revision compare now targets the ORIGIN buffer (query_replace_origin_buffer + buffer_revision) not the active one, so a drift-abort — which edits nothing — never spuriously fires the hook. The forward-search clamp uses the origin bytes' length, not active_buffer_len. Also (review Low/med): query_replace_active() added to the completion-popup modal-close guard, so a popup opened via the direct Lua start (ed.query_replace_start) can't linger rendered-but-unreachable while QR swallows keys. Tests: core drift-abort (both buffers untouched) + end-to-end focus-drift regression; ! fires after-edit exactly once for the batch; RET/Esc/C-g quit paths (keep replacements); DEL skips. Acceptance header corrected to match actual coverage. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/query-replace-framing.md | 15 +++- src/editor.rs | 23 ++++- src/editor_core.rs | 89 +++++++++++++++++- tests/query_replace_acceptance.rs | 144 +++++++++++++++++++++++++++++- 4 files changed, 260 insertions(+), 11 deletions(-) diff --git a/docs/query-replace-framing.md b/docs/query-replace-framing.md index e2a454e..b05f419 100644 --- a/docs/query-replace-framing.md +++ b/docs/query-replace-framing.md @@ -60,7 +60,20 @@ interactive y/n phase). Mirror the structure instead: `editor.rs` — the fifth member of the shadow family (minibuffer, search, menu, completion, **query-replace**). Add `query_replace_active()` to the `dispatch_idle()` disjunction and the -modal-close guard, exactly as the others. +completion-popup modal-close guard, exactly as the others. + +**Pin to the origin buffer (as-built fix).** The session records its +origin buffer, but every edit and cursor move goes through the +*active* window/buffer — and focus can drift mid-session (a click into +another split, a key from another frontend, both of which change the +active buffer outside the shadow). Applying an origin-buffer match to +whatever became active is buffer corruption. Guard it: +`query_replace_on_origin()` checks the active buffer still equals the +origin buffer before every edit and **aborts the session without +editing** on mismatch (never corrupt an unrelated buffer). The +`buffer.after-edit` revision compare (above) targets the *origin* +buffer specifically, not the active one, so a focus-drift abort — which +edits nothing — never spuriously fires the hook. **`buffer.after-edit` must fire from inside the shadow (P1).** A modal shadow `return`s before `dispatch_key`'s normal post-command edit diff --git a/src/editor.rs b/src/editor.rs index 5d45d0a..7656365 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -553,7 +553,10 @@ impl EditorState { { let mut core = self.core.borrow_mut(); if core.completion_popup_is_open() - && (core.menu_is_open() || core.search_active() || core.minibuffer.is_active()) + && (core.menu_is_open() + || core.search_active() + || core.query_replace_active() + || core.minibuffer.is_active()) { core.completion_popup_close(); } @@ -692,6 +695,13 @@ impl EditorState { /// mid-dispatch). fn active_buffer_revision(&self) -> Option { let id = self.core.borrow().active_buffer_id(); + self.buffer_revision(id) + } + + /// Edit revision of a specific buffer, or `None` if the registry no + /// longer knows it. Used by the query-replace shadow to compare the + /// *edited* (origin) buffer, not whichever is active. + fn buffer_revision(&self, id: crate::buffer::BufferId) -> Option { let reg = self.lua_host.registry().borrow(); reg.get(id).ok().map(crate::buffer::Buffer::revision) } @@ -832,7 +842,11 @@ impl EditorState { /// the hook once for the batch, which is what the debounced /// `didChange` wants. fn dispatch_query_replace_key(&mut self, chord: Chord) { - let pre_revision = self.active_buffer_revision(); + // Compare the *origin* buffer's revision (the one query-replace + // edits), not the active buffer's — they can differ if focus + // drifted, and the wrong-buffer guard may abort without editing. + let origin_buf = self.core.borrow().query_replace_origin_buffer(); + let pre = origin_buf.and_then(|id| self.buffer_revision(id)); match QueryReplaceKey::from_chord(chord) { QueryReplaceKey::Replace => self.core.borrow_mut().query_replace_replace(), QueryReplaceKey::Skip => self.core.borrow_mut().query_replace_skip(), @@ -843,7 +857,10 @@ impl EditorState { QueryReplaceKey::Quit => self.core.borrow_mut().query_replace_finish(), QueryReplaceKey::Ignore => {} } - if pre_revision != self.active_buffer_revision() { + // `!` applies many edits under one keypress; the single compare + // fires `buffer.after-edit` once for the batch (Q#QR1). + let post = origin_buf.and_then(|id| self.buffer_revision(id)); + if origin_buf.is_some() && pre != post { self.lua_host .run_hook("buffer.after-edit", mlua::MultiValue::new()); } diff --git a/src/editor_core.rs b/src/editor_core.rs index 35e163b..9eae480 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -824,6 +824,40 @@ impl EditorCore { self.query_replace.is_some() } + /// The buffer a running query-replace is pinned to, or `None`. The + /// dispatcher reads this so the `buffer.after-edit` revision compare + /// targets the *edited* buffer, not whichever is active. + #[must_use] + pub fn query_replace_origin_buffer(&self) -> Option { + self.query_replace.as_ref().map(|s| s.origin.0) + } + + /// Query-replace's wrong-buffer guard. Every edit and cursor move it + /// makes goes through the *active* window/buffer, but the session is + /// pinned to the buffer it started in — and focus can drift + /// mid-session (a click into another split, a key from another + /// frontend). Before touching the buffer, verify the active buffer + /// is still the origin buffer; if not, **abort without editing** so + /// a match found in the origin buffer can never be applied to an + /// unrelated one. Returns `true` when it is safe to proceed. + fn query_replace_on_origin(&mut self) -> bool { + let Some(origin_bid) = self.query_replace.as_ref().map(|s| s.origin.0) else { + return false; + }; + if self.active_buffer_id() == origin_bid { + return true; + } + // Focus moved off the origin buffer — abort, don't corrupt. + if let Some(session) = self.query_replace.take() { + self.search_store + .lock() + .expect("search store mutex poisoned") + .clear(session.origin.0); + } + self.status = "query-replace aborted: active buffer changed".into(); + false + } + /// Begin a query-replace from the cursor forward (Q#QR8). `regex` /// selects `query-replace-regexp` (Q#QR9). An invalid regex refuses /// to start (Q#QR2). Immediately advances to (and prompts on) the @@ -866,8 +900,8 @@ impl EditorCore { return; }; let bid = session.origin.0; - let start = session.next_from.min(self.active_buffer_len()) as usize; let bytes = self.buffer_bytes(bid); + let start = (session.next_from as usize).min(bytes.len()); let found = match &session.re { Some(re) => crate::search::find_first_regex_from(&bytes, re, start), None => crate::search::find_first_from(&bytes, &session.from, start), @@ -940,13 +974,16 @@ impl EditorCore { /// `y` / `SPC` — replace the current match, then advance to the next. pub fn query_replace_replace(&mut self) { - if self.query_replace_apply_current() { + if self.query_replace_on_origin() && self.query_replace_apply_current() { self.query_replace_advance(); } } /// `n` / `DEL` — leave the current match, advance past it to the next. pub fn query_replace_skip(&mut self) { + if !self.query_replace_on_origin() { + return; + } if let Some(session) = self.query_replace.as_mut() && let Some(range) = session.current { @@ -960,6 +997,9 @@ impl EditorCore { /// prompting, then finish (Q#QR6). One `after-edit` hook fires for /// the batch (the dispatcher compares revision across the handler). pub fn query_replace_all(&mut self) { + if !self.query_replace_on_origin() { + return; + } while self.query_replace_apply_current() { // Find the next match (mirrors advance's search, without the // highlight/prompt work — we're not stopping to ask). @@ -967,8 +1007,8 @@ impl EditorCore { break; }; let bid = session.origin.0; - let start = session.next_from.min(self.active_buffer_len()) as usize; let bytes = self.buffer_bytes(bid); + let start = (session.next_from as usize).min(bytes.len()); let found = match &session.re { Some(re) => crate::search::find_first_regex_from(&bytes, re, start), None => crate::search::find_first_from(&bytes, &session.from, start), @@ -987,6 +1027,9 @@ impl EditorCore { /// `.` — replace the current match, then finish (Q#QR6). pub fn query_replace_replace_and_quit(&mut self) { + if !self.query_replace_on_origin() { + return; + } self.query_replace_apply_current(); self.query_replace_finish(); } @@ -3747,6 +3790,46 @@ mod tests { assert_eq!(text_of(&s), "k _ K\n", "only the match at/after point"); } + #[test] + fn query_replace_aborts_when_active_buffer_changes() { + // The wrong-buffer merge-blocker: a session started in buffer X + // must never apply its match to a buffer that became active + // mid-session. Focus drifts (a click / cross-frontend key), then + // the next replace key aborts safely instead of corrupting. + let mut s = from_bytes(b"foo foo\n"); + let x = s.active_buffer_id(); + s.query_replace_begin("foo".into(), "bar".into(), false); + assert!(s.query_replace_active()); + + // Switch the active buffer to an unrelated one (focus drift). + let y = s.registry.borrow_mut().create("*other*"); + { + let reg = s.registry.borrow(); + let buf = reg.get(y).unwrap(); + let tv = crate::text_view::TextView::new(buf); + drop(reg); + let win = s.active_window_mut(); + win.buffer_id = y; + win.text_view = tv; + win.cursor = 0; + } + assert_eq!(s.active_buffer_id(), y); + + s.query_replace_replace(); // the y/replace key while drifted + assert!(!s.query_replace_active(), "drift aborts the session"); + assert_eq!(s.status, "query-replace aborted: active buffer changed"); + // Neither buffer was mutated by the aborted replace. + { + let reg = s.registry.borrow(); + let bx = reg.get(x).unwrap(); + let mut xb = vec![0u8; bx.len() as usize]; + bx.snapshot_rope().slice(0, bx.len(), &mut xb); + assert_eq!(&xb, b"foo foo\n", "origin buffer X untouched"); + let by = reg.get(y).unwrap(); + assert_eq!(by.len(), 0, "unrelated buffer Y untouched"); + } + } + #[test] fn query_replace_regex_replaces_and_invalid_refuses() { let mut s = from_bytes(b"a1 b2 c3\n"); diff --git a/tests/query_replace_acceptance.rs b/tests/query_replace_acceptance.rs index e9e0660..4fa69b3 100644 --- a/tests/query_replace_acceptance.rs +++ b/tests/query_replace_acceptance.rs @@ -1,9 +1,12 @@ //! Query-replace acceptance (Arc 2) — the interactive phase end-to-end //! through `dispatch_key`, exactly as a user (or a round-tripping GPU) -//! drives it: the `M-%` / `C-M-%` bindings, the y/n/!/./q vocabulary, -//! the three quit paths + nothing-matched-restores-origin, empty-to -//! deletion, offset-shift correctness, regex, the `buffer.after-edit` -//! hook firing on replaced text, and the `dispatch_idle`-false gate. +//! drives it: the `M-%` / `C-M-%` bindings; the full key vocabulary +//! (`y`/`SPC` replace, `n`/`DEL` skip, `!` all, `.` last); every quit +//! path (`q`, `RET`, `Esc`, `C-g`) keeping replacements, plus +//! nothing-matched-restores-origin; empty-to deletion; offset-shift +//! correctness (`a`→`aa` doesn't loop); regex; the `buffer.after-edit` +//! hook (once per `y`, and exactly once for an `!` batch); the +//! `dispatch_idle`-false gate; and the wrong-buffer/focus-drift abort. //! //! Framing: docs/query-replace-framing.md. @@ -280,3 +283,136 @@ fn replace_fires_after_edit_hook() { "buffer.after-edit fired for the replacement (before {before}, after {after})" ); } + +#[test] +fn bang_fires_after_edit_hook_once_for_the_batch() { + // Q#QR1: `!` applies many replacements under one keypress, but the + // debounced didChange wants a single after-edit — the shadow + // compares revision once across the whole handler. + let mut s = EditorState::new(); + s.lua_host + .lua() + .load( + r" + _G.EDITS = 0 + pmacs.hook.add('buffer.after-edit', function() _G.EDITS = _G.EDITS + 1 end) + ", + ) + .exec() + .expect("install after-edit counter"); + type_str(&mut s, "a a a a"); + goto_start(&s); + // Zero out the counter after the typing edits. + s.lua_host.lua().load("_G.EDITS = 0").exec().ok(); + start_query_replace(&mut s, "a", "b", false); + s.lua_host.lua().load("_G.EDITS = 0").exec().ok(); + press(&mut s, KeyCode::Char('!')); // four replacements in one keypress + let edits: i64 = s + .lua_host + .lua() + .load("return _G.EDITS") + .eval() + .expect("read counter"); + let (text, _) = probe(&s); + assert_eq!(text, "b b b b", "! replaced all four"); + assert_eq!(edits, 1, "after-edit fires exactly once for the ! batch"); +} + +#[test] +fn quit_via_ret_and_esc_keeps_replacements() { + // Q#QR10: RET and Esc both quit (keeping replacements), not just q. + for quit in [KeyCode::Enter, KeyCode::Esc] { + let mut s = EditorState::new(); + type_str(&mut s, "a a a"); + goto_start(&s); + start_query_replace(&mut s, "a", "b", false); + press(&mut s, KeyCode::Char('y')); // replace the first + press(&mut s, quit); // quit before the rest + let (text, active) = probe(&s); + assert_eq!(text, "b a a", "quit keeps the one replacement ({quit:?})"); + assert!(!active, "{quit:?} ends the session"); + } +} + +#[test] +fn ctrl_g_quits_keeping_replacements() { + // Q#QR10: C-g exits and KEEPS replacements (unlike isearch's C-g). + let mut s = EditorState::new(); + type_str(&mut s, "a a a"); + goto_start(&s); + start_query_replace(&mut s, "a", "b", false); + press(&mut s, KeyCode::Char('y')); + s.dispatch_key( + FrontendId::LOCAL, + key(KeyCode::Char('g'), KeyModifiers::CONTROL), + ); + let (text, active) = probe(&s); + assert_eq!(text, "b a a", "C-g keeps replacements (not an undo)"); + assert!(!active); +} + +#[test] +fn del_key_skips_like_n() { + let mut s = EditorState::new(); + type_str(&mut s, "a a a"); + goto_start(&s); + start_query_replace(&mut s, "a", "b", false); + press(&mut s, KeyCode::Backspace); // DEL/Backspace → skip first + press(&mut s, KeyCode::Char('y')); // replace second + press(&mut s, KeyCode::Char('q')); + let (text, _) = probe(&s); + assert_eq!( + text, "a b a", + "DEL skipped the first, y replaced the second" + ); +} + +#[test] +fn focus_drift_mid_session_aborts_without_touching_either_buffer() { + // The merge-blocker, end-to-end: a click into another buffer + // (simulated by switch_buffer, which the pointer path also uses) + // while query-replace is active. The next y must abort, not apply + // the origin-buffer match to the now-active unrelated buffer. + let mut s = EditorState::new(); + type_str(&mut s, "foo foo"); + goto_start(&s); + start_query_replace(&mut s, "foo", "bar", false); + assert!(probe(&s).1, "session active on the first match"); + + // Focus drifts to a fresh, unrelated buffer. + s.lua_host + .lua() + .load( + r" + _G.OTHER = pmacs.buffer.create('*drift*') + pmacs.window.switch_buffer(_G.OTHER) + ", + ) + .exec() + .expect("switch to another buffer"); + + press(&mut s, KeyCode::Char('y')); // the replace key, now drifted + assert!(!probe(&s).1, "drift aborts the session"); + let (drift_text, _) = probe(&s); // active buffer is *drift* + assert_eq!(drift_text, "", "the unrelated buffer was not edited"); + + // The origin buffer is also intact — switch back and check. + s.lua_host + .lua() + .load( + r" + for _, id in ipairs(pmacs.buffer.list()) do + if pmacs.describe.buffer(id).name == '*scratch*' then + pmacs.window.switch_buffer(id) + end + end + ", + ) + .exec() + .ok(); + assert_eq!( + probe(&s).0, + "foo foo", + "origin buffer untouched by the aborted replace" + ); +}