diff --git a/docs/active-work.md b/docs/active-work.md index 5d09e2a..3270b11 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -268,10 +268,11 @@ waits for a signal that is not coming. Stage 1's framing for **all** slices and governs the later branches; only Stage 0 was framed by the arc document itself. - **Scope of THIS branch: 1-pre only — the input seam. No behaviour - change.** `App::window_event` was **655 lines** and is now **33**: one - `route_event` call and one arm per route. Nothing below it could be - witnessed without a display, which is why the seam precedes every - other slice. + change.** `App::window_event` was **655 lines** and is now **four** — + call `dispatch_window_event`, exit if it asks. The dispatch it calls + is one `route_event` call and one arm per route. Nothing below it + could be witnessed without a display, which is why the seam precedes + every other slice. - **IMPLEMENTED in four commits, one per event family**, so each lands with its own witnesses and mutations rather than as one 600-line diff: `014110f` lifecycle (close / modifiers / resize), `7f0f9db` @@ -321,22 +322,26 @@ waits for a signal that is not coming. free function composing one decision function per family (`route_lifecycle`, `route_keyboard` + `route_key_action`, `route_pointer`). Performing stays on `App` in seven `apply_*` - methods. **A route names its LOCAL EFFECT**, not merely the family - that claims it, and the harness records routes — because - `CloseRequested` and `RedrawRequested` send the daemon nothing, so a - transcript of protocol traffic could not tell a handled arm from a - dropped one. + methods. A route carries the **decision**, not the effect: a wheel + route holds a delta, and whether that becomes a viewport update, a + panel event, a terminal event or nothing depends on `State`. Effects + are witnessed separately — see the P2 bullet below. - **Every moved body verified as the original, mechanically.** The keyboard body is byte-identical modulo two named conversions; the four pointer bodies were checked by re-running rustfmt on the pre-move text at the new indent level and diffing, since de-indenting by 8 columns - lets rustfmt rejoin lines. **13 witnesses, 17 mutations.** -- **P3 IS NOW MEASURED, NOT ASSUMED.** Replacing `window_event`'s whole - body with `let _ = (event_loop, event);` — a GUI that responds to no - input at all — leaves **all 256 `pmacs-gpu` tests green**. That is the - exception's true extent: `ActiveEventLoop` cannot exist outside a live - event loop, so **no** headless test in the crate observes the - delegation, not merely none of the new ones. + lets rustfmt rejoin lines. **22 witnesses — 13 routing, 9 effect — and + 23 mutations**, each failing its own rows, plus the P3 exception check + which must stay green. +- **P3 IS NOW MEASURED, NOT ASSUMED — and re-measured after the effect + harness landed.** Replacing `window_event`'s whole body with `let _ = + (event_loop, event);` — a GUI that responds to no input at all — + leaves **all 265 `pmacs-gpu` tests green** under `PMACS_REQUIRE_GPU=1` + at the current shape (it was 256 before the effect rows; the number is + re-run, not carried forward). That is the exception's true extent: + `ActiveEventLoop` cannot exist outside a live event loop, so **no** + headless test in the crate observes the delegation. What it covers is + now **one `if`**, not a 33-line match. - **A SECOND ACCEPTED STRUCTURAL EXCEPTION, found here and winit's rather than ours.** `KeyEvent` carries a `pub(crate) platform_specific` field, so **no `WindowEvent::KeyboardInput` can be @@ -347,12 +352,19 @@ waits for a signal that is not coming. the exception down); the family's only real decision is factored into `route_key_action(ElementState)` and witnessed directly; and what stays unwitnessed is one pattern arm containing a match and a call. -- **`event_loop.exit()` now appears in exactly two places, both inside - `window_event`.** The keyboard body was its second caller (the idle - Escape), so `apply_keyboard` returns an `EventOutcome` rather than - taking an `&ActiveEventLoop` — which is what keeps the bodies - reachable in principle. **Stage 1a's A4 deletes that branch**, at - which point `EventOutcome` has one variant and should go. +- **The crate now has exactly ONE executable `event_loop.exit()`**, in + `window_event`. Bodies return an `EventOutcome` rather than taking an + `&ActiveEventLoop`, which is what keeps them reachable in principle. + **`EventOutcome` has TWO producers and they differ in kind**: a native + close (`LifecycleRoute::Exit`), which must always exit, and + `apply_keyboard`'s idle Escape, which is a local quit. **Stage 1a's A4 + removes the keyboard producer only**, leaving **one** `Exit` + producer — the native close. **One producer is not one variant**: + `EventOutcome` survives because `dispatch_window_event` must still + distinguish `Continue` from `Exit` on every event, and only the close + exits. A4 changes `apply_keyboard`'s signature, not the type. An + earlier revision of this bullet claimed the type would collapse and + should go with the Escape branch; that was wrong. - **Slice order (each its own branch and PR):** `1-pre` → `1a`\* → `1b` → `1c` → `1d` → `1e`\*. **`1a` and `1e` are protocol-bearing (v24 `TextInput`, v25 `OpenTarget`/`OpenTargetResult`) and are @@ -374,10 +386,11 @@ waits for a signal that is not coming. file-watcher tests — with every other target green. Established as environmental three ways, in increasing strength: - **Structurally impossible for this branch to cause.** The branch - changes six files, five of them under `docs/`; **the whole - executable diff is `pmacs-gpu/src/main.rs`**, and `pmacs-gpu` is a - workspace **member but not a dependency** of the root package, so - the `m4_acceptance` binary never links it. + changes seven files, five of them under `docs/`; **the whole + executable diff is inside the `pmacs-gpu` crate** + (`src/main.rs` and `src/attach.rs`), and `pmacs-gpu` is a workspace + **member but not a dependency** of the root package, so the + `m4_acceptance` binary never links it. - **The marker is older than the session.** `/tmp/.git` is empty and was created at 20:14 CEST, **3.5 h before** the gate run at 23:45; `/tmp` held 8,920 entries. That is handoff §1's recorded hazard @@ -400,12 +413,21 @@ waits for a signal that is not coming. unnecessary, and the isolated `TMPDIR` is the correct remedy. It must be **outside `/tmp` and outside every git worktree** — a child of `/tmp` is not isolated, because `/tmp/.git` remains its ancestor. -- **GREEN under an isolated `TMPDIR`: all nine gates pass** (log - `20260811T215605Z-2664352`). fmt, clippy, lib, lib-crdt, - `gpu_invocation_acceptance`, **m4 168/0/3**, `PMACS_REQUIRE_GPU=1 -p - pmacs-gpu`, the **117-target `--workspace --no-fail-fast` sweep with - zero failures anywhere** (`m4_acceptance` running all 171 in it), and - `diff-check`. +- **GREEN under an isolated `TMPDIR`: all nine gates pass on the final + tree** (log `20260812T090034Z-2989598`, review round 2). fmt, clippy, + lib, lib-crdt, `gpu_invocation_acceptance`, **m4 168/0/3**, + `PMACS_REQUIRE_GPU=1 -p pmacs-gpu` (**265 tests, none filtered and + none skipped**, all nine effect rows included), the **117-target + `--workspace --no-fail-fast` sweep with zero failures anywhere** + (`m4_acceptance` running all 171 in it), and `diff-check`. Earlier + full-green runs at `20260811T215605Z-2664352` (round 1) and + `20260812T083735Z-2869707` (the P2 harness) are superseded by this + one. **What changed after it is doc comments and prose only** — the + `EventOutcome` correction below and this paragraph; `cargo fmt + --check`, `clippy -D warnings` and the 22 routing/effect rows were + re-run on the result. Stated rather than glossed, because "the gate + was green" and "the gate was green on exactly this tree" are + different claims. ## The GUI arc — Stage 0 MERGED as #236 (`f8ad3e7`) diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 3c099f5..281aac7 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -216,7 +216,7 @@ commands, read `docs/active-work.md` immediately after this file. roots but **not `TMPDIR`**, so `tempfile::tempdir()` still lands under a `/tmp` that may carry a marker. It surfaced inside a gate run on an unrelated lane (GUI 1-pre, whose whole **executable** diff - is `pmacs-gpu/src/main.rs`) as **`m4_24_bare_string_glob_stays_relative` + is inside the `pmacs-gpu` crate) as **`m4_24_bare_string_glob_stays_relative` and `m4_24_d3_fallback_base_is_the_smallest_attachment_dir`**, in both the `m4` step and the `--workspace` sweep, with every other target in the corpus green. diff --git a/docs/gui-stage1-input-framing.md b/docs/gui-stage1-input-framing.md index 45f48d7..c5553ff 100644 --- a/docs/gui-stage1-input-framing.md +++ b/docs/gui-stage1-input-framing.md @@ -148,11 +148,14 @@ blanket one: directly, with both misroute mutations failing that row alone. - **P3 is now measured, not assumed.** Replacing `window_event`'s entire body with `let _ = (event_loop, event);` — a GUI that responds to no - input at all — leaves **all 256 `pmacs-gpu` tests green**, not merely - the 13 routing rows. That is the exception's true extent: no headless - test anywhere in the crate observes the delegation. *(Revision 11 - shrinks what the exception covers: `window_event` is now four lines, - so the unwitnessed residue is one `if` rather than a 33-line match.)* + input at all — leaves **every `pmacs-gpu` test green**. That is the + exception's true extent: no headless test anywhere in the crate + observes the delegation. **Re-measured at the current shape after + revision 11: 265/265 under `PMACS_REQUIRE_GPU=1`** (it was 256 before + the effect rows existed, and the number is re-run rather than carried + forward). Revision 11 also shrinks what the exception *covers*: + `window_event` is four lines, so the unwitnessed residue is one `if` + rather than a 33-line match. **Revision 11 — P2 IS IMPLEMENTED AS WRITTEN. Revision 10's argument here was wrong and is retracted.** @@ -194,17 +197,30 @@ directions, which is P2's contract executable rather than asserted. The routing rows stay, and the division of labour is deliberate: the routing harness answers *where did this event go*, the effect harness -answers *what did it do*. The transcript row remains the routing -harness's sole P2-recording owner, which keeps its own mutation -surgical. +answers *what did it do*. **P2 is owned by the effect rows.** The +routing transcript row is only the sole owner of the *routing* +harness's own recording, which is what keeps that one mutation +surgical — revision 10 claimed it owned P2, and it never did. -**One design consequence worth carrying into 1a.** The keyboard arm was -the second caller of `event_loop.exit()` — the idle-Escape local quit — -so its body returns an `EventOutcome` rather than taking an -`&ActiveEventLoop`. `event_loop.exit()` now appears in exactly two -places, both inside `window_event`, and nowhere else in the crate. -**A4 deletes the Escape branch, at which point `EventOutcome` has one -variant and should go with it.** +**One further correction revision 11 carries, and it is the design +consequence 1a inherits.** Revision 10 stated that Stage 1a's A4 would +leave `EventOutcome` with a single variant and that the type should go +with the Escape branch. **Both are wrong.** + +`EventOutcome` has **two producers today**: `LifecycleRoute::Exit`, a +native window close that must always exit, and `apply_keyboard`'s idle +Escape, a local quit. **A4 removes the keyboard one**, leaving **exactly +one `Exit` producer** — the native close. + +**One producer is not one variant.** The type survives because +`dispatch_window_event` still has to distinguish `Continue` from +`Exit` on every event it handles: the overwhelming majority of +dispatches must *not* exit, and the native close must. What A4 actually +removes is `apply_keyboard`'s need to return an outcome at all, which is +a change to that one signature rather than to this type. + +The crate has **exactly one** executable `event_loop.exit()`, in +`window_event`. ### 1a — `TextInput` (v24) diff --git a/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index 0ca4608..3b16815 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -3226,8 +3226,9 @@ impl App { } else { // Q#S1-1 / A4 — the local quit, unchanged here and // deleted by Stage 1a: an idle Escape must reach the - // daemon. `window_event` performs the exit; this is - // the only reason a body needs an outcome at all. + // daemon. `window_event` performs the exit. This is the + // only reason a BODY needs an outcome; `EventOutcome` + // itself outlives A4, since a native close still exits. return EventOutcome::Exit; } return EventOutcome::Continue; @@ -3395,13 +3396,11 @@ impl App { /// exception and how far it reaches. **Performing** stays on `App`, in /// the `apply_*` methods the router's variants name. /// -/// The decision is what the route *is*, not merely which family claims -/// it: `Exit` is the local exit effect, `Resize` carries the clamped -/// surface extent, `Modifiers` carries the state mutation. **Two arms — -/// `CloseRequested` and `RedrawRequested` — send nothing outbound at -/// all**, so a harness recording only protocol traffic would leave them -/// invisible; that is why a route names its local effect and the harness -/// records routes. +/// A route carries the decision, not merely the family: `Resize` holds +/// the clamped extent, `Modifiers` the new state. **What a route does +/// NOT carry is the effect** — a `Wheel` may become a viewport update, a +/// panel event, a terminal event or nothing at all, depending on +/// `State`. Effects are witnessed separately, by `EffectHarness`. #[derive(Debug, Clone, Copy, PartialEq)] enum Route<'a> { /// The lifecycle family — see [`route_lifecycle`]. @@ -3417,16 +3416,25 @@ enum Route<'a> { Unrouted, } -/// What the event loop must do once a family's body has run. Only the -/// keyboard family produces anything but `Continue` today: an idle -/// Escape is a local quit. Returning the decision rather than taking an -/// `&ActiveEventLoop` is what keeps every body reachable from a test — -/// the crate's two `event_loop.exit()` call sites, this one and -/// `LifecycleRoute::Exit`, both sit in `window_event` and nowhere else. +/// What the event loop must do once a family's body has run. /// -/// Stage 1a's A4 deletes that branch (an idle Escape must reach the -/// daemon and never exit), at which point this type has one variant and -/// should go. +/// **Two producers, and they are not the same kind of thing.** +/// `LifecycleRoute::Exit` is a native window close, which must always +/// exit; `apply_keyboard` returns `Exit` for an idle Escape, which is a +/// local quit. Returning the decision rather than taking an +/// `&ActiveEventLoop` is what keeps every body reachable from a test: +/// the crate has **exactly one** executable `event_loop.exit()`, in +/// `window_event`. +/// +/// **Stage 1a's A4 removes the KEYBOARD producer only** — an idle +/// Escape must reach the daemon and never exit — leaving **one** `Exit` +/// producer, the native close. +/// +/// **One producer is not one variant.** This type survives A4 because +/// `dispatch_window_event` must still distinguish `Continue` from +/// `Exit` on every event it handles: nearly all of them must not exit, +/// and the close must. What A4 changes is `apply_keyboard`'s signature, +/// not this type. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum EventOutcome { Continue, @@ -3585,7 +3593,7 @@ fn route_pointer(event: &WindowEvent) -> Option { /// deliberately, not of outbound protocol traffic: `CloseRequested` /// exits and `RedrawRequested` repaints, and neither sends the daemon /// anything, so a transcript of daemon traffic alone cannot tell a -/// handled arm from a dropped one. A route names its local effect — +/// handled arm from a dropped one. A route records the decision — /// exit, resize extent, modifier mutation — which is what makes those /// arms observable here at all. /// @@ -3691,6 +3699,13 @@ impl EffectHarness { /// anything a body would legitimately send. const SENTINEL: char = '\u{e000}'; + /// Error ceiling on one outbound read. **Not a pacing device** — the + /// sentinel decides arrival — so this is set far above any plausible + /// drain: reaching it means the writer or encoder is broken, never + /// that the machine is busy. Without it a regression downstream of + /// `enqueue` wedges the gate instead of reddening it. + const READ_CEILING: std::time::Duration = std::time::Duration::from_secs(30); + /// Build the harness, or fail loudly. /// /// **Never skips.** `State::new_headless` returns `None` when no @@ -3717,6 +3732,9 @@ impl EffectHarness { let client = crate::attach::connect_stream_for_test(client_stream, |_| true) .expect("attach over socketpair"); let daemon = handshake.join().expect("handshake thread"); + daemon + .set_read_timeout(Some(Self::READ_CEILING)) + .expect("arm the outbound read ceiling"); // A document tall enough to scroll. A two-line fixture made the // wheel row pass vacuously: `scroll_by_lines` returns `None` @@ -3822,10 +3840,17 @@ impl EffectHarness { /// Push a sentinel through the same outbox and read until it comes /// back. Everything ahead of it belongs to the step just dispatched. /// - /// This is a **condition**, not a sleep: it blocks on the socket - /// until the writer thread has drained past the sentinel, so it is - /// insensitive to how many cores are free — the mistake PR #235's CI - /// red was made of. + /// **The sentinel is the success condition, and the timeout is only + /// an error ceiling** — the two are not the same thing and the + /// distinction is the whole design. Arrival is decided by the + /// sentinel, so the harness never infers "nothing was sent" from a + /// duration and is insensitive to how many cores are free; that is + /// the mistake PR #235's CI red was made of. But a blocking read + /// with no bound turns a regressed writer or encoder into a **wedged + /// gate** rather than a red one, and a hang is the worst failure + /// shape there is: it looks like slowness until the job is killed. + /// [`Self::READ_CEILING`] is therefore set far above any plausible + /// drain, so reaching it means broken, never busy. fn read_until_sentinel(&mut self) -> Vec { self.sentinel_seq += 1; let tag = self.sentinel_seq; @@ -3837,7 +3862,16 @@ impl EffectHarness { let mut seen = Vec::new(); loop { let event: pmacs_protocol::FrontendEvent = - pmacs_protocol::read_message(&mut self.daemon).expect("read outbound"); + match pmacs_protocol::read_message(&mut self.daemon) { + Ok(event) => event, + Err(e) => panic!( + "outbound read failed before the sentinel arrived \ + after {:?}: {e}. Either the writer or the encoder \ + regressed, or the step produced no sentinel at all. \ + Recorded so far: {seen:?}", + Self::READ_CEILING + ), + }; if is_sentinel(&event, tag) { return seen; } @@ -3856,9 +3890,11 @@ struct EffectSnapshot { } /// Modifier bits carrying the sentinel's sequence number, so a stale -/// sentinel cannot end the wrong step. Four bits is plenty: the harness -/// reads every sentinel it writes, so the counter only has to -/// distinguish neighbours. +/// sentinel cannot end the wrong step. **Three bits — Ctrl, Alt, Shift — +/// so the tag wraps every eight steps.** That is sufficient rather than +/// sloppy: the harness reads every sentinel it writes before issuing the +/// next, so a tag only ever has to distinguish itself from the one +/// immediately before it. #[cfg(test)] fn sentinel_mods(tag: u32) -> Modifiers { let mut mods = Modifiers::NONE; @@ -3915,10 +3951,10 @@ mod input_routing_tests { } /// The per-variant rows below drive [`route_event`] directly and the - /// transcript row drives the harness. That split is deliberate: it - /// leaves the transcript row as P2's sole owner, so a harness that - /// stopped recording an effect fails exactly one row instead of - /// every row. + /// transcript row drives [`RoutingHarness`]. That split keeps the + /// transcript row the only one that fails when the ROUTING harness + /// stops recording, so that mutation stays surgical. **P2 itself is + /// owned by the effect rows** — see [`EffectHarness`]. /// /// Events are bound to locals rather than passed as temporaries /// because a `Route` borrows the event it came from — the keyboard