From 0b1716b5cdd35beac300eb6d81607b7c524963d4 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 21:38:47 -0400 Subject: [PATCH] fix(journey): canonicalize the compile-directory expectation; unflip the grades MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1 on #203, two blocking findings, both accepted. The compile-directory assertions used the suite's lexical `canon()`, but `pmacs.project.detect` canonicalizes before walking (`canonicalize_or_passthrough`, `src/project.rs:509-511`), so the compile cwd is filesystem-canonical. On macOS `/var` is a symlink to `/private/var` and the two spellings disagree — both macOS legs failed while Ubuntu, where `/tmp` is not a symlink, stayed green. Fixed with a `detected_root()` expectation, and pinned by a fixture that launches through an explicit **symlink** so lexical and canonical paths disagree on every platform. That matters more than the fix: the original bite ran only on Linux, where nothing could make the two differ, so no amount of local mutation testing would have caught this. The new pin is the only one that goes red when the lexical expectation is restored. Second: `COHERENCE.md` §2's step-9 row was flipped to **Works** and the handoff said Stage 1b-1 was **LANDED**, while this PR is open. §25 is explicit that grades change only with landed evidence, never aspirationally. Both now describe the real state — the row stays Partial and names #203 as the open PR that closes it, §20 and the arc list say "in flight", and the handoff says IMPLEMENTED with the PR number. The flip is not dropped, it is owned: the active-work lane records the four places to change on merge, because an unowned doc flip is exactly how this ledger's drift starts. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- COHERENCE.md | 33 ++++++++-------- docs/active-work.md | 18 +++++++++ docs/agent-handoff.md | 7 +++- tests/journey_acceptance.rs | 77 ++++++++++++++++++++++++++++++++++++- 4 files changed, 115 insertions(+), 20 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index 5866238..2154a87 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -378,7 +378,7 @@ Full verdict table: | 6 | Language intelligence | **Partial** | Rust grammar bundled and auto-attaches; rust-analyzer preconfigured (`builtin/runtime/lsp.lua:44-52`) — but a missing binary fails silently (§1.2) and highlighting masks it. No LSP status command exists to diagnose | | 7 | Find symbol / file | **File: fixed (open by path merged #162; browsing #165). Symbol: works but undiscoverable** | No find-file/dired/picker existed at audit. Now `C-x C-f` opens a known path and `C-x d` / `C-x C-j` browse (flat listing, `dired` mode keymap); `M-.`/`M-?`/`C-c o` still bound but advertised nowhere and server-gated; no workspace-symbol command; `pmacs.index.*` has no UI | | 8 | Open terminal | **Works** | Full PTY with scrollback + modeline segment, bound to `C-c t` and configurable through three registered settings (`terminal.default-profile`, `terminal.scrollback-rows`, `terminal.escape-key`) plus named `pmacs.terminal.profiles` (PR #173), and searchable through `M-x terminal.copy-mode` / `C-c C-t`, which materializes the retained scrollback into an ordinary read-only buffer (Stage 2). Named limitations: `C-c t` is unreachable from *inside* a terminal window, where `C-c` is consumed as the escape — `M-x terminal` still works there; and there is still **no close/kill command**, which is the remaining half of this step's discoverability gap. *Was broken outright on the GPU frontend until the double terminal-layout sync was fixed: the child took a `SIGWINCH` storm at tick cadence, so typing into it was impossible while output still flowed.* | -| 9 | Build / test | **Works** | Journey Stage 1b-1: `C-c c` runs `compile.run`, and the first prompt is prefilled from the detected project kind (`pmacs.compile.defaults`, seeded `rust = "cargo build"`). The prompt **captures** its directory rather than re-resolving at accept time, so the command it offers and the directory it runs in cannot drift while the minibuffer waits. Still defaults cwd to the detected project root and parses Rust `-->` errors. Named limitation: after `pmacs ` the active buffer is dired's and pathless, so the cwd falls back to the process cwd — §8's execution-location model owns that | +| 9 | Build / test | **Partial** | `M-x compile.run` works, defaults cwd to the detected project root, and parses Rust `-->` errors — but no keybinding, an **empty first prompt** (`initial = last and last.cmdline or ""`, `builtin/runtime/compile.lua`), and no `cargo build` suggestion despite `ProjectKind::Rust` existing (`src/project.rs:78` — **not** `Cargo`, see §24). **PR #203 (open) closes all three**: `C-c c` runs `compile.run`, and the first prompt is prefilled from the detected project kind (`pmacs.compile.defaults`, seeded `rust = "cargo build"`). The prompt **captures** its directory rather than re-resolving at accept time, so the command it offers and the directory it runs in cannot drift while the minibuffer waits. Named limitation it does not fix: after `pmacs ` the active buffer is dired's and pathless, so the cwd falls back to the process cwd — §8's execution-location model owns that. **This row flips to Works when #203 merges**; per §25 a grade changes only on landed evidence | | 10 | Inspect error | **Partial (good once reached)** | `E:n W:n` modeline counts, underlines, `M-g n/p` + ``C-x ` `` walking a unified compile/grep/diag source, message echo, `RET` visits. Gated entirely on step 6 or 9 succeeding first | | 11 | See background work | **Works but undiscoverable** | `*workers*` view via `M-x editor.list-workers`; `C-c C-k` cancel-at-point. No keybinding, no statusline spinner/progress indicator anywhere (§9) | | 12 | Close + restore | **Partial** | Per-file cursor+scroll (saveplace), recent files, minibuffer history, autosave recovery all restore zero-config. Open-buffer set and window layout do **not**: desktop-save is opt-in (`pmacs.session.desktop_mode(true)`) *and* a documented no-op under a daemon (`src/desktop.rs:323-326`, `:353-356`, Q#DS9) | @@ -388,10 +388,10 @@ A journey observation worth keeping verbatim from the audit: C-M-s` opens all folds, while opening a file, opening a terminal, and running a build have no bindings at all. -All three of that observation's examples have now been answered — -opening a file by `C-x C-f` (#162), opening a terminal by `C-c t` -(#173), and running a build by `C-c c` (Journey Stage 1b-1). **The quote -stays as written**: it names a standing bias in how new work gets bound, +Two of that observation's three examples have been answered — opening a +file by `C-x C-f` (#162) and opening a terminal by `C-c t` (#173). +**Running a build still has no binding on `main`**; PR #203 is open and +adds `C-c c`. **The quote stays as written either way**: it names a standing bias in how new work gets bound, not three isolated omissions, and three fixes do not retire a bias. What has changed is that the bias no longer has an uncontested example in the golden journey — the next new surface that ships without a binding is @@ -1544,17 +1544,18 @@ blockers. **State: runs to step 5; thin from step 6 (§2). Mostly wiring, and unusually cheap:** directory-argument handling (**done**: Journey Stage 1a); a find-file surface (**done**: #162 open-by-path, #165 browsing); surfacing the LSP spawn failure with guidance (§1.2); a -compile keybinding + `cargo build`/`test` default (**done**: Journey -Stage 1b-1, from the existing `ProjectKind::Rust` — **not** `Cargo`, see -§24); a terminal keybinding (**done**: `C-c t`, #173); a welcome -buffer. The journey acceptance suite (§19) is the ratchet that +compile keybinding + `cargo build`/`test` default (**in flight**: +Journey Stage 1b-1, PR #203, from the existing `ProjectKind::Rust` — +**not** `Cargo`, see §24); a terminal keybinding (**done**: `C-c t`, +#173); a welcome buffer. The journey acceptance suite (§19) is the ratchet that keeps it fixed — it **exists now** (`tests/journey_acceptance.rs`, Stage 1a), seeded with steps 2, 3, and 5. Journey Stage 1b is the named remainder, and it splits: **1b-1 — the -compile binding + project-kind defaults — is done**; 1b-2 (LSP spawn -guidance, step 6) and 1b-3 (the welcome buffer, step 4) remain. The -ratchet is now seeded with steps 2, 3, 5 and 9. +compile binding + project-kind defaults — is in flight as PR #203**; +1b-2 (LSP spawn guidance, step 6) and 1b-3 (the welcome buffer, step 4) +remain. The ratchet is seeded with steps 2, 3 and 5, and gains step 9 +when #203 lands. ### Priority 2: Make workspace and location explicit @@ -1625,10 +1626,10 @@ implementation — this list is direction, not commitment): `resolve_target_buffer` unification, the destination-scope substrate, and the first journey acceptance suite. It routes `pmacs .` into #165's dired buffer rather than growing a second directory surface. - **Stage 1b-1 — landed**: the compile binding and project-kind - defaults, with the prompt capturing its directory rather than - re-resolving it at accept time. **Stage 1b-2 / 1b-3 — remaining**: - LSP-failure surfacing, welcome buffer. + **Stage 1b-1 — in flight (PR #203)**: the compile binding and + project-kind defaults, with the prompt capturing its directory rather + than re-resolving it at accept time. **Stage 1b-2 / 1b-3 — + remaining**: LSP-failure surfacing, welcome buffer. 2. **Discovery surface** (P4): the describe/list/where-is command family, M-x rich rows, help unification, help prefix. 3. **Transient keymap layer** (§6): the overlay scope + lifetime diff --git a/docs/active-work.md b/docs/active-work.md index 960294e..a4c577b 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -260,6 +260,24 @@ If it does not, stop and repair the remote/fetch configuration. `docs/journey-stage1b1-compile-defaults-framing.md` revision 2, approved after one review round (two blocking, two major, all accepted). **Implemented; PR open.** +- **ON MERGE OF #203, flip four places to landed.** The PR deliberately + ships them as *in flight*, because `COHERENCE.md` §25 says a grade + changes only on landed evidence and the PR is open: `COHERENCE.md` + §2's step-9 verdict row (Partial → **Works**), §2's post-table + keybinding-inversion paragraph (third example answered), §20 Priority + 1 and its arc list (in flight → done), and + `docs/agent-handoff.md` §1's arc bullet (IMPLEMENTED → LANDED). + **Recorded here because an unowned doc flip is exactly how this + ledger's drift starts** — the same rule-4 precondition that kept #176's + lane alive past its merge. +- **A lexical path expectation is wrong for anything detection touched.** + `pmacs.project.detect` canonicalizes before walking + (`canonicalize_or_passthrough`, `src/project.rs:509-511`) while the + suite's `canon()` is lexical, so the compile-directory assertions + passed on Ubuntu and **failed both macOS legs**, where `/var` is a + symlink to `/private/var`. Fixed with a `detected_root()` expectation + and pinned by a **symlinked fixture**, which reproduces the disagreement + on any platform — a Linux-only bite could not have caught it. - **Bites found two vacuous pins of my own.** The nested-project pin passed with the keybinding removed, because `minibuffer.contents()` is `""` both for an empty prefill and for no minibuffer at all — `""` diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 34d353d..f5a7d70 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -104,8 +104,11 @@ commands, read `docs/active-work.md` immediately after this file. interaction islands added, config-registry adoption, background-work attribution. Its §2 grades the golden journey; **Journey Stage 1a moved that grade off "broken at step 3"** — see the arc bullet below. -- **Journey arc (P1) — Stage 1b-1 LANDED** (`docs/journey-stage1b1-compile-defaults-framing.md`). - Journey step 9 moves **Partial → Works**: `C-c c` runs `compile.run`, +- **Journey arc (P1) — Stage 1b-1 IMPLEMENTED, PR #203 OPEN** + (`docs/journey-stage1b1-compile-defaults-framing.md`). Not landed: per + `COHERENCE.md` §25 a grade changes only on landed evidence, so §2's + step-9 row still reads **Partial** and flips at merge. + Journey step 9 will move **Partial → Works**: `C-c c` runs `compile.run`, and the first prompt is prefilled from the detected project kind via `pmacs.compile.defaults` (seeded `rust = "cargo build"`, extensible from `init.lua`). Lua, tests and docs; no Rust change, no protocol diff --git a/tests/journey_acceptance.rs b/tests/journey_acceptance.rs index c7b0569..74cd265 100644 --- a/tests/journey_acceptance.rs +++ b/tests/journey_acceptance.rs @@ -1354,6 +1354,23 @@ fn bound_detection_to(s: &EditorState, dir: &Path) { ); } +/// The project root as **detection** reports it. +/// +/// `pmacs.project.detect` canonicalizes before walking +/// (`canonicalize_or_passthrough`, `src/project.rs:509-511`), so a +/// `/var/folders/...` tempdir on macOS comes back as +/// `/private/var/folders/...`. `canon()` is *lexical* — it never +/// resolves symlinks — so it is the wrong expectation for any value +/// that has passed through detection, which is exactly what the compile +/// cwd is. Using it here failed both macOS CI legs while Ubuntu (where +/// `/tmp` is not a symlink) stayed green. +fn detected_root(path: &Path) -> String { + std::fs::canonicalize(path) + .unwrap_or_else(|_| path.to_path_buf()) + .to_string_lossy() + .into_owned() +} + fn minibuffer_active(s: &EditorState) -> bool { eval(s, "return pmacs.minibuffer.is_active()") } @@ -1483,7 +1500,9 @@ fn journey_step9_the_prompt_runs_in_the_directory_it_captured() { assert!(finished, "the accepted run must finish"); let text = named_text(&s, "*compilation*"); - let a_path = canon(a.path()); + // Detection-canonical, not lexical: the compile cwd came from + // `pmacs.project.detect`, and `pwd` reports the physical directory. + let a_path = detected_root(a.path()); assert!( text.contains(&format!("Directory: {a_path}")), "the header must name the directory the prompt captured, not the newly active one;\n{text}" @@ -1528,7 +1547,7 @@ fn journey_step9_the_offered_command_builds_the_project() { let text = named_text(&s, "*compilation*"); assert!( - text.contains(&format!("Directory: {}", canon(td.path()))), + text.contains(&format!("Directory: {}", detected_root(td.path()))), "the build runs in the detected project root;\n{text}" ); assert!( @@ -1669,3 +1688,57 @@ fn binary_available(name: &str) -> bool { .output() .is_ok_and(|o| o.status.success()) } + +/// **N** — the compile directory is *detection*-canonical, not lexical. +/// +/// This pin exists because its absence broke both macOS CI legs while +/// Ubuntu stayed green: `/var` is a symlink to `/private/var` there, so +/// a tempdir's lexical and canonical paths differ, and the original +/// assertions used the lexical one. +/// +/// Reproducing it on Linux needs an explicit symlink — which is also a +/// real configuration `Workspace::detect`'s own doc comment names +/// ("`/tmp/sandbox/foo` symlinked to `/home/user/code/foo`"). Launching +/// through the link makes the two paths disagree on every platform, so +/// the regression can no longer hide behind a filesystem that happens +/// not to use symlinks. +#[cfg(unix)] +#[test] +fn journey_step9_the_compile_directory_is_detection_canonical() { + let parent = tempfile::tempdir().expect("tempdir"); + let real = parent.path().join("real"); + std::fs::create_dir_all(real.join("src")).expect("mkdir real"); + std::fs::write( + real.join("Cargo.toml"), + b"[package]\nname = \"journey-fixture\"\nversion = \"0.1.0\"\nedition = \"2021\"\n", + ) + .expect("write Cargo.toml"); + std::fs::write(real.join("src/main.rs"), b"fn main() {}\n").expect("write main.rs"); + + let link = parent.path().join("link"); + std::os::unix::fs::symlink(&real, &link).expect("symlink"); + + // Precondition, or the pin is vacuous: the two spellings must + // actually differ, which is the whole condition macOS creates for + // free and Linux does not. + assert_ne!( + canon(&link), + detected_root(&link), + "the fixture must make lexical and canonical paths disagree" + ); + + let mut s = walk_to_open_file(&link, "Cargo.toml"); + press_compile_chord(&mut s); + exec(&s, "pmacs.minibuffer.set_contents('pwd')"); + press(&mut s, KeyCode::Enter); + let finished = pump_processes_until(&mut s, 10_000, |s| { + named_text(s, "*compilation*").contains("exited") + }); + assert!(finished, "the accepted run must finish"); + + let text = named_text(&s, "*compilation*"); + assert!( + text.contains(&format!("Directory: {}", detected_root(&link))), + "the header must name the directory detection resolved to;\n{text}" + ); +}