From 76cb13bda8b1e3bdc3abf884119c91efde54532e Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 11:39:40 -0400 Subject: [PATCH 1/9] docs(process): frame the signal diagnostic completeness lane (rev 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage B of the lane that merged as PR #176. Stage A parked every tolerance rule behind one condition — evidence — and the macOS failure on PR #191 is the first occurrence carrying Stage A's self-describing format, so the evidence now exists. It does not support any parked rule, and per review round 1 it does not support an identity claim either. This lane is evidence collection only: no tolerance rule, no retargeting, no disposition change. Round 1 raised two blocking and two major findings. All four are accepted, and all four were verified against the code first: - the landed acceptance at src/process.rs:2400 asserts the same pid three times, so substituting leader_pid for tcgetpgrp would pass it; - a numeric pgid cannot establish group identity, and no portable mechanism can (pidfd covers processes, not groups; macOS has none); - the PTY foreground-lookup fallback renders identically to a normal pipe child, and portable-pty discards the tcgetpgrp errno before pmacs sees it; - the report never names the signal, though disposition differs between fatal and non-fatal ones. Revision 1 was written to a session scratchpad rather than a branch, so it was not on githubsucks and review necessarily landed on Stage A's merged document instead. That is recorded in the revision history: work is portable only after it is committed and pushed, and this lane broke that rule on its first step. No code changes. Framing only, awaiting review round 2. --- ...-signal-diagnostic-completeness-framing.md | 338 ++++++++++++++++++ 1 file changed, 338 insertions(+) create mode 100644 docs/process-signal-diagnostic-completeness-framing.md diff --git a/docs/process-signal-diagnostic-completeness-framing.md b/docs/process-signal-diagnostic-completeness-framing.md new file mode 100644 index 0000000..37dbb95 --- /dev/null +++ b/docs/process-signal-diagnostic-completeness-framing.md @@ -0,0 +1,338 @@ +# Framing — make the signal diagnostic discriminating (evidence collection) + +**Revision 2.** Status: awaiting review round 2. Lane: +`process-signal-diagnostic-completeness`, worktree +`../pmacs-signal-identity`, based on `githubsucks/main` @ `391d38a`. + +**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 one +condition: evidence. Evidence has arrived (§1.2). It does not support any +of the parked rules, and — per review round 1 — it does not support an +identity claim either. + +**Evidence collection only. No tolerance rule, no change to which +process gets signalled, no disposition change.** Everything behavioural +is parked in §5. + +## Revision history + +**Revision 1 → 2**, after review round 1 (two blocking, two major). All +four accepted; all four verified against the code before acceptance. + +- **Rev 1 proposed measuring the pgid and then *targeting* it.** That is + a behavioural change resting on an identity claim a numeric pgid cannot + support (§1.5). Retargeting is parked (§5); this lane only *records*. +- **Rev 1 repeated the defect it was written to fix.** Its Bet 1 asserted + `getpgid(child) == pid`, which an implementation that ignored `getpgid` + and returned `pid` would satisfy — the same non-discrimination the + review found in the landed acceptance (§1.4). Every measurement in this + revision now requires a case where the two values **differ**. +- **Rev 1 scoped the PTY path out.** Review showed the PTY fallback is + indistinguishable from a normal pipe child in the report (§1.6), which + is a defect in the diagnostic this lane owns. Scope now includes it, + and the rev 1 sentence "this lane does not touch the PTY path" is + withdrawn. +- **Rev 1 never noticed the report omits the signal** (§1.7). + +**Rev 1 also mis-stated where the lane lived.** It was written to a +session scratchpad, which is not portable and was not on `githubsucks`, +so review round 1 necessarily landed on Stage A's revision 4 instead. +That is why round 1's line references point at the merged document. The +findings apply regardless — three of the four are defects in code that is +on `main` right now — but the process error is recorded here because +"work is portable only after it is committed and pushed" is a standing +project rule and this lane broke it 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 and no + behavioural change.** +- **Serves §9 (worker model), failure attribution.** Stage A made the + failure describe itself; this lane makes the description + *discriminating* — today several distinct failures render identically. +- **Interaction islands: none. Config registry: not adopted. + Background-work attribution: unchanged.** +- **No audited claim in COHERENCE.md changes**, so under §25 no + COHERENCE edit rides this PR. + + +## 1. Ground truth (verified at `391d38a`) + +### 1.1 Stage A landed and has now fired + +`signal_failure_report` and `LeaderObservation` merged as **PR #176 on +2026-07-26** (`62316a9`). The occurrence below is the first failure +carrying the new format rather than a bare errno. Stage A is the reason +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. **The 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` sets it), **not** the +PTY path; and `leader=live`, from a real `try_wait` against the real +child, so the leader had not exited and had not been reaped. + +### 1.3 Two written premises are falsified + +- **`src/process.rs:1246-1247`** — `tick_reap_ledger` justifies treating + any probe error as "nothing left we can reach" with the comment + "**EPERM cannot happen for our own children**". §1.2 is a + counterexample: our own child, alive, EPERM. +- **Stage A §1.3** — "the reap ledger is disjoint from this path", on the + grounds that the ledger arms only for `proc.spec.group` and PTY mode + cannot set it. §1.2's process **is** a `spec.group` process. Not + disjoint. + +Stage A's §1.2 entity-split analysis concerns the PTY path, where the +target is read from `tcgetpgrp`. **It does not apply to §1.2's +occurrence**, where the target is computed as `-leader_pid` with no read. + +### 1.4 The landed acceptance cannot discriminate (round 1, P1) + +`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` entirely 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 whole diagnostic is that these two entities can +diverge, and no test exercises a case where they do. This is the +vacuous-assertion family already recorded in the handoff. + +### 1.5 A numeric pgid cannot establish identity (round 1, P1) + +Rev 1 proposed reading the real pgid and targeting it. Review is right +that this does not establish identity: the value is read before `kill`, +the read-then-kill window remains, and a *number* cannot distinguish the +original group from a recycled one. + +**There is no portable mechanism that 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 at all. +Since the failures are macOS-only so far, no available mechanism makes +group signalling identity-safe. + +**Therefore this lane records and does not retarget.** Acceptance +criteria state what was observed, never that the observation is +sufficient. + +One narrowing fact, stated because it constrains the hypothesis space +and *not* as an identity claim: 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 other process can be +assigned it, so no *new* group can be created bearing that pgid value +during the window. This narrows recycling as a candidate **for that one +occurrence**; it says nothing about whether the group still contained a +signallable member, which is the question EPERM actually turns on. + +### 1.6 The PTY fallback is invisible in the report (round 1, P2) + +`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 different situations, one string. + +`portable-pty`'s implementation +(`portable-pty-0.9.0/src/unix.rs:374`) is: + +```rust +fn process_group_leader(&self) -> Option { + match unsafe { libc::tcgetpgrp(self.fd.0.as_raw_fd()) } { + pid if pid > 0 => Some(pid), + _ => None, + } +} +``` + +The errno is discarded, so "the tty has no foreground group" and +"`tcgetpgrp` failed" are already merged before pmacs sees the result. + +**`nix::unistd::tcgetpgrp` returns `Result`** (`nix-0.29.0/src/unistd.rs:368`, +ungated, and `pub mod unistd` at `lib.rs:183` is unconditional), so pmacs +can make this call itself and keep the errno **without `unsafe`** — which +matters because the crate is `#![forbid(unsafe_code)]`. + +### 1.7 The report omits which signal failed (round 1, P2) + +`signal_failure_report` (`:830-850`) takes target, leader pid, errno and +leader observation. **Not the signal.** `signal` (`:1074`) has it. + +This is not cosmetic: `signal` transitions state and arms the reap ledger +only for `SIGTERM | SIGKILL | SIGHUP` (`:1099-1116`), and the public Lua +surface accepts INT, USR1, USR2 and QUIT as well (`src/lua_bindings/mod.rs:8627-8629`). +A failed `SIGUSR1` and a failed `SIGTERM` have different consequences and +currently produce indistinguishable text. + +### 1.8 The disposition consequence, worse for the first call + +`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 already `Exiting`, ledger already armed, so SIGKILL escalation still happens. | +| The **first** one | State stays `Running`, ledger never armed. **No escalation is ever scheduled** — the child is abandoned. | + +Only the recoverable variant has been observed. This lane **pins** the +first-call variant and changes nothing about it (§5). + +### 1.9 Limits of the evidence + +- **Not reproduced locally.** Development is Linux; the failures are + macOS-only. No claim in this lane rests on a local repro of the EPERM. +- **Frequency: two occurrences, in different paths** — PR #172 was the + PTY path (`acc28`, luajit), §1.2 the group path (lua54). This is not + one flaky test. +- **The mechanism is not established**, and this lane does not propose + one. That is the point of the split. + + +## 2. Questions + +- **Q#DC1** — Can the two entities be made to diverge in a test? *Yes: + under a PTY with job control enabled, a shell places a background job + in its own process group and hands it the terminal, so `tcgetpgrp` != + leader pid. §3 Bet 1 builds exactly that.* +- **Q#DC2** — Should the PTY fallback get its own `TargetSource`? + *Proposed: yes, and pmacs should call `nix::unistd::tcgetpgrp` itself so + the fallback can report the errno rather than inheriting portable-pty's + discarded `None`.* +- **Q#DC3** — Should the report name the signal? *Proposed: yes, with a + contrasting non-fatal signal tested.* +- **Q#DC4** — Should the measured pgid be reported for `spec.group` + children? *Proposed: yes, as an observation clearly distinct from the + assumed value, and with no claim of sufficiency (§1.5).* +- **Q#DC5** — Should anything be retargeted or tolerated? **No. Parked.** + + +## 3. Bets + +Each bet names what falsifies it and what falsification teaches. + +- **Bet 1 — the divergence is constructible.** A PTY fixture where the + foreground group is *not* the leader: job control on, a background job + given the terminal. The rewritten acceptance asserts both exact values + and that they **differ**. + - *Falsified if* the fixture cannot be made deterministic in CI (shell + job-control timing). Then the lane says so and falls back to pinning + divergence at the `signal_target` unit level with an injected + foreground group, which is weaker and must be labelled as weaker. + - This is the finding that matters most: without it, the entire + diagnostic remains unverified in the only case it exists for. + +- **Bet 2 — the PTY fallback is reachable and distinguishable.** A test + drives the branch where the foreground-group lookup fails and asserts a + source string distinct from a pipe child's. + - *Falsified if* the branch cannot be reached without faking the + lookup. Then the seam is made injectable exactly as Stage A made the + kill injectable (Q#PD4), and that is stated rather than hidden. + +- **Bet 3 — naming the signal is free.** Thread `signal` into the report. + - *Falsified if* any existing exact-string test cannot be updated + mechanically. Those four sites (`:2408`, `:2435`, `:2485`, `:2517`) + are the highest-risk part of the diff: **a wholesale rewrite of + expected strings is how a format regression hides**, so each is + updated individually and listed in the PR body with before and after. + +- **Bet 4 — the measured pgid can disagree with the assumed one, and the + test proves the measurement is real.** A child that calls `setsid`, so + its pgid is genuinely not its parent-assumed value, is measured and the + two values asserted **different**. + - *Falsified if* no such case can be built — in which case the + measurement is unfalsifiable and should not ship, exactly per §1.4's + lesson. + +- **Bet 5 — §1.8's first-call abandonment is real.** Inject EPERM on the + *first* terminate; assert no ledger entry and state still `Running`. + - *Falsified if* the ledger is armed anyway, meaning §1.8 misreads + `signal`. + - **Pins current behaviour; does not fix it.** + + +## 4. Acceptance + +1. A PTY job-control fixture in which `tcgetpgrp` != leader pid, with + both exact values asserted and asserted to differ. The landed test at + `:2400` is **rewritten**, not supplemented, since it currently pins a + substitution as acceptable. +2. The PTY foreground-lookup fallback reports a source distinct from a + pipe child's leader-pid, with a test driving the real branch. +3. The report names the signal; at least one non-fatal signal + (`SIGUSR1`) is tested alongside `SIGTERM`, including that its + disposition differs. +4. For `spec.group` children the report carries the measured pgid as a + field distinct from the assumed one, renderable as unobservable, and + a test asserts a case where they **differ**. +5. Every exact-string test updated individually, each listed in the PR + body with before and after. No blanket rewrite. +6. §1.8's first-call abandonment pinned, labelled as pinning a known gap. +7. `docs/agent-handoff.md` records that "EPERM cannot happen for our own + children" is false, with the run link; the comment at `:1246` is + corrected in the same PR. +8. **No acceptance claims the telemetry establishes group identity.** + §1.5 governs; the PR body repeats it. + + +## 5. Parked + +- **Retargeting to the measured pgid.** Behavioural, and unsupported by + §1.5. Needs this lane's evidence first. +- **Any tolerance rule for EPERM or ESRCH.** Unmotivated across Stage A's + three revisions and still unmotivated. +- **§1.8's first-call abandonment.** Its own lane; disposition change. +- **Q#PS6** — `terminate` on an already-reaped process returning `Ok`. +- **`signal_target`'s read-then-kill of `tcgetpgrp`** — Stage A called it + "the most likely real fix site, still unframed". Still is. 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), unrelated to signals. + + +## 6. Gates + +Standard suite, each its own step with a real exit status, 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 reasons unrelated to this +lane. + +**PTY job-control tests are the load-sensitive kind.** They are run +repeatedly, and the PR body records the repetition count rather than a +single green. + + +## 7. Branch plan + +One branch, one PR. Order: Bet 1 first and alone, because it is the +finding that decides whether the diagnostic is worth extending at all. If +the divergence fixture cannot be made deterministic, the rest of the lane +is re-scoped rather than pushed through. From 872e4aacad6d13340fb9113f786a17f2aa98616f Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 12:09:34 -0400 Subject: [PATCH 2/9] =?UTF-8?q?docs(process):=20framing=20revision=203=20?= =?UTF-8?q?=E2=80=94=20close=20review=20round=202?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three blocking and three major findings, all accepted, all verified in the code before acceptance. - The PTY errno proposal is withdrawn. `nix::unistd::tcgetpgrp` needs `F: AsFd`; `MasterPty` exposes only `Option`; every route from a raw fd to `AsFd` is unsafe and the crate forbids unsafe. The claim is reduced to distinguishing the fallback without the errno. - Revision 2 called `getpgid` ungated. It is not: the gate is a block-level `feature!` opened 168 lines above the function, and rev 2 read the four lines immediately above it. Both calls are available because nix's own `signal` feature depends on `process`, verified with `cargo tree` and by compiling them. The lane now declares `process` explicitly rather than inheriting it. - Bet 4's `setsid` fixture was impossible: a `spec.group` child is already a process-group leader and a group leader's `setsid` fails with EPERM. Replaced with a `setpgid`-into-an-existing-group fixture with a readiness handshake and explicit cleanup. - "Recoverable" is withdrawn. `tick_reap_ledger` drops its entry on any probe error — including the EPERM this occurrence proves reachable — and discards the SIGKILL result while marking the entry killed. The claim is now that escalation remains scheduled, and that still-silent path is parked as its own lane. - Revision 2 falsified the wrong sentence. Stage A's disjointness claim was about the PTY path and remains true; what the occurrence falsifies is the ledger's own EPERM comment. - Failed SIGUSR1 and SIGTERM are disposition-identical, because every failed kill returns before the fatal-signal branch. Acceptance 3 now separates the failure-format comparison from a successful-call disposition control. - Bet 5 proposed a test that already exists at src/process.rs:2501. It is cited as ground truth and retained, not invented. No code changes. Framing only, awaiting review round 3. --- ...-signal-diagnostic-completeness-framing.md | 453 ++++++++++-------- 1 file changed, 261 insertions(+), 192 deletions(-) diff --git a/docs/process-signal-diagnostic-completeness-framing.md b/docs/process-signal-diagnostic-completeness-framing.md index 37dbb95..7d98975 100644 --- a/docs/process-signal-diagnostic-completeness-framing.md +++ b/docs/process-signal-diagnostic-completeness-framing.md @@ -1,107 +1,120 @@ # Framing — make the signal diagnostic discriminating (evidence collection) -**Revision 2.** Status: awaiting review round 2. Lane: +**Revision 3.** Status: awaiting review round 3. Lane: `process-signal-diagnostic-completeness`, worktree -`../pmacs-signal-identity`, based on `githubsucks/main` @ `391d38a`. +`../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 one -condition: evidence. Evidence has arrived (§1.2). It does not support any -of the parked rules, and — per review round 1 — it does not support an -identity claim either. +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 parked in §5. +**Evidence collection only. No tolerance rule, no change to which process +gets signalled, no disposition change.** Everything behavioural is in §5. ## Revision history -**Revision 1 → 2**, after review round 1 (two blocking, two major). All -four accepted; all four verified against the code before acceptance. +**Revision 2 → 3**, after review round 2 (three blocking, three major). +All six accepted; all six verified in the code before acceptance. -- **Rev 1 proposed measuring the pgid and then *targeting* it.** That is - a behavioural change resting on an identity claim a numeric pgid cannot - support (§1.5). Retargeting is parked (§5); this lane only *records*. -- **Rev 1 repeated the defect it was written to fix.** Its Bet 1 asserted - `getpgid(child) == pid`, which an implementation that ignored `getpgid` - and returned `pid` would satisfy — the same non-discrimination the - review found in the landed acceptance (§1.4). Every measurement in this - revision now requires a case where the two values **differ**. -- **Rev 1 scoped the PTY path out.** Review showed the PTY fallback is - indistinguishable from a normal pipe child in the report (§1.6), which - is a defect in the diagnostic this lane owns. Scope now includes it, - and the rev 1 sentence "this lane does not touch the PTY path" is - withdrawn. -- **Rev 1 never noticed the report omits the signal** (§1.7). +- **The PTY errno proposal had no safe fd bridge.** Withdrawn and reduced + (§1.6). This was rev 2's central new proposal and it does not survive + `#![forbid(unsafe_code)]`. +- **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 the EPERM this occurrence proves can + happen — 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. -**Rev 1 also mis-stated where the lane lived.** It was written to a -session scratchpad, which is not portable and was not on `githubsucks`, -so review round 1 necessarily landed on Stage A's revision 4 instead. -That is why round 1's line references point at the merged document. The -findings apply regardless — three of the four are defects in code that is -on `main` right now — but the process error is recorded here because -"work is portable only after it is committed and pushed" is a standing -project rule and this lane broke it on its first step. +**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 and no + 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* — today several distinct failures render identically. + *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**, so under §25 no - COHERENCE edit rides this PR. +- **No audited claim in COHERENCE.md changes**; under §25 no COHERENCE + edit rides this PR. -## 1. Ground truth (verified at `391d38a`) +## 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`). The occurrence below is the first failure -carrying the new format rather than a bare errno. Stage A is the reason -this document can exist. +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. **The rerun of the identical head passed 12/12**, -so the failure is intermittent, not deterministic: +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` sets it), **not** the -PTY path; and `leader=live`, from a real `try_wait` against the real -child, so the leader had not exited and had not been reaped. +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 Two written premises are falsified +### 1.3 What is actually falsified -- **`src/process.rs:1246-1247`** — `tick_reap_ledger` justifies treating - any probe error as "nothing left we can reach" with the comment - "**EPERM cannot happen for our own children**". §1.2 is a +- **`src/process.rs:1246-1247` is falsified.** `tick_reap_ledger` + justifies treating any probe error as "nothing left we can reach" with + the comment "**EPERM cannot happen for our own children**". §1.2 is a counterexample: our own child, alive, EPERM. -- **Stage A §1.3** — "the reap ledger is disjoint from this path", on the - grounds that the ledger arms only for `proc.spec.group` and PTY mode - cannot set it. §1.2's process **is** a `spec.group` process. Not - disjoint. +- **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 §1.2 entity-split analysis concerns the PTY path, where the -target is read from `tcgetpgrp`. **It does not apply to §1.2's -occurrence**, where the target is computed as `-leader_pid` with no read. +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 (round 1, P1) +### 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 @@ -111,50 +124,71 @@ target=-{pid} via tcgetpgrp, leader_pid={pid}, expected_group=-{pid}, leader=liv ``` — the same `pid` three times. **An implementation that ignored -`tcgetpgrp` entirely 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." +`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. -The premise of the whole diagnostic is that these two entities can -diverge, and no test exercises a case where they do. This is the -vacuous-assertion family already recorded in the handoff. +### 1.5 A numeric pgid cannot establish identity -### 1.5 A numeric pgid cannot establish identity (round 1, P1) +The value is read before `kill`, the window remains open, and a *number* +cannot distinguish the original group from a recycled one. -Rev 1 proposed reading the real pgid and targeting it. Review is right -that this does not establish identity: the value is read before `kill`, -the read-then-kill window remains, 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. -**There is no portable mechanism that 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 at all. -Since the failures are macOS-only so far, no available mechanism makes -group signalling identity-safe. +**This lane therefore records and does not retarget.** No acceptance +claims the telemetry is sufficient. -**Therefore this lane records and does not retarget.** Acceptance -criteria state what was observed, never that the observation 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. -One narrowing fact, stated because it constrains the hypothesis space -and *not* as an identity claim: 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 other process can be -assigned it, so no *new* group can be created bearing that pgid value -during the window. This narrows recycling as a candidate **for that one -occurrence**; it says nothing about whether the group still contained a -signallable member, which is the question EPERM actually turns on. +### 1.5a The nix surface is gated, and available for a non-obvious reason -### 1.6 The PTY fallback is invisible in the report (round 1, P2) +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, and the errno cannot be recovered `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 different situations, one string. +pipe child renders identically.** Two situations, one string. -`portable-pty`'s implementation -(`portable-pty-0.9.0/src/unix.rs:374`) is: +`portable-pty` (`portable-pty-0.9.0/src/unix.rs:374`) discards the errno: ```rust fn process_group_leader(&self) -> Option { @@ -165,155 +199,193 @@ fn process_group_leader(&self) -> Option { } ``` -The errno is discarded, so "the tty has no foreground group" and -"`tcgetpgrp` failed" are already merged before pmacs sees the result. +**Rev 2 proposed calling `nix::unistd::tcgetpgrp` ourselves to keep the +errno. That is withdrawn.** `tcgetpgrp` requires `F: AsFd`, and +`MasterPty` exposes only `fn as_raw_fd(&self) -> Option` +(`portable-pty-0.9.0/src/lib.rs:114`). Every route from a `RawFd` to +something implementing `AsFd` — `BorrowedFd::borrow_raw`, +`OwnedFd::from_raw_fd`, `File::from_raw_fd` — is `unsafe`, and the crate +is `#![forbid(unsafe_code)]`. There is no safe bridge, and duplicating +the fd does not create one because the duplication itself needs the same +unsafe conversion. -**`nix::unistd::tcgetpgrp` returns `Result`** (`nix-0.29.0/src/unistd.rs:368`, -ungated, and `pub mod unistd` at `lib.rs:183` is unconditional), so pmacs -can make this call itself and keep the errno **without `unsafe`** — which -matters because the crate is `#![forbid(unsafe_code)]`. +**Reduced claim:** the fallback is distinguished **without** the errno. +`None` is all pmacs can observe, and the report says exactly that. The +errno is recorded here as unavailable-by-construction so a later lane +does not re-propose it. -### 1.7 The report omits which signal failed (round 1, P2) +### 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. +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. -This is not cosmetic: `signal` transitions state and arms the reap ledger -only for `SIGTERM | SIGKILL | SIGHUP` (`:1099-1116`), and the public Lua -surface accepts INT, USR1, USR2 and QUIT as well (`src/lua_bindings/mod.rs:8627-8629`). -A failed `SIGUSR1` and a failed `SIGTERM` have different consequences and -currently produce indistinguishable text. +**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, worse for the first call +### 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 already `Exiting`, ledger already armed, so SIGKILL escalation still happens. | +| 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. | -Only the recoverable variant has been observed. This lane **pins** the -first-call variant and changes nothing about it (§5). +**Rev 2 called the first row "recoverable" and claimed "SIGKILL +escalation still happens". Unsupported.** `tick_reap_ledger` +(`:1249-1254`): -### 1.9 Limits of the evidence +```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 +} +``` -- **Not reproduced locally.** Development is Linux; the failures are - macOS-only. No claim in this lane rests on a local repro of the EPERM. -- **Frequency: two occurrences, in different paths** — PR #172 was the - PTY path (`acc28`, luajit), §1.2 the group path (lua54). This is not - one flaky test. +Since §1.3 falsifies the premise that EPERM cannot occur here, an EPERM +probe **drops the entry and cancels escalation silently**, and a failed +`SIGKILL` is recorded as if it succeeded. So the honest statement is that +escalation remains *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. That is the point of the split. + one. ## 2. Questions - **Q#DC1** — Can the two entities be made to diverge in a test? *Yes: - under a PTY with job control enabled, a shell places a background job - in its own process group and hands it the terminal, so `tcgetpgrp` != - leader pid. §3 Bet 1 builds exactly that.* + under a PTY with job control on, a shell places a background job in its + own process group and gives it the terminal, so `tcgetpgrp` != leader + pid.* - **Q#DC2** — Should the PTY fallback get its own `TargetSource`? - *Proposed: yes, and pmacs should call `nix::unistd::tcgetpgrp` itself so - the fallback can report the errno rather than inheriting portable-pty's - discarded `None`.* -- **Q#DC3** — Should the report name the signal? *Proposed: yes, with a - contrasting non-fatal signal tested.* + *Proposed: yes, reporting only that the lookup yielded `None` (§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 clearly distinct from the - assumed value, and with no claim of sufficiency (§1.5).* -- **Q#DC5** — Should anything be retargeted or tolerated? **No. Parked.** + 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 -Each bet names what falsifies it and what falsification teaches. - - **Bet 1 — the divergence is constructible.** A PTY fixture where the - foreground group is *not* the leader: job control on, a background job + foreground group is not the leader: job control on, a background job given the terminal. The rewritten acceptance asserts both exact values - and that they **differ**. - - *Falsified if* the fixture cannot be made deterministic in CI (shell - job-control timing). Then the lane says so and falls back to pinning - divergence at the `signal_target` unit level with an injected - foreground group, which is weaker and must be labelled as weaker. - - This is the finding that matters most: without it, the entire - diagnostic remains unverified in the only case it exists for. + **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. + - Without this the diagnostic remains unverified in the only case it + exists for. - **Bet 2 — the PTY fallback is reachable and distinguishable.** A test - drives the branch where the foreground-group lookup fails and asserts a - source string distinct from a pipe child's. + drives the branch where the lookup returns `None` and asserts a source + string distinct from a pipe child's. - *Falsified if* the branch cannot be reached without faking the - lookup. Then the seam is made injectable exactly as Stage A made the - kill injectable (Q#PD4), and that is stated rather than hidden. + 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 existing exact-string test cannot be updated - mechanically. Those four sites (`:2408`, `:2435`, `:2485`, `:2517`) - are the highest-risk part of the diff: **a wholesale rewrite of - expected strings is how a format regression hides**, so each is - updated individually and listed in the PR body with before and after. + - *Falsified if* any exact-string test cannot be updated mechanically. -- **Bet 4 — the measured pgid can disagree with the assumed one, and the - test proves the measurement is real.** A child that calls `setsid`, so - its pgid is genuinely not its parent-assumed value, is measured and the - two values asserted **different**. - - *Falsified if* no such case can be built — in which case the - measurement is unfalsifiable and should not ship, exactly per §1.4's - lesson. - -- **Bet 5 — §1.8's first-call abandonment is real.** Inject EPERM on the - *first* terminate; assert no ledger entry and state still `Running`. - - *Falsified if* the ledger is armed anyway, meaning §1.8 misreads - `signal`. - - **Pins current behaviour; does not fix it.** +- **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. A PTY job-control fixture in which `tcgetpgrp` != leader pid, with - both exact values asserted and asserted to differ. The landed test at - `:2400` is **rewritten**, not supplemented, since it currently pins a - substitution as acceptable. +1. A PTY job-control fixture where `tcgetpgrp` != leader pid, both exact + values asserted and asserted to differ. The test at `:2400` is + **rewritten**, not supplemented — it currently pins a substitution as + acceptable. 2. The PTY foreground-lookup fallback reports a source distinct from a - pipe child's leader-pid, with a test driving the real branch. -3. The report names the signal; at least one non-fatal signal - (`SIGUSR1`) is tested alongside `SIGTERM`, including that its - disposition differs. + pipe child's, with a test driving the real branch. **No errno claim** + (§1.6). +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, and - a test asserts a case where they **differ**. -5. Every exact-string test updated individually, each listed in the PR - body with before and after. No blanket rewrite. -6. §1.8's first-call abandonment pinned, labelled as pinning a known gap. -7. `docs/agent-handoff.md` records that "EPERM cannot happen for our own + field distinct from the assumed one, renderable as unobservable, with + a test asserting a case where they **differ** (§3 Bet 4). +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 (§1.5a). +8. `docs/agent-handoff.md` records that "EPERM cannot happen for our own children" is false, with the run link; the comment at `:1246` is corrected in the same PR. -8. **No acceptance claims the telemetry establishes group identity.** - §1.5 governs; the PR body repeats it. +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 -- **Retargeting to the measured pgid.** Behavioural, and unsupported by - §1.5. Needs this lane's evidence first. +- **The reap ledger's silent cancellation** (§1.8): an EPERM probe drops + the entry and a failed `SIGKILL` is marked as killed. Now known + reachable. **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.** Its own lane; disposition change. +- **§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 called it - "the most likely real fix site, still unframed". Still is. This lane - makes it *observable*, not fixed. +- **`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), unrelated to signals. + `~/.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, nothing after -the command that could mask it: `cargo fmt --check`; `cargo clippy +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); @@ -322,17 +394,14 @@ test --lib --features crdt`; `compile_mode_acceptance`; `git diff --check`. **All local runs use an isolated `XDG_CONFIG_HOME`** — without it -`compile_mode_acceptance` fails 11 tests for reasons unrelated to this -lane. +`compile_mode_acceptance` fails 11 tests for unrelated reasons. -**PTY job-control tests are the load-sensitive kind.** They are run -repeatedly, and the PR body records the repetition count rather than a -single green. +**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. Order: Bet 1 first and alone, because it is the -finding that decides whether the diagnostic is worth extending at all. If -the divergence fixture cannot be made deterministic, the rest of the lane -is re-scoped rather than pushed through. +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. From df508807629103d4131126be62c0b7aae066ca3f Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 12:28:48 -0400 Subject: [PATCH 3/9] =?UTF-8?q?docs(process):=20framing=20revision=204=20?= =?UTF-8?q?=E2=80=94=20correct=20review=20assumptions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Integrate canonical main, restore PTY errno capture through the safe filedescriptor duplication bridge, narrow the macOS occurrence to what it actually measured, and make the job-control fixture foreground and discriminating. Record the portable framing lane and exact recovery commands in the active-work ledger. --- docs/active-work.md | 41 ++++- ...-signal-diagnostic-completeness-framing.md | 164 +++++++++++++----- 2 files changed, 154 insertions(+), 51 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index a863a4f..99613f7 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -1,9 +1,9 @@ # Active work — cross-machine resume ledger -**Snapshot: 2026-07-29.** 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. +**Snapshot: 2026-07-29; process-signal lane updated 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 @@ -110,6 +110,39 @@ declares canonical will pass on a tree the rest of this file does not describe. If it does not, stop and repair the remote/fetch configuration. +## Process-signal diagnostic completeness — REVISION 4 AWAITING REVIEW + +- **Portable branch:** `githubsucks/process-signal-diagnostic-completeness`; + worktree `../pmacs-signal-identity`. Framing only, no runtime changes + and no PR. The governing document is + `docs/process-signal-diagnostic-completeness-framing.md`, revision 4. +- **State:** canonical `githubsucks/main` @ `4cd4a7b` is integrated. + Revision 4 is awaiting review round 4. The branch corrects revision + 3's claimed base, its overstatement of what the macOS group-target + EPERM established, its foreground/background job-control fixture, and + its missed safe PTY-fd bridge. `filedescriptor::OwnedHandle::dup` + preserves the `tcgetpgrp` errno without `unsafe` in pmacs. +- **Boundary:** evidence collection only. No signal retargeting, + tolerance, disposition change, or reap-ledger repair is authorized. + Group identity remains unprovable; the ledger's silent cancellation is + parked as its own lane. +- **Verification:** exact `portable-pty 0.9.0`, `filedescriptor 0.8.3`, + nix 0.29.0, `src/process.rs`, and branch-ancestry surfaces were read. + Documentation-only verification is `git diff --check` plus the + canonical-main ancestry check; no runtime gate is claimed for a + framing-only revision. +- **Recovery from a clean checkout:** + + ```sh + git fetch githubsucks --prune + git worktree add ../pmacs-signal-identity \ + -b process-signal-diagnostic-completeness \ + githubsucks/process-signal-diagnostic-completeness + cd ../pmacs-signal-identity + git merge-base --is-ancestor 4cd4a7b HEAD + git status --short --branch + ``` + ## The CRDT half of the test corpus is dark in CI — NEEDS A LANE - **No branch, no framing yet.** Found while gating #166, then measured diff --git a/docs/process-signal-diagnostic-completeness-framing.md b/docs/process-signal-diagnostic-completeness-framing.md index 7d98975..1326292 100644 --- a/docs/process-signal-diagnostic-completeness-framing.md +++ b/docs/process-signal-diagnostic-completeness-framing.md @@ -1,6 +1,6 @@ # Framing — make the signal diagnostic discriminating (evidence collection) -**Revision 3.** Status: awaiting review round 3. Lane: +**Revision 4.** Status: awaiting review round 4. 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). @@ -17,12 +17,38 @@ gets signalled, no disposition change.** Everything behavioural is in §5. ## Revision history +**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. -- **The PTY errno proposal had no safe fd bridge.** Withdrawn and reduced - (§1.6). This was rev 2's central new proposal and it does not survive - `#![forbid(unsafe_code)]`. +- **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 @@ -32,9 +58,9 @@ All six accepted; all six verified in the code before acceptance. 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 the EPERM this occurrence proves can - happen — and discards the `SIGKILL` result while marking the entry - killed. + *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 @@ -97,12 +123,21 @@ Established: the target source is `group` — the `spec.group` pipe path 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 is actually falsified +### 1.3 What the occurrence actually invalidates -- **`src/process.rs:1246-1247` is falsified.** `tick_reap_ledger` - justifies treating any probe error as "nothing left we can reach" with - the comment "**EPERM cannot happen for our own children**". §1.2 is a - counterexample: our own child, alive, EPERM. +- **`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 @@ -180,7 +215,7 @@ one-line change that makes a real requirement visible. 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, and the errno cannot be recovered +### 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 — @@ -199,20 +234,38 @@ fn process_group_leader(&self) -> Option { } ``` -**Rev 2 proposed calling `nix::unistd::tcgetpgrp` ourselves to keep the -errno. That is withdrawn.** `tcgetpgrp` requires `F: AsFd`, and +`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`). Every route from a `RawFd` to -something implementing `AsFd` — `BorrowedFd::borrow_raw`, -`OwnedFd::from_raw_fd`, `File::from_raw_fd` — is `unsafe`, and the crate -is `#![forbid(unsafe_code)]`. There is no safe bridge, and duplicating -the fd does not create one because the duplication itself needs the same -unsafe conversion. +(`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: -**Reduced claim:** the fallback is distinguished **without** the errno. -`None` is all pmacs can observe, and the report says exactly that. The -errno is recorded here as unavailable-by-construction so a later lane -does not re-propose it. +- `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 @@ -253,10 +306,13 @@ if now >= entry.deadline && !entry.killed { } ``` -Since §1.3 falsifies the premise that EPERM cannot occur here, an EPERM -probe **drops the entry and cancels escalation silently**, and a failed -`SIGKILL` is recorded as if it succeeded. So the honest statement is that -escalation remains *scheduled*, not that it happens. +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 @@ -284,11 +340,12 @@ assertion (`:2517`) is one of the four sites acceptance 5 must update. ## 2. Questions - **Q#DC1** — Can the two entities be made to diverge in a test? *Yes: - under a PTY with job control on, a shell places a background job in its - own process group and gives it the terminal, so `tcgetpgrp` != leader - pid.* + 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, reporting only that the lookup yielded `None` (§1.6).* + *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` @@ -300,9 +357,12 @@ assertion (`:2517`) is one of the four sites acceptance 5 must update. ## 3. Bets - **Bet 1 — the divergence is constructible.** A PTY fixture where the - foreground group is not the leader: job control on, a background job - given the terminal. The rewritten acceptance asserts both exact values - **and that they differ**. + 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. @@ -310,8 +370,9 @@ assertion (`:2517`) is one of the four sites acceptance 5 must update. exists for. - **Bet 2 — the PTY fallback is reachable and distinguishable.** A test - drives the branch where the lookup returns `None` and asserts a source - string distinct from a pipe child's. + 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. @@ -339,8 +400,12 @@ assertion (`:2517`) is one of the four sites acceptance 5 must update. **rewritten**, not supplemented — it currently pins a substitution as acceptable. 2. The PTY foreground-lookup fallback reports a source distinct from a - pipe child's, with a test driving the real branch. **No errno claim** - (§1.6). + 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 @@ -355,10 +420,13 @@ assertion (`:2517`) is one of the four sites acceptance 5 must update. 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 (§1.5a). -8. `docs/agent-handoff.md` records that "EPERM cannot happen for our own - children" is false, with the run link; the comment at `:1246` is - corrected in the same PR. +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. @@ -366,9 +434,11 @@ assertion (`:2517`) is one of the four sites acceptance 5 must update. ## 5. Parked -- **The reap ledger's silent cancellation** (§1.8): an EPERM probe drops - the entry and a failed `SIGKILL` is marked as killed. Now known - reachable. **Its own lane** — disposition questions, second site. +- **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. From 0d2ad499691a66a7d0acb0f633cb0ea9c32f212c Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 12:49:47 -0400 Subject: [PATCH 4/9] test(process): make the group-directed acceptance discriminating (Bet 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Framing §3 Bet 1 and acceptance 1. Committed alone, per the branch plan: this bet decides whether the diagnostic is worth extending at all, so its result belongs in history before anything depends on it. The previous version of `a_group_directed_kill_failure_reports_target_and_leader_separately` spawned `/bin/sleep` on a PTY and asserted the same pid three times, conceding in its own doc comment that the values "are asserted to agree only because nothing has moved the terminal". The entire premise of the diagnostic is that the terminal's foreground group and the spawned leader are different entities, and no test exercised a case where they were. The fixture now drives job control: `bash -m` runs a foreground job in a fresh process group and hands it the terminal, so `tcgetpgrp` reports a group that is not the leader. The trailing `; :` is load-bearing — with a single simple command `bash -c` execs in place, which would leave the leader owning the terminal and silently restore the agreement. The bounded wait is also load-bearing rather than defensive. A probe of this fixture observed the foreground group as the leader FIRST and only then as the job's group, so measuring immediately would pin the non-divergent case. The fixture additionally asserts the diverged group still holds a live member, so a divergent number cannot come from a dead group. Falsified in both directions by substituting `leader_pid` for the `tcgetpgrp` result in `signal_target`: - the new test FAILS — target=-1020100 (leader) against the expected -1020103 (foreground group); - the OLD test, restored verbatim alongside the same mutation, PASSES. That pair is the finding: the previous acceptance pinned the substitution as acceptable. `/bin/bash` is declared as an optional test dependency and armed with PMACS_REQUIRE_BASH on BOTH CI platforms, not only Linux — the signal failures this diagnostic exists to explain have so far occurred only on macOS. The guard tests the exact path the fixture spawns rather than `which bash`, because a system with bash on PATH but not at `/bin/bash` would pass a `which` guard and then fail the spawn. Verified: both arming arms exercised against an absent path (unarmed skips, armed fails by name); 20/20 repetitions of the test; `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, and `cargo test --lib` (1877 passed) all clean; ci.yml parses. No production behaviour changes. --- .github/workflows/ci.yml | 12 +++ README.md | 9 +++ src/process.rs | 162 +++++++++++++++++++++++++++++++++++---- 3 files changed, 168 insertions(+), 15 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c9a9983..56afb21 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -218,12 +218,24 @@ 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 + # divergence fixture, which needs `/bin/bash` specifically: it + # relies on `-m` putting a foreground job in its own process group + # and handing it the terminal, so that `tcgetpgrp` differs from the + # spawned leader. Without that divergence the group-directed + # acceptance cannot tell a real foreground-group read from a + # `leader_pid` substitution. Armed on BOTH platforms, unlike the + # variables above — macOS ships `/bin/bash`, and the macOS legs are + # exactly where the signal failures this diagnostic exists for have + # actually occurred. - 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: '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/README.md b/README.md index cd91a87..c9de0a8 100644 --- a/README.md +++ b/README.md @@ -227,6 +227,15 @@ 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**, but armed on every CI platform). The + signal diagnostic's group-directed acceptance needs a terminal whose + foreground process group is *not* the spawned leader, and `bash -m` + produces exactly that 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 **and** macOS — the failures this diagnostic exists to explain + have so far only appeared on macOS. - **`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/src/process.rs b/src/process.rs index f0b851c..13716e7 100644 --- a/src/process.rs +++ b/src/process.rs @@ -2304,18 +2304,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. /// @@ -2396,24 +2384,168 @@ mod tests { /// 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. + /// 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"; + + /// The tty's current foreground process group, read through the same + /// `MasterPty` accessor production uses. `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, .. + } => master.process_group_leader(), + ChildHandle::Pipes(_) => None, + } + } + + /// Fixture for Q#DC1: a PTY child whose terminal foreground group is + /// genuinely **not** the spawned leader. + /// + /// `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 five separate facts **that are not the same + /// fact repeated**. + /// + /// The pre-Stage-B version of this test 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` entirely and substituted `leader_pid` passed it. Since + /// the whole premise of the diagnostic is that these two entities can + /// diverge, that test pinned the substitution as acceptable. + /// + /// This version drives job control so the terminal genuinely belongs + /// to a different process group, and asserts the two values **differ** + /// as well as asserting each exactly. #[test] fn a_group_directed_kill_failure_reports_target_and_leader_separately() { + // Guard on the exact path the fixture spawns, not on `which bash`. + // The two can disagree — a system with bash on PATH but not at + // `/bin/bash` would pass a `which` guard and then fail the spawn, + // which is the guard failing to guard the thing it names. + 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 \ + a_group_directed_kill_failure_reports_target_and_leader_separately" + ); + return; + } + let mut sup = ProcessSupervisor::new(); - let (id, pid) = spawn_live_pty(&mut sup, "diag-group"); + let (id, pid, fg) = spawn_pty_with_diverged_foreground_group(&mut sup, "diag-group"); + + let leader_i32 = i32::try_from(pid).expect("pid fits i32"); + assert_ne!( + fg, leader_i32, + "the fixture must make the foreground group differ from the leader; \ + equal values would let a leader_pid substitution pass" + ); 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: {} (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 tty reported, the exact \ leader pid, and observes the leader as live" ); + // The target came from the terminal, not from the leader. Stated + // as its own assertion so a regression that reintroduces the + // substitution fails here by name rather than inside a long + // string comparison. + assert!( + err.contains(&format!("target=-{fg} via tcgetpgrp")), + "the target must be the terminal's foreground group: {err}" + ); + assert!( + !err.contains(&format!("target=-{pid} via tcgetpgrp")), + "the target must NOT be the leader pid: {err}" + ); + let _ = sup.signal(id, Signal::SIGKILL); } From b27df705bbfccc33da4b510aee3d7dd24f0d2df6 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 13:08:47 -0400 Subject: [PATCH 5/9] feat(process): make the signal diagnostic discriminating (Bets 2-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Framing acceptances 2, 3, 4, 5, 7 and 8. Evidence collection only: no tolerance rule, no change to which process is signalled, no disposition change. Three distinct failures previously rendered as one string. **The PTY fallback is now named.** When a PTY's foreground-group lookup yields no group, the target falls back to the leader — and until now that rendered "leader-pid", identical to a pipe child that never had a terminal. `portable-pty::MasterPty::process_group_leader` collapses every failure into `None` before pmacs can see it, so the errno was gone too. pmacs now performs the query itself and reports four distinct outcomes: no master fd, a failed duplicate with its errno, a failed `tcgetpgrp` with its errno, and a non-positive answer. Doing that without `unsafe` is the interesting part. `nix::unistd:: tcgetpgrp` needs `AsFd`; `MasterPty` exposes only `Option`; and every std route between them is `unsafe`, which this crate forbids. `filedescriptor::OwnedHandle::dup` takes any `AsRawFd` through a safe blanket impl and returns an owned handle that IS `AsFd`, so a lifetime-tied view implementing one safe trait is the whole bridge. The borrow is what makes it sound: the view cannot outlive the master, so the descriptor cannot close underneath it. **The report names the signal.** A failed SIGUSR1 and a failed SIGTERM were the same 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. A separate control pins that the fatal/non-fatal difference is real for calls that SUCCEED, which is what gives the first test its meaning. **`measured_group` is a real observation.** `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 rather than evidence. `getpgid` supplies the one field that can disagree. It establishes no identity — it is read inside the same read-then-act window, and no portable mechanism closes that for a group. Bites, each by an actual revert, all observed to fail: - collapsing the PTY fallback back into a bare "leader-pid"; - dropping `signal=` from the report; - making the measured group restate the pid it was handed; - replacing the job-control fixture with a plain `sleep`, as a positive control on the divergence fixture itself. All four exact-string sites were updated individually, never by a blanket rewrite: a wholesale rewrite of expected strings is how a format regression hides. `:2501`'s first-call disposition pin is retained and updated for the new format rather than replaced. `nix`'s `process` feature is now declared explicitly. It already arrived transitively — nix's own `signal` feature depends on it — which is stable but invisible, and a real requirement resting on another feature's internals is one refactor away from vanishing. `filedescriptor` is declared directly for the same reason: pmacs now calls its API. The reap ledger's comment claiming "EPERM cannot happen for our own children" is corrected. Its bounded-growth policy is unchanged, but the justification was wrong: the probe targets a group, and owning the spawned child says nothing about a group unless the child is still a member — which nothing measures. The handoff records this together with the limit of the evidence: the occurrence does NOT establish that the child itself received EPERM. --- Cargo.lock | 1 + Cargo.toml | 18 +- docs/agent-handoff.md | 25 ++ src/process.rs | 523 +++++++++++++++++++++++++++++++++++++++--- 4 files changed, 540 insertions(+), 27 deletions(-) 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/docs/agent-handoff.md b/docs/agent-handoff.md index e381de0..f31f2fc 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -223,6 +223,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 diff --git a/src/process.rs b/src/process.rs index 13716e7..7f412ae 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_failure`). Always `None` outside tests. + forced_pty_lookup: Option, } /// One armed group in the reap ledger. @@ -726,16 +729,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 +790,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 +878,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(failure) => Err(failure), + 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,12 +966,45 @@ 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, ) -> String { @@ -841,10 +1016,19 @@ fn signal_failure_report( } else { String::new() }; + // Only the spawn-group path computes its target from the assumption, + // so it is the only one where a measurement can contradict anything. + // A PTY target came from the terminal and a leader-directed target is + // not a group at all. + let measured = if matches!(target.source, TargetSource::SpawnGroup) { + measured_group_of(leader_pid) + } else { + String::new() + }; 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 +1134,7 @@ impl ProcessSupervisor { reap_ledger: HashMap::new(), group_term_grace: GROUP_TERM_GRACE, forced_kill_errno: None, + forced_pty_lookup: None, } } @@ -963,6 +1148,21 @@ 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. + /// + /// The three non-success 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. Injecting only the *lookup result* + /// leaves the branch itself, the fallback target choice, the leader + /// observation against the real child, and the report construction + /// all running as production code. Consumed by one call. + #[cfg(test)] + fn force_next_pty_lookup_failure(&mut self, failure: PtyLookupFailure) { + self.forced_pty_lookup = Some(failure); + } + /// Override the SIGTERM-to-SIGKILL grace window. Test helper. pub fn set_grace_period(&mut self, d: Duration) { self.grace_period = d; @@ -1080,7 +1280,8 @@ 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)?; // 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 +1295,7 @@ 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)); } if matches!(signal, Signal::SIGTERM | Signal::SIGKILL | Signal::SIGHUP) { proc.state = ProcessState::Exiting { @@ -1243,9 +1444,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; } @@ -2388,6 +2604,20 @@ mod tests { /// 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 same /// `MasterPty` accessor production uses. `None` for a pipe /// generation, or when the terminal reports no foreground group. @@ -2524,7 +2754,7 @@ mod tests { let err = sup.terminate(id).expect_err("injected EPERM must fail"); let expected = format!( - "kill: {} (target=-{fg} 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!( @@ -2549,6 +2779,247 @@ mod tests { 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_failure(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. @@ -2564,7 +3035,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!( @@ -2614,7 +3085,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!( @@ -2646,7 +3117,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"); From e1cf1fc76b1049a61c3c3bfe15c87f1034205bae Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 13:23:32 -0400 Subject: [PATCH 6/9] =?UTF-8?q?docs:=20absorption=20pass=20=E2=80=94=20six?= =?UTF-8?q?=20merged=20lanes=20retired,=20handoff=20brought=20current?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight PRs merged on 2026-07-29/30 (#188, #190, #191, #194, #195, #196, #197, #198) and `docs/active-work.md` had drifted to 1,854 lines, most of it describing work already on `main`. Rule 4 permits removal once a lane's facts reach `docs/agent-handoff.md`; this does both halves in one commit so the precondition is satisfied rather than deferred. Handoff §1: the head-of-`main` anchor moves from `6c9e765` to `4cd4a7b` and its date from 2026-07-28 to 2026-07-30. New entries for the three implementation PRs that had no record — #190's pre-filesystem delete refusal, #191's Stage 1 adoption of the generated-buffer contract, and #196's dired Stage 2a — plus #188 recorded as landed rather than proposed. Ledger: six merged lanes removed (781 lines). Two lanes are NOT removed and are rewritten to their remaining plan instead, because their arcs are unfinished: 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" — so a removal keyed on "the PR merged" would have discarded live planning. The header now states the rule as ARC-done rather than PR-merged. Two method notes are recorded in the ledger because they nearly cost real work: - A census by PR number is a proxy, not a measurement. Counting `#NNN` in the handoff reported that five of these lanes had no record at all. Counting by content found most already documented, with the real gap being the implementation PRs while their framings were recorded. The absorption written from the first count would have duplicated existing entries. - #194 and #195 keep their lessons in §3 and §5 rather than §1, which is why a number search finds them once each. Sufficient under rule 3: durable knowledge needs a home, not a required section. Docs only. No code, no protocol change. --- docs/active-work.md | 1336 ++--------------------------------------- docs/agent-handoff.md | 63 +- 2 files changed, 120 insertions(+), 1279 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index a863a4f..af59d0b 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. ## Folding lane (Arc 6) — Stages 1 and 2 MERGED; Stage 3 (GPU) is next @@ -763,787 +307,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` @@ -1568,6 +331,25 @@ git worktree add --track \ ## Closed since the last snapshot +- **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..dd465b2 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, @@ -766,6 +775,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 From 66f73013fcee4cad5d2d4610b90729878cd15c6d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 14:10:39 -0400 Subject: [PATCH 7/9] =?UTF-8?q?test(process):=20take=20Bet=201's=20fallbac?= =?UTF-8?q?k=20=E2=80=94=20macOS=20does=20not=20diverge?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI falsified framing Bet 1. Both macOS legs reported job control never moved the terminal off the leader (leader=8542, foreground groups observed: [8542]) with the terminal staying on the leader for the entire 10s bounded wait. Linux diverges reliably — 20/20 locally and green on both ubuntu legs — so this is a platform difference rather than a flake, and rerunning past it would have been wrong. The framing named this outcome and prescribed the response, so that is what ships rather than an improvised fix: - The divergent case is pinned by INJECTING the foreground group at the `signal_target` seam. Deterministic, runs on every platform. The injection seam widens from failure-only to either outcome; the branch, target choice, leader observation against the real child, and report construction all remain production code. - Verified still discriminating: the `leader_pid`-substitution mutation fails it, `target=-1707909` against an expected `-1707910`. That was the whole point of the original rewrite and it survives the fallback. - The real shell is retained as corroboration in `job_control_really_diverges_the_foreground_group`, Linux-only. It skips on macOS by PLATFORM CHECK rather than by arming: the precondition genuinely does not hold there, so running it would assert a false claim about macOS instead of finding a bug. - Framing revision 5 records the falsification and states exactly how the injected pin is weaker — it proves the target is read from the lookup rather than substituted from the leader; it does not by itself prove any real shell produces that divergence. `PMACS_REQUIRE_BASH` moves to Linux-only. The earlier reasoning for arming both platforms — macOS is where the failures happen, so Linux-only leaves it dark where it matters — was right about the diagnostic and wrong about this test, which cannot produce its precondition on macOS at all. Arming it there made a missing binary fatal for a test that can never run. The measurement is recorded in ci.yml and the README so it is not re-derived. Gates on this tree: 11 gates, 4471 tests, zero failures. --- .github/workflows/ci.yml | 24 +-- README.md | 22 ++- ...-signal-diagnostic-completeness-framing.md | 42 ++++- src/process.rs | 162 ++++++++++++------ 4 files changed, 175 insertions(+), 75 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 56afb21..0bc7ef3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -220,22 +220,26 @@ jobs: # skip fatal where the tool is guaranteed. # # PMACS_REQUIRE_BASH arms the signal diagnostic's job-control - # divergence fixture, which needs `/bin/bash` specifically: it - # relies on `-m` putting a foreground job in its own process group - # and handing it the terminal, so that `tcgetpgrp` differs from the - # spawned leader. Without that divergence the group-directed - # acceptance cannot tell a real foreground-group read from a - # `leader_pid` substitution. Armed on BOTH platforms, unlike the - # variables above — macOS ships `/bin/bash`, and the macOS legs are - # exactly where the signal failures this diagnostic exists for have - # actually occurred. + # 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: '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/README.md b/README.md index c9de0a8..6225678 100644 --- a/README.md +++ b/README.md @@ -227,15 +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**, but armed on every CI platform). The - signal diagnostic's group-directed acceptance needs a terminal whose - foreground process group is *not* the spawned leader, and `bash -m` - produces exactly that 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 **and** macOS — the failures this diagnostic exists to explain - have so far only appeared on macOS. +- **`/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/process-signal-diagnostic-completeness-framing.md b/docs/process-signal-diagnostic-completeness-framing.md index 1326292..7dad7fe 100644 --- a/docs/process-signal-diagnostic-completeness-framing.md +++ b/docs/process-signal-diagnostic-completeness-framing.md @@ -1,6 +1,6 @@ # Framing — make the signal diagnostic discriminating (evidence collection) -**Revision 4.** Status: awaiting review round 4. Lane: +**Revision 5.** Status: implemented; PR open. 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). @@ -17,6 +17,33 @@ gets signalled, no disposition change.** Everything behavioural is in §5. ## Revision history +**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. @@ -366,8 +393,17 @@ assertion (`:2517`) is one of the four sites acceptance 5 must update. - *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. - - Without this the diagnostic remains unverified in the only case it - exists for. + - **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` diff --git a/src/process.rs b/src/process.rs index 7f412ae..70c1c80 100644 --- a/src/process.rs +++ b/src/process.rs @@ -478,8 +478,8 @@ pub struct ProcessSupervisor { /// 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_failure`). Always `None` outside tests. - forced_pty_lookup: Option, + /// `force_next_pty_lookup`). Always `None` outside tests. + forced_pty_lookup: Option>, } /// One armed group in the reap ledger. @@ -881,7 +881,7 @@ struct SignalTarget { fn signal_target( proc: &ManagedProcess, pid: u32, - forced_lookup: Option, + forced_lookup: Option>, ) -> Result { if let Some(runtime) = proc.runtime.as_ref() && let ChildHandle::Pty { @@ -895,7 +895,7 @@ fn signal_target( // produced a bare `LeaderPid`, identical to a pipe child that // never had a terminal at all. let lookup = match forced_lookup { - Some(failure) => Err(failure), + Some(outcome) => outcome, None => pty_foreground_group(master.as_ref()), }; return match lookup { @@ -1151,16 +1151,26 @@ impl ProcessSupervisor { /// Test seam for the PTY foreground-group lookup, on the same terms /// as [`Self::force_next_kill_errno`] and for the same reason. /// - /// The three non-success 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. Injecting only the *lookup result* - /// leaves the branch itself, the fallback target choice, the leader - /// observation against the real child, and the report construction - /// all running as production code. Consumed by one call. + /// 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_failure(&mut self, failure: PtyLookupFailure) { - self.forced_pty_lookup = Some(failure); + fn force_next_pty_lookup(&mut self, outcome: Result) { + self.forced_pty_lookup = Some(outcome); } /// Override the SIGTERM-to-SIGKILL grace window. Test helper. @@ -2706,50 +2716,46 @@ mod tests { /// 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 five separate facts **that are not the same - /// fact repeated**. + /// leader's own state, as facts **that are not the same fact + /// repeated**. /// - /// The pre-Stage-B version of this test 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` entirely and substituted `leader_pid` passed it. Since - /// the whole premise of the diagnostic is that these two entities can - /// diverge, that test pinned the substitution as acceptable. + /// 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. /// - /// This version drives job control so the terminal genuinely belongs - /// to a different process group, and asserts the two values **differ** - /// as well as asserting each exactly. + /// **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() { - // Guard on the exact path the fixture spawns, not on `which bash`. - // The two can disagree — a system with bash on PATH but not at - // `/bin/bash` would pass a `which` guard and then fail the spawn, - // which is the guard failing to guard the thing it names. - 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 \ - a_group_directed_kill_failure_reports_target_and_leader_separately" - ); - return; - } - let mut sup = ProcessSupervisor::new(); - let (id, pid, fg) = spawn_pty_with_diverged_foreground_group(&mut sup, "diag-group"); + 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 fixture must make the foreground group differ from the leader; \ - equal values would let a leader_pid substitution pass" + "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"); @@ -2759,17 +2765,16 @@ mod tests { ); assert_eq!( err, expected, - "the report names the exact group the tty reported, the exact \ + "the report names the exact group the lookup returned, the exact \ leader pid, and observes the leader as live" ); - // The target came from the terminal, not from the leader. Stated - // as its own assertion so a regression that reintroduces the - // substitution fails here by name rather than inside a long - // string comparison. + // 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 terminal's foreground group: {err}" + "the target must be the group the lookup returned: {err}" ); assert!( !err.contains(&format!("target=-{pid} via tcgetpgrp")), @@ -2779,6 +2784,57 @@ mod tests { 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" + ); + + // And the production lookup — not the fixture's own polling — + // reports that same group. + let observed = foreground_pgid(&sup, id).expect("PTY reports a foreground group"); + assert_eq!( + observed, fg, + "the production accessor must see what the fixture waited for" + ); + + 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. /// @@ -2811,7 +2867,7 @@ mod tests { let mut sup = ProcessSupervisor::new(); let (id, pid) = spawn_live_pty(&mut sup, "diag-pty-fallback"); - sup.force_next_pty_lookup_failure(failure); + 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"); From 95897f75636a0bd3ffa10b4a1cadbac8bce04e42 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 14:27:46 -0400 Subject: [PATCH 8/9] fix(process): sample the group before the kill; make corroboration real MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 4: three blocking, one major. All accepted. **`measured_group` was sampled after the failure.** It ran inside `signal_failure_report`, after the `kill` and after `observe_leader`, while the framing and the function's own doc both 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, so the field describes the target that was attempted rather than the state the failure left behind. **The Linux corroboration did not exercise the production lookup.** Its helper read `portable_pty::process_group_leader` — the accessor this lane stopped using on the signal path — so `pty_foreground_group` could have fallen back on every call with every test still green. Forcing it to always fall back demonstrates the hole exactly: the corroboration fails, and the injected pin PASSES, because the injected tests supply the group themselves and structurally cannot detect a broken lookup. The helper now calls the production lookup, and the corroboration forces only the kill so the report is built from a real terminal read. The residual limitation is recorded rather than left to the green: on macOS `pty_foreground_group` has no end-to-end coverage, because the platform cannot produce the precondition. **The framing did not update its own acceptance contract.** Revision 5 recorded Bet 1's falsification in the revision history and in the bet, and left the normative criterion demanding the real-shell rewrite — the "implementation quietly diverges from the contract" shape this project already recorded as a lesson on #191/#188. Acceptance 1 now describes the injected pin, 1a adds the corroboration and its macOS limitation, and 4 states the sampling order. The ledger is synchronized: revision 6, four commits, 4,471 tests, bash armed on Linux only. **`TargetSource`'s doc had the wrong classification.** Two of the four variants target the leader pid, not one, and the pid-versus-group split does not line up with PTY-versus-pipe — which is why the fallback needed its own variant rather than reusing `LeaderPid`. Gates: 11 gates, 4,471 tests, zero failures. --- docs/active-work.md | 29 ++++--- ...-signal-diagnostic-completeness-framing.md | 53 ++++++++++-- src/process.rs | 85 ++++++++++++++----- 3 files changed, 128 insertions(+), 39 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 33ad59d..28be8ca 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -122,11 +122,13 @@ If it does not, stop and repair the remote/fetch configuration. - **Portable branch:** `githubsucks/process-signal-diagnostic-completeness`; worktree `../pmacs-signal-identity`. Governing document: - `docs/process-signal-diagnostic-completeness-framing.md`, **revision 4, - approved** after three review rounds. -- **State:** implemented and gated; canonical `githubsucks/main` @ - `b8e18f6` (the ledger absorption #199) is integrated. Two commits — - Bet 1 alone, then Bets 2–4 — per the framing's branch plan. + `docs/process-signal-diagnostic-completeness-framing.md`, **revision 6**; + the contract was approved at revision 4 and has been revised twice + since to match what shipped. +- **State:** **PR #200 open**, four review rounds closed, held for + review. Canonical `githubsucks/main` @ `b8e18f6` (the ledger + absorption #199) is integrated. Four commits: Bet 1 alone, Bets 2–4, + Bet 1's fallback after CI falsified it, then round 4's corrections. - **Boundary, unchanged:** evidence collection only. No signal retargeting, tolerance, disposition change, or reap-ledger repair. **Group identity remains unprovable** (framing §1.5): the measured @@ -143,7 +145,7 @@ If it does not, stop and repair the remote/fetch configuration. claiming "EPERM cannot happen for our own children" is corrected **without** claiming the child itself received EPERM. - **Verification at the merged tree** (not inherited from the pre-merge - head): 11 gates, **4469 tests, zero failures** — fmt, diff-check, + head): 11 gates, **4471 tests, zero failures** — fmt, diff-check, clippy, `--lib`, `--lib --features crdt`, compile-mode, copy-mode in both feature configurations, bottom-panel Stage 1, M4 with the basedpyright skip, and required GPU. The job-control divergence @@ -157,11 +159,16 @@ If it does not, stop and repair the remote/fetch configuration. Separately, the **pre-Stage-B test was restored verbatim under the substitution mutation and PASSED**, which is the finding that justified rewriting it rather than adding to it. -- **`/bin/bash` is a declared optional test dependency**, armed by - `PMACS_REQUIRE_BASH` on **both** CI platforms rather than Linux only: - the failures this diagnostic exists to explain have so far occurred - only on macOS, so arming it Linux-only would leave it dark exactly - where it matters. +- **Bet 1 was falsified by CI and the framing's fallback shipped.** + `bash -m` diverges on Linux and **never on macOS**, where both legs + observed the terminal stay with the leader for a full 10s wait. The + divergent case is now pinned by **injecting** the foreground group + (runs everywhere, weaker); a Linux-only corroboration drives a real + shell and is the **only** test exercising `pty_foreground_group` + end-to-end. `/bin/bash` is a declared optional test dependency armed + by `PMACS_REQUIRE_BASH` on **Linux only** — macOS ships bash but + cannot produce the precondition, so arming it there would make a + missing binary fatal for a test that can never run. - **Recovery from a clean checkout:** ```sh diff --git a/docs/process-signal-diagnostic-completeness-framing.md b/docs/process-signal-diagnostic-completeness-framing.md index 7dad7fe..5c41617 100644 --- a/docs/process-signal-diagnostic-completeness-framing.md +++ b/docs/process-signal-diagnostic-completeness-framing.md @@ -1,6 +1,6 @@ # Framing — make the signal diagnostic discriminating (evidence collection) -**Revision 5.** Status: implemented; PR open. Lane: +**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). @@ -17,6 +17,32 @@ 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. @@ -431,10 +457,24 @@ assertion (`:2517`) is one of the four sites acceptance 5 must update. ## 4. Acceptance -1. A PTY job-control fixture where `tcgetpgrp` != leader pid, both exact - values asserted and asserted to differ. The test at `:2400` is - **rewritten**, not supplemented — it currently pins a substitution as +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 @@ -450,7 +490,10 @@ assertion (`:2517`) is one of the four sites acceptance 5 must update. 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). + a test asserting a case where they **differ** (§3 Bet 4). **It is + sampled before the `kill`**, not during report construction, so it + describes the target that was attempted rather than the state left + behind by the failure. 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. diff --git a/src/process.rs b/src/process.rs index 70c1c80..ea9b377 100644 --- a/src/process.rs +++ b/src/process.rs @@ -717,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 @@ -976,7 +984,7 @@ fn observe_leader(proc: &mut ManagedProcess) -> LeaderObservation { /// 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 +/// 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 @@ -1007,6 +1015,7 @@ fn signal_failure_report( 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) { @@ -1016,15 +1025,11 @@ fn signal_failure_report( } else { String::new() }; - // Only the spawn-group path computes its target from the assumption, - // so it is the only one where a measurement can contradict anything. - // A PTY target came from the terminal and a leader-directed target is - // not a group at all. - let measured = if matches!(target.source, TargetSource::SpawnGroup) { - measured_group_of(leader_pid) - } 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} (signal={signal:?}, target={} via {}, leader_pid={leader_pid}{expected}{measured}, leader={})", target.pid.as_raw(), @@ -1292,6 +1297,13 @@ impl ProcessSupervisor { }; 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 @@ -1305,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, signal, 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 { @@ -2628,15 +2647,19 @@ mod tests { (id, spawn_started_pid(sup, id)) } - /// The tty's current foreground process group, read through the same - /// `MasterPty` accessor production uses. `None` for a pipe + /// 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, .. - } => master.process_group_leader(), + } => pty_foreground_group(master.as_ref()).ok(), ChildHandle::Pipes(_) => None, } } @@ -2824,12 +2847,28 @@ mod tests { "a real job-control shell must move the terminal off the leader" ); - // And the production lookup — not the fixture's own polling — - // reports that same group. - let observed = foreground_pgid(&sup, id).expect("PTY reports a foreground group"); + // 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!( - observed, fg, - "the production accessor must see what the fixture waited for" + 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); From 262b7c0b6f649350913d66d284692925bd72647d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 30 Jul 2026 14:41:50 -0400 Subject: [PATCH 9/9] docs(process): stop overclaiming the pre-kill sample; drop a dead comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 5: one blocking, one major. Both documentation-only; the runtime fixes from round 4 are unchanged. **The pre-kill sample was overclaimed.** Acceptance 4 said the measured group "describes the target that was attempted". It does not. `getpgid` and `kill` remain separated by the same read-then-act window §1.5 is built on, 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. The criterion now says it records pre-kill evidence about the attempted target, and adds an explicit sentence forbidding any acceptance from claiming otherwise. This is worth naming rather than quietly editing: the framing's whole spine is that this telemetry establishes less than it appears to, and round 4 fixed a real ordering defect and then inflated the fix's meaning inside the document that governs it. **`95897f7`'s commit message carries the same overclaim.** It is not amended — force-pushing a branch under review to rewrite history would hide the error rather than correct it. This message is the correction, and the two read in sequence. **A dead doc comment was documenting a constant.** The original pre-Stage-B acceptance-1 block survived the rewrite and came to rest above `const BASH`, so the file explained a string constant with the sentence this entire lane exists to remove — "here they are asserted to agree only because nothing has moved the terminal", describing a test that no longer exists. Removed; only the BASH explanation remains. Verified the overclaim is gone from the whole tree, not only the cited line: no hits in `docs/` or `src/`. Gates: 11 gates, 4,471 tests, zero failures. --- ...process-signal-diagnostic-completeness-framing.md | 11 +++++++++-- src/process.rs | 12 ------------ 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/docs/process-signal-diagnostic-completeness-framing.md b/docs/process-signal-diagnostic-completeness-framing.md index 5c41617..b945da7 100644 --- a/docs/process-signal-diagnostic-completeness-framing.md +++ b/docs/process-signal-diagnostic-completeness-framing.md @@ -492,8 +492,15 @@ assertion (`:2517`) is one of the four sites acceptance 5 must update. 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 - describes the target that was attempted rather than the state left - behind by the failure. + 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. diff --git a/src/process.rs b/src/process.rs index ea9b377..33fdf8f 100644 --- a/src/process.rs +++ b/src/process.rs @@ -2617,18 +2617,6 @@ 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. - /// - /// 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. /// 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";