From dda90a2c379d6bc7f698b0c15b3abc1095093ec9 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 14:09:46 +0200 Subject: [PATCH 01/20] docs: frame worker identity Stage 1 (revision 1) COHERENCE.md section 0 names background-work ownership as one of two remaining thin ends of the golden journey, and section 20 puts it outside Priority 1 while conceding it is the last of that priority own work. Section 9 grades the worker model "mechanism without identity". The felt gap is narrower than the arc and is checkable: grep -c for spinner/progress/busy in src/statusline.rs returns 0, so section 3 promise of "visible asynchronous work" is false today unless the user knows to run M-x editor.list-workers. The git Stage 1 lane in flight right now records a deliberate negative section 9 impact for exactly this reason; this lane is the one that repays it. Two scouting findings shaped the staging rather than confirmed it. PendingJob carries eight fields, not the seven the audit lists, and the eighth doc comment cites section 9 by name as the reason identity belongs on the job and not in a side map. So this extends a merged decision instead of introducing one. pmacs.process.list filters to LineOriented, dating to the vterm Stage 1 commit, and three acceptance suites use #pmacs.process.list() as a leak detector. Widening that accessor to show terminal PTYs would inflate all three baselines. Making PTYs visible therefore moves to Stage 2 behind a separate accessor, which is a better answer than editing tests that are correctly detecting a semantic change. NO WIRE CHANGE, and that is load-bearing for scheduling: discovery Stage 2 holds the v22-to-v23 bump slot and git Stage 2 is queued behind it. The activity indicator is a fourth pmacs.statusline.register provider on the existing StatuslineSegments vector. The framing also flags a deliberate deviation from the audit rather than quietly taking it: section 9 names owner/purpose/parent together, and Stage 1 takes only the first two, because an unpopulated parent field reads as "no parent" rather than "not tracked". Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 43 ++++ docs/worker-identity-framing.md | 380 ++++++++++++++++++++++++++++++++ 2 files changed, 423 insertions(+) create mode 100644 docs/worker-identity-framing.md diff --git a/docs/active-work.md b/docs/active-work.md index ad3851a..958216e 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -265,6 +265,48 @@ also removed: this branch's "R8 NEEDS A LANE" investigation block, and durable facts are in the retired registry row and the handoff §6 census. +## Worker identity Stage 1 (§9) — BRANCHED, framing in review + +**Written with the lane's first commit**, per the standing correction +from #171 and #215. + +**Branch `worker-identity-stage1`**, base `githubsucks/main` @ +`4bc55e8` (the #225 merge). **`githubsucks/worker-identity-stage1` is +the authoritative tip** — the ref, not a SHA. Recover with +`git fetch githubsucks && git checkout worker-identity-stage1`. + +- **Framing `docs/worker-identity-framing.md`, revision 1**, in review. + Scope: `COHERENCE.md` §9's "mechanism without identity", and journey + step 11 — the last of Priority 1's own work, sitting in another + section's arc. +- **NO WIRE CHANGE**, which is what lets this run beside the two lanes + already in flight. The statusline activity indicator is a **fourth** + `pmacs.statusline.register` provider (terminal/syntax/lsp are the + three existing adopters), evaluated per frame inside `paint_frame` + (`src/editor.rs:4560`) and riding the existing `StatuslineSegments` + vector. No variant, no bump. +- **Scope:** `owner`/`purpose` on `PendingJob` and `ProcessSpec` + through the single allocation funnel (`src/async_runtime.rs:746`, + which every dispatcher and `register_external` passes through), the + handler name that `pmacs.workers.dispatch` currently discards, the + `*workers*` rendering, and the indicator. +- **Two scouting findings that shaped the design**, both verified: + `PendingJob` carries **eight** fields, not the audit's seven, and the + eighth's doc comment **cites §9 by name** as the reason identity + belongs on the job rather than in a side map — so this extends a + merged decision. And **`pmacs.process.list` filters to + `LineOriented`** (`src/lua_bindings/mod.rs:8980`), with **three + acceptance suites using `#pmacs.process.list()` as a leak detector**, + so making terminal PTYs visible is deferred to Stage 2 with a + separate accessor rather than by widening this one. +- **Deliberate deviation from the audit, flagged for review:** §9 names + owner/purpose/**parent** together as the prerequisite; Stage 1 takes + only the first two. A parent needs an ambient "currently-running job" + context, and an unpopulated `parent` reads as "no parent" rather than + "not tracked" (Q#W-5). +- **Gates:** `scripts/gate --acceptance `. No + `--protocol`. + ## Discovery Stage 2 — PR #228 OPEN, **MERGE-BLOCKED** **PR #228** — https://github.com/levineuwirth/pmacs/pull/228. Opened @@ -726,6 +768,7 @@ authoritative tip** — the ref, not a SHA. Recover with sweep step each fail the suite. ||||||| parent of 72bbb96 (docs: LSP LaTeX coverage framing revision 2, on a branch at last) ||||||| parent of 312ec7a (docs: frame Discovery Stage 2 (revision 2) — M-x rows) +||||||| parent of 8f86908 (docs: frame worker identity Stage 1 (revision 1)) ## QoL arc retirement — PR #224 OPEN (docs only) diff --git a/docs/worker-identity-framing.md b/docs/worker-identity-framing.md new file mode 100644 index 0000000..9f9c38b --- /dev/null +++ b/docs/worker-identity-framing.md @@ -0,0 +1,380 @@ +# Worker identity — Stage 1: what is running, and who asked for it + +**Status: framing pass, revision 1. Pre-implementation. Awaiting +approval.** + +--- + +## 1. Why this, and why now + +`COHERENCE.md` §9 grades the worker model **mechanism without +identity**, and §0 names **step 11 (background-work ownership)** as one +of the two remaining thin ends of the golden journey. §20 Priority 1 is +blunt about where that leaves things: + +> **The remaining thin end is no longer inside this priority.** Step 1 +> is install, which is **P8**; step 11 is background-work ownership, +> which is §9. + +So this is the last of Priority 1's own journey, sitting in another +section's arc. Everything else P1 named has landed. + +**The felt gap is smaller and sharper than the arc.** §9's audit ends +with a claim that is checkable, and I checked it: + +> **No progress indicator exists anywhere** — no statusline spinner, no +> busy count. + +`grep -c -i "spinner\|progress\|busy" src/statusline.rs` returns **0**. +So §3's promise of "visible asynchronous work" is **false today** unless +the user knows to run `M-x editor.list-workers`. Every build, LSP index, +grep, parse and — as of the lane merging beside this one — every `git +status` runs with no indication that anything is happening at all. + +**And the git Stage 1 lane in flight right now makes it worse, by its +own admission.** `docs/git-integration-framing.md` Q#G-5 states it +plainly: git runs as a spawned process, spawned processes do not appear +in `*workers*`, and the lane therefore "adds a fifth thing that runs in +the background and is not attributable from one place". It accepted that +cost because these are short-lived reads. This lane is the one that +repays it. + +## 2. Ground truth + +Scouted in the tree, not recalled from the audit — and the audit has +drifted in one place, recorded below. + +- **The audit's `PendingJob` field list is stale, and the drift is + informative.** §9 lists seven fields; the struct + (`src/async_runtime.rs:367-411`) carries **eight**. The addition is + `resource: Option`, from dired Stage 2a — and **its doc + comment cites `COHERENCE.md` §9 by name** as the reason it is a field + on the job rather than a side map: + + > `COHERENCE.md` §9 is why this is a field on the job and not a side + > map — the parse job→buffer link already lives in a side map and §9 + > names that as the defect. + + So the precedent for putting identity **on the job** is already set, + already argued, and already merged. This lane extends a decision + rather than introducing one. + +- **There is a SINGLE allocation funnel, and that is what makes this + tractable.** Every job in the system is born in `allocate` + (`src/async_runtime.rs:746`), which delegates to + `allocate_with_resource` (`:757`). The ten `dispatch_*` methods + (`:803`–`:980`) and `register_external` (`:1011`, used by MCP and LSP) + all pass through it. An identity field added there reaches every job + by construction — there is no second birth site to miss. + +- **The two-function split is itself a warning.** `allocate_with_resource` + exists only because one prior lane needed one extra parameter. A + second lane doing the same produces + `allocate_with_resource_and_identity`, and a third produces something + worse. This is the point to collapse it (Q#W-1). + +- **`JobKind` is still a closed 12-variant enum** + (`src/async_runtime.rs:305-343`) — Sleep, ComputeSum, EmitN, Grep, + Parse, FsReadDir, FsStat, FsRename, FsChmod, FsRemove, McpRequest, + LspRequest. Confirmed unchanged since the audit. + +- **A third-party job's own name is retained nowhere.** + `pmacs.workers.dispatch(name, args, opts)` + (`builtin/runtime/async.lua:369`) looks `name` up in a `handlers` + table and calls it; the handler must itself call one of the builtin + dispatchers, so the job records that builtin's `JobKind` and **`name` + is discarded at the call**. The audit's "every third-party job renders + under a builtin's label" is exact, and the fix is cheap: the name is + in hand at the one place that throws it away. + +- **`ProcessSpec` has one identity field and it is a convention** + (`src/process.rs:193-235`): `label: String`, documented as + "human-readable ... surfaced in events and the `pmacs.process.list` + output". No owner, no purpose, no parent. Callers spell it however + they like (`lsp:{name}`, a terminal buffer name). + +And the two findings that actually shape the design: + +- **A statusline provider API already exists, with three Lua adopters.** + `pmacs.statusline.register` is live in `terminal.lua:477`, + `syntax.lua:551` and `lsp.lua:1145`, taking + `{ name, side, priority, face, fn(ctx) }` and returning a string or + `nil`. An activity indicator is a **fourth registration**, not a new + mechanism. + + **And it is evaluated per frame**: `evaluate_statusline` is called + inside `paint_frame` (`src/editor.rs:4560`), before the long mutable + core borrow. So an indicator updates while work is in flight without + any new tick machinery — and, decisively for scheduling, **without + touching the wire**. `EvaluatedStatuslineSegment` is already + `Vec`-valued on an existing message; a fourth provider adds an element, + not a variant. + +- **`pmacs.process.list` deliberately hides terminal PTYs, and + un-hiding them is NOT free.** The binding filters to + `AnsiParserProfile::LineOriented` + (`src/lua_bindings/mod.rs:8980-8984`). `git log -S` dates that filter + to `bbc1f33 feat(vterm): add Stage 1 terminal core` — terminals were + excluded on purpose. + + **Three acceptance suites use `#pmacs.process.list()` as a leak + detector**: `tests/m6_8_multi_repl_acceptance.rs:385`/`:459` ("size + must not grow across cycles"), `tests/compile_mode_acceptance.rs:133`/ + `:458` ("process list returns to baseline"), and + `tests/lean4_stage1_acceptance.rs:327`/`:349`. **Removing the filter + would inflate every one of those baselines by each open terminal.** + + This is why §9's "a terminal PTY appears in no user-visible activity + view" is a real defect with a **non-obvious fix**, and why this lane + does not casually widen the existing accessor (Q#W-4). + +## 3. The staging, and why the line falls where it does + +§9's full statement wants owner, workspace, buffer, parent, children, +latency class, cancellation scope, resource budget, execution location, +progress, and failure attribution. **Two of those cannot be built at +all right now**: `Workspace` is §7, graded *missing*, and `Location` is +§8, graded *missing (architecture ready)*. A lane that added +`workspace: Option` would be adding a field typed on a +thing that does not exist. + +**Stage 1 (this lane): identity on the job and the process, and the +first indicator. NO WIRE CHANGE.** + +- `owner` and `purpose` on `PendingJob`, carried through the single + allocation funnel, and on `ProcessSpec` alongside the existing + `label`. +- `pmacs.workers.dispatch` stops discarding the registered handler name. +- `*workers*` renders owner and purpose. +- **A statusline activity indicator** — the fourth provider + registration, and the part a user feels on day one. + +**Stage 2 (separate lane): join the planes.** One activity view over +jobs, processes, LSP servers and terminals. This is what Stage 1's +identity is *for* — the audit's own conclusion is that "the four views +exist precisely because there is no common key to merge them on". It +also owns the terminal-visibility decision (Q#W-4), because that is a +question about the unified view, not about the accessor. + +**Stage 3 (unscheduled): the tree and scoped cancellation.** +`parent`/`children`, and cancel-by-owner / by-buffer / by-subtree. This +needs an ambient "currently-running job" context so a child dispatched +inside a job can find its parent without every call site threading it — +a real mechanism with its own failure modes, and the reason parent is +**not** in Stage 1 (Q#W-5). + +**Workspace and location are never this arc's**, at any stage. They +arrive from §7 and §8 and this arc consumes them. + +**The line falls at the wire on purpose, and it is again a scheduling +decision.** The discovery Stage 2 lane holds the v22→v23 bump slot, and +git Stage 2 is already queued behind it. `PROTOCOL_VERSION` is a strict +serialization point. Stage 1 here touching no wire is what lets it run +beside both. + +## 4. Coherence impact (§20) + +- **§9 worker ownership — the direct target**, and specifically the + audit's named prerequisite: *"Owner/purpose/parent fields on the job + and process specs are the prerequisite; the unified view and the + ownership tree fall out of them."* Stage 1 takes owner and purpose. +- **Journey step 11 — the direct target.** §0 names background-work + ownership as one of two remaining thin ends. This does not close the + step (Stage 2's unified view is most of that) but it is the first + thing that makes work *visible*, which is what step 11 is about. +- **§3 zero-configuration state:** repairs a claim that is currently + false. "Visible asynchronous work" becomes true by default, with no + configuration and no command to know about. +- **Interaction islands (§6): none added.** The indicator is a + statusline provider; it intercepts no keys and adds no precedence + rung. +- **§14 workbench primitives: untouched.** `*workers*` already exists; + this changes what it renders, not what renders it. +- **Config registry:** one setting at most, and my vote is a *visibility* + toggle only (Q#W-6). +- **The debt this repays is named and dated.** `git-integration-framing.md` + Q#G-5 recorded a deliberate negative §9 impact. This lane does not + fully discharge it — a labelled process is still not in `*workers*` + until Stage 2 — but it makes the label structured rather than + conventional, which is the prerequisite. + +## 5. Open questions + +### Q#W-1 — how is identity supplied at the allocation funnel? + +The existing shape is `allocate(kind, supersede, stream)` delegating to +`allocate_with_resource(kind, supersede, stream, resource)`. Adding two +more positional parameters gives a five-argument function and a +six-argument variant, and the next lane adds a seventh. + +*My vote: **collapse the pair into one funnel taking a struct***, e.g. +`allocate(JobSpec { kind, supersede, stream, resource, identity })`, +with `JobSpec` carrying a `Default`-derived constructor so the ten +dispatchers read as named-field literals rather than positional soup. +Ten call sites plus `register_external` is a bounded, mechanical edit, +and it removes the `_with_resource` wart rather than adding beside it. + +**The counter-argument, which is real:** this touches every dispatcher +in a lane whose subject is identity, which is scope the reviewer did not +ask for. **If review prefers the minimal edit**, the alternative is one +more parameter on the existing pair, and the collapse becomes its own +small lane. I would rather be told than assume. + +### Q#W-2 — what IS an owner? **(the hard one)** + +This is the question that decides whether the field is useful or +decorative, and I do not think it should be answered by whatever is +convenient at the call site. + +Candidates: the **package** that registered the code (P3's +`CurrentlyLoadingPackage` signal already exists and §20 P3 names +owner-carrying registrations as its work unit); the **command** that +the user invoked; or the **subsystem** (lsp, syntax, git, compile). + +*My vote: **`owner` is a package-or-builtin identity, `purpose` is the +human sentence.*** Concretely: `owner = "lsp"` / `purpose = "indexing +src/editor.rs"`. The reasons: + +- It is the only one of the three that a **third party** can be + attributed by, which is the whole point of attribution — a user + wanting to know why their editor is busy is usually asking *whose + code* is doing it. +- It aligns this arc with P3 rather than duplicating it. §20 says P3's + ownership arc "unblocks ... package-scoped task cancellation in §9", + so the two are meant to share a notion of owner. + +**Named risk, stated rather than hidden:** P3 has not been built, so +Stage 1 populates `owner` from a **static per-subsystem constant** at +each dispatcher, not from a live package signal. That is honest for +builtins and gives third-party Lua nothing better than today until P3 +lands. **If review thinks a field that third parties cannot populate is +premature, deferring `owner` and shipping only `purpose` is a coherent +smaller lane** — and it would still fix the indicator, which is the felt +part. + +### Q#W-3 — what does the indicator actually show? + +*My vote: **a count with the busiest purpose, and nothing when idle*** +— e.g. `⋯2 lsp: indexing`, absent entirely at zero. + +- **Absent at zero, not `0 jobs`.** A statusline segment that is always + present costs width forever to say "nothing is happening". The + existing providers already return `nil` to render nothing + (`lsp.lua:1156`), so this is the established idiom. +- **A count, not a spinner.** A spinner needs an animation frame clock + and says only "something"; a count says how much. Per-frame evaluation + makes either possible, so this is a product choice, not a constraint. +- **Not names plural.** One purpose keeps it to a bounded width; the + full list is what `*workers*` is for. + +### Q#W-4 — do terminal PTYs become visible in Stage 1? + +**No — and the reason is evidence, not caution.** `pmacs.process.list` +filters to `LineOriented`, and three acceptance suites assert on +`#pmacs.process.list()` as a leak baseline (§2). Widening that accessor +would inflate all three with every open terminal, and "fix the tests" +is the wrong response to a test that is correctly detecting a semantic +change. + +*My vote: **leave the accessor alone in Stage 1**, and let Stage 2's +unified view introduce a **separate** enumeration that includes PTYs.* +The leak detectors keep asserting what they were written to assert; the +new surface answers the new question. Two accessors with different +contracts is better than one accessor whose meaning silently changed +under its existing callers. + +### Q#W-5 — does `parent` belong in Stage 1? + +*My vote: **no**, and this is where I would most expect to be +overruled.* The audit names owner/purpose/**parent** together as the +prerequisite, so leaving one out is a deviation I should justify. + +The justification: owner and purpose are **values a dispatcher already +knows** at the call site. A parent is not — it is whatever job is +*currently running* when a child is dispatched, which means either an +ambient context (a mechanism, with re-entrancy and cleanup failure +modes) or threading a parameter through every intermediate layer. A +`parent` field that nothing populates is worse than no field: it renders +as `None` everywhere and reads as "this job has no parent" rather than +"this system does not track parents". + +Stage 3 builds the ambient and the field together, where the field can +be tested by a populated case. + +### Q#W-6 — is any of this configurable? + +*My vote: **one boolean, `ui.activity-indicator` (default `true`), +through `pmacs.config.define`.*** §11 grades the registry "partial +(foundation only)" and this document's sibling framings have both +resisted speculative settings — but a permanently-visible statusline +element is different in kind from an internal behaviour: it costs width +on every frame, and "I do not want this in my modeline" is a +preference someone will genuinely hold on day one rather than a +hypothetical. `git.enabled` and `ui.line-wrap` are the precedent shape. + +No setting for owner/purpose capture itself — that is substrate, not +preference. + +## 6. Verification + +- **Every job carries an identity, asserted at the funnel, not per + dispatcher.** The point of a single allocation site is that one + assertion covers all ten dispatchers plus `register_external`; a test + that checks three dispatchers individually would pass while a + fourteenth added later carries nothing. +- **A `pmacs.workers.dispatch("name", ...)` job reports `"name"`**, not + the builtin `JobKind` label underneath it — the exact defect §9 names, + witnessed on a handler registered from Lua. +- **`register_external` jobs carry identity too** (MCP and LSP), since + they bypass the worker pool entirely and are the ones most likely to + be missed. +- **The statusline shows nothing at idle**, asserted as *absent + segment*, not as empty string — a zero-width segment still consumes a + separator. +- **The statusline shows a count while work is in flight**, witnessed + through the real per-frame evaluation path (`paint_frame`), not by + calling the provider function directly. A provider that works in + isolation and never gets evaluated is the failure this must exclude. +- **The indicator honours `ui.activity-indicator = false`** (Q#W-6), + witnessed as an absent segment with work genuinely in flight — the + case that separates "disabled" from "idle". +- **`#pmacs.process.list()` is UNCHANGED for every existing caller** + (Q#W-4). The three leak-detector suites + (`m6_8_multi_repl_acceptance`, `compile_mode_acceptance`, + `lean4_stage1_acceptance`) are the assertion, and they must pass + untouched. **If any of them needs editing, the design is wrong**, and + that is the signal to stop rather than to adjust a baseline. +- **A spawned process carries structured owner/purpose alongside its + existing `label`**, and **`label`'s current callers keep working + unchanged** — `lsp:{name}` and terminal buffer names are live + conventions with existing consumers. +- **Both frontends render the segment**, since it rides the existing + `StatuslineSegments` path — asserted for the grid TUI and + `pmacs-gpu`, because "it is on an existing message" is a claim about + the producer and says nothing about whether a consumer draws it. + +**What this will NOT prove:** that background work is attributable from +one place (that is Stage 2's unified view — this lane makes it +*possible*, not *done*), that a terminal PTY is visible anywhere +(Q#W-4), that cancellation can range over an owner (Stage 3), or that a +third-party package's own identity flows through (Q#W-2 — blocked on +P3). + +Gates via `scripts/gate --acceptance `. **No +`--protocol`**: this lane has no wire change, which is the property that +lets it run beside the two lanes already in flight. + +## 7. Not in scope + +`Workspace` and `Location` fields (§7/§8 — the entities do not exist). +`parent`/`children` and the ownership tree (Stage 3, Q#W-5). Scoped +cancellation of any kind — cancel-all, by-kind, by-buffer, by-owner, +by-subtree (Stage 3; there is nothing to range over until identity +exists). The unified activity view joining the four planes (Stage 2). +Making terminal PTYs visible (Stage 2, Q#W-4). Widening `JobKind` or +making it open — third-party jobs are attributed by `owner`/`purpose`, +which is the point, and reopening a closed wire-adjacent enum is a +separate decision. Latency classes and resource budgets (§9 names them; +neither has a consumer yet). Supersession coverage — §9 notes parse jobs +and MCP requests pass `None`, which is a real defect and a **different** +one. P3's package-ownership signal (Q#W-2 depends on it and says so). From e1ca382ea575a927c12551fbd1c899a1d9d9d080 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 14:34:59 +0200 Subject: [PATCH 02/20] docs: worker identity framing revision 2 --- drop owner, specify the name path Two review blockers, both correct, both about the same failure: a field or a claim that looks like attribution without being it. BLOCKER 1 --- owner is not honest before P3. Revision 1 proposed owner = package-or-builtin while populating it from static per-subsystem constants at each dispatcher. Those disagree. A generic dispatcher has no trustworthy knowledge of who invoked it, and pmacs.process.spawn is callable by any package, so a static "lsp" label is an origin or category and would confidently misattribute third-party work to a builtin at exactly the point section 9 wants attribution. owner is removed rather than renamed. origin or subsystem would be honest wording, but a second string field beside purpose, used to group the view, gets adopted as ownership by the next reader regardless of its name --- and it would squat on the slot P3 has to fill. Stage 2 needs a grouping key and should get a real one. No P3 alignment is claimed any more. BLOCKER 2 --- the handler name needs a mechanism, not a parameter. Revision 1 said the name was "in hand at the one place that throws it away". That was wrong about the call chain, and re-reading it is what showed why: dispatch(name) calls an arbitrary handler, which calls a Lua wrapper, which calls the Rust binding, and name is a parameter of none of them. Worse, async.lua:337-345 documents the wrapper layer as bypassable --- other runtime files are told to call their own raw _dispatch_* primitives --- so capturing in the wrappers would miss exactly the callers attribution exists for. Q#W-2 is rewritten as a contract: a dispatch-name stack owned by the async runtime and read at allocate, the same single funnel Q#W-1 collapses. Seven rules, including the two that decide whether it is better than nothing --- unwind-safe popping, because one erroring handler would otherwise poison every later dispatch with a stale name, and composition rather than replacement of a caller-supplied purpose, because replacing recreates blocker 1 in a new place. It also answers the objection it invites: why is this ambient allowed when Q#W-5 defers parent for needing one. Because they are different mechanisms --- this is a synchronous single-threaded extent with a deterministic pop, and parent needs a lifetime model spanning ticks and post-settlement callbacks. Verification takes the reviewer wording fix: presence is a type obligation now that purpose is non-optional in a private JobSpec, so the compiler proves it and the tests prove semantics at representative entry paths. The handler witness must be a registered handler calling a real dispatcher, not a synthetic funnel test. The title changed too: "who asked for it" overclaimed once owner left. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 22 ++- docs/worker-identity-framing.md | 322 +++++++++++++++++++++++--------- 2 files changed, 254 insertions(+), 90 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 958216e..03c3567 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -275,21 +275,33 @@ from #171 and #215. the authoritative tip** — the ref, not a SHA. Recover with `git fetch githubsucks && git checkout worker-identity-stage1`. -- **Framing `docs/worker-identity-framing.md`, revision 1**, in review. +- **Framing `docs/worker-identity-framing.md`, revision 2**, in review. Scope: `COHERENCE.md` §9's "mechanism without identity", and journey step 11 — the last of Priority 1's own work, sitting in another section's arc. +- **Revision 2 took two blockers.** `owner` is **removed entirely**: + populated from static per-subsystem constants it is an origin, not an + owner, and would misattribute third-party work at the exact point §9 + wants attribution. It is not retained under a safer name either — + `origin`/`subsystem` would be adopted as ownership by use and would + squat on the slot P3 must fill. And the handler-name recovery was + **respecified as a mechanism**: revision 1 claimed the name was "in + hand at the one place that throws it away", which was wrong about the + call chain (`dispatch` → arbitrary handler → Lua wrapper → Rust + binding, with the wrapper layer documented as bypassable). - **NO WIRE CHANGE**, which is what lets this run beside the two lanes already in flight. The statusline activity indicator is a **fourth** `pmacs.statusline.register` provider (terminal/syntax/lsp are the three existing adopters), evaluated per frame inside `paint_frame` (`src/editor.rs:4560`) and riding the existing `StatuslineSegments` vector. No variant, no bump. -- **Scope:** `owner`/`purpose` on `PendingJob` and `ProcessSpec` +- **Scope:** a **required** `purpose` on `PendingJob` and `ProcessSpec` through the single allocation funnel (`src/async_runtime.rs:746`, - which every dispatcher and `register_external` passes through), the - handler name that `pmacs.workers.dispatch` currently discards, the - `*workers*` rendering, and the indicator. + which every dispatcher and `register_external` passes through), a + runtime-owned dispatch-name ambient recovering the handler name that + `pmacs.workers.dispatch` currently discards, the `*workers*` + rendering, and the indicator. Non-optional so the **compiler**, not a + test, proves every caller supplied one. - **Two scouting findings that shaped the design**, both verified: `PendingJob` carries **eight** fields, not the audit's seven, and the eighth's doc comment **cites §9 by name** as the reason identity diff --git a/docs/worker-identity-framing.md b/docs/worker-identity-framing.md index 9f9c38b..1b5fdb4 100644 --- a/docs/worker-identity-framing.md +++ b/docs/worker-identity-framing.md @@ -1,8 +1,22 @@ -# Worker identity — Stage 1: what is running, and who asked for it +# Worker identity — Stage 1: what is running, and what it is doing -**Status: framing pass, revision 1. Pre-implementation. Awaiting +*(Revision 1 was subtitled "and who asked for it". With `owner` +removed that title overclaimed the lane: it answers **what**, and — +under `pmacs.workers.dispatch` — **under which registered handler**. +Neither is who owns it.)* + +**Status: framing pass, revision 2. Pre-implementation. Awaiting approval.** +**Revision 2 removes `owner` and respecifies the handler-name path, +after review found the first dishonest and the second unbuildable as +described.** `owner` populated from static per-subsystem constants is +an *origin*, not an owner, and would misattribute third-party work at +exactly the point §9 wants attribution. And "the name is in hand at the +one place that throws it away" was **wrong about the call chain** — it +is thrown away across three layers, one of which callers are documented +to bypass. Both re-scouted in the tree. + --- ## 1. Why this, and why now @@ -78,14 +92,36 @@ drifted in one place, recorded below. Parse, FsReadDir, FsStat, FsRename, FsChmod, FsRemove, McpRequest, LspRequest. Confirmed unchanged since the audit. -- **A third-party job's own name is retained nowhere.** - `pmacs.workers.dispatch(name, args, opts)` - (`builtin/runtime/async.lua:369`) looks `name` up in a `handlers` - table and calls it; the handler must itself call one of the builtin - dispatchers, so the job records that builtin's `JobKind` and **`name` - is discarded at the call**. The audit's "every third-party job renders - under a builtin's label" is exact, and the fix is cheap: the name is - in hand at the one place that throws it away. +- **A third-party job's own name is retained nowhere, and recovering it + is NOT cheap. Revision 1 said it was, and was wrong about the call + chain.** The full path, read rather than assumed: + + ``` + pmacs.workers.dispatch(name, args, opts) -- async.lua:369 + → handlers[name](args, opts) -- arbitrary Lua + → dispatch_grep(spec, opts) -- Lua wrapper, :312 + → async_mod._dispatch_grep(spec, supersede_key(opts), max_batch) + → the Rust binding → allocate() + ``` + + **`name` is not a parameter of any layer below the first.** The Rust + dispatchers accept job arguments, a supersede key and stream data — + nothing else. So revision 1's "change the allocation funnel and the + name is recovered" is false: changing `allocate` gives the name + nowhere to arrive *from*. + + **And the wrapper layer cannot be the capture point either.** + `async.lua:337-345` deliberately exposes `pmacs.workers._new_handle` / + `_new_stream` so that "other builtin runtime files (`pmacs.fs` in + M8.1, future siblings) can construct handles for ids dispatched + through **their own raw `_dispatch_*` primitives**". A handler that + goes straight to `async_mod._dispatch_*` bypasses `dispatch_grep` and + friends entirely — and those are precisely the callers doing + non-standard work, i.e. the ones attribution is for. + + The audit's "every third-party job renders under a builtin's label" + is exact. The mechanism that fixes it is Q#W-2, and it is a real + mechanism, not a parameter. - **`ProcessSpec` has one identity field and it is a convention** (`src/process.rs:193-235`): `label: String`, documented as @@ -138,17 +174,36 @@ all right now**: `Workspace` is §7, graded *missing*, and `Location` is `workspace: Option` would be adding a field typed on a thing that does not exist. -**Stage 1 (this lane): identity on the job and the process, and the -first indicator. NO WIRE CHANGE.** +**Stage 1 (this lane): a required `purpose` on the job and the process, +and the first indicator. NO WIRE CHANGE. NO `owner`.** -- `owner` and `purpose` on `PendingJob`, carried through the single - allocation funnel, and on `ProcessSpec` alongside the existing +- **`purpose`, non-optional**, on `PendingJob`, carried through the + single allocation funnel, and on `ProcessSpec` alongside the existing `label`. -- `pmacs.workers.dispatch` stops discarding the registered handler name. -- `*workers*` renders owner and purpose. +- **A dispatch-identity ambient** so `pmacs.workers.dispatch` stops + discarding the registered handler name (Q#W-2). +- `*workers*` renders `purpose`. - **A statusline activity indicator** — the fourth provider registration, and the part a user feels on day one. +**`owner` is deliberately absent, and revision 1 was wrong to include +it.** The proposal was `owner = "lsp"` populated from a static +per-subsystem constant at each dispatcher. But a generic dispatcher has +no trustworthy knowledge of who invoked it, and `pmacs.process.spawn` +is callable by any package — so a static subsystem label is an +**origin or category, not an owner**, and it would confidently +misattribute third-party work to a builtin at exactly the point §9 +wants attribution. A field that asserts a falsehood is worse than an +absent one: `*workers*` would *look* attributed while naming the wrong +party. + +**Nor is it retained under a safer name.** Calling it `origin` or +`subsystem` would be honest, but a second string field sitting beside +`purpose` and grouping the view would be *adopted* as ownership by the +next reader regardless of its name — and it would squat on the slot +P3's real package signal has to fill. Stage 2 needs a grouping key; it +should get a real one, not a placeholder promoted by use. + **Stage 2 (separate lane): join the planes.** One activity view over jobs, processes, LSP servers and terminals. This is what Stage 1's identity is *for* — the audit's own conclusion is that "the four views @@ -177,7 +232,11 @@ beside both. - **§9 worker ownership — the direct target**, and specifically the audit's named prerequisite: *"Owner/purpose/parent fields on the job and process specs are the prerequisite; the unified view and the - ownership tree fall out of them."* Stage 1 takes owner and purpose. + ownership tree fall out of them."* **Stage 1 takes ONE of the three + — `purpose`.** `owner` waits for P3 to supply a package signal worth + recording (§3); `parent` waits for Stage 3 (Q#W-5). Taking one of + three named prerequisites is a deviation from the audit, and it is + stated here rather than left to be noticed. - **Journey step 11 — the direct target.** §0 names background-work ownership as one of two remaining thin ends. This does not close the step (Stage 2's unified view is most of that) but it is the first @@ -195,8 +254,12 @@ beside both. - **The debt this repays is named and dated.** `git-integration-framing.md` Q#G-5 recorded a deliberate negative §9 impact. This lane does not fully discharge it — a labelled process is still not in `*workers*` - until Stage 2 — but it makes the label structured rather than - conventional, which is the prerequisite. + until Stage 2 — but it makes the process state *what it is doing* in + a required field rather than a caller-spelled convention. +- **No P3 alignment is claimed.** Revision 1 argued this lane aligned + with P3's ownership arc. With `owner` removed, it does not: P3 stays + entirely ahead of it, and this lane deliberately leaves that slot + empty rather than filling it with something P3 would have to displace. ## 5. Open questions @@ -208,11 +271,18 @@ more positional parameters gives a five-argument function and a six-argument variant, and the next lane adds a seventh. *My vote: **collapse the pair into one funnel taking a struct***, e.g. -`allocate(JobSpec { kind, supersede, stream, resource, identity })`, -with `JobSpec` carrying a `Default`-derived constructor so the ten -dispatchers read as named-field literals rather than positional soup. -Ten call sites plus `register_external` is a bounded, mechanical edit, -and it removes the `_with_resource` wart rather than adding beside it. +`allocate(JobSpec { kind, supersede, stream, resource, purpose })`, so +the ten dispatchers read as named-field literals rather than positional +soup. Ten call sites plus `register_external` is a bounded, mechanical +edit, and it removes the `_with_resource` wart rather than adding +beside it. + +**`JobSpec` is private, and `purpose` is non-optional.** Private +because the public dispatcher APIs should not grow a parameter every +time this arc adds a field; non-optional because that is what makes the +compiler, rather than a test, the thing that proves every caller +supplied one (§6). A `Default` impl would defeat exactly that, so +`purpose` is not defaulted even if other fields are. **The counter-argument, which is real:** this touches every dispatcher in a lane whose subject is identity, which is scope the reviewer did not @@ -220,42 +290,88 @@ ask for. **If review prefers the minimal edit**, the alternative is one more parameter on the existing pair, and the collapse becomes its own small lane. I would rather be told than assume. -### Q#W-2 — what IS an owner? **(the hard one)** +### Q#W-2 — the dispatch identity path **(rewritten in rev 2)** -This is the question that decides whether the field is useful or -decorative, and I do not think it should be answered by whatever is -convenient at the call site. +Revision 1 treated this as a parameter-passing detail. §2 shows it is +not: `name` dies at `pmacs.workers.dispatch` and nothing below it takes +a name, so the value must be carried *out of band* across an arbitrary +handler. -Candidates: the **package** that registered the code (P3's -`CurrentlyLoadingPackage` signal already exists and §20 P3 names -owner-carrying registrations as its work unit); the **command** that -the user invoked; or the **subsystem** (lsp, syntax, git, compile). +**The capture point is Rust, not Lua**, and the reason is the bypass in +§2. If the ambient lived in the Lua wrapper layer, a handler calling +`async_mod._dispatch_*` directly — the documented pattern for runtime +files with their own primitives — would produce an unattributed job, +and those are the callers attribution exists for. Putting it in the +runtime means it is read at `allocate`, **the same single funnel Q#W-1 +is already collapsing**. One mechanism, one site, no path around it. -*My vote: **`owner` is a package-or-builtin identity, `purpose` is the -human sentence.*** Concretely: `owner = "lsp"` / `purpose = "indexing -src/editor.rs"`. The reasons: +*My vote: **a dispatch-name stack owned by the async runtime***, with +`pmacs.workers.dispatch` bracketing its handler call through two +runtime-internal bindings (`_push_dispatch_name` / `_pop_dispatch_name`). -- It is the only one of the three that a **third party** can be - attributed by, which is the whole point of attribution — a user - wanting to know why their editor is busy is usually asking *whose - code* is doing it. -- It aligns this arc with P3 rather than duplicating it. §20 says P3's - ownership arc "unblocks ... package-scoped task cancellation in §9", - so the two are meant to share a notion of owner. +**The contract, in full:** -**Named risk, stated rather than hidden:** P3 has not been built, so -Stage 1 populates `owner` from a **static per-subsystem constant** at -each dispatcher, not from a live package signal. That is honest for -builtins and gives third-party Lua nothing better than today until P3 -lands. **If review thinks a field that third parties cannot populate is -premature, deferring `owner` and shipping only `purpose` is a coherent -smaller lane** — and it would still fix the indicator, which is the felt -part. +1. **Extent is the SYNCHRONOUS handler call, and nothing more.** Push + before, pop after. Every job reaching `allocate` during that window + carries the name. +2. **Work dispatched later is NOT covered, deliberately.** A job + dispatched from an `on_complete` callback or a resumed coroutine + runs ticks later, outside the extent, and carries only its own + `purpose`. Pretending otherwise would need the asynchronous + lifetime mechanism this lane defers (Q#W-5). +3. **Nesting is a stack; innermost wins.** Handler `a` calling + `pmacs.workers.dispatch("b", …)` gives jobs allocated inside `b` the + name `b`, and restores `a` on return. +4. **Fan-out shares the name.** A handler dispatching five jobs + produces five jobs named alike. They *were* all dispatched under it; + that is the fact being recorded, not a collision. +5. **Unwind-safe, and this is the one that makes a naive version worse + than none.** A handler that errors must still pop — otherwise one + failure poisons every subsequent dispatch in the session with a + stale name, and the feature silently starts lying. `pmacs.workers. + dispatch` runs the handler under `pcall`, pops, and rethrows. +6. **Precedence over a caller-supplied purpose: COMPOSE, do not + replace.** Where the dispatch site supplied its own purpose, the + recorded value is `": "`; where it did not, the + recorded value is `""`. Replacing would recreate blocker 1 in + a new place — `dispatch_grep` supplies `"grep: …"`, and letting that + win would lose the third party again, while letting the name win + would discard the only description of the actual work. Composition + is capped at the innermost name by rule 3, so no unbounded chain. +7. **Outside any extent, nothing changes.** A builtin invoked directly + records its own `purpose`. + +**A known and accepted property, stated rather than discovered later:** +the ambient captures *causal* extent, not *intent*. If a handler +synchronously triggers unrelated work — an edit that schedules a parse +— that job is inside the window and takes the name. Within a +synchronous extent I think that is the honest reading ("this ran +because that handler ran"), and it is the only definition enforceable +at a single funnel. **If review disagrees, the alternative is +capture-at-the-Lua-wrapper**, which is narrower and misses the raw +`_dispatch_*` callers — a trade of false positives for false negatives, +and I would rather over-attribute inside a synchronous call than +silently drop the third-party case. + +**Why this ambient is admissible while Q#W-5's is not.** They are not +the same mechanism. This one is a synchronous, single-threaded, bounded +dynamic extent with a deterministic pop — a `let` binding in disguise. +A `parent` ambient must span a job's *asynchronous* lifetime, across +ticks, through callbacks that run after the parent settled. The first +is a stack; the second is a lifetime model. ### Q#W-3 — what does the indicator actually show? -*My vote: **a count with the busiest purpose, and nothing when idle*** -— e.g. `⋯2 lsp: indexing`, absent entirely at zero. +*My vote: **a count plus the oldest in-flight job's `purpose`, and +nothing when idle*** — e.g. `⋯2 lsp: indexing`, absent entirely at +zero. With `owner` gone (§3) `purpose` is the only identity there is, +which is also why it is required rather than optional. + +**Oldest, not newest or "busiest".** Revision 1 said "busiest", which +is not a defined quantity — jobs carry no cost estimate. Oldest is +computable from `dispatched_at`, which `PendingJob` already has, and it +answers the question a user actually asks of a stuck editor: *what is +taking so long?* - **Absent at zero, not `0 jobs`.** A statusline segment that is always present costs width forever to say "nothing is happening". The @@ -285,21 +401,31 @@ under its existing callers. ### Q#W-5 — does `parent` belong in Stage 1? -*My vote: **no**, and this is where I would most expect to be -overruled.* The audit names owner/purpose/**parent** together as the -prerequisite, so leaving one out is a deviation I should justify. +*My vote: **no.*** The audit names owner/purpose/**parent** together as +the prerequisite, and after revision 2 this lane takes only `purpose` — +so both omissions need justifying, not just this one. `owner`'s is in +§3; `parent`'s is here. -The justification: owner and purpose are **values a dispatcher already -knows** at the call site. A parent is not — it is whatever job is -*currently running* when a child is dispatched, which means either an -ambient context (a mechanism, with re-entrancy and cleanup failure -modes) or threading a parameter through every intermediate layer. A -`parent` field that nothing populates is worse than no field: it renders -as `None` everywhere and reads as "this job has no parent" rather than -"this system does not track parents". +`purpose` is a **value the dispatcher already knows** at the call site. +A parent is not — it is whatever job is *currently running* when a +child is dispatched. A `parent` field that nothing populates is worse +than no field: it renders as `None` everywhere and reads as "this job +has no parent" rather than "this system does not track parents". -Stage 3 builds the ambient and the field together, where the field can -be tested by a populated case. +**And the objection revision 2 has to answer, since it now builds an +ambient of its own (Q#W-2):** why is one admissible and not the other? +Because they are not the same mechanism. Q#W-2's extent is +synchronous, single-threaded, and bounded by one function call, with a +deterministic pop on both the normal and the error path. A `parent` +ambient must identify the running job *across ticks* — a job dispatched +from an `on_complete` callback should name the job whose completion +fired it, and that callback runs after the parent settled, on the main +thread, outside any dispatch call. That is a lifetime model, not a +stack, and it is Stage 3's subject rather than a field this lane can +add cheaply. + +Stage 3 builds the lifetime model and the field together, where the +field can be tested by a populated case. ### Q#W-6 — is any of this configurable? @@ -312,22 +438,42 @@ on every frame, and "I do not want this in my modeline" is a preference someone will genuinely hold on day one rather than a hypothetical. `git.enabled` and `ui.line-wrap` are the precedent shape. -No setting for owner/purpose capture itself — that is substrate, not +No setting for `purpose` capture itself — that is substrate, not preference. ## 6. Verification -- **Every job carries an identity, asserted at the funnel, not per - dispatcher.** The point of a single allocation site is that one - assertion covers all ten dispatchers plus `register_external`; a test - that checks three dispatchers individually would pass while a - fourteenth added later carries nothing. -- **A `pmacs.workers.dispatch("name", ...)` job reports `"name"`**, not - the builtin `JobKind` label underneath it — the exact defect §9 names, - witnessed on a handler registered from Lua. -- **`register_external` jobs carry identity too** (MCP and LSP), since - they bypass the worker pool entirely and are the ones most likely to - be missed. +- **Presence is enforced by the COMPILER, not by a test.** `purpose` is + non-optional in `JobSpec`, so a dispatcher that supplies none does not + build. Revision 1 claimed a single funnel assertion proved "every job + carries an identity"; **it does not** — a funnel test proves the + funnel stores what it was handed, and says nothing about whether + fourteen callers handed it anything meaningful. Presence is a type + obligation; the tests below are for *semantics*. +- **Representative entry paths assert the semantics**, one per distinct + shape rather than one per dispatcher: a pool dispatcher, an + `register_external` job (MCP/LSP bypass the worker pool entirely and + are the likeliest to be missed), and a spawned process. +- **A `pmacs.workers.dispatch("name", …)` job reports `"name"`**, and + the witness is **a handler registered from Lua that calls a real + dispatcher** — not a synthetic funnel test. A test that pushes the + ambient by hand proves the stack works and leaves the actual defect + (`name` dying in an arbitrary handler) unwitnessed. +- **The ambient survives a failing handler** (Q#W-2 rule 5): a handler + that errors, then a subsequent unrelated dispatch, asserting the + second job does **not** carry the first's name. This is the + regression that would otherwise appear as intermittent + misattribution long after the lane lands. +- **Nesting and fan-out** (rules 3–4): a handler dispatching two jobs + gives both its name; a handler dispatching through another registered + handler gives the inner jobs the inner name and restores the outer. +- **Composition, not replacement** (rule 6): a handler calling a + dispatcher that supplies its own purpose yields `": "` + — asserted for both halves, since a test on the prefix alone passes + when the description is dropped. +- **Work dispatched from an `on_complete` callback carries no handler + name** (rule 2) — the boundary of the extent, asserted deliberately + so it reads as designed rather than broken. - **The statusline shows nothing at idle**, asserted as *absent segment*, not as empty string — a zero-width segment still consumes a separator. @@ -344,7 +490,7 @@ preference. `lean4_stage1_acceptance`) are the assertion, and they must pass untouched. **If any of them needs editing, the design is wrong**, and that is the signal to stop rather than to adjust a baseline. -- **A spawned process carries structured owner/purpose alongside its +- **A spawned process carries a required `purpose` alongside its existing `label`**, and **`label`'s current callers keep working unchanged** — `lsp:{name}` and terminal buffer names are live conventions with existing consumers. @@ -356,9 +502,11 @@ preference. **What this will NOT prove:** that background work is attributable from one place (that is Stage 2's unified view — this lane makes it *possible*, not *done*), that a terminal PTY is visible anywhere -(Q#W-4), that cancellation can range over an owner (Stage 3), or that a -third-party package's own identity flows through (Q#W-2 — blocked on -P3). +(Q#W-4), that cancellation can range over an owner (Stage 3), or **that +any job is attributed to the PACKAGE responsible for it** — `purpose` +records what work is being done and, under `pmacs.workers.dispatch`, +which registered handler it ran under. Neither is package ownership, +which waits for P3 (§3). Gates via `scripts/gate --acceptance `. **No `--protocol`**: this lane has no wire change, which is the property that @@ -366,15 +514,19 @@ lets it run beside the two lanes already in flight. ## 7. Not in scope +**`owner`, in any spelling** — including `origin` or `subsystem` (§3). +The slot stays empty until P3 can fill it with a package signal; +nothing in this lane may be promoted into it later by use. `Workspace` and `Location` fields (§7/§8 — the entities do not exist). `parent`/`children` and the ownership tree (Stage 3, Q#W-5). Scoped cancellation of any kind — cancel-all, by-kind, by-buffer, by-owner, by-subtree (Stage 3; there is nothing to range over until identity exists). The unified activity view joining the four planes (Stage 2). Making terminal PTYs visible (Stage 2, Q#W-4). Widening `JobKind` or -making it open — third-party jobs are attributed by `owner`/`purpose`, -which is the point, and reopening a closed wire-adjacent enum is a -separate decision. Latency classes and resource budgets (§9 names them; +making it open — third-party jobs are described by `purpose`, which is +the point, and reopening a closed wire-adjacent enum is a separate +decision. Latency classes and resource budgets (§9 names them; neither has a consumer yet). Supersession coverage — §9 notes parse jobs and MCP requests pass `None`, which is a real defect and a **different** -one. P3's package-ownership signal (Q#W-2 depends on it and says so). +one. P3's package-ownership signal — §3 defers `owner` to it and makes +no claim of alignment with it. From 7730f87bbad46e1519984e9259fb862afc36d133 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 14:41:38 +0200 Subject: [PATCH 03/20] docs: worker identity framing revision 3 --- the ambient must not yield Review found that revision 2 asserted the property its whole design rested on. It called the dispatch-name extent "synchronous" and never checked. A registered handler is arbitrary Lua running inside pmacs.async and may call Handle:await(), which parks the coroutine with the name still pushed --- so every tick callback and every other coroutine allocating a job in the meantime inherits it. The existing tests already await inside pcall, so this is the ordinary shape of an awaiting handler, not a corner case. Rule 1 now enforces non-yieldability instead of assuming it, and the enforcement was already in this file: Handle:await refuses to run inside pmacs.window.commit_to (async.lua:87-90), with a comment giving exactly this reasoning --- yielding "would restore the scope while this coroutine is still parked". _in_dispatch_name_scope joins _in_commit_scope in the same place. Three details decide whether the guard holds, and all three are pinned: it rejects before the park, not after; it rejects unconditionally rather than only when the handle is incomplete, because a guard keyed on whether the job happened to finish first passes under test and fails intermittently in production; and it covers both yield points. That last one is a finding. pmacs.async.yield_to_next_tick (async.lua:243-245) also yields and is public. Guarding only await would have left the hole open through a second door. Which exposes Q#W-7: the existing commit_to guard has exactly that gap today. yield_to_next_tick carries no _in_commit_scope refusal, so Journey Stage 1a Q#JR14b invariant has a second entrance. Reported rather than patched, and reachability by a real caller is explicitly UNPROVEN --- it is a code reading, not a repro. My vote is to fix it in this lane since the lane already edits that function family, but it is another lane invariant so it is a question. Q#W-5 justification is rewritten rather than left standing. It argued parent was deferrable because its ambient would span asynchronous lifetimes while this one did not --- an argument revision 2 was not entitled to make, since its own ambient could be parked by any awaiting handler. With rule 1 the distinction is real: this extent cannot be suspended, and parent cannot be rescued by refusing to yield because yielding is the mechanism it needs. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 16 ++- docs/worker-identity-framing.md | 168 +++++++++++++++++++++++++++----- 2 files changed, 158 insertions(+), 26 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 03c3567..ce25317 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -275,7 +275,7 @@ from #171 and #215. the authoritative tip** — the ref, not a SHA. Recover with `git fetch githubsucks && git checkout worker-identity-stage1`. -- **Framing `docs/worker-identity-framing.md`, revision 2**, in review. +- **Framing `docs/worker-identity-framing.md`, revision 3**, in review. Scope: `COHERENCE.md` §9's "mechanism without identity", and journey step 11 — the last of Priority 1's own work, sitting in another section's arc. @@ -289,6 +289,20 @@ the authoritative tip** — the ref, not a SHA. Recover with hand at the one place that throws it away", which was wrong about the call chain (`dispatch` → arbitrary handler → Lua wrapper → Rust binding, with the wrapper layer documented as bypassable). +- **Revision 3 took a third blocker: the ambient's extent is not + synchronous.** A handler may `Handle:await()` and park with the name + still pushed, leaking attribution to unrelated later work. Rule 1 now + **enforces** non-yieldability, modelled on the existing + `_in_commit_scope()` refusal in `Handle:await` + (`builtin/runtime/async.lua:87-90`) — rejecting before the park, + unconditionally rather than only when a yield would occur, and + covering **both** yield points. +- **Q#W-7 — a pre-existing defect found while scouting that guard, and + reported rather than patched.** `pmacs.async.yield_to_next_tick()` + (`async.lua:243-245`) is public, yields, and carries **no** + `_in_commit_scope` refusal — so Journey Stage 1a's Q#JR14b invariant + has a second entrance. Reachability by a real caller is **unproven**. + Awaiting the user's call on whether this lane fixes it. - **NO WIRE CHANGE**, which is what lets this run beside the two lanes already in flight. The statusline activity indicator is a **fourth** `pmacs.statusline.register` provider (terminal/syntax/lsp are the diff --git a/docs/worker-identity-framing.md b/docs/worker-identity-framing.md index 1b5fdb4..31f5230 100644 --- a/docs/worker-identity-framing.md +++ b/docs/worker-identity-framing.md @@ -5,9 +5,19 @@ removed that title overclaimed the lane: it answers **what**, and — under `pmacs.workers.dispatch` — **under which registered handler**. Neither is who owns it.)* -**Status: framing pass, revision 2. Pre-implementation. Awaiting +**Status: framing pass, revision 3. Pre-implementation. Awaiting approval.** +**Revision 3 closes a hole in revision 2's ambient: the extent it +called "synchronous" is not.** A registered handler is arbitrary Lua +and may `Handle:await()`, parking the coroutine with the name still +pushed so that unrelated later work inherits it. Rule 1 now **enforces** +non-yieldability rather than assuming it, following the guard this file +already carries for `pmacs.window.commit_to`. Scouting that guard +turned up a second yield point it does not cover — Q#W-7, a +pre-existing defect in another lane's invariant, reported rather than +patched in silence. + **Revision 2 removes `owner` and respecifies the handler-name path, after review found the first dishonest and the second unbuildable as described.** `owner` populated from static per-subsystem constants is @@ -129,6 +139,22 @@ drifted in one place, recorded below. output". No owner, no purpose, no parent. Callers spell it however they like (`lsp:{name}`, a terminal buffer name). +- **A dynamic scope that must not be yielded out of ALREADY EXISTS + here, guard and rationale included.** `Handle:await()` refuses to run + inside `pmacs.window.commit_to` (`builtin/runtime/async.lua:87-90`), + raising *"await: cannot await inside pmacs.window.commit_to; await + first, then commit"*. Its comment states the hazard in general terms: + yielding out of the extent "would restore the scope while this + coroutine is still parked, so the rest of the commit would resume + ambient". `commit_to` itself is "an RAII guard on the Rust stack" — + the same shape this lane needs. + +- **There are TWO yield points, not one.** `Handle:await()` yields at + `async.lua:95`; **`pmacs.async.yield_to_next_tick()` yields at + `async.lua:244`** and is public (`pmacs.async` is `async_public`, + `:247`). Any rule about a non-yieldable extent has to cover both. The + `commit_to` guard covers only the first — see Q#W-7. + And the two findings that actually shape the design: - **A statusline provider API already exists, with three Lua adopters.** @@ -290,13 +316,26 @@ ask for. **If review prefers the minimal edit**, the alternative is one more parameter on the existing pair, and the collapse becomes its own small lane. I would rather be told than assume. -### Q#W-2 — the dispatch identity path **(rewritten in rev 2)** +### Q#W-2 — the dispatch identity path **(rewritten in rev 2, rule 1 added in rev 3)** Revision 1 treated this as a parameter-passing detail. §2 shows it is not: `name` dies at `pmacs.workers.dispatch` and nothing below it takes a name, so the value must be carried *out of band* across an arbitrary handler. +**Revision 2 then called the extent "synchronous" and assumed it. +Review found that it is not.** A registered handler is arbitrary Lua +running inside `pmacs.async`, and it may call `Handle:await()` — a +legal, yieldable path that the existing tests already exercise inside +`pcall`. While a handler is parked, its pushed name **stays on the +stack**, and every tick callback and every other coroutine that +allocates a job in the meantime inherits it. That is not a corner case; +it is the ordinary shape of a handler that awaits. + +So rule 1 below is no longer an observation about how handlers happen +to behave. It is an **enforced** property, and the enforcement already +has a precedent in this exact file (§2a). + **The capture point is Rust, not Lua**, and the reason is the bypass in §2. If the ambient lived in the Lua wrapper layer, a handler calling `async_mod._dispatch_*` directly — the documented pattern for runtime @@ -311,9 +350,35 @@ runtime-internal bindings (`_push_dispatch_name` / `_pop_dispatch_name`). **The contract, in full:** -1. **Extent is the SYNCHRONOUS handler call, and nothing more.** Push - before, pop after. Every job reaching `allocate` during that window - carries the name. +1. **THE EXTENT IS NON-YIELDABLE, AND THAT IS ENFORCED, NOT ASSUMED.** + Awaiting inside a dispatch-name scope is **refused**, because + yielding would park the coroutine with the name still pushed and + hand it to whatever allocates next. + + The guard is modelled on the one already in the file (§2): + `_in_dispatch_name_scope()` joins `_in_commit_scope()` as a refusal + in the same place, with the same shape of message and the same + remedy — **await first, then dispatch**. + + Three details that decide whether the guard actually holds: + + - **It rejects BEFORE parking.** The `commit_to` guard is the first + thing in `await`, ahead of the `_is_complete` check and the + `coroutine.yield`. The new one sits beside it, for the same + reason: a guard that fires after the yield has already happened + guards nothing. + - **It rejects UNCONDITIONALLY, not only when the handle is + incomplete.** A guard that fires only when a yield would really + occur has behaviour depending on whether the job happened to + finish first — it would pass under test and fail in production, + intermittently. `commit_to`'s guard is unconditional and this one + matches it. + - **`await` is NOT the only yield point.** + `pmacs.async.yield_to_next_tick()` (`async.lua:243-245`) yields + too, and is public. It gets the same refusal. Guarding only + `await` would leave the hole open through a second door — see + Q#W-7, because the existing `commit_to` guard has exactly that + gap today. 2. **Work dispatched later is NOT covered, deliberately.** A job dispatched from an `on_complete` callback or a resumed coroutine runs ticks later, outside the extent, and carries only its own @@ -343,22 +408,29 @@ runtime-internal bindings (`_push_dispatch_name` / `_pop_dispatch_name`). **A known and accepted property, stated rather than discovered later:** the ambient captures *causal* extent, not *intent*. If a handler -synchronously triggers unrelated work — an edit that schedules a parse -— that job is inside the window and takes the name. Within a -synchronous extent I think that is the honest reading ("this ran -because that handler ran"), and it is the only definition enforceable -at a single funnel. **If review disagrees, the alternative is +triggers unrelated work within its extent — an edit that schedules a +parse — that job takes the name. Because rule 1 makes the extent +non-yieldable, that window is bounded by a single un-parked call, and +within such a window I think "this ran because that handler ran" is the +honest reading. It is also the only definition enforceable at a single +funnel. **If review disagrees, the alternative is capture-at-the-Lua-wrapper**, which is narrower and misses the raw `_dispatch_*` callers — a trade of false positives for false negatives, -and I would rather over-attribute inside a synchronous call than -silently drop the third-party case. +and I would rather over-attribute inside a bounded call than silently +drop the third-party case. **Why this ambient is admissible while Q#W-5's is not.** They are not -the same mechanism. This one is a synchronous, single-threaded, bounded -dynamic extent with a deterministic pop — a `let` binding in disguise. -A `parent` ambient must span a job's *asynchronous* lifetime, across -ticks, through callbacks that run after the parent settled. The first -is a stack; the second is a lifetime model. +the same mechanism — **and revision 2 was entitled to that claim only +after rule 1 made it true.** As written in revision 2 the extent could +be parked by any awaiting handler, which is most of the way to the +asynchronous lifetime I used as the reason for deferring `parent`. +With rule 1 the difference is real and enforced: this is a +single-threaded dynamic extent that **cannot** be suspended, with a +deterministic pop on both the normal and the error path. A `parent` +ambient must span a job's asynchronous lifetime by design — across +ticks, through callbacks that run after the parent settled — and cannot +be fixed by refusing to yield, because yielding is the whole point. The +first is a stack; the second is a lifetime model. ### Q#W-3 — what does the indicator actually show? @@ -412,21 +484,54 @@ child is dispatched. A `parent` field that nothing populates is worse than no field: it renders as `None` everywhere and reads as "this job has no parent" rather than "this system does not track parents". -**And the objection revision 2 has to answer, since it now builds an +**And the objection this has to answer, since the lane now builds an ambient of its own (Q#W-2):** why is one admissible and not the other? -Because they are not the same mechanism. Q#W-2's extent is -synchronous, single-threaded, and bounded by one function call, with a -deterministic pop on both the normal and the error path. A `parent` +Because Q#W-2's extent **cannot be suspended** — rule 1 refuses both +yield points, so it is bounded by one un-parked call with a +deterministic pop on the normal and the error path. + +**That distinction is only load-bearing because rule 1 exists.** +Revision 2 asserted this same paragraph while its ambient *could* be +parked by any awaiting handler, which made the two mechanisms far more +alike than the argument admitted. The honest version: a `parent` ambient must identify the running job *across ticks* — a job dispatched from an `on_complete` callback should name the job whose completion -fired it, and that callback runs after the parent settled, on the main -thread, outside any dispatch call. That is a lifetime model, not a -stack, and it is Stage 3's subject rather than a field this lane can -add cheaply. +fired it, and that callback runs after the parent settled, outside any +dispatch call. Refusing to yield cannot rescue it, because yielding is +the mechanism it needs. That is a lifetime model, not a stack, and it +is Stage 3's subject rather than a field this lane can add cheaply. Stage 3 builds the lifetime model and the field together, where the field can be tested by a populated case. +### Q#W-7 — the same hole exists in `commit_to` today **(new in rev 3)** + +Found while scouting rule 1, and reported rather than quietly patched. + +`Handle:await()` refuses to run inside `pmacs.window.commit_to` +(`async.lua:87-90`) precisely so a coroutine cannot park with the +frontend scope pushed. **But `pmacs.async.yield_to_next_tick()` +(`async.lua:243-245`) also yields, is public, and carries no such +refusal.** A coroutine inside `commit_to` can therefore park through +that door and produce exactly the misrouting the `await` guard exists +to prevent. Journey Stage 1a's Q#JR14b invariant has a second entrance. + +I have **not** verified that a real caller does this — the reachability +of the bug is unproven, and I would rather say so than dress a +code-reading up as a repro. + +*My vote: **fix it in this lane, in the same commit as rule 1.*** It is +one refusal in a function this lane is already editing, in the same +family, for the same reason. The alternative — ship a document that +explains the hazard in detail, add the guard for the new scope, and +leave the identical gap open beside it — is how a codebase teaches its +next reader that the rule is optional. + +**But it is another lane's invariant**, so it is a question rather than +an assumption. If review prefers it separate, it should be its own +small lane *before* this one, and this framing should say so; what it +should not be is discovered a third time. + ### Q#W-6 — is any of this configurable? *My vote: **one boolean, `ui.activity-indicator` (default `true`), @@ -459,6 +564,19 @@ preference. dispatcher** — not a synthetic funnel test. A test that pushes the ambient by hand proves the stack works and leaves the actual defect (`name` dying in an arbitrary handler) unwitnessed. +- **Awaiting inside a handler is REFUSED, and the scope restores after + the refusal** (Q#W-2 rule 1). Two assertions, and the second is the + load-bearing one: a guard that raises but leaves the name pushed has + converted a silent misattribution into a silent misattribution plus + an error. The witness dispatches again after the rejection and + asserts the new job carries **no** stale name. +- **`pmacs.async.yield_to_next_tick()` inside a handler is refused + too**, with the same restore-after assertion. Guarding one yield + point and not the other leaves the hole open through a second door + (§2). +- **The refusal fires even when the awaited handle is already + complete** (rule 1) — the case that separates an unconditional guard + from one whose behaviour depends on a race. - **The ambient survives a failing handler** (Q#W-2 rule 5): a handler that errors, then a subsequent unrelated dispatch, asserting the second job does **not** carry the first's name. This is the From 26a26006bb6a76b1f96b1830242c1ece3dffe329 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 15:08:41 +0200 Subject: [PATCH 04/20] docs: worker identity framing revision 4 --- scope rule 1, take Q#W-7 Two changes, both from review, both about claiming exactly what is enforced and no more. Rule 1 said it covered "all yield points". It covers the two supported pmacs yield APIs. Raw coroutine.yield remains reachable: R46 is a convention ("package code uses :await() rather than coroutine.yield", async.lua:26-27), not an enforcement, and the scheduler diagnoses a non-Handle yield only after the fact --- step() resumes at :197 and inspects what came back at :212, by which point the coroutine has already suspended and the enclosing dispatch never returns to run its pop. No refusal sited in a yield helper can intercept that. So the property is stated as what it is: the supported ways to yield are refused inside the scope, and an R46 violation can still leak the name --- loudly, through pmacs.error into *errors*, but unrestored. Section 6 says explicitly that this is NOT asserted, because a test implying coverage the design lacks is worse than the recorded gap. Q#W-7 is approved into this lane rather than split out. Same supported helper, same invariant, same async.lua edit family; splitting would preserve a known hole without reducing integration risk. So yield_to_next_tick gains both refusals --- the new _in_dispatch_name_scope and the missing _in_commit_scope --- and the commit_to gap closes in the same commit as rule 1. Its witnesses are the same pair as rule 1: the refusal fires AND the scope restores, on the reasoning that a guard which raises while leaving the scope pushed trades a silent fault for a loud one and fixes neither. Reachability by a real caller stays UNPROVEN and the framing says so in three places, including here. The defect was found by reading; the tests pin the guard rather than reproducing a user-visible bug. Nobody should later cite this as evidence the bug was observed. The causal-extent paragraph and the Q#W-5 comparison were both re-scoped to match, since both leaned on "non-yieldable" as an unqualified property. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 23 +++++-- docs/worker-identity-framing.md | 118 +++++++++++++++++++++++--------- 2 files changed, 104 insertions(+), 37 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index ce25317..b1a2cc7 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -265,7 +265,7 @@ also removed: this branch's "R8 NEEDS A LANE" investigation block, and durable facts are in the retired registry row and the handoff §6 census. -## Worker identity Stage 1 (§9) — BRANCHED, framing in review +## Worker identity Stage 1 (§9) — BRANCHED, pre-implementation **Written with the lane's first commit**, per the standing correction from #171 and #215. @@ -275,7 +275,8 @@ from #171 and #215. the authoritative tip** — the ref, not a SHA. Recover with `git fetch githubsucks && git checkout worker-identity-stage1`. -- **Framing `docs/worker-identity-framing.md`, revision 3**, in review. +- **Framing `docs/worker-identity-framing.md`, revision 4, APPROVED + 2026-08-09** after four review rounds. Scope: `COHERENCE.md` §9's "mechanism without identity", and journey step 11 — the last of Priority 1's own work, sitting in another section's arc. @@ -298,11 +299,23 @@ the authoritative tip** — the ref, not a SHA. Recover with unconditionally rather than only when a yield would occur, and covering **both** yield points. - **Q#W-7 — a pre-existing defect found while scouting that guard, and - reported rather than patched.** `pmacs.async.yield_to_next_tick()` + APPROVED for repair in this lane.** `pmacs.async.yield_to_next_tick()` (`async.lua:243-245`) is public, yields, and carries **no** `_in_commit_scope` refusal — so Journey Stage 1a's Q#JR14b invariant - has a second entrance. Reachability by a real caller is **unproven**. - Awaiting the user's call on whether this lane fixes it. + has a second entrance. Same helper, same invariant, same edit family, + so splitting it would have preserved a known hole without reducing + integration risk. **Reachability by a real caller is UNPROVEN** — the + defect was found by reading, and the tests pin the guard rather than + reproducing a user-visible bug. That belongs in the commit message so + nobody later cites this as an observed failure. +- **Revision 4 also scoped rule 1's claim to what it enforces.** + Revision 3 said "all yield points"; it covers **the two supported + pmacs yield APIs**. Raw `coroutine.yield` stays reachable — R46 is a + convention, and the scheduler diagnoses a non-Handle yield only after + the coroutine has suspended (`async.lua:197` resumes, `:212` + inspects), so no refusal in a yield helper can intercept it. Recorded + as a residual, and explicitly **not** covered by a test that would + imply otherwise. - **NO WIRE CHANGE**, which is what lets this run beside the two lanes already in flight. The statusline activity indicator is a **fourth** `pmacs.statusline.register` provider (terminal/syntax/lsp are the diff --git a/docs/worker-identity-framing.md b/docs/worker-identity-framing.md index 31f5230..d86d502 100644 --- a/docs/worker-identity-framing.md +++ b/docs/worker-identity-framing.md @@ -5,8 +5,17 @@ removed that title overclaimed the lane: it answers **what**, and — under `pmacs.workers.dispatch` — **under which registered handler**. Neither is who owns it.)* -**Status: framing pass, revision 3. Pre-implementation. Awaiting -approval.** +**Status: revision 4, APPROVED 2026-08-09. Implementation may +proceed.** + +**Revision 4 scopes rule 1's claim to what it can actually enforce, and +takes Q#W-7 into this lane.** Revision 3 said the rule covered "all +yield points"; it covers **the two supported pmacs yield APIs**. Raw +`coroutine.yield` stays reachable — R46 is a convention, and the +scheduler diagnoses a non-Handle yield only *after* the coroutine has +suspended (`async.lua:197` resumes, `:212` inspects), so no refusal +sited in a yield helper can intercept it. The residual is named in §2 +rather than papered over. **Revision 3 closes a hole in revision 2's ambient: the extent it called "synchronous" is not.** A registered handler is arbitrary Lua @@ -14,9 +23,11 @@ and may `Handle:await()`, parking the coroutine with the name still pushed so that unrelated later work inherits it. Rule 1 now **enforces** non-yieldability rather than assuming it, following the guard this file already carries for `pmacs.window.commit_to`. Scouting that guard -turned up a second yield point it does not cover — Q#W-7, a -pre-existing defect in another lane's invariant, reported rather than -patched in silence. +turned up a second supported yield API it does not cover — Q#W-7, a +pre-existing defect in another lane's invariant. Revision 3 reported it +rather than patching it in silence; **revision 4 fixes it here, on +approval**, since it is the same helper, the same invariant and the +same edit family. **Revision 2 removes `owner` and respecifies the handler-name path, after review found the first dishonest and the second unbuildable as @@ -149,12 +160,28 @@ drifted in one place, recorded below. ambient". `commit_to` itself is "an RAII guard on the Rust stack" — the same shape this lane needs. -- **There are TWO yield points, not one.** `Handle:await()` yields at - `async.lua:95`; **`pmacs.async.yield_to_next_tick()` yields at - `async.lua:244`** and is public (`pmacs.async` is `async_public`, +- **There are TWO SUPPORTED yield APIs, not one.** `Handle:await()` + yields at `async.lua:95`; **`pmacs.async.yield_to_next_tick()` yields + at `async.lua:244`** and is public (`pmacs.async` is `async_public`, `:247`). Any rule about a non-yieldable extent has to cover both. The `commit_to` guard covers only the first — see Q#W-7. +- **Raw `coroutine.yield` remains reachable, and NO guard of this shape + can cover it.** R46 is a convention — *"package code uses `:await()` + rather than `coroutine.yield`"* (`async.lua:26-27`) — not an + enforcement. The scheduler does diagnose a non-Handle yield + (`async.lua:217-223`, *"use Handle:await() per R46"*), **but only + after the fact**: `step` calls `coroutine.resume(co)` at `:197` and + inspects what came back at `:212`, by which point the coroutine has + already suspended. A refusal placed in a yield helper is never + consulted, and the enclosing `pmacs.workers.dispatch` never returns + to run its pop. + + So the honest bound is: a package that violates R46 *inside* a + dispatch-name scope can leak the name. It is not silent — the + scheduler raises it through `pmacs.error` into `*errors*` — but the + scope is not restored, and this framing does not claim otherwise. + And the two findings that actually shape the design: - **A statusline provider API already exists, with three Lua adopters.** @@ -373,12 +400,20 @@ runtime-internal bindings (`_push_dispatch_name` / `_pop_dispatch_name`). finish first — it would pass under test and fail in production, intermittently. `commit_to`'s guard is unconditional and this one matches it. - - **`await` is NOT the only yield point.** - `pmacs.async.yield_to_next_tick()` (`async.lua:243-245`) yields - too, and is public. It gets the same refusal. Guarding only - `await` would leave the hole open through a second door — see - Q#W-7, because the existing `commit_to` guard has exactly that - gap today. + - **It covers BOTH SUPPORTED YIELD APIs — and that is the exact + extent of the claim.** `pmacs.async.yield_to_next_tick()` + (`async.lua:243-245`) yields too, and is public, so it gets the + same refusal; guarding only `await` would leave the hole open + through a second door (and Q#W-7 is the proof that this happens, + because `commit_to` has exactly that gap today). + + **What rule 1 does NOT cover is raw `coroutine.yield`** (§2). + R46 forbids it to package code by convention only, and the + scheduler's diagnostic fires *after* suspension, so no refusal + sited in a yield helper can intercept it. Revision 3 said "all + yield points" and was overclaiming. The property is: **the + supported ways to yield are refused inside the scope; an R46 + violation can still leak the name, loudly.** 2. **Work dispatched later is NOT covered, deliberately.** A job dispatched from an `on_complete` callback or a resumed coroutine runs ticks later, outside the extent, and carries only its own @@ -409,10 +444,11 @@ runtime-internal bindings (`_push_dispatch_name` / `_pop_dispatch_name`). **A known and accepted property, stated rather than discovered later:** the ambient captures *causal* extent, not *intent*. If a handler triggers unrelated work within its extent — an edit that schedules a -parse — that job takes the name. Because rule 1 makes the extent -non-yieldable, that window is bounded by a single un-parked call, and -within such a window I think "this ran because that handler ran" is the -honest reading. It is also the only definition enforceable at a single +parse — that job takes the name. Because rule 1 refuses both supported +yield APIs, that window is bounded by a single un-parked call for any +caller obeying R46, and within such a window I think "this ran because +that handler ran" is the honest reading. (A caller violating R46 is +outside this property, and outside rule 1 — §2.) It is also the only definition enforceable at a single funnel. **If review disagrees, the alternative is capture-at-the-Lua-wrapper**, which is narrower and misses the raw `_dispatch_*` callers — a trade of false positives for false negatives, @@ -504,7 +540,7 @@ is Stage 3's subject rather than a field this lane can add cheaply. Stage 3 builds the lifetime model and the field together, where the field can be tested by a populated case. -### Q#W-7 — the same hole exists in `commit_to` today **(new in rev 3)** +### Q#W-7 — the same hole exists in `commit_to` today — **RESOLVED, fixed here (rev 4)** Found while scouting rule 1, and reported rather than quietly patched. @@ -520,17 +556,22 @@ I have **not** verified that a real caller does this — the reachability of the bug is unproven, and I would rather say so than dress a code-reading up as a repro. -*My vote: **fix it in this lane, in the same commit as rule 1.*** It is -one refusal in a function this lane is already editing, in the same -family, for the same reason. The alternative — ship a document that -explains the hazard in detail, add the guard for the new scope, and -leave the identical gap open beside it — is how a codebase teaches its -next reader that the rule is optional. +**RESOLVED — approved for this lane.** It is the same supported yield +helper, the same invariant, and the same `async.lua` edit family; +splitting it would preserve a known hole without reducing integration +risk. So `yield_to_next_tick` gains **both** refusals — the new +`_in_dispatch_name_scope()` and the missing `_in_commit_scope()` — and +the `commit_to` gap closes in the same commit as rule 1. -**But it is another lane's invariant**, so it is a question rather than -an assumption. If review prefers it separate, it should be its own -small lane *before* this one, and this framing should say so; what it -should not be is discovered a third time. +**Its witnesses are the same pair as rule 1's, not a smoke test:** the +refusal fires, **and** the commit scope is restored afterwards. A guard +that raises while leaving the scope pushed converts a silent misrouting +into a noisy one and fixes nothing. + +Reachability by a real caller stays **unproven** — this is a defect +found by reading, and the tests pin the guard rather than reproducing a +user-visible bug. That distinction belongs in the commit message too, +so nobody later cites this as evidence the bug was observed. ### Q#W-6 — is any of this configurable? @@ -571,9 +612,18 @@ preference. an error. The witness dispatches again after the rejection and asserts the new job carries **no** stale name. - **`pmacs.async.yield_to_next_tick()` inside a handler is refused - too**, with the same restore-after assertion. Guarding one yield - point and not the other leaves the hole open through a second door - (§2). + too**, with the same restore-after assertion. Guarding one supported + yield API and not the other leaves the hole open through a second + door (§2). +- **`yield_to_next_tick` inside `pmacs.window.commit_to` is refused, + and the commit scope restores after the refusal** (Q#W-7) — the + pre-existing gap, closed here. Both halves asserted, for the same + reason as rule 1's: a refusal that leaves the scope pushed has + swapped a silent fault for a loud one. +- **NOT asserted, and deliberately: that a raw `coroutine.yield` + inside either scope is prevented.** It is not (§2). Writing a test + that "proves" coverage this design does not have would be worse than + the gap, and the gap is recorded instead. - **The refusal fires even when the awaited handle is already complete** (rule 1) — the case that separates an unconditional guard from one whose behaviour depends on a race. @@ -632,6 +682,10 @@ lets it run beside the two lanes already in flight. ## 7. Not in scope +**Making raw `coroutine.yield` safe inside either dynamic scope** (§2, +rule 1). R46 forbids it by convention and the scheduler diagnoses it +after the fact; closing it properly means enforcement the runtime does +not have, and this lane claims only the two supported yield APIs. **`owner`, in any spelling** — including `origin` or `subsystem` (§3). The slot stays empty until P3 can fill it with a package signal; nothing in this lane may be promoted into it later by use. From 689fb8333dbefd3a9731fedfc989cb7bad773b0d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 16:33:41 +0200 Subject: [PATCH 05/20] =?UTF-8?q?feat(workers):=20a=20required=20purpose?= =?UTF-8?q?=20on=20every=20job=20and=20process=20=E2=80=94=20worker=20iden?= =?UTF-8?q?tity=20Stage=201?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `COHERENCE.md` §9 grades the worker model "mechanism without identity", and §0 names step 11 (background-work ownership) as one of the two remaining thin ends of the golden journey. The mechanism half is solid — cancellation, supersession, streaming, frame-aware draining, `*workers*`. The identity half was absent: `PendingJob` carried no description of what it was doing, `pmacs.workers.dispatch` discarded the registered handler name three layers above anything that takes one, and §9's "no progress indicator exists anywhere" was checkable and true. Framing: `docs/worker-identity-framing.md` (revision 4, approved). What lands: **A required `purpose`, on the job and on the process.** Non-optional, with no `Default`, so the compiler — not a test — is what proves every dispatcher supplied one. `allocate` / `allocate_with_resource` collapse into ONE private `JobSpec`-taking funnel (Q#W-1): the two-function split existed only because one prior lane needed one extra parameter, and a second lane doing the same produces `allocate_with_resource_and_identity`. `register_external` gains a `purpose` parameter rather than deriving one, because its `JobKind` is `McpRequest`/`LspRequest` for every method — a category, not a description. **A dispatch-name ambient (Q#W-2), read at that same single funnel.** The capture point is Rust, not the Lua wrapper layer, because a handler reaching straight for `pmacs._async._dispatch_*` bypasses the wrappers entirely — and those are precisely the callers attribution exists for. Seven rules; the ones that decide whether it is honest: - **Rule 1 — the extent is NON-YIELDABLE, and that is ENFORCED.** Both supported yield APIs refuse inside it, modelled on the `commit_to` refusal already in `async.lua`. The guards reject BEFORE parking and reject UNCONDITIONALLY: one placed after `_is_complete` would fire only when a yield really occurred, passing under test and failing intermittently in production. - **A raw `coroutine.yield` is NOT covered, and nothing here claims it is.** R46 is a convention, and the scheduler inspects the yielded value only after `coroutine.resume` returns — by which point the coroutine has already suspended — so no refusal sited in a yield helper is ever consulted. The residual is recorded in the framing §2 and in the suite's module docs rather than papered over with a test that would imply coverage this design lacks. - **Rule 5 — unwind-safe.** A raising handler still pops. A version that did not would let one failure poison every later dispatch in the session with a stale name: the feature would stop failing loudly and start lying silently. The bracketing also has to preserve the tail call it replaced: `dispatch` was `return handler(args, opts)` and propagated EVERY return value, so the pop/rethrow runs behind a varargs boundary rather than a `local ok, result = pcall(...)` that would silently truncate a multi-value handler. Varargs rather than `table.pack`, because that is Lua 5.2 surface and LuaJIT is this project's default backend. - **Rule 6 — compose, do not replace.** `": "`, because letting the dispatcher's purpose win loses the third party again and letting the name win discards the only description of the actual work. **A statusline activity indicator** — the fourth `pmacs.statusline.register` adopter, after `mode`, `terminal` and `lsp`. A count plus the OLDEST in-flight job's purpose ("busiest" is not a defined quantity; jobs carry no cost estimate), and **absent entirely** when idle rather than a zero-width segment that costs modeline width forever to say nothing is happening. Gated by one setting, `ui.activity-indicator` (boolean, default true, Q#W-6) — a permanently-visible modeline element is a preference someone genuinely holds on day one. No setting for purpose capture itself: that is substrate. **NO WIRE CHANGE.** The indicator rides the existing `StatuslineSegments` vector, so a fourth provider adds an element, not a variant. `PROTOCOL_VERSION` and `ADVERTISED_PROTOCOL_VERSION` are untouched — which is the property that lets this run beside the two lanes holding the bump slot. **Q#W-7 — a pre-existing defect, repaired here, and NOT one anybody has observed.** `Handle:await()` refuses inside `pmacs.window.commit_to` precisely so a coroutine cannot park with the frontend scope pushed (Journey Stage 1a, Q#JR14b). But `pmacs.async.yield_to_next_tick()` also yields, is public, and carried no such refusal — so that invariant had a second entrance, and a coroutine could produce exactly the misrouting the `await` guard exists to prevent. It gains both refusals here: the same supported yield helper, the same invariant, the same edit family, so splitting it would have preserved a known hole without reducing integration risk. **Reachability by a real caller is UNPROVEN.** This was found by reading the guard family while scouting rule 1, not by reproducing a fault. No production caller is known to yield through that door inside a commit, and the test pins the guard rather than reproducing a user-visible bug. Nobody should later cite this commit as evidence the bug was observed in the wild. Its witness is a PAIR, like rule 1's: the refusal fires **and** the commit scope is restored afterwards — a guard that raises while leaving the scope pushed converts a silent fault into a loud one and fixes neither. `journey_acceptance` carries the established `commit_to` pins — forged-destination refusal, scope-and-restore on normal return and on raise, the await refusal, delivery to the requesting frontend. It passes **untouched**, which is what says this closed a gap in Journey Stage 1a's semantics rather than altering them. What is deliberately NOT here, and why it is worth saying: - **No `owner`, in any spelling** — not `origin`, not `subsystem` (§3). Populated from static per-subsystem constants it would be an origin, not an owner, and would confidently misattribute third-party work to a builtin at exactly the point §9 wants attribution. A field that asserts a falsehood is worse than an absent one. The slot stays empty until P3 can fill it with a real package signal. - **No `parent`** (Q#W-5). An unpopulated field renders as `None` everywhere and reads as "this job has no parent" rather than "this system does not track parents". Stage 3 builds the lifetime model and the field together. Consequences worth recording: - `ProcessSpec::new` takes a third argument. The 40-odd call sites are almost all tests; the three production ones (LSP, MCP, terminal) supply real descriptions. `pmacs.process.spawn`'s Lua surface keeps `purpose` OPTIONAL, falling back to the label — requiring it there would break every existing caller for no coverage the compiler is not already providing, and a caller's own label is not a fabrication. - `pmacs.process.list` gains a `purpose` KEY on each row and enumerates exactly the same processes (Q#W-4). Terminal PTYs stay hidden: three acceptance suites use `#pmacs.process.list()` as a leak baseline, and widening the accessor would inflate all three. Stage 2's unified view owns that decision. - `statusline_segments_acceptance`'s builtin-provider inventory grows to `["activity", "mode", "terminal", "lsp"]`. That assertion exists to grow when a builtin provider is added. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- builtin/runtime/async.lua | 150 ++++- pmacs-gpu/src/main.rs | 89 +++ src/async_runtime.rs | 378 +++++++++-- src/lsp.rs | 21 +- src/lua_bindings/mod.rs | 98 ++- src/mcp.rs | 38 +- src/process.rs | 95 ++- src/terminal/session.rs | 3 +- src/workers_buffer.rs | 73 +- tests/m4_acceptance.rs | 18 +- tests/statusline_segments_acceptance.rs | 7 +- tests/vterm_stage1_acceptance.rs | 2 +- tests/worker_identity_acceptance.rs | 849 ++++++++++++++++++++++++ 13 files changed, 1682 insertions(+), 139 deletions(-) create mode 100644 tests/worker_identity_acceptance.rs diff --git a/builtin/runtime/async.lua b/builtin/runtime/async.lua index af74cc1..000be49 100644 --- a/builtin/runtime/async.lua +++ b/builtin/runtime/async.lua @@ -88,6 +88,28 @@ function Handle:await() error("await: cannot await inside pmacs.window.commit_to; " .. "await first, then commit") end + -- Worker identity Stage 1 (Q#W-2 rule 1): `pmacs.workers.dispatch` + -- pushes the registered handler's name for the dynamic extent of the + -- handler call, so that jobs allocated inside it are attributable to + -- the third party that asked for them. Parking here would leave the + -- name pushed while this coroutine is suspended, and every job + -- allocated in the meantime --- in any coroutine, on any later tick + -- --- would inherit it. Same hazard, same shape, same remedy as the + -- commit-scope refusal above. + -- + -- Two properties this placement buys, both load-bearing: + -- + -- * it rejects BEFORE parking (ahead of the `_is_complete` check and + -- the `coroutine.yield`), because a guard consulted after the yield + -- has already happened guards nothing; + -- * it rejects UNCONDITIONALLY, not only when a yield would really + -- occur. A guard that fires only for an incomplete handle would + -- pass or fail depending on whether the job happened to settle + -- first --- green under test, intermittent in production. + if async_mod._in_dispatch_name_scope() then + error("await: cannot await inside pmacs.workers.dispatch; " .. + "await first, then dispatch") + end if not async_mod._is_complete(self._id) then -- Yield self so pmacs.async's step() can park us. R46 carve-out: -- this `coroutine.yield` is runtime code; package code uses @@ -240,7 +262,28 @@ setmetatable(async_public, { end, }) +-- The SECOND supported yield API. `Handle:await()` is the first; any +-- rule about a non-yieldable dynamic extent has to cover both, or the +-- extent stays open through a second door. +-- +-- Both refusals below are that rule. The commit-scope one is a +-- **pre-existing gap being closed** (worker identity framing Q#W-7): +-- Journey Stage 1a's Q#JR14b invariant was enforced on `:await()` only, +-- so a coroutine inside `pmacs.window.commit_to` could park through here +-- and produce exactly the misrouting that guard exists to prevent. +-- +-- Placement is the whole point: both fire *before* the `coroutine.yield` +-- below, and both fire unconditionally. A refusal sited after the yield +-- would never run in the case it exists for. function async_public.yield_to_next_tick() + if async_mod._in_commit_scope() then + error("yield_to_next_tick: cannot yield inside pmacs.window.commit_to; " .. + "yield first, then commit") + end + if async_mod._in_dispatch_name_scope() then + error("yield_to_next_tick: cannot yield inside pmacs.workers.dispatch; " .. + "yield first, then dispatch") + end coroutine.yield({ _is_pmacs_next_tick = true }) end @@ -366,12 +409,63 @@ local handlers = { end, } +-- Worker identity Stage 1 (Q#W-2): `name` used to die here. +-- +-- The audit's "every third-party job renders under a builtin's label" is +-- exact, and the reason is this function: the handler is arbitrary Lua, +-- nothing below it takes a name, and a handler that reaches straight for +-- `pmacs._async._dispatch_*` bypasses the wrapper layer entirely. So the +-- name is pushed onto a runtime-owned stack for the dynamic extent of +-- the handler call and read at `allocate`, the single funnel every job +-- passes through. Seven rules govern it; five are visible here: +-- +-- 1. The extent is NON-YIELDABLE, and that is enforced rather than +-- assumed --- see the refusals in `Handle:await` and +-- `pmacs.async.yield_to_next_tick`. +-- 3. Nesting is a stack; innermost wins. +-- 4. Fan-out shares the name: five jobs dispatched by one handler are +-- five jobs named alike. They *were* all dispatched under it. +-- 5. UNWIND-SAFE, and this is the one that makes a naive version worse +-- than none. A handler that raises must still pop --- otherwise one +-- failure poisons every subsequent dispatch in the session with a +-- stale name, and the feature starts lying silently instead of +-- failing loudly. Hence pcall, pop, rethrow. +-- 7. Outside any extent nothing changes: a builtin invoked directly +-- records its own purpose. +-- +-- Rule 2 (work dispatched later, from an `on_complete` callback or a +-- resumed coroutine, is deliberately NOT covered) and rule 6 +-- (composition, `": "`) live on the Rust side. +-- +-- The pop/rethrow half, hoisted so it is written once and allocates +-- nothing per dispatch. +-- +-- Varargs across a function boundary, NOT `local ok, result = pcall(…)`: +-- this function used to be `return handler(args, opts)`, which +-- propagates EVERY return value, and bracketing it must not silently +-- truncate a handler that returns more than one. `table.pack` / +-- `table.unpack` would say the same thing but are Lua 5.2 surface, and +-- LuaJIT is this project's default backend (`Cargo.toml`: +-- `default = ["luajit"]`). +local function finish_dispatch(ok, ...) + async_mod._pop_dispatch_name() + if not ok then + -- Level 0: the handler's error travels unchanged. R45's structured + -- errors are tables, and a re-raise that appended position info + -- would corrupt a plain-string error and be silently ignored for a + -- table one --- so neither shape is served by the default level. + error((...), 0) + end + return ... +end + function pmacs.workers.dispatch(name, args, opts) local handler = handlers[name] if handler == nil then error("pmacs.workers.dispatch: unknown handler '" .. tostring(name) .. "'") end - return handler(args, opts) + async_mod._push_dispatch_name(name) + return finish_dispatch(pcall(handler, args, opts)) end function pmacs.workers.register(name, handler) @@ -581,6 +675,60 @@ function pmacs._async.tick() end end +-- --------------------------------------------------------------------------- +-- Statusline activity indicator (worker identity Stage 1, Q#W-3/Q#W-6). +-- --------------------------------------------------------------------------- +-- +-- `COHERENCE.md` §9 records that no progress indicator exists anywhere +-- --- no spinner, no busy count --- which makes §3's promise of "visible +-- asynchronous work" false unless the user knows to run +-- `M-x editor.list-workers`. This is the fourth `pmacs.statusline.register` +-- adopter (after `mode`, `terminal` and `lsp`) and the first thing that +-- makes background work visible without a command. +-- +-- No wire change: `pmacs.statusline.register` rides the existing +-- `StatuslineSegments` vector, so a fourth provider adds an ELEMENT, not +-- a variant. That is what lets this lane run beside the two holding the +-- protocol-bump slot. + +-- A visibility toggle, and only that (Q#W-6). A permanently-visible +-- statusline element is different in kind from an internal behaviour: it +-- costs modeline width on every frame, and "I do not want this in my +-- modeline" is a preference someone genuinely holds on day one. There is +-- deliberately NO setting for purpose capture itself --- that is +-- substrate, not preference. +pmacs.config.define { + name = "ui.activity-indicator", + description = "Show a modeline count of in-flight background jobs, with the oldest job's purpose. Absent entirely when nothing is running.", + type = "boolean", + default = true, + mutability = "live", +} + +pmacs.statusline.register { + name = "activity", + side = "right", + -- Above `terminal` (10) and `lsp` (0): when the modeline is too narrow + -- for everything, "the editor is busy, on this" is the segment worth + -- keeping. Right-side display order is priority-ascending, so it also + -- lands nearest the protected cursor/scroll group. + priority = 20, + face = "ui.modeline.activity", + fn = function(_ctx) + if pmacs.config.get("ui.activity-indicator") ~= true then return nil end + -- `_activity_summary` rather than `pmacs.workers.snapshot()`: this + -- runs once per visible window per frame, and a snapshot would clone + -- the whole 64-entry completed ring that the indicator never reads. + local summary = async_mod._activity_summary() + -- nil, not "" and not "0 jobs": the evaluator treats an empty string + -- as "no segment" too, but a zero-count string would be a segment + -- that costs width forever to say nothing is happening. Absence is + -- the design (Q#W-3), so absence is what this returns. + if summary == nil then return nil end + return "⋯" .. tostring(summary.in_flight) .. " " .. summary.purpose + end, +} + -- Diagnostic / test helpers: number of parked coroutines, number of -- pending Rust-side jobs. Used by Rust integration tests to drive the -- runtime to quiescence. diff --git a/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index 78eb64a..8f5d817 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -14939,6 +14939,95 @@ mod tests { assert_eq!(after[2].1, Color::rgb(20, 220, 40)); } + /// Worker identity Stage 1 (`docs/worker-identity-framing.md` §6): + /// the GPU half of "both frontends render the segment". + /// + /// The activity indicator adds no wire message — it rides the + /// existing `StatuslineSegments` vector as a fourth provider's + /// element. But that is a claim about the **producer**, and says + /// nothing about whether a consumer draws it, which is why this + /// exists on the consumer side. + /// + /// Two properties specific to this segment, neither of which the + /// existing rich-runs test covers: + /// + /// * its face (`ui.modeline.activity`) is **deliberately absent + /// from `ThemeFacts`** — no theme sets it, and `theme_facts_msg` + /// ships only faces that resolve — so a consumer that dropped + /// segments with an unknown face would silently lose the one + /// thing telling the user the editor is busy; + /// * its text leads with a non-ASCII `⋯`, which a byte-oriented + /// composition step would mangle. + #[test] + fn the_activity_segment_survives_an_unthemed_face_and_a_non_ascii_lead() { + let Some(mut state) = headless_or_skip(500, 280, "text") else { + return; + }; + let buffer_id = BufferId::next(); + state.current_buffer_id = Some(buffer_id); + state.status_facts = Some(status_facts(buffer_id, None)); + state.own_cursor = Some(OwnCursor { buffer_id, byte: 0 }); + // One themed face, and NOT the activity one: the point is that + // the theme has an opinion about some segments and none about + // this one. + apply_faces( + &mut state, + vec![theme_face( + "ui.modeline.lsp", + CellStyle { + fg: CellColor::Rgb(20, 220, 40), + ..CellStyle::default() + }, + )], + ); + apply_statusline( + &mut state, + buffer_id, + Vec::new(), + vec![ + statusline_segment("LSP:rust", "ui.modeline.lsp"), + statusline_segment("⋯2 lsp textDocument/definition", "ui.modeline.activity"), + ], + ); + + let right = state.compose_status_runs(); + let text: String = right.iter().map(|(text, _)| text.as_str()).collect(); + assert!( + text.contains("⋯2 lsp textDocument/definition"), + "the activity segment must reach the composed right runs \ + intact: {text:?}" + ); + let activity = right + .iter() + .find(|(run, _)| run.contains('⋯')) + .expect("activity run"); + assert_eq!( + activity.1, + state.status_right_base_color(), + "an unthemed modeline face falls back to the base colour \ + rather than dropping the segment" + ); + assert_eq!( + right[0].1, + Color::rgb(20, 220, 40), + "and its themed neighbour still takes its own colour" + ); + + // And it survives the real shaping pass, not only composition. + let _ = state.render_offscreen(); + let shaped: String = state + .status_runs + .as_ref() + .expect("right shaped") + .iter() + .map(|(text, _)| text.as_str()) + .collect(); + assert!( + shaped.contains("⋯2 lsp textDocument/definition"), + "{shaped:?}" + ); + } + #[test] fn modal_left_precedence_suppresses_custom_left_but_preserves_right() { let Some(mut state) = headless_or_skip(420, 260, "text") else { diff --git a/src/async_runtime.rs b/src/async_runtime.rs index 3620a32..e60d0ae 100644 --- a/src/async_runtime.rs +++ b/src/async_runtime.rs @@ -408,6 +408,47 @@ struct PendingJob { /// job→buffer link already lives in a side map and §9 names that as /// the defect. resource: Option, + /// What this job is doing, in words a user can read (worker + /// identity Stage 1, `COHERENCE.md` §9). + /// + /// **Not an owner.** It records *what work* is running and — when + /// the job was born inside a `pmacs.workers.dispatch` extent — the + /// registered handler name it ran under. Neither is the package + /// responsible for it; that slot is deliberately empty until P3 can + /// fill it with a real package signal (framing §3). + /// + /// Non-optional by construction: [`JobSpec`] has no `Default`, so a + /// dispatcher that supplies none does not compile. + purpose: String, +} + +/// Everything one job is born with. +/// +/// **Private, and deliberately so** (framing Q#W-1). The two-function +/// `allocate` / `allocate_with_resource` split existed only because one +/// prior lane needed one extra parameter; a second lane doing the same +/// produces `allocate_with_resource_and_identity`. Collapsing the pair +/// into a struct means the next field is a named literal at each of the +/// eleven construction sites rather than another positional parameter on +/// a public signature. +/// +/// **There is no `Default` impl, and that is the point.** `purpose` is +/// what makes the compiler — not a test — the thing that proves every +/// dispatcher supplied one (framing §6). A `Default` would let a new +/// dispatcher write `..Default::default()` and silently ship an empty +/// identity. +struct JobSpec<'a> { + /// Which builtin handler this job runs. + kind: JobKind, + /// Supersede key, if the dispatch opted into supersession. + supersede: Option<&'a str>, + /// `Some(max_batch)` marks this as a streaming dispatch. + stream: Option, + /// Filesystem mutation this job performs, for the settle-time + /// reconcile (dired Stage 2a). + resource: Option, + /// What the job is doing. See [`PendingJob::purpose`]. + purpose: String, } /// A settled filesystem mutation, with the paths the worker consumed @@ -490,6 +531,9 @@ pub struct ActiveJobInfo { /// True if this is a streaming dispatch (`emit_n`, `grep`, ...); /// false if it's request/reply (`sleep`, `compute_sum`). pub is_stream: bool, + /// What this job is doing (worker identity Stage 1). Rendered by + /// `*workers*` and by the statusline activity indicator. + pub purpose: String, } /// One row in the `*workers*` buffer's "completed" section: a job @@ -507,6 +551,8 @@ pub struct CompletedJobInfo { pub settled_age_ms: u64, /// Supersede key (if any) the job was dispatched under. pub supersede_key: Option, + /// What this job was doing (worker identity Stage 1). + pub purpose: String, /// Terminal outcome. `None` is unreachable here --- only /// settled jobs land in the completed ring. pub outcome: JobOutcome, @@ -543,9 +589,33 @@ struct CompletedSlot { dispatched_at: Instant, settled_at: Instant, supersede_key: Option, + purpose: String, outcome: JobOutcome, } +/// What the statusline activity indicator needs, and nothing more +/// (framing Q#W-3). +/// +/// A dedicated read surface rather than [`WorkersSnapshot`]: the +/// indicator is evaluated once per visible window per frame, and a +/// snapshot clones the whole completed ring (up to +/// [`COMPLETED_RING_CAP`] entries) that the indicator never looks at. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct ActivitySummary { + /// How many jobs are in flight. Always ≥ 1 — an idle runtime + /// returns `None` rather than a zero count, because a segment that + /// is always present costs modeline width forever to say "nothing + /// is happening". + pub in_flight: usize, + /// The **oldest** in-flight job's purpose. + /// + /// Oldest, not newest and not "busiest": jobs carry no cost + /// estimate, so "busiest" is not a defined quantity, while oldest + /// is computable from `dispatched_at` and answers the question a + /// user actually asks of a stuck editor. + pub oldest_purpose: String, +} + /// One frame's worth of streamed items for a single stream id, /// returned by [`AsyncRuntime::take_stream_batches`]. T M3.5. #[derive(Clone, Debug)] @@ -610,6 +680,29 @@ pub struct AsyncRuntime { /// only contended at parse settle/take time --- never inside the /// editor's hot path. T M4.1. parse_handoff: Arc>>>, + /// Registered handler names of the `pmacs.workers.dispatch` calls + /// currently on the stack (worker identity Stage 1, Q#W-2). + /// + /// `pmacs.workers.dispatch(name, …)` looks `name` up, calls the + /// handler, and returns whatever it returns — **`name` is not a + /// parameter of any layer below that call**, and a handler that + /// reaches straight for `pmacs._async._dispatch_*` bypasses the Lua + /// wrapper layer entirely. So the name has to travel out of band, and + /// it is read here, at the one allocation funnel every job passes + /// through. + /// + /// A stack, not a slot: nesting is real (a handler may dispatch + /// through another registered handler) and innermost wins. + /// + /// **The extent is non-yieldable, and `async.lua` enforces it** — + /// both supported yield APIs refuse inside it, because parking a + /// coroutine with a name still pushed hands that name to whatever + /// allocates next. The one hole is a raw `coroutine.yield`, which + /// violates R46 and which no refusal sited in a yield helper can + /// intercept (the scheduler only sees the yielded value after the + /// coroutine has already suspended). That residual is recorded in + /// `docs/worker-identity-framing.md` §2, not claimed closed. + dispatch_names: RefCell>, } /// Default cap on stream items delivered in a single drain. 1024 @@ -650,6 +743,7 @@ impl AsyncRuntime { frame_target_ms: Cell::new(DEFAULT_FRAME_TARGET_MS), completed: RefCell::new(VecDeque::with_capacity(COMPLETED_RING_CAP)), parse_handoff: Arc::new(Mutex::new(HashMap::new())), + dispatch_names: RefCell::new(Vec::new()), } } @@ -733,34 +827,83 @@ impl AsyncRuntime { self.default_max_batch.set(n.clamp(1, 1_000_000)); } + /// Push a `pmacs.workers.dispatch` handler name for the dynamic + /// extent of that handler's call (worker identity Stage 1, Q#W-2). + /// + /// Paired with [`Self::pop_dispatch_name`] by + /// `pmacs.workers.dispatch`, which brackets the handler call under + /// `pcall` so a raising handler still pops. An unpaired push is the + /// failure mode that matters: it would poison every later dispatch + /// in the session with a stale name, and the feature would start + /// lying silently rather than loudly. + pub fn push_dispatch_name(&self, name: impl Into) { + self.dispatch_names.borrow_mut().push(name.into()); + } + + /// Pop the innermost dispatch-handler name. No-op when the stack is + /// already empty — an unbalanced pop is a Lua-side bug, and + /// panicking here would turn it into a torn editor rather than a + /// missing label. + pub fn pop_dispatch_name(&self) { + self.dispatch_names.borrow_mut().pop(); + } + + /// Whether a `pmacs.workers.dispatch` handler is on the stack. + /// + /// Read from Lua as `pmacs._async._in_dispatch_name_scope()`. Both + /// supported yield APIs refuse while it is set (Q#W-2 rule 1), for + /// the same reason `Handle:await` refuses inside + /// `pmacs.window.commit_to`: yielding would park the coroutine with + /// the name still pushed, and the next allocation — in any + /// coroutine, on any later tick — would inherit it. + #[must_use] + pub fn in_dispatch_name_scope(&self) -> bool { + !self.dispatch_names.borrow().is_empty() + } + + /// The innermost dispatch-handler name, if any. Nesting is a stack + /// and innermost wins (Q#W-2 rule 3). + #[must_use] + pub fn current_dispatch_name(&self) -> Option { + self.dispatch_names.borrow().last().cloned() + } + /// Register a fresh pending entry and return its id + cancel /// token. The token is what the worker closure polls; the entry /// is what `tick` updates on reply. /// - /// If `supersede_key` is `Some(key)`, any in-flight predecessor + /// **This is the single allocation funnel**: every job in the + /// system — the ten `dispatch_*` methods and + /// [`Self::register_external`] alike — is born here, which is what + /// makes the identity field reachable by construction rather than by + /// audit. + /// + /// If `spec.supersede` is `Some(key)`, any in-flight predecessor /// under the same key has its cancel token flipped *before* this /// allocation returns, and the `key → id` table is updated to /// point at the new id. The predecessor's pending entry is /// retained --- its worker will produce a `Cancelled` reply that /// `tick` then surfaces. - fn allocate( - &self, - kind: JobKind, - supersede_key: Option<&str>, - stream: Option, - ) -> (JobId, CancellationToken) { - self.allocate_with_resource(kind, supersede_key, stream, None) - } - - /// [`Self::allocate`], plus the filesystem mutation this job - /// performs. Only the two mutating fs dispatchers pass `resource`. - fn allocate_with_resource( - &self, - kind: JobKind, - supersede_key: Option<&str>, - stream: Option, - resource: Option, - ) -> (JobId, CancellationToken) { + /// + /// The recorded purpose **composes** with any dispatch-name ambient + /// rather than replacing it (Q#W-2 rule 6): `": "` + /// where the dispatcher described its own work, `""` where it + /// did not. Letting the dispatcher's purpose win would lose the + /// third-party caller all over again; letting the name win would + /// discard the only description of the actual work. + fn allocate(&self, spec: JobSpec<'_>) -> (JobId, CancellationToken) { + let JobSpec { + kind, + supersede: supersede_key, + stream, + resource, + purpose, + } = spec; + let purpose = match self.current_dispatch_name() { + Some(name) if purpose.is_empty() => name, + Some(name) => format!("{name}: {purpose}"), + None => purpose, + }; let id = self.next_job_id.fetch_add(1, Ordering::Relaxed); let cancel = CancellationToken::new(); if let Some(key) = supersede_key { @@ -788,6 +931,7 @@ impl AsyncRuntime { kind, dispatched_at: Instant::now(), resource, + purpose, }, ); (id, cancel) @@ -801,7 +945,13 @@ impl AsyncRuntime { /// dispatched under `key` is cancelled before this dispatch /// returns. T M3.4 / [spec §6.3]. pub fn dispatch_sleep(&self, ms: i64, supersede: Option<&str>) -> JobId { - let (id, cancel) = self.allocate(JobKind::Sleep, supersede, None); + let (id, cancel) = self.allocate(JobSpec { + kind: JobKind::Sleep, + supersede, + stream: None, + resource: None, + purpose: format!("sleep {}ms", ms.max(0)), + }); let bus = self.workers.clone(); let total = Duration::from_millis(ms.max(0).unsigned_abs()); self.pool.dispatch(move |_pool| { @@ -816,7 +966,13 @@ impl AsyncRuntime { /// the granular cancel boundary. `supersede` follows the same /// rule as [`Self::dispatch_sleep`]. pub fn dispatch_compute_sum(&self, n: u64, supersede: Option<&str>) -> JobId { - let (id, cancel) = self.allocate(JobKind::ComputeSum, supersede, None); + let (id, cancel) = self.allocate(JobSpec { + kind: JobKind::ComputeSum, + supersede, + stream: None, + resource: None, + purpose: format!("sum 1..{n}"), + }); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { let kind = run_compute_sum(&cancel, n); @@ -842,7 +998,13 @@ impl AsyncRuntime { max_batch: Option, ) -> JobId { let cap = max_batch.map_or_else(|| self.default_max_batch.get(), |n| n.clamp(1, 1_000_000)); - let (id, cancel) = self.allocate(JobKind::EmitN, supersede, Some(cap)); + let (id, cancel) = self.allocate(JobSpec { + kind: JobKind::EmitN, + supersede, + stream: Some(cap), + resource: None, + purpose: format!("emit {count} items"), + }); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { run_emit_n(&cancel, &bus, id, count); @@ -870,7 +1032,13 @@ impl AsyncRuntime { max_batch: Option, ) -> JobId { let cap = max_batch.map_or_else(|| self.default_max_batch.get(), |n| n.clamp(1, 1_000_000)); - let (id, cancel) = self.allocate(JobKind::Grep, supersede, Some(cap)); + let (id, cancel) = self.allocate(JobSpec { + kind: JobKind::Grep, + supersede, + stream: Some(cap), + resource: None, + purpose: format!("grep {:?} in {}", spec.pattern, spec.root.display()), + }); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { run_grep(&cancel, &bus, id, spec); @@ -897,7 +1065,13 @@ impl AsyncRuntime { /// in-flight predecessor under the same key has its cancel token /// flipped synchronously. T M4.1 / [spec §6.3]. pub fn dispatch_parse(&self, spec: ParseRequest, supersede: Option<&str>) -> JobId { - let (id, cancel) = self.allocate(JobKind::Parse, supersede, None); + let (id, cancel) = self.allocate(JobSpec { + kind: JobKind::Parse, + supersede, + stream: None, + resource: None, + purpose: format!("parse {}", spec.language_name), + }); let bus = self.workers.clone(); let handoff = self.parse_handoff.clone(); self.pool.dispatch(move |_pool| { @@ -922,7 +1096,13 @@ impl AsyncRuntime { tolerance: ReadDirTolerance, supersede: Option<&str>, ) -> JobId { - let (id, cancel) = self.allocate(JobKind::FsReadDir, supersede, None); + let (id, cancel) = self.allocate(JobSpec { + kind: JobKind::FsReadDir, + supersede, + stream: None, + resource: None, + purpose: format!("read_dir {}", path.display()), + }); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { let kind = run_fs_read_dir(&cancel, &path, tolerance); @@ -934,7 +1114,13 @@ impl AsyncRuntime { /// Dispatch a `stat(path)` job. Returns one [`FsDirEntry`] of /// metadata for `path`. T M8.1. pub fn dispatch_fs_stat(&self, path: PathBuf, supersede: Option<&str>) -> JobId { - let (id, cancel) = self.allocate(JobKind::FsStat, supersede, None); + let (id, cancel) = self.allocate(JobSpec { + kind: JobKind::FsStat, + supersede, + stream: None, + resource: None, + purpose: format!("stat {}", path.display()), + }); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { let kind = run_fs_stat(&cancel, &path); @@ -948,15 +1134,16 @@ impl AsyncRuntime { pub fn dispatch_fs_rename(&self, from: PathBuf, to: PathBuf, supersede: Option<&str>) -> JobId { // The closure below MOVES both paths; the pending entry is the // only thing that still knows them when the reply lands. - let (id, cancel) = self.allocate_with_resource( - JobKind::FsRename, + let (id, cancel) = self.allocate(JobSpec { + kind: JobKind::FsRename, supersede, - None, - Some(ResourceOp::Rename { + stream: None, + resource: Some(ResourceOp::Rename { from: from.clone(), to: to.clone(), }), - ); + purpose: format!("rename {} -> {}", from.display(), to.display()), + }); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { let kind = run_fs_rename(&cancel, &from, &to); @@ -967,7 +1154,13 @@ impl AsyncRuntime { /// Dispatch a `chmod(path, mode)` job. T M8.1. pub fn dispatch_fs_chmod(&self, path: PathBuf, mode: u32, supersede: Option<&str>) -> JobId { - let (id, cancel) = self.allocate(JobKind::FsChmod, supersede, None); + let (id, cancel) = self.allocate(JobSpec { + kind: JobKind::FsChmod, + supersede, + stream: None, + resource: None, + purpose: format!("chmod {mode:o} {}", path.display()), + }); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { let kind = run_fs_chmod(&cancel, &path, mode); @@ -978,12 +1171,13 @@ impl AsyncRuntime { /// Dispatch a `remove(path)` job. T M8.1. pub fn dispatch_fs_remove(&self, path: PathBuf, supersede: Option<&str>) -> JobId { - let (id, cancel) = self.allocate_with_resource( - JobKind::FsRemove, + let (id, cancel) = self.allocate(JobSpec { + kind: JobKind::FsRemove, supersede, - None, - Some(ResourceOp::Remove { path: path.clone() }), - ); + stream: None, + resource: Some(ResourceOp::Remove { path: path.clone() }), + purpose: format!("remove {}", path.display()), + }); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { let kind = run_fs_remove(&cancel, &path); @@ -1008,12 +1202,27 @@ impl AsyncRuntime { /// same supervisor (DAP, etc.) reuse this surface. /// /// `supersede` follows the same rule as the worker dispatchers. + /// + /// `purpose` is **required and has no derivable fallback** here, + /// which is why it is a parameter rather than something this method + /// composes for itself. The ten pool dispatchers each know what + /// their own job does; `register_external` knows only a `JobKind` + /// that is `McpRequest` or `LspRequest` — a category, not a + /// description. The caller is the only party that can say + /// `"lsp textDocument/definition"`. pub fn register_external( &self, kind: JobKind, supersede: Option<&str>, + purpose: impl Into, ) -> (JobId, CancellationToken) { - self.allocate(kind, supersede, None) + self.allocate(JobSpec { + kind, + supersede, + stream: None, + resource: None, + purpose: purpose.into(), + }) } /// Settle an externally-registered job with a JSON value. Wakes @@ -1206,6 +1415,7 @@ impl AsyncRuntime { dispatched_at: job.dispatched_at, settled_at: now, supersede_key: job.supersede_key.clone(), + purpose: job.purpose.clone(), outcome, }); } @@ -1243,6 +1453,7 @@ impl AsyncRuntime { supersede_key: j.supersede_key.clone(), cancel_requested: j.cancel.is_cancelled(), is_stream: j.stream_buffer.is_some(), + purpose: j.purpose.clone(), }) .collect(); // Stable order: oldest first. The buffer renderer renders in @@ -1262,12 +1473,51 @@ impl AsyncRuntime { .as_millis() as u64, settled_age_ms: now.saturating_duration_since(c.settled_at).as_millis() as u64, supersede_key: c.supersede_key.clone(), + purpose: c.purpose.clone(), outcome: c.outcome.clone(), }) .collect(); WorkersSnapshot { active, completed } } + /// What the statusline activity indicator shows, or `None` when + /// nothing is in flight (worker identity Stage 1, Q#W-3). + /// + /// `None` at zero is the contract, not an optimization: the + /// indicator renders **no segment at all** when idle, because a + /// statusline element that is always present costs modeline width + /// forever to say "nothing is happening". + /// + /// Scans the pending table rather than reusing + /// [`Self::workers_snapshot`]: this runs once per visible window per + /// frame, and a snapshot would clone the whole completed ring that + /// the indicator never reads. + #[must_use] + pub fn activity_summary(&self) -> Option { + let pending = self.pending.borrow(); + let mut in_flight = 0usize; + let mut oldest: Option<(&Instant, &str)> = None; + for job in pending.values() { + if !matches!(job.state, PendingState::Running) { + continue; + } + in_flight += 1; + // Strictly-earlier wins, so the first job seen holds the + // slot against later ties. `HashMap` iteration order is + // arbitrary, so two jobs dispatched in the same `Instant` + // resolve arbitrarily — a tie between simultaneous jobs has + // no right answer to lose. + if oldest.is_none_or(|(seen, _)| job.dispatched_at < *seen) { + oldest = Some((&job.dispatched_at, job.purpose.as_str())); + } + } + let (_, purpose) = oldest?; + Some(ActivitySummary { + in_flight, + oldest_purpose: purpose.to_owned(), + }) + } + /// Drain the per-stream accumulators into one batch each. Each /// returned batch is bounded by the stream's `max_batch`; items /// beyond the cap stay in the accumulator until the next call. @@ -1876,23 +2126,25 @@ mod tests { fn tick_reports_resources_in_bus_arrival_order_not_allocation_order() { fn run(reverse: bool) -> Vec { let rt = AsyncRuntime::with_pool_size(1); - let (a, _) = rt.allocate_with_resource( - JobKind::FsRename, - None, - None, - Some(ResourceOp::Rename { + let (a, _) = rt.allocate(JobSpec { + kind: JobKind::FsRename, + supersede: None, + stream: None, + resource: Some(ResourceOp::Rename { from: PathBuf::from("/tmp/a-from"), to: PathBuf::from("/tmp/a-to"), }), - ); - let (b, _) = rt.allocate_with_resource( - JobKind::FsRemove, - None, - None, - Some(ResourceOp::Remove { + purpose: "rename a".to_owned(), + }); + let (b, _) = rt.allocate(JobSpec { + kind: JobKind::FsRemove, + supersede: None, + stream: None, + resource: Some(ResourceOp::Remove { path: PathBuf::from("/tmp/b-gone"), }), - ); + purpose: "remove b".to_owned(), + }); let order = if reverse { [b, a] } else { [a, b] }; for id in order { rt.workers @@ -1936,23 +2188,25 @@ mod tests { #[test] fn a_failed_or_cancelled_resource_job_is_not_harvested() { let rt = AsyncRuntime::with_pool_size(1); - let (failed, _) = rt.allocate_with_resource( - JobKind::FsRename, - None, - None, - Some(ResourceOp::Rename { + let (failed, _) = rt.allocate(JobSpec { + kind: JobKind::FsRename, + supersede: None, + stream: None, + resource: Some(ResourceOp::Rename { from: PathBuf::from("/tmp/nope"), to: PathBuf::from("/tmp/also-nope"), }), - ); - let (cancelled, _) = rt.allocate_with_resource( - JobKind::FsRemove, - None, - None, - Some(ResourceOp::Remove { + purpose: "rename nope".to_owned(), + }); + let (cancelled, _) = rt.allocate(JobSpec { + kind: JobKind::FsRemove, + supersede: None, + stream: None, + resource: Some(ResourceOp::Remove { path: PathBuf::from("/tmp/never"), }), - ); + purpose: "remove never".to_owned(), + }); rt.workers .send( ASYNC_REPLY_TOPIC, diff --git a/src/lsp.rs b/src/lsp.rs index f5630b6..43632f9 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -167,7 +167,11 @@ impl LspServerSpec { } fn to_process_spec(&self) -> ProcessSpec { - let mut p = ProcessSpec::new(format!("lsp:{}", self.label), &self.command); + let mut p = ProcessSpec::new( + format!("lsp:{}", self.label), + &self.command, + format!("language server for {}", self.label), + ); p.args.clone_from(&self.args); p.cwd.clone_from(&self.cwd); p.env.clone_from(&self.env); @@ -1587,9 +1591,15 @@ impl LspManager { uri: &str, ) -> JobId { let supersede = format!("lsp:{method}:{}:{uri}", sid.raw()); - let (job_id, token) = self - .runtime - .register_external(JobKind::LspRequest, Some(&supersede)); + // Worker identity Stage 1: `register_external` bypasses the + // worker pool, so its `JobKind` is the undifferentiated + // `LspRequest` for every method. The method and the document are + // the only thing that makes one row distinguishable from another + // in `*workers*`. + let purpose = format!("lsp {method} {uri}"); + let (job_id, token) = + self.runtime + .register_external(JobKind::LspRequest, Some(&supersede), purpose); self.pending_external.insert( (sid, req_id), PendingExternal { @@ -4538,7 +4548,8 @@ mod resource_reconciliation_tests { let runtime = mgr.runtime.clone(); let mut register = |rid: u64, uri: &str| { - let (job_id, token) = runtime.register_external(JobKind::LspRequest, None); + let (job_id, token) = + runtime.register_external(JobKind::LspRequest, None, format!("lsp hover {uri}")); mgr.pending_routes.insert( (a, rid), ResponseRoute::Hover { diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index b2de320..9535f81 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -7566,6 +7566,73 @@ pub fn install_async( })?, )?; + // Worker identity Stage 1 (Q#W-2): the dispatch-name ambient. + // + // `pmacs.workers.dispatch(name, …)` is the one place a third-party + // job's own name exists, and nothing below it takes a name — the + // Rust dispatchers accept job arguments, a supersede key and stream + // data, and a handler reaching straight for `_dispatch_*` bypasses + // the Lua wrapper layer entirely. So the name travels out of band + // and is read at `allocate`, the single funnel every job passes + // through. + // + // Runtime-internal, underscore-prefixed: package code calls + // `pmacs.workers.dispatch`, which brackets these itself under + // `pcall`. A package pushing by hand and failing to pop would poison + // every later dispatch in the session with a stale name. + { + let rt = runtime.clone(); + async_mod.set( + "_push_dispatch_name", + lua.create_function(move |_, name: String| { + rt.push_dispatch_name(name); + Ok(()) + })?, + )?; + } + + { + let rt = runtime.clone(); + async_mod.set( + "_pop_dispatch_name", + lua.create_function(move |_, ()| { + rt.pop_dispatch_name(); + Ok(()) + })?, + )?; + } + + // The refusal predicate, the sibling of `_in_commit_scope` above and + // enforced for the same reason: a coroutine that parks inside the + // extent leaves the name pushed, and every job allocated in the + // meantime — in any coroutine, on any later tick — inherits it. + { + let rt = runtime.clone(); + async_mod.set( + "_in_dispatch_name_scope", + lua.create_function(move |_, ()| Ok(rt.in_dispatch_name_scope()))?, + )?; + } + + // The statusline activity indicator's read surface (Q#W-3). Returns + // `nil` when nothing is in flight — the indicator renders no segment + // at all when idle, so "absent" has to be representable. + { + let rt = runtime.clone(); + async_mod.set( + "_activity_summary", + lua.create_function(move |lua, ()| { + let Some(summary) = rt.activity_summary() else { + return Ok(mlua::Value::Nil); + }; + let t = lua.create_table_with_capacity(0, 2)?; + t.set("in_flight", summary.in_flight)?; + t.set("purpose", summary.oldest_purpose)?; + Ok(mlua::Value::Table(t)) + })?, + )?; + } + { let rt = runtime.clone(); async_mod.set( @@ -7728,7 +7795,7 @@ fn workers_snapshot_to_lua(lua: &Lua, runtime: &SharedAsyncRuntime) -> mlua::Res let out = lua.create_table()?; let active = lua.create_table_with_capacity(snap.active.len(), 0)?; for (i, job) in snap.active.iter().enumerate() { - let row = lua.create_table_with_capacity(0, 6)?; + let row = lua.create_table_with_capacity(0, 7)?; row.set("id", job.id)?; row.set("kind", job.kind.label())?; row.set("age_ms", job.age_ms)?; @@ -7737,12 +7804,13 @@ fn workers_snapshot_to_lua(lua: &Lua, runtime: &SharedAsyncRuntime) -> mlua::Res } row.set("cancel_requested", job.cancel_requested)?; row.set("is_stream", job.is_stream)?; + row.set("purpose", job.purpose.as_str())?; active.set(i + 1, row)?; } out.set("active", active)?; let completed = lua.create_table_with_capacity(snap.completed.len(), 0)?; for (i, job) in snap.completed.iter().enumerate() { - let row = lua.create_table_with_capacity(0, 7)?; + let row = lua.create_table_with_capacity(0, 8)?; row.set("id", job.id)?; row.set("kind", job.kind.label())?; row.set("duration_ms", job.duration_ms)?; @@ -7750,6 +7818,7 @@ fn workers_snapshot_to_lua(lua: &Lua, runtime: &SharedAsyncRuntime) -> mlua::Res if let Some(key) = &job.supersede_key { row.set("supersede", key.as_str())?; } + row.set("purpose", job.purpose.as_str())?; let (status, value): (&'static str, mlua::Value) = match &job.outcome { JobOutcome::Complete(JobResult::Unit) => ("ok", mlua::Value::Nil), JobOutcome::Complete(JobResult::Sum(v)) => ( @@ -8680,6 +8749,21 @@ fn parse_restart(name: &str) -> mlua::Result { fn lua_to_spec(table: &Table) -> mlua::Result { let label: String = table.get("label").unwrap_or_else(|_| "unnamed".to_owned()); let command: String = table.get("command")?; + // Worker identity Stage 1: required on the Rust struct, optional at + // this surface, falling back to the label. + // + // Requiring it here would break every existing `pmacs.process.spawn` + // caller, and the compiler obligation this lane is buying is on the + // *Rust* construction sites — the ones a future field would silently + // skip. A Lua caller that supplies nothing gets its own label back, + // which is what the caller already chose to call this work; it is + // less informative than a real description but it is not a + // fabrication, which is the bar `owner` failed (framing §3). + let purpose: String = table + .get::>("purpose") + .ok() + .flatten() + .unwrap_or_else(|| label.clone()); let args: Vec = table.get("args").unwrap_or_default(); let cwd: Option = table.get("cwd").ok().flatten(); let env_table: Option = table.get("env").ok().flatten(); @@ -8762,6 +8846,7 @@ fn lua_to_spec(table: &Table) -> mlua::Result { }; Ok(ProcessSpec { label, + purpose, command, args, cwd: cwd.map(std::path::PathBuf::from), @@ -8985,11 +9070,18 @@ pub fn install_process(lua: &Lua, supervisor: &SharedProcessSupervisor) -> mlua: .collect(); let out = lua.create_table_with_capacity(ids.len(), 0)?; for (i, id) in ids.iter().enumerate() { - let row = lua.create_table_with_capacity(0, 3)?; + let row = lua.create_table_with_capacity(0, 4)?; row.set("id", ProcessIdLua(*id))?; if let Some(spec) = sup.spec(*id) { row.set("label", spec.label.as_str())?; row.set("command", spec.command.as_str())?; + // Worker identity Stage 1: a new KEY on each + // existing row. The row COUNT is deliberately + // untouched — three acceptance suites assert on + // `#pmacs.process.list()` as a leak detector + // (framing Q#W-4), and widening what this + // enumerates would inflate all three baselines. + row.set("purpose", spec.purpose.as_str())?; } if let Some(state) = sup.state(*id) { row.set("state", state_to_lua(lua, state)?)?; diff --git a/src/mcp.rs b/src/mcp.rs index b1db5f4..f1085ca 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -175,7 +175,11 @@ impl McpServerSpec { } fn to_process_spec(&self) -> ProcessSpec { - let mut p = ProcessSpec::new(format!("mcp:{}", self.label), &self.command); + let mut p = ProcessSpec::new( + format!("mcp:{}", self.label), + &self.command, + format!("MCP server {}", self.label), + ); p.args.clone_from(&self.args); p.cwd.clone_from(&self.cwd); p.env.clone_from(&self.env); @@ -873,7 +877,9 @@ impl McpManager { } let req_id = next_request_id(client); let body = make_request(req_id, &method, params); - let (job_id, token) = self.runtime.register_external(JobKind::McpRequest, None); + let (job_id, token) = + self.runtime + .register_external(JobKind::McpRequest, None, format!("mcp {method}")); client.pending_external.insert( req_id, PendingExternal { @@ -948,7 +954,11 @@ impl McpManager { // (1) Cache hit. if let Some(ResourceCacheState::Cached { result }) = self.resource_cache.get(&key).cloned() { - let (job_id, _token) = self.runtime.register_external(JobKind::McpRequest, None); + let (job_id, _token) = self.runtime.register_external( + JobKind::McpRequest, + None, + format!("mcp resources/read {uri} (cached)"), + ); self.runtime.complete_external_ok(job_id, result); return Ok(job_id); } @@ -959,7 +969,11 @@ impl McpManager { // independently. if let Some(ResourceCacheState::InFlight { request_id }) = self.resource_cache.get(&key) { let in_flight_rid = *request_id; - let (job_id, token) = self.runtime.register_external(JobKind::McpRequest, None); + let (job_id, token) = self.runtime.register_external( + JobKind::McpRequest, + None, + format!("mcp resources/read {uri}"), + ); if let Some(p) = client.pending_external.get_mut(&in_flight_rid) { p.awaiters.push(Awaiter { job_id, token }); return Ok(job_id); @@ -974,7 +988,11 @@ impl McpManager { // (3) Cache miss: dispatch. let req_id = next_request_id(client); let body = make_request(req_id, "resources/read", json!({ "uri": uri })); - let (job_id, token) = self.runtime.register_external(JobKind::McpRequest, None); + let (job_id, token) = self.runtime.register_external( + JobKind::McpRequest, + None, + format!("mcp resources/read {uri}"), + ); client.pending_external.insert( req_id, PendingExternal { @@ -1063,10 +1081,13 @@ impl McpManager { // than referenced by `json!`); avoids a needless-pass-by- // value clippy complaint and matches `send_request`'s shape. let mut params_map = Map::new(); + let purpose = format!("mcp tools/call {name}"); params_map.insert("name".into(), Value::String(name)); params_map.insert("arguments".into(), arguments); let body = make_request(req_id, "tools/call", Value::Object(params_map)); - let (job_id, token) = self.runtime.register_external(JobKind::McpRequest, None); + let (job_id, token) = self + .runtime + .register_external(JobKind::McpRequest, None, purpose); client.pending_external.insert( req_id, PendingExternal { @@ -1125,10 +1146,13 @@ impl McpManager { } let req_id = next_request_id(client); let mut params_map = Map::new(); + let purpose = format!("mcp prompts/get {name}"); params_map.insert("name".into(), Value::String(name)); params_map.insert("arguments".into(), arguments); let body = make_request(req_id, "prompts/get", Value::Object(params_map)); - let (job_id, token) = self.runtime.register_external(JobKind::McpRequest, None); + let (job_id, token) = self + .runtime + .register_external(JobKind::McpRequest, None, purpose); client.pending_external.insert( req_id, PendingExternal { diff --git a/src/process.rs b/src/process.rs index c4a9277..17a5365 100644 --- a/src/process.rs +++ b/src/process.rs @@ -196,6 +196,23 @@ pub struct ProcessSpec { /// so multiple processes can run the same binary with /// distinguishable labels. pub label: String, + /// What this process is doing, in words a user can read (worker + /// identity Stage 1, `COHERENCE.md` §9). + /// + /// **Required, and not the same thing as [`Self::label`].** The + /// label is an *identity* — `lsp:rust-analyzer`, a terminal's buffer + /// name — spelled however the caller likes, so that two processes + /// running the same binary can be told apart. The purpose is a + /// *description*: it answers "what is happening", which is the + /// question §3's promise of visible asynchronous work is about and + /// which a label chosen for uniqueness routinely does not answer. + /// + /// **Not an owner**, in any spelling. It records what the process is + /// doing, not which package asked for it; `pmacs.process.spawn` is + /// callable by any package, so a value derived here would + /// misattribute third-party work to a builtin at exactly the point + /// §9 wants attribution (framing §3). + pub purpose: String, /// Program to execute. Looked up via the system PATH unless an /// absolute path is supplied. pub command: String, @@ -237,10 +254,21 @@ pub struct ProcessSpec { impl ProcessSpec { /// Construct a spec with the bare-minimum fields. Convenience /// for tests and one-off scripts. + /// + /// `purpose` is a parameter rather than something derived from the + /// label because it is a required field with no honest default + /// (worker identity Stage 1): deriving it from the label would make + /// every process claim its identity *is* its description, which is + /// exactly the conflation the field exists to undo. #[must_use] - pub fn new(label: impl Into, command: impl Into) -> Self { + pub fn new( + label: impl Into, + command: impl Into, + purpose: impl Into, + ) -> Self { Self { label: label.into(), + purpose: purpose.into(), command: command.into(), args: Vec::new(), cwd: None, @@ -2722,6 +2750,7 @@ mod tests { let spec = ProcessSpec::new( "unpublished-terminal", "/definitely/not/a/real/pmacs-terminal-program", + "test process", ); assert!(supervisor.spawn_terminal(spec).is_err()); supervisor.tick(); @@ -2732,7 +2761,7 @@ mod tests { #[test] fn spawn_pipes_lifecycle_started_then_exited() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("echo-test", "/bin/sh"); + let mut spec = ProcessSpec::new("echo-test", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "echo hello && exit 0".into()]; let id = sup.spawn(spec).expect("spawn"); let events = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); @@ -2892,7 +2921,7 @@ mod tests { /// 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"); + let mut spec = ProcessSpec::new(name, "/bin/sleep", "test process"); spec.args = vec!["30".into()]; spec.mode = ProcessMode::Pty { rows: 24, @@ -2943,7 +2972,7 @@ mod tests { sup: &mut ProcessSupervisor, name: &str, ) -> (ProcessId, u32, i32) { - let mut spec = ProcessSpec::new(name, BASH); + let mut spec = ProcessSpec::new(name, BASH, "test process"); spec.args = vec![ "--noprofile".into(), "--norc".into(), @@ -3194,7 +3223,7 @@ mod tests { #[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"); + let mut spec = ProcessSpec::new("diag-pipe-leader", "/bin/sleep", "test process"); spec.args = vec!["30".into()]; let id = sup.spawn(spec).expect("spawn"); let pid = spawn_started_pid(&mut sup, id); @@ -3227,7 +3256,7 @@ mod tests { 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"); + let mut spec = ProcessSpec::new("diag-signal-name", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "sleep 30".into()]; spec.group = true; let id = sup.spawn(spec).expect("spawn"); @@ -3280,7 +3309,7 @@ mod tests { let mut sup = ProcessSupervisor::new(); let temp = tempfile::TempDir::new().expect("tempdir"); let ready = temp.path().join("usr1-trapped"); - let mut spec = ProcessSpec::new("diag-disposition-live", "/bin/sh"); + let mut spec = ProcessSpec::new("diag-disposition-live", "/bin/sh", "test process"); // Ignore USR1 so the successful non-fatal signal cannot end the // child and confuse the state assertion with a real exit — and // then WAIT for the child to say it has done so. `Started` is @@ -3344,7 +3373,7 @@ mod tests { let mut sup = ProcessSupervisor::new(); let temp = tempfile::TempDir::new().expect("tempdir"); let ready = temp.path().join("usr1-trapped"); - let mut spec = ProcessSpec::new("diag-trap-readiness", "/bin/sh"); + let mut spec = ProcessSpec::new("diag-trap-readiness", "/bin/sh", "test process"); spec.args = vec!["-c".into(), trapped_usr1_command(&ready, "sleep 1; ")]; spec.group = true; let id = sup.spawn(spec).expect("spawn"); @@ -3435,7 +3464,7 @@ mod tests { #[test] fn a_leader_directed_kill_failure_reports_the_fallback_branch() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("diag-leader", "/bin/sleep"); + let mut spec = ProcessSpec::new("diag-leader", "/bin/sleep", "test process"); spec.args = vec!["30".into()]; let id = sup.spawn(spec).expect("spawn"); let pid = spawn_started_pid(&mut sup, id); @@ -3484,7 +3513,7 @@ mod tests { #[test] fn a_failure_after_the_child_exits_reports_the_leader_as_exited() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("diag-exited", "/bin/sh"); + let mut spec = ProcessSpec::new("diag-exited", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "exit 3".into()]; let id = sup.spawn(spec).expect("spawn"); // NOT `spawn_started_pid`: draining ticks, and this child exits @@ -3512,7 +3541,7 @@ mod tests { #[test] fn an_injected_failure_changes_no_state_and_arms_no_ledger() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("diag-disposition", "/bin/sh"); + let mut spec = ProcessSpec::new("diag-disposition", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "sleep 30".into()]; spec.group = true; let id = sup.spawn(spec).expect("spawn"); @@ -3557,7 +3586,7 @@ mod tests { #[test] fn observing_the_leader_does_not_consume_the_exit_event() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("diag-one-event", "/bin/sh"); + let mut spec = ProcessSpec::new("diag-one-event", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "exit 7".into()]; spec.mode = ProcessMode::Pty { rows: 24, @@ -3599,7 +3628,7 @@ mod tests { let mut sup = ProcessSupervisor::new(); // `sleep 30` is long enough that the test definitely needs // to terminate it deliberately. - let mut spec = ProcessSpec::new("sleeper", "/bin/sh"); + let mut spec = ProcessSpec::new("sleeper", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "sleep 30".into()]; let id = sup.spawn(spec).expect("spawn"); // Wait for Started so we have a pid. @@ -3628,7 +3657,7 @@ mod tests { // implementation blocked the caller in `write_all` here — // which in the editor was the main thread, wedging the frame // loop whenever an LSP server fell behind on its stdin. - let mut spec = ProcessSpec::new("stdin-ignorer", "/bin/sh"); + let mut spec = ProcessSpec::new("stdin-ignorer", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "sleep 30".into()]; let id = sup.spawn(spec).expect("spawn"); let _ = drain_until(&mut sup, id, Duration::from_secs(2), |evs| { @@ -3654,7 +3683,7 @@ mod tests { // payload back followed by a clean exit proves the writer // thread drains its queue before dropping the pipe (the // flush-then-EOF contract `close_stdin` documents). - let mut spec = ProcessSpec::new("cat-echo", "/bin/sh"); + let mut spec = ProcessSpec::new("cat-echo", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "cat".into()]; let id = sup.spawn(spec).expect("spawn"); let _ = drain_until(&mut sup, id, Duration::from_secs(2), |evs| { @@ -3700,7 +3729,7 @@ mod tests { fn restart_on_crash_respawns_after_nonzero_exit() { let mut sup = ProcessSupervisor::new(); sup.set_restart_backoff(Duration::from_millis(10)); - let mut spec = ProcessSpec::new("crasher", "/bin/sh"); + let mut spec = ProcessSpec::new("crasher", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "exit 7".into()]; spec.restart = RestartPolicy::OnCrash; let id = sup.spawn(spec).expect("spawn"); @@ -3731,7 +3760,7 @@ mod tests { #[test] fn restart_never_does_not_respawn_after_clean_exit() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("oneshot", "/bin/sh"); + let mut spec = ProcessSpec::new("oneshot", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "exit 0".into()]; let id = sup.spawn(spec).expect("spawn"); let _ = drain_until(&mut sup, id, Duration::from_secs(2), has_exited); @@ -3760,7 +3789,7 @@ mod tests { let pid = { let mut sup = ProcessSupervisor::new(); sup.set_grace_period(Duration::from_millis(200)); - let mut spec = ProcessSpec::new("victim", "/bin/sh"); + let mut spec = ProcessSpec::new("victim", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "sleep 30".into()]; let id = sup.spawn(spec).expect("spawn"); // Drain until Started so we know the pid. @@ -3798,7 +3827,7 @@ mod tests { #[test] fn pty_mode_child_sees_a_tty() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("ttytest", "/bin/sh"); + let mut spec = ProcessSpec::new("ttytest", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "tty".into()]; spec.mode = ProcessMode::default_pty(); let id = sup.spawn(spec).expect("spawn"); @@ -3835,7 +3864,7 @@ mod tests { #[test] fn m6_1_pty_resize_delivers_sigwinch_to_child() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("winch-watch", "/bin/sh"); + let mut spec = ProcessSpec::new("winch-watch", "/bin/sh", "test process"); // Trap WINCH, print READY for synchronization, then loop on // a short sleep so SIGWINCH can interrupt and fire the trap. spec.args = vec![ @@ -3880,7 +3909,7 @@ mod tests { #[test] fn m6_1_pty_mode_lifecycle_started_then_exited() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("pty-exit", "/bin/sh"); + let mut spec = ProcessSpec::new("pty-exit", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "echo done && exit 0".into()]; spec.mode = ProcessMode::default_pty(); let id = sup.spawn(spec).expect("spawn"); @@ -3915,7 +3944,7 @@ mod tests { #[test] fn m6_1_pty_raw_mode_disables_kernel_echo() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("raw-stty", "/bin/sh"); + let mut spec = ProcessSpec::new("raw-stty", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "stty -a".into()]; spec.mode = ProcessMode::default_pty(); // Raw by default. let id = sup.spawn(spec).expect("spawn"); @@ -3937,7 +3966,7 @@ mod tests { #[test] fn m6_1_pty_canonical_mode_keeps_kernel_echo() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("canon-stty", "/bin/sh"); + let mut spec = ProcessSpec::new("canon-stty", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "stty -a".into()]; spec.mode = ProcessMode::Pty { rows: 24, @@ -3991,7 +4020,7 @@ mod tests { // buffers. const TOTAL: usize = 10 * 1024 * 1024; let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("byte-flood", "/bin/sh"); + let mut spec = ProcessSpec::new("byte-flood", "/bin/sh", "test process"); spec.args = vec!["-c".into(), format!("head -c {TOTAL} /dev/zero")]; let id = sup.spawn(spec).expect("spawn"); @@ -4067,7 +4096,7 @@ mod tests { #[test] fn m6_2_pty_streaming_coalesces_per_tick() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("chunky-stream", "/bin/sh"); + let mut spec = ProcessSpec::new("chunky-stream", "/bin/sh", "test process"); // 1 MiB of zeros from /dev/zero. The reader thread reads in // [`BYTE_CHUNK_SIZE`] (8 KiB) chunks --- ~128 reads --- all // queued onto the bounded channel within microseconds of @@ -4116,7 +4145,7 @@ mod tests { #[test] fn m6_2_ansi_enabled_pty_emits_structured_events() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("ansi-stream", "/bin/sh"); + let mut spec = ProcessSpec::new("ansi-stream", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "printf '\\033[31mhi\\033[0m\\n'".into()]; spec.mode = ProcessMode::Pty { rows: 24, @@ -4198,7 +4227,7 @@ mod tests { let handle = std::thread::spawn(move || { let mut sup = ProcessSupervisor::new(); sup.set_grace_period(Duration::from_millis(300)); - let mut spec = ProcessSpec::new("forever-flood", "/bin/sh"); + let mut spec = ProcessSpec::new("forever-flood", "/bin/sh", "test process"); // Continuous writer; SIGTERM kills it (no signal handler). spec.args = vec!["-c".into(), "while :; do printf 'X'; done".into()]; let id = sup.spawn(spec).expect("spawn"); @@ -4324,7 +4353,7 @@ mod tests { let handle = std::thread::spawn(move || { let mut sup = ProcessSupervisor::new(); sup.set_grace_period(Duration::from_millis(300)); - let mut spec = ProcessSpec::new("orphan-holds-pipe", "setsid"); + let mut spec = ProcessSpec::new("orphan-holds-pipe", "setsid", "test process"); // `setsid --fork` forks and the parent exits, so the // *recorded* pid terminates promptly (letting `poll_one` // reach the teardown path) while `cat` survives holding the @@ -4412,7 +4441,7 @@ mod tests { // ----------------------------------------------------------------- fn sh_group_spec(label: &str, script: &str) -> ProcessSpec { - let mut spec = ProcessSpec::new(label, "/bin/sh"); + let mut spec = ProcessSpec::new(label, "/bin/sh", "test process"); spec.args = vec!["-c".into(), script.to_owned()]; spec.stdin = StdinMode::Null; spec.group = true; @@ -4546,7 +4575,7 @@ mod tests { ); // Control: a non-group child inherits the test process's // group instead of leading its own. - let mut plain = ProcessSpec::new("plain", "/bin/sh"); + let mut plain = ProcessSpec::new("plain", "/bin/sh", "test process"); plain.args = vec!["-c".into(), "sleep 30".into()]; let plain_id = sup.spawn(plain).expect("spawn plain"); let plain_events = drain_until(&mut sup, plain_id, Duration::from_secs(2), |evs| { @@ -5017,7 +5046,7 @@ mod tests { fn maybe_restart_inert_once_shut_down() { let mut sup = ProcessSupervisor::new(); sup.set_restart_backoff(Duration::from_millis(30)); - let mut spec = ProcessSpec::new("restarter", "/bin/sh"); + let mut spec = ProcessSpec::new("restarter", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "echo x".into()]; spec.restart = RestartPolicy::Always; let id = sup.spawn(spec).expect("spawn"); @@ -5158,7 +5187,7 @@ mod tests { #[test] fn group_and_null_stdin_rejected_under_pty() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("pty-null", "/bin/sh"); + let mut spec = ProcessSpec::new("pty-null", "/bin/sh", "test process"); spec.mode = ProcessMode::default_pty(); spec.stdin = StdinMode::Null; let err = sup @@ -5169,7 +5198,7 @@ mod tests { "error points at pipe mode: {err}" ); - let mut spec = ProcessSpec::new("pty-group", "/bin/sh"); + let mut spec = ProcessSpec::new("pty-group", "/bin/sh", "test process"); spec.mode = ProcessMode::default_pty(); spec.group = true; let err = sup diff --git a/src/terminal/session.rs b/src/terminal/session.rs index c731fb0..47fae56 100644 --- a/src/terminal/session.rs +++ b/src/terminal/session.rs @@ -305,7 +305,8 @@ impl TerminalManager { buffer.set_read_only(true); core.registry.borrow_mut().insert(buffer); - let mut process_spec = ProcessSpec::new(buffer_name, spec.command); + let purpose = format!("terminal running {}", spec.command); + let mut process_spec = ProcessSpec::new(buffer_name, spec.command, purpose); process_spec.args = spec.args; process_spec.cwd = spec.cwd; process_spec.env = spec.env; diff --git a/src/workers_buffer.rs b/src/workers_buffer.rs index 6a6eeb4..02d6d03 100644 --- a/src/workers_buffer.rs +++ b/src/workers_buffer.rs @@ -14,19 +14,34 @@ //! ```text //! Workers (active: 2, completed: 5) //! -//! ID Kind Age Supersede Status -//! ------ ----------- -------- ---------- ---------- -//! #5 grep 412ms search running -//! #6 sleep 18ms running (cancel pending) +//! ID Kind Age Supersede Purpose Status +//! ------ ----------- -------- ---------- ------------------------ ---------- +//! #5 grep 412ms search search: grep "fn" in /x running +//! #6 sleep 18ms sleep 18ms running (cancel pending) //! //! Recent (newest first) //! -//! ID Kind Duration Supersede Outcome -//! ------ ----------- -------- ---------- ---------- -//! #4 grep 1242ms search cancelled (3s ago) -//! #3 compute_sum 2ms ok (3s ago) +//! ID Kind Duration Supersede Purpose Outcome +//! ------ ----------- -------- ---------- ------------------------ ---------- +//! #4 grep 1242ms search search: grep "fn" in /x cancelled (3s ago) +//! #3 compute_sum 2ms sum 1..100 ok (3s ago) //! ``` //! +//! # Purpose (worker identity Stage 1, `COHERENCE.md` §9) +//! +//! The `Purpose` column is what turns "twelve rows named `lsp_request`" +//! into a readable account of what the editor is doing. `Kind` names the +//! builtin dispatcher a job funnelled through, which for every +//! third-party job is a builtin's label rather than the caller's; the +//! purpose carries the work's own description and, under +//! `pmacs.workers.dispatch`, the registered handler name it ran under. +//! +//! It is placed **before** `Status` and padded, because `Status` is +//! variable-width (`running (cancel pending) [stream]`) and two +//! ragged trailing columns render as noise. An over-long purpose pushes +//! `Status` right rather than being truncated: losing the end of a path +//! is a worse failure than an uneven column. +//! //! Lua reads the snapshot via `pmacs.workers.snapshot()`; the //! `pmacs.workers.show()` builtin invokes [`render`] on it and //! returns the buffer id. Auto-refresh hooks into @@ -43,6 +58,11 @@ use crate::buffer_registry::BufferRegistry; /// Canonical name for the workers observability buffer. pub const WORKERS_BUFFER_NAME: &str = "*workers*"; +/// Minimum column width the `Purpose` column is padded to. Purposes +/// longer than this push the trailing column right rather than being +/// truncated (see the module docs). +const PURPOSE_WIDTH: usize = 24; + /// Render `snapshot` into the `*workers*` buffer (creating it if /// absent), replacing its full contents. Returns the buffer id /// and the Edits produced by the replacement (zero, one, or two — @@ -119,13 +139,17 @@ fn format_snapshot(snapshot: &WorkersSnapshot) -> String { let _ = writeln!(text); let _ = writeln!( text, - "{:<7} {:<11} {:>9} {:<11} Status", - "ID", "Kind", "Age", "Supersede" + "{:<7} {:<11} {:>9} {:<11} {:9} {:<11} ----------", - "------", "-----------", "---------", "-----------" + "{:<7} {:<11} {:>9} {:<11} {: String { let _ = writeln!(text); let _ = writeln!( text, - "{:<7} {:<11} {:>9} {:<11} Outcome", - "ID", "Kind", "Duration", "Supersede" + "{:<7} {:<11} {:>9} {:<11} {:9} {:<11} ----------", - "------", "-----------", "---------", "-----------" + "{:<7} {:<11} {:>9} {:<11} {:9} {key:<11} {status}"); + let purpose = &job.purpose; + let _ = writeln!( + text, + "{id:<7} {kind:<11} {age:>9} {key:<11} {purpose:9} {key:<11} {outcome} ({age} ago)" + "{id:<7} {kind:<11} {duration:>9} {key:<11} {purpose: bool { #[test] fn m4_4_lifecycle_spawn_and_exit() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("hello", "/bin/sh"); + let mut spec = ProcessSpec::new("hello", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "printf hi && exit 0".into()]; let id = sup.spawn(spec).expect("spawn"); let evs = drain_until(&mut sup, id, Duration::from_secs(5), has_exit_event); @@ -983,7 +983,7 @@ fn m4_4_lifecycle_spawn_and_exit() { #[test] fn m4_4_lifecycle_signal_terminates() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("victim", "/bin/sh"); + let mut spec = ProcessSpec::new("victim", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "sleep 30".into()]; let id = sup.spawn(spec).expect("spawn"); let _ = drain_until(&mut sup, id, Duration::from_secs(2), |evs| { @@ -1020,7 +1020,11 @@ fn m4_4_lifecycle_signal_terminates() { fn m4_4_lifecycle_crash_surfaces_as_event() { let mut sup = ProcessSupervisor::new(); // Path that will reliably not resolve. - let spec = ProcessSpec::new("ghost", "/this/binary/does/not/exist/pmacs-m4-4"); + let spec = ProcessSpec::new( + "ghost", + "/this/binary/does/not/exist/pmacs-m4-4", + "test process", + ); let _ = sup.spawn(spec); // spawn returns Err but the event is still emitted sup.tick(); let evs = sup.take_all_events(); @@ -1037,7 +1041,7 @@ fn m4_4_lifecycle_crash_surfaces_as_event() { fn m4_4_restart_policy_on_crash_respawns() { let mut sup = ProcessSupervisor::new(); sup.set_restart_backoff(Duration::from_millis(10)); - let mut spec = ProcessSpec::new("flap", "/bin/sh"); + let mut spec = ProcessSpec::new("flap", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "exit 9".into()]; spec.restart = RestartPolicy::OnCrash; let id = sup.spawn(spec).expect("spawn"); @@ -1070,7 +1074,7 @@ fn m4_4_restart_policy_on_crash_respawns() { #[test] fn m4_4_restart_policy_never_does_not_respawn() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("oneshot", "/bin/sh"); + let mut spec = ProcessSpec::new("oneshot", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "exit 0".into()]; let id = sup.spawn(spec).expect("spawn"); let _ = drain_until(&mut sup, id, Duration::from_secs(2), has_exit_event); @@ -1099,7 +1103,7 @@ fn m4_4_no_zombies_after_editor_drop() { let pid: u32 = { let mut sup = ProcessSupervisor::new(); sup.set_grace_period(Duration::from_millis(200)); - let mut spec = ProcessSpec::new("zombie-test", "/bin/sh"); + let mut spec = ProcessSpec::new("zombie-test", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "sleep 60".into()]; let id = sup.spawn(spec).expect("spawn"); let _ = drain_until(&mut sup, id, Duration::from_secs(2), |evs| { @@ -1136,7 +1140,7 @@ fn m4_4_no_zombies_after_editor_drop() { #[test] fn m4_4_pty_mode_child_observes_a_tty() { let mut sup = ProcessSupervisor::new(); - let mut spec = ProcessSpec::new("ttytest", "/bin/sh"); + let mut spec = ProcessSpec::new("ttytest", "/bin/sh", "test process"); spec.args = vec!["-c".into(), "tty".into()]; spec.mode = ProcessMode::default_pty(); let id = sup.spawn(spec).expect("spawn"); diff --git a/tests/statusline_segments_acceptance.rs b/tests/statusline_segments_acceptance.rs index a4dc5f1..c577af4 100644 --- a/tests/statusline_segments_acceptance.rs +++ b/tests/statusline_segments_acceptance.rs @@ -136,7 +136,12 @@ fn a01_04_registry_contract_limits_epochs_and_results() { .iter() .map(|provider| provider.name.as_str()) .collect::>(), - ["mode", "terminal", "lsp"], + // `activity` is worker identity Stage 1's fourth adopter, and it + // sorts first because `async.lua` is loaded before `syntax.lua`, + // `terminal.lua` and `lsp.lua`. This is an INVENTORY assertion: + // it grows when a builtin provider is added, which is exactly + // what it is for. + ["activity", "mode", "terminal", "lsp"], "built-in providers are discoverable in registration order" ); let before_epochs = { diff --git a/tests/vterm_stage1_acceptance.rs b/tests/vterm_stage1_acceptance.rs index 489ca9a..0c7a312 100644 --- a/tests/vterm_stage1_acceptance.rs +++ b/tests/vterm_stage1_acceptance.rs @@ -398,7 +398,7 @@ fn editor_shutdown_kills_term_ignoring_terminal_child() { #[test] fn terminal_tick_does_not_take_non_terminal_process_events() { let mut state = EditorState::new_with_roots(&crate::iso::roots()); - let mut process = pmacs::process::ProcessSpec::new("ordinary", "/bin/sh"); + let mut process = pmacs::process::ProcessSpec::new("ordinary", "/bin/sh", "test process"); process.args = vec!["-c".into(), "printf ordinary".into()]; let ordinary_id = state .process_supervisor diff --git a/tests/worker_identity_acceptance.rs b/tests/worker_identity_acceptance.rs new file mode 100644 index 0000000..d7bdbfb --- /dev/null +++ b/tests/worker_identity_acceptance.rs @@ -0,0 +1,849 @@ +// tests/worker_identity_acceptance.rs --- worker identity Stage 1. + +//! Worker identity Stage 1 (`docs/worker-identity-framing.md` §6, +//! `COHERENCE.md` §9). +//! +//! §9 grades the worker model **mechanism without identity**: a job +//! carries a `JobKind` naming the builtin dispatcher it funnelled +//! through, so every third-party job renders under a builtin's label, +//! and no progress indicator exists anywhere. This suite pins what +//! Stage 1 does about that — a required `purpose` on the job and the +//! process, the dispatch-name ambient that stops +//! `pmacs.workers.dispatch` discarding its handler name, and the first +//! indicator a user sees without running a command. +//! +//! # What is NOT here, deliberately +//! +//! **Presence is enforced by the COMPILER, not by anything below.** +//! `JobSpec::purpose` is non-optional and `JobSpec` has no `Default`, so +//! a dispatcher that supplies none does not build. A funnel test would +//! prove only that the funnel stores what it was handed, and would say +//! nothing about whether fourteen callers handed it anything meaningful. +//! Everything below is about *semantics*. +//! +//! **That a raw `coroutine.yield` inside either dynamic scope is +//! prevented — it is not.** R46 forbids package code from yielding +//! raw, but it is a convention, and the scheduler diagnoses a non-Handle +//! yield only *after* the coroutine has suspended (`async.lua` resumes, +//! then inspects what came back), so no refusal sited in a yield helper +//! is ever consulted. Rule 1 claims **the two supported yield APIs** and +//! nothing more. A test that "proved" coverage this design does not have +//! would be worse than the recorded gap, so the gap is recorded instead +//! (framing §2, §6, §7). +//! +//! **That background work is attributable from one place** (Stage 2's +//! unified view), **that a terminal PTY is visible anywhere** (Q#W-4), +//! or **that any job is attributed to the PACKAGE responsible for it**. +//! `purpose` records what work is being done and, under +//! `pmacs.workers.dispatch`, which registered handler it ran under. +//! Neither is package ownership, which waits for P3 — and there is no +//! `owner` field, in any spelling, for it to squat on (framing §3, §7). + +use std::collections::HashMap; +use std::time::{Duration, Instant}; + +use pmacs::async_runtime::JobKind; +use pmacs::cell::{Cell, CellGrid, CellSize, Glyph}; +use pmacs::editor::EditorState; +use pmacs::protocol::FrontendId; +use pmacs::statusline::{ + StatuslineEvaluationOutcome, StatuslineEvaluationTarget, StatuslineProviderId, + evaluate_statusline, +}; + +#[path = "common/iso.rs"] +mod iso; + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +fn exec(state: &EditorState, source: &str) { + state.lua_host.lua().load(source.to_owned()).exec().unwrap(); +} + +fn eval(state: &EditorState, source: &str) -> T { + state.lua_host.lua().load(source.to_owned()).eval().unwrap() +} + +fn editor() -> EditorState { + let state = EditorState::new_with_roots(&iso::roots()); + exec(&state, "pmacs.lsp.config = {}"); + state +} + +/// Drive the async runtime until nothing is in flight and no coroutine +/// is parked. How many frames that takes is not knowable in advance, so +/// this never counts them. +/// +/// Quiescence is measured as **no `Running` job**, not as an empty +/// pending table. Most jobs here are dispatched and never awaited — +/// that is the shape the indicator exists to describe — and a settled +/// entry stays in the pending table until someone takes its result, so +/// `pending_count() == 0` would never come true. +fn pump(state: &mut EditorState) { + let deadline = Instant::now() + Duration::from_secs(10); + loop { + let idle: bool = eval( + state, + "return pmacs._async.parked_count() == 0 + and #pmacs.workers.snapshot().active == 0", + ); + if idle { + return; + } + assert!(Instant::now() < deadline, "async pump deadline exceeded"); + state.tick_async(); + } +} + +/// The purposes of every job the runtime currently has in flight. +/// +/// Read through the **Lua** snapshot surface, which is what `*workers*` +/// and any package consume, rather than through the Rust struct. +fn active_purposes(state: &EditorState) -> Vec { + eval( + state, + "local out = {} + for _, job in ipairs(pmacs.workers.snapshot().active) do + out[#out + 1] = job.purpose + end + return out", + ) +} + +fn paint(state: &EditorState, rows: u32, cols: u32) -> Vec { + let mut cells = vec![Cell::default(); (rows * cols) as usize]; + let mut grid = CellGrid { + cells: &mut cells, + stride: cols, + size: CellSize::new(rows, cols), + }; + let _ = pmacs::editor::paint_frame( + state, + FrontendId::LOCAL, + &HashMap::new(), + &mut grid, + CellSize::new(rows, cols), + ); + cells +} + +fn row_text(cells: &[Cell], cols: u32, row: u32) -> String { + (0..cols) + .map( + |column| match &cells[(row * cols + column) as usize].glyph { + Glyph::Char(ch) => *ch, + Glyph::Cluster(bytes) => std::str::from_utf8(bytes) + .ok() + .and_then(|text| text.chars().next()) + .unwrap_or(' '), + Glyph::Continuation => ' ', + }, + ) + .collect() +} + +/// The registration handle of the builtin activity provider. +fn activity_provider(state: &EditorState) -> StatuslineProviderId { + state + .statusline_registry + .borrow() + .providers() + .into_iter() + .find(|provider| provider.name == "activity") + .expect("builtin activity provider") + .id +} + +/// The activity provider's segment for `LOCAL`'s only window, or `None` +/// when it produced **no segment at all**. +/// +/// `Option`, never `String`, is the whole point of this helper: +/// "absent" and "empty" must be distinguishable, because a zero-width +/// segment still consumes a separator in the composed modeline. +fn activity_segment(state: &EditorState) -> Option { + let id = activity_provider(state); + let evaluation = evaluate_statusline( + state.lua_host.lua(), + &state.core, + &state.statusline_registry, + StatuslineEvaluationTarget::Grid { + frontend_id: FrontendId::LOCAL, + }, + ); + let StatuslineEvaluationOutcome::Ready(windows) = evaluation.outcome else { + panic!("statusline evaluation must be ready in a single-window editor"); + }; + windows + .iter() + .flat_map(|window| window.left.iter().chain(window.right.iter())) + .find(|segment| segment.provider_id == id) + .map(|segment| segment.text.clone()) +} + +/// Dispatch one job that will still be **in flight** when the caller +/// looks, without sleeping. +/// +/// A pending entry leaves `Running` only inside `AsyncRuntime::tick`, so +/// a dispatch with no intervening tick is in flight by construction — +/// no wall-clock race, and no worker left sleeping past the test. +fn dispatch_one_in_flight(state: &EditorState) { + exec(state, "IN_FLIGHT = pmacs.workers.sleep(50)"); +} + +// --------------------------------------------------------------------------- +// 1 — purpose reaches the three structurally distinct entry paths +// --------------------------------------------------------------------------- + +/// One per distinct **shape**, not one per dispatcher: a pool +/// dispatcher, an `register_external` job, and a spawned process. +/// +/// `register_external` is here because MCP and LSP bypass the worker +/// pool entirely — they are the likeliest paths for a later field to be +/// added to `PendingJob` and quietly missed — and because its `JobKind` +/// is the undifferentiated `LspRequest`/`McpRequest` for every method, +/// so `purpose` is the only thing that tells two of its rows apart. +#[test] +fn every_entry_shape_records_what_its_work_is() { + let mut state = editor(); + + // (a) A pool dispatcher. + dispatch_one_in_flight(&state); + let purposes = active_purposes(&state); + assert_eq!(purposes.len(), 1, "one job in flight: {purposes:?}"); + assert_eq!( + purposes[0], "sleep 50ms", + "a pool job records the work, not just its handler's name" + ); + + // (b) An externally-settled job. The purpose is a PARAMETER here + // because `register_external` has nothing to derive one from: its + // kind is a category, not a description. + let (job_id, _token) = state.async_runtime.register_external( + JobKind::LspRequest, + None, + "lsp textDocument/definition file:///tmp/x.rs", + ); + let purposes = active_purposes(&state); + assert!( + purposes + .iter() + .any(|p| p == "lsp textDocument/definition file:///tmp/x.rs"), + "an externally-registered job carries its caller's description: {purposes:?}" + ); + state.async_runtime.complete_external_cancelled(job_id); + + // (c) A spawned process. `label` keeps its existing meaning and its + // existing callers; `purpose` is the new, separate answer to "what + // is this doing". + exec( + &state, + r#"P = pmacs.process.spawn { + label = "sh-1", + purpose = "probing the repository for a build system", + command = "/bin/sh", + args = { "-c", "sleep 5" }, + }"#, + ); + let rows: Vec = eval( + &state, + "local out = {} + for _, row in ipairs(pmacs.process.list()) do + out[#out + 1] = row.label .. ' | ' .. row.purpose + end + return out", + ); + assert!( + rows.iter() + .any(|row| row == "sh-1 | probing the repository for a build system"), + "a spawned process carries a purpose ALONGSIDE its label: {rows:?}" + ); + exec(&state, "pmacs.process.terminate(P)"); + + pump(&mut state); +} + +/// A `pmacs.process.spawn` caller that supplies no purpose keeps +/// working, and gets its own label back rather than an empty field. +/// +/// The Rust struct's field is required — the compiler enforces that at +/// every construction site. This surface is deliberately lenient, +/// because requiring it here would break every existing caller for no +/// coverage the compiler is not already providing. +#[test] +fn a_process_spawned_without_a_purpose_falls_back_to_its_label() { + let mut state = editor(); + exec( + &state, + r#"P = pmacs.process.spawn { + label = "legacy-caller", + command = "/bin/sh", + args = { "-c", "sleep 5" }, + }"#, + ); + let purpose: String = eval( + &state, + "for _, row in ipairs(pmacs.process.list()) do + if row.label == 'legacy-caller' then return row.purpose end + end + return ''", + ); + assert_eq!(purpose, "legacy-caller"); + exec(&state, "pmacs.process.terminate(P)"); + pump(&mut state); +} + +/// Q#W-4's preservation half, pinned here as well as by the three +/// leak-detector suites: `purpose` is a new KEY on each existing row and +/// changes nothing about **which** processes `list()` enumerates. +/// +/// `m6_8_multi_repl_acceptance`, `compile_mode_acceptance` and +/// `lean4_stage1_acceptance` all assert on `#pmacs.process.list()` as a +/// leak baseline. If any of them needs editing, the design is wrong. +#[test] +fn process_list_still_hides_terminal_ptys() { + let mut state = editor(); + let before: usize = eval(&state, "return #pmacs.process.list()"); + exec( + &state, + "T = pmacs.terminal.open { command = '/bin/sh', args = { '-c', 'sleep 5' } }", + ); + let after: usize = eval(&state, "return #pmacs.process.list()"); + assert_eq!( + before, after, + "a terminal PTY must stay invisible to pmacs.process.list (Q#W-4)" + ); + assert!( + eval::(&state, "return pmacs.terminal.is_terminal(T)"), + "precondition: the PTY really was opened" + ); + // No explicit close: terminals have no Lua teardown surface, and + // `EditorState::drop` shuts the supervisor down with SIGTERM then + // SIGKILL, so the child cannot outlive the test. + pump(&mut state); +} + +// --------------------------------------------------------------------------- +// 2 — the dispatch-name ambient (Q#W-2) +// --------------------------------------------------------------------------- + +/// **Rule 7 + the defect itself.** A job dispatched through +/// `pmacs.workers.dispatch("name", …)` reports `"name"`. +/// +/// The witness is a handler **registered from Lua that calls a real +/// dispatcher**, not a synthetic push of the ambient. A test that +/// pushed the name by hand would prove the stack works and leave the +/// actual defect — `name` dying inside an arbitrary handler, three +/// layers above anything that takes a name — completely unwitnessed. +#[test] +fn a_dispatched_job_reports_the_registered_handler_name() { + let mut state = editor(); + exec( + &state, + "pmacs.workers.register('indexer', function() + return pmacs.workers.sleep(50) + end) + H = pmacs.workers.dispatch('indexer')", + ); + let purposes = active_purposes(&state); + assert_eq!(purposes.len(), 1, "one job in flight: {purposes:?}"); + assert!( + purposes[0].starts_with("indexer"), + "the third party's own name must survive the call chain: {purposes:?}" + ); + + // Rule 7: outside any extent, nothing changes. + exec(&state, "DIRECT = pmacs.workers.sleep(50)"); + let purposes = active_purposes(&state); + assert!( + purposes.iter().any(|p| p == "sleep 50ms"), + "a builtin invoked directly records its own purpose: {purposes:?}" + ); + pump(&mut state); +} + +/// **Rule 6 — COMPOSE, do not replace.** Both halves asserted, because +/// a test on the prefix alone passes when the description is dropped, +/// and a test on the description alone passes when the third party is +/// lost again. +#[test] +fn a_dispatched_job_composes_the_handler_name_with_the_work() { + let mut state = editor(); + exec( + &state, + "pmacs.workers.register('indexer', function() + return pmacs.workers.sleep(50) + end) + H = pmacs.workers.dispatch('indexer')", + ); + let purposes = active_purposes(&state); + assert_eq!( + purposes, + vec!["indexer: sleep 50ms".to_owned()], + "letting the name win discards the work; letting the work win \ + loses the third party" + ); + pump(&mut state); +} + +/// **Rules 3 and 4 — nesting is a stack (innermost wins) and fan-out +/// shares the name.** +#[test] +fn nesting_takes_the_innermost_name_and_fan_out_shares_it() { + let mut state = editor(); + exec( + &state, + "pmacs.workers.register('inner', function() + -- Fan-out: two jobs under one handler. + A = pmacs.workers.sleep(50) + B = pmacs.workers.sleep(51) + return A + end) + pmacs.workers.register('outer', function() + pmacs.workers.dispatch('inner') + -- Back in `outer`'s extent: the stack restored on return. + C = pmacs.workers.sleep(52) + return C + end) + pmacs.workers.dispatch('outer')", + ); + let mut purposes = active_purposes(&state); + purposes.sort(); + assert_eq!( + purposes, + vec![ + "inner: sleep 50ms".to_owned(), + "inner: sleep 51ms".to_owned(), + "outer: sleep 52ms".to_owned(), + ], + "innermost wins inside, and the outer name is restored after" + ); + pump(&mut state); +} + +/// **Rule 5 — unwind-safe, and this is the one that makes a naive +/// version worse than none.** +/// +/// A handler that raises must still pop. Otherwise one failure poisons +/// every subsequent dispatch in the session with a stale name, and the +/// feature stops failing loudly and starts lying silently — a +/// regression that would surface as intermittent misattribution long +/// after the lane landed. +#[test] +fn a_raising_handler_still_pops_its_name() { + let mut state = editor(); + exec( + &state, + "pmacs.workers.register('boom', function() error('handler failed') end) + OK, ERR = pcall(pmacs.workers.dispatch, 'boom')", + ); + assert!( + !eval::(&state, "return OK"), + "the handler's error must still reach the caller" + ); + assert!( + eval::(&state, "return tostring(ERR)").contains("handler failed"), + "and must reach it unchanged" + ); + + exec(&state, "LATER = pmacs.workers.sleep(50)"); + let purposes = active_purposes(&state); + assert_eq!( + purposes, + vec!["sleep 50ms".to_owned()], + "an unrelated later dispatch must not inherit the failed \ + handler's name: {purposes:?}" + ); + pump(&mut state); +} + +/// **Preservation.** `pmacs.workers.dispatch` was `return +/// handler(args, opts)` — a tail call that propagates **every** return +/// value. Bracketing it must not quietly truncate that. +/// +/// A `local ok, result = pcall(...)` bracketing would pass every other +/// test in this file and lose a two-value handler's second value with no +/// error anywhere, which is the shape of regression that surfaces months +/// later in somebody else's package. +#[test] +fn dispatch_still_propagates_every_value_the_handler_returns() { + let mut state = editor(); + let values: Vec = eval( + &state, + "pmacs.workers.register('multi', function() + return pmacs.workers.sleep(50), 'second', 'third' + end) + local a, b, c = pmacs.workers.dispatch('multi') + return { type(a), tostring(b), tostring(c) }", + ); + assert_eq!( + values, + vec!["table".to_owned(), "second".to_owned(), "third".to_owned()], + "a multi-value handler must survive the bracketing" + ); + pump(&mut state); +} + +/// **Rule 2 — work dispatched LATER is not covered, deliberately.** +/// +/// A job dispatched from an `on_complete` callback runs ticks later, +/// outside the extent, and carries only its own purpose. Asserted so +/// that the boundary reads as designed rather than as broken; covering +/// it would need the asynchronous lifetime mechanism Stage 3 owns +/// (Q#W-5). +#[test] +fn work_dispatched_from_a_completion_callback_carries_no_handler_name() { + let mut state = editor(); + exec( + &state, + "LATE = nil + pmacs.workers.register('deferred', function() + local h = pmacs.workers.sleep(1) + h:on_complete(function() + LATE = pmacs.workers.sleep(50) + end) + return h + end) + pmacs.workers.dispatch('deferred')", + ); + // One tick settles the first job and fires the callback; the job the + // callback dispatches is what this test is about, so do not pump to + // quiescence before reading it. + let deadline = Instant::now() + Duration::from_secs(10); + while !eval::(&state, "return LATE ~= nil") { + assert!(Instant::now() < deadline, "callback never fired"); + state.tick_async(); + } + let purposes = active_purposes(&state); + assert_eq!( + purposes, + vec!["sleep 50ms".to_owned()], + "the extent is the handler CALL, not the job's lifetime: {purposes:?}" + ); + pump(&mut state); +} + +// --------------------------------------------------------------------------- +// 3 — rule 1: the extent is non-yieldable, and that is ENFORCED +// --------------------------------------------------------------------------- + +/// **Rule 1, first supported yield API.** Two assertions, and the +/// second is the load-bearing one. +/// +/// A guard that raises but leaves the name pushed has converted a silent +/// misattribution into a silent misattribution *plus* an error. So the +/// witness dispatches again after the rejection and asserts the new job +/// carries no stale name. +#[test] +fn awaiting_inside_a_handler_is_refused_and_the_scope_restores() { + let mut state = editor(); + // The awaited handle is created OUTSIDE the extent on purpose: the + // second assertion below is about what a job allocated *after* the + // refusal carries, and a job the handler allocated for itself would + // legitimately wear the handler's name and blur that. + exec( + &state, + "OUTSIDE = pmacs.workers.sleep(1) + REFUSAL = nil + pmacs.workers.register('awaits', function() + local ok, err = pcall(function() return OUTSIDE:await() end) + REFUSAL = (not ok) and tostring(err) or '' + return OUTSIDE + end) + pmacs.async(function() pmacs.workers.dispatch('awaits') end)", + ); + let refusal: String = eval(&state, "return REFUSAL"); + assert!( + refusal.contains("cannot await inside") && refusal.contains("pmacs.workers.dispatch"), + "the refusal must name the rule it enforces; got {refusal:?}" + ); + assert!( + !eval::(&state, "return pmacs._async._in_dispatch_name_scope()"), + "a refused await must still leave the scope popped" + ); + + exec(&state, "AFTER = pmacs.workers.sleep(50)"); + let purposes = active_purposes(&state); + assert!( + purposes.iter().any(|p| p == "sleep 50ms"), + "and a later dispatch must carry no stale name: {purposes:?}" + ); + assert!( + !purposes.iter().any(|p| p.starts_with("awaits:")), + "no job allocated after the refusal may inherit the handler's \ + name: {purposes:?}" + ); + pump(&mut state); +} + +/// **Rule 1, unconditionally.** The refusal fires even when the awaited +/// handle has already settled. +/// +/// This is the case that separates an unconditional guard from one whose +/// behaviour depends on a race: a guard placed after the `_is_complete` +/// check would fire only when a yield would really occur, passing under +/// test and failing intermittently in production depending on whether +/// the job happened to finish first. +#[test] +fn the_await_refusal_fires_even_for_an_already_complete_handle() { + let mut state = editor(); + exec(&state, "SETTLED = pmacs.workers.sleep(0)"); + let deadline = Instant::now() + Duration::from_secs(10); + while !eval::(&state, "return SETTLED:is_complete()") { + assert!(Instant::now() < deadline, "the canary never settled"); + state.tick_async(); + } + + exec( + &state, + "REFUSAL = nil + pmacs.workers.register('awaits-settled', function() + local ok, err = pcall(function() return SETTLED:await() end) + REFUSAL = (not ok) and tostring(err) or '' + return pmacs.workers.sleep(50) + end) + pmacs.workers.dispatch('awaits-settled')", + ); + let refusal: String = eval(&state, "return REFUSAL"); + assert!( + refusal.contains("cannot await inside") && refusal.contains("pmacs.workers.dispatch"), + "a settled handle must be refused too, or the guard's behaviour \ + depends on a race; got {refusal:?}" + ); + assert!( + !eval::(&state, "return pmacs._async._in_dispatch_name_scope()"), + "and the scope must still be popped" + ); + pump(&mut state); +} + +/// **Rule 1, second supported yield API.** Guarding `:await()` and not +/// `yield_to_next_tick` would leave the extent open through a second +/// door — and Q#W-7 below is the proof that exactly that happens when +/// only one door is guarded. +#[test] +fn yield_to_next_tick_inside_a_handler_is_refused_and_the_scope_restores() { + let mut state = editor(); + exec( + &state, + "REFUSAL = nil + pmacs.workers.register('yields', function() + local ok, err = pcall(pmacs.async.yield_to_next_tick) + REFUSAL = (not ok) and tostring(err) or '' + return pmacs.workers.sleep(50) + end) + pmacs.async(function() pmacs.workers.dispatch('yields') end)", + ); + let refusal: String = eval(&state, "return REFUSAL"); + assert!( + refusal.contains("cannot yield inside") && refusal.contains("pmacs.workers.dispatch"), + "the second yield API must refuse too; got {refusal:?}" + ); + assert!( + !eval::(&state, "return pmacs._async._in_dispatch_name_scope()"), + "and must leave the scope popped" + ); + + exec(&state, "AFTER = pmacs.workers.sleep(51)"); + let purposes = active_purposes(&state); + assert!( + purposes.iter().any(|p| p == "sleep 51ms"), + "a later dispatch must carry no stale name: {purposes:?}" + ); + pump(&mut state); +} + +// --------------------------------------------------------------------------- +// 4 — Q#W-7: the same hole in `commit_to`, closed here +// --------------------------------------------------------------------------- + +/// **Q#W-7 — a pre-existing defect, found by reading and repaired in +/// this lane.** +/// +/// `Handle:await()` refuses inside `pmacs.window.commit_to` precisely so +/// a coroutine cannot park with the frontend scope pushed (Journey Stage +/// 1a, Q#JR14b). But `pmacs.async.yield_to_next_tick()` also yields, is +/// public, and carried **no** such refusal — so that invariant had a +/// second entrance. +/// +/// **Reachability by a real caller is UNPROVEN.** No production caller +/// is known to yield through this door inside a commit; this pins the +/// guard rather than reproducing a user-visible bug. +/// +/// Both halves asserted, for the same reason as rule 1's: a refusal that +/// leaves the scope pushed swaps a silent misrouting for a loud one and +/// fixes neither. +#[test] +fn yield_to_next_tick_inside_commit_to_is_refused_and_the_commit_scope_restores() { + let mut state = editor(); + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join("alpha.txt"), b"alpha\n").expect("write"); + + // A GENUINE destination, produced by the production capture: the + // listener claims (returns false), so nothing commits and what lands + // in `dest` is exactly the userdata dired would have received. + // Nothing in a test can construct one. + exec( + &state, + "dest = nil + pmacs.hook.add('path.open-directory', function(_, d) dest = d return false end)", + ); + state.open_directory_target(dir.path()); + pump(&mut state); + assert!( + eval::(&state, "return dest ~= nil"), + "the chain must hand listeners a destination" + ); + + exec( + &state, + "REFUSAL = nil + pmacs.async(function() + local ok, err = pcall(pmacs.window.commit_to, dest, function() + pmacs.async.yield_to_next_tick() + end) + REFUSAL = (not ok) and tostring(err) or '' + end)", + ); + let refusal: String = eval(&state, "return REFUSAL"); + assert!( + refusal.contains("cannot yield inside") && refusal.contains("commit_to"), + "the second door into the commit scope must be shut; got {refusal:?}" + ); + assert!( + !eval::(&state, "return pmacs._async._in_commit_scope()"), + "and the commit scope must still be restored afterwards" + ); + pump(&mut state); +} + +// --------------------------------------------------------------------------- +// 5 — the statusline activity indicator (Q#W-3, Q#W-6) +// --------------------------------------------------------------------------- + +/// **Absent at zero, asserted as an absent SEGMENT rather than as an +/// empty string.** A zero-width segment still consumes a separator in +/// the composed modeline, so "returns nothing" and "returns nothing +/// visible" are different claims and only one of them is the design. +#[test] +fn the_indicator_produces_no_segment_at_all_when_nothing_is_running() { + let state = editor(); + assert_eq!( + activity_segment(&state), + None, + "an idle editor must produce NO activity segment" + ); +} + +/// **A count plus the oldest in-flight job's purpose, witnessed through +/// the real per-frame evaluation path.** +/// +/// Driven through `paint_frame`, not by calling the provider function +/// directly: a provider that works in isolation and never gets evaluated +/// is exactly the failure this must exclude. +#[test] +fn the_indicator_shows_a_count_and_the_oldest_purpose_in_a_painted_frame() { + let mut state = editor(); + exec( + &state, + "FIRST = pmacs.workers.sleep(50) + SECOND = pmacs.workers.grep({ root = '/tmp', pattern = 'zzz-no-match' })", + ); + + let cells = paint(&state, 24, 160); + let modeline = row_text(&cells, 160, 22); + assert!( + modeline.contains("⋯2 sleep 50ms"), + "the painted modeline must carry the count and the OLDEST job's \ + purpose (not the newest); got {modeline:?}" + ); + + // And the same value reaches the evaluator's segment vector, which is + // what the semantic frontend ships. + assert_eq!( + activity_segment(&state).as_deref(), + Some("⋯2 sleep 50ms"), + "the segment and the painted row must agree" + ); + + exec(&state, "SECOND:cancel()"); + pump(&mut state); +} + +/// **Q#W-6 — the setting, witnessed with work genuinely in flight.** +/// +/// The discriminating case: an assertion taken on an idle editor cannot +/// tell "disabled" from "nothing is happening", which is the only thing +/// this setting changes. +#[test] +fn the_indicator_honours_its_setting_while_work_is_in_flight() { + let mut state = editor(); + dispatch_one_in_flight(&state); + assert!( + activity_segment(&state).is_some(), + "precondition: work is in flight and the indicator is on" + ); + + exec(&state, "pmacs.config.set('ui.activity-indicator', false)"); + assert_eq!( + activity_segment(&state), + None, + "disabled means NO segment, with work still running" + ); + + exec(&state, "pmacs.config.set('ui.activity-indicator', true)"); + assert!( + activity_segment(&state).is_some(), + "and re-enabling brings it back without a restart" + ); + pump(&mut state); +} + +/// The setting is a real registry entry, not an ad-hoc global: it is +/// discoverable through `pmacs.config.describe` like every other +/// setting, which is what `COHERENCE.md` §11 grades. +#[test] +fn the_setting_is_registered_with_a_true_default() { + let state = editor(); + let (kind, default): (String, bool) = eval( + &state, + "local d = pmacs.config.describe('ui.activity-indicator') + return d.type, d.default", + ); + assert_eq!(kind, "boolean"); + assert!(default, "visible by default — no configuration, no command"); +} + +// --------------------------------------------------------------------------- +// 6 — `*workers*` renders the purpose +// --------------------------------------------------------------------------- + +/// The view §9 already has, now answering §9's question. +/// +/// `Kind` names the builtin dispatcher a job funnelled through, which +/// for a third-party job is a builtin's label rather than the caller's; +/// the `Purpose` column is what carries the caller's own account. +#[test] +fn the_workers_buffer_renders_the_purpose_column() { + let mut state = editor(); + exec( + &state, + "pmacs.workers.register('indexer', function() + return pmacs.workers.sleep(50) + end) + pmacs.workers.dispatch('indexer') + BUF = pmacs.workers.show()", + ); + let text: String = eval(&state, "return BUF:slice(0, BUF:len())"); + assert!( + text.contains("Purpose"), + "the active table must have a Purpose column:\n{text}" + ); + assert!( + text.contains("indexer: sleep 50ms"), + "and the row must render it:\n{text}" + ); + exec(&state, "pmacs.workers.hide()"); + pump(&mut state); +} From 1ca76e055bcaa606f76865ed6fd9abc248c9d8d0 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 16:57:28 +0200 Subject: [PATCH 06/20] docs: the worker-identity lane is implemented, with its gate outcome MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Section-local update to `docs/active-work.md`'s "Worker identity Stage 1 (§9)" block, which was written pre-implementation with the lane's first commit. It now records what actually shipped rather than what was planned, because the difference is where the reviewable claims are. Three things it records that a status flip alone would not: - **`journey_acceptance` passed UNTOUCHED (47/47).** Q#W-7 edits the `commit_to` guard family, so that suite was the lane's stop signal: any established pin needing an edit would have meant the change altered Journey Stage 1a's semantics rather than closing a gap in them. The same for all three `#pmacs.process.list()` leak detectors, which are Q#W-4's preservation claim. - **One pre-existing assertion did change**, and it is named here so the change is not mistaken for an accommodation: the builtin statusline provider inventory in `statusline_segments_acceptance` grows by the fourth adopter. That assertion exists to grow. - **Two residuals, stated rather than tested around.** A raw `coroutine.yield` inside either dynamic scope still leaks the scope, and Q#W-7's reachability by a real caller is unproven. Also lists the surfaces that changed shape — the collapsed allocation funnel, the two grown constructor signatures, the new required fields — for anyone rebasing a concurrent lane onto this one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 55 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 3 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index b1a2cc7..7a7d649 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -265,7 +265,7 @@ also removed: this branch's "R8 NEEDS A LANE" investigation block, and durable facts are in the retired registry row and the handoff §6 census. -## Worker identity Stage 1 (§9) — BRANCHED, pre-implementation +## Worker identity Stage 1 (§9) — IMPLEMENTED, no PR yet **Written with the lane's first commit**, per the standing correction from #171 and #215. @@ -343,8 +343,57 @@ the authoritative tip** — the ref, not a SHA. Recover with only the first two. A parent needs an ambient "currently-running job" context, and an unpopulated `parent` reads as "no parent" rather than "not tracked" (Q#W-5). -- **Gates:** `scripts/gate --acceptance `. No - `--protocol`. +- **Gates:** `scripts/gate --acceptance worker_identity_acceptance + --acceptance journey_acceptance --acceptance + statusline_segments_acceptance`. No `--protocol` — no wire change. +- **IMPLEMENTED at `1aca0ee`**, one commit on top of the four framing + revisions. `tests/worker_identity_acceptance.rs` is the new suite: 18 + tests, plus one consumer-side witness beside the private renderer in + `pmacs-gpu`. +- **`journey_acceptance` passed UNTOUCHED (47/47)** — the stop signal + did not fire. Q#W-7 edits the `commit_to` guard family, so any of its + established pins needing an edit would have meant this altered Journey + Stage 1a's semantics rather than closing a gap in them. Its diff + versus `main` is empty, and so is the diff for all three + `#pmacs.process.list()` leak-detector suites + (`m6_8_multi_repl_acceptance`, `compile_mode_acceptance`, + `lean4_stage1_acceptance`) — Q#W-4's preservation claim, checked the + way the framing asked. +- **One pre-existing assertion did change, and it is an inventory + rather than a contract**: `statusline_segments_acceptance`'s builtin + provider list becomes `["activity", "mode", "terminal", "lsp"]`. + `activity` sorts first because `async.lua` is loaded before + `syntax.lua`, `terminal.lua` and `lsp.lua`. That assertion exists to + grow when a builtin provider is added; it is listed here so the change + is not mistaken for an accommodation. +- **20 mutation checks, each test falsified by removing its own fix.** + The ones worth naming: siting the `await` guard *inside* the + `_is_complete` branch (the already-complete case then slips through — + which is the whole reason the guard is unconditional); replacing + `pcall`/pop/rethrow with a bare handler call (a raising handler leaves + the name pushed and the *next* dispatch inherits it); composing + `""` instead of `": "` and vice versa (each half + passes the other's test); `first()` instead of `last()` on the name + stack; oldest→newest in `activity_summary`; and, on the GPU side, + painting an unthemed modeline face as the band colour, which would + have made the indicator invisible without failing anything else. + One of the twenty is a **preservation** check rather than a new + claim: bracketing `pmacs.workers.dispatch` with + `local ok, result = pcall(...)` truncates a handler that returns more + than one value, which every other test in the suite tolerates. +- **Two residuals, stated rather than tested around.** Raw + `coroutine.yield` inside either dynamic scope still leaks the scope — + loudly, through `pmacs.error`, but it leaks; no refusal sited in a + yield helper can intercept it (framing §2). And Q#W-7's reachability + by a real caller stays **unproven**: the commit message says so, and + the test pins the guard rather than reproducing a fault. +- **Surfaces that changed shape, for anyone rebasing onto this:** + `AsyncRuntime::allocate`/`allocate_with_resource` collapsed into one + private `JobSpec`-taking funnel; `register_external` grew a third + parameter; `ProcessSpec::new` grew a third parameter (~40 call sites, + nearly all tests); `ActiveJobInfo`/`CompletedJobInfo`/`ProcessSpec` + each grew a required `purpose` field. `pmacs.process.spawn`'s Lua + surface keeps `purpose` optional, falling back to the label. ## Discovery Stage 2 — PR #228 OPEN, **MERGE-BLOCKED** From d01cde9432bb5c0b0f67caad04c79ef68fd6ce08 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 17:32:10 +0200 Subject: [PATCH 07/20] =?UTF-8?q?fix(process):=20pmacs.process.spawn=20REQ?= =?UTF-8?q?UIRES=20a=20purpose=20=E2=80=94=20review=20blocker?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1 on worker identity Stage 1. The lane shipped `purpose` as a required field on `ProcessSpec` but made it OPTIONAL at the `pmacs.process.spawn` Lua surface, defaulting to `label`. **That preserved compatibility and delivered nothing.** `COHERENCE.md` §9's complaint about `ProcessSpec` is precisely that `label` is "caller-supplied, unvalidated convention" — so a purpose defaulting to the label hands every existing caller back the exact convention this lane exists to replace. The approved framing said required; this makes it required where callers actually are. The two fields answer different questions and neither substitutes for the other. `label` IDENTIFIES — `lsp:rust-analyzer`, a terminal's buffer name — so two processes running the same binary can be told apart. `purpose` DESCRIBES: it answers "what is happening", which is what §3's promise of visible asynchronous work is about, and which a label chosen for uniqueness routinely does not answer. **The refusal covers five shapes, not one.** Absent; empty; whitespace-only; wrong type; and metatable-provided. The middle two matter because they satisfy the type and defeat the point exactly as copying the label across would — R42 already rejects whitespace-only `description`s in the config registry for the same reason, and a required field that accepts `""` is not required in any sense a reader benefits from. The read is RAW, matching the posture `stdin` and `group` already document in the same function: a spec table is plain data, so `__index` cannot smuggle a purpose in. Every refusal also asserts **the process list is unchanged**. A validation that rejects after spawning has already done the thing it was rejecting. **This is a BREAKING CHANGE to a public Lua API, taken deliberately and now rather than later.** Weighed and reported rather than decided silently: §10 grades extension trust "missing (one class)" and P7 package lifecycle has not started, so the third-party population calling this binding is ~zero and the cost of the change only rises from here. Checked for a reason that would be wrong and found none — `pmacs.process.spawn` has no API-reference documentation and no stability promise anywhere in `docs/`; the guide's only mentions are an audit-rule classification and a pointer to the bundled REPL, and its semver language governs *packages'* own versioning, not pmacs's Lua surface. `lua_to_spec` has exactly one caller, so the blast radius is this one binding. Eleven executable call sites updated, each with a real description rather than the label copied across — copying it would satisfy the type and defeat the point as surely as the default did: builtin/packages/repl/init.lua "interactive session" builtin/runtime/compile.lua "compiling: " builtin/runtime/lean.lua "checking the Lean toolchain version…" tests/fixtures/pmacs-magit/status.lua the full argv, not just the subcommand the label carries — "git log" and "git log --oneline -20" are one label and different work tests/compile_mode_acceptance.rs (4), tests/m4_acceptance.rs (1), tests/worker_identity_acceptance.rs (2) `lean.lua`'s site is the clearest case for the field: its comment said the label was where "a user wondering why their editor touched `lake` finds an owner" — one string doing identity AND explanation, which is the conflation being undone. The label stays a key; the purpose is now the sentence. Two references are deliberately NOT updated: `src/audit/mod.rs` and `tests/m7_9_acceptance.rs` contain `pmacs.process.spawn("ls")` as **audit fixture source text**. It is lexed by the audit engine, never executed, and editing it would change what those rule tests scan. `required_purpose` is extracted rather than inlined because inlining it pushed `lua_to_spec` past the 100-line clippy bound — the validation has its own rules and its own rationale, so it gets its own function instead of an `#[allow]`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- builtin/packages/repl/init.lua | 4 ++ builtin/runtime/compile.lua | 5 ++ builtin/runtime/lean.lua | 10 ++- src/lua_bindings/mod.rs | 72 +++++++++++++++---- tests/compile_mode_acceptance.rs | 12 ++-- tests/fixtures/pmacs-magit/status.lua | 4 ++ tests/m4_acceptance.rs | 1 + tests/worker_identity_acceptance.rs | 99 ++++++++++++++++++++------- 8 files changed, 161 insertions(+), 46 deletions(-) diff --git a/builtin/packages/repl/init.lua b/builtin/packages/repl/init.lua index 3b336f6..df8d5ca 100644 --- a/builtin/packages/repl/init.lua +++ b/builtin/packages/repl/init.lua @@ -259,6 +259,10 @@ function repl.spawn(opts) local spec = { label = name, + -- Worker identity Stage 1: the label is the REPL's session name, + -- which distinguishes two REPLs from each other and says nothing + -- about what is running. The purpose names the interpreter. + purpose = "interactive " .. h._display_name .. " session", command = argv[1], args = args, pty = { rows = rows, cols = cols, mode = "raw" }, diff --git a/builtin/runtime/compile.lua b/builtin/runtime/compile.lua index 22fc643..7645416 100644 --- a/builtin/runtime/compile.lua +++ b/builtin/runtime/compile.lua @@ -875,6 +875,11 @@ local function start_run(slot, cmdline, opts) -- stdin, own process group, TERM=dumb. local spec = { label = slot.label, + -- Worker identity Stage 1: the label distinguishes one compile slot + -- from another; the purpose is the command the user actually asked + -- for, which is what they want to see when they wonder why the + -- editor is busy. + purpose = "compiling: " .. cmdline, command = "/bin/sh", args = { "-c", "exec 2>&1; " .. cmdline }, env = { TERM = "dumb" }, diff --git a/builtin/runtime/lean.lua b/builtin/runtime/lean.lua index 09ad280..6ccb9dd 100644 --- a/builtin/runtime/lean.lua +++ b/builtin/runtime/lean.lua @@ -494,10 +494,14 @@ local function start_probe(root) -- "lake": a user pointing `command` at an absolute path to lake should -- have THAT probed, not whatever `lake` resolves to on PATH. local spec = { - -- COHERENCE §9: `ProcessSpec.label` is the only identity a process - -- carries, and it is what `pmacs.process.list` renders. A user - -- wondering why their editor touched `lake` finds an owner here. + -- COHERENCE §9: `ProcessSpec.label` identifies the process, and it + -- is what `pmacs.process.list` renders alongside the purpose. A user + -- wondering why their editor touched `lake` finds it here. label = "lean:lake-version-probe", + -- Worker identity Stage 1: the label was carrying both jobs — the + -- identity AND the explanation — which is the conflation the purpose + -- field exists to undo. The label stays a key; this is the sentence. + purpose = "checking the Lean toolchain version before starting a server", command = cfg.command, args = { "--version" }, stdin = "null", diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 9535f81..c47b678 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -8746,24 +8746,66 @@ fn parse_restart(name: &str) -> mlua::Result { }) } +/// Read the **required** `purpose` out of a `pmacs.process.spawn` spec +/// (worker identity Stage 1, `COHERENCE.md` §9). +/// +/// An earlier revision of this lane defaulted the field to `label` so +/// that existing callers kept working. That preserved compatibility and +/// delivered nothing: §9's complaint about `ProcessSpec` is precisely +/// that `label` is "caller-supplied, unvalidated convention", so a +/// purpose defaulting to the label hands every caller back the +/// convention this lane exists to replace. +/// +/// The two fields answer different questions and neither substitutes for +/// the other. `label` **identifies** — `lsp:rust-analyzer`, a terminal's +/// buffer name — so that two processes running the same binary can be +/// told apart. `purpose` **describes**: it answers "what is happening", +/// which is the question §3's promise of visible asynchronous work is +/// about, and which a label chosen for uniqueness routinely does not +/// answer. +/// +/// # Errors +/// +/// Absent, empty, whitespace-only, or non-string. Empty and +/// whitespace-only are rejected because they satisfy the type and defeat +/// the point exactly as copying the label across would — R42 already +/// rejects whitespace-only `description`s in the config registry for the +/// same reason. +/// +/// The read is **raw**, matching the posture `stdin` and `group` already +/// document in [`lua_to_spec`]: a spec table is plain data, so a +/// metatable cannot smuggle a purpose in through `__index`. +fn required_purpose(table: &Table) -> mlua::Result { + let purpose = match table.raw_get::("purpose") { + Ok(mlua::Value::String(value)) => value.to_str()?.to_owned(), + Ok(mlua::Value::Nil) => { + return Err(mlua::Error::external( + "pmacs.process.spawn: purpose is required — a short description of what \ + this process is DOING, e.g. purpose = \"running the project's test suite\". \ + It is not the label: the label identifies the process, the purpose says \ + what it is for.", + )); + } + Ok(other) => { + return Err(mlua::Error::external(format!( + "pmacs.process.spawn: purpose must be a string; got {}", + other.type_name() + ))); + } + Err(error) => return Err(error), + }; + if purpose.trim().is_empty() { + return Err(mlua::Error::external( + "pmacs.process.spawn: purpose must not be empty or whitespace-only", + )); + } + Ok(purpose) +} + fn lua_to_spec(table: &Table) -> mlua::Result { let label: String = table.get("label").unwrap_or_else(|_| "unnamed".to_owned()); let command: String = table.get("command")?; - // Worker identity Stage 1: required on the Rust struct, optional at - // this surface, falling back to the label. - // - // Requiring it here would break every existing `pmacs.process.spawn` - // caller, and the compiler obligation this lane is buying is on the - // *Rust* construction sites — the ones a future field would silently - // skip. A Lua caller that supplies nothing gets its own label back, - // which is what the caller already chose to call this work; it is - // less informative than a real description but it is not a - // fabrication, which is the bar `owner` failed (framing §3). - let purpose: String = table - .get::>("purpose") - .ok() - .flatten() - .unwrap_or_else(|| label.clone()); + let purpose = required_purpose(table)?; let args: Vec = table.get("args").unwrap_or_default(); let cwd: Option = table.get("cwd").ok().flatten(); let env_table: Option
= table.get("env").ok().flatten(); diff --git a/tests/compile_mode_acceptance.rs b/tests/compile_mode_acceptance.rs index 1801641..54ecf37 100644 --- a/tests/compile_mode_acceptance.rs +++ b/tests/compile_mode_acceptance.rs @@ -1836,7 +1836,8 @@ fn r1f6_wrong_spec_types_error_instead_of_defaulting() { &s, r#" local ok, err = pcall(pmacs.process.spawn, - { label = "t", command = "/bin/true", stdin = true }) + { label = "t", purpose = "type-check probe", command = "/bin/true", + stdin = true }) return ok, tostring(err) "#, ); @@ -1846,7 +1847,8 @@ fn r1f6_wrong_spec_types_error_instead_of_defaulting() { &s, r#" local ok, err = pcall(pmacs.process.spawn, - { label = "t", command = "/bin/true", group = "true" }) + { label = "t", purpose = "type-check probe", command = "/bin/true", + group = "true" }) return ok, tostring(err) "#, ); @@ -2234,7 +2236,8 @@ fn r3f3_spec_fields_are_raw_reads_metatables_not_honored() { &s, r#" local spec = setmetatable( - { label = "mt", command = "/bin/sh", args = { "-c", "sleep 30" } }, + { label = "mt", purpose = "raw-read probe", command = "/bin/sh", + args = { "-c", "sleep 30" } }, { __index = function(_, k) if k == "group" then return true end return nil @@ -2265,7 +2268,8 @@ fn r3f3_spec_fields_are_raw_reads_metatables_not_honored() { &s, r#" local spec = setmetatable( - { label = "mt2", command = "/bin/sh", args = { "-c", "exit 0" } }, + { label = "mt2", purpose = "raw-read probe", command = "/bin/sh", + args = { "-c", "exit 0" } }, { __index = function() error("hostile spec metatable") end }) local ok = pcall(pmacs.process.spawn, spec) return ok diff --git a/tests/fixtures/pmacs-magit/status.lua b/tests/fixtures/pmacs-magit/status.lua index 283b9a6..59077c0 100644 --- a/tests/fixtures/pmacs-magit/status.lua +++ b/tests/fixtures/pmacs-magit/status.lua @@ -54,6 +54,10 @@ function M.run_git(args, opts) opts = opts or {} local id = pmacs.process.spawn { label = "git " .. (args[1] or ""), + -- Worker identity Stage 1: `purpose` is required. The full argument + -- vector, not just the subcommand the label carries -- "git log" and + -- "git log --oneline -20" are the same label and different work. + purpose = "git " .. table.concat(args, " "), command = "git", args = args, cwd = opts.cwd, diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index d182eb5..d706305 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -1173,6 +1173,7 @@ fn m4_4_lua_surface_drives_lifecycle() { r#" local id = pmacs.process.spawn { label = "lua-hello", + purpose = "greeting the Lua surface end to end", command = "/bin/sh", args = { "-c", "printf hi-from-lua && exit 0" }, } diff --git a/tests/worker_identity_acceptance.rs b/tests/worker_identity_acceptance.rs index d7bdbfb..96decfb 100644 --- a/tests/worker_identity_acceptance.rs +++ b/tests/worker_identity_acceptance.rs @@ -264,33 +264,84 @@ fn every_entry_shape_records_what_its_work_is() { pump(&mut state); } -/// A `pmacs.process.spawn` caller that supplies no purpose keeps -/// working, and gets its own label back rather than an empty field. +/// **`pmacs.process.spawn` REFUSES a spec with no purpose, and spawns +/// nothing.** /// -/// The Rust struct's field is required — the compiler enforces that at -/// every construction site. This surface is deliberately lenient, -/// because requiring it here would break every existing caller for no -/// coverage the compiler is not already providing. +/// An earlier revision of this lane defaulted the field to `label` so +/// that existing callers kept working. That preserved compatibility and +/// delivered nothing: §9's complaint about `ProcessSpec` is precisely +/// that `label` is "caller-supplied, unvalidated convention", so a +/// purpose defaulting to the label hands every caller back the +/// convention this lane exists to replace. +/// +/// Four refusals, each asserted the same way — the call raises, the +/// message names the field, and **the process list is unchanged**, +/// because a validation that rejects after spawning has already done the +/// thing it was rejecting: +/// +/// * absent; +/// * empty, and whitespace-only — these satisfy the type and defeat the +/// point exactly as copying the label would (R42 rejects +/// whitespace-only config descriptions for the same reason); +/// * wrong type; +/// * **metatable-provided**, which is the `stdin`/`group` raw-read +/// posture: a spec table is plain data, so a purpose cannot be +/// smuggled in through `__index`. #[test] -fn a_process_spawned_without_a_purpose_falls_back_to_its_label() { +fn spawning_without_a_real_purpose_is_refused_and_starts_nothing() { let mut state = editor(); - exec( - &state, - r#"P = pmacs.process.spawn { - label = "legacy-caller", - command = "/bin/sh", - args = { "-c", "sleep 5" }, - }"#, - ); - let purpose: String = eval( - &state, - "for _, row in ipairs(pmacs.process.list()) do - if row.label == 'legacy-caller' then return row.purpose end - end - return ''", - ); - assert_eq!(purpose, "legacy-caller"); - exec(&state, "pmacs.process.terminate(P)"); + let baseline: usize = eval(&state, "return #pmacs.process.list()"); + + for (label, spec, expected) in [ + ( + "absent", + r#"{ label = "x", command = "/bin/sh", args = { "-c", "sleep 5" } }"#, + "purpose is required", + ), + ( + "empty", + r#"{ label = "x", purpose = "", command = "/bin/sh", args = { "-c", "sleep 5" } }"#, + "must not be empty", + ), + ( + "whitespace-only", + r#"{ label = "x", purpose = " ", command = "/bin/sh", args = { "-c", "sleep 5" } }"#, + "must not be empty", + ), + ( + "wrong type", + r#"{ label = "x", purpose = 7, command = "/bin/sh", args = { "-c", "sleep 5" } }"#, + "purpose must be a string", + ), + ( + "metatable-provided", + r#"setmetatable( + { label = "x", command = "/bin/sh", args = { "-c", "sleep 5" } }, + { __index = function(_, k) + if k == "purpose" then return "smuggled" end + return nil + end })"#, + "purpose is required", + ), + ] { + let (ok, err): (bool, String) = eval( + &state, + &format!( + "local ok, err = pcall(pmacs.process.spawn, {spec}) + return ok, tostring(err)" + ), + ); + assert!(!ok, "{label}: spawn must refuse"); + assert!( + err.contains(expected), + "{label}: the refusal must name the field and the rule; got {err:?}" + ); + assert_eq!( + eval::(&state, "return #pmacs.process.list()"), + baseline, + "{label}: a refused spawn must start no process" + ); + } pump(&mut state); } From d4a69cae14af22038f0804fab8f8e4c9a5621e03 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 17:33:55 +0200 Subject: [PATCH 08/20] docs: record review round 1, and name the statusline adopter that is not named after its file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two section-local edits, no reflowing. **`docs/worker-identity-framing.md` §2** named the three existing statusline adopters by FILE — `terminal.lua`, `syntax.lua`, `lsp.lua` — which is accurate and misleading together: `syntax.lua` registers its provider under the name **`"mode"`**, so the registry inventory reads `["mode", "terminal", "lsp"]` and a reader looking for the syntax adopter by name does not find one. That is what made this lane's change to `tests/statusline_segments_acceptance.rs` surprising, and the next reader should not have to rediscover it. Also records that where a fourth registration sorts is decided by **load order**, not by name. **`docs/active-work.md`'s lane block** records review round 1: the `pmacs.process.spawn` blocker and its fix at `2162737`, the five refused shapes, the eleven updated call sites, the two audit-fixture occurrences that are deliberately untouched, the three added mutation checks, and the two acceptance suites the round added to the gate line (`compile_mode_acceptance` and `m8_6_acceptance`, because the round moved their spawn call sites and `m8_6` covers the `pmacs-magit` package fixture). It also records the breaking-change decision with the reasoning that justified it, rather than only the outcome: the binding has no API-reference documentation and no stability promise, `lua_to_spec` has one caller, and §10/P7 put the third-party population at ~zero — so the cost of requiring the field is at its minimum now and rises from here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 47 ++++++++++++++++++++++++++++----- docs/worker-identity-framing.md | 12 +++++++++ 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 7a7d649..474e6a9 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -345,9 +345,14 @@ the authoritative tip** — the ref, not a SHA. Recover with "not tracked" (Q#W-5). - **Gates:** `scripts/gate --acceptance worker_identity_acceptance --acceptance journey_acceptance --acceptance - statusline_segments_acceptance`. No `--protocol` — no wire change. -- **IMPLEMENTED at `1aca0ee`**, one commit on top of the four framing - revisions. `tests/worker_identity_acceptance.rs` is the new suite: 18 + statusline_segments_acceptance --acceptance compile_mode_acceptance + --acceptance m8_6_acceptance`. No `--protocol` — no wire change. + `compile_mode` and `m8_6` joined at review round 1, which moved their + spawn call sites; `m8_6` covers the `pmacs-magit` fixture, and a newly + required field is exactly the kind of change that breaks a package + fixture quietly. +- **IMPLEMENTED at `1aca0ee`**, with review round 1's blocker fixed at + `2162737`. `tests/worker_identity_acceptance.rs` is the new suite: 18 tests, plus one consumer-side witness beside the private renderer in `pmacs-gpu`. - **`journey_acceptance` passed UNTOUCHED (47/47)** — the stop signal @@ -366,7 +371,7 @@ the authoritative tip** — the ref, not a SHA. Recover with `syntax.lua`, `terminal.lua` and `lsp.lua`. That assertion exists to grow when a builtin provider is added; it is listed here so the change is not mistaken for an accommodation. -- **20 mutation checks, each test falsified by removing its own fix.** +- **23 mutation checks, each test falsified by removing its own fix.** The ones worth naming: siting the `await` guard *inside* the `_is_complete` branch (the already-complete case then slips through — which is the whole reason the guard is unconditional); replacing @@ -380,20 +385,48 @@ the authoritative tip** — the ref, not a SHA. Recover with One of the twenty is a **preservation** check rather than a new claim: bracketing `pmacs.workers.dispatch` with `local ok, result = pcall(...)` truncates a handler that returns more - than one value, which every other test in the suite tolerates. + than one value, which every other test in the suite tolerates. Round + 1 added three more against the spawn refusal: restoring the + label fallback, accepting an empty/whitespace-only purpose, and + reading the field non-raw so a metatable can smuggle one in. - **Two residuals, stated rather than tested around.** Raw `coroutine.yield` inside either dynamic scope still leaks the scope — loudly, through `pmacs.error`, but it leaks; no refusal sited in a yield helper can intercept it (framing §2). And Q#W-7's reachability by a real caller stays **unproven**: the commit message says so, and the test pins the guard rather than reproducing a fault. +- **Review round 1 blocker — `pmacs.process.spawn` now REQUIRES + `purpose`.** The first implementation made it optional at the Lua + surface, falling back to `label`. That preserved compatibility and + delivered nothing: §9's complaint about `ProcessSpec` is exactly that + `label` is "caller-supplied, unvalidated convention", so a purpose + defaulting to it hands every caller back the convention the lane exists + to replace. Refused on five shapes — absent, empty, whitespace-only, + wrong type, metatable-provided — each asserting the process list is + unchanged, since a validation that rejects after spawning has already + done the thing it rejected. +- **That is a BREAKING CHANGE to a public Lua API, taken now on + purpose.** §10 grades extension trust "missing (one class)" and P7 + package lifecycle has not started, so the third-party population is + ~zero and the cost only rises later. Checked for a reason that would be + wrong and found none: `pmacs.process.spawn` has no API-reference + documentation and no stability promise in `docs/` (the package-author + guide's only mentions are an audit-rule classification and a pointer to + the bundled REPL; its semver language governs packages' own versioning, + not pmacs's Lua surface), and `lua_to_spec` has exactly one caller. + **Eleven executable call sites updated**, each with a real description + rather than the label copied across: `repl/init.lua`, `compile.lua`, + `lean.lua`, the `pmacs-magit` fixture, and seven in tests. The two + `pmacs.process.spawn("ls")` occurrences in `src/audit/mod.rs` and + `tests/m7_9_acceptance.rs` are **audit fixture source text** — lexed, + never executed — and are deliberately untouched. - **Surfaces that changed shape, for anyone rebasing onto this:** `AsyncRuntime::allocate`/`allocate_with_resource` collapsed into one private `JobSpec`-taking funnel; `register_external` grew a third parameter; `ProcessSpec::new` grew a third parameter (~40 call sites, nearly all tests); `ActiveJobInfo`/`CompletedJobInfo`/`ProcessSpec` - each grew a required `purpose` field. `pmacs.process.spawn`'s Lua - surface keeps `purpose` optional, falling back to the label. + each grew a required `purpose` field, and `pmacs.process.spawn` + requires `purpose` in its spec table. ## Discovery Stage 2 — PR #228 OPEN, **MERGE-BLOCKED** diff --git a/docs/worker-identity-framing.md b/docs/worker-identity-framing.md index d86d502..b2280a4 100644 --- a/docs/worker-identity-framing.md +++ b/docs/worker-identity-framing.md @@ -191,6 +191,18 @@ And the two findings that actually shape the design: `nil`. An activity indicator is a **fourth registration**, not a new mechanism. + **The three are named by FILE above and by NAME in the registry, and + the two do not line up.** `syntax.lua` registers its provider as + **`"mode"`** (it projects the major mode, `syntax.lua:552`), so the + registry inventory reads `["mode", "terminal", "lsp"]` — which is what + `tests/statusline_segments_acceptance.rs` asserts. Recorded because it + is genuinely surprising: a reader looking for the syntax adopter by + name does not find one. A fourth registration therefore changes that + assertion, and where the new name sorts depends on **load order**, not + on the name: `async.lua` is evaluated before `syntax.lua`, + `terminal.lua` and `lsp.lua` (`src/editor.rs`), so a provider + registered there lands first. + **And it is evaluated per frame**: `evaluate_statusline` is called inside `paint_frame` (`src/editor.rs:4560`), before the long mutable core borrow. So an indicator updates while work is in flight without From 37a81227c750d955e5b4ca24ddc8817f8c99c001 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 17:45:07 +0200 Subject: [PATCH 09/20] docs: R7 has a second occurrence, and this time the fragments were captured MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `attach::tests::managed_retry_survives_transients_and_uses_the_successful_stream` failed once at this lane's `scripts/gate` **`gpu` step** on 2026-08-09. Judged against this file rather than rerun-and-shrugged. **It matches R7 on all three of its required fragments**, verified rather than inferred: transient sequence must attach: Attach(Handshake(Io(Os { code: 32, kind: BrokenPipe, message: "Broken pipe" }))) **That capture is the point.** U2 and U3 both record the identical loss — "output was filtered to the `FAILED` line" — and U3 says outright that the recurring mistake was its author's, twice, with a mechanical fix: read the durable log, never the live stream. The gate writes `NN-gpu.log` for exactly this, and reading it turned what would have been a third unjudgeable `U` note into a second occurrence of a row that had one. The flavor is a third one (`PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu`, neither occurrence 1's `--features crdt` sweep nor U3's default-features workspace sweep). Recorded because this file's own R2 worked example treats flavor as outside matching. **The merge-base control R7 asked for was run, and it settles nothing.** 15 runs at `4bc55e8`, green — but the observing branch was green over 30 runs too (15 isolated selector, 15 full suite), so neither side reproduced and the comparison separates nothing. Logged as a null result, not as exculpation. Per the rerun rule, all 45 green runs establish **intermittence only**. **And one causal path is named rather than dismissed:** this lane adds a GPU-heavy `render_offscreen` test to `pmacs-gpu`'s test module. It touches no `attach.rs`, no protocol and no wire — but it does add a concurrent test to the same binary, and the failing test is a socket handshake on a one-second deadline. Contention is a plausible `BrokenPipe` mechanism and 30 green runs do not exclude it. The row now says what the discriminating control would be if there is a third occurrence: remove the added test, not go to the merge base. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/ci-red-signatures.md | 44 ++++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 7 deletions(-) diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index 0e4e6a9..c94c73b 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -496,16 +496,46 @@ Stage 4; the lane touches no `pmacs-gpu` code at all. | **selector** | `-p pmacs-gpu attach::tests::managed_retry_survives_transients_and_uses_the_successful_stream` | | **job / flavor** | local (Linux), `cargo test --workspace --features crdt --no-fail-fast`, i.e. under full-sweep load | | **required fragments** | `transient sequence must attach` + `Handshake(Io(` + `BrokenPipe` (or `code: 32`) | -| **status** | **new incident, unreproduced — causal status UNRESOLVED** | -| **what IS established** | one occurrence at `pmacs-gpu/src/attach.rs:1680`; the test drives a scripted transient-then-success sequence over a real socket pair | +| **status** | **SECOND OCCURRENCE 2026-08-09 — causal status still UNRESOLVED** | +| **what IS established** | **two** occurrences at `pmacs-gpu/src/attach.rs:1680`, the second with all three fragments **verified** rather than inferred; the test drives a scripted transient-then-success sequence over a real socket pair | | **what is NOT** | whether the broken pipe is the *fixture's* writer closing early or a real retry-path defect. **This row is not a claim that it is harmless** | -| **rerun evidence** | 6 isolated runs green, plus a full `--workspace --features crdt` sweep green (113 targets). Per the rerun rule this establishes **intermittence only** | +| **rerun evidence** | occurrence 1: 6 isolated runs green, plus a full `--workspace --features crdt` sweep green (113 targets). Occurrence 2: **30 green on the observing branch** (15 isolated selector, 15 full `-p pmacs-gpu`) **plus a 15-run merge-base control, also green**. Per the rerun rule all of this establishes **intermittence only** | | **retirement** | hardening that removes the named mechanism plus a discriminating witness — or a diagnosis showing the fixture, not the code, closes the pipe | -**Not attributed to this lane**, and the reasoning is not merely "my -diff looks unrelated": Stage 4 adds no wire surface, no protocol -version change, and touches no file in `pmacs-gpu`. A merge-base -control would settle it if this recurs. +**Not attributed to the observing lane**, and in neither case is the +reasoning merely "my diff looks unrelated": long-lines Stage 4 added no +wire surface, no protocol version change, and touched no file in +`pmacs-gpu`. + +**Second occurrence — worker identity Stage 1, 2026-08-09, local +(Linux).** Recorded at the `scripts/gate` **`gpu` step** +(`PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu`), which is a **third +flavor**: not the `--features crdt` sweep of occurrence 1, and not U3's +default-features workspace sweep. Two things make it a match rather than +a `U` note: + +* **The fragments were captured this time.** `transient sequence must + attach: Attach(Handshake(Io(Os { code: 32, kind: BrokenPipe, message: + "Broken pipe" })))` — all three of the row's required fragments, + verified against the durable gate log rather than a filtered live + stream. **That is what U2 and U3 both lost**, and it is why U3 could + not be judged a recurrence. Reading the gate's own `NN-gpu.log` is the + mechanical fix U3 prescribed, and it worked. +* **The merge-base control R7 asked for was run** — 15 runs at `4bc55e8`, + green. It is **non-discriminating**, not exculpatory: the observing + branch was equally green over 30 runs, so neither side reproduced and + the control separates nothing. Recorded as a null result rather than + as evidence. + +**One causal path is NOT excluded and is named here rather than +dismissed.** The observing lane added a test to `pmacs-gpu`'s test module +(`main.rs`) — a GPU-heavy `render_offscreen` case. It touches no +`attach.rs`, no protocol, and no wire, but it does add a concurrent test +to the same binary, and the failing test is a socket handshake with a +one-second deadline. Contention is a plausible mechanism for a +`BrokenPipe`, and 30 green runs do not rule it out. If a third occurrence +lands, **run the control with the added test removed** rather than at the +merge base — that is the discriminating comparison this one was not. ### U2 — `m6_1_pty_raw_mode_disables_kernel_echo`, one local occurrence From 70262888b434fee952c3681460dbbe060cb5d16b Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 19:33:07 +0200 Subject: [PATCH 10/20] fix(workers): a safe display-text boundary for purpose and handler names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2, findings P2a and P2b, plus P3's stale recovery summaries. Three defects, and the fix is deliberately different in each place because the constraint is. ## P2a — invalid UTF-8 bypassed the `purpose` diagnostic `required_purpose` read the field with `value.to_str()?`. Lua strings are BYTE strings, so `purpose = string.char(255)` is a value a caller can write, and `?` surfaced mlua's generic conversion error BEFORE this lane's own diagnostic was ever constructed: the caller was told neither the field nor the rule. **This is the third time this project has hit the class** — an unowned Lua string converted with `?` ahead of the owned message; the destination-capture lane corrected the same shape two rounds ago. It refused before spawning and nothing leaked, so the defect was the message, not the outcome. The conversion failure is now mapped onto this function's own message, and the new acceptance row asserts on message CONTENT so retyping the read as a bare `?` breaks the test rather than silently degrading the error. Auditing the rest of the lane's diff for the same class turned up exactly one more: `_push_dispatch_name` took `name: String`, so a registered handler name that was not valid UTF-8 failed at first dispatch with mlua's generic message. It now takes `mlua::String` and maps that failure onto an owned diagnostic naming the argument and the rule. Those are the only two Lua-string reads this lane added; every other binding it adds takes `()`. ## P2b — no safe display-text boundary. Two halves, two different fixes ### Handler names are refused at the source `pmacs.workers.register` type-checked its name and nothing more, which was defensible while the name died inside `dispatch`. It no longer dies there: the ambient carries it into every job the handler allocates and composes it into `purpose`, which `*workers*` and the modeline both render. So it now gets `purpose`'s meaningful-value standard — non-empty, not whitespace-only — plus control characters, which have no legitimate place in a registered identifier. ### Purposes are ESCAPED at presentation, not rejected at the registry A purpose may legitimately contain a newline: a filesystem path can, and `pmacs-magit`'s spawn purpose is a whole argv. **This is the shape of the `#228` decision, and it is consistent with it** — the one-line constraint belongs to the surface that has it, not to the registry that does not. There, `Command.description` stays free-form and the two single-row consumers clip with `description_first_line`. Here the equivalent is escaping rather than clipping, because a purpose's later words are load-bearing: an argv's second word says which file, and a clip would drop it silently. `purpose_for_one_row` states the property it exists for: **a row must not be able to forge another row.** It escapes `\n`, `\r`, `\t` and the rest of the Unicode `Cc` class (which covers ESC, so a purpose cannot open a terminal escape sequence either), borrows unchanged when there is nothing to escape — making byte-identity structural rather than asserted — and deliberately does NOT escape backslashes: no number of them produces a second row, and doubling them would cost byte-identity for ordinary text. Two surfaces call it: the `*workers*` rows, and `ActivitySummary`, which exists for one consumer that has exactly one row. `pmacs.workers.snapshot()` is this lane's `describe-command` and stays raw, which is what makes this a rendering decision rather than data loss — asserted, not assumed. ## P3 — two stale recovery summaries `docs/worker-identity-framing.md` still said "Implementation may proceed"; it is implemented. `docs/active-work.md` still said Stage 1 takes the "first two" of owner/purpose/parent — `owner` was REMOVED in revision 2, so it takes one of the three, and the claim the whole `owner` argument overturned was still standing in the volatile state of record. Both fixed section-locally. ## Verification `tests/worker_identity_acceptance.rs`, 18 -> 24 tests: * invalid-UTF-8 purpose refused by THIS lane's message, asserted on content, alongside the absent / empty / whitespace / wrong-type / metatable rows; * a whitespace-only handler name and a control-character one are each refused AT `register`, asserted on the error and on the handler not being installed (dispatch reports `unknown handler`); * a non-UTF-8 handler name is refused before the handler runs, with the dispatch-name stack left empty; * a purpose containing a newline renders as ONE row in `*workers*` and as one line in the modeline — through the real rendering path, the latter through a painted frame as well as the evaluator; * **a purpose crafted to look like a row boundary does not produce a second row** — asserted by counting rows, with the escaped text asserted present so a renderer that dropped the purpose entirely could not pass; * a purpose with no control characters is byte-identical on both surfaces, fixtured with a literal backslash, a literal `\v`, quotes and a non-ASCII character. Mutation-checked, seven guards, each failing its own test and no other: the purpose UTF-8 diagnostic; the `_push_dispatch_name` one; the register whitespace guard; the register control-character guard; the `*workers*` call site; the `ActivitySummary` call site; and `purpose_for_one_row` itself neutered to the identity, which fails both surfaces' tests and nothing else. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- builtin/runtime/async.lua | 34 ++++ docs/active-work.md | 14 +- docs/worker-identity-framing.md | 5 +- src/async_runtime.rs | 81 +++++++- src/lua_bindings/mod.rs | 48 ++++- src/workers_buffer.rs | 12 +- tests/worker_identity_acceptance.rs | 305 +++++++++++++++++++++++++++- 7 files changed, 476 insertions(+), 23 deletions(-) diff --git a/builtin/runtime/async.lua b/builtin/runtime/async.lua index 000be49..eb813da 100644 --- a/builtin/runtime/async.lua +++ b/builtin/runtime/async.lua @@ -468,6 +468,26 @@ function pmacs.workers.dispatch(name, args, opts) return finish_dispatch(pcall(handler, args, opts)) end +-- Worker identity Stage 1: the name registered here is DISPLAY TEXT. +-- +-- It used to be type-checked and nothing more, which was defensible +-- while it died inside `dispatch`. It no longer dies there: the ambient +-- carries it into every job the handler allocates, and it is composed +-- into `purpose` as `": "`, which the `*workers*` table +-- and the modeline indicator both render. So it gets the same +-- meaningful-value standard `purpose` already gets in +-- `required_purpose` (`src/lua_bindings/mod.rs`) --- and one rule +-- `purpose` deliberately does NOT get. +-- +-- The asymmetry is the point. A purpose may legitimately contain a +-- newline: a filesystem path can, and `pmacs-magit`'s spawn purpose is a +-- whole argv --- so its one-line constraint is enforced by ESCAPING at +-- the surfaces that have one row (`purpose_for_one_row`), following the +-- `#228` decision on `Command.description`. A registered handler NAME +-- has no such case. It is an identifier a package chooses for itself and +-- passes back to `dispatch`, so a control character in it is a mistake +-- or an attempt at one, and refusing at the source costs nobody +-- anything. function pmacs.workers.register(name, handler) -- Allows future Rust-side modules (or test harnesses) to register -- additional dispatchable names. v0.1 has no plugin loader but the @@ -475,6 +495,20 @@ function pmacs.workers.register(name, handler) if type(name) ~= "string" then error("pmacs.workers.register: name must be a string") end + -- Empty and whitespace-only satisfy the type and say nothing --- the + -- exact pair `required_purpose` rejects, and the exact pair R42 + -- rejects for config descriptions. + if name:match("^%s*$") ~= nil then + error("pmacs.workers.register: name must not be empty or whitespace-only") + end + -- `%c` is the C control class: NUL, the C0 range, DEL. A newline + -- forges a row in `*workers*`, a CR rewrites one on a terminal and an + -- ESC starts a sequence in one. Checked AFTER the whitespace rule so + -- a name that is only "\n" reports the emptier problem, which is the + -- one the caller can act on. + if name:find("%c") ~= nil then + error("pmacs.workers.register: name must not contain control characters") + end if type(handler) ~= "function" then error("pmacs.workers.register: handler must be a function") end diff --git a/docs/active-work.md b/docs/active-work.md index 474e6a9..83f2701 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -339,10 +339,16 @@ the authoritative tip** — the ref, not a SHA. Recover with so making terminal PTYs visible is deferred to Stage 2 with a separate accessor rather than by widening this one. - **Deliberate deviation from the audit, flagged for review:** §9 names - owner/purpose/**parent** together as the prerequisite; Stage 1 takes - only the first two. A parent needs an ambient "currently-running job" - context, and an unpopulated `parent` reads as "no parent" rather than - "not tracked" (Q#W-5). + owner/**purpose**/parent together as the prerequisite; Stage 1 takes + **only `purpose`** — one of the three, not two. `owner` was removed in + revision 2: nothing in the runtime knows which package asked for a + job, so an `owner` field could only have been filled with the same + handler name `purpose` already carries, and an empty one reads as + "unowned" rather than "not tracked". `parent` is out for the matching + reason — it needs an ambient "currently-running job" context, and an + unpopulated `parent` reads as "no parent" rather than "not tracked" + (Q#W-5). The package-ownership slot stays **deliberately empty** until + P3 can fill it with a real signal (framing §3, §7). - **Gates:** `scripts/gate --acceptance worker_identity_acceptance --acceptance journey_acceptance --acceptance statusline_segments_acceptance --acceptance compile_mode_acceptance diff --git a/docs/worker-identity-framing.md b/docs/worker-identity-framing.md index b2280a4..aa165e0 100644 --- a/docs/worker-identity-framing.md +++ b/docs/worker-identity-framing.md @@ -5,8 +5,9 @@ removed that title overclaimed the lane: it answers **what**, and — under `pmacs.workers.dispatch` — **under which registered handler**. Neither is who owns it.)* -**Status: revision 4, APPROVED 2026-08-09. Implementation may -proceed.** +**Status: revision 4, APPROVED 2026-08-09. IMPLEMENTED — see +`docs/active-work.md` for the commits, the gate outcome and the review +rounds.** **Revision 4 scopes rule 1's claim to what it can actually enforce, and takes Q#W-7 into this lane.** Revision 3 said the rule covered "all diff --git a/src/async_runtime.rs b/src/async_runtime.rs index e60d0ae..284d2ee 100644 --- a/src/async_runtime.rs +++ b/src/async_runtime.rs @@ -58,8 +58,10 @@ //! search with cooperative cancellation and frame-boundary coalescing. //! Tree-sitter and LSP land in M4 on the same dispatch shape. +use std::borrow::Cow; use std::cell::{Cell, RefCell}; use std::collections::{HashMap, VecDeque}; +use std::fmt::Write; use std::path::{Path, PathBuf}; use std::rc::Rc; use std::sync::atomic::{AtomicU64, Ordering}; @@ -607,15 +609,87 @@ pub struct ActivitySummary { /// is always present costs modeline width forever to say "nothing /// is happening". pub in_flight: usize, - /// The **oldest** in-flight job's purpose. + /// The **oldest** in-flight job's purpose, already passed through + /// [`purpose_for_one_row`]. /// /// Oldest, not newest and not "busiest": jobs carry no cost /// estimate, so "busiest" is not a defined quantity, while oldest /// is computable from `dispatched_at` and answers the question a /// user actually asks of a stuck editor. + /// + /// Escaped here rather than at the Lua provider because this struct + /// **is** the indicator's read surface — it exists for one consumer, + /// and that consumer has exactly one row. `workers_snapshot` is the + /// free-form path and stays raw. pub oldest_purpose: String, } +/// A `purpose` rendered for a surface that gives it exactly **one row**. +/// +/// # A row must not be able to forge another row +/// +/// That is the property, and it is the only reason this exists. A +/// purpose is free-form text supplied by whoever dispatched the work, +/// and it is legitimately multi-line: a filesystem path may contain a +/// newline, and `pmacs-magit`'s spawn purpose is a whole argv. Rendered +/// raw into a row-per-job table, one such purpose becomes two physical +/// lines — the second of which the reader has no way to tell from a real +/// job row, because a real job row is just text in the same buffer. +/// The same applies to `\r`, which rewrites a rendered line in place on +/// a terminal, and to `\u{1b}`, which starts an escape sequence in one. +/// +/// # Escape, do not reject, and do not clip +/// +/// This follows the `#228` decision recorded on +/// [`crate::command::Command::description`]: the one-line constraint +/// belongs to the **surface that has it**, not to the registry that does +/// not. There, a free-form description is clipped by +/// `Command::description_first_line` at the two single-row consumers +/// while the registry keeps every line. Here the equivalent is escaping +/// rather than clipping, because a purpose's later lines are not +/// decoration — an argv's second word is as load-bearing as its first, +/// and a clip would silently drop the part that says which file. +/// +/// `pmacs.workers.snapshot()` is this lane's `describe-command`: it +/// hands Lua the raw purpose, so nothing is lost, only made safe where +/// a row boundary means something. +/// +/// # What is not escaped +/// +/// A backslash. Escaping it would make a purpose containing no control +/// characters **not** byte-identical after this call, and byte-identity +/// for ordinary text is a property worth more than distinguishing a +/// literal `\n` from an escaped newline — the ambiguity is cosmetic, +/// while forging a row is not, and no amount of literal backslashes +/// produces a second row. +#[must_use] +pub fn purpose_for_one_row(purpose: &str) -> Cow<'_, str> { + // `char::is_control` is the Unicode `Cc` category: C0 (`\0`–`\x1f`), + // `\x7f`, and C1 (`\u{80}`–`\u{9f}`, which includes NEL). Borrowing + // when there is nothing to do keeps the common path allocation-free + // AND makes the byte-identity property structural rather than + // asserted. + if !purpose.contains(char::is_control) { + return Cow::Borrowed(purpose); + } + let mut out = String::with_capacity(purpose.len() + 8); + for ch in purpose.chars() { + match ch { + '\n' => out.push_str("\\n"), + '\r' => out.push_str("\\r"), + '\t' => out.push_str("\\t"), + other if other.is_control() => { + // `\u{1b}`, the same spelling Rust's own `escape_debug` + // uses, so the rendered form is one a reader can paste + // back into either language and get the byte returned. + let _ = write!(out, "\\u{{{:x}}}", other as u32); + } + other => out.push(other), + } + } + Cow::Owned(out) +} + /// One frame's worth of streamed items for a single stream id, /// returned by [`AsyncRuntime::take_stream_batches`]. T M3.5. #[derive(Clone, Debug)] @@ -1514,7 +1588,10 @@ impl AsyncRuntime { let (_, purpose) = oldest?; Some(ActivitySummary { in_flight, - oldest_purpose: purpose.to_owned(), + // The modeline is one row and a segment is one line; + // `purpose_for_one_row` is what keeps a purpose carrying a + // newline (a path, an argv) from breaking it. + oldest_purpose: purpose_for_one_row(purpose).into_owned(), }) } diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index c47b678..406a3d6 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -7580,12 +7580,27 @@ pub fn install_async( // `pmacs.workers.dispatch`, which brackets these itself under // `pcall`. A package pushing by hand and failing to pop would poison // every later dispatch in the session with a stale name. + // + // `mlua::String`, not `String`: the parameter is a Lua BYTE string, + // so an `mlua`-driven `String` conversion would refuse a non-UTF-8 + // name with a generic message naming neither the argument nor the + // rule. `pmacs.workers.register` enforces the rest of the + // display-text standard (non-empty, no control characters) but + // cannot see UTF-8 validity from Lua 5.1, so the byte-level half is + // enforced here — the one point where Rust sees the name — with a + // message that names both. { let rt = runtime.clone(); async_mod.set( "_push_dispatch_name", - lua.create_function(move |_, name: String| { - rt.push_dispatch_name(name); + lua.create_function(move |_, name: mlua::String| { + let Ok(text) = name.to_str() else { + return Err(mlua::Error::external( + "pmacs.workers.dispatch: handler name must be valid UTF-8 — it is \ + displayed to the user as part of every job's purpose.", + )); + }; + rt.push_dispatch_name(&*text); Ok(()) })?, )?; @@ -8766,18 +8781,35 @@ fn parse_restart(name: &str) -> mlua::Result { /// /// # Errors /// -/// Absent, empty, whitespace-only, or non-string. Empty and -/// whitespace-only are rejected because they satisfy the type and defeat -/// the point exactly as copying the label across would — R42 already -/// rejects whitespace-only `description`s in the config registry for the -/// same reason. +/// Absent, empty, whitespace-only, non-string, or **not valid UTF-8**. +/// Empty and whitespace-only are rejected because they satisfy the type +/// and defeat the point exactly as copying the label across would — R42 +/// already rejects whitespace-only `description`s in the config registry +/// for the same reason. +/// +/// The UTF-8 case is a **reachable input class, not an internal +/// invariant**: Lua strings are byte strings, so `purpose = +/// string.char(255)` is a value a caller can write. Converting it with +/// `?` would surface mlua's generic conversion error *before* any of the +/// diagnostics below is constructed, and the caller would be told +/// neither the field nor the rule — so the conversion failure is mapped +/// onto this function's own message instead. /// /// The read is **raw**, matching the posture `stdin` and `group` already /// document in [`lua_to_spec`]: a spec table is plain data, so a /// metatable cannot smuggle a purpose in through `__index`. fn required_purpose(table: &Table) -> mlua::Result { let purpose = match table.raw_get::("purpose") { - Ok(mlua::Value::String(value)) => value.to_str()?.to_owned(), + Ok(mlua::Value::String(value)) => match value.to_str() { + Ok(text) => text.to_owned(), + Err(_) => { + return Err(mlua::Error::external( + "pmacs.process.spawn: purpose must be valid UTF-8 — it is displayed \ + to the user in *workers* and in the modeline, and arbitrary bytes \ + have no display form there.", + )); + } + }, Ok(mlua::Value::Nil) => { return Err(mlua::Error::external( "pmacs.process.spawn: purpose is required — a short description of what \ diff --git a/src/workers_buffer.rs b/src/workers_buffer.rs index 02d6d03..2a3bd59 100644 --- a/src/workers_buffer.rs +++ b/src/workers_buffer.rs @@ -42,6 +42,12 @@ //! `Status` right rather than being truncated: losing the end of a path //! is a worse failure than an uneven column. //! +//! This table is **one row per job**, and the purpose is the only free +//! text in it, so every row goes through +//! [`crate::async_runtime::purpose_for_one_row`]: a row must not be able +//! to forge another row. See that function for why the escaping lives +//! here rather than as a rule on the purpose itself. +//! //! Lua reads the snapshot via `pmacs.workers.snapshot()`; the //! `pmacs.workers.show()` builtin invokes [`render`] on it and //! returns the buffer id. Auto-refresh hooks into @@ -50,7 +56,7 @@ use std::fmt::Write; use crate::async_runtime::{ - ActiveJobInfo, CompletedJobInfo, JobOutcome, JobResult, WorkersSnapshot, + ActiveJobInfo, CompletedJobInfo, JobOutcome, JobResult, WorkersSnapshot, purpose_for_one_row, }; use crate::buffer::{Buffer, BufferId, EditOp}; use crate::buffer_registry::BufferRegistry; @@ -197,7 +203,7 @@ fn write_active_row(text: &mut String, job: &ActiveJobInfo) { if job.is_stream { status.push_str(" [stream]"); } - let purpose = &job.purpose; + let purpose = purpose_for_one_row(&job.purpose); let _ = writeln!( text, "{id:<7} {kind:<11} {age:>9} {key:<11} {purpose:9} {key:<11} {purpose:(&state, "return RAN"), + "and it must refuse BEFORE running the handler" + ); + assert!( + !eval::(&state, "return pmacs._async._in_dispatch_name_scope()"), + "a push that failed must leave no name on the stack" + ); + pump(&mut state); +} + /// **Rule 7 + the defect itself.** A job dispatched through /// `pmacs.workers.dispatch("name", …)` reports `"name"`. /// @@ -898,3 +1053,145 @@ fn the_workers_buffer_renders_the_purpose_column() { exec(&state, "pmacs.workers.hide()"); pump(&mut state); } + +// --------------------------------------------------------------------------- +// 7 — the display-text boundary: a row must not forge another row +// --------------------------------------------------------------------------- +// +// A purpose is free-form caller text and is legitimately multi-line — a +// filesystem path may contain a newline and `pmacs-magit`'s spawn +// purpose is a whole argv — so the one-line constraint belongs to the +// surfaces that have one line, not to the purpose. That is `#228`'s +// decision on `Command.description`, applied here as escaping rather +// than clipping, because a purpose's later words are load-bearing. +// +// Every test below drives the REAL rendering path. Calling +// `purpose_for_one_row` directly would prove the escaper escapes and say +// nothing about whether either surface calls it. +// +// `register_external` is the witness in all three because it is the one +// entry shape whose purpose is verbatim caller text: the pool +// dispatchers all `format!` their own, and `{:?}` in those formats +// already escapes, so a hostile purpose cannot reach a row through them. + +/// **The spoofing property, and the whole reason the escaping exists.** +/// +/// A purpose crafted to look like a row boundary followed by a plausible +/// job row does not produce a second row. Asserted by COUNTING the rows, +/// not by looking for the escape sequence: a renderer that dropped the +/// purpose entirely would satisfy "no forged row" while destroying the +/// feature, so the escaped text is asserted present in the surviving row +/// as well. +#[test] +fn a_purpose_shaped_like_a_row_boundary_does_not_produce_a_second_row() { + let mut state = editor(); + let (job_id, _token) = state.async_runtime.register_external( + JobKind::LspRequest, + None, + "lsp definition\n#99 grep 0ms forged \ + cancelled by nobody", + ); + exec(&state, "BUF = pmacs.workers.show()"); + let text: String = eval(&state, "return BUF:slice(0, BUF:len())"); + + let job_rows: Vec<&str> = text.lines().filter(|line| line.starts_with('#')).collect(); + assert_eq!( + job_rows.len(), + 1, + "one job must render as exactly ONE row:\n{text}" + ); + assert!( + job_rows[0].contains("lsp definition\\n#99"), + "and the break must be rendered, escaped, INSIDE that row:\n{text}" + ); + assert!( + !text.contains("\n#99"), + "no line may begin with the forged id:\n{text}" + ); + + // `#228`'s other half, and what makes this a rendering decision + // rather than data loss: the free-form surface still hands Lua every + // byte, unescaped. + let raw = active_purposes(&state); + assert!( + raw.iter().any(|purpose| purpose.contains('\n')), + "pmacs.workers.snapshot() is the raw path and must stay raw: {raw:?}" + ); + + exec(&state, "pmacs.workers.hide()"); + state.async_runtime.complete_external_cancelled(job_id); + pump(&mut state); +} + +/// **The modeline is one line, and that is enforced where the modeline +/// reads.** +/// +/// Two assertions, because they exclude different failures: the segment +/// carries no break at all (a composed modeline splicing one would +/// misplace every segment after it), and the escaped text survives the +/// real per-frame paint rather than only the evaluator. +#[test] +fn a_purpose_that_spans_lines_reaches_the_modeline_as_one_line() { + let mut state = editor(); + let (job_id, _token) = state.async_runtime.register_external( + JobKind::LspRequest, + None, + "lsp didOpen\nfile:///tmp/x.rs", + ); + + let segment = activity_segment(&state).expect("work is in flight, so a segment exists"); + assert!( + !segment.contains(['\n', '\r']), + "a modeline segment is ONE line: {segment:?}" + ); + assert_eq!( + segment, "⋯1 lsp didOpen\\nfile:///tmp/x.rs", + "and the break is escaped in place, not clipped away" + ); + + let cells = paint(&state, 24, 160); + let modeline = row_text(&cells, 160, 22); + assert!( + modeline.contains("⋯1 lsp didOpen\\nfile:///tmp/x.rs"), + "the painted modeline must carry it too; got {modeline:?}" + ); + + state.async_runtime.complete_external_cancelled(job_id); + pump(&mut state); +} + +/// **A purpose with no control characters is byte-identical after +/// escaping** — on both surfaces. +/// +/// The fixture is chosen to break a careless escaper: a literal +/// backslash (which a JSON-style escaper would double, and which is +/// deliberately NOT escaped here — no number of backslashes produces a +/// second row), a `\v` that is text rather than a vertical tab, quotes, +/// and a non-ASCII character. +#[test] +fn a_purpose_with_no_control_characters_is_unchanged_by_the_boundary() { + const PURPOSE: &str = r#"grep "fn \d+" in /tmp/pro—ject\v2"#; + + let mut state = editor(); + let (job_id, _token) = + state + .async_runtime + .register_external(JobKind::LspRequest, None, PURPOSE); + + exec(&state, "BUF = pmacs.workers.show()"); + let text: String = eval(&state, "return BUF:slice(0, BUF:len())"); + assert!( + text.contains(PURPOSE), + "the *workers* row must reproduce an ordinary purpose byte for byte:\n{text}" + ); + + assert_eq!( + activity_segment(&state).as_deref(), + Some(format!("⋯1 {PURPOSE}").as_str()), + "and so must the modeline segment" + ); + + exec(&state, "pmacs.workers.hide()"); + state.async_runtime.complete_external_cancelled(job_id); + pump(&mut state); +} From 028016333cb4c0de0f22a969eb7d7c23f5771913 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 19:41:57 +0200 Subject: [PATCH 11/20] docs: record review round 2, and R7's control finally discriminated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two files, no code. ## `docs/active-work.md` — review round 2 The lane's volatile block gains the round-2 record: the three findings, why P2a's fix is a mapped diagnostic and P2b's is escaping at presentation rather than rejection at the registry, and the seven new mutation checks. The gate outcome is recorded with its step counts and the two stop-signal facts — `journey_acceptance` 47/47 and all three `#pmacs.process.list()` leak detectors byte-identical to `main`. It also records what P2a's audit found and did NOT fix: `pmacs.process.spawn`'s other string fields still convert generically. That is pre-existing and out of this lane's diff, and it is named so it is not silently inherited by whoever reads the fixed `purpose` read and assumes the rest matches. ## `docs/ci-red-signatures.md` — R7's third occurrence, and U4 **R7 reproduced, and the control the second-occurrence note prescribed finally discriminated — against its own hypothesis.** Occurrence 2 left exactly one causal path open: the observing lane had added a GPU-heavy `render_offscreen` test to the same binary, and contention with a one-second socket handshake was plausible. That note prescribed the control to run if a third occurrence landed — with the added test removed, not at the merge base. A third occurrence landed, at the gate's `gpu` step, with all three fragments verified against the durable log. The control was run. **Ten full `-p pmacs-gpu` runs with the added test: 10/10 green. Ten with it `#[ignore]`d, nothing else changed: 1 failure in 10, all three fragments present.** Removing the suspect made the failure more frequent, so the concurrent-test path is excluded — no contention story from that test survives that direction. The more useful result is the rate. This is the first rerun in R7's history to reproduce anything at all, and it puts the failure at roughly 1-in-10 under ordinary `-p pmacs-gpu` load. Three sightings were not enough to bisect a handshake; 1-in-10 is. The row now says so, and tells the next agent to instrument which side closes the pipe rather than re-run for green. The lane is still not attributed — now for a measured reason rather than an argument from diff shape: the arm without the lane's only `pmacs-gpu` addition is the arm that went red. **U4** records the other two reds from that same gate run: `criterion_1_end_of_line_typing_completes_sub_frame_per_keystroke` and `composition_overhead_under_ten_percent`, both wall-clock budget assertions, failing together in `04-lib-crdt` and both green in isolation and in the next full run. Fragments captured, so unlike U1–U3 it is matchable — it is a `U` row for want of a mechanism, not for want of evidence. The signature named is **the pair**: two budget tests failing in one run and neither in the next is far more likely to be one loaded machine than two simultaneous regressions, and a future run that reds only one of them is a different incident. Neither row claims harmlessness, and the concurrent-worktree load confound is recorded as a rival explanation rather than as a finding, because it was not measured. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 63 +++++++++++++++++++++++++++++++++++++-- docs/ci-red-signatures.md | 63 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 121 insertions(+), 5 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 83f2701..5de19eb 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -358,8 +358,9 @@ the authoritative tip** — the ref, not a SHA. Recover with required field is exactly the kind of change that breaks a package fixture quietly. - **IMPLEMENTED at `1aca0ee`**, with review round 1's blocker fixed at - `2162737`. `tests/worker_identity_acceptance.rs` is the new suite: 18 - tests, plus one consumer-side witness beside the private renderer in + `2162737` and review round 2's three findings at `6661125`. + `tests/worker_identity_acceptance.rs` is the new suite: **24 tests**, + plus one consumer-side witness beside the private renderer in `pmacs-gpu`. - **`journey_acceptance` passed UNTOUCHED (47/47)** — the stop signal did not fire. Q#W-7 edits the `commit_to` guard family, so any of its @@ -426,6 +427,64 @@ the authoritative tip** — the ref, not a SHA. Recover with `pmacs.process.spawn("ls")` occurrences in `src/audit/mod.rs` and `tests/m7_9_acceptance.rs` are **audit fixture source text** — lexed, never executed — and are deliberately untouched. +- **Review round 2 — the display-text boundary, fixed at `6661125`.** + Three findings, and the fix is deliberately different in each place + because the constraint is. + - **P2a: invalid UTF-8 bypassed the `purpose` diagnostic.** + `required_purpose` read the field with `value.to_str()?`; Lua strings + are BYTE strings, so `purpose = string.char(255)` surfaced mlua's + generic conversion error before this lane's own message existed. It + refused before spawning, so nothing leaked — the defect was the + message. **Third occurrence of this class in the project** (the + destination-capture lane corrected the same shape two rounds ago), so + the whole diff was audited for it: exactly one more, + `_push_dispatch_name` taking `name: String`, now `mlua::String` with + an owned diagnostic. Those two are the only Lua-string reads this + lane added; every other binding it adds takes `()`. The remaining + `pmacs.process.spawn` fields (`label`, `command`, `args`, `env`, + `cwd`) still convert generically — **pre-existing, untouched, and + named here rather than silently inherited.** + - **P2b, half one: handler names are refused at the source.** + `pmacs.workers.register` type-checked and nothing more, which was + fine while the name died inside `dispatch`. It no longer dies there, + so the name now gets `purpose`'s meaningful-value standard plus + control characters. + - **P2b, half two: purposes are ESCAPED at presentation, not rejected + at the registry — consistent with the `#228` decision.** A purpose + may legitimately contain a newline (a path can; `pmacs-magit`'s spawn + purpose is an argv), so the one-line constraint belongs to the + surface that has one row. `purpose_for_one_row` states the property + it exists for — **a row must not be able to forge another row** — + escapes the Unicode `Cc` class (so ESC cannot open a terminal + sequence either), borrows unchanged when there is nothing to escape + (byte-identity is structural, not asserted), and does **not** escape + backslashes: no number of them makes a second row, and doubling them + would cost byte-identity for ordinary text. Two callers: the + `*workers*` rows and `ActivitySummary`, which exists for one consumer + with exactly one row. `pmacs.workers.snapshot()` is the + `describe-command` of this lane and stays raw — asserted, so a clip + that deleted the text everywhere would fail rather than pass. + - **P3: two stale recovery summaries**, both fixed section-locally — + the framing doc's "Implementation may proceed", and this file's claim + that Stage 1 took the "first two" of owner/purpose/parent. It takes + **one**: `owner` was removed in revision 2, and the claim that + argument overturned was still standing here. + - **Seven more mutation checks, each failing its own test and no + other** (30 for the lane): the two UTF-8 diagnostics, the two + register guards, the two escaping call sites, and + `purpose_for_one_row` neutered to the identity — which fails both + surfaces' tests and nothing else, since it is the shared helper. + - **All 13 gate steps green at `6661125`** (log + `20260809T173314Z-1552101`): lib 1920, lib-crdt 2105, + worker_identity 24, journey **47/47 UNTOUCHED**, statusline 7, + compile_mode 73, m8_6 12, m4 151, gpu 242. The three + `#pmacs.process.list()` leak detectors and `journey_acceptance` are + **byte-identical to `main`** in round 2 — the stop signals did not + fire, and round 2 edited no test outside its own suite. **The + preceding run of the same command was red on three tests and none of + them was this diff's** — R7 for the third time plus two wall-clock + budget tests; recorded in `docs/ci-red-signatures.md` rather than + re-run away silently. - **Surfaces that changed shape, for anyone rebasing onto this:** `AsyncRuntime::allocate`/`allocate_with_resource` collapsed into one private `JobSpec`-taking funnel; `register_external` grew a third diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index c94c73b..f54fb48 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -496,10 +496,10 @@ Stage 4; the lane touches no `pmacs-gpu` code at all. | **selector** | `-p pmacs-gpu attach::tests::managed_retry_survives_transients_and_uses_the_successful_stream` | | **job / flavor** | local (Linux), `cargo test --workspace --features crdt --no-fail-fast`, i.e. under full-sweep load | | **required fragments** | `transient sequence must attach` + `Handshake(Io(` + `BrokenPipe` (or `code: 32`) | -| **status** | **SECOND OCCURRENCE 2026-08-09 — causal status still UNRESOLVED** | -| **what IS established** | **two** occurrences at `pmacs-gpu/src/attach.rs:1680`, the second with all three fragments **verified** rather than inferred; the test drives a scripted transient-then-success sequence over a real socket pair | +| **status** | **THIRD OCCURRENCE 2026-08-09 — causal status still UNRESOLVED, but one candidate mechanism is now EXCLUDED** | +| **what IS established** | **three** occurrences at `pmacs-gpu/src/attach.rs:1680`, the second and third with all three fragments **verified** rather than inferred; the test drives a scripted transient-then-success sequence over a real socket pair. **The added GPU test is not the mechanism** — see the third-occurrence control below | | **what is NOT** | whether the broken pipe is the *fixture's* writer closing early or a real retry-path defect. **This row is not a claim that it is harmless** | -| **rerun evidence** | occurrence 1: 6 isolated runs green, plus a full `--workspace --features crdt` sweep green (113 targets). Occurrence 2: **30 green on the observing branch** (15 isolated selector, 15 full `-p pmacs-gpu`) **plus a 15-run merge-base control, also green**. Per the rerun rule all of this establishes **intermittence only** | +| **rerun evidence** | occurrence 1: 6 isolated runs green, plus a full `--workspace --features crdt` sweep green (113 targets). Occurrence 2: **30 green on the observing branch** (15 isolated selector, 15 full `-p pmacs-gpu`) **plus a 15-run merge-base control, also green**. Occurrence 3: 5 isolated selector runs green, 10 full `-p pmacs-gpu` runs green **with** the added test, and **1 failure in 10 with the added test `#[ignore]`d** — the first rerun in this row's history that reproduced anything. Per the rerun rule the green runs establish intermittence only; the red control run is what carries the exclusion | | **retirement** | hardening that removes the named mechanism plus a discriminating witness — or a diagnosis showing the fixture, not the code, closes the pipe | **Not attributed to the observing lane**, and in neither case is the @@ -537,6 +537,39 @@ one-second deadline. Contention is a plausible mechanism for a lands, **run the control with the added test removed** rather than at the merge base — that is the discriminating comparison this one was not. +**Third occurrence — worker identity Stage 1 review round 2, +2026-08-09, local (Linux). Same selector, same `gpu`-step flavor, all +three fragments verified** against the durable gate log +(`20260809T172606Z-1387979/11-gpu.log`): `transient sequence must +attach: Attach(Handshake(Io(Os { code: 32, kind: BrokenPipe, message: +"Broken pipe" })))`. A match on this file's own rule, not a `U` note. + +**The control the second-occurrence note prescribed was run, and this +time it discriminated — against the hypothesis.** Ten full +`PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu` runs with the added +`render_offscreen` test present: **10/10 green**. Ten more with that +test `#[ignore]`d, changing nothing else: **1 failure in 10**, carrying +all three required fragments +(`without/run-6.log`, `pmacs-gpu/src/attach.rs:1680`). + +So the concurrent-GPU-test path named above is **excluded**: removing +the suspect made the failure *more* frequent, not less, which no +contention story from that test survives. What the run does establish is +that **the failure reproduces on demand at roughly 1-in-10 under +ordinary `-p pmacs-gpu` load** — the first time any rerun in this row's +history has reproduced it at all. That is a materially better starting +point than three isolated sightings, and it is the fact a diagnosis +should be built on: the rate makes a bisect of `attach.rs`'s handshake +path affordable, where before it was not. + +**It is still not attributed to the observing lane**, and now for a +measured reason rather than an argument from diff shape: the arm without +the lane's only `pmacs-gpu` addition is the arm that went red. + +**What would retire it is unchanged** — the mechanism, not the rate. +The next agent to touch this row should reproduce at 1-in-10 and +instrument which side closes the pipe, rather than re-running for green. + ### U2 — `m6_1_pty_raw_mode_disables_kernel_echo`, one local occurrence Has a selector, which U1 lacks — but still no fragments, so it cannot @@ -582,6 +615,30 @@ it again here by piping a sweep through `grep`. The fix is mechanical: stream. A signature that is cheap to capture and impossible to reconstruct should never be traded for terminal brevity. +### U4 — two wall-clock budget tests fail together in one `lib-crdt` step + +Recorded during worker identity Stage 1 review round 2, 2026-08-09, in +the same gate run that produced R7's third occurrence. **Fragments were +captured**, so unlike U1–U3 this one is matchable — it is a `U` row +because it has one occurrence and no mechanism, not because the evidence +was lost. + +| field | value | +|---|---| +| **selector** | `--lib --features crdt optimistic::tests::criterion_1_end_of_line_typing_completes_sub_frame_per_keystroke` **and** `editor::tests::composition_overhead_under_ten_percent`, failing in the same run | +| **job / flavor** | local (Linux), `scripts/gate` step `04-lib-crdt`, with sibling worktrees building concurrently | +| **required fragments** | `criterion 1: per-keystroke orchestrator time` + `exceeds 1ms`; and `composition machinery added more than 10% overhead` | +| **status** | **new incident, one occurrence, not reproduced** | +| **what IS established** | both are **wall-clock budget assertions** — 1.264ms against a 1ms budget, and 1.297× against a 1.10× budget — so both are load-sensitive by construction. Both green in an isolated rerun of exactly those two selectors, and both green in the next full gate run of the same command (2105 passed) | +| **what is NOT** | whether the machine's concurrent load caused it. The confound is real (this machine runs one shared `CARGO_TARGET_DIR` and several worktrees) but **was not measured**, so it is a rival explanation, not a finding | +| **rival explanation not excluded** | a genuine regression in either path. Nothing in the observing diff touches the optimistic-echo orchestrator or the composition pipeline, but "my diff looks unrelated" is not evidence, and this row does not treat it as such | + +**Two budget tests failing in one run and neither in the next is the +signature worth matching**, more than either name alone: a real +regression in two unrelated subsystems at once is far less likely than +one loaded machine. If a future run reds **one** of these without the +other, that is a different incident and should be judged as one. + **The retirements are not occurrences and do not close the log.** R1 and R3 stay live, and each retired row keeps its signature so a later red matching one reopens it. From 31352692c86c2aff981abb02e0e73109f9b539bc Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 22:08:18 +0200 Subject: [PATCH 12/20] fix(process): name the surface a process purpose actually reaches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `required_purpose`'s invalid-UTF-8 refusal told the caller that their process purpose "is displayed to the user in *workers* and in the modeline". Neither is a process surface. Stage 1 deliberately keeps processes out of `*workers*` — which lists async JOBS — and out of the statusline activity indicator; a process's purpose is exposed through `pmacs.process.list` and nowhere else, and joining the two planes is Stage 2's work (framing §3, Q#W-4). The refusal is correct and stays: a purpose that cannot be displayed anywhere should still be refused, and nothing spawns either way. What was wrong is the reason given to the user, which pointed them at two places their process will never appear. A diagnostic that misdescribes the system is worse than a terse one, because it sends the reader looking in the wrong place. The job-side twin diverges rather than converging. `_push_dispatch_name` refuses a non-UTF-8 handler name for the same reason, and there `*workers*` and the modeline are the RIGHT answer — the name is composed into every job's purpose and a job renders in both. It said only "as part of every job's purpose", which names no surface at all, so it now names the two it reaches. The two messages must not collapse into one sentence: whichever wording won would be wrong on the other side. Verification. `the_two_utf8_refusals_each_name_the_surface_their_own_text_reaches` asserts both directions, positive AND negative — the process message contains `pmacs.process.list` and NOT `*workers*`/`modeline`, the job message contains both of those and NOT `pmacs.process.list`. The negative halves are the anti-collapse guard; without them a later "unify the wording" edit reintroduces exactly one wrong sentence and passes every other test in the file. The existing row-table assertion in `spawning_without_a_real_purpose_is_refused_and_starts_nothing` now runs as far as the surface name too, so the same edit breaks two tests. Three mutation checks, each red on its own claim: restoring the old process wording fails both content assertions; collapsing the job message onto the process wording fails only the new test (which is the point — the old job test asserted the prefix alone and could not see it); restoring the job message's original vague wording fails it too. The doc comments were fixed with the literals. `required_purpose`'s rustdoc now states which surface its message names and why it names neither of the other two, and the `_push_dispatch_name` comment states the converse. A corrected string whose doc comment still argues the other way is one refactor from reverting itself. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- src/lua_bindings/mod.rs | 27 +++++++++-- tests/worker_identity_acceptance.rs | 75 +++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 6 deletions(-) diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 406a3d6..1a9795a 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -7589,6 +7589,15 @@ pub fn install_async( // cannot see UTF-8 validity from Lua 5.1, so the byte-level half is // enforced here — the one point where Rust sees the name — with a // message that names both. + // + // And it names the surfaces a JOB reaches, which are `*workers*` and + // the modeline activity indicator. The sibling refusal in + // `required_purpose` deliberately names a different one + // (`pmacs.process.list`), because a spawned process reaches neither + // of these in Stage 1. The two must not converge on one sentence: + // whichever wording won would be wrong on the other side, and a + // diagnostic that misdescribes the system sends the reader looking + // in the wrong place. { let rt = runtime.clone(); async_mod.set( @@ -7597,7 +7606,9 @@ pub fn install_async( let Ok(text) = name.to_str() else { return Err(mlua::Error::external( "pmacs.workers.dispatch: handler name must be valid UTF-8 — it is \ - displayed to the user as part of every job's purpose.", + composed into every job's purpose, which is displayed to the user \ + in *workers* and in the modeline, and arbitrary bytes have no \ + display form there.", )); }; rt.push_dispatch_name(&*text); @@ -8795,6 +8806,16 @@ fn parse_restart(name: &str) -> mlua::Result { /// neither the field nor the rule — so the conversion failure is mapped /// onto this function's own message instead. /// +/// That message names **`pmacs.process.list`**, which is the whole of +/// where a process's purpose surfaces in Stage 1. It deliberately does +/// *not* name `*workers*` or the modeline indicator: both are **job** +/// surfaces, a spawned process appears in neither, and joining the two +/// planes is Stage 2's work (framing §3, Q#W-4). A diagnostic that +/// named them would send the reader looking for their process somewhere +/// it will never appear — worse than a terse one. The job-side twin of +/// this refusal, on `_push_dispatch_name`, names those two surfaces for +/// the matching reason: a job really does reach them. +/// /// The read is **raw**, matching the posture `stdin` and `group` already /// document in [`lua_to_spec`]: a spec table is plain data, so a /// metatable cannot smuggle a purpose in through `__index`. @@ -8805,8 +8826,8 @@ fn required_purpose(table: &Table) -> mlua::Result { Err(_) => { return Err(mlua::Error::external( "pmacs.process.spawn: purpose must be valid UTF-8 — it is displayed \ - to the user in *workers* and in the modeline, and arbitrary bytes \ - have no display form there.", + to the user in pmacs.process.list, and arbitrary bytes have no \ + display form there.", )); } }, diff --git a/tests/worker_identity_acceptance.rs b/tests/worker_identity_acceptance.rs index 54fc332..f5dcef7 100644 --- a/tests/worker_identity_acceptance.rs +++ b/tests/worker_identity_acceptance.rs @@ -290,8 +290,11 @@ fn every_entry_shape_records_what_its_work_is() { /// error before this lane's own diagnostic was ever constructed. The /// refusal is not the interesting part — it refuses either way, and /// nothing spawns either way — the MESSAGE is, which is why the -/// assertion is on content. Retyping this read as a bare `?` breaks -/// the row rather than silently degrading the error; +/// assertion is on content, and why the expected text now runs as far +/// as the **surface** the message names. Retyping this read as a bare +/// `?` breaks the row rather than silently degrading the error, and +/// naming the wrong surface breaks it too — see +/// `the_two_utf8_refusals_each_name_the_surface_their_own_text_reaches`; /// * **metatable-provided**, which is the `stdin`/`group` raw-read /// posture: a spec table is plain data, so a purpose cannot be /// smuggled in through `__index`. @@ -325,7 +328,8 @@ fn spawning_without_a_real_purpose_is_refused_and_starts_nothing() { "invalid UTF-8", r#"{ label = "x", purpose = "run " .. string.char(255), command = "/bin/sh", args = { "-c", "sleep 5" } }"#, - "purpose must be valid UTF-8", + "purpose must be valid UTF-8 — it is displayed to the user in \ + pmacs.process.list", ), ( "metatable-provided", @@ -534,6 +538,71 @@ fn a_handler_name_that_is_not_valid_utf8_is_refused_before_the_handler_runs() { pump(&mut state); } +/// **A diagnostic that names the wrong surface is worse than a terse +/// one, and the two UTF-8 refusals do not name the same surface.** +/// +/// Both messages tell the caller *why* their bytes are refused: the text +/// gets displayed, and arbitrary bytes have no display form. But the two +/// values reach **different** places, and Stage 1 makes that difference +/// deliberately: +/// +/// * a **job**'s purpose — which a handler name is composed into — is +/// rendered by `*workers*` and by the modeline activity indicator; +/// * a **process**'s purpose is exposed through `pmacs.process.list` +/// and nothing else. Processes are kept out of `*workers*` and out of +/// the indicator until Stage 2's unified view (framing §3, Q#W-4). +/// +/// So the process-side message must not send a caller to `*workers*` to +/// look for a process that will never be listed there, and the job-side +/// message must not send them to an accessor that enumerates no jobs. +/// **Both directions are asserted, positive and negative**, because a +/// later edit that "unified the wording" would otherwise reintroduce +/// exactly one wrong sentence in exactly one of the two places and pass +/// every other test in this file. +#[test] +fn the_two_utf8_refusals_each_name_the_surface_their_own_text_reaches() { + let mut state = editor(); + + let (spawned, process_err): (bool, String) = eval( + &state, + r#"local ok, err = pcall(pmacs.process.spawn, { + label = "x", purpose = "run " .. string.char(255), + command = "/bin/sh", args = { "-c", "sleep 5" } }) + return ok, tostring(err)"#, + ); + assert!(!spawned, "precondition: the spawn must refuse"); + assert!( + process_err.contains("pmacs.process.list"), + "a process purpose reaches pmacs.process.list, and the refusal must \ + say so; got {process_err:?}" + ); + assert!( + !process_err.contains("*workers*") && !process_err.contains("modeline"), + "and it must NOT name the job surfaces a process never reaches; \ + got {process_err:?}" + ); + + let (dispatched, job_err): (bool, String) = eval( + &state, + "pmacs.workers.register('bad' .. string.char(255), function() end) + local ok, err = pcall(pmacs.workers.dispatch, 'bad' .. string.char(255)) + return ok, tostring(err)", + ); + assert!(!dispatched, "precondition: the dispatch must refuse"); + assert!( + job_err.contains("*workers*") && job_err.contains("modeline"), + "a handler name reaches both job surfaces, and the refusal must name \ + them; got {job_err:?}" + ); + assert!( + !job_err.contains("pmacs.process.list"), + "and it must NOT name the process accessor, which enumerates no jobs; \ + got {job_err:?}" + ); + + pump(&mut state); +} + /// **Rule 7 + the defect itself.** A job dispatched through /// `pmacs.workers.dispatch("name", …)` reports `"name"`. /// From 1a00d8130d9ec53a5da14b9b10d49de5cb92be87 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 22:08:43 +0200 Subject: [PATCH 13/20] docs: record review round 3, and a sweep red that moves each run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lane entry gains round 3: the wrong-surface diagnostic, why the job and process refusals now say different things, the anti-collapse test and its three mutation checks. Written here rather than left in the commit message because this file is what a recovering agent reads. `docs/ci-red-signatures.md` gains **U5**. Two consecutive `scripts/gate` runs of the same command, on the same tree, red on step `12-sweep` with a DIFFERENT wall-clock render-budget test each time — 224ms and 258ms against a 200ms budget, 114ms against a 100ms budget, at load average 12.9/23.9 with sibling worktrees building. Each passes in an isolated rerun of its own selector, and no selector reds twice. The rotating selector is the signature, and it is a stronger one than any single test name: a regression that moved between three unrelated render paths on an unchanged tree is far less likely than one loaded machine. The observing diff is two string literals, their doc comments and one test, and touches no render path at all. Kept separate from U4 rather than merged. U4 is two budget tests in `04-lib-crdt` failing TOGETHER; this is three render-budget tests in `12-sweep` failing ONE PER RUN. Merging them would assert a shared mechanism nothing in hand shows, and the load confound stays unmeasured in both — a rival explanation, not a finding. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 45 +++++++++++++++++++++++++++++++++++++++ docs/ci-red-signatures.md | 22 +++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/docs/active-work.md b/docs/active-work.md index 5de19eb..731dfb1 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -485,6 +485,51 @@ the authoritative tip** — the ref, not a SHA. Recover with them was this diff's** — R7 for the third time plus two wall-clock budget tests; recorded in `docs/ci-red-signatures.md` rather than re-run away silently. +- **Review round 3 — a diagnostic that named the wrong surface, fixed + at `b2e8efd`.** `required_purpose`'s invalid-UTF-8 refusal told the + caller their process purpose "is displayed to the user in `*workers*` + and in the modeline". **Neither is a process surface.** Stage 1 + deliberately keeps processes out of both (Q#W-4, framing §3) — a + process's purpose is exposed through `pmacs.process.list` and nothing + else — so the message sent the reader looking for their process in two + places it will never appear. The refusal itself is correct and stays: + a purpose with no display form anywhere is still refused. + - **The two UTF-8 refusals now name different surfaces, because they + reach different ones.** The job-side twin (`_push_dispatch_name`) + legitimately names `*workers*` and the modeline — a handler name is + composed into a job's purpose, and a job does render in both — so it + was made to say so explicitly rather than left at the vaguer "as + part of every job's purpose", which named no surface at all and + would have made the divergence unassertable. + - **A new test asserts both directions, positive and negative** + (`the_two_utf8_refusals_each_name_the_surface_their_own_text_reaches`, + 26 in the suite): the process message contains `pmacs.process.list` + and **not** `*workers*`/`modeline`; the job message contains both of + those and **not** `pmacs.process.list`. The existing row-table + assertion in `spawning_without_a_real_purpose_is_refused_and_starts_nothing` + now runs as far as the surface name too. Without the negative half a + later "unify the wording" edit reintroduces exactly one wrong + sentence and passes everything else. + - **Three mutation checks, each red on its own claim:** restoring the + old process wording fails both content assertions; collapsing the + job message onto the process wording fails only the new test (which + is the point — the old job test asserted the prefix alone); and + restoring the job message's original vague wording fails it too. + - **The rustdoc carried the same defect risk and was fixed with it** — + `required_purpose` now states which surface it names and why not the + other two, and the `_push_dispatch_name` comment states the + converse. A string literal corrected while its doc comment still + argues the other way is one refactor from reverting itself. + - **Gate: 12 of 13 steps green; step `12-sweep` red on a wall-clock + render-budget test, twice, on a DIFFERENT test each run** (logs + `20260809T195332Z-2113672` and `20260809T200120Z-2427128`, load + average 12.9/23.9 with sibling lanes building). All three pass in + isolated reruns; the diff is two string literals, their doc comments + and one test, and touches no render path. Recorded as **U5** in + `docs/ci-red-signatures.md` rather than re-run away silently. + `journey_acceptance` **47/47 UNTOUCHED** and the three + `#pmacs.process.list()` leak detectors unedited — the stop signals + did not fire. - **Surfaces that changed shape, for anyone rebasing onto this:** `AsyncRuntime::allocate`/`allocate_with_resource` collapsed into one private `JobSpec`-taking funnel; `register_external` grew a third diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index f54fb48..e95fd2b 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -639,6 +639,28 @@ regression in two unrelated subsystems at once is far less likely than one loaded machine. If a future run reds **one** of these without the other, that is a different incident and should be judged as one. +### U5 — a *different* wall-clock render-budget test reds each sweep + +Recorded during worker identity Stage 1 review round 3, 2026-08-09. +**Two consecutive `scripts/gate` runs of the same command, on the same +tree, red on step `12-sweep` with a different test each time** — which +is the signature, and it is a stronger one than any single selector. + +| field | value | +|---|---| +| **selector** | run 1: `--test m8_2_acceptance dired_open_renders_10k_entries_under_200ms` **and** `--test m8_9_acceptance outline_5_level_100_entry_renders_within_100ms`; run 2: `--test dired_acceptance dired_renders_10k_entries_within_200ms` | +| **job / flavor** | local (Linux), `scripts/gate` step `12-sweep` (`cargo test --workspace --no-fail-fast`), **load average 12.9 / 23.9** with sibling worktrees building concurrently | +| **required fragments** | `must render within 200ms; took ` / `open() (parse + render) took ` + `spec budget is 100ms` | +| **status** | **new incident, three selectors, none reproduced** | +| **what IS established** | all three are **wall-clock render-budget assertions** (224ms and 258ms against a 200ms budget; 114ms against a 100ms budget), so all three are load-sensitive by construction. Each was green in an isolated rerun of its own selector, and no selector reds twice. The observing diff is **two string literals, their doc comments and one test** — it touches no render path at all, and cannot | +| **what is NOT** | that load caused it. The one-shared-`CARGO_TARGET_DIR` confound is real and again **unmeasured**, so it stays a rival explanation rather than a finding | +| **relation to U4** | same shape, different step and different tests: U4 is two budget tests in `04-lib-crdt` failing **together**; this is three render-budget tests in `12-sweep` failing **one per run**. Kept separate rather than merged, because merging would assert a shared mechanism nothing here shows | + +**The rotating selector is the thing to match.** A regression that +moved between three unrelated render paths on an unchanged tree is far +less likely than one loaded machine; a future run that reds the *same* +one of these twice is a different incident and should be judged as one. + **The retirements are not occurrences and do not close the log.** R1 and R3 stay live, and each retired row keeps its signature so a later red matching one reopens it. From 3570e1ad42013a2729e940b0d893d27a2274cca4 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 22:14:15 +0200 Subject: [PATCH 14/20] docs: the third gate run is green, and U5 says so MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two reds recorded a moment ago were followed by a full green run of the same command on the same tree — all 13 steps, log `20260809T200907Z-2672209`. Both the lane entry and U5 now carry that, because a signature row that records only the reds overstates them: the green rerun is part of the evidence, not a reason to delete the row. The row stays live and stays U-classified. Three load-sensitive render budgets going red one per run and then green is consistent with a loaded machine and with nothing else in hand; it is not a measurement of one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 14 ++++++++------ docs/ci-red-signatures.md | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 731dfb1..5a5e58c 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -520,12 +520,14 @@ the authoritative tip** — the ref, not a SHA. Recover with other two, and the `_push_dispatch_name` comment states the converse. A string literal corrected while its doc comment still argues the other way is one refactor from reverting itself. - - **Gate: 12 of 13 steps green; step `12-sweep` red on a wall-clock - render-budget test, twice, on a DIFFERENT test each run** (logs - `20260809T195332Z-2113672` and `20260809T200120Z-2427128`, load - average 12.9/23.9 with sibling lanes building). All three pass in - isolated reruns; the diff is two string literals, their doc comments - and one test, and touches no render path. Recorded as **U5** in + - **Gate: all 13 steps green at `cb7730d`** (log + `20260809T200907Z-2672209`). **The two preceding runs of the same + command were red on step `12-sweep`, on a DIFFERENT wall-clock + render-budget test each time** (`20260809T195332Z-2113672`, + `20260809T200120Z-2427128`; load average 12.9/23.9 with sibling + lanes building). All three pass in isolated reruns, none reds twice, + and the diff is two string literals, their doc comments and one + test — no render path is touched. Recorded as **U5** in `docs/ci-red-signatures.md` rather than re-run away silently. `journey_acceptance` **47/47 UNTOUCHED** and the three `#pmacs.process.list()` leak detectors unedited — the stop signals diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index e95fd2b..94b315b 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -652,7 +652,7 @@ is the signature, and it is a stronger one than any single selector. | **job / flavor** | local (Linux), `scripts/gate` step `12-sweep` (`cargo test --workspace --no-fail-fast`), **load average 12.9 / 23.9** with sibling worktrees building concurrently | | **required fragments** | `must render within 200ms; took ` / `open() (parse + render) took ` + `spec budget is 100ms` | | **status** | **new incident, three selectors, none reproduced** | -| **what IS established** | all three are **wall-clock render-budget assertions** (224ms and 258ms against a 200ms budget; 114ms against a 100ms budget), so all three are load-sensitive by construction. Each was green in an isolated rerun of its own selector, and no selector reds twice. The observing diff is **two string literals, their doc comments and one test** — it touches no render path at all, and cannot | +| **what IS established** | all three are **wall-clock render-budget assertions** (224ms and 258ms against a 200ms budget; 114ms against a 100ms budget), so all three are load-sensitive by construction. Each was green in an isolated rerun of its own selector, no selector reds twice, and **the third run of the same command on the same tree was green on all 13 steps** (log `20260809T200907Z-2672209`). The observing diff is **two string literals, their doc comments and one test** — it touches no render path at all, and cannot | | **what is NOT** | that load caused it. The one-shared-`CARGO_TARGET_DIR` confound is real and again **unmeasured**, so it stays a rival explanation rather than a finding | | **relation to U4** | same shape, different step and different tests: U4 is two budget tests in `04-lib-crdt` failing **together**; this is three render-budget tests in `12-sweep` failing **one per run**. Kept separate rather than merged, because merging would assert a shared mechanism nothing here shows | From 80b3dcf8973caaa431dd35966dc6ee709f670995 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 22:29:20 +0200 Subject: [PATCH 15/20] docs: the suite is 25, not 26 The previous round left the suite at 24 and this round adds one test, so it is 25. grep -c on the test attribute confirms 25 and the run reports 25/25. The bullet said 26, and I repeated it upstream without counting. The entry now shows the arithmetic --- 24 before, plus one --- rather than just a corrected number, and names the earlier figure so a reader who saw it does not treat this as a second suite. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/active-work.md b/docs/active-work.md index 5a5e58c..8fb9121 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -503,7 +503,9 @@ the authoritative tip** — the ref, not a SHA. Recover with would have made the divergence unassertable. - **A new test asserts both directions, positive and negative** (`the_two_utf8_refusals_each_name_the_surface_their_own_text_reaches`, - 26 in the suite): the process message contains `pmacs.process.list` + 25 in the suite — 24 before this round, plus this one; an earlier + revision of this bullet said 26): the process message contains + `pmacs.process.list` and **not** `*workers*`/`modeline`; the job message contains both of those and **not** `pmacs.process.list`. The existing row-table assertion in `spawning_without_a_real_purpose_is_refused_and_starts_nothing` From 7675142d4f47efebdb8ff2deca19797421b93b22 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 22:40:54 +0200 Subject: [PATCH 16/20] docs: U2 has a second occurrence, and this time the fragments were read The worker-identity tip gate went red on step 03-lib with one failure: m6_1_pty_raw_mode_disables_kernel_echo. U2 already had that exact selector but no fragments, so it could not be matched. Reading the durable gate log rather than filtering a rerun supplies them. The fragment reframes the failure. stty -a returned the EMPTY STRING, not a wrong mode --- so this is not raw mode failing to disable echo, it is stty producing no output at all, which points at PTY or spawn readiness under load rather than termios handling. The assertion own message is misleading on exactly that point, and anyone diagnosing it from the message will look in the wrong place. Occurrence 2 also EXCLUDES the change under test, which occurrence 1 could not. The tree carried zero code change since a 13/13 green run on this same lane --- the only delta was three lines of markdown. A docs edit cannot break a PTY test, so the diff is ruled out as a cause rather than merely doubted. Isolated rerun passes in 0.01s. Still no mechanism, and the row says so. Two occurrences establish intermittence and a load correlation; neither establishes cause. The row now names the discriminating control for a third: loop the selector under synthetic load logging stty output every iteration, since whether stty is empty EVERY time it fails is what separates a readiness race from a termios one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/ci-red-signatures.md | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index 94b315b..e85ad72 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -570,19 +570,24 @@ the lane's only `pmacs-gpu` addition is the arm that went red. The next agent to touch this row should reproduce at 1-in-10 and instrument which side closes the pipe, rather than re-running for green. -### U2 — `m6_1_pty_raw_mode_disables_kernel_echo`, one local occurrence +### U2 — `m6_1_pty_raw_mode_disables_kernel_echo`, SECOND occurrence; fragments captured -Has a selector, which U1 lacks — but still no fragments, so it cannot -be matched either. Recorded so a recurrence is recognisable. +Recorded first with a selector but **no fragments**, so it could not be +matched. The second occurrence (2026-08-09, worker-identity lane) was +read from the durable gate log rather than a filtered rerun, so the row +is now matchable — and the fragment changes what the failure looks +like. | field | value | |---|---| | **selector** | `--lib process::tests::m6_1_pty_raw_mode_disables_kernel_echo` | | **job / flavor** | local (Linux), during `cargo test --tests --no-fail-fast` — the lib target alongside a full PTY-heavy corpus | -| **required fragments** | **none captured** — output was filtered to the `FAILED` line | -| **status** | **new incident, unreproduced** | -| **what IS established** | it failed once (`1916 passed; 1 failed`), in no registry row, under a full-corpus run | -| **what is NOT** | any mechanism. Not reproduced in a later full `--tests --no-fail-fast` sweep (108 targets, exit 0) nor in 3 isolated `--lib` runs (1917/0 each) | +| **required fragments** | `panicked at src/process.rs:3953` · `raw mode should disable echo; stty -a output was: ""` — **captured on occurrence 2** | +| **status** | **two occurrences, load-correlated; the diff is EXCLUDED on occurrence 2** | +| **what IS established** | Occurrence 1: failed once (`1916 passed; 1 failed`) under a full-corpus run. Occurrence 2: `1919 passed; 1 failed` in `scripts/gate` step `03-lib` at load ~21, and **the tree contained ZERO code change since a 13/13 green run on the same lane** — the only delta was three lines of `docs/active-work.md`. A markdown edit cannot break a PTY test, so the change under test is ruled out as a cause rather than merely doubted. Passes isolated (`1 passed`, 0.01s) | +| **what the fragment REFRAMES** | `stty -a` returned the **empty string**, not a wrong mode. So this is not "raw mode failed to disable echo" — it is `stty` producing **no output at all**, which points at PTY/spawn readiness under load rather than at termios handling. The assertion's own message is misleading on this point, and anyone diagnosing it from the message alone will look in the wrong place | +| **what is NOT** | any mechanism, still. Not reproduced in a later full sweep (108 targets, exit 0), nor in 3 isolated `--lib` runs (1917/0 each), nor in the isolated rerun after occurrence 2. Two occurrences establish intermittence and a load correlation; neither establishes cause | +| **discriminating control if a third lands** | run the selector in a loop under synthetic load with `stty -a` output logged on every iteration — the open question is whether `stty` is empty *every* time it fails, which would separate a readiness race from a termios one | | **rival explanation not excluded** | leaked `pmacs --daemon` processes, which the handoff names as a standing confound for any load-sensitive local red | ### U3 — the R7 selector again, fragments lost the same way U2's were From 71d262e08d9793ac0addc8be8da951b484f7ff40 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 22:44:46 +0200 Subject: [PATCH 17/20] docs: U2 was already known, and my fragment claim overreached Two corrections, both mine, both the same failure the row exists to warn about. First, this is at least the THIRD occurrence, not the second, and the fragment was not newly captured. docs/active-work.md records a 2026-08-06 loaded --features crdt run failing this selector AND m6_1_pty_canonical_mode_keeps_kernel_echo with the same stty -a output was: "" --- and it already proposed a mechanism family, read-before- write on the child output, the shape of R4 and R6. So the row claim that no mechanism had been proposed was false of the tree it was written in. The evidence was in this repository the whole time; I wrote a registry row without reading the registry neighbour. Second, the fragment does not show what I said it showed. The test inspects collect_stdout(&evs) after drain_until --- what the SUPERVISOR collected. It cannot distinguish stty never writing from the PTY dropping the bytes from event collection missing them. I wrote "stty produced no output at all", which asserts a mechanism the test cannot see, in the same row that says no mechanism is established. What survives is narrower and still worth having: this is not a termios failure, since nothing observed shows echo configured wrongly. Which of child-never-wrote, delivery-lost, collection-missed is open. The control changes accordingly. Sampling the collected string more times cannot separate those three however often it fails; the next occurrence needs the full process event stream and the child exit disposition captured, cross-checked against the R4/R6 readiness family that the 2026-08-06 entry already implicates. The gate conclusion is unaffected: a markdown-only delta cannot cause a PTY failure, so worker identity code is excluded as a cause. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/ci-red-signatures.md | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index e85ad72..0162573 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -570,24 +570,34 @@ the lane's only `pmacs-gpu` addition is the arm that went red. The next agent to touch this row should reproduce at 1-in-10 and instrument which side closes the pipe, rather than re-running for green. -### U2 — `m6_1_pty_raw_mode_disables_kernel_echo`, SECOND occurrence; fragments captured +### U2 — `m6_1_pty_raw_mode_disables_kernel_echo`, THIRD known occurrence -Recorded first with a selector but **no fragments**, so it could not be -matched. The second occurrence (2026-08-09, worker-identity lane) was -read from the durable gate log rather than a filtered rerun, so the row -is now matchable — and the fragment changes what the failure looks -like. +**Corrected 2026-08-09 after review.** A previous edit of this row +called the 2026-08-09 failure the *second* occurrence and claimed it +captured the fragment for the first time. **Both were wrong**, and the +evidence was already in this repository: +`docs/active-work.md` records a **2026-08-06** loaded `--features crdt` +run failing this selector *and* `m6_1_pty_canonical_mode_keeps_kernel_echo` +with the same `stty -a output was: ""`, and it already proposed a +mechanism family — **read-before-write on the child's output**, the +shape of **R4** (readiness predicate satisfied by an empty file) and +**R6** (readiness file never published). + +So the fragment was captured before, under another feature flavor, and +this row's earlier "no mechanism has been proposed" was false of the +tree it was written in. | field | value | |---|---| | **selector** | `--lib process::tests::m6_1_pty_raw_mode_disables_kernel_echo` | | **job / flavor** | local (Linux), during `cargo test --tests --no-fail-fast` — the lib target alongside a full PTY-heavy corpus | -| **required fragments** | `panicked at src/process.rs:3953` · `raw mode should disable echo; stty -a output was: ""` — **captured on occurrence 2** | -| **status** | **two occurrences, load-correlated; the diff is EXCLUDED on occurrence 2** | +| **required fragments** | `panicked at src/process.rs:3953` · `raw mode should disable echo; stty -a output was: ""` | +| **status** | **at least three occurrences, load-correlated; the diff is EXCLUDED on the 2026-08-09 one** | | **what IS established** | Occurrence 1: failed once (`1916 passed; 1 failed`) under a full-corpus run. Occurrence 2: `1919 passed; 1 failed` in `scripts/gate` step `03-lib` at load ~21, and **the tree contained ZERO code change since a 13/13 green run on the same lane** — the only delta was three lines of `docs/active-work.md`. A markdown edit cannot break a PTY test, so the change under test is ruled out as a cause rather than merely doubted. Passes isolated (`1 passed`, 0.01s) | -| **what the fragment REFRAMES** | `stty -a` returned the **empty string**, not a wrong mode. So this is not "raw mode failed to disable echo" — it is `stty` producing **no output at all**, which points at PTY/spawn readiness under load rather than at termios handling. The assertion's own message is misleading on this point, and anyone diagnosing it from the message alone will look in the wrong place | +| **what the fragment ACTUALLY shows** | **The supervisor collected empty stdout** — `drain_until` then `collect_stdout(&evs)` (`src/process.rs:3948-3951`); the assertion inspects that string. It does **NOT** establish that `stty` emitted nothing: the bytes could have been lost in PTY delivery or in event collection. An earlier edit of this row said "`stty` produced no output at all", which asserts a mechanism the test cannot see. What is true is narrower and still useful: this is not a *termios* failure — nothing shows echo being configured wrongly — but which of {child never wrote, PTY dropped it, collection missed it} is open. The assertion's message invites the wrong reading, since it prints an empty string as though it were `stty`'s answer | | **what is NOT** | any mechanism, still. Not reproduced in a later full sweep (108 targets, exit 0), nor in 3 isolated `--lib` runs (1917/0 each), nor in the isolated rerun after occurrence 2. Two occurrences establish intermittence and a load correlation; neither establishes cause | -| **discriminating control if a third lands** | run the selector in a loop under synthetic load with `stty -a` output logged on every iteration — the open question is whether `stty` is empty *every* time it fails, which would separate a readiness race from a termios one | +| **discriminating control for the next occurrence** | capture the **full process event stream and the child's exit disposition**, not only the collected string — that is what separates "child never wrote" from "delivery or collection lost it", and the collected string cannot distinguish them however many times it is sampled. Cross-check against R4/R6's readiness family, which `docs/active-work.md`'s 2026-08-06 entry already implicates | +| **cross-reference** | `docs/active-work.md` — 2026-08-06 occurrence, `--features crdt`, **both** the raw and canonical selectors, same fragment, read-before-write hypothesis | | **rival explanation not excluded** | leaked `pmacs --daemon` processes, which the handoff names as a standing confound for any load-sensitive local red | ### U3 — the R7 selector again, fragments lost the same way U2's were From 68c6d6732c493a59b8eafef9a68345e977da0f36 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 9 Aug 2026 22:56:47 +0200 Subject: [PATCH 18/20] docs: U2 enumerates three occurrences, not two The header and the mechanism boundary were corrected last round; the TABLE still numbered two. It called the 2026-08-09 worker run "Occurrence 2", omitted the 2026-08-06 CRDT occurrence from the enumeration entirely, and concluded "Two occurrences establish intermittence" --- in the row I had just rewritten because it omitted that same occurrence. That is the head-and-body split this session keeps reproducing, this time inside a single table, in the row whose whole purpose is to be the authoritative account of what is known. The row now enumerates all three, and says which one carries the most weight: the 2026-08-06 CRDT run, because it shows the failure is not confined to one feature flavor and can take the raw and canonical selectors at once. That is a fact neither of the other two supplies. Also corrected: "what is NOT: any mechanism, still" is now "no mechanism is ESTABLISHED", because one IS proposed --- read-before- write on the child output, the R4/R6 readiness family. Proposed is not confirmed, and the row says so rather than flattening the distinction in either direction. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/ci-red-signatures.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index 0162573..0f5cc34 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -593,9 +593,9 @@ tree it was written in. | **job / flavor** | local (Linux), during `cargo test --tests --no-fail-fast` — the lib target alongside a full PTY-heavy corpus | | **required fragments** | `panicked at src/process.rs:3953` · `raw mode should disable echo; stty -a output was: ""` | | **status** | **at least three occurrences, load-correlated; the diff is EXCLUDED on the 2026-08-09 one** | -| **what IS established** | Occurrence 1: failed once (`1916 passed; 1 failed`) under a full-corpus run. Occurrence 2: `1919 passed; 1 failed` in `scripts/gate` step `03-lib` at load ~21, and **the tree contained ZERO code change since a 13/13 green run on the same lane** — the only delta was three lines of `docs/active-work.md`. A markdown edit cannot break a PTY test, so the change under test is ruled out as a cause rather than merely doubted. Passes isolated (`1 passed`, 0.01s) | +| **what IS established** | **Three occurrences.** **(1)** the original: failed once (`1916 passed; 1 failed`) under a full-corpus `--tests --no-fail-fast` run, fragments not captured. **(2) 2026-08-06**, loaded `--features crdt`: this selector **and** `m6_1_pty_canonical_mode_keeps_kernel_echo` both failed with the same `stty -a output was: ""` — the first capture, and the occurrence that proposed the read-before-write family. **(3) 2026-08-09**, worker-identity tip: `1919 passed; 1 failed` in `scripts/gate` step `03-lib` at load ~21, and **the tree contained ZERO code change since a 13/13 green run on the same lane** — the only delta was three lines of `docs/active-work.md`. A markdown edit cannot break a PTY test, so the change under test is ruled out as a cause rather than merely doubted. Passes isolated (`1 passed`, 0.01s). **Occurrence 2 is the one that matters most**: it shows the failure is not confined to one feature flavor and can take both selectors at once | | **what the fragment ACTUALLY shows** | **The supervisor collected empty stdout** — `drain_until` then `collect_stdout(&evs)` (`src/process.rs:3948-3951`); the assertion inspects that string. It does **NOT** establish that `stty` emitted nothing: the bytes could have been lost in PTY delivery or in event collection. An earlier edit of this row said "`stty` produced no output at all", which asserts a mechanism the test cannot see. What is true is narrower and still useful: this is not a *termios* failure — nothing shows echo being configured wrongly — but which of {child never wrote, PTY dropped it, collection missed it} is open. The assertion's message invites the wrong reading, since it prints an empty string as though it were `stty`'s answer | -| **what is NOT** | any mechanism, still. Not reproduced in a later full sweep (108 targets, exit 0), nor in 3 isolated `--lib` runs (1917/0 each), nor in the isolated rerun after occurrence 2. Two occurrences establish intermittence and a load correlation; neither establishes cause | +| **what is NOT** | **No mechanism is ESTABLISHED** — one is *proposed*: read-before-write on the child's output, the R4/R6 readiness family (occurrence 2). Proposed is not confirmed, and nothing here discriminates it from PTY delivery or event-collection loss. Not reproduced in a later full sweep (108 targets, exit 0), nor in 3 isolated `--lib` runs (1917/0 each), nor in the isolated rerun after occurrence 3. **Three occurrences establish intermittence and a load correlation; none establishes cause** | | **discriminating control for the next occurrence** | capture the **full process event stream and the child's exit disposition**, not only the collected string — that is what separates "child never wrote" from "delivery or collection lost it", and the collected string cannot distinguish them however many times it is sampled. Cross-check against R4/R6's readiness family, which `docs/active-work.md`'s 2026-08-06 entry already implicates | | **cross-reference** | `docs/active-work.md` — 2026-08-06 occurrence, `--features crdt`, **both** the raw and canonical selectors, same fragment, read-before-write hypothesis | | **rival explanation not excluded** | leaked `pmacs --daemon` processes, which the handoff names as a standing confound for any load-sensitive local red | From 0449d67a7f00385fb8b2d9e267d2b91ec82737d7 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Mon, 10 Aug 2026 00:02:31 +0200 Subject: [PATCH 19/20] docs: warn that this lane U4/U5 must become U6/U7 on rebase gate-protocol-build independently defines its own U4 and U5 --- a macOS lua54 PTY-resize failure and a Ctrl-C-as-SIGINT failure --- and it merges first, so on main those ids are taken. A rebase that resolves the textual conflict without renumbering leaves two different incidents sharing an id, which is precisely the failure this file matching rule exists to prevent. The registry authority rests on ids meaning one thing. The warning enumerates all four sites rather than saying "renumber the rows", because one of them is a prose cross-reference inside U5 relation-to-U4 field and another is in active-work.md --- both easy to miss when the conflict presenting itself is two adjacent headings. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/ci-red-signatures.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index 0f5cc34..51f642d 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -630,6 +630,22 @@ it again here by piping a sweep through `grep`. The fix is mechanical: stream. A signature that is cheap to capture and impossible to reconstruct should never be traded for terminal brevity. +> [!IMPORTANT] +> **RENUMBER THESE TO U6 AND U7 WHEN THIS LANE REBASES.** The +> `gate-protocol-build` lane independently defines its own **U4** and +> **U5** (a macOS `lua54` PTY-resize failure and a Ctrl-C-as-`SIGINT` +> failure), and it merges **first** — so on `main` those ids are taken. +> +> A rebase that resolves the textual conflict without renumbering leaves +> **two different incidents sharing an id**, which is exactly the +> failure this file's matching rule exists to prevent. +> +> **Every site, so none is missed:** +> 1. this heading (`### U4 …`) +> 2. `### U5 …` below +> 3. U5's **relation to U4** field, which names it in prose +> 4. `docs/active-work.md` — "Recorded as **U5**" in the worker lane entry + ### U4 — two wall-clock budget tests fail together in one `lib-crdt` step Recorded during worker identity Stage 1 review round 2, 2026-08-09, in From 6983496b747d772e6a3d2a6059be257b73be3062 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Mon, 10 Aug 2026 14:52:49 +0200 Subject: [PATCH 20/20] docs: renumber this lane U4/U5 to U6/U7 --- and git did not warn The pre-rebase warning was right, and the mechanism is worth recording because it is the quiet kind. gate-protocol-build landed its own U4 and U5 in #229. On this rebase git merged docs/ci-red-signatures.md WITHOUT A CONFLICT --- the two lanes appended their rows in different places, so there was nothing textual to resolve --- and produced two ### U4 and two ### U5 headings describing entirely different incidents. No marker, no complaint. That is the failure the matching rule exists to prevent, arriving through the one path a careful conflict resolution would never catch: there was no conflict to resolve. Renumbered across all four sites the warning enumerated: both headings, the prose relation-to-U4 field inside what is now U7, and the active-work.md reference. Ids verified unique afterwards rather than assumed. The warning block itself is retired in place, replaced by a note saying what was done and why, so the next reader sees a completed action rather than an outstanding one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 2 +- docs/ci-red-signatures.md | 22 ++++------------------ 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 8fb9121..7262ab5 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -529,7 +529,7 @@ the authoritative tip** — the ref, not a SHA. Recover with `20260809T200120Z-2427128`; load average 12.9/23.9 with sibling lanes building). All three pass in isolated reruns, none reds twice, and the diff is two string literals, their doc comments and one - test — no render path is touched. Recorded as **U5** in + test — no render path is touched. Recorded as **U7** in `docs/ci-red-signatures.md` rather than re-run away silently. `journey_acceptance` **47/47 UNTOUCHED** and the three `#pmacs.process.list()` leak detectors unedited — the stop signals diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index 51f642d..8ef65c7 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -630,23 +630,9 @@ it again here by piping a sweep through `grep`. The fix is mechanical: stream. A signature that is cheap to capture and impossible to reconstruct should never be traded for terminal brevity. -> [!IMPORTANT] -> **RENUMBER THESE TO U6 AND U7 WHEN THIS LANE REBASES.** The -> `gate-protocol-build` lane independently defines its own **U4** and -> **U5** (a macOS `lua54` PTY-resize failure and a Ctrl-C-as-`SIGINT` -> failure), and it merges **first** — so on `main` those ids are taken. -> -> A rebase that resolves the textual conflict without renumbering leaves -> **two different incidents sharing an id**, which is exactly the -> failure this file's matching rule exists to prevent. -> -> **Every site, so none is missed:** -> 1. this heading (`### U4 …`) -> 2. `### U5 …` below -> 3. U5's **relation to U4** field, which names it in prose -> 4. `docs/active-work.md` — "Recorded as **U5**" in the worker lane entry +*(Renumbered from U4/U5 to **U6/U7** on the rebase onto `0857bf4`: `gate-protocol-build` landed its own U4/U5 in #229, and git merged both files **without a conflict**, producing duplicate ids across four sites. The pre-rebase warning is retired here because it has been carried out.)* -### U4 — two wall-clock budget tests fail together in one `lib-crdt` step +### U6 — two wall-clock budget tests fail together in one `lib-crdt` step Recorded during worker identity Stage 1 review round 2, 2026-08-09, in the same gate run that produced R7's third occurrence. **Fragments were @@ -670,7 +656,7 @@ regression in two unrelated subsystems at once is far less likely than one loaded machine. If a future run reds **one** of these without the other, that is a different incident and should be judged as one. -### U5 — a *different* wall-clock render-budget test reds each sweep +### U7 — a *different* wall-clock render-budget test reds each sweep Recorded during worker identity Stage 1 review round 3, 2026-08-09. **Two consecutive `scripts/gate` runs of the same command, on the same @@ -685,7 +671,7 @@ is the signature, and it is a stronger one than any single selector. | **status** | **new incident, three selectors, none reproduced** | | **what IS established** | all three are **wall-clock render-budget assertions** (224ms and 258ms against a 200ms budget; 114ms against a 100ms budget), so all three are load-sensitive by construction. Each was green in an isolated rerun of its own selector, no selector reds twice, and **the third run of the same command on the same tree was green on all 13 steps** (log `20260809T200907Z-2672209`). The observing diff is **two string literals, their doc comments and one test** — it touches no render path at all, and cannot | | **what is NOT** | that load caused it. The one-shared-`CARGO_TARGET_DIR` confound is real and again **unmeasured**, so it stays a rival explanation rather than a finding | -| **relation to U4** | same shape, different step and different tests: U4 is two budget tests in `04-lib-crdt` failing **together**; this is three render-budget tests in `12-sweep` failing **one per run**. Kept separate rather than merged, because merging would assert a shared mechanism nothing here shows | +| **relation to U6** | same shape, different step and different tests: U6 is two budget tests in `04-lib-crdt` failing **together**; this is three render-budget tests in `12-sweep` failing **one per run**. Kept separate rather than merged, because merging would assert a shared mechanism nothing here shows | **The rotating selector is the thing to match.** A regression that moved between three unrelated render paths on an unchanged tree is far