From 25b07be97b97e2ce10947cad0bc750c1ecf72499 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 26 Jul 2026 16:39:23 -0400 Subject: [PATCH] fix(journey): correct Q#JR3, report the post-dispatch buffer, unvacuate two pins Four review findings, all confirmed against the tree. Q#JR3 was false. `replace_active_buffer` does not drop the startup scratch buffer -- its body is one `switch_active_buffer` call, which reassigns the window's buffer_id and removes nothing. The claim came from that function's own doc comment, wrong for as long as it has existed, and rev 5 propagated it into the framing and into new documentation this branch added. Both comments are corrected here, because this PR was adding further false references to a claim P4 depends on. Actually removing the stale scratch is buffer-lifetime work and stays out. The daemon bootstrap could report the wrong buffer. The directory arm captured the destination id, ran the resolver chain synchronously, then returned the captured id -- so a handler that opened something synchronously through commit_to had already replaced the window's buffer, and the reply paired one buffer's snapshot with another's identity. It also returned early, skipping the post-hook revalidation the framing said stayed active. The arm now re-reads the destination after dispatch and rehomes through `non_side_target` as the file arm does. Pinned by a test whose handler claims synchronously. N11 tested neither RET nor self-insert: it called display_file and buf:insert directly, so it stayed green with dired's RET binding, its entry dispatch, and the editor's self-insert path all broken. Both gestures now go through dispatch_key. P7 is removed rather than weakened. Q#JR12 has nothing to pin -- `had_file = file.is_some()` and a directory is Some like any other, so no directory-specific branch exists to break. The old test never armed restore and hard-coded had_file, so it could not fail against any implementation. Also adds the daemon bootstrap pins (N2, N5) and fixes an insertion that had orphaned a `#[cfg(feature = "crdt")]` from the test it guarded -- which would have made one new test dark and one existing test escape its gate. Framing: docs/journey-stage1a-framing.md rev 6. --- docs/journey-stage1a-framing.md | 110 ++++++++++++++--- src/daemon.rs | 213 +++++++++++++++++++++++++++++++- src/editor.rs | 32 +++-- tests/journey_acceptance.rs | 104 +++++++++++----- 4 files changed, 401 insertions(+), 58 deletions(-) diff --git a/docs/journey-stage1a-framing.md b/docs/journey-stage1a-framing.md index 36f40e4..b5833cc 100644 --- a/docs/journey-stage1a-framing.md +++ b/docs/journey-stage1a-framing.md @@ -1,6 +1,7 @@ # Journey Stage 1a — open a directory, on one path -**Status: framing, rev 5, awaiting approval.** +**Status: framing, rev 6 — APPROVED at rev 5; rev 6 records +corrections found during implementation.** **Serves `COHERENCE.md` §2 (the golden product journey), §19 (coherence acceptance tests), §20 Priority 1.** @@ -76,6 +77,43 @@ acceptance tests), §20 Priority 1.** - The dired accessor spelling, revision heading, and Stage 2 ledger claim are corrected. +- rev 6 (2026-07-26) — **corrections found while implementing**, not a + new design round. Four, all confirmed against the tree: + - **Q#JR3 was false.** `replace_active_buffer` does *not* drop the + startup scratch buffer; its body is one `switch_active_buffer` call, + which reassigns `aw.buffer_id` and removes nothing. The claim came + from that function's own doc comment (`editor.rs:1071`), which has + been wrong for as long as it has existed, and rev 5 propagated it + into §2.2, §3, P4, and the decision list without checking the body. + Corrected in all four places; the stale comment is corrected in this + PR too, since this PR would otherwise add *more* false references to + it. **Actually removing the stale scratch is separate work** — + buffer-lifetime changes have their own consequences (what else holds + the id, what `C-x b` lists) and are not smuggled into a directory-open + stage. + - **The daemon bootstrap could report the wrong buffer** (§4.5). The + directory arm captured `dest.buffer`, ran the resolver chain + *synchronously*, then returned the captured id — so a handler that + opened something synchronously (through `commit_to`, the supported + way) had already replaced the window's buffer, and the reply would + pair one buffer's snapshot with another's identity. The early return + also skipped the post-hook revalidation this framing claimed stayed + active. Rev 6 decides: **report what the window actually holds after + the dispatch**, and rehome through `non_side_target` exactly as the + file arm does. + - **N11 tested neither `RET` nor self-insert.** It called + `display_file` and `buf:insert` directly, so it stayed green with + dired's `RET` binding, its entry dispatch, and the editor's + self-insert path all broken — most of what "the journey works" means. + Both gestures are now dispatched as real keys. + - **P7 was vacuous and is removed, not weakened.** Q#JR12 has nothing + to pin: `run` computes `had_file = file.is_some()` and a directory + path is `Some` like any other, so suppression is structural and the + named mutation would require inventing the branch first. The rev 5 + test additionally never armed restore and hard-coded `had_file`, so + it asserted nothing about `run`. Q#JR12 is downgraded to an + observation. + --- ## 0.5. Coherence impact (`COHERENCE.md` §20, required since #163) @@ -160,7 +198,7 @@ daemon bootstrap (`daemon.rs:1641`). **Local startup is not one of them** | Displayed name | `path.display()` raw (`editor.rs:772`) | `path.display()` raw | | `NotFound` arm | empty path-backed buffer, `[new file]` | identical | | Dedup | none | `find_buffer_for_path` | -| Window install | `replace_active_buffer` — drops the startup scratch (`editor.rs:797`) | none; caller installs | +| Window install | `replace_active_buffer` — switches the ACTIVE window (`editor.rs:797`). **It does not drop the startup scratch** (rev 6): its body is one `switch_active_buffer` call, which reassigns `aw.buffer_id` and removes nothing. The doc comment claiming otherwise was wrong before this stage and is corrected in this PR | none; caller installs | | Error type | `io::Error`, bare | `String`, prefixed `cannot open {path}: ` | **The two agree on every observable except the error prefix and the @@ -319,7 +357,9 @@ invariant rather than working around it. ## 3. The unification (Q#JR1) `EditorState::open` becomes a thin caller of `resolve_target_buffer`, -keeping `replace_active_buffer` (which drops the startup scratch, Q#JR3) +keeping `replace_active_buffer` (which switches the **active** window, +Q#JR3 as corrected in rev 6 — it does not destroy the old scratch, and +never did) and keeping its "fire the hook after the core borrow ends" structure (`editor.rs:786-795`) — listeners re-enter `pmacs.editor.*` and re-borrow the core (Q#JR1a). @@ -328,9 +368,15 @@ the core (Q#JR1a). `pmacs /root/secret` names the file, which today's bare message does not. This is the *only* user-visible change from the unification (§2.2). -**Q#JR12** — a directory argument counts as "had a file argument" and -suppresses desktop restore, on Q#DS7's reasoning: a positional argument -means "open this", not "restore my session". +**Q#JR12 (downgraded to an observation, rev 6)** — a directory argument +suppresses desktop restore, on Q#DS7's reasoning that a positional +argument means "open this" rather than "restore my session". This needs +no work and cannot be pinned: `run` computes `had_file = file.is_some()` +(`editor.rs:3152`), and a directory path is `Some` like any other, so +there is no directory-specific branch that could get it wrong. Rev 5 +carried an acceptance for it; that test never armed restore and +hard-coded `had_file`, asserting nothing, and is removed rather than +repaired. --- @@ -538,9 +584,26 @@ problem is solved. ### 4.5 Q#JR9 — what the bootstrap reply names, and what it shows `open_initial_target` on a `Directory` installs nothing: it dispatches the -resolver, then replies `Opened { buffer_id }` naming the buffer the fresh -view's primary document window already holds. §2.8's reassert reasserts -that same buffer — already correct, therefore harmless. +resolver, then replies `Opened { buffer_id }` naming **whatever the +destination window holds once that dispatch returns** — re-read, not the +id captured beforehand (Q#JR9b, rev 6). + +The distinction is not academic. The chain runs **synchronously**. +dired's handler defers, because its listing must await; a user's resolver +is under no such obligation, and one that opens something synchronously +through `commit_to` — the supported way to do it — has already replaced +the window's buffer by the time the reply is built. Reporting the +captured id would pair one buffer's snapshot with another's identity, and +the frontend would render a document nobody asked for. + +Re-reading also subsumes the case where a hook closed the window, so this +arm rehomes through `non_side_target` exactly as the file arm's reassert +does, rather than returning early and skipping that check — which rev 5's +implementation did while this section claimed the revalidation stayed +active. + +Absent a synchronous claimant the re-read yields the buffer the window +already held, which is the ordinary case. **That buffer is not necessarily `*scratch*`.** `build_fresh_frontend_view` clones **LOCAL's primary document buffer** (`daemon.rs:2997`) — M10.9 made @@ -723,8 +786,13 @@ is **removed rather than recast**: it proved nothing N1 does not. window unchanged; `find_file_accepting_a_directory_reports_instead_of_raising` passes unmodified. *Mutation:* route `display_file` into the resolver chain. -- **P7 — desktop restore stays suppressed (Q#JR12).** - *Mutation:* pass `false` for `had_file` on the directory path. +- **P7 — REMOVED in rev 6.** Q#JR12 is structural: `run` computes + `had_file = file.is_some()` and a directory path is `Some` like any + other, so there is no directory-specific branch to break and the named + mutation would have to invent one first. Rev 5's test never armed + restore and hard-coded `had_file`, so it could not fail against any + implementation. Removed rather than repaired — a green test that cannot + fail reads as coverage. - **P8 — startup errors name the file (Q#JR4).** A non-`NotFound`, non-directory failure produces a message containing `cannot open` and the path. *(Legitimately N-shaped for the prefix, P-shaped for the @@ -851,7 +919,12 @@ discovered late. - **Q#JR1** `EditorState::open` adopts `resolve_target_buffer` wholesale. - **Q#JR1a** The hook fires outside the core borrow. - **Q#JR2** *Withdrawn (rev 2)* — its premise was false. -- **Q#JR3** The scratch drop (`replace_active_buffer`) is preserved. +- **Q#JR3 (corrected rev 6)** Startup keeps using + `replace_active_buffer`, which switches the **active** window — not + because it drops the old scratch (it does not, and never did) but + because an `install_buffer_in_window` elsewhere would load the file + while leaving the user looking at scratch. Removing the stale scratch + buffer is separate work. - **Q#JR4** Startup errors gain the `cannot open {path}: ` prefix. - **Q#JR5** `resolve_target_buffer` returns a typed `ResolvedTarget`. - **Q#JR5b** Both `HookKind` types are written path-qualified. @@ -860,12 +933,17 @@ discovered late. builtins do not subscribe; dired is a replaceable fallback slot. - **Q#JR8** `ResolvedTarget::Directory` carries an explicitly normalized path. -- **Q#JR9** The bootstrap reply names the window's pre-existing buffer — - LOCAL's primary document buffer, not necessarily scratch. Accepted and - documented. +- **Q#JR9** The bootstrap reply names the destination window's buffer — + absent a synchronous claimant, LOCAL's primary document buffer, not + necessarily scratch. Accepted and documented. +- **Q#JR9b (rev 6)** That id is **re-read after the dispatch**, and the + arm rehomes through `non_side_target` rather than returning early: a + synchronous resolver may already have replaced the buffer. - **Q#JR10** An unclaimed directory with the handler cleared exits 0 with a status message. -- **Q#JR12** A directory argument suppresses desktop restore. +- **Q#JR12 (observation, rev 6)** A directory argument suppresses + desktop restore structurally, via `had_file = file.is_some()`. No work, + no pin. - **Q#JR13** `display_file` keeps its directory-is-an-error contract. - **Q#JR14** The destination `{frontend, window, buffer}` is captured at resolve time; `commit_to` preflights and scopes the **entire** diff --git a/src/daemon.rs b/src/daemon.rs index 4a7e08f..1451a0c 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1661,11 +1661,51 @@ fn open_initial_target( let dest = editor .capture_directory_destination(frontend_id, origin_window) .ok_or_else(|| format!("cannot open {}: no document window", path.display()))?; - let buffer_id = dest.buffer; editor.dispatch_directory_open(&path, dest); editor.reconcile_panel_layout(frontend_id); + + // The reply must name what the window ACTUALLY holds now, not + // what it held before the dispatch. + // + // The chain runs synchronously. dired's handler defers (it + // spawns a coroutine for the listing), but a user's resolver + // is under no such obligation: a handler that opens something + // synchronously -- through `commit_to`, which is exactly the + // supported way to do it -- has already replaced this + // window's buffer by the time we get here. Reporting the + // captured id would then send the snapshot of one buffer and + // the identity of another, and the frontend would render a + // document nobody asked for. + // + // Re-reading also covers the case a hook closed the window, + // which is why this rehomes through `non_side_target` exactly + // as the file arm's reassert does rather than returning early + // and skipping that check. + let mut core = editor.core.borrow_mut(); + core.active_frontend = frontend_id; + let destination = if core + .views + .get(&frontend_id) + .is_some_and(|view| view.layout.iter_ids().contains(&origin_window)) + { + origin_window + } else { + core.non_side_target(frontend_id) + .map_err(|error| format!("cannot reselect {}: {error}", path.display()))? + }; + core.focus_window(frontend_id, destination); + let buffer_id = core + .windows + .get(&destination) + .map(|window| window.buffer_id) + .ok_or_else(|| format!("cannot reselect {}: window died", path.display()))?; return Ok(OpenedInitialTarget { buffer_id, + // False whether or not the chain replaced the buffer: an + // untouched destination is pre-existing and already + // published, and a buffer a synchronous handler installed + // went through the ordinary display path, which publishes + // on its own terms. publish_to_replicas: false, }); } @@ -4924,6 +4964,177 @@ mod tests { ); } + /// **N2** (Journey Stage 1a) — a DIRECTORY initial target reaches + /// readiness instead of failing. + /// + /// This deliberately supersedes the directory half of the GPU + /// initial-target framing's Q#GT6 and its acceptance 10, which + /// required `IsADirectory` to fail before window creation. + /// Permission-denied and every other pre-readiness failure keep that + /// contract. + #[test] + fn initial_target_directory_reaches_ready() { + use crate::editor::EditorState; + use crate::protocol::FrontendId; + + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join("alpha.txt"), b"alpha\n").expect("write"); + + let mut editor = EditorState::new(); + editor + .lua_host + .lua() + .load("pmacs.lsp.config = {}") + .exec() + .expect("wipe lsp config"); + let fid = FrontendId(131); + let view = build_fresh_frontend_view(&mut editor, false, false); + editor.core.borrow_mut().register_frontend_view(fid, view); + + let opened = open_initial_target( + &mut editor, + fid, + InitialTarget { + path: dir.path().as_os_str().as_bytes().to_vec(), + cwd: dir.path().as_os_str().as_bytes().to_vec(), + }, + ) + .expect("a directory target must reach readiness, not fail"); + + // The reply names a live buffer in a live document window: a + // valid, ready session. The listing arrives later, asynchronously. + let core = editor.core.borrow(); + assert!( + core.registry.borrow().contains(opened.buffer_id), + "the reported buffer must exist so its snapshot can be sent" + ); + let active = core.views[&fid].active; + assert_eq!( + core.windows[&active].buffer_id, opened.buffer_id, + "the reported buffer is the one the document window shows" + ); + } + + /// **N5** — the bootstrap buffer is not necessarily `*scratch*`. + /// + /// `build_fresh_frontend_view` clones LOCAL's PRIMARY DOCUMENT + /// buffer, so when LOCAL holds a real document the fresh session + /// briefly displays and snapshots it. Q#JR9 accepts that rather than + /// introducing a placeholder; this observes it instead of assuming. + #[test] + fn initial_target_directory_reports_a_non_scratch_primary() { + use crate::editor::EditorState; + use crate::protocol::FrontendId; + + let dir = tempfile::tempdir().expect("tempdir"); + let doc = dir.path().join("already-open.txt"); + std::fs::write(&doc, b"local document\n").expect("write"); + + // LOCAL holds a real document, not scratch. + let mut editor = EditorState::open(doc.clone()).expect("open"); + editor + .lua_host + .lua() + .load("pmacs.lsp.config = {}") + .exec() + .expect("wipe lsp config"); + let local_primary = editor + .core + .borrow() + .primary_document_buffer(FrontendId::LOCAL) + .expect("LOCAL always has a document window"); + + let fid = FrontendId(132); + let view = build_fresh_frontend_view(&mut editor, false, false); + editor.core.borrow_mut().register_frontend_view(fid, view); + + let opened = open_initial_target( + &mut editor, + fid, + InitialTarget { + path: dir.path().as_os_str().as_bytes().to_vec(), + cwd: dir.path().as_os_str().as_bytes().to_vec(), + }, + ) + .expect("a directory target must reach readiness"); + + assert_eq!( + opened.buffer_id, local_primary, + "the bootstrap reply names LOCAL's primary document buffer, \ + which is a real document here rather than *scratch*" + ); + } + + /// **N2b (rev 6)** — a resolver that claims SYNCHRONOUSLY is reported + /// correctly. + /// + /// The bug this pins: the arm captured the destination buffer id + /// *before* dispatching the chain and reported that. The chain runs + /// synchronously, so a handler that opens something immediately — + /// through `commit_to`, the supported way — had already replaced the + /// window's buffer, and the reply paired one buffer's snapshot with + /// another's identity. + /// + /// Falsified by reporting the captured id instead of re-reading. + #[test] + fn initial_target_directory_reports_what_a_synchronous_handler_installed() { + use crate::editor::EditorState; + use crate::protocol::FrontendId; + + let dir = tempfile::tempdir().expect("tempdir"); + + let mut editor = EditorState::new(); + editor + .lua_host + .lua() + .load( + "pmacs.lsp.config = {} + claimed = pmacs.buffer.create('*claimed*') + pmacs.path.set_directory_handler(function(path, dest) + pmacs.window.commit_to(dest, function() + pmacs.window.display(claimed, { select = true }) + end) + end)", + ) + .exec() + .expect("install a synchronous handler"); + + let fid = FrontendId(133); + let view = build_fresh_frontend_view(&mut editor, false, false); + editor.core.borrow_mut().register_frontend_view(fid, view); + + let opened = open_initial_target( + &mut editor, + fid, + InitialTarget { + path: dir.path().as_os_str().as_bytes().to_vec(), + cwd: dir.path().as_os_str().as_bytes().to_vec(), + }, + ) + .expect("a claimed directory target must reach readiness"); + + // Compare by NAME: the reported id must be the handler's buffer, + // and naming it is what makes the failure legible when it is not. + let core = editor.core.borrow(); + let reported_name = core + .registry + .borrow() + .get(opened.buffer_id) + .expect("the reported buffer exists") + .name() + .to_string(); + assert_eq!( + reported_name, "*claimed*", + "the reply must name what the handler installed, not the \ + buffer captured before the dispatch" + ); + let active = core.views[&fid].active; + assert_eq!( + core.windows[&active].buffer_id, opened.buffer_id, + "…and that buffer is what the window shows" + ); + } + /// Bottom-panel §1.3 #1/#3/#21 — the three Projection producers whose /// only production caller is `dispatcher_loop`, pinned at the named /// seams that loop calls. Round 2 finding: reverting any of them to diff --git a/src/editor.rs b/src/editor.rs index 1e94d92..9c204bc 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -891,10 +891,19 @@ impl EditorState { /// /// * **The window install.** `resolve_target_buffer` deliberately /// does not touch windows, so the caller places the buffer. - /// Startup uses [`Self::replace_active_buffer`] specifically - /// because it drops the just-created scratch buffer; an - /// `install_buffer_in_window` here would leave a stray `*scratch*` - /// behind every `pmacs FILE` (Q#JR3). + /// Startup uses [`Self::replace_active_buffer`], which switches + /// the ACTIVE window — an `install_buffer_in_window` into some + /// other window would load the file and leave the user looking at + /// scratch (Q#JR3). + /// + /// It does **not** destroy the scratch buffer, despite what + /// `replace_active_buffer`'s own doc comment has long claimed: + /// that function only calls `switch_active_buffer`, which + /// reassigns the window's `buffer_id` and removes nothing. The + /// startup scratch survives in the registry, and did before this + /// stage too. Changing that is buffer-lifetime work with its own + /// consequences (what else may hold the id, what `C-x b` should + /// list) and is deliberately not smuggled in here. /// * **Firing the hook outside the core borrow.** Listeners /// re-enter `pmacs.editor.*`, which re-borrows the core /// (Q#JR1a) — the same reason the daemon bootstrap and @@ -1068,9 +1077,18 @@ impl EditorState { } } - /// Switch the active window to `buffer_id`, dropping any old - /// scratch buffer if the active window's previous buffer has no - /// other windows referencing it. Returns silently on a stale id. + /// Switch the active window to `buffer_id`. Returns silently on a + /// stale id. + /// + /// **Corrected (Journey Stage 1a).** This comment previously claimed + /// it dropped "any old scratch buffer if the active window's + /// previous buffer has no other windows referencing it". It never + /// did: the body is one `switch_active_buffer` call, which reassigns + /// `aw.buffer_id` and removes nothing from the registry. The claim + /// was load-bearing enough that a framing decision (Q#JR3) and an + /// acceptance pin were written against it before anyone checked the + /// body. Removing the stale scratch may well be worth doing; it is + /// separate work, and this comment no longer promises it. fn replace_active_buffer(&self, buffer_id: crate::buffer::BufferId) { let mut core = self.core.borrow_mut(); let _ = core.switch_active_buffer(buffer_id); diff --git a/tests/journey_acceptance.rs b/tests/journey_acceptance.rs index 591536d..f1e4c03 100644 --- a/tests/journey_acceptance.rs +++ b/tests/journey_acceptance.rs @@ -31,8 +31,10 @@ use std::path::Path; use std::time::{Duration, Instant}; +use crossterm::event::{KeyCode, KeyEvent, KeyEventKind, KeyEventState, KeyModifiers}; use pmacs::editor::EditorState; use pmacs::editor_core::normalize_buffer_path; +use pmacs::protocol::FrontendId; use tempfile::TempDir; // --------------------------------------------------------------------------- @@ -65,6 +67,35 @@ fn pump(s: &mut EditorState) { } } +fn key(code: KeyCode, mods: KeyModifiers) -> KeyEvent { + KeyEvent { + code, + modifiers: mods, + kind: KeyEventKind::Press, + state: KeyEventState::NONE, + } +} + +fn press(s: &mut EditorState, code: KeyCode) { + s.dispatch_key(FrontendId::LOCAL, key(code, KeyModifiers::NONE)); +} + +fn type_char(s: &mut EditorState, c: char) { + s.dispatch_key(FrontendId::LOCAL, key(KeyCode::Char(c), KeyModifiers::NONE)); +} + +/// The 0-based line an entry renders on, found by its trailing name +/// column -- the same shape `dired_acceptance` uses. +fn line_of(s: &EditorState, name: &str) -> usize { + let text = active_text(s); + for (index, line) in text.lines().enumerate() { + if line.trim_end().ends_with(name) { + return index; + } + } + panic!("no listing line for {name:?} in:\n{text}"); +} + /// A project a journey can plausibly be run against. fn project() -> TempDir { let td = tempfile::tempdir().expect("tempdir"); @@ -333,11 +364,18 @@ fn journey_a_raising_resolver_suppresses_the_fallback_and_reports() { // Step 5 — edit immediately // --------------------------------------------------------------------------- -/// **N11** — the journey's step-3-into-step-5 path: start on a -/// directory, visit a listed file, and type into *that* file. +/// **N11** — the journey's step-3-into-step-5 path, through the real +/// input path at every step: start on a directory, press `RET` on a +/// listed file, then type a character into it. +/// +/// Rev 6 correction: this previously called `display_file` and +/// `buf:insert` directly, so it stayed green with dired's `RET` binding, +/// its entry dispatch, or the editor's self-insert path all broken — +/// which is most of what "the journey works" is supposed to mean. Both +/// gestures are now dispatched as keys. /// /// Deliberately not a self-insert into the dired buffer, whose intercept -/// rejects every edit — asserting an edit lands there would contradict +/// rejects every edit: asserting an edit lands there would contradict /// the read-only contract rather than pin the journey. #[test] fn journey_step5_editing_a_file_reached_through_the_directory() { @@ -345,21 +383,24 @@ fn journey_step5_editing_a_file_reached_through_the_directory() { let mut s = launch(td.path()); assert!(active_name(&s).starts_with("*dired:")); - let target = td.path().join("alpha.txt"); - exec( - &s, - &format!( - "pmacs.window.display_file({:?}, {{ select = true }})", - target.display().to_string() - ), - ); + // Seat on the entry, then VISIT it with the real key. + let line = line_of(&s, "alpha.txt"); + exec(&s, &format!("pmacs.editor.move_to_line({line})")); + press(&mut s, KeyCode::Enter); pump(&mut s); - exec(&s, "pmacs.window.buffer():insert(0, 'EDITED ')"); + assert_eq!( + active_name(&s), + td.path().join("alpha.txt").display().to_string(), + "RET on a listed file must visit it" + ); + + // And type into it with the real key. + type_char(&mut s, 'X'); let text = active_text(&s); assert!( - text.starts_with("EDITED "), - "the edit must land in the visited file's buffer; got {text:?}" + text.starts_with('X'), + "a self-insert must land in the visited file's buffer; got {text:?}" ); assert!( buffer_count(&s) >= 2, @@ -466,26 +507,21 @@ fn preservation_an_unreadable_file_reports_with_its_path() { ); } -/// **P7** — a directory argument suppresses desktop restore, on the same -/// reasoning a file argument does (Q#DS7): a positional argument means -/// "open this", not "restore my session". -/// -/// *Mutation:* pass `false` for `had_file` on the directory path. -#[test] -fn preservation_a_directory_argument_suppresses_desktop_restore() { - let td = project(); - let mut s = launch(td.path()); - // Arm the restore AFTER startup, then confirm the startup path - // treated its argument as a positional open: `had_file` is what - // `run` passes, and a directory must set it. - let had_file = true; - s.restore_desktop_if_armed(had_file); - assert!( - !status(&s).contains("desktop-restore"), - "a positional directory argument must not trigger a restore; got {:?}", - status(&s) - ); -} +// **P7 — removed in rev 6, not weakened.** +// +// Q#JR12 said a directory argument must suppress desktop restore, and +// rev 5 carried a pin for it. There is nothing to pin. `run` computes +// `had_file = file.is_some()` (`editor.rs:3152`) and a directory path is +// `Some` like any other, so the suppression is structural: no +// directory-specific branch exists that could get it wrong, and the +// named mutation ("pass false for `had_file` on the directory path") +// would require inventing the branch first. +// +// The rev 5 test also never armed desktop restore and hard-coded +// `had_file = true` after startup, so it asserted nothing about `run`'s +// decision and would have passed against any implementation. Keeping a +// green test that cannot fail is worse than having none: it reads as +// coverage. Q#JR12 is downgraded to an observation in the framing. /// **P6** — `display_file` keeps its directory-is-an-error contract and /// does not enter the resolver chain.