diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c9a9983..0bc7ef3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -218,12 +218,28 @@ jobs: # binary is absent (a minimal container must not fail `--lib` # without ever testing pmacs) and this variable is what makes the # skip fatal where the tool is guaranteed. + # + # PMACS_REQUIRE_BASH arms the signal diagnostic's job-control + # corroboration test, which needs `/bin/bash` and `-m` putting a + # foreground job in its own process group with the terminal. + # + # Linux only, and NOT because macOS lacks bash — it ships 3.2. CI + # measured the difference: on both macOS legs a non-interactive + # `bash -m` kept the terminal on the leader for a full 10s wait, so + # the divergence the test needs does not occur there. The test skips + # on non-Linux by platform check; arming it on macOS would only make + # a missing binary fatal for a test that cannot run anyway. + # + # The divergent case itself is pinned on every platform by + # injecting the foreground group instead (framing Bet 1's stated + # fallback, after the real fixture failed its own falsifier). - run: cargo test --all-targets --no-default-features --features ${{ matrix.lua }} -- --test-threads=1 env: PMACS_REQUIRE_LSP: ${{ runner.os == 'Linux' && '1' || '' }} PMACS_REQUIRE_SHELLS: ${{ runner.os == 'Linux' && '1' || '' }} PMACS_REQUIRE_LUA: ${{ runner.os == 'Linux' && '1' || '' }} PMACS_REQUIRE_SETSID: ${{ runner.os == 'Linux' && '1' || '' }} + PMACS_REQUIRE_BASH: ${{ runner.os == 'Linux' && '1' || '' }} - run: cargo test --doc --no-default-features --features ${{ matrix.lua }} # The workspace default member is only the root `pmacs` package, so # the runs above never execute pmacs-protocol's own tests — the diff --git a/Cargo.lock b/Cargo.lock index b80dbae..610ddd0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2574,6 +2574,7 @@ dependencies = [ "codebook-tree-sitter-latex", "crossbeam", "crossterm", + "filedescriptor", "loro", "mlua", "nix 0.29.0", diff --git a/Cargo.toml b/Cargo.toml index 4e21592..e8319f9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -262,12 +262,28 @@ arborium-lean = "2.18" # T M4.4 process supervisor: signal sending without `unsafe`. Keep # the feature surface tight to keep build time low. `poll` feeds the # compile-mode group readers (cancellable poll-based reads, Q#CM3). -nix = { version = "0.29", default-features = false, features = ["signal", "user", "fs", "term", "socket", "poll"] } +# +# `process` is listed explicitly even though it already arrives +# transitively: nix's own `signal` feature depends on it, so `getpgid` +# and `tcgetpgrp` compile today without being asked for. That is +# stable but invisible, and a real requirement that depends on another +# feature's internals is one refactor away from vanishing. The signal +# diagnostic calls both directly. +nix = { version = "0.29", default-features = false, features = ["signal", "user", "fs", "term", "socket", "poll", "process"] } # T M4.4 PTY mode: portable abstraction over openpty / fork+exec # with controlling-tty wiring. The crate uses internal `unsafe` # but exposes a fully safe API; pmacs's own `unsafe_code = "forbid"` # rule still holds. portable-pty = "0.9" +# Safe file-descriptor duplication, already in the tree through +# `portable-pty`. Declared directly because the signal diagnostic calls +# `OwnedHandle::dup` itself: `MasterPty` exposes only a `RawFd`, and +# every std route from a raw fd to something implementing `AsFd` is +# `unsafe`. `dup` takes any `AsRawFd` through a safe blanket impl and +# returns an owned handle that IS `AsFd`, which is what lets pmacs call +# `nix::unistd::tcgetpgrp` — and keep the errno portable-pty discards — +# without a single `unsafe` block of its own. +filedescriptor = "0.8" # T M4.5 LSP wire format: JSON-RPC 2.0 bodies inside Content-Length # framing. Used only on LSP and similar protocols that require JSON # specifically; in-process workers continue to use MessagePack diff --git a/README.md b/README.md index cd91a87..6225678 100644 --- a/README.md +++ b/README.md @@ -227,6 +227,19 @@ translation) are routed through trampolines that exec these tools. **skips** when `setsid` is absent, so a minimal or BusyBox environment still runs `cargo test --lib`; set `PMACS_REQUIRE_SETSID=1` to make that skip a failure, as CI does on Linux. +- **`/bin/bash`** (**optional**, Linux only). The signal diagnostic's + job-control corroboration test needs a terminal whose foreground + process group is not the spawned leader, which `bash -m` produces by + running a foreground job in its own process group. The path matters: + the test spawns `/bin/bash` directly rather than resolving `bash` on + `PATH`, and skips when that path is absent. Set `PMACS_REQUIRE_BASH=1` + to make the skip a failure, as CI does on Linux. + + **It is deliberately not armed on macOS**, which ships bash 3.2 but + where a non-interactive `bash -m` was measured in CI to keep the + terminal on the leader — so the divergence the test needs never + happens there. The divergent case is pinned on every platform by + injecting the foreground group instead. - **`git`** (added in M7.2). Required for any package operation: the package fetcher shells out to `git` to clone, fetch, and resolve refs, with a deterministic environment diff --git a/docs/active-work.md b/docs/active-work.md index d8326fe..09c5c9d 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -1,17 +1,25 @@ # Active work — cross-machine resume ledger -**Snapshot: 2026-07-29.** This file records volatile work that has not +**Snapshot: 2026-07-30.** This file records volatile work that has not 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. -**No lane below is retained past its merge.** This snapshot removes the -resource-op delete guard (#186) and dired Stage 2 framing (#171) lanes -the moment their PRs merged, because the same commit put their -load-bearing decisions into `docs/agent-handoff.md` §1 — rule 4's -precondition, satisfied deliberately rather than deferred. The -bottom-panel lane is not removed: 2B-2 landing leaves 2B-3 and Stage 3 -ahead of it, so the lane is rewritten to the remaining plan. +**This snapshot is an absorption pass.** Eight PRs merged on 2026-07-29 +and 2026-07-30 (#188, #190, #191, #194, #195, #196, #197, #198) and the +ledger had drifted to 1,854 lines carrying six lanes whose work was +already on `main`. Those six are removed and their load-bearing +decisions are in `docs/agent-handoff.md` §1 — rule 4's precondition, +satisfied rather than deferred. The file is now 609 lines. + +**A lane is removed when its ARC is done, not when a PR merges.** Two +lanes survive their sub-stage merges and are rewritten to the remaining +plan rather than deleted: generated-buffer immutability (Stage 1 merged, +Stage 2 not started) and bottom-panel (Stage 2 complete, Stage 3 ahead). +The bottom-panel block said so in its own text — *"this lane is not +removed at 2B-3's merge"* — and a wholesale removal keyed on "the PR +merged" would have discarded live planning. Read each block before +cutting it. **#188's lane arrived with #188**, which is the point: with several PRs open, a lane written on `main` for work that lands elsewhere @@ -245,503 +253,39 @@ If it does not, stop and repair the remote/fetch configuration. never been enforced. Any CI job that compiles the `crdt` targets has to fix them first or it will be red on arrival. -## Generated-buffer immutability lane (Arc: workbench primitives) — STAGE 1 OPEN +## Generated-buffer immutability lane (Arc: workbench primitives) — STAGE 1 MERGED; STAGE 2 IS NEXT -**Framing: [PR #188](https://github.com/levineuwirth/pmacs/pull/188), -revision 7, approved and merged to `main` as `27b1185`. #188 owns the -acceptance contract; this lane adopts it.** On 2026-07-29 the user -directed #191 to fold its review corrections into this branch and then -merged #188, settling the implementation authority and merge ordering. -The contract is now -`docs/generated-buffer-immutability-framing.md` on canonical `main`. +**Framing #188 (revision 7) and Stage 1 #191 are both on `main` @ +`4cd4a7b`.** Their durable facts are in `docs/agent-handoff.md` §1 — +including the contract-ownership rule (the framing owns the acceptance +criteria; an implementation adopts them and may not restate or narrow +them) and why `dired`/`listview` were the correct first two families. -- **Branch `generated-buffer-immutability-stage1`**, worktree - `../pmacs-gbi-stage1`. `githubsucks/main` is integrated into it. - Measured when this line was written: - - ``` - $ git rev-parse --short githubsucks/main - 27b1185 - $ git log --oneline -1 githubsucks/main - 27b1185 Merge pull request #188 from levineuwirth/generated-buffer-immutability - $ git merge-base --is-ancestor githubsucks/main HEAD && echo "main IS integrated" - main IS integrated - ``` - - **That is a reading, not a constant, and it went stale inside this - lane's own review round.** `main` moved four times while the lane was - open: #187 -> #192 -> #193 -> #188. An earlier revision of this bullet pasted - the same three commands with `64883eb` and the same `main IS - integrated` line, and #193 merged between writing it and pushing it --- - so the pasted output was false in the tree that carried it. Pasting - command output is necessary and **not sufficient**: re-measure at push - time, and treat any base SHA in this file as expired on sight. -- **What Stage 1 ships.** `dired.lua`'s `paint` and `listview.lua`'s - `render` write through `pmacs.buffer.set_generated_contents` (zero - `bypass_intercept` writes remain in either file); `listview` gains - Q#GB13 ownership-by-handle with `<2>`..`<99>` disambiguation and - Q#GB18's identity-routed `panels` list in the **same** commit; - Q#GB6's cursor/view-top clamp plus selection clamp-or-clear in both - `EditorCore::notify_buffer_edit` and `rebuild_views_for`; - listview refresh reseating through the already-notified view rather - than a redundant same-buffer switch; and Q#GB16(a)'s corrected fold - status string. No protocol change, no new Lua surface, no new - interaction island. -- **Why these two families first, and it is not "the cheap half".** - `compile.lua:219` and `builtin/commands/default.lua:855` rebind all - seven undo chords to a no-op; `dired.lua` and `listview.lua` rebind - **nothing**, so a bare `C-/` emptied a listing and a panel. Stage 1 - closes the only two families reachable without `M-x`. -- **Review round 1 found the stale selection anchor and four acceptance - contract mismatches.** Its provisional drop-on-stale fix stopped the - crash but intentionally waited on #188 to decide the selection rule; - criteria 5 and 7 likewise recorded evidence without claiming to - replace the framing. That evidence produced #188 revision 7. -- **Review round 2 closes both remaining P1 findings against revision - 7.** - - **Q#GB6 now matches at both sites.** Cursor and anchor clamp to the - new extent; a selection survives shortened unless an endpoint - movement collapses it, in which case it clears. `acc16h` and - `acc16i` each drive a real caller and assert both the surviving - region and collapsed case. Unconditional drop and bare clamp are - separately falsified. - - **The Stage 1 criteria are adopted without local substitutes.** - Criterion 5 has the exact rope-refusal + byte-identity half and the - Rust-lifted named-intercept half for both adopters. Criterion 7 now - bites the named fan-out mutation for both adopters: listview refresh - no longer rebuilds the view with a redundant same-buffer switch. - Criteria 11 and 12 carry the framing's `[main]` classification and - also record where its narrower Q#GB13-without-Q#GB18 pre-image - fails. -- **Stage 2 still owes everything with new Rust in it**, per the - framing's cut: `Buffer::apply_generated_edit` + `GeneratedOutcome` + - the `{ generated = true }` option + its own `run_buffer_edit` arm; +- **Stage 2 is not started and has no branch.** It owns everything with + new Rust in it: `Buffer::apply_generated_edit` + `GeneratedOutcome` + + the `{ generated = true }` option and its `run_buffer_edit` arm; `set_generated_contents` reimplemented over it; Q#GB10's path-backed refusal and `mark_clean`; Q#GB15's `identity_protected`; Q#GB13/GB18 for `compile.lua` and the search panel; Q#GB5's `ensure_slot` lock; - conversion of the remaining 13 write sites; and the three - `compile_mode_acceptance` intruder tests converted per Q#GB12. -- **Verification at code checkpoint `5d92348`.** The ledger commit on - top is docs-only; `cargo fmt --check` and `git diff --check` are - re-run after it. - `cargo fmt --check` clean; `cargo clippy --workspace --all-targets -- - -D warnings` clean; library **1,863 passed + 3 ignored** default and - **2,048 passed + 4 ignored** CRDT; `listview_acceptance` **17**, - `dired_acceptance` **31**, `folding_acceptance` **21**, - `terminal_copy_mode_acceptance` **18** default and **19** with - `--features crdt` — judge that step by the count, because `acc16e` is - `#[cfg(feature = "crdt")]` and a default run never compiles it; M4 - **121 passed + 3 ignored + 1 filtered** with `--skip basedpyright`; - required GPU **202/202**. The first GPU attempt inside the tool - sandbox failed three managed-attach socket tests and left the - closed-outbox reader blocked; the authoritative rerun outside that - socket sandbox passed all 202. `git diff --check` clean. -- **The dired 200 ms perf test is load-sensitive, and the conversion - costs it nothing.** Review saw `dired_renders_10k_entries_within_200ms` - take 241 ms in a combined run and pass alone. Measured here: 0.09 s - isolated over five runs, and the whole 31-test suite finishes in - 0.12 s, so 241 ms was contention rather than a regression. Measured - against the pre-image as well, by swapping in `main`'s `dired.lua` - (the `bypass_intercept` paint): **0.09 s either way over five runs - each**. A whole-buffer `set_generated_contents` costs the same as the - bypass replace it replaces, which discharges Q#GB4's measurement - obligation for the whole-buffer case only — the streaming case is - Stage 2's and is not touched here. -- **Bites, re-run under `scripts/bite`'s positive control (#192).** - A bare `bite: OK` from the pre-#192 script is weaker than it looks, so - every result below is from the current script or from a mutation - harness carrying the same control (named tests must pass on the - working tree and at least one must have run). - - **Falsified by revert, all `OK (assertion)` — not `OK (COMPILE)`:** - `builtin/runtime/listview.lua` for criteria 1, 2, 9 and 10; - `builtin/runtime/dired.lua` for criteria 3 and 13a; - `src/lua_bindings/fold.rs` for 13b; `src/editor_core.rs` for 8, 8b - and both selection-normalization pins. - - **Falsified by a named mutation, each observed to fail:** the - fan-out drop in the `set_generated_contents` binding (criterion 7); - deleting `self.read_only = false` (criterion 4, both adopters); - deleting `add_intercept` and `set_round_trip_input` at each adopter - (criteria 5 and 6); the name-keyed `panel_for_buffer` (criteria 11 - and 12); adopting at the variant limit (criterion 10); the old fold - status string (13b); deleting each clamp (8, 8b); deleting the - selection helper from either site; unconditionally dropping a stale - anchor; and retaining a selection that an endpoint clamp collapsed. - Criterion 7's fan-out drop now fails by assertion in **both** - listview and dired. -- **Recovery:** + the remaining 13 write sites; and the three `compile_mode_acceptance` + intruder tests converted per Q#GB12. +- **It collides with dired Stage 2b**, which changes `paint`'s callers. + Whichever starts second integrates first. - ```sh - git fetch githubsucks - git worktree add ../pmacs-gbi-stage1 generated-buffer-immutability-stage1 - cd ../pmacs-gbi-stage1 - cargo test --test listview_acceptance --test dired_acceptance - cargo test --test terminal_copy_mode_acceptance --features crdt - ``` -## Bottom-panel lane (Arc 7) — 2B-3 OPEN; Stage 2 is COMPLETE with it +## Bottom-panel lane (Arc 7) — STAGE 2 COMPLETE; STAGE 3 IS THE LAST STEP -Stage 1, the Stage 2 framing, Stage 2A, Stage 2B-1, and Stage 2B-2 are -all on `main`. **Stage 2B-3 — the GPU panel band, compatible protocol-v21 -activation, and the negotiated `panel_capable` flip — is this lane's open -work**, and it completes Stage 2. Framing revision 5's three-way split of -2B was explicitly approved on 2026-07-27; revision 6 records PR #184's -review correction. +**Stage 1, the Stage 2 framing, and Stages 2A, 2B-1, 2B-2 and 2B-3 are +all on `main` @ `4cd4a7b`** (#155, #175, #177, #184, #187, #198). Stage 2 +is complete. Durable facts are in `docs/agent-handoff.md` §1, including +the v20-baseline / v21-negotiated handshake that Stage 2B-3 made +compatible. -- **Branch `bottom-panel-stage2b3`, worktree `../pmacs-bp-2b3`, cut fresh - from landed `main` @ `e003b81`** (`Merge pull request #190 from - levineuwirth/resource-op-delete-guard-impl`) — measured with - `git log --oneline -1 githubsucks/main`, not carried over from a - briefing. Not stacked on `../pmacs-bp-stage2b2`: each slice starts from - landed main, which is the house pattern and what 2B-2 did. -- **The activation mechanism, and why the alternatives fail.** The - constraint inherited from 2B-1's review is that the unsolicited `Hello` - may not become 21: it is server-first, so a shipped v20 frontend rejects - an unknown version *before* it can send `AttachRequest`. What 2B-3 ships - instead is a **frontend counter-offer**: - `ADVERTISED_PROTOCOL_VERSION` becomes a permanent compatibility - *baseline* (20), the frontend answers - `requested_protocol_version(baseline)` — its own `PROTOCOL_VERSION` when - the baseline is current, a verbatim echo of anything older — and the - daemon records `negotiated_session_version(offer)`. The daemon needed - **no change** to accept it; it already recorded `req.protocol_version`. - Rejected alternatives, each for a reason the server-first shape forces: - growing `Hello`/`InstanceCapabilities` (postcard structs are positional, - so it breaks the *other* direction symmetrically and cannot be verified - against shipped binaries); a client-first hint (an old client writes - nothing until it has read `Hello`, so any probe is a timeout); - sniffing `instance_identity.pmacs_version` (version-string branching on - a field documented as display-only); and a second post-`Hello` daemon - message (an old client reads it as an `InstanceMessage` and dies on the - unknown discriminant). -- **The window this leaves open is named, not hidden.** A daemon whose own - `PROTOCOL_VERSION` equals the baseline rejects an offer above its - supported range. A single `AttachRequest` cannot mean both "I want 21" - and "≤ 20", so compatibility is preserved for old *frontends* — the - direction that matters, since the daemon is what a user leaves running — - and the window closes on the next daemon restart. It surfaces as an - explicit `GoodbyeReason::VersionMismatch` naming both versions, pinned by - `an_unsupported_offer_is_refused_by_name`. -- **Review round 1 closed six findings at `PR #198`, four of them one - shape.** The panel layer was a *partial port* of the document/terminal - layer and the new tests asserted the declaration side only, so each - omission was invisible. Audited as a port rather than patched as a list, - which found two more gaps of the same shape that review had not named: - the headless probe never armed the panel wire at all (so nothing could - ever exercise a band), and a disconnect left the band on screen. - - **Geometry agreed in one place instead of three.** The declaration used - the stable probe while painting and hit-testing used the - document-dependent `mono_advance`, and the declaration inset its width - by `TEXT_LEFT` against the parent framing's explicit x=0 full-width - contract. The advance is now cached **behind the declaration** - (`PanelBand::declared_advance`), so the three cannot drift: there is one - value. Framing rev 7 R7-1/R7-2 records the widened contracts. - - **One classifier owns "does the band claim this pixel".** Four pointer - handlers each decided for themselves and three simply did not ask, so - right-click and wheel were applied to the document underneath and a held - left button was reported as a hover. `PointerSurface` / - `classify_pointer_surface` makes forgetting the band impossible to do - quietly, and makes the routing testable without a window or a daemon. - - **`focused` decides the caret.** The producer ships `cursor` for a - passive panel too, so painting it unconditionally put a second insertion - caret on screen. - - **Planned underlines were never consumed.** Straight forms now ride the - quad batch, curly the squiggle pipeline — the terminal path's split. - - **`VersionMismatch { server }` is the instance's `PROTOCOL_VERSION`.** - Reporting the advertised baseline told a frontend the daemon topped out - at 20 while it spoke 21. **The acceptance had pinned the wrong value**, - so the test was holding the bug in place; fixed and re-pinned. - - **Criterion 54 is satisfied, not caveated.** The panel-hosted probe is - `a54_real_daemon_real_pty_and_headless_gpu_render_one_panel_hosted_terminal` - — one real daemon, one real PTY child, real wgpu, through - `display = "panel"`. Opening a terminal and *moving* it was the first - attempt and is subtly wrong: the buffer ends up displayed twice and the - document window keeps projecting it, so the acceptance cannot tell a - panel-hosted child from a document one. -- **Round 1 verification: 41 bites, 41 OK.** The original 23 re-run after the - round-1 changes (none went vacuous) plus 18 new ones covering every changed - pin. The forbidden act is bited twice: mutating the constant trips a - const-block assertion at *build* time (stronger outcome, weaker evidence), - so a narrowed swap that compiles both ways mutates the daemon's `Hello` - value directly. **A probe bite must rebuild `pmacs-gpu`** — `cargo test` - does not rebuild the sibling binary the acceptance runs as a process, and - without that the mutation never reaches it and the bite reads as vacuous - for the wrong reason. -- **What 12/12 green did not catch.** The reviewer confirmed all twelve - checks, `bottom_panel_stage2b_gpu_acceptance` 4/4, and required GPU 216/216 - on the head that carried all six defects. The gate suite is not the check - on a slice like this; the assertions are. -- **Six commits:** the activation mechanism, the three-boundary split plus - the band's machinery, the paint/input wiring, the acceptance suites, the - bite fixes, and the round-1 port completion — plus docs. -- **Verification on the reviewed head** (all redirected to files and - checked by their own exit codes, never through a pipe): recorded in the - PR body and reproduced by the commands in `CLAUDE.md`. -- **23 falsifying mutations, all executed, all biting**, using an in-place - mutation runner rather than `scripts/bite` because several claims live in - the *same file* as their test and a whole-file swap would not compile. - **Three assertions were VACUOUS on the first pass and the mutation runs - are what found them** — a fixed-point contrast check, a pixel test that - passed with the band painting nothing, and a monospace fixture that could - not tell two glyph-advance derivations apart. A fourth finding was about - the code, not the tests: "no panel frame reaches a v20 session" is - defence in depth, not the placement gate, and passes with the capability - gate removed entirely. -- **Two real defects the new tests caught in the implementation**, both - recorded in `docs/agent-handoff.md` §1: `edge_scroll_direction` has no - upper bound, so reclassifying its boundary was necessary but not - sufficient; and `apply_panel_payload` ignored the exhaustion latch. -- **Cross-machine recovery (fresh clone):** - - ```sh - git fetch githubsucks --prune - git switch --track -c bottom-panel-stage2b3 githubsucks/bottom-panel-stage2b3 - git rev-parse HEAD - ``` - -- **Stage 3 (the adopter default flip) is the arc's last step** and is not - started. This lane is not removed at 2B-3's merge: Stage 3 remains ahead - of it. - -- **Stage 2B-2 MERGED as #187.** One review round of five findings on top - of the implementation, 12/12 green, 22/22 mutations biting. Its durable - lessons are in `docs/agent-handoff.md` §1. -- **What PR #187 shipped, dark by construction:** the semantic daemon's - `FrontendCellGeometry` epoch machine; one reconciled panel grid - derivation; `PanelFrame::{Present, Absent}` projection on both document - and terminal semantic paths; stable presentation epochs; resize and - pointer validation against the live window/buffer/epochs; the panel's - own statusline context; and pre-drain semantic panel-terminal resize. - It does not add the GPU consumer or enable the capability. -- **Review round 1 closed five findings plus one sweep result at - `3ecb03d`.** The wire-area clamp became durable hide state; a stale - same-buffer reopen can no longer retain input authority; semantic panel - terminals resize before child drain; `NoMessage` retains a published - band baseline while `Invalidated` clears it; wheel activation follows - the terminal-only focus rule; and legally wide panels clamp their PTY - content without disappearing. -- **Review round 2 closed two findings at `bfaaf2b` plus this ledger - commit.** Side affinity can replace the buffer while preserving the - `WindowId`, so retained panel statusline segments are now keyed by the - full `(WindowId, BufferId)` presentation. Every authoritative `Absent` - also clears that baseline, including duplicate-suppressed `Absent`, so - a later `Present` under `NoMessage` cannot resurrect peer state that - was already cleared. Two acceptance tests bite those exact transitions. - This lane and `docs/agent-handoff.md` now name the open PR, current - landed base, checkpoint, and 2B-3 ordering instead of calling 2B-2 - merely “next.” -- **Round-2 verification at code checkpoint `bfaaf2b`:** formatting and - strict workspace Clippy; library **1,863 passed + 3 ignored** default - and **2,048 passed + 4 ignored** CRDT; bottom-panel Stage 1 / 2A / - 2B-1 / 2B-2 **46 / 17 / 16 / 28**; statusline **8 CRDT**; semantic - routing **2 CRDT**; M4 **121 passed + 3 ignored + 1 filtered**; - required GPU **202/202**; isolated-config full workspace sweep; and - `git diff --check`. The first workspace sweep had one GPU rendering - failure in `failures_and_display_math_render_as_source`; that test had - passed in the immediately preceding required-GPU run, passed alone, - and the complete workspace rerun passed. Real-daemon and managed-attach - cases were rerun outside the tool sandbox after its local-socket policy - produced `Operation not permitted`; the authoritative reruns passed. -- **Cross-machine recovery (fresh clone):** - - ```sh - git fetch githubsucks --prune - git switch --track -c bottom-panel-stage2b2 githubsucks/bottom-panel-stage2b2 - git rev-parse HEAD - ``` - - #187 has landed, so `githubsucks/main` already contains this work and - the branch is retained only for provenance. 2B-3 was cut from `main`. -- **Stage 2B-1 MERGED as #184** (`main` @ `6bee09d`, 2026-07-28; all - twelve checks green on the reviewed head `5539b6e`; two review rounds - plus a gate-found follow-up). Branch - `githubsucks/bottom-panel-stage2b` and worktree `../pmacs-bp-stage2b` - are retained and carry nothing unmerged. Durable facts — the - schema-support-versus-advertisement split, the shared `wire_grid` - boundary, authoritative `Absent`, and the two epochs — are in - `docs/agent-handoff.md` §1 per rule 3, not here. -- **What 2B-1 deliberately did not do**, because 2B-2 and 2B-3 must not - re-litigate it: no producer, no consumer, no capability change. - `panel_capable` is still `false` for every semantic session, so the - journey grade is unchanged and every shipped v20 client remains - attachable. -- **2B-3 discharged the hard constraint 2B-1's review set**: the - *compatibility-preserving* activation is the frontend counter-offer - described at the top of this lane, and the unsolicited `Hello` is - unchanged. 2B-1's real-daemon acceptance for the v20 rejection point - still passes untouched. -- **Two review rounds, and what each cost.** Round 1: `PanelFrame` - needed an explicit `buffer_id`, the transport ratchet had to drive the - real attach path rather than a detached codec assertion, and shared - grid bounds needed one validator. Round 2: the server-first `Hello` - made the advertised v20↔v21 compatibility one-way; `COHERENCE.md` and - the handoff still named only v20 schema support; framing §9 named a - nonexistent aggregate 2B suite instead of the three exact slice - suites; and the panel plus copied-terminal "one byte over" fixtures - were actually two bytes over. -- **The full gate — not review — found two version-ladder omissions and - one probe contradiction.** The statusline and Vterm Stage 3 ladders - still pinned v20 and rejected v21. Separately, Vterm Stage 3's - headless probe exited its loop as soon as resize plus two nonuniform - composites were observed, while its acceptance later required the PTY - child's `VTERMROW` output in the final frame; the v20-compatible - handshake made that scheduling race deterministic, so the report - sampled a blank frame. The probe now waits for the exact child-output - observation its acceptance asserts. -- **The probe fix then leaked its own fixture, and that is the reusable - lesson.** The generic runner hard-coded the *producer* fixture's - `VTERMROW` breadcrumb, so the CAT input fixture could satisfy every - assertion yet never satisfy the loop exit — it waited out the - 20-second safety deadline and passed on the deadline. Producer probes - now name their own required frame text while input probes finish on - the latched echo, and the report exposes `completion_observed` which - **both** paths assert, so a deadline-driven pass cannot hide a stall - again. -- **Final verification on the reviewed head:** `cargo fmt --check`; - strict workspace Clippy; library **1,849 passed + 3 ignored** default - and **2,034 passed + 4 ignored** CRDT; bottom-panel Stage 1 / 2A / - 2B-1 **46 / 17 / 16**; folding Stage 2 **48**; GPU font **11**; - statusline **8 CRDT**; m11_5 semantic **2 CRDT**; GPU initial target - and invocation **15 / 15 CRDT**; the handshake consumers m5_5 / m5_7 / - mode-system wiring **36 / 7 / 1 CRDT**; Vterm Stages 1 / 2 / 3 - **10 / 6 / 9 CRDT**, including the required real daemon + real PTY + - real wgpu probe; M4 **121 passed + 3 ignored + 1 filtered**; required - GPU **202/202**; the isolated-config one-invocation full workspace - sweep; and `git diff --check`. - - Retained as classification rather than erased: the first - required-GPU pass was **201/202** on - `a_fraction_draws_rule_pixels_between_its_operand_rows`, a rendering - test structurally outside a protocol-only diff, which passed - immediately in single-threaded isolation and **202/202** on the - mandatory complete rerun. Separately, library and Vterm attempts - *inside the restricted tool sandbox* produced `Operation not - permitted` failures in socket-based attach tests; the authoritative - outside-sandbox reruns passed. -- **Ordering for the rest of the arc is fixed:** 2B-2 branches from - landed `main`; 2B-3 branches only after 2B-2 lands; Stage 3 (the - adopter default flip) last. Each slice starts fresh from landed main. - -- **Stage 2A MERGED as #177** (`main` @ `0a3fcd1`, 2026-07-26, all twelve - checks green at `8424172`, three review rounds). Branch - `githubsucks/bottom-panel-stage2a` and worktree `../pmacs-bp-stage2a` - are retained and carry nothing unmerged. Five commits: the classified - census routing, the painter extraction + acceptance, the lane record, - then the round-1, round-2 and round-3 review fixes. **No protocol - change; no behavior change for any frontend today** — with - `panel_capable = false` for semantic sessions, - `primary_document_window` returns `view.active` in every existing - configuration, so this is seam adoption that becomes load-bearing in - 2B. -- **Stage 2A verification on its merge result:** `cargo fmt --check` clean; strict - workspace Clippy clean; **1,832 default + 2,015 CRDT** library tests; - `bottom_panel_stage2a_acceptance` **17**; bottom-panel Stage 1 46; - statusline segments 8 CRDT; m11_5 semantic 2 CRDT; GPU initial target - 14 CRDT; terminal config 12 CRDT; vterm Stage 1/2 10 / 6; folding - Stage 2 48; M4 121; required GPU 202; `git diff --check` clean. -- **Every routed producer is now pinned at a seam its production caller - uses, and each pin was falsified by revert**: #1 follow, #2 lazy CRDT - upgrade, #3 `CursorByte`, #5 decorations, #7 `Viewport` (aligns - without focusing), #8 `Pointer` (aligns and focuses), #9 the - terminal-context gate, #12 statusline, #21 the publication filter, - plus the focus-class negatives. #1/#3/#21 required extracting three - named helpers, because their only production caller is - `dispatcher_loop`, which no test can drive. -- **Three lessons about the TESTS, not the code, all from review:** - (a) a *structural* test comparing the two authorities directly does - **not** catch a misrouted consumer — only consumer-level assertions - do; (b) a daemon-path test must `register_session` or the event is - dropped at the uninstalled-session check before reaching the code - under test; (c) a discriminating fixture must make the two routings - DISAGREE — comparing two non-terminal buffers, or two windows with no - selection, yields the same answer either way and proves nothing. - Round 2 found four of my own pins vacuous by exactly these shapes, and - round 3 found two more problems of the same family: a pin placed at a - HELPER while production called it from a producer (reverting only the - producer's call site left every test green), and a socket-pair - assertion whose blocking read made a regression HANG instead of fail. - Both now assert at the producer, with read timeouts on every read. -- **Review round 1 closed: 4 P1 + 2 P2, all real.** The P1s were a - stale-`Pointer` focus steal (the failed-alignment arm returned the - window, so #8's activation focused it before `dispatch_pointer` - rejected the buffer), the missing A2A-2 two-context fan-out, a census - suite that asserted the AUTHORITY rather than the CONSUMERS, and the - missing main integration. **Two of the new pins were themselves - vacuous on the first attempt** — the dispatcher test passed because an - unregistered session is dropped at `daemon.rs:1962` before reaching - the aligner, and the painter test was a fixed-point check that - survived deleting `text_view.render`. Both now fail under their own - bite. -- **`vterm_stage3_acceptance::a37` is a pre-existing flake here**, not a - Stage 2A regression: measured **6/8 failures on the base commit** and - **7/8 on the branch** in matched isolated samples. It needs a real - daemon + real PTY + headless GPU and is documented load-sensitive. - It also silently returns `ok` unless `pmacs-gpu` has been built, and - is `crdt`-gated so CI never runs it at all. -- **Two suites are dark without `--features crdt`**: - `m11_5_semantic_acceptance` reports **0 tests** and - `gpu_initial_target_acceptance` reports **1** in the default config. - Both are semantic-census suites, so Stage 2A must be gated with the - feature on or its most relevant coverage never executes. - -- Stage 1 merged as **#155** (`main` @ `e745068`, 2026-07-24, after two - review rounds). No protocol change. Durable substrate facts live in - `docs/agent-handoff.md` §1; the two round lessons are in §5. -- Landed-docs follow-up merged as **#156** (`main` @ `d152120`, - 2026-07-25). -- **Stage 2 framing: `docs/bottom-panel-stage2-framing.md` revision 6** - is on branch `githubsucks/bottom-panel-stage2b` (revision 5 is commit - `56301ed` there), - worktree `../pmacs-bp-stage2b`. Revisions 1–4 remain on - `githubsucks/bottom-panel-stage2-framing` (head `4fbd47f`, four - framing commits, revision 4 at `49757e5`). Round 1 closed 2 blocking + - 3 high; - round 2 closed 1 blocking + 2 high + 1 medium and decided both open - items; round 3 closed 1 blocking + 1 high + 1 medium. No open items - remain. Revision 5 adds no decision; it records the approved - 2B-1/2B-2/2B-3 implementation split. Revision 6 corrects the - server-first compatibility contract, durable protocol claims, exact - acceptance-suite names, and `limit + 1` fixture. The - parent framing `docs/bottom-panel-framing.md` (rev 4) remains - authoritative, **including its acceptance criteria 37–55**. -- Retained, carrying nothing unmerged: branch `bottom-panel` and worktree - `../pmacs-bottom-panel`. -- **Stage 2 ships as four serial implementation slices**, each landing - before the next branches: - **2A** = classified §1.3 census routing + `paint_frame` per-window - painter extraction (with the active-window auto-scroll preparation), no - protocol change; **2B-1** = reserved protocol schema **v21**, with - production advertisement held at v20, - (`InstanceMessage::PanelFrame` plus - `FrontendEvent::{FrontendCellGeometry, PanelResizeRows, PanelPointer}`, - gated both directions, each extended enum byte-pinned on its own - previous final variant); **2B-2** = daemon panel projection and epoch - machine; **2B-3** = compatible v21 activation, the GPU band, and the - negotiated `panel_capable` flip. - Stage 3 is the adopter default flip. -- **Correction — this entry previously mis-stated the census contract.** - It is **not** "route every consumer through `primary_document_window`". - Q#BP14 classifies the 23 reads into four classes and routes only the - **Projection** class that way; focus/input (#13–#15, #23), focus chrome - and surface-routed (#16–#19), and focus/session (#20) keep their own - authorities. Rerouting them would break remote-op validation and - application, `DispatchIdle`, presence, focused search/menu/completion - routing, and terminal bell ownership. The Stage 2 framing carries the - full table. -- **The GPU document bottom is three boundaries, not one.** - `text_area_bottom` (`pmacs-gpu/src/main.rs:8490`) is today - `status_band_top`, `geometry_capacity_bottom`, and - `document_text_bottom` at once. Once a band is installed they diverge: - the status chrome must stay pixel-identical at the physical window - bottom while document consumers move. A blanket rewrite of that helper - moves both together and passes an "everything moved" assertion, so the - Stage 2 criterion asserts **both directions in one scenario**. The - census is 20 production sites (8 status-owned, 12 document-owned) + 1 - definition + 8 test sites = 29 matches; the framing carries the - per-site table. The three easiest to misclassify are document - completion `:6140`, minibuffer candidates `:7351`, and edge scrolling - `:8561` — each with its own visible symptom. -- **Folding Stage 3 and this arc's Stage 2 both touch the semantic - projection.** Whichever is framed second re-scouts the other's landed - state. +- **Stage 3 — the adopter default flip — is the arc's last step and is + not started.** This lane stays until it lands; it is not removed at + 2B-3's merge. +- **DAP waits for Stage 2, not Stage 1** — that dependency is now + satisfied. ## Test ambient-root isolation — FRAMING OPEN, revision 4 @@ -809,787 +353,6 @@ has **no branch and no framing yet**. `FrontendView.fold_projection` to `true` for semantic frontends, which Stage 2 deliberately left `false` (Q#FD21). -## Resource-op delete guard implementation — PR #190 OPEN, review round 2 closed - -- Portable branch: `githubsucks/resource-op-delete-guard-impl`, worktree - `../pmacs-rd-impl`. Implements the framing merged as #186 - (`docs/resource-op-delete-guard-framing.md`, revision 5 plus its new - §§9-10). Position against `main`, as pasted command output rather than a - remembered constant — **`main` moved while this lane was being - written**: - - ``` - $ git merge-base HEAD githubsucks/main - b7bf2c664475c36b60cf7c0361ea75cd3c3b2315 - ``` - - That is the whole durable fact, and it is deliberately the ONLY - number pasted here. **An ahead-count cannot be recorded in the file - it counts**: writing it is a commit, so the value is stale by one the - instant it is written, and the previous attempt at this entry proved - it — a pasted `4 0` read `5 0` at the pushed head. Run - `git rev-list --left-right --count HEAD...githubsucks/main` when you - need it; the merge-base above is what tells you whether the answer is - still meaningful. - - **Re-measure the merge-base too before relying on it.** `main` moved - twice while this lane's round-1 fixes were being written (#192 and - #193), then twice more during round 2 (#188 and #194). This branch - integrates through #194. `main` has branch protection now, so a stale - base is not merely untidy: all 12 checks must pass on the merging head, - and a conflicting PR builds no merge ref at all, so a green run from - before the move reads as current when it is not. -- **The framing's §8 branch plan is superseded and cannot be followed.** - It says "one PR — #186, which becomes the implementation PR", written - when #186 was still open. #186 merged as framing-only, so the - implementation got its own branch and PR. Nothing about the decisions - changes; only the branch plan. Both the framing's header and its §8 now - say so on their own pages. -- **Layer 1 (the primitive) and Layer 2 (the applier + server-request - boundary) are both complete and both pinned through their production - paths.** The Layer 2 gap the first commit named — criteria 11, 11a-11d, - 12, 13, 15 having no production-path pin — is closed. -- **Review round 1 found four defects; all four are fixed and all four - are recorded in the framing's new §9**, because two of them were - corrections *to that document*, and a correction living only in a test - comment is invisible to the next reader of the framing: - - **P1 §9.3 — the preflight broke ordered resource ops.** Every delete - was judged against the filesystem's *initial* state, so a valid - `create X -> delete X` (or `rename A -> B -> delete B`) was refused - with a fabricated `NotFound` about a path the batch was about to - create. A regression this lane introduced. **Decision: defer, do not - simulate** — a delete whose target is related by component-aware path - containment to a path an *earlier* op creates, renames, or removes is - left to the primitive. Q#RD3 already calls the check a filter, not a - transaction. `edit` ops are deliberately excluded, so the - buffer-and-filesystem half still fires early for untouched targets - (criterion 11c depends on exactly that). - - **P1 §9.5 — the required production-boundary acceptances were - missing.** Landed: 11, 11a-11d, 12 (both directions), 13, 15. - - **P1 §9.4 — mid-batch failures were misreported as complete aborts.** - `apply_workspace_edit` now returns `nil, message, applied_op_count`, - and ONE renderer serves both the status line and the server's - `failureReason`. All three callers updated. - - **P2 §9.2 — non-recursive deletes inspected descendants.** `recursive` - is now a parameter of the shared query. The counterexample is an - orphan: a modified buffer at `tree/gone.rs` whose file is already gone - blocked a non-recursive delete of the now-*empty* `tree/`. -- **Review round 2 found two more defects; both are fixed and recorded in - the framing's new §10:** - - **P1 §10.1 — dependency filtering compared raw path spellings.** - `create dir/./x -> delete dir/x` was wrongly preflight-refused even - though both operations name the same lexical path. The comparison - now runs both sides through the registry's existing lexical - `pmacs.path.canonicalize` normalizer before component-aware - containment. This is deliberately comparison-only: operation - execution still receives the server's original path, and no - filesystem/symlink canonicalization was added. - - **P1 §10.2 — a failing first plan item could mutate while reporting - “nothing was mutated.”** `apply_workspace_edit` now returns an - `execution_started` fact in addition to the completed-item count. - Only parse/plan/preflight failures claim that nothing changed. Once - execution starts, the shared renderer conservatively says the - failing operation may have changed state. Criteria 22a and 22b pin - both forms: a multi-edit text item whose first edit lands before its - second edit fails, and a resource rename that creates destination - parents before the filesystem rename fails. -- **`delete_verdict` is narrowed, and #171 inherits the narrowed - version.** Q#RD6's shared query is this lane's to own; descendant - matching is now reserved for recursive deletes. Q#RD5's "inspect widely, - mutate narrowly" is unchanged in substance — "widely" means the set the - op can actually destroy. -- **Criterion 3's stated bite: fixed by fixing the SETUP, not the doc.** - The framing says it fails against buffer-first ordering. Against the - first shipped setup it did not (a directory target with no buffer bound - to it), and §9.2's narrowing would then have left that setup with no - bite at all. The buffer is now bound to the *exact* deleted path — a - file opened, then replaced on disk by a non-empty directory, so a - non-recursive `remove_dir` fails with `ENOTEMPTY` deterministically and - under any uid. Both stated pre-images now bite, so the framing's wording - needed no amendment after all. -- **The fake is one parameterized mode, not eight.** - `PMACS_FAKE_LSP_MODE=applyeditplan` reads its whole `WorkspaceEdit` from - `PMACS_FAKE_LSP_EDIT_PLAN` and publishes the client's response to - `PMACS_FAKE_LSP_APPLYEDIT_SINK` (written `.part`-then-rename, so a - polling reader never sees a partial record). Fail-closed: an unreadable - plan sends no `applyEdit` and reports itself through the sink. - `pmacs_fake_lsp` is a cargo BIN resolved through - `env!("CARGO_BIN_EXE_...")`, so every CI leg builds it and a missing - binary is a build failure — there is deliberately no - skip-and-return-ok arm. -- **Criterion 15's stub is hosted in `m4_acceptance`, and the gate list - moved with it.** `lsp_dispatch_seams_acceptance` is struck from the - framing's §7 gate list AND its §8 touch table in the same edit, under - §8's permitted simplification. It is still *run* as a gate, because - `builtin/runtime/lsp.lua` changed. -- Acceptance: criteria 1-16, §9's 18, 19a-19c and 20, plus §10's 21 and - 22a-22b, all in `tests/m4_acceptance.rs` and prefixed `rd`. 28 tests. -- Bite verification uses `scripts/bite` **with the positive control** it - gained in #192, merged into this lane. The pre-image for the round-1 - fixes is this lane's own first commit `1873be6`, not `main` — those - defects were introduced by it. Per-criterion results are in the commit - message. The round-2 criteria 21, 22a, and 22b each pass on the - round-2 code checkpoint `cb7fe81` and produce a clean assertion - failure against its pre-image `c804dd5`. -- Gates green at the round-2 tree: fmt; clippy `-D warnings`; `--lib` - **1863**; `--lib --features crdt` **2048**; `m4_acceptance` **149** - passed, **3** ignored, **1** filtered; `lsp_dispatch_seams_acceptance` - **15**; `dired_acceptance` - **25** and `autosave_acceptance` **29** (the framing's watch items); - required GPU **202**; full isolated-config workspace sweep; `git diff - --check` clean. The only warning in the non-Clippy CRDT build is the - pre-existing `unused_mut` in `src/daemon.rs`; strict Clippy is clean. -- Recovery from a clean checkout: - `git fetch githubsucks && git worktree add ../pmacs-rd-impl - -b resource-op-delete-guard-impl githubsucks/resource-op-delete-guard-impl`. - -## dired Stage 2a — rename/delete reconciliation — PR #196 OPEN, review round 1 closed - -- Portable branch: `githubsucks/dired-stage2-impl`, worktree - `../pmacs-dired-s2`. Implements **Stage 2a only** of the framing merged - as #171 (`docs/dired-stage2-framing.md` rev 9, §5/§6/§10 — the - substrate transaction, no dired surface). Position against `main`, as - pasted command output rather than a remembered constant: - - ``` - $ git merge-base HEAD githubsucks/main - e003b81cdd577140fc77330bd4578d3090696877 - ``` - - That base is the #190 merge, and #190 matters here specifically: - Stage 2a **adopts** its `delete_verdict` refusal rather than - reinventing one, and lifts its walk query out into - `editor_core::buffers_bound_under` so the guard and both - reconciliation seams cannot disagree about which buffers an operation - touches. **Re-measure the merge-base before relying on it** — - `main` has branch protection, all 12 checks must pass on the merging - head, and a conflicting PR builds no merge ref at all, so a green run - from before a move reads as current when it is not. **Re-measured after - round 1: `main` had not moved, so no integration was needed** — that is - a reading of the tree, not a standing fact. -- **What 2b and 2c still owe, stated so the split boundary is auditable.** - 2a ships **no user-visible surface at all** and no dired code: the - `dired_acceptance` count is deliberately unchanged at **25**, and a - moved count there would mean it touched something it should not have. - 2b owes the mark and operation layer (`m u U t d x D R w M`), - `pmacs.minibuffer.confirm` plus its `src/editor.rs` load-sequence line, - `pmacs.killring.push`, dired's own `resource.renamed` subscriber, and - acceptance 1–22, 33, 39–41. 2c owes `mkdir`/`copy`/`remove_dir_all`, - `JobKind` 12 → 15, `dired.recursive-deletes`, and acceptance 42–47. -- **The split boundary has not moved since rev 9.** It was re-checked - against this tree: #188 (generated-buffer immutability Stage 1) did not - convert dired's `paint`, so §3.1's coordination note is still an - obligation of that lane rather than a collision with this one, and - nothing in this diff touches `builtin/runtime/dired.lua`. -- **Two m4 rows were re-pinned, and that is a behaviour change to a - landed lane's assertions.** `rd9` and `rd14` pinned #190's deliberate - restraint on the `apply_resource_op` delete arm — descendants stay - orphaned, only the first of two duplicate path-bound buffers is - reconciled — and both doc comments gave the same reason: widening - would have routed N buffers through `remove_buffer_and_fire`, phase 2 - without phase 1, leaving up to N windows on removed ids. - `EditorCore::reconcile_delete` composes both phases, so the constraint - is discharged and the old assertions became the defect. Each row now - asserts BOTH directions — reconciled away **and** no window holding a - removed id — and each direction is bite-verified. -- **One framing claim is wrong and is corrected at the test, not - silently worked around.** §5's G1 says a stale captured path - "materializes a phantom" by reaching `resolve_target_buffer`'s - `NotFound` arm. It does not: `pmacs.buffer.find_or_open` calls - `crate::file_io::load_file` directly and maps the error, so a missing - path **raises**, and the `NotFound` arm belongs to - `resolve_target_buffer`, which serves `pmacs.window.display_file` and - the startup/daemon target rather than that binding. The defect is real - and smaller: the `pcall` swallows the raise, so the user is stranded - wherever the last applied op left them. Acceptance 34 is restructured - to bite on that (its plan edits another file first, which is what makes - the restore observable at all) and the correction is recorded in the - test's own doc comment. -- **Two bites were vacuous as the framing specified them, and both - reasons are worth keeping.** Item 28's *rename* row cannot pin the - walk's containment rule: `reconcile_rename` calls - `Path::strip_prefix` to rebuild a descendant's tail, and that is - component-aware too, so a string-prefix walk is silently corrected a - second time. The row moved to the **delete** side, where the walk's - verdict IS the kill list. Item 30's composition-order assertion was a - tautology: the LSP attach leaves `diagnostic` **last** in the stack, and - moving the last element to the end is a no-op, so a remove-and-re-push - was indistinguishable from an in-place mutation; the row now pushes one - more overlay after it and asserts that precondition explicitly. -- **23 acceptance criteria are bite-verified by executed mutation**, each - labelled `OK (assertion)` — none merely `OK (COMPILE)`, and none - vacuous. Items 25, 27, 28, 29 (both directions), 30 (both mutations), - 31, 31b (both gates), 31d (both halves), 34, 50 (both mutations), 51, - 52, 53b, 54, 55, plus the two re-pinned m4 rows in three - configurations. -- **Review round 1 found four defects; all four are fixed, and all four - were the same shape — a failure that left state wrong and told nobody.** - Worth keeping as one lesson rather than four bugs: every one of them - was a `pcall` or a discarded return value, and each *looked* like - defensive coding. - - **P1 — delete refusals were silent.** `reconcile_delete_and_fire` - returned `kept_modified` and `refused` and both production callers - discarded them, so a last-buffer refusal or the asynchronous - modified-buffer race left the file gone and the buffer still bound to - it — and the next `C-x C-s` recreates the deleted file. Reporting - moved **inside the shared seam**, for the same reason the - reconciliation lives there: a caller that has to remember to report - is a caller that will forget. Channel is `EditorCore::status`; - **not `pmacs.error`**, which is defined only by a test stub, so a - report there would have been the same silence. - - **P2 — the LSP subscribers swallowed their own reconciliation - failures.** Ignored `pcall`s made the callback return successfully, - so the `all-must-succeed` logger had nothing to log. A shared - failure sink now attributes each step and raises **after** the loop, - because a fix that aborts on the first failure would leave every - other attachment unreconciled — that wrong fix is itself a - bite-verified mutation. - - **P2 — `forget_uri` left purged requests live in the client.** It - dropped `pending_routes` and `pending_external` but not the ids - `send_request` puts in `LspClient.pending`, and recorded nothing in - `cancelled_rids`, so a server that never replies leaked the entry and - a late reply surfaced as a generic unrouted response. The per-rid - work is now extracted from `drain_cancelled_externals` as - `abandon_request` and **reused** rather than copied. - - **P2 — acceptance 35 was unpinned even after the G1 correction.** - With a plain delete the forbidden path fallback is unobservable: - `find_or_open` raises out of `load_file` and the `pcall` swallows it, - so both assertions passed with the fallback present. The plan now - deletes the origin's file **and recreates it**, which gives the - fallback something to open. The corrected G1 explanation also reached - the production comments, which still repeated the false - `resolve_target_buffer::NotFound` story — *a correction that stops at - the test comment has only half landed.* -- **One round-1 pin passed with its own bug restored, and the reason is - reusable.** Acceptance 53 asserted `contains("only.txt")` for the - buffer-name attribution — but the status line opens with - `deleted only.txt:`, the deleted path's **basename**, so stripping the - attribution changed nothing the assertion could see. Both halves now - assert the buffer's *own* name, which for a path-backed buffer is the - full path and which only the attribution can produce. **A pin written - to close a review finding is exactly the kind that passes with the bug - restored**, and the detector was running the bite rather than reading - the assertion. -- **31 bites now, all executed, every one labelled `OK (assertion)`** — - the original 23 plus 8 for round 1 (report call removed; refusal reason - unattributed; kept-modified name dropped; subscriber failures - swallowed; the wrong fix that aborts the loop; `forget_uri` skipping - `abandon_request`; and the forbidden path fallback restored, which must - fail acceptance 34 **and** 35 independently). -- Verification at this head, each gate run to its own file and its own - exit code checked (never through a pipe): `cargo fmt --check` clean; - `cargo clippy --workspace --all-targets -- -D warnings` clean; - `cargo test --lib` **1,876** passed / 3 ignored; `--lib --features - crdt` **2,061** / 4 ignored; the new - `resource_reconciliation_acceptance` **25** default and **25** crdt; - `dired_acceptance` **25** and **25** crdt, deliberately unmoved; the - frozen additivity gate `m8_1` **10** / `m8_2` **15** / `m8_3` **32**, - all unchanged; `m4_acceptance -- --skip basedpyright` **149** passed / - 3 ignored / 1 filtered; `lsp_multi_root_acceptance` **13**; - `lsp_dispatch_seams_acceptance` **15**; - `typed_edit_chain_acceptance` **13**; `journey_acceptance` **24** - (the ratchet floor, asserted as a count rather than a colour); - `gpu_invocation_acceptance` **15** crdt — **and that number is only - real with `pmacs` and `pmacs-gpu` built first**, which is the `a37` - trap in §5: the same command reported 12 failures before the build and - 15 passes after, so a red run there is not evidence of a regression - until the binaries exist; `PMACS_REQUIRE_GPU=1 cargo test -p - pmacs-gpu` **202**; isolated-`XDG_CONFIG_HOME` workspace sweep with - `--no-fail-fast` **3,559** passed across **104** suites, 19 ignored, 0 - failed; `git diff --check` clean. Every one of those was run as its own - step with its own exit status checked — never `cmd | tail` inside an - `&&` chain, which returns *tail's* status and has masked a real failure - in this repo before. -- **Ownership, per the framing's own warning.** §16 says 2a must not run - concurrently with **Journey Stage 1b**, because 1b's LSP - spawn-failure reporting lands in `builtin/runtime/lsp.lua`'s - attachment lifecycle and 1b's compile/binding half touches - `src/editor_core.rs` — the same two files 2a rewrites, where the - conflicts are semantic rather than textual so a clean `git merge` - proves nothing. **1b must not be started while this PR is open.** No - other lane in flight touches them: #188 is `dired.lua`/`buffer.rs` - generated-buffer writes, and the bottom-panel and CI lanes are - elsewhere. -- Recovery from a clean checkout: - `git fetch githubsucks && git worktree add ../pmacs-dired-s2 - -b dired-stage2-impl githubsucks/dired-stage2-impl`. - -## Generated-buffer immutability framing lane — MERGED AS PR #188 - -- Portable branch: `githubsucks/generated-buffer-immutability`; worktree - `../pmacs-generated-immutability`. **PR #188 landed on `main` as - `27b1185` on 2026-07-29**, after forking from - `githubsucks/main` @ `ad41cf1` and integrating through `5e186c7` — - #189 (clean), then #186 and #171 (`docs/active-work.md` conflict), - then #187 (the same file again, after it removed the two landed - framing lanes), #192 at merge commit `76cfaac`, and #193 - (`docs/active-work.md` conflict again) after revision 7's first push. - Revision 6 was reviewed at head `55c3061`; revision 7 closes that - round. The retained branch is provenance only. Framing only — - `docs/generated-buffer-immutability-framing.md`, revision 7, plus this - lane. **No runtime code, no protocol change.** -- **APPROVED and merged after six review rounds** (thirty-two findings, - twenty-two P1, ten P2). Revision 7 is the governing contract. -- **Stage 1 implementation is PR #191, open. The boundary is explicit - and has already been needed twice:** #188 owns the **acceptance - contract**; #191 **adopts** criteria and may not restate, narrow, or - reclassify them. Where an implementation finds a criterion impossible, - the framing is revised and re-approved first. The **selection-anchor - clamp** is Q#GB6's to specify and #191's to implement, and both must - describe the same rule. Round 5 found #191 had locally restated Stage 1 - criteria 5 and 7 while #188 still carried the originals — a divergence - neither lane's gates can catch, because each is green against its own - description. -- **Round 5's two corrections that other lanes need:** - - **`Window::Selection::anchor` is an unclamped byte position and it - PANICS**, not merely dangles. `Window::region` (`src/window.rs:472-479`) - clamps neither endpoint; `region_bytes` (`src/editor_core.rs:4184-4191`) - hands the result to `Rope::slice`, which asserts at `src/rope.rs:145`. - Reproduced by #191: select 0..30, shrink the buffer to two bytes, - copy. The fix is **clamp-or-clear** in **both** `notify_buffer_edit` - (`src/editor_core.rs:1836-1850`, clamps nothing today) and - `rebuild_views_for` (`:1865-1882`, clamps cursor and view_top but not - selection) — and the rule is already in the tree for the terminal's - own selection type at `src/terminal/view.rs:715-721`. A stale anchor - also reaches the presence broadcast (`src/presence.rs:122-123`). - - **`Buffer`'s `on_edit` broadcast stops at the first error in FOUR - places**, so a view later in attach order keeps pre-edit offsets: - `src/buffer.rs:1288` (`apply_edit` / `apply_edit_skip_intercepts`), - `:1250` (the no-op early-return arm), `:1033` - (`apply_remote_crdt_op` — the replica import path), and `:1543` - (`broadcast_on_edit`, i.e. **undo and redo**). Any lane relying on a - buffer-attached view staying in step with the rope is affected. -- **What it frames.** The class-wide half of the `set_generated_contents` - invariant that `docs/agent-handoff.md` §4 and `COHERENCE.md` §14 both - record as unfinished: `Buffer::undo` gates on `ensure_writable()` - (`src/buffer.rs:1302`) and never consults the intercept chain, so the - `add_intercept`-plus-`bypass_intercept` idiom leaves the rope writable - and every affected buffer emptiable. All five families were reproduced - by execution at `ad41cf1`, not inferred; the transcripts are in the - document's §0 and §2. -- **Recommended primitive:** `Buffer::apply_generated_edit(op)`, exposed - as a `{ generated = true }` option on the existing Lua mutators, with - `set_generated_contents` reimplemented as its whole-buffer wrapper. It - is the only candidate in which the buffer is never observably unlocked. - **Revision 3 pins the transaction** (framing §3.4): its **own** - `run_buffer_edit` arm — **not** the bypass arm, which calls - `begin_edit`, which calls `ensure_writable` first (`src/buffer.rs:725`) - and would refuse every generated write to a locked buffer — one - `&mut Buffer` method with every exit named. - **Revision 4 replaces revision 3's cleanup predicate.** Cleanup is - driven by an explicit five-variant `GeneratedOutcome` reported by the - apply, **not** inferred from `revision`. Inferring it was wrong three - ways: a successful no-op (`src/buffer.rs:1245-1253` returns `Ok` - without bumping `revision`) kept history the contract forbids; a CRDT - mid-transaction failure happens **upstream of `revision` entirely** - (`:1140-1163`), so it was neither cleaned nor detected; and the - unconditional relock **locked a fresh buffer that was never - successfully written**. `NoOp` clears, `Rejected` restores the entry - lock state, `Diverged` clears nothing and surfaces. **Revision 5 keeps - the five outcomes but preserves the `Edit` in - `AppliedThenFailed { edit, error }`: the borrow-free Lua finisher fans - it out to window caches and replica mirrors before returning the - error.** Collapsing to `Result` inside `Buffer` was too early. - **Revision 6 replaces the delete→insert enumeration with a - `crdt_mutated` flag**, because `export_updates_since` can fail after - every successful CRDT op shape, and changes all four `Buffer` - broadcast loops to continue-and-retain-first-error. - **Revision 7 installs quarantine at common divergence detection - before generated or ordinary callers map the outcome.** The monotonic - poison blocks all three CRDT snapshot exporters, daemon-origin - queueing and every later owner-generated write; criteria 16c and 16d - pin the containment boundary. Repair remains deferred. -- **Two stages, two PRs.** Stage 1 — listview ownership fix **plus its - identity-routing fix in the same PR**, dired and listview adopting the - shipped primitive, the window-coordinate clamp, and the fold decision. - Stage 2 — the new primitive, compile's nine write sites, the search - panel's four, compile/search ownership + routing, the path-backed - refusal plus `mark_clean`, and the terminal-only - `identity_protected` guard. **No Lua unlock ships.** -- **Nine facts this lane landed for other lanes:** - - **`bypass_intercept` is the wrong inventory key.** It misses - `*buffer-list*`, `*help*` and `*workers*`, which are generated with - plain writes and no intercept at all. `docs/agent-handoff.md` §4's - four-row table inherits that blind spot — **and undercounts by one**: - `src/help.rs:354` `replace_help_buffer` is a fifth writer mechanism - (own find-or-create, `Buffer::apply_edit`, own `mark_clean`) writing - the **same** `*help*` buffer as `default.lua:1239`, which does not - mark clean. Two owners, one buffer, two copies of the name constant - across the FFI boundary. - - **`COHERENCE.md` §14's listview consumer list was wrong and is now - FIXED** — PR #189 (`main` @ `7586905`) landed exactly the correction - this lane measured. Nothing owed. Recorded so it is not re-asserted. - - **Three writers adopt any buffer sharing their name** — - `listview.lua:95`, `compile.lua:263`, `default.lua:861-868` — against - a rule the tree already states at `terminal.lua:300-305` and - implements at `dired.lua:476-504`. Measured: a foreign - `*references*` is clobbered and left permanently un-editable, and a - `pmacs.compile.run` that **raises on validation** still leaves a - foreign `*compilation*` un-editable. Today `M-x buffer.undo` — this - arc's bug — is the only recovery, so the arc must not lock these - buffers before fixing ownership. - - **Disambiguating a name breaks the sites that read one.** Census in - framing §2.10: 19 units across 14 grep lines, two genuinely broken. - `listview.lua:44`'s `panels[d.name]` (written under the *requested* - name at `:97`, read under the *actual* name) has **four** consumers, - and the fourth — `listview.open:118-123`'s never-capture-a-panel - guard — fails **inverted and silently**, capturing a panel as its own - `q` target. `compile.lua:216`'s `is_generated_buffer` has two. - `compile.lua`'s `slots` is **not** affected: keyed by a module - constant at both ends, with `slot_for_buffer` id-based. - - **`read_only` is one boolean serving THREE policies** (framing - §2.11): the generated lock; terminal identity - (`src/terminal/session.rs:305`); and, as a *reader*, - `src/lua_bindings/fold.rs:313`'s "is this a document buffer" test, - pinned by `tests/folding_acceptance.rs:570`. Consequence for any - lane: **locking a buffer silently disables `pmacs.fold.fold` on it**, - with the status `fold rejected: not a document buffer`. - - **The SHIPPED `set_generated_contents` can overwrite a live terminal - identity buffer.** It does `self.read_only = false` unconditionally - (`src/buffer.rs:546`), so it lifts a lock it did not install, writes, - and re-locks. Present on `main`, untested, unframed anywhere before - revision 4. Refused in Stage 2 by the `identity_protected` field — - an **intrinsic** flag marked once by a crate-private monotonic - `mark_identity_protected()` in `TerminalSession::open`, never written - by `set_read_only`. Revision 3 tried to infer this from the lock's - provenance instead; that broke the lift-and-restore idiom at - `tests/terminal_copy_mode_acceptance.rs:578-584`, and the general - lesson is that a **derived** fact must be maintained by every - mutation of what it derives from — and `set_read_only` is `pub`. - - **`acc16e` is `crdt`-gated and is the only shipped consumer of the - lift-and-restore idiom.** `cargo test --test - terminal_copy_mode_acceptance` **without** `--features crdt` never - compiles it, so a green run of that suite proves nothing about the - seam. Any lane touching `read_only` semantics must run it with the - feature and confirm `acc16e` is in the count. - - **`identity_protected` is not generated-lock provenance.** Revision - 4 tried to use “not a terminal identity buffer” as proof that the - generated primitive installed the lock; it is not. Revision 5 - therefore removes `pmacs.buffer.unlock_generated` from the arc - entirely. Wdired's future generated→editable transition remains - dired Stage 3 work and must be owner-specific or use the eventual - lock-policy enum. - - **The CRDT `Replace` mid-transaction divergence is real and - unowned.** `crdt.delete` then `crdt.insert` (`src/buffer.rs:1140-1163`); - if the first succeeds and the second fails, the code's own comment - says "the CRDT is mid-transaction ... This is an invariant - violation." It reaches `apply_edit` and `apply_edit_skip_intercepts` - today and is reported as an ordinary `CrdtRejected`, so nothing - distinguishes it. This lane names and contains it; **repair is - deferred and unowned.** Revision 6 makes classification total with a - `crdt_mutated` flag: any later error, including - `export_updates_since` after `Insert`, `Delete` or `Replace`, is - `Diverged`. Revision 7 installs a common monotonic poison before - either generated or ordinary API mapping, blocks - `initial_target_snapshot`, `send_buffer_snapshots`, - `export_buffer_snapshot` and `queue_daemon_origin_crdt_op`, and - rejects every later owner-generated write with `CrdtQuarantined`. - Criteria 16c and 16d fault-inject the complete boundary under - `cargo test --lib --features crdt`; there is no public - fault-injection API and no four-variant fallback. -- **Overlap warning.** Stage 2 touches `src/lua_bindings/mod.rs`'s buffer - mutator bindings and `src/buffer.rs`. Do not run it concurrently with - the `apply_resource_op` lane or the bottom-panel 2B work without - assigning those files to one lane first. The framing itself touches - neither. -- **Cross-lane, settled, not re-decided here.** #186 owns the urgent - pre-filesystem refusal for synchronous `apply_resource_op`; #171 later - owns full post-delete lifecycle reconciliation, including the async - race where a buffer becomes modified after dired dispatch. **#171's - Q#DR25 is deferred INTO this lane** — confirmed against #171 revision 7 - (`fd7ae37`), which states that dired's listing becoming immutable is - "owned by the `generated-buffer-immutability` lane" and that "Stage 2 - does not implement it, does not gate on it, and carries no acceptance - for it." This lane's Stage 1 claims that work. **Neither ordering - conflicts**: #171 Stage 2b changes `paint`'s callers, this lane changes - `paint` itself. Revisions 1 and 2 of this framing never mentioned - Q#DR25 at all; revision 3 §9b records it. -- **Re-measured at `ad41cf1` while scouting: 276 CRDT-dark tests** - (3,251 vs 3,527), by - `cargo test --all-targets --no-default-features --features lua54[,crdt] -- --list | grep -c ': test$'`. - Recorded here because the section above asks for exactly that and - warns against quoting a stale figure; it does not replace that - section's per-target census, which was not re-derived. - -## Test-improvement arc, lane 2 — silent-skip arming - -- Portable branch: `githubsucks/silent-skip-arming`, worktree - `../pmacs-skiparm`. Implements `TEST_IMPROVEMENT.md` §1.2 and §5.4. -- **Base, measured at write time rather than quoted:** - - ``` - $ git log --oneline -1 githubsucks/main - 5e186c7 Merge pull request #193 from levineuwirth/test-improvement-audit - ``` - - The previous revision of this entry said "base measured at write - time, pasted below" and then pasted nothing: the script meant to - substitute it reported success and silently matched no text, and the - claim was not re-read. Recorded because it is the same defect this - ledger keeps catching one level up — **asserting a measurement is not - making one, and a tool reporting success is not the measurement - either.** -- Recovery from a clean checkout: - `git fetch githubsucks && git worktree add ../pmacs-skiparm - -b silent-skip-arming githubsucks/silent-skip-arming`. -- **The defect:** `let Ok(_) = which_binary(x) else { eprintln!(..); - return; }` reports GREEN when the tool is absent, and CI installed - none of the tools. A block of real-language-server and multi-shell - tests had therefore **never once executed their bodies** in CI while - reporting success. A suite that cannot distinguish "passed" from - "never ran" is worse than a missing one, because it reads as - coverage. -- **The fix is the project's own pattern.** `PMACS_REQUIRE_*` already - makes a missing GPU fatal for `vterm_stage3_acceptance`; this adds - `PMACS_REQUIRE_LSP`, `PMACS_REQUIRE_SHELLS` and `PMACS_REQUIRE_LUA`, - plus the CI step that installs the tools. Per-tool variables, not one - blanket flag, so a tool that must stay unarmed keeps its decision - visible at the call site. -- **`basedpyright` is deliberately NOT installed and NOT armed.** Its - test has no timeout and hangs forever — root cause is the - non-interruptible reader-thread join in `RuntimeHandles::drop`, - already a named deferral in `src/process.rs`. The `test` job has no - `timeout-minutes` either. Arming it today would trade a vacuous green - for a six-hour hang across four legs. `PMACS_REQUIRE_PYRIGHT` exists - and is never set, so the flip is one line once lane 4 (the hang) and - lane 3 (timeouts) land. **Do not arm it before both.** -- **A trap found while writing the workflow, not after:** the natural - Actions idiom `${{ runner.os == 'Linux' && '1' || '' }}` sets the - variable to the EMPTY STRING elsewhere, and `var_os().is_some()` is - true for `Some("")`. That would have armed the guard on exactly the - runners with no tools installed. The helper therefore treats empty as - unset. `PMACS_REQUIRE_GPU` has the same latent shape and is safe only - because it is set literally. -- **Verified by execution in all three states**, on a tool genuinely - absent from this machine (`vscode-json-language-server`): unset -> - skips green; armed -> hard failure naming the CI step; empty string - -> skips green. The armed failure is the bite, and on `main` it - cannot occur because no guard exists. -- **The tests pass when they actually run** — which was the open - question, since none of them had. Armed locally: 11 `m6_5` + 8 `m6_8` - REPL tests green, and all six real-LSP tests (clangd x2, gopls x2, - rust-analyzer x2) green individually. -- **rust-analyzer is installed in the Linux-gated step, not via the - toolchain action's `components:`.** The first revision put it there, - which applies to *every* matrix leg — and **presence, not - `PMACS_REQUIRE_LSP`, is what decides whether a gated test body - runs**. That would have executed the two rust-analyzer tests on macOS - for the first time ever, on the legs that are simultaneously the CI - critical path and the documented flake surface, while this entry - claimed Linux only. The variables not being set there would only have - meant absence was tolerated; it would not have kept the tests - skipped. Text and workflow now agree. -- **Tool versions are pinned** (`gopls@v0.16.2`, - `vscode-langservers-extracted@4.10.0`, - `yaml-language-server@1.15.0`). `@latest` and bare `npm install -g` - make CI drift with upstream releases, so a bad publish breaks CI with - no commit here to bisect against. Caching the built `gopls` on the - pinned version is a follow-up, not done here. -- **§1.2 is NOT fully closed by this lane.** The guards arm the - *entry* skip only. `tests/m4_acceptance.rs`'s mid-test rust-analyzer - bail ("workspace likely still indexing; skipping") survives, so even - armed, that test's only assertion can still vanish under load — - precisely when a regression would show. Mid-test skips are their own - shape and want their own pass. -- **Not this lane's to fix, recorded so it is not mistaken for - oversight:** the generated-buffer immutability lane above still reads - "PR #188 OPEN, PROPOSED" and #188 has merged. Rule 4 forbids - relabelling it and permits removal only once its durable facts reach - `docs/agent-handoff.md`, which #188 did not touch — it changed the - framing and this ledger only. So the absorption is genuinely owed, - and the natural carrier is the arc's own next PR (#191, Stage 1), - not a testing lane reaching across into someone else's arc. -- **Follow-up owed after this merges:** delete - `githubsucks/handoff-2026-07-20`. Removing the documentation lane - removes the only pointer to that branch, so nothing will otherwise - remind anyone it still exists on the remote. -- Linux only for now, deliberately: macOS needs the brew equivalents - and roughly doubles install cost on the slowest matrix leg. The - variables stay unset there, so those tests skip cleanly. -- Also removes the **documentation lane**, whose disposition the ledger - left undecided pending confirmation that its branch carried nothing - unique. Confirmed by measurement: `githubsucks/handoff-2026-07-20` is - **1 ahead, 365 behind**, and its entire unique diff is four doc files - at 42 insertions against 88 deletions — merging it would *revert* - current documentation. The section said "whoever confirms the branch - carries nothing unique removes the section"; this is that. - -## Test-improvement arc, lane 3a — CI timeouts and concurrency - -- Portable branch: `githubsucks/ci-timeouts-concurrency`, worktree - `../pmacs-ci3`. Workflow only — **no product code, no tests changed.** -- **Base, measured at write time:** - - ``` - $ git log --oneline -1 githubsucks/main - b7bf2c6 Merge pull request #194 from levineuwirth/silent-skip-arming - ``` - -- Ships the three cheap, deterministic items of `TEST_IMPROVEMENT.md` - §5-6. The larger ones — nextest (§6.3), the serial/parallel split - (§6.2), the parallel canary leg (§5.6), the nightly cron (§5.5), and - the macOS matrix trim (§6.4) — are **deliberately not here**: each - changes what CI certifies or how it runs, and each wants its own - decision rather than riding a timeout patch. -- **`timeout-minutes` on every job (§5.2).** Measured before changing: - **7 of 8 jobs had none** and inherited GitHub's 360-minute default; - only `m6-perf-gates` had one (15). A single hung test therefore burnt - six hours, times four on the test matrix. - **This is the gate that must land before `PMACS_REQUIRE_PYRIGHT` can - ever be set** — lane 2 left basedpyright unarmed precisely because - this did not exist. -- **The ceilings are 25, and 35 for the test job — anchored on observed - execution, corrected in review.** Revision 1 cited "~14.6 min, ample - headroom", which was one reading quoted as a property. Re-measured - over two windows: **17 min** max over 25 runs and **15.8 min** over - 12, both macOS/luajit; every other job under 4 min. Against 17, a - flat 25 is ~1.5x, not "ample". - - `timeout-minutes` counts **execution, not queue** — a 33-minute - wall-clock run in that window executed its longest job in 17 — so - **no run in observed history would have been killed** by either - value. - - The real exposure is what the window does *not* contain: a **cold - cache**. A stable-toolchain bump invalidates Swatinem's key on - every leg simultaneously, and a cold macOS debug build plus suite - is the plausible way a *healthy* run overruns. It would present as - four legs timing out at once, the day after a Rust release. - - So the test job takes 35 (~2x its observed max) and the rest keep - 25 (~6x theirs), and **the diagnosis is written into the workflow - before the event**: simultaneous four-leg timeouts after a - toolchain release are a cold cache, not a hang; a single leg - timing out beside passing siblings is the hang case. -- **`concurrency` with `cancel-in-progress` (§6.1)**, scoped to pull - requests. `github.event.pull_request.number` is empty on a push to - `main`, so the fallback keys those by SHA and no `main` run can - cancel another — cancelling one would leave the branch-protection - record ambiguous about a commit that already landed. -- **`-p pmacs-protocol` clippy (§5.7).** Verified passing locally - *before* proposing it, so adding it cannot turn CI red on arrival. - The root-package clippy never covered it: the workspace default - member is only `pmacs`. -- **§5.1 branch protection is DONE, not deferred** — it belongs in - neither this lane's shipped list nor its deferrals, and review was - right that its absence from both was an omission. It was enabled - earlier in this session; verified against the API at review time: - - ``` - $ gh api repos/levineuwirth/pmacs/branches/main/protection - {"enforce_admins":false,"force_push":false,"required_checks":12,"strict":false} - ``` - - All 12 checks required; `strict` off deliberately, so a PR need not - rebase every time `main` moves (this repository's ledger contention - makes strict expensive); `enforce_admins` off so the user retains an - override. **This matters to the concurrency comment**, which - justifies exempting `main` pushes by appeal to "the - branch-protection record" — that record now exists, so the - justification is real rather than aspirational. -- **Required status checks are NAME-COUPLED to job names, and this - lane's own deferrals will break them.** A required context that no - longer exists does not fail — it leaves every PR pinned on - "Expected — waiting for status", indefinitely, which is - `main` becoming unmergeable by policy rather than by a red run. - Three deferrals above change job names or the matrix: the macOS trim - (§6.4) removes two contexts outright, and nextest (§6.3) or the - serial/parallel split (§6.2) rename or add them. - **Rule: any job rename, removal, or matrix change updates the - branch-protection required-checks list in the same motion.** Recorded - here because this is the entry that both enabled protection and named - the lanes that will invalidate it. -- Recovery from a clean checkout: - `git fetch githubsucks && git worktree add ../pmacs-ci3 - -b ci-timeouts-concurrency githubsucks/ci-timeouts-concurrency`. - -## Test-improvement arc, lane 4 — process teardown stdin deadlock - -- Portable branch: `githubsucks/process-teardown-stdin-deadlock`, - worktree `../pmacs-hang`. Implements - `docs/process-teardown-stdin-deadlock-framing.md` (rev 3: one review - round, then a CI round that falsified the reproduction). -- **Base, measured rather than quoted:** - - ``` - $ git log --oneline -1 githubsucks/main - e003b81 Merge pull request #190 from levineuwirth/resource-op-delete-guard-impl - ``` - -- Recovery from a clean checkout: - `git fetch githubsucks && git worktree add ../pmacs-hang - -b process-teardown-stdin-deadlock - githubsucks/process-teardown-stdin-deadlock`. -- **The defect:** `RuntimeHandles::drop` joined its reader threads in - the `Drop` **body**, which runs before any field drops. The - `ChildStdin` sink lives in the `stdin` **field**, so it could only be - released after the join returned — and the join waited on readers - blocked in `read()` on pipes whose write ends the child still held, - because the child never got the stdin EOF that would have made it - exit. A closed cycle inside one function; teardown hung forever. -- **This is the root cause of the `m4_5_basedpyright` hang** that has - parked `--workspace` sweeps (once for 2h26m) and forced - `-- --skip basedpyright` into every gate recipe. The handoff's §3 - claim that the desktop's binary was broken is **retired by this PR**: - the binary was fine. `basedpyright-langserver` is a uv console script - that runs bundled `node` via `subprocess.run` and **waits**; at - teardown `shutdown()` SIGTERMs the recorded pid (the wrapper), which - dies without forwarding, and **that** orphans node to `PPid: 1` - holding the pipes. A direct binary like `clangd` is a genuine child - whose pipes close on reap. That is the whole of the "intermittent" - story. -- **Corrected in review round 2:** rev 1–3 said the wrapper "spawns node - and exits". Wrong — and refutable from evidence already in hand, since - the initialize handshake succeeds, which a wrapper that exited at spawn - could not have done. The `PPid: 1` observation was taken *after* - `shutdown()` had killed the wrapper. **We create the orphan.** The fix - is unaffected; the parked follow-up changes from "tolerate - self-orphaning servers" to "stop orphaning them" (signal the group). -- **Diagnosis method, because reproduce-first was the instruction:** - gdb thread stacks plus `/proc` fd forensics on a live wedged process, - both pipe ends identified in both processes, reproduced 5/5. Three - earlier reproductions were vacuous — see the handoff §5 lesson; the - shipped test carries two positive controls because of it. -- Verification (each gate its own step, real exit status, no - `cmd | tail`): fmt 0; `git diff --check` 0; clippy 0; `--lib` 1864 - passed; `--lib --features crdt` 2049 passed; **`m4_acceptance` - without the skip 150 passed in 2.66s with the basedpyright test - `ok`**; the **eleven** PTY/REPL/worker/panel suites of the framing's - Bet 2 all 0 (144 tests); `PMACS_REQUIRE_GPU=1 -p pmacs-gpu` 202 - passed. Bite verified by revert: `ok` in 2.03s with the fix, FAILED on - timeout at 10.00s without it, both controls passing first. -- **CI round 1 falsified the reproduction, and the control is what - caught it.** Three Test legs failed on `9b1cf3d`'s predecessor: the - synthetic child used `sh -c 'cat <&0 & exit 0'`, and `<&0` does not - defeat the `/dev/null` rule it was chosen for — the rule applies - *before explicit redirections*, so fd 0 is already `/dev/null` and the - redirect duplicates it onto itself. `bash` skips the default when a - stdin redirect is present; **`dash`, which is Ubuntu's and CI's - `/bin/sh`, does not.** Local probing through `/bin/sh` could not see - it. Now `setsid --fork cat`, with no shell at all. **Lesson recorded in - the handoff §5: never probe shell behaviour through `/bin/sh` — name - the implementation.** -- **`acc28` on macos/lua54 was a flake, established not assumed.** - `bottom_panel_stage1_acceptance::acc28` failed once on that leg; - rerunning the same job on the *identical* head passed, and the suite is - 46/46 locally. It is now in Bet 2's falsifier list — its absence from - rev 1 was a real gap, since it drives real child input through a PTY in - a panel and this PR changes PTY-mode teardown ordering. -- **Not fixed here, parked in the framing §5:** cancellable non-group - `read` (covers a child that ignores EOF, and one that stops draining - while `write_all` is blocked); the orphaned-server **leak** — post-fix - the server exits by cooperation, not enforcement. -- `CLAUDE.md`'s `--skip basedpyright` entry is deliberately untouched. - Dropping it is a separate proposal owed evidence of repeated green. - The timeout precondition is **already satisfied** — #195 (this PR's - base) gave every job a `timeout-minutes` — so the only remaining reason - `PMACS_REQUIRE_PYRIGHT` stays unarmed is that CI does not install - basedpyright at all; arming it would fail rather than test anything. -- Adds `PMACS_REQUIRE_SETSID`, armed on Linux. The teardown test's - fixture needs `setsid --fork`, which is util-linux rather than - coreutils, so it **skips** when absent (the standard `--lib` gate must - not hard-fail a minimal container on an undeclared tool) and the - variable makes that skip fatal where the tool is guaranteed. Both arms - verified against a PATH with `setsid` genuinely removed: unarmed skips, - armed FAILS. README's test-dependency list declares it. - ## Parked lane: kill-ring browser + persistence - Portable branch: `githubsucks/kill-ring-browser` @@ -1614,6 +377,46 @@ git worktree add --track \ ## Closed since the last snapshot +- **Process-signal diagnostic completeness — MERGED as #200** + (`main` @ `a2a92bb`), atop Stage A #176. `docs/process-signal-diagnostic-completeness-framing.md` + revision 6; framing approved at revision 4 after three rounds, then + five review rounds on the implementation. Durable facts are in + `docs/agent-handoff.md` §1. **Evidence collection only** — no + tolerance rule, no retargeting, no disposition change. + - **Bet 1 was falsified by CI and the framing's own fallback shipped.** + `bash -m` diverges the terminal's foreground group on Linux and + never on macOS. The divergent case is pinned by injection + everywhere; a Linux-only corroboration drives a real shell and is + the **only** test exercising `pty_foreground_group` end-to-end, so + **on macOS that lookup has no end-to-end coverage**. + - **Group identity remains unprovable**, and the pre-kill sample does + not change that: moving `getpgid` before the `kill` removed a + post-hoc reading, it did not make the reading contemporaneous. + - **Still parked, each needing its own lane:** the reap ledger's + silent cancellation (an EPERM probe drops the entry; a failed + `SIGKILL` is marked killed) — **being scoped next**; retargeting to + the measured pgid; any EPERM/ESRCH tolerance rule; Q#PS6; and + `signal_target`'s read-then-kill of `tcgetpgrp` on the PTY path. + +- **Six lanes removed by the 2026-07-30 absorption pass**, all merged, + all with their durable facts in `docs/agent-handoff.md` §1: + **#190** resource-op delete-guard implementation; **#196** dired + Stage 2a (rename/delete reconciliation — Stage 2b remains, unstarted + and without a lane); **#188** generated-buffer immutability framing + (revision 7, the governing contract); **#194** silent-skip arming; + **#195** CI timeouts and concurrency; **#197** the process teardown + stdin deadlock. + - **#194 and #195 kept their lessons in §3 and §5 rather than §1**, + which is why a PR-number search of the handoff finds them only once + each. That is sufficient under rule 3 — durable knowledge has a + home, not a required section. + - **A census by PR number is a proxy, not a measurement.** Counting + `#NNN` in the handoff said five of these lanes had no record at all; + counting by *content* found most already documented, with the real + gap being the implementation PRs specifically (#190, #191, #196) + while their framings were recorded. The absorption written from the + first count would have duplicated existing entries. + - **Terminal configuration + copy mode arc — BOTH STAGES MERGED, lane removed.** Stage 1 **#173** (`main` @ `cf54270`, one review round) and Stage 2 **#178** (`main` @ `fe8b8ba`, **four review rounds**, twelve diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index e381de0..b4629f5 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -58,9 +58,18 @@ reads it the way you just did. For volatile branches, checkpoints, verification, and recovery commands, read `docs/active-work.md` immediately after this file. -## 1. Where the project stands (2026-07-28) +## 1. Where the project stands (2026-07-30) -- `main` @ `6c9e765` (the dired Stage 2 framing #171 and the resource-op +- **`main` @ `4cd4a7b`.** Eight PRs landed since the previous anchor, in + this order: the generated-buffer immutability **framing** #188, the + resource-op delete-guard **implementation** #190, silent-skip arming + #192/#193/#194, CI timeouts and concurrency #195, the process teardown + stdin-deadlock fix #197, dired Stage 2a #196, generated-buffer + immutability **Stage 1** #191, and bottom-panel **Stage 2B-3** #198. + Each has its own bullet below; this line is the head-of-`main` anchor + and nothing else. +- **Previous anchor, retained for provenance:** `6c9e765` (the dired + Stage 2 framing #171 and the resource-op delete guard framing #186 — both framing-only, no runtime code, no implementation started — atop the docs-only coherence listview correction #189, @@ -223,6 +232,31 @@ commands, read `docs/active-work.md` immediately after this file. remain parked pending the evidence this diagnostic produces, as does `terminate` idempotence for an already-reaped process (a different failure, so a different PR). + - **The diagnostic fired, and what it showed.** macOS CI, PR #191, + [run 30553376486](https://github.com/levineuwirth/pmacs/actions/runs/30553376486/job/90907461258): + `target=-8619 via group, leader_pid=8619, expected_group=-8619, + leader=live` — the `spec.group` pipe path, not the PTY path, with the + leader observed alive by a real `try_wait`. A rerun of the identical + head passed 12/12, so it is intermittent. + - **Owning the child does not license dismissing a group error, and + the occurrence does not prove the child received EPERM.** The failed + target was the *group* `-8619`; `try_wait` observed the *process* + `8619`. Nothing measured `getpgid(8619)`, so the two are not known to + refer to the same thing. What is settled is narrower and still + enough: a group target computed from the spawn-time `pgid == pid` + assumption returned EPERM while the leader was alive, which retires + "EPERM cannot happen for our own children" as a reason to discard an + arbitrary group-directed error. Attributing the errno to the child + would repeat the exact error that killed three tolerance rules — + concluding something about one entity from something about another. + - **A field named like an observation can be a restatement of its + input.** `expected_group` is `-leader_pid`, and on the spawn-group + path the target is `-leader_pid` too, so the report printed the same + number three times and their agreement was arithmetic. Stage B adds a + `measured_group` from `getpgid`, which is the only field able to + disagree — and it still establishes no identity, because it is read + inside the same read-then-act window and no portable mechanism closes + that for a *group* (`pidfd` covers a process; macOS has neither). - **Lean 4 arc (Arc 8) — stages 1, 2, 3a, 3b, 4a, 4b ALL LANDED** (`docs/lean4-mode-framing.md`; #160, #161, #167, #170, #179, #181). pmacs edits Lean 4: `arborium-lean` highlighting, a `lean4` major mode, `⟨⟩ ⦃⦄ ⟮⟯` pairs, and a `lake serve` language @@ -766,6 +800,56 @@ commands, read `docs/active-work.md` immediately after this file. `mark_document_stale` takes no `LspServerId` at all while creating URI keys across three stores. A route purge keyed on request responses cannot cover either. +- **Resource-op delete-guard implementation LANDED — #190**, atop the + framing #186 below. The pre-filesystem refusal now exists: a + synchronous `apply_resource_op` delete that would destroy unsaved work + is refused before the filesystem is touched, rather than after. + - **A refusal that no production path can reach passes every + direct-call test.** The guard is asserted through the outermost + user-reachable seam and falsified by revert, not by calling the + check directly. This is the general rule now recorded in §5. + - **Delete refusals must be visible.** Review round 2 found them + silent — the operation declined and the user learned nothing. + Reporting goes through `pmacs.editor.set_status`; `pmacs.error` is + still a channel defined only by a test stub (§5). + - **A URI-keyed store is not one store.** Purging a route keyed on + request responses covers neither `mark_document_stale` (which takes + no `LspServerId` while creating URI keys across three stores) nor + `DiagnosticView`, whose URI is fixed at construction. +- **dired Stage 2a LANDED — #196** (`docs/dired-stage2-framing.md` + rev 9, §5/§6/§10 — the substrate transaction only, **no dired + surface**). Rename and delete reconciliation across the path owners a + rename actually crosses. + - **A rename is a transaction across five owners**, and this is the + fact that forced the 2a/2b split: the buffer path, the buffer name, + the URI-keyed LSP stores plus `DiagnosticView`, dired's pathless + handles, and a captured Lua local that no transaction can reach. + Stage 2b owns everything needing new Rust primitives. + - **New LSP resource subscribers must not swallow reconciliation + failures**, and `forget_uri` must not leave purged requests live in + `LspClient.pending` — both were review findings, both now pinned. +- **Generated-buffer immutability Stage 1 LANDED — #191**, adopting the + contract framed in #188. `dired.lua`'s `paint` and `listview.lua`'s + `render` write through `pmacs.buffer.set_generated_contents`; zero + `bypass_intercept` writes remain in either file. + - **Why these two families first, and it is not the cheap half.** + `compile.lua` and `builtin/commands/default.lua` rebind all seven + undo chords to a no-op; `dired.lua` and `listview.lua` rebind + **nothing**, so a bare `C-/` emptied a listing and a panel. Stage 1 + closes the only two families reachable without `M-x`. + - **The framing owns the acceptance contract; the implementation + adopts it.** Review round 5 found #191 had locally restated Stage 1 + criteria while #188 still carried the originals. Where an + implementation finds a criterion impossible, the framing is revised + and re-approved first — it is not narrowed in place. + - **Stage 2 still owes everything with new Rust in it:** + `Buffer::apply_generated_edit`, the `{ generated = true }` option, + Q#GB10's path-backed refusal, Q#GB15's `identity_protected`, + Q#GB5's `ensure_slot` lock, and the remaining 13 write sites. +- **Generated-buffer immutability framing LANDED — #188** + (`docs/generated-buffer-immutability-framing.md`, revision 7; six + review rounds, thirty-two findings). Revision 7 is the governing + contract for the whole arc. - **Resource-op delete guard framing LANDED (document only) — #186** (`docs/resource-op-delete-guard-framing.md`, revision 5; five review rounds). **Approved as a framing; no runtime code.** It owns the diff --git a/docs/process-signal-diagnostic-completeness-framing.md b/docs/process-signal-diagnostic-completeness-framing.md new file mode 100644 index 0000000..b945da7 --- /dev/null +++ b/docs/process-signal-diagnostic-completeness-framing.md @@ -0,0 +1,563 @@ +# Framing — make the signal diagnostic discriminating (evidence collection) + +**Revision 6.** Status: implemented; PR open, review round 4 closed. Lane: +`process-signal-diagnostic-completeness`, worktree +`../pmacs-signal-identity`, based on `githubsucks/main` @ `4cd4a7b` +(re-measure at branch time; this is a reading, not a constant). + +**This is Stage B of the lane whose Stage A merged as PR #176** +(`docs/process-signal-tolerance-framing.md`, revision 4). Stage A made a +failing `kill` self-describing and parked every tolerance rule behind +evidence. Evidence arrived (§1.2). It supports none of the parked rules, +no identity claim, and — per review round 2 — no claim that today's +escalation path is safe. + +**Evidence collection only. No tolerance rule, no change to which process +gets signalled, no disposition change.** Everything behavioural is in §5. + +## Revision history + +**Revision 5 → 6**, after review round 4 (three blocking, one major). +All four accepted. + +- **`measured_group` was sampled AFTER the failed `kill`** and after + `observe_leader`, while both the framing and the function's own doc + said before. A concurrent group change would have made the diagnostic + report post-failure state as evidence about the attempted target. It + is now sampled in `signal` before the kill and passed into the report. +- **The Linux corroboration did not exercise the production lookup.** + Its helper read `portable_pty::process_group_leader` — the accessor + this lane stopped using — so `pty_foreground_group` could have fallen + back on every call with every test still green. Demonstrated: forcing + it to always fall back leaves the injected pin **passing** and only + the corroboration failing. The helper now calls the production lookup, + and the corroboration forces *only* the kill so the report is built + from a real terminal read. +- **This document did not update its own acceptance contract** (§4.1). + Revision 5 recorded the falsification in the revision history and Bet + 1 but left the normative criterion demanding the real-shell rewrite — + the exact "implementation quietly diverges from the contract" shape + this project recorded as a lesson on #191/#188. +- **`TargetSource`'s doc had the wrong classification.** Two of four + variants now target the leader pid, not one, and the pid-versus-group + split does not line up with PTY-versus-pipe — which is *why* + `PtyForegroundFallback` needed its own variant. + +**Revision 4 → 5**, after implementation. **Bet 1 was falsified by CI**, +and the framing's own fallback is what shipped. + +- **`bash -m` does not diverge on macOS.** Both macOS legs reported + `job control never moved the terminal off the leader (leader=8542, + foreground groups observed: [8542])` — the terminal stayed with the + leader for the entire 10s bounded wait. It diverges reliably on Linux + (20/20 locally, green on both ubuntu legs), so this is a platform + difference, not a flake, and rerunning would have been wrong. +- **The stated fallback was taken**: the divergent case is now pinned by + **injecting** the foreground group at the `signal_target` seam, and + §3 Bet 1 records it as *weaker* than a real shell rather than + quietly equivalent. Verified still discriminating — the + `leader_pid`-substitution mutation fails it (`target=-1707909` against + an expected `-1707910`). +- **The real fixture is retained as corroboration**, Linux-only, under + `job_control_really_diverges_the_foreground_group`. It is skipped on + macOS by platform check rather than by arming, because the + precondition genuinely does not hold there; running it would assert a + false claim about macOS instead of finding a bug. +- **`PMACS_REQUIRE_BASH` moves to Linux-only.** Rev 4's reasoning for + arming it on both platforms — "macOS is where the failures happen, so + arming it Linux-only leaves it dark where it matters" — was correct + about the *diagnostic* and wrong about *this test*, which cannot + produce its precondition on macOS at all. The diagnostic's macOS + coverage comes from the injected pin, which runs everywhere. + +**Revision 3 → 4**, after review round 3 (three blocking, one major). +All four accepted and checked against the exact APIs, process model, and +branch ancestry before revision. + +- **Rev 3's "no safe fd bridge" conclusion was still too absolute** + (§1.6). `filedescriptor::OwnedHandle::dup` accepts an `AsRawFd` through + its safe `AsRawFileDescriptor` blanket implementation and returns an + owned value implementing `AsFd`. A lifetime-tied wrapper around + `MasterPty::as_raw_fd` therefore bridges to + `nix::unistd::tcgetpgrp` with no `unsafe` in pmacs. The crate is + already resolved through `portable-pty`; this lane declares it + directly and restores errno capture. +- **The occurrence did not prove "our own child, alive, EPERM"** (§1.3). + The failed target was a *group* and `try_wait` observed the leader + process. No measurement established that the leader still belonged to + that group. What is invalidated is using ownership of the spawned + child to dismiss an arbitrary group-target error. +- **Bet 1 called a terminal-owning job "background"** (§3). A background + group is, by definition, not the terminal's foreground group. The + fixture now names `/bin/bash`, launches a foreground job in its own + group, and waits for the actual terminal handoff before measuring. +- **The branch-base line described the scout, not the ancestry.** Rev 3's + merge-base with `4cd4a7b` was still `391d38a`. Canonical main is now + integrated, and the lane is recorded in `docs/active-work.md`. + +**Revision 2 → 3**, after review round 2 (three blocking, three major). +All six accepted; all six verified in the code before acceptance. + +- **Rev 3 concluded that the PTY errno proposal had no safe fd bridge.** + It therefore withdrew and reduced rev 2's central new proposal (§1.6). + Revision 4 supersedes that conclusion after checking + `filedescriptor`'s safe duplication API. +- **Rev 2 said `getpgid` was "ungated". It is not** (§1.5a). The claim + came from reading the four lines above the function; the gate is a + block-level `feature!` opened 168 lines earlier. Same error shape as + the truncated-output trap already in the handoff, committed inside a + document about non-discriminating evidence. +- **Bet 4's `setsid` fixture was impossible** (§3, Bet 4). A `spec.group` + child is already a process-group leader, and a group leader's `setsid` + fails with EPERM. +- **"Recoverable" was unsupported** (§1.8). The ledger drops its entry on + *any* probe error — including an EPERM that ownership of the recorded + child cannot rule out for a group target — and discards the `SIGKILL` + result while marking the entry killed. +- **Rev 2 falsified the wrong Stage A sentence** (§1.3). Stage A's + disjointness claim was about the **PTY** path and remains true. +- **Rev 2's signal-disposition argument was wrong** (§1.7). Failed + signals are all disposition-identical. +- **Bet 5 proposed a test that already exists** (§1.9). Cited as ground + truth now, not invented. + +**Revision 1 → 2**, after review round 1 (two blocking, two major); all +accepted. Rev 1 proposed *retargeting* to a measured pgid — a behavioural +change resting on an identity claim a number cannot support; it asserted +`getpgid(child) == pid`, which an implementation ignoring `getpgid` would +satisfy; it scoped the PTY path out; and it never noticed the report +omits the signal. + +**Rev 1 was written to a session scratchpad rather than a branch**, so it +was never on `githubsucks` and review round 1 necessarily landed on +Stage A's merged document. Recorded because "work is portable only after +it is committed and pushed" is a standing rule this lane broke on its +first step. + + +## 0. Coherence impact (COHERENCE §20) + +- **Journey step 8, "Open a terminal"** (§2), teardown half, plus every + compile/grep run through `spec.group`. **No grade change, no + behavioural change.** +- **Serves §9 (worker model), failure attribution.** Stage A made the + failure describe itself; this lane makes the description + *discriminating*, because several distinct failures render identically + today. +- **Interaction islands: none. Config registry: not adopted. + Background-work attribution: unchanged.** +- **No audited claim in COHERENCE.md changes**; under §25 no COHERENCE + edit rides this PR. + + +## 1. Ground truth (verified at `4cd4a7b`) + +### 1.1 Stage A landed and has now fired + +`signal_failure_report` and `LeaderObservation` merged as **PR #176 on +2026-07-26** (`62316a9`). §1.2 is the first failure carrying the new +format rather than a bare errno. Stage A is why this document can exist. + +### 1.2 The new occurrence, verbatim + +PR #191, `Test (macos-latest / lua54)`, +[run 30553376486](https://github.com/levineuwirth/pmacs/actions/runs/30553376486/job/90907461258), +`process::tests::repeated_terminate_does_not_extend_ledger_deadline`. +1873 passed, 1 failed. **A rerun of the identical head passed 12/12**, so +the failure is intermittent, not deterministic: + +``` +re-terminate: "kill: EPERM: Operation not permitted + (target=-8619 via group, leader_pid=8619, expected_group=-8619, leader=live)" +``` + +Established: the target source is `group` — the `spec.group` pipe path +(`signal_target` `:774-780`; `sh_group_spec` `:3402`) — **not** the PTY +path; and `leader=live`, from a real `try_wait` against the real child, +so the leader had neither exited nor been reaped. + +### 1.3 What the occurrence actually invalidates + +- **`src/process.rs:1246-1247` uses an invalid premise.** + `tick_reap_ledger` justifies treating any probe error as "nothing left + we can reach" with the comment "**EPERM cannot happen for our own + children**". But the operation is group-directed: ownership of the + spawned child says nothing unless that child is still a member of the + targeted group. +- **§1.2 does not prove EPERM was "for our own child".** The failed + target was group `-8619`; `leader=live` observed process `8619`. + Nothing measured `getpgid(8619)`, so the occurrence establishes only + that a group target computed from the spawn-time assumption returned + EPERM while the leader process was alive. That is enough to invalidate + the comment as a reason to discard arbitrary group errors, but not to + attribute the errno to the child. +- **Stage A §1.3 is *not* falsified.** It said the ledger is disjoint + from **the PTY path**, because the ledger arms only for + `proc.spec.group` and PTY mode cannot set it. That remains true. §1.2 + is the separate `spec.group` path. Rev 2 conflated "this path" with + "the signal path generally" and claimed a falsification it had not + made. + +Stage A's entity-split analysis concerns the PTY path, where the target +is read from `tcgetpgrp`. It does not apply to §1.2, where the target is +computed as `-leader_pid` with no read at all. + +### 1.4 The landed acceptance cannot discriminate + +`a_group_directed_kill_failure_reports_target_and_leader_separately` +(`:2400`) spawns a PTY child and asserts the exact string + +``` +target=-{pid} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=live +``` + +— the same `pid` three times. **An implementation that ignored +`tcgetpgrp` and substituted `leader_pid` would pass.** The test's own doc +comment concedes it: "here they are asserted to agree only because +nothing has moved the terminal." The premise of the diagnostic is that +these entities can diverge, and nothing exercises a case where they do. + +### 1.5 A numeric pgid cannot establish identity + +The value is read before `kill`, the window remains open, and a *number* +cannot distinguish the original group from a recycled one. + +**No portable mechanism closes this.** `pidfd_open` + `pidfd_send_signal` +close pid reuse for a single *process* on Linux; there is no +process-*group* equivalent, and macOS has no pidfd. The failures are +macOS-only, so nothing available makes group signalling identity-safe. + +**This lane therefore records and does not retarget.** No acceptance +claims the telemetry is sufficient. + +One narrowing fact, stated as narrowing and **not** as identity: POSIX +does not free a child's pid until the parent reaps it, and §1.2 observed +`leader=live` from a `try_wait` that had not reaped. While that pid is +held, no *new* group can be created bearing that pgid value. This makes +recycling an unlikely explanation **for that one occurrence**, and says +nothing about whether the group still held a signallable member — which +is what EPERM actually turns on. + +### 1.5a The nix surface is gated, and available for a non-obvious reason + +Both calls this lane would use live inside block-level gates: + +- `getpgid` (`nix-0.29.0/src/unistd.rs:335`) is inside + `feature! { #![feature = "process"] }` opened at `:167`. +- `tcgetpgrp` (`:368`) is inside + `feature! { #![all(feature = "process", feature = "term")] }` at `:360`. + +pmacs declares `nix` with `features = ["signal", "user", "fs", "term", +"socket", "poll"]` — **`process` is not listed**. It is enabled anyway +because **nix's own `signal` feature depends on `process`**, verified +with `cargo tree -e features -i nix:0.29.0`: + +``` +├── nix feature "process" +│ └── nix feature "signal" +│ └── pmacs v1.0.0 +``` + +Confirmed by compiling both calls against the real dependency graph. + +**This is stable but implicit.** The lane adds `process` to pmacs' own +feature list so the dependency is declared rather than inherited — a +one-line change that makes a real requirement visible. + +*Rev 2 asserted `getpgid` was "ungated", from reading the four lines +above it. The gate was 168 lines up. Recorded because it is the same +defect class this document exists to fix.* + +### 1.6 The PTY fallback is invisible; a safe owned-dup bridge preserves errno + +`signal_target` (`:757-785`): when the PTY branch's +`master.process_group_leader()` returns `None`, control falls through — +`spec.group` is rejected at spawn for PTY mode — and returns +`TargetSource::LeaderPid`, rendered "leader-pid" (`:738`). **A normal +pipe child renders identically.** Two situations, one string. + +`portable-pty` (`portable-pty-0.9.0/src/unix.rs:374`) discards the errno: + +```rust +fn process_group_leader(&self) -> Option { + match unsafe { libc::tcgetpgrp(self.fd.0.as_raw_fd()) } { + pid if pid > 0 => Some(pid), + _ => None, + } +} +``` + +`nix::unistd::tcgetpgrp` requires `F: AsFd`, while +`MasterPty` exposes only `fn as_raw_fd(&self) -> Option` +(`portable-pty-0.9.0/src/lib.rs:114`). Rev 3 inspected only the standard +library's raw-to-owned constructors and concluded every bridge required +`unsafe`. That missed the safe duplication abstraction already in the +dependency graph: + +- `filedescriptor::OwnedHandle::dup(&F)` is safe + (`filedescriptor-0.8.3/src/lib.rs:230`); +- on Unix, `filedescriptor` implements `AsRawFileDescriptor` for every + `T: AsRawFd` (`src/unix.rs:20`); +- `OwnedHandle` implements `AsFd` (`src/unix.rs:64`). + +A small wrapper holds a borrow of `MasterPty` for its lifetime and +implements the safe `AsRawFd` trait by returning the master's reported +fd. `OwnedHandle::dup` consumes that borrowed view immediately and +returns an independently owned duplicate; `tcgetpgrp(&owned)` then +preserves the `Errno`. **No raw-to-owned constructor and no `unsafe` +appears in pmacs.** `filedescriptor 0.8.3` is already in `Cargo.lock` +through `portable-pty`; this lane adds it as a direct dependency because +pmacs now calls its API. + +`OwnedHandle::dup` is itself fallible and preserves its Unix +`std::io::Error` source. That failure must not be collapsed into the +terminal query. The PTY result is therefore four-way and discriminating: + +- a positive pgid selects the foreground group, as today; +- a duplicate failure falls back to the leader and reports the + `duplicate-master-fd` stage plus its OS errno; +- a `tcgetpgrp` error falls back to the leader and reports the errno; +- absence of a master fd is a distinct unavailable source, not forged + into an errno. + +### 1.7 The report omits which signal failed + +`signal_failure_report` (`:830-850`) takes target, leader pid, errno and +leader observation — **not the signal**. `signal` (`:1074`) has it, and +the public Lua surface accepts INT, USR1, USR2 and QUIT besides the fatal +three (`src/lua_bindings/mod.rs:8627-8629`). A failed `SIGUSR1` and a +failed `SIGTERM` are today textually indistinguishable. + +**Rev 2 justified this by claiming their dispositions differ. That was +wrong.** `signal` returns `Err` at `:1092-1098`, *before* the fatal-signal +branch at `:1099`, so **every failed kill is disposition-identical** +regardless of signal. The disposition difference is real only for +**successful** calls. The reporting gap stands on its own: you cannot +tell which signal failed. Acceptance 3 separates the two. + +### 1.8 The disposition consequence, and why "recoverable" was wrong + +`signal` returns `Err` **before** the state transition and **before** +arming the ledger: + +| Which `terminate` hits EPERM | Consequence | +|---|---| +| A **later** one (§1.2's case) | Caller sees `Err`. State is already `Exiting` and a ledger entry **remains scheduled**. | +| The **first** one | State stays `Running`, ledger never armed. **No escalation is ever scheduled** — the child is abandoned. | + +**Rev 2 called the first row "recoverable" and claimed "SIGKILL +escalation still happens". Unsupported.** `tick_reap_ledger` +(`:1249-1254`): + +```rust +if nix::sys::signal::kill(Pid::from_raw(-*pgid), None).is_err() { + return false; // drops on ANY error, incl. EPERM +} +if now >= entry.deadline && !entry.killed { + let _ = nix::sys::signal::kill(..., Some(Signal::SIGKILL)); // result discarded + entry.killed = true; // marked killed regardless +} +``` + +If a ledger probe returns EPERM, it **drops the entry and cancels +escalation silently**; if its `SIGKILL` fails, the result is recorded as +if it succeeded. §1.2 did not observe either ledger call — it observed a +later explicit `SIGTERM` to the same assumed group number — so the +ledger failure is an exposed, still-unmeasured hazard rather than an +observed occurrence. The honest statement remains that escalation is +*scheduled*, not that it happens. + +**This still-silent path is parked, explicitly** (§5) rather than +absorbed: it is a second site with its own disposition questions, and +folding it in would repeat Stage A rev 3's error of implementing Stage B +inside Stage A. + +### 1.9 The first-call variant is already pinned + +`an_injected_failure_changes_no_state_and_arms_no_ledger` (`:2501`) +already spawns a `spec.group` child, injects EPERM on the **first** +`terminate`, and asserts `Running` plus an empty ledger. **Rev 2's Bet 5 +proposed inventing it.** It is ground truth, and its exact-string +assertion (`:2517`) is one of the four sites acceptance 5 must update. + +### 1.10 Limits of the evidence + +- **Not reproduced locally.** Development is Linux; failures are + macOS-only. No claim rests on a local repro of the EPERM. +- **Two occurrences, in different paths** — PR #172 was the PTY path + (`acc28`, luajit), §1.2 the group path (lua54). Not one flaky test. +- **The mechanism is not established**, and this lane does not propose + one. + + +## 2. Questions + +- **Q#DC1** — Can the two entities be made to diverge in a test? *Yes: + under a PTY, `/bin/bash` with job control enabled launches a + **foreground** job in its own process group and hands it the terminal, + so `tcgetpgrp` != leader pid.* +- **Q#DC2** — Should the PTY fallback get its own `TargetSource`? + *Proposed: yes. A failed duplicate or terminal lookup reports its stage + and errno; a missing master fd reports unavailable (§1.6).* +- **Q#DC3** — Should the report name the signal? *Proposed: yes, on the + reporting argument alone (§1.7).* +- **Q#DC4** — Should the measured pgid be reported for `spec.group` + children? *Proposed: yes, as an observation distinct from the assumed + value, with no sufficiency claim (§1.5).* +- **Q#DC5** — Retarget or tolerate anything? **No. Parked.** + + +## 3. Bets + +- **Bet 1 — the divergence is constructible.** A PTY fixture where the + foreground group is not the leader: `/bin/bash --noprofile --norc -m` + launches a foreground child in a fresh process group. The fixture + performs a bounded wait until `tcgetpgrp` itself reports the non-leader + group, asserts that group still has a live member as the positive + control, and only then injects the failing `kill`. The rewritten + acceptance asserts both exact values **and that they differ**. + - *Falsified if* the fixture cannot be made deterministic in CI. Then + the lane falls back to pinning divergence at the `signal_target` unit + level with an injected foreground group, and labels that as weaker. + - **OUTCOME: falsified on macOS.** Both macOS legs observed the + terminal stay with the leader for the full bounded wait; Linux + diverges reliably. The fallback shipped: the divergent case is + pinned by injection everywhere, and the real shell corroborates it + on Linux only. + - **The injected pin is weaker, and here is exactly how.** It proves + the target is read from the *lookup* rather than substituted from + the leader — the substitution mutation still fails it. It does + **not**, by itself, prove any real shell produces that divergence; + `job_control_really_diverges_the_foreground_group` carries that, on + one platform. + +- **Bet 2 — the PTY fallback is reachable and distinguishable.** A test + drives all three non-success arms: a duplicate errno, a `tcgetpgrp` + errno, and a missing master fd. Each source is distinct from a pipe + child's and from the others. + - *Falsified if* the branch cannot be reached without faking the + lookup — in which case the seam is made injectable exactly as Stage A + made the kill injectable (Q#PD4), stated rather than hidden. + +- **Bet 3 — naming the signal is free.** Thread `signal` into the report. + - *Falsified if* any exact-string test cannot be updated mechanically. + +- **Bet 4 — a `spec.group` child's measured pgid can be made to differ + from its pid.** **Not via `setsid`:** `spec.group` sets + `process_group(0)` before exec, so the recorded child is already a + process-group leader, and a group leader's `setsid` fails with EPERM. + Forking a `setsid` helper does not help either — `getpgid(recorded_pid)` + still observes the wrapper. + The fixture instead has the recorded child **`setpgid` into another + existing group in the same session**, with a readiness handshake before + the measurement and explicit cleanup of the anchor group afterwards. + - *Falsified if* no such fixture is deterministic — in which case the + measurement is unfalsifiable and **does not ship**, per §1.4's lesson. + + +## 4. Acceptance + +1. The divergent case is pinned on **every** platform: a group-directed + failure whose target differs from the leader pid, with both exact + values asserted and asserted to differ. The pre-Stage-B test is + **rewritten**, not supplemented — it pinned a substitution as + acceptable. + + **The divergence is injected at the `signal_target` seam**, per Bet + 1's falsification: a real `bash -m` fixture diverges on Linux and + never on macOS. The injected form is weaker and §3 Bet 1 says how. + +1a. **A Linux-only corroboration** drives a real job-control shell, + forces *only* the kill failure, and asserts the production report + names the real foreground group. This is the sole test that exercises + `pty_foreground_group` end-to-end — every other test supplies the + group itself and therefore cannot detect a lookup that always falls + back. **Residual limitation, stated rather than buried: on macOS the + production lookup has no end-to-end coverage**, because the platform + cannot produce the precondition. +2. The PTY foreground-lookup fallback reports a source distinct from a + pipe child's. Separate tests drive the duplicate-error and + `tcgetpgrp`-error arms and assert the exact stage and errno; a third + drives the unavailable-fd arm. If one cannot be produced reliably + through a real PTY, the lookup result is injected while the branch, + target choice, real child observation, and report construction remain + production code (§3 Bet 2). +3. The report names the signal, split into two independent checks: + (a) a **failure-format** comparison showing `SIGUSR1` and `SIGTERM` + failures differ *in text only*, both leaving state and ledger + unchanged; and (b) a **successful-call disposition control** showing + a successful `SIGUSR1` does not transition state or arm the ledger + while a successful `SIGTERM` does. +4. For `spec.group` children the report carries the measured pgid as a + field distinct from the assumed one, renderable as unobservable, with + a test asserting a case where they **differ** (§3 Bet 4). **It is + sampled before the `kill`**, not during report construction, so it + records pre-kill evidence about the target that was attempted rather + than state left behind by the failure. + + **It does not describe the group at the moment the `kill` executed.** + `getpgid` and `kill` remain separated by the read-then-act window + §1.5 describes, so the sample can be stale by the time the signal is + delivered. Moving it earlier removes a *post-hoc* reading; it does + not make the reading contemporaneous, and no acceptance may claim it + does. +5. All four exact-string sites — `:2408`, `:2435`, `:2485`, `:2517` — + updated **individually**, each listed in the PR body with before and + after. No blanket rewrite: that is how a format regression hides. +6. `:2501`'s existing first-call pin is **retained and cited**, updated + only for the new format. +7. `process` added to pmacs' declared `nix` features, and + `filedescriptor 0.8` declared directly for the safe PTY-fd duplicate + (§1.5a, §1.6). +8. `docs/agent-handoff.md` records that ownership of the recorded child + cannot justify dismissing an error from a group target, with the run + link and §1.2's measurement limit; the comment at `:1246` is corrected + in the same PR without claiming that the child itself received EPERM. +9. **No acceptance claims the telemetry establishes group identity** + (§1.5), and none claims escalation is guaranteed (§1.8). The PR body + repeats both. + + +## 5. Parked + +- **The reap ledger's silent cancellation** (§1.8): if a probe returns + EPERM the entry is dropped, and a failed `SIGKILL` is marked as killed. + The explicit-signal occurrence exposes the premise but did not observe + either ledger call. **Its own lane** — disposition questions, second + site. +- **Retargeting to the measured pgid.** Behavioural; unsupported by §1.5. +- **Any tolerance rule for EPERM or ESRCH.** Unmotivated across Stage A's + three revisions and still unmotivated. +- **§1.8's first-call abandonment.** Pinned at `:2501`, not fixed here. +- **Q#PS6** — `terminate` on an already-reaped process returning `Ok`. +- **`signal_target`'s read-then-kill of `tcgetpgrp`** — Stage A's "most + likely real fix site, still unframed". This lane makes it *observable*, + not fixed. +- **`compile_mode_acceptance` reading the developer's real + `~/.config/pmacs/init.lua`** — separate defect, 11 local failures, + invisible in CI. + + +## 6. Gates + +Standard suite, each its own step with a real exit status and nothing +after the command that could mask it: `cargo fmt --check`; `cargo clippy +--workspace --all-targets -- -D warnings`; `cargo test --lib`; `cargo +test --lib --features crdt`; `compile_mode_acceptance`; +`terminal_copy_mode_acceptance` (both feature configurations); +`bottom_panel_stage1_acceptance`; `cargo test --test m4_acceptance -- +--skip basedpyright`; `PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu`; +`git diff --check`. + +**All local runs use an isolated `XDG_CONFIG_HOME`** — without it +`compile_mode_acceptance` fails 11 tests for unrelated reasons. + +**PTY job-control tests are load-sensitive.** Run repeatedly; the PR body +records the repetition count, not a single green. + + +## 7. Branch plan + +One branch, one PR. Bet 1 first and alone: it decides whether the +diagnostic is worth extending at all. If the divergence fixture cannot be +made deterministic, the lane is re-scoped rather than pushed through. diff --git a/src/process.rs b/src/process.rs index f0b851c..33fdf8f 100644 --- a/src/process.rs +++ b/src/process.rs @@ -477,6 +477,9 @@ pub struct ProcessSupervisor { /// observation still runs against the real child handle; a stubbed /// observation would bypass the code path under test. forced_kill_errno: Option, + /// Test seam for the PTY foreground-group lookup (see + /// `force_next_pty_lookup`). Always `None` outside tests. + forced_pty_lookup: Option>, } /// One armed group in the reap ledger. @@ -714,10 +717,18 @@ impl ChildHandle { /// 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. +/// `kill` can possibly mean. Two of the four aim at the spawned child +/// itself — [`Self::LeaderPid`] for a pipe child with no group, and +/// [`Self::PtyForegroundFallback`] for a PTY whose terminal named no +/// group. The other two aim at a *group*: +/// [`Self::SpawnGroup`] at one computed from the spawn-time `pgid == +/// pid` assumption, and [`Self::ForegroundGroup`] at one read from the +/// terminal, which can belong to something the supervisor never spawned. +/// +/// The pid-versus-group split is the classification that matters here, +/// and it does **not** line up with the PTY-versus-pipe split — which is +/// exactly why the fallback needed its own variant instead of reusing +/// `LeaderPid`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum TargetSource { /// The tty's current foreground process group, read at signal @@ -726,16 +737,58 @@ enum TargetSource { ForegroundGroup, /// A `group = true` pipe child leading its own process group. SpawnGroup, - /// The child's own pid. + /// The child's own pid, for a pipe child that leads no group. LeaderPid, + /// A **PTY** child whose foreground-group lookup did not yield a + /// group, so the target fell back to the leader pid. + /// + /// Distinct from [`Self::LeaderPid`] on purpose. Before this + /// variant existed both rendered "leader-pid", so a PTY whose + /// terminal query failed was indistinguishable in the report from + /// an ordinary pipe child that never had a terminal — two very + /// different situations reading as one. + PtyForegroundFallback(PtyLookupFailure), +} + +/// Why a PTY's foreground-group lookup produced no group. +/// +/// Each arm is a different fact and none is forged into another: a +/// missing fd is not an errno, and a failure to *duplicate* the master +/// is not a failure to *query* the terminal. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum PtyLookupFailure { + /// The master reported no file descriptor to query. + NoMasterFd, + /// Duplicating the master fd failed, so the terminal was never + /// queried at all. + Duplicate(nix::errno::Errno), + /// `tcgetpgrp` itself failed on a successfully duplicated fd. + Query(nix::errno::Errno), + /// The terminal answered, but with a non-positive group id, which + /// names no group. + NonPositive(i32), +} + +impl PtyLookupFailure { + fn render(self) -> String { + match self { + Self::NoMasterFd => "no-master-fd".to_owned(), + Self::Duplicate(e) => format!("duplicate-master-fd: {e}"), + Self::Query(e) => format!("tcgetpgrp: {e}"), + Self::NonPositive(v) => format!("tcgetpgrp-non-positive: {v}"), + } + } } impl TargetSource { - fn as_str(self) -> &'static str { + fn render(self) -> String { match self { - Self::ForegroundGroup => "tcgetpgrp", - Self::SpawnGroup => "group", - Self::LeaderPid => "leader-pid", + Self::ForegroundGroup => "tcgetpgrp".to_owned(), + Self::SpawnGroup => "group".to_owned(), + Self::LeaderPid => "leader-pid".to_owned(), + Self::PtyForegroundFallback(why) => { + format!("pty-leader-fallback({})", why.render()) + } } } @@ -745,6 +798,85 @@ impl TargetSource { } } +/// A lifetime-tied view of a `MasterPty`'s file descriptor. +/// +/// `MasterPty` exposes only `Option`, and every std route from a +/// raw fd to something implementing `AsFd` — `BorrowedFd::borrow_raw`, +/// `OwnedFd::from_raw_fd`, `File::from_raw_fd` — is `unsafe`, which this +/// crate forbids. `filedescriptor::OwnedHandle::dup` accepts any +/// `AsRawFd` through a safe blanket impl and hands back an owned handle +/// that *is* `AsFd`, so implementing this one safe trait is the whole +/// bridge. +/// +/// The borrow is what makes it sound: the view cannot outlive the master +/// it read the descriptor from, so the fd cannot have been closed +/// underneath it. +struct MasterFdView<'a> { + fd: std::os::fd::RawFd, + _master: &'a (dyn portable_pty::MasterPty + Send), +} + +impl std::os::fd::AsRawFd for MasterFdView<'_> { + fn as_raw_fd(&self) -> std::os::fd::RawFd { + self.fd + } +} + +/// Recover the OS errno from a `filedescriptor` error. +/// +/// Its error type is an enum of thiserror variants, each carrying a +/// `std::io::Error` as a `#[source]` rather than exposing +/// `raw_os_error` itself. Walking the source chain and downcasting keeps +/// every variant working, including ones added later, instead of +/// matching the one arm that exists today. +/// +/// Returns `UnknownErrno` when the chain carries no OS error, rather +/// than inventing a plausible one — a forged errno in a diagnostic is +/// worse than an honest absence. +fn os_errno_of(err: &filedescriptor::Error) -> nix::errno::Errno { + let mut current: Option<&(dyn std::error::Error + 'static)> = Some(err); + while let Some(e) = current { + if let Some(io) = e.downcast_ref::() + && let Some(code) = io.raw_os_error() + { + return nix::errno::Errno::from_raw(code); + } + current = e.source(); + } + nix::errno::Errno::UnknownErrno +} + +/// Read the terminal's foreground process group, keeping the errno. +/// +/// `portable_pty::MasterPty::process_group_leader` collapses every +/// failure into `None`, so pmacs could not tell "this tty has no +/// foreground group" from "the query failed and here is why". This does +/// the query itself and returns the reason on every non-success path. +fn pty_foreground_group( + master: &(dyn portable_pty::MasterPty + Send), +) -> Result { + let Some(fd) = master.as_raw_fd() else { + return Err(PtyLookupFailure::NoMasterFd); + }; + let view = MasterFdView { + fd, + _master: master, + }; + let owned = filedescriptor::OwnedHandle::dup(&view) + .map_err(|e| PtyLookupFailure::Duplicate(os_errno_of(&e)))?; + match nix::unistd::tcgetpgrp(&owned) { + Ok(pgrp) => { + let raw = pgrp.as_raw(); + if raw > 0 { + Ok(raw) + } else { + Err(PtyLookupFailure::NonPositive(raw)) + } + } + Err(e) => Err(PtyLookupFailure::Query(e)), + } +} + /// 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). @@ -754,18 +886,36 @@ struct SignalTarget { source: TargetSource, } -fn signal_target(proc: &ManagedProcess, pid: u32) -> Result { +fn signal_target( + proc: &ManagedProcess, + pid: u32, + forced_lookup: Option>, +) -> Result { if let Some(runtime) = proc.runtime.as_ref() && let ChildHandle::Pty { _master: master, .. } = &runtime.child - && let Some(pgrp) = master.process_group_leader() - && pgrp > 0 { - return Ok(SignalTarget { - pid: Pid::from_raw(-pgrp), - source: TargetSource::ForegroundGroup, - }); + // A PTY child is always group-directed when the terminal names a + // foreground group. When it does not, the target falls back to + // the leader — and *why* it fell back is carried into the source + // so the report can say it. Previously every one of these paths + // produced a bare `LeaderPid`, identical to a pipe child that + // never had a terminal at all. + let lookup = match forced_lookup { + Some(outcome) => outcome, + None => pty_foreground_group(master.as_ref()), + }; + return match lookup { + Ok(pgrp) => Ok(SignalTarget { + pid: Pid::from_raw(-pgrp), + source: TargetSource::ForegroundGroup, + }), + Err(why) => Ok(SignalTarget { + pid: Pid::from_raw(i32::try_from(pid).map_err(|e| e.to_string())?), + source: TargetSource::PtyForegroundFallback(why), + }), + }; } // `group = true` pipe children lead a fresh process group // (`process_group(0)` at spawn ⇒ pgid == pid), so fatal signals @@ -824,14 +974,48 @@ fn observe_leader(proc: &mut ManagedProcess) -> LeaderObservation { } } -/// 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. +/// The leader's process group as the kernel reports it, for a target +/// that was *computed* from the spawn-time assumption `pgid == pid`. +/// +/// `expected_group` is that assumption restated — it is `-leader_pid`, +/// and on the `SpawnGroup` path the target is `-leader_pid` too, so the +/// two agreeing is arithmetic rather than evidence. This is the only +/// field in the report that can disagree with the input, which is what +/// makes it worth printing. +/// +/// **It does not establish identity** (framing §1.5). It is read before +/// the `kill`, in the same read-then-act window, and a number cannot +/// distinguish the original group from a recycled one. No portable +/// mechanism can: `pidfd` closes pid reuse for a process, not a group, +/// and macOS has none at all. This records an observation; it settles +/// nothing. +fn measured_group_of(leader_pid: u32) -> String { + let Ok(raw) = i32::try_from(leader_pid) else { + return ", measured_group=unobservable(pid out of range)".to_owned(); + }; + match nix::unistd::getpgid(Some(Pid::from_raw(raw))) { + Ok(pgid) => format!(", measured_group=-{}", pgid.as_raw()), + Err(e) => format!(", measured_group=unobservable({e})"), + } +} + +/// Render a failing `kill` as the facts of Q#PD1. The disposition is +/// unchanged (Q#PD2) — this only replaces a message that said nothing +/// but the errno. +/// +/// The signal is named because it could not be recovered otherwise: a +/// failed `SIGUSR1` and a failed `SIGTERM` were previously identical +/// text. Note this is a *reporting* gap only — every failed `kill` +/// returns before the fatal-signal branch, so failed signals are +/// disposition-identical whatever they are. The disposition difference +/// is real only for calls that succeed. fn signal_failure_report( target: SignalTarget, leader_pid: u32, + signal: Signal, errno: nix::errno::Errno, leader: &LeaderObservation, + measured: Option<&str>, ) -> String { let expected = if target.source.is_group() { match i32::try_from(leader_pid) { @@ -841,10 +1025,15 @@ fn signal_failure_report( } else { String::new() }; + // Supplied by the caller, which samples it BEFORE the `kill`. Doing + // it here would describe the group as it stands *after* the failure + // and after `observe_leader`, which is post-hoc state presented as + // evidence about the attempted target. + let measured = measured.unwrap_or(""); format!( - "kill: {errno} (target={} via {}, leader_pid={leader_pid}{expected}, leader={})", + "kill: {errno} (signal={signal:?}, target={} via {}, leader_pid={leader_pid}{expected}{measured}, leader={})", target.pid.as_raw(), - target.source.as_str(), + target.source.render(), leader.render(), ) } @@ -950,6 +1139,7 @@ impl ProcessSupervisor { reap_ledger: HashMap::new(), group_term_grace: GROUP_TERM_GRACE, forced_kill_errno: None, + forced_pty_lookup: None, } } @@ -963,6 +1153,31 @@ impl ProcessSupervisor { self.forced_kill_errno = Some(errno); } + /// Test seam for the PTY foreground-group lookup, on the same terms + /// as [`Self::force_next_kill_errno`] and for the same reason. + /// + /// Injects either outcome. The failure arms — no master fd, a failed + /// duplicate, a failed `tcgetpgrp` — cannot be produced on demand + /// from a healthy PTY: they need an exhausted descriptor table or a + /// master that has stopped being a terminal. + /// + /// The **success** arm exists because a genuinely divergent + /// foreground group is not portable. `bash -m` produces one on + /// Linux and **does not on macOS**, where the terminal stays with + /// the leader for the whole wait (observed in CI on both macOS + /// legs). Injecting the group keeps the divergent case pinned + /// everywhere; `job_control_really_diverges_the_foreground_group` + /// corroborates it against a real shell where the platform allows. + /// + /// Either way the injection covers only the *lookup result*: the + /// branch, the target choice, the leader observation against the + /// real child, and the report construction all run as production + /// code. Consumed by one call. + #[cfg(test)] + fn force_next_pty_lookup(&mut self, outcome: Result) { + self.forced_pty_lookup = Some(outcome); + } + /// Override the SIGTERM-to-SIGKILL grace window. Test helper. pub fn set_grace_period(&mut self, d: Duration) { self.grace_period = d; @@ -1080,7 +1295,15 @@ impl ProcessSupervisor { else { return Err(format!("process {id} is not running")); }; - let target = signal_target(proc, pid)?; + let forced_lookup = self.forced_pty_lookup.take(); + let target = signal_target(proc, pid, forced_lookup)?; + // Sample the real group BEFORE signalling. Only the spawn-group + // path computes its target from the `pgid == pid` assumption, so + // it is the only one a measurement can contradict; a PTY target + // came from the terminal and a leader-directed target is not a + // group at all. + let measured = + matches!(target.source, TargetSource::SpawnGroup).then(|| measured_group_of(pid)); // 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 @@ -1094,7 +1317,14 @@ impl ProcessSupervisor { // 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)); + return Err(signal_failure_report( + target, + pid, + signal, + errno, + &leader, + measured.as_deref(), + )); } if matches!(signal, Signal::SIGTERM | Signal::SIGKILL | Signal::SIGHUP) { proc.state = ProcessState::Exiting { @@ -1243,9 +1473,24 @@ impl ProcessSupervisor { let now = Instant::now(); self.reap_ledger.retain(|pgid, entry| { // ESRCH: no such group — done. Any other probe error is - // also treated as "nothing left we can reach" (EPERM - // cannot happen for our own children) so the ledger - // cannot grow without bound. + // also treated as "nothing left we can reach", so the + // ledger cannot grow without bound. + // + // **That is a bounded-growth policy, not a claim that the + // group is gone.** This comment previously justified it with + // "EPERM cannot happen for our own children". That reasoning + // does not hold: the probe targets a *group*, and owning the + // spawned child says nothing about a group unless the child + // is still a member of it — which nothing here measures. A + // group-directed EPERM against a live leader has since been + // observed in CI (macOS, PR #191, run 30553376486), via an + // explicit signal rather than this probe. + // + // So this arm can silently cancel an escalation, and the + // `SIGKILL` below can fail while the entry is marked killed. + // Both are known and deliberately unchanged here: the + // diagnostic lane that found them does not alter + // disposition. Fixing it is its own lane. if nix::sys::signal::kill(Pid::from_raw(-*pgid), None).is_err() { return false; } @@ -2304,18 +2549,6 @@ mod tests { /// `/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. /// @@ -2384,39 +2617,492 @@ mod tests { } } - /// 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. + /// The job-control shell the divergence fixture drives. Named once so + /// the availability guard and the spawn cannot drift apart. + const BASH: &str = "/bin/bash"; + + /// A plain PTY child, for tests that care about the PTY *branch* + /// rather than about job control. + 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 tty's current foreground process group, read through the + /// **production** lookup — not `portable_pty`'s + /// `process_group_leader`, which this crate no longer uses on the + /// signal path. Reading it any other way would let + /// `pty_foreground_group` fall back on every call while every test + /// that depends on it stayed green. `None` for a pipe + /// generation, or when the terminal reports no foreground group. + fn foreground_pgid(sup: &ProcessSupervisor, id: ProcessId) -> Option { + let runtime = sup.processes.get(&id)?.runtime.as_ref()?; + match &runtime.child { + ChildHandle::Pty { + _master: master, .. + } => pty_foreground_group(master.as_ref()).ok(), + ChildHandle::Pipes(_) => None, + } + } + + /// Fixture for Q#DC1: a PTY child whose terminal foreground group is + /// genuinely **not** the spawned leader. /// - /// 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. + /// `bash -m` enables job control, so it runs the script's command in + /// a fresh process group and hands that group the terminal. The + /// trailing `; :` matters — with a single simple command `bash -c` + /// execs in place, which would leave the leader owning the terminal + /// and silently restore the very agreement this fixture exists to + /// break. + /// + /// **The wait is load-bearing, not defensive.** The handoff is not + /// instantaneous: a probe of this exact fixture observed the + /// foreground group as the leader first and only then as the job's + /// group. Measuring immediately would pin the non-divergent case and + /// the test would assert the opposite of its purpose. + /// + /// Returns `(id, leader_pid, foreground_pgid)` with the two pids + /// known to differ and the foreground group known to hold a live + /// member. + fn spawn_pty_with_diverged_foreground_group( + sup: &mut ProcessSupervisor, + name: &str, + ) -> (ProcessId, u32, i32) { + let mut spec = ProcessSpec::new(name, BASH); + spec.args = vec![ + "--noprofile".into(), + "--norc".into(), + "-m".into(), + "-c".into(), + "sleep 30; :".into(), + ]; + spec.mode = ProcessMode::Pty { + rows: 24, + cols: 80, + mode: TerminalMode::Canonical, + }; + let id = sup.spawn(spec).expect("spawn"); + let leader = spawn_started_pid(sup, id); + + let leader_i32 = i32::try_from(leader).expect("pid fits i32"); + let deadline = Instant::now() + Duration::from_secs(10); + let mut observed: Vec = Vec::new(); + let mut diverged = None; + while Instant::now() < deadline { + if let Some(fg) = foreground_pgid(sup, id) { + if observed.last() != Some(&fg) { + observed.push(fg); + } + if fg > 0 && fg != leader_i32 { + diverged = Some(fg); + break; + } + } + std::thread::sleep(Duration::from_millis(25)); + } + + let fg = diverged.unwrap_or_else(|| { + panic!( + "job control never moved the terminal off the leader \ + (leader={leader}, foreground groups observed: {observed:?})" + ) + }); + + // Positive control: a divergent number proves nothing if the + // group is already dead. The signal target must be a group that + // could actually receive a signal. + nix::sys::signal::kill(Pid::from_raw(-fg), None).unwrap_or_else(|e| { + panic!("foreground group {fg} has no live member ({e}); divergence is vacuous") + }); + + (id, leader, fg) + } + + /// Q#DC1 / 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 facts **that are not the same fact + /// repeated**. + /// + /// The pre-Stage-B version spawned `/bin/sleep` on a PTY and asserted + /// the same pid three times, conceding in its own comment that the + /// values "are asserted to agree only because nothing has moved the + /// terminal". An implementation that ignored `tcgetpgrp` and + /// substituted `leader_pid` passed it — so it pinned the substitution + /// as acceptable. + /// + /// **The foreground group is injected, not produced by a shell.** + /// Framing Bet 1 wagered that a real job-control fixture would be + /// deterministic in CI; it is not. `bash -m` diverges reliably on + /// Linux and never on macOS, where CI observed the terminal stay with + /// the leader for a full 10s wait on both legs. The framing's stated + /// fallback is this: pin the divergence at the `signal_target` level + /// with an injected foreground group, and **say plainly that it is + /// weaker** than a real one. + /// + /// What it still proves: the target is read from the *lookup* rather + /// than substituted from the leader, because the two values differ + /// here and the assertion names both. What it no longer proves on its + /// own: that a real shell ever produces that divergence — + /// `job_control_really_diverges_the_foreground_group` carries that, + /// on the platforms where it is real. #[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"); + // A foreground group that is deliberately NOT the leader. + let leader_i32 = i32::try_from(pid).expect("pid fits i32"); + let fg = leader_i32 + 1; + assert_ne!( + fg, leader_i32, + "the injected group must differ from the leader or this test \ + cannot distinguish a substitution" + ); + + sup.force_next_pty_lookup(Ok(fg)); 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)", + "kill: {} (signal=SIGTERM, target=-{fg} 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 \ + "the report names the exact group the lookup returned, the exact \ leader pid, and observes the leader as live" ); + // Stated separately so a regression that reintroduces the + // substitution fails by name rather than inside a long string + // comparison. + assert!( + err.contains(&format!("target=-{fg} via tcgetpgrp")), + "the target must be the group the lookup returned: {err}" + ); + assert!( + !err.contains(&format!("target=-{pid} via tcgetpgrp")), + "the target must NOT be the leader pid: {err}" + ); + let _ = sup.signal(id, Signal::SIGKILL); } + /// Corroboration for the injected divergence above: a **real** shell + /// under job control does hand the terminal to a different process + /// group, and the production lookup reads it. + /// + /// Linux-only by arming. macOS is not a skip-because-untested: CI + /// observed `bash -m` there keep the terminal on the leader for the + /// entire bounded wait, on both legs, so the precondition this test + /// needs genuinely does not hold on that platform. Running it there + /// would assert a false claim about macOS rather than find a bug. + #[test] + fn job_control_really_diverges_the_foreground_group() { + if !std::path::Path::new(BASH).exists() { + let armed = std::env::var_os("PMACS_REQUIRE_BASH").is_some_and(|v| !v.is_empty()); + assert!( + !armed, + "PMACS_REQUIRE_BASH is set but {BASH} does not exist: the \ + job-control divergence fixture cannot run" + ); + eprintln!( + "{BASH} not present; skipping job_control_really_diverges_the_foreground_group" + ); + return; + } + if !cfg!(target_os = "linux") { + eprintln!( + "job control does not hand over the terminal for a \ + non-interactive `bash -m` on this platform; skipping" + ); + return; + } + + let mut sup = ProcessSupervisor::new(); + let (id, pid, fg) = spawn_pty_with_diverged_foreground_group(&mut sup, "diag-jobctl"); + + let leader_i32 = i32::try_from(pid).expect("pid fits i32"); + assert_ne!( + fg, leader_i32, + "a real job-control shell must move the terminal off the leader" + ); + + // Force ONLY the kill failure. The lookup is left alone, so + // `pty_foreground_group` runs for real against a real terminal + // and the report below is built from what it returned. + // + // This is the assertion that makes the injected pin meaningful: + // without it, `pty_foreground_group` could fall back on every + // call and every other test here would still pass, because they + // all supply the group themselves. + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + let expected = format!( + "kill: {} (signal=SIGTERM, target=-{fg} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=live)", + nix::errno::Errno::EPERM + ); + assert_eq!( + err, expected, + "the production lookup must report the real foreground group" + ); + assert!( + !err.contains("pty-leader-fallback"), + "a healthy terminal must not take the fallback branch: {err}" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#DC2 / acceptance 2 — a PTY whose foreground-group lookup fails + /// is distinguishable from a pipe child that never had a terminal. + /// + /// Before this, both rendered "leader-pid". The PTY fallback was + /// therefore invisible: a terminal query that failed, and a process + /// with no terminal at all, produced the same word. Each arm now + /// names its own stage, and `portable-pty`'s + /// `process_group_leader` — which collapses every failure into + /// `None` before pmacs can see it — is bypassed so the errno + /// survives. + #[test] + fn a_pty_foreground_lookup_failure_names_its_stage() { + let arms = [ + (PtyLookupFailure::NoMasterFd, "no-master-fd".to_owned()), + ( + PtyLookupFailure::Duplicate(nix::errno::Errno::EMFILE), + format!("duplicate-master-fd: {}", nix::errno::Errno::EMFILE), + ), + ( + PtyLookupFailure::Query(nix::errno::Errno::ENOTTY), + format!("tcgetpgrp: {}", nix::errno::Errno::ENOTTY), + ), + ( + PtyLookupFailure::NonPositive(0), + "tcgetpgrp-non-positive: 0".to_owned(), + ), + ]; + + for (failure, rendered) in arms { + let mut sup = ProcessSupervisor::new(); + let (id, pid) = spawn_live_pty(&mut sup, "diag-pty-fallback"); + + sup.force_next_pty_lookup(Err(failure)); + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + // The target falls back to the leader — positive, not a + // negated group — and the source says why. + let expected = format!( + "kill: {} (signal=SIGTERM, target={pid} via pty-leader-fallback({rendered}), leader_pid={pid}, leader=live)", + nix::errno::Errno::EPERM + ); + assert_eq!(err, expected, "arm {failure:?} must name its own stage"); + + // And it must NOT read like a pipe child. + assert!( + !err.contains("via leader-pid,"), + "a PTY fallback must not render as a bare pipe leader target: {err}" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + } + + /// The companion half of acceptance 2: a genuine pipe child still + /// renders "leader-pid", so the two really are distinct strings + /// rather than both having moved. + /// + /// Asserted here as well as in the leader-directed test because a + /// rename of one side would otherwise pass every test — the pair is + /// the point, not either string alone. + #[test] + fn a_pipe_child_still_renders_a_bare_leader_target() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-pipe-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::EPERM); + let err = sup.terminate(id).expect_err("injected EPERM must fail"); + + assert!( + err.contains(&format!("target={pid} via leader-pid,")), + "a pipe child with no group renders the bare leader source: {err}" + ); + assert!( + !err.contains("pty-leader-fallback"), + "a pipe child never took the PTY branch: {err}" + ); + + let _ = sup.signal(id, Signal::SIGKILL); + } + + /// Q#DC3 / acceptance 3(a) — the report names the signal, so two + /// failures that differ only in which signal was sent are no longer + /// the same text. + /// + /// **They differ in text only.** Every failed `kill` returns before + /// the fatal-signal branch, so both leave the state and the ledger + /// exactly as they were. That is asserted here rather than assumed, + /// because revision 2 of the framing claimed the opposite. + #[test] + fn a_failed_signal_names_which_signal_and_changes_nothing() { + let mut reports = Vec::new(); + for signal in [Signal::SIGTERM, Signal::SIGUSR1] { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-signal-name", "/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); + + sup.force_next_kill_errno(nix::errno::Errno::EPERM); + let err = sup + .signal(id, signal) + .expect_err("injected EPERM must fail"); + + assert!( + err.contains(&format!("signal={signal:?},")), + "the report must name {signal:?}: {err}" + ); + assert!( + matches!( + sup.processes.get(&id).expect("record").state, + ProcessState::Running { .. } + ), + "a failed {signal:?} must not transition the record" + ); + assert!( + sup.reap_ledger.is_empty(), + "a failed {signal:?} must not arm the ledger" + ); + + reports.push(err.replace(&format!("{pid}"), "")); + let _ = nix::sys::signal::kill( + Pid::from_raw(-i32::try_from(pid).unwrap()), + Signal::SIGKILL, + ); + } + + assert_ne!( + reports[0], reports[1], + "SIGTERM and SIGUSR1 failures must no longer be identical text" + ); + } + + /// Q#DC3 / acceptance 3(b) — the disposition control. A *successful* + /// non-fatal signal changes nothing, while a *successful* fatal one + /// transitions the record and arms the ledger. + /// + /// This is the check that gives the previous test its meaning: it + /// shows the fatal/non-fatal distinction is real, and therefore that + /// "failed signals are disposition-identical" is a statement about + /// the failure path rather than about signals generally. + #[test] + fn a_successful_signal_disposition_depends_on_whether_it_is_fatal() { + let mut sup = ProcessSupervisor::new(); + let mut spec = ProcessSpec::new("diag-disposition-live", "/bin/sh"); + // Ignore USR1 so the successful non-fatal signal cannot end the + // child and confuse the state assertion with a real exit. + spec.args = vec!["-c".into(), "trap '' USR1; sleep 30".into()]; + spec.group = true; + let id = sup.spawn(spec).expect("spawn"); + let pid = spawn_started_pid(&mut sup, id); + + sup.signal(id, Signal::SIGUSR1).expect("USR1 delivers"); + assert!( + matches!( + sup.processes.get(&id).expect("record").state, + ProcessState::Running { .. } + ), + "a successful non-fatal signal leaves the record Running" + ); + assert!( + sup.reap_ledger.is_empty(), + "a successful non-fatal signal arms no ledger entry" + ); + + sup.terminate(id).expect("TERM delivers"); + assert!( + matches!( + sup.processes.get(&id).expect("record").state, + ProcessState::Exiting { .. } + ), + "a successful fatal signal transitions the record to Exiting" + ); + assert!( + !sup.reap_ledger.is_empty(), + "a successful fatal signal arms the group reap ledger" + ); + + let _ = + nix::sys::signal::kill(Pid::from_raw(-i32::try_from(pid).unwrap()), Signal::SIGKILL); + } + + /// Q#DC4 / acceptance 4 — the measured group is a real observation, + /// not a restatement of the input. + /// + /// `expected_group` is `-leader_pid` by construction, so on the + /// spawn-group path it can never disagree with the target. The + /// measured field is the only one that can, and this proves it does: + /// a child placed into an *anchor* group reports that group, not its + /// own pid. + /// + /// Without this the field would be exactly the vacuous readout the + /// framing was written to eliminate — an implementation returning + /// `-pid` unconditionally would satisfy every other test. + #[test] + fn the_measured_group_reports_the_real_group_not_the_pid() { + use std::os::unix::process::CommandExt as _; + + // An anchor process leading its own group. + let mut anchor = std::process::Command::new("/bin/sleep"); + anchor.arg("30"); + anchor.process_group(0); + let mut anchor = anchor.spawn().expect("spawn anchor"); + let anchor_pgid = i32::try_from(anchor.id()).expect("pid fits i32"); + + // A second process placed INTO the anchor's group, so its pgid + // is genuinely not its own pid. + let mut joiner = std::process::Command::new("/bin/sleep"); + joiner.arg("30"); + joiner.process_group(anchor_pgid); + let mut joiner = joiner.spawn().expect("spawn joiner"); + let joiner_pid = joiner.id(); + + assert_ne!( + i32::try_from(joiner_pid).unwrap(), + anchor_pgid, + "precondition: the joiner must not be the anchor itself" + ); + + let rendered = measured_group_of(joiner_pid); + assert_eq!( + rendered, + format!(", measured_group=-{anchor_pgid}"), + "the measurement must report the group the kernel actually has" + ); + assert_ne!( + rendered, + format!(", measured_group=-{joiner_pid}"), + "and must NOT restate the pid it was given" + ); + + let _ = joiner.kill(); + let _ = joiner.wait(); + let _ = anchor.kill(); + let _ = anchor.wait(); + } + /// 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. @@ -2432,7 +3118,7 @@ mod tests { 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)", + "kill: {} (signal=SIGTERM, target={pid} via leader-pid, leader_pid={pid}, leader=live)", nix::errno::Errno::ESRCH ); assert_eq!( @@ -2482,7 +3168,7 @@ mod tests { 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))", + "kill: {} (signal=SIGTERM, target={pid} via leader-pid, leader_pid={pid}, leader=exited(code 3))", nix::errno::Errno::EPERM ); assert_eq!( @@ -2514,7 +3200,7 @@ mod tests { 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)", + "kill: {} (signal=SIGTERM, target=-{pid} via group, leader_pid={pid}, expected_group=-{pid}, measured_group=-{pid}, leader=live)", nix::errno::Errno::EPERM ); assert_eq!(err, expected, "a group=true pipe child reports via group");