diff --git a/COHERENCE.md b/COHERENCE.md index acd744d..6aaad38 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -120,7 +120,7 @@ asymmetry**, and **per-arc coherence debt**. Coherence-shaped work already in flight at audit time: find-file / dired Stage 0 (`C-x C-f`, merged #162, `docs/dired-framing.md`) and its -Stage 1 directory view (PR #165), bottom panel Stage 1 (merged #155), +Stage 1 directory view (merged #165), bottom panel Stage 1 (merged #155), multi-root LSP affinity (merged #161), the config registry foundation (merged #127). @@ -195,7 +195,7 @@ working, unreachable capability: time; a complete 1,384-line dired existed only as a frozen test fixture (`tests/fixtures/pmacs-dired/init.lua`). **Fixed:** dired Stage 0 opens a path (`C-x C-f`, merged #162) and Stage 1 ships the - browsing view as a builtin (`C-x d` / `C-x C-j`, PR #165). The fixture + browsing view as a builtin (`C-x d` / `C-x C-j`, merged #165). The fixture stays frozen — its `install_local` + `require` routing *is* the M8 package-universality proof (Q#DR1) — and shrinking it is scheduled after Stage 3. @@ -363,11 +363,11 @@ Full verdict table: |---|---|---|---| | 1 | Install | **Partial** | Source build only: `cargo build --release --workspace --features pmacs/crdt` (`README.md`). No binaries, no packaging. Runtime deps (`/bin/sh`, git, tar, coreutils) documented, never checked at runtime | | 2 | Launch unconfigured | **Works** | `EditorState::new()` → empty `*scratch*`; missing config is not an error (`src/config.rs:7-9`); recentf/saveplace/autosave default-on | -| 3 | Open real project | **Missing at the CLI** | `pmacs .` still exits 1 (above): `load_file` does `File::open` (which succeeds on a directory) then `read_to_end` → EISDIR, which is not `NotFound`, so `resolve_target_buffer`'s create-a-`[new file]` arm never fires. Dired Stage 1 (PR #165) supplies the buffer a directory should resolve *to*; routing `pmacs .` into it is Journey Stage 1's work, which must not invent a second directory surface | +| 3 | Open real project | **Missing at the CLI** | `pmacs .` still exits 1 (above): `load_file` does `File::open` (which succeeds on a directory) then `read_to_end` → EISDIR, which is not `NotFound`, so `resolve_target_buffer`'s create-a-`[new file]` arm never fires. Dired Stage 1 (merged #165) supplies the buffer a directory should resolve *to*; routing `pmacs .` into it is Journey Stage 1's work, which must not invent a second directory surface | | 4 | Understand interface | **Partial** | Mode line gives name/modified/L:C/scroll + mode/LSP/terminal segments; but no welcome text (`EditorCore::new` sets `status: String::new()`), no cheat sheet, and `C-h` deletes a word (§18) | | 5 | Edit | **Works** | Full CUA + Emacs keymap in 161 lines (`builtin/keymaps/default.lua`); isearch, query-replace, kill ring, undo/redo, auto-indent/pair/comment, atomic save. Genuinely excellent zero-config | | 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 PR #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 | +| 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 | **Partial** | `M-x compile.run` works, defaults cwd to detected project root, parses Rust `-->` errors — but no keybinding, an **empty first prompt** (`initial = last and last.cmdline or ""`, `builtin/runtime/compile.lua:1134-1138`), and no `cargo build`/`cargo test` suggestion despite `ProjectKind::Cargo` existing (`src/project.rs:77`) | | 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 | @@ -467,7 +467,7 @@ level is the one missing. Audited level-by-level: **Beginner** (should see: files, buffers, search, diagnostics, terminal, build actions, menus, missing-tool guidance): -- files ✓ since #162 / PR #165 (`C-x C-f` opens a path, `C-x d` browses; +- files ✓ since #162 / #165 (`C-x C-f` opens a path, `C-x d` browses; neither is advertised anywhere but the keymap) · buffers ✓ (`C-x b`, `*buffer-list*`) · search ✓ (`C-s`/`C-r`/`C-M-s`; project.search is M-x-only) · diagnostics ✓ once a server runs · terminal ✓ but @@ -1254,7 +1254,7 @@ Primitive-by-primitive against the list above: hierarchy, package dependency graph, worker trees, git status) will each need it; building it once *before* dired's directory view and the workers tree harden their own conventions is exactly this - section's point. Dired Stage 1 (PR #165) landed **without** inventing + section's point. Dired Stage 1 (merged #165) landed **without** inventing one: its listing is flat (Emacs parity), and the recursive in-buffer case — `i` insert-subdirectory — is a named deferral in `docs/dired-framing.md` §13, which is where a shared tree primitive @@ -1418,7 +1418,8 @@ greets a new user says nothing (`EditorCore::new` sets an empty status). Note the dependency: five of the ten onboarding steps above currently -lead somewhere broken or invisible (find a file — in flight; inspect a +lead somewhere broken or invisible (find a file — the mechanism is fixed +since #162/#165 but is advertised nowhere except the keymap; inspect a diagnostic — silent-failure risk; view workers — undiscoverable; setting provenance — unanswerable). Onboarding is correctly sequenced *after* the P1/P4 fixes, but the cheap floor — a welcome buffer in @@ -1482,7 +1483,7 @@ Establish the end-to-end workflow; treat regressions as release blockers. **State: broken at step 3 (§2). Mostly wiring, and unusually cheap:** directory-argument handling (the remaining half of step 3 — dired Stage 1 landed the buffer it should resolve to); a find-file -surface (**done**: #162 open-by-path, PR #165 browsing); surfacing the +surface (**done**: #162 open-by-path, #165 browsing); surfacing the LSP spawn failure with guidance (§1.2); a compile keybinding + `cargo build`/`test` default from the existing `ProjectKind::Cargo`; a terminal keybinding; a welcome buffer. The @@ -1552,7 +1553,9 @@ implementation — this list is direction, not commitment): 1. **Journey Stage 1** (P1): directory open + compile defaults + LSP-failure surfacing + bindings + welcome buffer + the first - journey acceptance suite. Rides alongside the in-flight dired arc. + journey acceptance suite. Dired Stage 1 has landed (#165), so the + buffer a directory resolves *to* already exists; this arc routes + `pmacs .` into it rather than growing a second directory surface. 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 ba77bd1..503689f 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -5,6 +5,18 @@ landed on `main`. Read it after `docs/agent-handoff.md`. Remove completed entries when their PR merges; do not let this become a second permanent backlog. +**One lane below is retained past its merge, and says so at its own +head**: the PTY terminate diagnostic (#176), because no landed-doc PR +owns moving its facts to `docs/agent-handoff.md` yet, and rule 4 removes +a lane only *after* that move. Every other merged lane has been removed — +the Lean 4 and GPU-terminal-input headers this paragraph used to +disclaim are gone, as are the inline-math (#172), dired (#169), and +terminal config + copy mode (#180) lanes. + +**Trust the canonical-base line below over any lane header**: if a PR +number appears in `git log --first-parent githubsucks/main`, it has +landed regardless of what a lane says. + ## Repository authority - Canonical development URL: @@ -14,7 +26,8 @@ backlog. machine-local: `origin` may name this canonical URL, a release mirror, or something else, and therefore has no authority by name alone. - Canonical base at this snapshot: - `githubsucks/main` @ `fe8b8ba` (terminal copy mode #178 atop the + `githubsucks/main` @ `74301d1` (the dired Stage 1 landed docs #169 and + the PTY-terminate diagnostic #176, atop terminal copy mode #178, the GPU-terminal-input landed docs #168, Lean 4 Stage 4a #179, bottom-panel Stage 2A #177, the bottom-panel Stage 2 framing #175, terminal configuration Stage 1 #173, Lean 4 Stage 3b #170, Stage 3a @@ -25,6 +38,8 @@ backlog. #162, Lean 4 Stage 1 #160, and the minimap blank-slab fix #159; protocol v20). The previous snapshot named `a27f646`; the recovery check below accepts it or anything newer. + **Lanes below that name an older base have not been re-based; derive + their integration surface from `git diff ..main`.** - On the transfer source, `origin/main` named a release mirror at `d3fa632` and lagged badly. On the current destination, `origin` names the canonical URL. This difference is why all recovery begins by @@ -58,9 +73,106 @@ git worktree list git status --short --branch ``` -The `git log` command must expose `fe8b8ba` or a newer intentional main. +The `git log` command must expose `74301d1` — the base named above — or a +newer intentional main. Keep this threshold and the canonical-base line in +step: a recovery check that accepts an older commit than the base it +declares canonical will pass on a tree the rest of this file does not +describe. If it does not, stop and repair the remote/fetch configuration. +## PTY terminate diagnostic lane — MERGED (PR #176) + +> **Lane retained deliberately, and it is the next one to close.** #176 +> merged into `main` @ `bf8878f` (2026-07-26); rule 4 below removes a +> merged lane, but only after its durable facts reach +> `docs/agent-handoff.md`. **That absorption is unowned** — no landed-doc +> PR exists for #176 — so removing the lane now would delete the record +> instead of moving it. Whoever opens that PR removes this section. + +- Portable branch: `githubsucks/pty-terminate-eperm`; worktree + `../pmacs-math-slice`. **PR #176**, base `main`, based on `ccf29e3` + with `c93f9ee` (#175) merged in. +- Approved framing: `docs/process-signal-tolerance-framing.md` + **revision 4**, after three review rounds. +- **Diagnostic only. No disposition change.** Every call that failed + before still fails, with no state transition and no reap-ledger + arming. `src/process.rs` is the only source file touched. +- **Why nothing is fixed:** revisions 1–3 each proposed a *tolerance* + rule and all three were rejected as unsound in the same way — each + concluded something about a process from something that was not about + that process. Rev 1 from an errno alone (EPERM means the caller lacks + permission, not that the id was recycled); rev 2 from `try_wait`, + which observes the spawned **leader** while a PTY signal targets + `-tcgetpgrp(...)`, entities that diverge exactly when job control has + moved the terminal; rev 3 from group-directed **ESRCH**, which proves + only that the selected foreground group vanished. +- **Two facts that killed the original argument.** `group = true` is + *rejected* for PTY mode at spawn (`src/process.rs:1428-1429`), so the + reap ledger never applies to the PTY path at all; and the ledger + comment (`:1075`) says EPERM "cannot happen for our own children" and + drops the entry for **bounded growth** — not a ruling that EPERM means + dead. +- **The CI evidence never established the child had exited.** The probe's + last source statement is a file write and CPython teardown does not + synchronise with it, so no tolerance rule could even be shown to fix + the symptom. That is the whole reason the lane is diagnostic. +- What ships: a failing `kill` now reports five separate facts — target + source, target kind/value, spawn-time group, errno, and the leader's + real `try_wait` state. The test seam injects the **kill result only**, + never the observation, so the real `ChildHandle::try_wait` runs against + the real child. +- **Not "strictly additive".** `try_wait` reaps and caches, so an exited + child may be reaped earlier than otherwise. Safe because + `portable-pty` 0.9.0 returns a `std::process::Child` on Unix and + delegates `try_wait` to it, so `poll_one` still sees the cached + status — pinned by an exactly-one-terminal-event test rather than + assumed. +- Round-1 review fixes: the exited-child tests no longer use a fixed + sleep as proof of exit (nix's `waitid` is unavailable on macOS and + `libc::waitid` needs `unsafe`, which the crate forbids), instead + driving the production diagnostic in a bounded loop until it observes + the exit; and every assertion is now exact message equality built from + the kernel-assigned pid, since the substring forms would have accepted + a hardcoded target or a wrong exit code. +- Bites, all verified rather than assumed: tolerating the failure fails + the disposition test; stubbing the leader observation fails three + tests including the one-event pin; a hardcoded target fails four; a + wrong exit code fails two. +- **The sweep found a real defect in these tests, not a flake.** + `observing_the_leader_does_not_consume_the_exit_event` failed with + "process ProcessId(26) is not running": the pid helper drained for + `Started`, and **`drain_until` ticks**. A tick can observe an + immediately-exiting child and move the record out of `Running`, after + which `signal` never reaches the diagnostic at all, so the bounded + loop spun to its limit. It passed standalone because the drain + returned on `Started` before `poll_one` saw the exit; only load lost + the race. Fast-exiting children now read the pid straight from the + supervisor record (no tick), and the loop fails fast if the record + left `Running`. **Verified under matched load: 0/15 with all 16 cores + saturated, while the old ticking helper fails 1/10 — the fix is + load-bearing.** +- Verification: fmt, `git diff --check`, strict workspace clippy clean; + lib 1,838 + CRDT 2,015 (both +6, exactly the new tests); GPU 202; M4 + 121; bottom-panel 46; compile-mode 67; vterm 9/6/5; **isolated-config + `--no-fail-fast` sweep 3,258 across 93 suites, zero failures**. + Earlier sweeps on this branch showed two failures and then one; the + totals reconcile (3,256/2 → 3,257/1 → 3,258/0, same test count). The + two that were genuinely unrelated — + `read_dir_supersede_cancels_in_flight_predecessor` (known + pre-existing) and + `headless_snapshot_round_trip_summary_restores_the_minimap` — are + load-contention flakes; the second is structurally unreachable from + this diff, since `pmacs-gpu` depends on `pmacs-protocol` and never on + `pmacs`. +- **Parked, each with its reason:** all tolerance rules (need the + evidence this PR produces); `terminate` idempotence for an + already-reaped process (independent fix, different failure, one + feature per PR); and `signal_target`'s read-then-kill of `tcgetpgrp` + — still the most likely real fix site. +- **The lane closes when this merges.** It does not wait for the flake + to recur; the next occurrence carries its own evidence under whoever's + PR, and a Stage B framing follows then. + ## Lean 4 lane (Arc 8) — Stages 1, 2, 3a, 3b, 4a MERGED; 4b is next - **Stages 1, 2, 3a and 3b are MERGED** — #160 (`main` @ `0827dd1`), @@ -231,160 +343,6 @@ If it does not, stop and repair the remote/fetch configuration. --check` clean. - Stage 4b (the input method) is NOT in this PR and not started. -## Dired lane — Stages 0 and 1 MERGED; Stage 2 framing in review (#171) - -> **Lane retained deliberately.** Rule 4 below removes a lane after -> merge, but its durable facts must reach `docs/agent-handoff.md` first, -> and that absorption is the job of the **open landed-doc PR #169**. -> Writing it from here would put two PRs on the same text. #169 removes -> this lane; the Stage 2 framing PR #171 is still open besides. - -- Approved framing: `docs/dired-framing.md` **revision 6** — rev 5 is the - approved text (merged as its own docs PR #164), rev 6 adds §0's Stage 1 - implementation notes (S1-1…S1-9). Stages 2 (marks and operations) and 3 - (wdired) each get their own detailed framing after the prior stage lands. -- **Stage 0 (`C-x C-f` find-file) MERGED as #162** (`main` @ `2af1ab3`, - 2026-07-25, one review round, 12/12 CI green). Durable facts moved to - `docs/agent-handoff.md` §1 per rule 3 below. -- **Stage 1 branch: `githubsucks/dired-stage1`**, worktree - `../pmacs-dired-stage1`, based on `githubsucks/main` @ `8c86d34` (the - framing merge #164). **A fresh cut, not a rebase:** the older `dired` - branch (`ffdd642`, worktree `../pmacs-dired-arc`) was based on the - superseded `0827dd1` and carried only the framing content #164 already - put on `main`, so merging it would have reconciled two histories of one - document. It is left untouched and carries nothing unmerged. -- **Stage 1 implemented; no wire change (protocol stays v20).** What - landed on the branch: - - `builtin/runtime/dired.lua`: one buffer per directory named - `*dired:*` with the handle-table ownership check; - read-only intercept + `set_round_trip_input`; the `dired` major mode - and its mode-scoped keymap (`RET`/`f`, `^`, `n`/`p`, `g`, `q`, `s`); - basename cursor re-seating across every wholesale repaint; - `display_file` for file visits and same-window reuse for directory - descent; `C-x d` / `C-x C-j`; the `dired.kill-when-opening` setting. - Loaded after `window.lua`. - - `src/fs.rs`: `ReadDirTolerance`, `FsDirEntryError`, `FsDirListing`, - and one walk that either fails on a per-entry condition or records it - (Q#DR6). `src/async_runtime.rs` carries the listing in - `ReplyKind::ReadDir` / `JobResult::ReadDir`; `src/lua_bindings/mod.rs` - keys the Lua result **shape** on `errors.is_some()`, so the bare array - the frozen M8.2 fixture consumes with `ipairs` is untouched; - `builtin/runtime/fs.lua` validates read-op opts and **rejects unknown - keys** (a typo'd `tolerant` used to degrade silently to fatal). - - `src/editor_core.rs` + `src/lua_bindings/mod.rs`: - `normalize_buffer_path` is `pub` and exposed as - `pmacs.path.canonicalize` — Q#DR2's preferred end state, so no Lua - mirror exists and Stage 2 owes no mirror removal. This makes B2 - ("tolerant `read_dir` is the only Rust change") false by one small - binding, deliberately. - - `tests/dired_acceptance.rs`: 22 tests over framing items 1–16, - dispatch-driven; item 17 is the m8_1/m8_2/m8_3 additivity gate. -- **The framing claim the substrate falsified (S1-2):** R2-3 expected a - dedicated dired panel to carry its dedication across a descent. - `display_buffer` never replaces the buffer in a slot dedicated to - another one — it discards every side-specific parameter and falls back - to the document window (Q#BP3 2.iii), and the exact-window arm errors. - Dired does not unpin the user's panel; both arms are pinned. -- **The vacuity the bites found (S1-3):** acceptance 3c cannot pin the - descent *routing*. Dired holds focus in its own panel, so a raw - `switch_buffer` lands in the same window and every 3c assertion holds - either way. Dedication is the only discriminator, so the - dedicated-panel test is the real pin — and the vacuity is documented at - the assertion rather than relabelled. -- **The pre-existing test dired's first mode-scoped binding broke - (S1-4):** `describe_key_identifies_every_default_binding` asserted every - binding in the stack resolves through `describe.key` context-free, which - held only while the modes table was empty. It now sets the effective - context per binding and explicitly *clears* the mode for global ones, - because a leaked mode legitimately shadows a global chord of the same - name (dired's `RET` shadows `edit.newline-and-indent`). -- Durable substrate facts, independent of this arc: - - `pmacs.buffer.kill` (not `remove`) redirects windows off a doomed - buffer before removal, so `kill-when-opening` kills **after** the - replacement is displayed. - - Interactive origin does **not** survive an await: work resumed in - `tick_async` sees no `InteractiveCommandOrigin`, so `pmacs.window.*` - acts for the *ambient* active frontend (S1-9). - - Kinds are lstat-based in both `read_dir` and `stat`, so nothing in an - entry says whether a symlink points at a directory; `RET` probes by - trying to list it (S1-8). - - A path-backed buffer's *name* is its full path, not its basename — - worth knowing before writing any name assertion. - - `C-x d` takes **no** completion source on purpose (S1-5): with one, - RET on an empty field opens whatever sorts first, and - RET-on-where-you-are is the gesture the binding exists for. The field - is prefilled instead. -- **Bite verification:** 15 claims, each mutated in place and required to - fail the test that names it. `dired.lua` is new, so `scripts/bite`'s - file swap does not apply; every mutation was applied and reverted with - `git checkout --`. One came back VACUOUS and is recorded above. -- **Review round 1 addressed** (framing rev 7, S1-10…S1-12). Three - behavioral fixes, each bite-verified: `dired.revert`'s re-seat is - guarded on the active buffer (an ambient `move_to_line` after an await - moved an unrelated buffer's cursor — the buffer-level instance of - S1-9); `fmt_size` keeps the column width past ten digits, because - `_layout` is a contract Stage 3 is planned against; and the symlink - descent dropped its probe, since `open_directory`'s - changed-nothing-on-failure invariant *is* the probe (it was listing the - target directory twice). Plus a consecutive-`readdir`-error cap, because - **nothing cancels a dired listing** — it carries no supersede key, so - cancellation was never the backstop the tolerant loop implicitly relied - on. Naming/comment findings taken as-is. - - Durable process lesson, hit twice now: a mutation-bite helper restores - with `git checkout --`, which reverts to **HEAD** — so a fix must be - committed *before* it is bitten. Round 1's fixes were briefly wiped by - exactly that. -- **Canonical main integrated twice** — at `46a1b8f` (multi-root LSP - affinity #161) and again at `b889873` (GPU terminal input #166), both - merged rather than rebased per the #135/#137 precedent so the review - anchors stay addressable. Each conflict was a single doc hunk resolved - as the union: this lane owns COHERENCE's journey step 7 file half, #161 - owns the in-flight list, #166 owns step 8's GPU-terminal addendum. - Three things worth carrying: - - **A conflicting PR silently stops running CI.** GitHub builds - `pull_request` runs against the merge ref, which does not exist while - the PR conflicts, so no run is created and nothing reports a - failure — the checks list simply stays as it was. Three pushes to - this branch produced no CI at all before the cause was found. Watch - `mergeable` on a long-lived lane, not just the check list. - - #161's own COHERENCE finding **falsified a claim in this lane's - module doc**: `pmacs.error` is never defined in production, so an - uncaught raise inside a `pmacs.async` coroutine does not reach - `*errors*` as the comment said. It reaches a bare `error()` inside - `pmacs._async.tick()`, whose result `tick_async` discards with - `let _ =` — i.e. nowhere. That makes dired's per-coroutine `pcall` + - `set_status` load-bearing rather than tidy, and the comment now says - so. - - **A lane in review against a fast-moving `main` needs its gates rerun - per integration, not per push.** Main advanced twice inside this - review round, and the second time landed while the first - integration's sweep was still running. The numbers below describe the - twice-merged tree. -- Verification on the twice-merged tree (`main` @ `b889873`): - `cargo fmt --check` clean; strict workspace Clippy clean; **1,832 - default + 2,009 CRDT** library tests; dired acceptance **25 default + - 25 CRDT**; m8_1 10 / m8_2 15 / m8_3 32 unchanged; multi-root 13 and - vterm Stage 3 5 (both suites main added, green under this lane's - `mod.rs` and `editor.rs` changes); M4 121; required GPU 155; - **isolated-`XDG_CONFIG_HOME` workspace sweep 3,205 passed across 93 - suites, zero failures**; `git diff --check` clean. The sweep needs the - isolated config for the reason recorded in the bottom-panel lane - below. -- Coherence (framing §0.5, required since #163): serves `COHERENCE.md` §20 - Priority 1, which names this work explicitly; journey step 7's file half - goes from no surface to a surface; **adds no interaction island** — keys - are a mode-scoped keymap, and wdired will be a mode swap; adopts - `pmacs.config` for `dired.kill-when-opening`; inherits §9's - worker-attribution gap for its `read_dir` jobs without worsening it. The - audited claims this changes are updated in `COHERENCE.md` itself, per its - §25. -- **Boundary with the Journey Stage 1 arc** (`COHERENCE.md` §20 arc-cut - 1): CLI directory-argument handling (`pmacs .` exits 1) belongs there, - not here — Stage 1 does **not** fix it. The two meet at - `resolve_target_buffer`; dired supplies the buffer a directory should - resolve *to*, and `pmacs .` should route into it rather than growing a - second directory surface. - ## The CRDT half of the test corpus is dark in CI — NEEDS A LANE - **No branch, no framing yet.** Found while gating #166, then measured @@ -396,8 +354,10 @@ If it does not, stop and repair the remote/fetch configuration. Every `#[cfg(feature = "crdt")]` test is therefore **not compiled** in CI, not merely skipped. - **Measured, `--list` under CI's exact flags versus the same flags plus - `crdt`: 3,170 vs 3,443 — 273 tests dark.** Re-measured at `fe8b8ba` - (2026-07-26). **The number moves with every merge and must be + `crdt`: 3,176 vs 3,449 — 273 tests dark.** Re-measured at `74301d1` + (2026-07-26; at `fe8b8ba` it read 3,170 vs 3,443, the same 273 dark — + #176 added six tests, none of them `crdt`-gated). **The number moves + with every merge and must be re-measured, not quoted.** #168 reported 3,024 vs 3,288 — 264 dark, 177 in the library — at `1b6a084`; #178 then added CRDT-only generated-buffer coverage, and other lanes landed CRDT tests in @@ -405,7 +365,7 @@ If it does not, stop and repair the remote/fetch configuration. | dark | CI | full | target | |---:|---:|---:|---| - | 185 | 1,842 | 2,027 | **the library itself** (`src/lib.rs`) | + | 185 | 1,848 | 2,033 | **the library itself** (`src/lib.rs`) | | 21 | 15 | 36 | `m5_5_acceptance` | | 13 | 1 | 14 | `gpu_invocation_acceptance` | | 13 | 1 | 14 | `gpu_initial_target_acceptance` | @@ -491,8 +451,8 @@ If it does not, stop and repair the remote/fetch configuration. `process::tests::setsid_escapee_is_not_reaped_and_teardown_reclaims_readers` — `active_reader_probe` returning `None` at `process.rs:3179` ("live runtime probe"). **Pre-existing and unrelated to #178:** that branch - did not touch `src/process.rs` (last changed by the Darwin PTY - signal-name fix), and the test passed 10/10 standalone; the observed + did not touch `src/process.rs` at all, and the test passed 10/10 + standalone; the observed failures were during parallel full-suite runs. That localizes the trigger to suite load or interaction, but does **not** distinguish parallelism from another full-suite effect — no serial full-suite bite @@ -505,10 +465,15 @@ If it does not, stop and repair the remote/fetch configuration. test names were captured. - **A second standing obstacle for this lane:** `cargo clippy --workspace --all-targets --features crdt -- -D warnings` **fails on `main`** — - re-verified at `fe8b8ba`: four errors in `src/daemon.rs` - (`useless_conversion` at 3996, missing doc backticks at 4076, - `too_many_lines` 112/100 at 4083, an unneeded `mut` at 4965) and one in - `tests/auto_indent_crdt_acceptance.rs:42` (doc backticks). The + measured at `74301d1`: seven errors before the build aborts, four in + `src/daemon.rs` (`useless_conversion` at 3996, missing doc backticks at + 4076, `too_many_lines` 112/100 at 4083, an unneeded `mut` at 4965) and + three in `tests/vterm_stage3_acceptance.rs` (`too_many_lines` at 637 + and 793, a redundant `continue` at 843). **Treat that as a lower + bound, not an inventory:** Clippy abandons the remaining targets once + one fails, and a run on an older tree surfaced a further doc-backticks + error in `tests/auto_indent_crdt_acceptance.rs:42` that this run never + reached. The standing gate list runs Clippy without `crdt`, so these lints have never been enforced. Any CI job that compiles the `crdt` targets has to fix them first or it will be red on arrival. @@ -717,6 +682,38 @@ git worktree add --track \ `../pmacs-terminal-config` and `../pmacs-terminal-copy-mode` are retained. The gate-run flake found while gating #178 moved to the CI `crdt`-coverage lane above, which owns its discrimination. +- **Dired Stage 1 (the directory view) — MERGED as #165** (`main` @ + `c8ec8f3`, 2026-07-25, after one review round). pmacs has a directory + surface: `C-x d` / `C-x C-j`, one read-only buffer per directory named + `*dired:*`, a `dired` major mode carrying + `RET`/`f`, `^`, `n`/`p`, `g`, `q`, `s`. No wire change (v20). The Rust is + two things — a per-entry-tolerant `read_dir` (Q#DR6), which had to be + Rust because `read_dir_blocking` fails a whole listing on any of five + per-entry conditions and a tolerant wrapper cannot be written in Lua at + all, and `normalize_buffer_path` going `pub` as + `pmacs.path.canonicalize` (Q#DR2's preferred end state, so no Lua mirror + exists and Stage 2 owes no mirror removal). The frozen m8_1/m8_2/m8_3 + counts are unchanged, which is the additivity gate. 15 claims + bite-verified; one came back VACUOUS (acceptance 3c cannot pin descent + routing — dired holds focus in its own panel, so dedication is the only + discriminator) and is documented at the assertion rather than + relabelled. Its branch (`dired-stage1`) and worktree + (`../pmacs-dired-stage1`) are done; the abandoned `dired` branch + (`ffdd642`, `../pmacs-dired-arc`) was superseded by a fresh cut and + carries nothing unmerged. **Stage 2 (marks and operations) and Stage 3 + (wdired) each still need their own framing**, and the frozen fixture + shrinks after Stage 3. Durable substrate facts and both new ops lessons + live in `docs/agent-handoff.md` §§1/5; the implementation notes are + `docs/dired-framing.md` §0, S1-1…S1-12. Two named forward items for + Stage 2: `apply_resource_op`'s rename rebind is exact-PathBuf-equality, + first-match-only, looked up with the raw path while stored paths are + normalized — so a directory rename strands every buffer under it, and + `pmacs.fs.rename` has zero production callers, so it can be fixed at + the primitive; and Q#DR5's seam is the main-thread drain + `AsyncRuntime::tick`, not `_take_result`, where rename settles as an + undifferentiated `ReplyKind::FsUnit` and so must be keyed on + `JobKind::FsRename`. + - **GPU terminal input (the double terminal-layout sync) — MERGED as #166** (`main` @ `b889873`, 2026-07-25, one review round, all twelve checks green after a macOS PTY-timing rerun). The dispatcher applied **both** diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 883d860..7b1173f 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -41,8 +41,10 @@ commands, read `docs/active-work.md` immediately after this file. ## 1. Where the project stands (2026-07-26) -- `main` @ `fe8b8ba` (terminal copy mode #178 atop the GPU-terminal-input - landed docs #168, Lean 4 Stage 4a #179, bottom-panel Stage 2A +- `main` @ `74301d1` (the dired Stage 1 landed docs #169 and the + PTY-terminate diagnostic #176, atop terminal copy mode #178, the + GPU-terminal-input landed docs #168, Lean 4 Stage 4a #179, + bottom-panel Stage 2A #177, the bottom-panel Stage 2 framing #175, terminal configuration Stage 1 #173, Lean 4 Stage 3b #170, Stage 3a #167, the CRDT undo repro #157, the inline-math landed-doc refresh #172, the bottom-panel @@ -231,13 +233,78 @@ commands, read `docs/active-work.md` immediately after this file. against an open buffer yet fails to load one that is not open — find-file expands the tilde Lua-side. Loading through the normalized path is a named deferral. - - **Stage 1 (the directory view) MERGED as #165** — the builtin - `dired.lua`, the per-entry-tolerant `read_dir` opt, and - `pmacs.path.canonicalize`. Its durable facts have **not** been - absorbed here yet: that is the job of the open landed-doc PR - **#169**, and duplicating it from this PR would put two authorities - on the same text. Until #169 merges, `docs/active-work.md`'s dired - lane remains the record. +- **dired Stage 1 — the directory view — LANDED — #165** + (`docs/dired-framing.md` §0, S1-1…S1-12; merge `c8ec8f3`; one review + round). pmacs now has a directory surface: `C-x d` / `C-x C-j` open a + read-only listing, one buffer per directory named + `*dired:*`, with a `dired` major mode whose + mode-scoped keymap carries `RET`/`f`, `^`, `n`/`p`, `g`, `q`, `s`. + Protocol unchanged at **v20**. **Stage 2 (marks and operations) and + Stage 3 (wdired) each still need their own framing**; the frozen + fixture shrinks after Stage 3. + - **The Rust is confined to two things**: a per-entry-tolerant + `read_dir` (`ReadDirTolerance {Fatal, PerEntry}` → + `FsDirListing {entries, errors}`), because `read_dir_blocking` fails + a whole listing on any of five per-entry conditions and the tolerant + wrapper its own module doc delegates to package authors **cannot be + written in Lua** (one error value, no partial vec); and + `editor_core::normalize_buffer_path` becoming `pub`, exposed as + `pmacs.path.canonicalize`. Only non-UTF-8 **names** stay fatal — + byte-preserving paths would be needed. The Lua result **shape** keys + on `errors.is_some()`, so the bare array the frozen M8.2 fixture + consumes with `ipairs` is untouched. + - **Exposing a core normalizer beat mirroring it in Lua.** A Lua mirror + would have been a second canonical form — the same class of bug as + the five tab-width constants (#137). Applies to any future Lua-side + path reckoning. + - **A fixed-width column must be fixed-width for every input.** The + exported `pmacs.dired._layout` (MARK 0, KIND 2, PERMS 3–12, SIZE 13, + MTIME 24, NAME 41) is the contract Stage 3 reads offsets from, and + `%10d` overflows at ≥10 GB, silently shifting every column right of + it. Sizes now fall back to a width-clamped magnitude (K/M/G/T/P/E). + - **An ambient action must be gated on the buffer it assumes.** A + revert's cursor re-seat settles a tick or more later, by which time + the user may have switched buffers; the paint names its buffer and is + safe, but seating is ambient. This is the buffer-level instance of + the rule below that interactive origin does not survive an await. + - **A failure IS an answer — don't probe first.** Kinds are lstat-based + in both `read_dir` and `stat`, so nothing in an entry says whether a + symlink points at a directory. `RET` tries to list it and treats the + failure as the answer; an explicit probe was a second full + `read_dir`, so a descent listed twice. + - **Unbounded per-entry error collection needs a cap when nothing + cancels the work.** A dired listing carries no supersede key, so + cancellation was never the backstop the tolerant loop implicitly + relied on (`READDIR_MAX_CONSECUTIVE_ENTRY_ERRORS = 1024`). + - **This is the first builtin with mode-scoped keys** (#129's first + non-detection consumer), which broke the pre-existing + `describe_key_identifies_every_default_binding`: it asserted every + binding resolves through `describe.key` context-free, which held only + while the modes table was empty. It now sets the effective context + per binding and explicitly **clears** the mode for global ones, + because a leaked mode legitimately shadows a global chord of the same + name (dired's `RET` shadows `edit.newline-and-indent`), plus a floor + assertion that at least one mode-scoped binding exists. + - **A dedicated panel does not carry its dedication across a descent** + — the framing expected it to. `display_buffer` never replaces the + buffer in a slot dedicated to another one; it discards every + side-specific parameter and falls back to the document window (Q#BP3 + 2.iii), and the exact-window arm errors. Dired does not unpin the + user's panel; both arms are pinned. + - Smaller facts worth knowing before touching this code: a path-backed + buffer's **name is its full path**, not its basename, which matters + for any name assertion; `pmacs.buffer.kill` (not `remove`) redirects + windows off a doomed buffer first, so `dired.kill-when-opening` kills + **after** the replacement is displayed; ownership is checked against + the handle table only, never the buffer name; and `C-x d` takes **no** + completion source on purpose (with one, `RET` on an empty field opens + whatever sorts first, and RET-where-you-are is the gesture the binding + exists for — the field is prefilled instead). + - Verification at merge: 1,832 default + 2,009 CRDT library tests; + dired acceptance 25 + 25 CRDT; the frozen m8_1 10 / m8_2 15 / m8_3 32 + unchanged, which is the additivity gate for the `read_dir` change; M4 + 121; required GPU 155; isolated-`XDG_CONFIG_HOME` workspace sweep + 3,205 across 93 suites. 15 claims bite-verified. - Protocol **v20** (`SUPPORTED=[6..=20]`; v16 = `ThemeFacts`, v17 = `FontFacts`, v18 = `StatuslineSegments`, v19 = terminal frames/events, v20 = the GPU initial-target semantic bootstrap family). @@ -1125,6 +1192,24 @@ final variant — its own round-trip cannot detect a discriminant shift. trap-guarded one-file swap over read-only `git show`, with an inverted verdict (exit 0 iff the tests FAIL against the old version), making bite-verification machine-checkable. +- **A fix must be COMMITTED before it is bitten.** `scripts/bite` + restores by `git checkout --`, which reverts the file to **HEAD**, not + to the state it found — so any uncommitted work in a bitten file is + destroyed. A whole review round's fixes were wiped this way during + #165. Corollary for a NEW file: the swap-over-`git show` mode does not + apply at all, so its claims must be bitten by hand-editing, which makes + the commit-first rule load-bearing rather than hygienic. +- **A CONFLICTING PR silently runs no CI at all.** GitHub builds + `pull_request` workflow runs against the PR's **merge ref**, which it + does not create while the branch conflicts with its base. So pushes + land, the branch updates, no run is ever queued, and **nothing reports + the absence** — the checks list simply keeps showing the last + successful run, which reads as current. Three pushes to #165 produced + zero CI before the cause was found, and `gh pr checks` returns nothing + usable here. On any lane that lives through a moving `main`, check + `gh pr view --json mergeable,mergeStateStatus,headRefOid` and + confirm a run exists **for the current head sha**, not merely that a + recent run was green. - **Stacked PRs**: retarget the child to main BEFORE merging the parent — GitHub auto-closes a PR whose base branch is deleted and cannot reopen it (#104 → re-opened as #105). diff --git a/docs/dired-framing.md b/docs/dired-framing.md index ada853e..83e6f1a 100644 --- a/docs/dired-framing.md +++ b/docs/dired-framing.md @@ -1,7 +1,9 @@ # Dired — framing **Revision 7 — 2026-07-25. Status: APPROVED; Stage 0 MERGED as #162; -Stage 1 IN REVIEW as PR #165, review round 1 addressed.** +Stage 1 MERGED as #165 (`main` @ `c8ec8f3`, one review round). Stage 2 +(marks and operations) and Stage 3 (wdired) each still need their own +framing before implementation; the frozen fixture shrinks after Stage 3.** Rev 1 passed a ground-truth review; rev 2 fixed round 1's seven findings; rev 3 fixed round 2's six and was approved; rev 4 recorded what Stage 0's implementation falsified in the approved text (§0); rev 5 adds the diff --git a/docs/process-signal-tolerance-framing.md b/docs/process-signal-tolerance-framing.md new file mode 100644 index 0000000..1d3c45d --- /dev/null +++ b/docs/process-signal-tolerance-framing.md @@ -0,0 +1,258 @@ +# Framing — make the PTY terminate failure self-describing (diagnostic only) + +**Revision 4.** Status: awaiting review round 4. Lane: +`pty-terminate-eperm`, worktree `../pmacs-math-slice`, based on +`githubsucks/main` @ `ccf29e3`. + +**Diagnostic only. No disposition changes, no tolerance rules, no +behavioural fix.** Every rule this document proposed across revisions 1 +to 3 is parked (§5). The lane's entire deliverable is that the next +occurrence of the failure explains itself. + +## Revision history + +**Revision 3 → 4**, after review round 3 (two blocking, one major) and +its scope call. All accepted. + +- **Group-directed ESRCH was also unsafe**, for the same reason EPERM + was: it proves the selected *foreground group* vanished, not that the + leader exited. A job-control race — foreground job exits after + `tcgetpgrp` and before `kill`, shell alive and not yet reclaiming the + terminal — would have been reported as success with the leader never + signalled. Rev 3's acceptance 7 pinned that unsafe behaviour. **All + tolerance is parked** (§5). +- **Rev 3's Stage A implemented Stage B.** It declared itself + diagnostic-only, then listed tolerance and bookkeeping acceptances. + Removed. +- **Q#PS6 (already-reaped `terminate` is `Ok`) is parked separately.** + It is an independent behavioural fix answering a different failure; + under one-feature/one-PR it does not ride with instrumentation. +- **"Strictly additive / cannot regress behaviour" was overstated** and + is narrowed (Q#PD3). +- The injected-kill seam is restored as an explicit decision (Q#PD4). + +**Rounds 1–3, for the record.** Rev 1 classified on errno alone and +claimed a live owned child cannot yield EPERM — false. Rev 2 gated on +`try_wait`, which observes the leader while a PTY signal targets the +foreground group — unsound whenever those diverge, and it could not be +shown to fix the observed failure at all. Rev 3 corrected EPERM but left +ESRCH unsafe and mixed the stages. **Three consecutive designs were +wrong in the same direction: each tried to conclude something about a +process from something that was not about that process.** + + +## 0. Coherence impact (COHERENCE §20) + +- **Journey step 8, "Open a terminal"** (§2), teardown half. **No grade + change and no behavioural change** — this lane only improves what a + failure reports. +- **Serves §9 (worker model), failure attribution**, in its most literal + sense: an error that names only an errno cannot be attributed. +- **Interaction islands: none. Config registry: not adopted. + Background-work attribution: unchanged.** +- **No audited claim in COHERENCE.md changes**, so under §25 no + COHERENCE edit rides this PR. + + +## 1. Ground truth (scouted @ `ccf29e3`, re-verified each revision) + +### 1.1 The failure reports an errno and nothing else + +`ProcessSupervisor::signal` (`src/process.rs:921`) maps the `kill` +failure to `format!("kill: {e}")` (`:931`). That string is everything a +reader gets. + +### 1.2 The signal target is not the observation target + +- **Signal target** — `signal_target` (`:687`) returns `-pgrp` for a + PTY, where `pgrp = master.process_group_leader()`: the tty's + **current foreground process group**, read at signal time. +- **Observation target** — `ChildHandle::try_wait` (`:668`) observes the + **spawned leader**. + +They coincide only while the leader owns the terminal. Job control is +precisely the mechanism that makes them diverge, and the PTY path is +**always group-directed by design** — spawn rejects `group = true` for +PTY mode with the rationale that "PTY children already lead their own +session and are signaled group-wide" (`:1428-1429`). + +**This is why every tolerance rule across rev 1–3 failed review**, and +why the diagnostic must record the target and the leader state as +*separate* facts. + +### 1.3 The reap ledger is disjoint from this path + +`tick_reap_ledger` (`:1075`) treats any probe error as "nothing left we +can reach" for **bounded growth**, asserting EPERM "cannot happen for our +own children". It is armed only for `proc.spec.group`, which PTY mode +cannot set. Rev 1's "asymmetry" argument was a misreading; withdrawn. + +### 1.4 The observed failure, and the limits of the evidence + +macOS CI, PR #172 (**docs-only** diff), `Test (macos-latest / luajit)`, +`acc28_child_input_and_the_c_c_escape_work_unchanged_in_a_panel` +([attempt 1](https://github.com/levineuwirth/pmacs/actions/runs/30177276839/attempts/1)): + +``` +in function 'terminate' +cause: ExternalError(Process("kill: EPERM: Operation not permitted")) +``` + +**Established:** the errno, and the call path +(`pmacs.terminal.terminate` → `session.rs:566` → `signal`). + +**Not established:** that the child had exited (the probe's last source +statement is a file write at +`tests/bottom_panel_stage1_acceptance.rs:2239`; CPython teardown follows +and does not synchronise with it); that any pgid was recycled; or what +the signal target actually was. + +**This is the whole reason the lane is diagnostic.** Every candidate fix +needs at least one of those three facts, and none is available. + +### 1.5 Caller inventory + +| Caller | Disposition | +|---|---| +| `src/lsp.rs:1364`, `:2427` | discards (`let _ =`) | +| `src/mcp.rs:1229`, `:1239`, `:1915` | discards (`let _ =`) | +| `src/terminal/session.rs:319`, `:607`, `:635` | discards (`let _ =`) | +| **`src/terminal/session.rs:566`** (propagating at `:577`) | **propagates** as `TerminalError::Process` | +| supervisor-internal `shutdown` path | discards | +| `src/lua_bindings/mod.rs:8150`, `:8164` | propagates to Lua | +| `src/lua_bindings/mod.rs:8717` | propagates (via `session.rs:566`) | +| `src/daemon.rs:4162` | **test-only** `.expect`, not production | + +No test in the repository asserts either error string, so widening the +message breaks nothing. + +### 1.6 `portable-pty` caches the exit status on Unix + +Pinned `portable-pty 0.9.0`: `spawn_command` returns +`std::process::Child` (`unix.rs:228`), and `impl Child for +std::process::Child::try_wait` delegates to +`std::process::Child::try_wait` (`lib.rs:271-277`), which caches into +`self.status`. Both `ChildHandle` variants therefore cache. + + +## 2. Decisions + +### Q#PD1 — what the widened error records + +On a `kill` failure in `signal`, the error carries: + +| Field | Why | +|---|---| +| **target source** — `tcgetpgrp` vs `group` vs `leader-pid` fallback | which branch of `signal_target` (`:687`) ran | +| **target kind and value** — `-pgid` or `pid`, with the number | the entity actually signalled | +| **spawn-time pgid / leader pid** | a divergence from the target is the job-control hypothesis, visible only by comparison | +| **errno** | as today | +| **leader `try_wait` state** — `exited(status)` / `live` / `unobservable(e)` | separates "the leader is gone" from "the group we signalled is gone" — the distinction all three failed designs collapsed | + +Every candidate Stage B rule is decidable from these five together, and +none is decidable from the errno alone. + +### Q#PD2 — the disposition is preserved exactly + +The call still fails, with the same `Err`, in every case. No state +transition changes, no ledger arming changes, no tolerance. A reader +diffing behaviour should find none. + +### Q#PD3 — the honest claim is "no disposition change", not "strictly additive" + +Rev 3 said the diagnostic was only an error-string change and could not +regress behaviour. **That overstated it.** `try_wait` on an exited child +**reaps it and caches the status**, so consulting it in the failure path +is an internal state change: the child may be reaped earlier than it +otherwise would be. + +Observably safe, because both variants cache (§1.6) and `poll_one` +(`:1133`) will still see `Ok(Some(_))` and emit its event. But safe by +argument is not safe by assertion, so the terminate-failure-then-tick +event pin is retained (acceptance 5). + +### Q#PD4 — the injected-kill seam injects the KILL, never the observation + +Acceptance 5 needs a forced `kill` failure while the **real** +`ChildHandle::try_wait` runs against the **real** child. A stubbed +observation would bypass exactly the code path in question. + +So the seam is a test-only override of the *kill attempt's result*, +consumed once by the signal path; everything downstream — target +selection, the observation, the error construction — runs for real. This +also makes the diagnostic's own fields testable without racing the +kernel. + +### Q#PD5 — nothing else lands here + +No tolerance rule, no idempotence change, no `signal_target` change. See +§5. + + +## 3. Bets (falsifiable) + +- **B1 — The five fields are sufficient to discriminate the §1.4 + hypotheses.** Falsified if a recurrence carries all five and still + leaves the cause ambiguous — which would itself be a finding worth + having. +- **B2 — Widening the message breaks no caller.** Evidence: §1.5, and no + test asserts the string. + +*Retracted across revisions and not reinstated:* rev 1's "a live owned +child cannot yield EPERM"; rev 2's "exit observation suffices"; rev 2's +"this removes the failure class"; rev 3's "group ESRCH is safe to +tolerate". + + +## 4. Acceptance + +1. A group-directed `kill` failure produces an error carrying all five + Q#PD1 fields, with the target rendered as `-pgid` and the leader + state distinct from it. +2. A leader-directed `kill` failure does the same, with the target + rendered as `pid` and the target source recorded as the fallback + branch. +3. The leader state renders each of `exited(status)`, `live`, and + `unobservable(e)` correctly. +4. **The disposition is unchanged**: every injected failure still + returns `Err`, with no state transition and no ledger arming + (Q#PD2). Falsified by revert — flipping any arm to `Ok` fails this. +5. **Forced injected kill failure against the real PTY child + observation**, then tick: exactly one exit event, with the correct + status (Q#PD3/Q#PD4). A fully stubbed observation does not satisfy + this and is rejected as vacuous. +6. The existing suites stay green, pinning "no behavioural change" from + the outside. + + +## 5. Parked (not deferred-and-forgotten — each needs its own evidence) + +- **All tolerance rules.** Group-directed EPERM *and* ESRCH both fail on + the §1.2 entity split; leader-directed tolerance is plausible but + unmotivated until evidence shows the fallback branch is ever taken. + Needs Stage A evidence first. +- **Q#PS6, `terminate` on an already-reaped process returning `Ok`.** + Independent behavioural fix, different failure (§1.6 of rev 3), its + own lane under one-feature/one-PR. +- **`signal_target`'s read-then-kill of `tcgetpgrp`** — still the most + likely real fix site, still unframed. +- `terminate` cancelling pending restarts; PTYs in + `pmacs.process.list`; any change to `C-c` delivery. + + +## 6. Gates + +Full suite per `CLAUDE.md`. Touched suites: +`bottom_panel_stage1_acceptance`, the vterm stages, and +`compile_mode_acceptance`. Sweep with `-- --skip basedpyright`. + + +## 7. Branch plan + +`pty-terminate-eperm`, one PR, diagnostic only. This framing is its first +commit; the instrumentation and its tests are the second. + +**The lane then closes.** It does not wait for the flake to recur: the +next occurrence — whenever it happens, under whoever's PR — carries its +own evidence, and Stage B is framed then. Math work proceeds immediately +after this lands. diff --git a/src/process.rs b/src/process.rs index 2b53bf3..9e02629 100644 --- a/src/process.rs +++ b/src/process.rs @@ -470,6 +470,13 @@ pub struct ProcessSupervisor { /// TERM→KILL window used when arming the ledger. Constant /// [`GROUP_TERM_GRACE`] in production; overridable in tests. group_term_grace: Duration, + /// Q#PD4 test seam: forces the next `kill(2)` attempt in + /// [`Self::signal`] to fail with this errno, consumed once. + /// Always `None` in production — there is no way to set it outside + /// `cfg(test)`. It replaces the *kill result only*, so the leader + /// observation still runs against the real child handle; a stubbed + /// observation would bypass the code path under test. + forced_kill_errno: Option, } /// One armed group in the reap ledger. @@ -684,7 +691,50 @@ impl ChildHandle { } } -fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { +/// Which branch of [`signal_target`] chose the target (Q#PD1). +/// +/// Recorded on failure because the branches differ in what a failing +/// `kill` can possibly mean: only [`Self::LeaderPid`] aims at the +/// spawned child itself. The other two aim at a *group*, which for a +/// PTY is read from the terminal and can belong to something the +/// supervisor never spawned. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum TargetSource { + /// The tty's current foreground process group, read at signal + /// time. Diverges from the leader exactly when job control has + /// moved the terminal. + ForegroundGroup, + /// A `group = true` pipe child leading its own process group. + SpawnGroup, + /// The child's own pid. + LeaderPid, +} + +impl TargetSource { + fn as_str(self) -> &'static str { + match self { + Self::ForegroundGroup => "tcgetpgrp", + Self::SpawnGroup => "group", + Self::LeaderPid => "leader-pid", + } + } + + /// Whether the target is a process group rather than one process. + fn is_group(self) -> bool { + matches!(self, Self::ForegroundGroup | Self::SpawnGroup) + } +} + +/// The entity a signal was actually aimed at, plus the branch that +/// chose it. Carried so a failure can report the target as a fact +/// separate from the leader's state (Q#PD1). +#[derive(Debug, Clone, Copy)] +struct SignalTarget { + pid: Pid, + source: TargetSource, +} + +fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { if let Some(runtime) = proc.runtime.as_ref() && let ChildHandle::Pty { _master: master, .. @@ -692,7 +742,10 @@ fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { && let Some(pgrp) = master.process_group_leader() && pgrp > 0 { - return Ok(Pid::from_raw(-pgrp)); + return Ok(SignalTarget { + pid: Pid::from_raw(-pgrp), + source: TargetSource::ForegroundGroup, + }); } // `group = true` pipe children lead a fresh process group // (`process_group(0)` at spawn ⇒ pgid == pid), so fatal signals @@ -700,11 +753,80 @@ fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { // (Q#CM3). if proc.spec.group { let pgid = i32::try_from(pid).map_err(|e| e.to_string())?; - return Ok(Pid::from_raw(-pgid)); + return Ok(SignalTarget { + pid: Pid::from_raw(-pgid), + source: TargetSource::SpawnGroup, + }); } - Ok(Pid::from_raw( - i32::try_from(pid).map_err(|e| e.to_string())?, - )) + Ok(SignalTarget { + pid: Pid::from_raw(i32::try_from(pid).map_err(|e| e.to_string())?), + source: TargetSource::LeaderPid, + }) +} + +/// The spawned leader's state at the moment a `kill` failed (Q#PD1). +/// +/// Deliberately reported *beside* the target rather than folded into a +/// verdict: for a PTY the two are different entities whenever job +/// control has moved the terminal, and three successive designs for +/// this code were unsound precisely because they collapsed them. +enum LeaderObservation { + Exited(TermStatus), + Live, + Unobservable(String), + NoRuntime, +} + +impl LeaderObservation { + fn render(&self) -> String { + match self { + Self::Exited(TermStatus::Exited(code)) => format!("exited(code {code})"), + Self::Exited(TermStatus::Signaled(sig)) => format!("exited(signal {sig})"), + Self::Live => "live".to_owned(), + Self::Unobservable(e) => format!("unobservable({e})"), + Self::NoRuntime => "no-runtime".to_owned(), + } + } +} + +/// Observe the spawned leader. Note this *reaps* an exited child and +/// caches its status; that is why Q#PD3 claims "no disposition change" +/// rather than "strictly additive", and why an event-count test pins +/// that `poll_one` still emits exactly one exit event afterwards. +fn observe_leader(proc: &mut ManagedProcess) -> LeaderObservation { + let Some(runtime) = proc.runtime.as_mut() else { + return LeaderObservation::NoRuntime; + }; + match runtime.child.try_wait() { + Ok(Some(status)) => LeaderObservation::Exited(status), + Ok(None) => LeaderObservation::Live, + Err(e) => LeaderObservation::Unobservable(e), + } +} + +/// Render a failing `kill` as the five facts of Q#PD1. The disposition +/// is unchanged (Q#PD2) — this only replaces a message that said +/// nothing but the errno. +fn signal_failure_report( + target: SignalTarget, + leader_pid: u32, + errno: nix::errno::Errno, + leader: &LeaderObservation, +) -> String { + let expected = if target.source.is_group() { + match i32::try_from(leader_pid) { + Ok(p) => format!(", expected_group=-{p}"), + Err(_) => String::new(), + } + } else { + String::new() + }; + format!( + "kill: {errno} (target={} via {}, leader_pid={leader_pid}{expected}, leader={})", + target.pid.as_raw(), + target.source.as_str(), + leader.render(), + ) } /// Termination status of one generation. Internal --- the supervisor @@ -807,9 +929,20 @@ impl ProcessSupervisor { shut_down: false, reap_ledger: HashMap::new(), group_term_grace: GROUP_TERM_GRACE, + forced_kill_errno: None, } } + /// Q#PD4 test seam: make the next `kill(2)` attempt in + /// [`Self::signal`] report `errno` instead of calling the kernel. + /// Consumed by that one attempt. Everything downstream — target + /// selection, the leader observation against the real child, and + /// the error construction — runs unmodified. + #[cfg(test)] + fn force_next_kill_errno(&mut self, errno: nix::errno::Errno) { + self.forced_kill_errno = Some(errno); + } + /// Override the SIGTERM-to-SIGKILL grace window. Test helper. pub fn set_grace_period(&mut self, d: Duration) { self.grace_period = d; @@ -928,7 +1061,21 @@ impl ProcessSupervisor { return Err(format!("process {id} is not running")); }; let target = signal_target(proc, pid)?; - nix::sys::signal::kill(target, Some(signal)).map_err(|e| format!("kill: {e}"))?; + // Q#PD4: the seam injects the KILL attempt's result only — + // never the observation below — so target selection, the real + // `ChildHandle::try_wait` against the real child, and the error + // construction all run for real. Consumed once. + let kill_result = match self.forced_kill_errno.take() { + Some(errno) => Err(errno), + None => nix::sys::signal::kill(target.pid, Some(signal)), + }; + if let Err(errno) = kill_result { + // Q#PD1/Q#PD2: the failure describes itself; the + // disposition is unchanged — this still returns `Err`, + // with no state transition and no ledger arming. + let leader = observe_leader(proc); + return Err(signal_failure_report(target, pid, errno, &leader)); + } if matches!(signal, Signal::SIGTERM | Signal::SIGKILL | Signal::SIGHUP) { proc.state = ProcessState::Exiting { pid, @@ -2131,6 +2278,290 @@ mod tests { ); } + /// Spawn a PTY child that leads its own session and stays alive + /// until terminated, returning its id and OS pid. + /// + /// `/bin/sleep` directly rather than through a shell: a shell may + /// place the command in a different foreground process group, and + /// these tests assert the exact target the tty reports. + fn spawn_live_pty(sup: &mut ProcessSupervisor, name: &str) -> (ProcessId, u32) { + let mut spec = ProcessSpec::new(name, "/bin/sleep"); + spec.args = vec!["30".into()]; + spec.mode = ProcessMode::Pty { + rows: 24, + cols: 80, + mode: TerminalMode::Canonical, + }; + let id = sup.spawn(spec).expect("spawn"); + (id, spawn_started_pid(sup, id)) + } + + /// The OS pid straight from the supervisor's own record, WITHOUT + /// ticking. + /// + /// `drain_until` ticks, and a tick can observe a fast child's exit + /// and transition the record out of `Running` — after which + /// `signal` returns "is not running" and never reaches the + /// diagnostic at all. Any test whose child exits promptly must read + /// the pid this way. (Found by the parallel workspace sweep: the + /// drain-based helper raced only under load.) + fn record_pid(sup: &ProcessSupervisor, id: ProcessId) -> u32 { + match sup.processes.get(&id).expect("record").state { + ProcessState::Running { pid, .. } | ProcessState::Exiting { pid, .. } => pid, + ProcessState::Starting => panic!("spawn has not reported a pid yet"), + ProcessState::Terminated(_) => { + panic!("the record already left Running; the pid is unavailable") + } + } + } + + /// Drain until `Started` and return the OS pid it carries. Safe + /// only for children that outlive the drain; see [`record_pid`]. + fn spawn_started_pid(sup: &mut ProcessSupervisor, id: ProcessId) -> u32 { + let evs = drain_until(sup, id, Duration::from_secs(5), |evs| { + evs.iter() + .any(|e| matches!(e.kind, ProcessEventKind::Started { .. })) + }); + evs.iter() + .find_map(|e| match e.kind { + ProcessEventKind::Started { pid } => Some(pid), + _ => None, + }) + .expect("Started carries a pid") + } + + /// Drive the production diagnostic until it observes the leader as + /// exited, bounded by `timeout`. + /// + /// A fixed sleep is NOT proof of exit — on a loaded runner the child + /// can still be live, which would turn these tests into false + /// failures. This synchronises on the very observation under test. + /// Each failing attempt leaves the record untouched, because the + /// failure path returns before any bookkeeping (Q#PD2), so looping + /// is side-effect free. + fn terminate_until_leader_exited( + sup: &mut ProcessSupervisor, + id: ProcessId, + timeout: Duration, + ) -> String { + let deadline = Instant::now() + timeout; + loop { + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + if err.contains("leader=exited(") { + return err; + } + assert!( + !err.contains("is not running"), + "the record left Running before the diagnostic could run, so \ + this test never exercised it: {err}" + ); + assert!( + Instant::now() < deadline, + "leader never observed as exited within {timeout:?}: {err}" + ); + std::thread::sleep(Duration::from_millis(10)); + } + } + + /// Q#PD1 acceptance 1 — a group-directed failure names the target, + /// the branch that chose it, the expected group, the errno, and the + /// leader's own state, as five separate facts. + /// + /// Asserted as an exact message against the pid the kernel actually + /// assigned, so a hardcoded target could not satisfy it. The leader + /// field is the one that matters: for a PTY the signal goes to the + /// terminal's foreground group, a different entity from the spawned + /// child whenever job control has moved the terminal. Three rejected + /// designs for this code collapsed the two; the report keeps them + /// apart, and here they are asserted to agree only because nothing + /// has moved the terminal. + #[test] + fn a_group_directed_kill_failure_reports_target_and_leader_separately() { + let mut sup = ProcessSupervisor::new(); + let (id, pid) = spawn_live_pty(&mut sup, "diag-group"); + + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + let expected = format!( + "kill: {} (target=-{pid} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=live)", + nix::errno::Errno::EPERM + ); + assert_eq!( + err, expected, + "the report names the exact target the tty reported, the exact \ + leader pid, and observes the leader as live" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#PD1 acceptance 2 — a leader-directed failure records the + /// fallback branch and a positive target, and omits the group field + /// that would be meaningless for it. Exact message again. + #[test] + fn a_leader_directed_kill_failure_reports_the_fallback_branch() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-leader", "/bin/sleep"); + spec.args = vec!["30".into()]; + let id = sup.spawn(spec).expect("spawn"); + let pid = spawn_started_pid(&mut sup, id); + + sup.force_next_kill_errno(nix::errno::Errno::ESRCH); + let err = sup.terminate(id).expect_err("injected ESRCH must fail"); + + let expected = format!( + "kill: {} (target={pid} via leader-pid, leader_pid={pid}, leader=live)", + nix::errno::Errno::ESRCH + ); + assert_eq!( + err, expected, + "a non-group pipe child targets its own pid, and the group \ + field is omitted where it has no meaning" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#PD1 acceptance 3 — every leader state renders distinctly. The + /// `Unobservable` and `NoRuntime` arms cannot be produced by a real + /// child on demand, so they are pinned directly; `live` and `exited` + /// are pinned through the real path by the tests around this one. + #[test] + fn every_leader_observation_renders_distinctly() { + assert_eq!( + LeaderObservation::Exited(TermStatus::Exited(0)).render(), + "exited(code 0)" + ); + assert_eq!( + LeaderObservation::Exited(TermStatus::Signaled("SIGTERM".into())).render(), + "exited(signal SIGTERM)" + ); + assert_eq!(LeaderObservation::Live.render(), "live"); + assert_eq!( + LeaderObservation::Unobservable("try_wait: boom".into()).render(), + "unobservable(try_wait: boom)" + ); + assert_eq!(LeaderObservation::NoRuntime.render(), "no-runtime"); + } + + /// Q#PD1 acceptance 3, exited arm through the REAL path — the leader + /// has genuinely exited and the report carries its exact code, not + /// merely "some exit". + #[test] + fn a_failure_after_the_child_exits_reports_the_leader_as_exited() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-exited", "/bin/sh"); + spec.args = vec!["-c".into(), "exit 3".into()]; + let id = sup.spawn(spec).expect("spawn"); + // NOT `spawn_started_pid`: draining ticks, and this child exits + // immediately. + let pid = record_pid(&sup, id); + + let err = terminate_until_leader_exited(&mut sup, id, Duration::from_secs(10)); + + let expected = format!( + "kill: {} (target={pid} via leader-pid, leader_pid={pid}, leader=exited(code 3))", + nix::errno::Errno::EPERM + ); + assert_eq!( + err, expected, + "the exact exit code is observed from the real child, not \ + inferred from the errno" + ); + } + + /// Q#PD2 acceptance 4 — **the disposition is unchanged.** An + /// injected failure still fails, and neither the state transition + /// nor the reap-ledger arming runs. This is the assertion that + /// separates a diagnostic from the tolerance rules three review + /// rounds rejected; flipping any arm to `Ok` fails it. + #[test] + fn an_injected_failure_changes_no_state_and_arms_no_ledger() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-disposition", "/bin/sh"); + spec.args = vec!["-c".into(), "sleep 30".into()]; + spec.group = true; + let id = sup.spawn(spec).expect("spawn"); + let pid = spawn_started_pid(&mut sup, id); + assert!( + sup.reap_ledger.is_empty(), + "precondition: nothing armed before the attempt" + ); + + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + let expected = format!( + "kill: {} (target=-{pid} via group, leader_pid={pid}, expected_group=-{pid}, leader=live)", + nix::errno::Errno::EPERM + ); + assert_eq!(err, expected, "a group=true pipe child reports via group"); + + assert!( + matches!( + sup.processes.get(&id).expect("record").state, + ProcessState::Running { .. } + ), + "a failed kill must not transition the record to Exiting" + ); + assert!( + sup.reap_ledger.is_empty(), + "a failed kill must not arm the reap ledger" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#PD3/Q#PD4 acceptance 5 — the diagnostic consults the REAL + /// `ChildHandle::try_wait` on the REAL child, which reaps it and + /// caches the status. `poll_one` must still emit exactly one exit + /// event, carrying the exact code. + /// + /// A stubbed observation would bypass the double-`try_wait` path + /// entirely and pin nothing, so the injection replaces the kill + /// result only. + #[test] + fn observing_the_leader_does_not_consume_the_exit_event() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-one-event", "/bin/sh"); + spec.args = vec!["-c".into(), "exit 7".into()]; + spec.mode = ProcessMode::Pty { + rows: 24, + cols: 80, + mode: TerminalMode::Canonical, + }; + let id = sup.spawn(spec).expect("spawn"); + // NOT `spawn_started_pid`: draining ticks, and a tick can reap + // this immediately-exiting child before the diagnostic runs. + let _ = record_pid(&sup, id); + + // Drives `observe_leader`, which try_waits the real PTY child + // for the first time and reaps it. + let err = terminate_until_leader_exited(&mut sup, id, Duration::from_secs(10)); + assert!( + err.contains("leader=exited(code 7)"), + "the real handle was consulted and carries the exact code: {err}" + ); + + // The supervisor's own try_wait must still see that status. + let evs = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); + let terminal: Vec = evs + .iter() + .filter_map(|e| match e.kind { + ProcessEventKind::Exited { code, .. } => Some(code), + ProcessEventKind::Signaled { .. } => Some(-1), + _ => None, + }) + .collect(); + assert_eq!( + terminal, + vec![7], + "exactly one terminal event survives the diagnostic's try_wait, \ + carrying the child's real exit code" + ); + } #[test] fn signal_terminates_a_running_child() { let mut sup = ProcessSupervisor::new();