From 9a0df216184729eaad7374cf9840f339941edd05 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 15:09:07 -0400 Subject: [PATCH 1/3] docs: frame the GPU terminal double layout-sync defect Reported symptom: text input in a terminal does not work on the GPU frontend while the TUI is fine. Root cause, measured against a real daemon, a real PTY child, and the real pmacs-gpu attach client: the dispatcher loop applies BOTH terminal-layout syncs to EVERY attached frontend each tick. A semantic session satisfies both conditions -- it has a term_sizes entry from AttachRequest and a semantic terminal declaration -- so the PTY is resized twice per tick forever, because the grid arm runs the TUI placement helper that subtracts a modeline the GPU never draws. The result is a SIGWINCH storm plus screen reflow at tick cadence, which makes an interactive line editor unusable while child output keeps flowing. Measured 730 terminal frames in 20 s for a static screen. Revision 2 answers Q#GT4 from the code rather than deferring it, which changes the fix from a one-line guard into a split of sync_terminal_layout: the grid arm is also the only per-tick controller-liveness release a GPU session gets, and the semantic arm neither performs it nor can be made to, because the buffer-follow snapshot clears the viewport declaration that would drive it. The doc also records three falsified hypotheses, why the Stage 3 real-path acceptance passes on the broken tree, and a COHERENCE.md section 6 citation error (two replica frontends have two different optimistic classifiers; the audit names only one). No code changes in this commit. --- docs/gpu-terminal-input-framing.md | 415 +++++++++++++++++++++++++++++ 1 file changed, 415 insertions(+) create mode 100644 docs/gpu-terminal-input-framing.md diff --git a/docs/gpu-terminal-input-framing.md b/docs/gpu-terminal-input-framing.md new file mode 100644 index 0000000..a383c14 --- /dev/null +++ b/docs/gpu-terminal-input-framing.md @@ -0,0 +1,415 @@ +# GPU terminal input — the double terminal-layout sync + +**Revision 2 — approved 2026-07-25. Scouted against canonical `main` @ +`8c86d34`; implemented on branch `gpu-terminal-input` off `main` @ `46a1b8f`, +whose only delta (#161) touches no file on this fix surface. Protocol stays +v20.** + +Revision 2 answers Q#GT4 from the code instead of deferring it, which changes +the proposed fix from a one-line guard to a **split of `sync_terminal_layout` +into a frontend-kind-neutral liveness half and a grid-only geometry half**; +rescores B1 as half-false; gives acceptance criteria 2 and 3 a landable +observation seam; and corrects four line citations plus the criterion-4 +rationale. Revision 1's diagnosis is unchanged — the defect, its measurements, +and the three falsified hypotheses all stand. + +Reported symptom: *"Text input within the terminal doesn't work on GUI, this +is fine in TUI."* + +This is a bug-fix framing, not a feature. It repairs a defect in Vterm Stage 3 +(#135) that ships on `main` today, and it closes the acceptance hole that let +the defect ship: the Stage 3 real-path acceptance drives a terminal session +end to end, and *still could not see this*. + +## Summary of the defect + +Every dispatcher tick, the daemon applies **both** terminal-layout syncs to +**every** attached frontend: + +```rust +// src/daemon.rs:1536-1554 (current main) +for frontend_id in &attached_fids { + if let Some(size) = term_sizes.get(frontend_id).copied() { + editor.sync_terminal_layout(*frontend_id, size); // GRID path + } + if let Some((buffer_id, size)) = semantic_states + .get(frontend_id) + .and_then(SemanticRenderState::terminal_viewport) + { + editor.sync_semantic_terminal_layout(*frontend_id, buffer_id, size); // SEMANTIC path + } +} +``` + +They are written as twins — the comment on the semantic arm even says *"right +beside the grid sync"* — but they are applied as **siblings, not +alternatives**. A GPU session has an entry in `term_sizes` (its `AttachRequest` +carries an initial cell size, and `Resize` events maintain it) *and* a +semantic terminal declaration. So both run, every tick. + +The two disagree by construction, and the semantic arm's own doc comment says +why: + +> the frontend declared a CONTENT rectangle, so this consumes the size +> directly instead of running the TUI placement helper, **which would subtract +> a modeline the GPU never drew**. + +That is exactly what the grid arm then does. Measured, on a real daemon with a +real PTY and the real GPU attach client: + +``` +PROBE sync_semantic old=Some(24x80) declared=25x92 +PROBE manager.resize BufferId(3) 25x92 +PROBE manager.resize BufferId(3) 22x80 +PROBE sync_semantic old=Some(22x80) declared=25x92 +PROBE manager.resize BufferId(3) 25x92 +PROBE manager.resize BufferId(3) 22x80 +... +``` + +The PTY is resized **twice per dispatcher tick, forever**. Each resize is a +`TIOCSWINSZ` + `SIGWINCH` to the child and a screen reflow in +`TerminalScreen`, so the child gets a SIGWINCH storm at tick cadence and the +screen alternates between two geometries. An interactive line editor +(readline, zle, fish's reader) redraws on every SIGWINCH, so what the user +types is continuously destroyed before it can settle — while ordinary child +*output* keeps flowing, which is why the terminal looks alive. + +Measured user-visible effect, real bash `-i` in the real GPU path, typing one +character: + +| | frames for a static screen | typed `Z` ever visible at the prompt | +|---|---|---| +| `main` today | **730** in a 20 s window | **no** | +| with the guard | **2** | (see Q#GT5 — a separate question) | + +The TUI is unaffected: a grid session has no semantic terminal declaration, so +only one arm ever runs for it. This is a **frontend-kind** defect, which is +why it presents as "GUI broken, TUI fine". + +## Ground truth (measured this session, not inferred) + +Everything below was established against `main` @ `8c86d34` with a real +daemon, a real PTY child, and the real `pmacs-gpu` attach client. The probe +harness is preserved (see "Verification plan"). + +### What is *not* wrong — three hypotheses falsified + +Recording these because each is a plausible-looking cause that a future +reader (or a review round) will re-propose. + +1. **The GPU's optimistic-CRDT path is not implicated.** The first hypothesis + was that a typed character becomes a `CrdtOp` against the read-only + terminal identity buffer and is dropped. It does not. Terminal buffers are + already marked round-trip — `core.set_round_trip_input(buffer_id, true)` + at `src/terminal/session.rs:338`, beside `set_read_only(true)` — so + `dispatch_idle_for` returns **false** while a terminal window is focused, + the daemon publishes `DispatchIdle { idle: false }`, and the GPU's + `daemon_intercepts_keys()` is true. Measured on the wire: + `dispatch_idle_in_terminal=false`, `intercept_in_terminal=true`, + `input_route=send_key(intercept)`. The optimistic gate is shut. +2. **Key transport is not implicated.** The keystroke reaches the daemon, + resolves a terminal view key, encodes, and is written to the PTY without + error: `PROBE dispatch_key ... terminal_key=Some(TerminalViewKey { .. })`, + `PROBE terminal transport encode=Some([90])`, `PROBE after send status=""`. + With a `cat` child the byte comes back on screen through the whole real GPU + path (`echoed_typed_char=true`). +3. **The `pmacs --attach` TUI replica does *not* share the defect.** It gates + its optimistic path on `dispatch_idle` alone (`src/attach.rs:843`), and + that signal is already correct for terminals per (1). + +### The mechanism + +- `EditorInstance::sync_terminal_layout` (`src/editor.rs:1195`) is the grid + path: it runs the TUI placement helper over the frontend's *frame* size. +- `EditorInstance::sync_semantic_terminal_layout` (`src/editor.rs:1331`) is + the semantic path: it consumes a declared *content* rectangle directly. +- Both resolve the same controller and call `TerminalManager::resize` on the + same session. Each has a correct `old_size == size` idempotence guard + (`src/editor.rs:1239` grid, `src/editor.rs:1360` semantic) — the guards are + individually sound and jointly useless, because each arm sees the size the + *other* just installed. +- `TerminalViewStore::record_view_size` (`src/terminal/view.rs:276-292`) + returns `true` for any valid declaration with no unchanged-size dedupe, + which is why the semantic arm re-fires every tick against the grid arm's + flip rather than settling. +- Loop order is grid first, semantic second, so the screen *ends* each tick at + the declared size. That is why rendering looks alive while the child is + whipsawed — and why a frame-based assertion is the wrong instrument + (acceptance criteria 2 and 3). +- `TerminalScreen::changed` bumps `generation` per mutation + (`src/terminal/screen.rs:1467`), which is why generation advances by + **exactly 2** per tick — one bump per resize. +- Frame suppression is full-struct equality + (`self.last_terminal_frame.as_ref() == Some(&frame)`, + `src/semantic_render.rs:882`). It is behaving correctly: the frames really + do differ. The churn is upstream, and fixing the churn fixes the frame + storm. **No suppression change is proposed.** + +### Why the Stage 3 acceptance could not catch it + +`a37_real_daemon_real_pty_and_headless_gpu_render_one_terminal_session` +(`tests/vterm_stage3_acceptance.rs:637`) is a genuine real-daemon + +real-PTY + real-wgpu path, and it still passes on the broken tree. Three +reasons, each worth keeping: + +1. Its child is `sh` printing 400 rows on a timer. **A frame storm is + invisible against a child that legitimately produces ~400 frames**, and its + only frame-count assertion is `frames >= 2`. +2. Its input step is `client.send_key(...)` called **directly** + (`pmacs-gpu/src/main.rs:784-785`), so it pins transport, not routing — and + it asserts nothing about the result of that input reaching the child. +3. It resizes **once, deliberately**, and asserts the new width comes back. + A geometry that oscillates *through* the asserted width satisfies that + assertion. This is the project's own "a geometric readout is not a state + predicate" lesson (`docs/active-work.md`, bottom-panel round 2) in a new + place: `observed_resized_frame` says "a frame at this width arrived", not + "the geometry settled at this width". + +## Decisions + +**Q#GT1 — Where does the fix go?** `sync_terminal_layout` is **split**, and +only its geometry half is gated by frontend kind. A bare "skip the grid arm +for semantic frontends" guard is wrong — see Q#GT4, which establishes that the +grid arm is also the only per-tick controller-liveness release. Not by +removing `term_sizes` for semantic sessions: semantic key and mouse dispatch +hard-depend on it (`src/daemon.rs:2191-2213`). + +The function has three separable concerns +(`src/editor.rs:1195-1260`), and they do not split where the name suggests: + +| lines | concern | frontend kind | +|---|---|---| +| 1199 | `reconcile_panel_layout` (Q#BP2b per-tick defensive) | **neutral** | +| 1200-1221 | controller liveness: released when the frontend has no view, or its active window no longer shows that terminal | **neutral** — reads only `core.views` / `core.windows` / the controller, never `term_size` | +| 1222-1259 | TUI placement (`window_placements`) + `resize` | **grid only** | + +So the daemon loop becomes: run the neutral half for every attached frontend +every tick, then exactly one geometry arm per frontend kind. +`sync_terminal_layout` survives as the composition of both halves, so +`editor::run`'s in-process loop and `LOCAL` keep byte-identical behavior. The +liveness half must run **once** per frontend per tick — reconciliation is +idempotent, so a double call is safe rather than wrong, but the loop should +not pay for it. + +**The trap inside the split:** the third release, at `src/editor.rs:1226` +(no placement found for the window), looks like liveness and is **not** — it +is grid geometry. A semantic frontend has no `window_placements` entry at all, +so moving that arm into the neutral half would release a GPU session's +controller on every single tick. That would be a new defect of exactly the +family this framing fixes, so it stays in the grid half. + +**Q#GT2 — Which arm wins for a semantic frontend?** The semantic one, +unconditionally. It is the only arm that consumes a *content* rectangle; the +grid arm's modeline subtraction is meaningless for a frontend that draws no +modeline into the terminal band. A GPU frontend that has not yet declared a +terminal viewport gets **neither** arm, which is correct: the terminal keeps +the geometry it was opened with until the frontend declares one. + +**Q#GT3 — Is the guard "no semantic state" or "not a semantic session"?** +`semantic_states` keyed by frontend id is the same map the semantic arm reads +one line later, so the two arms become provably exclusive by construction +rather than by two independent predicates that could drift apart. Rejected +alternative: keying on the negotiated `semantic_render` capability bit — it is +the *same* fact one indirection away, and the pair could then disagree. + +**Q#GT4 — Does anything else in `sync_terminal_layout` need to keep running +for a semantic frontend? Yes: the controller-liveness release, and the +semantic arm neither performs it nor can be made to.** Revision 1 left this +open; the code answers it. + +`release_controller` is called from exactly five sites, all in +`src/editor.rs`: the three grid-arm early returns (1210, 1219, 1226), +`dispatch_focus(gained = false)` (1189), and `reconcile_panel_layout`'s +unsatisfiable-panel path (848). **`sync_semantic_terminal_layout` releases +nothing.** When the window has switched away, `semantic_terminal_key` returns +`None` (`src/editor.rs:1278` — `window.buffer_id != buffer_id`) and the arm +returns `false` without touching the controller. + +Growing a release inside the semantic arm — revision 1's stated fallback for +B1 — **cannot work**, and the reason is worth keeping: when a GPU window +switches from the terminal to a document, the buffer-follow snapshot clears +the viewport declaration (`on_buffer_snapshot_sent` sets +`terminal_viewport = None`, `src/semantic_render.rs:574`), so +`terminal_viewport()` returns `None` and the semantic arm **stops running +entirely** for that frontend. A release placed inside it would never execute +in precisely the scenario that needs it. + +Nor do the other two sites cover it: `dispatch_focus(false)` fires on +whole-frontend focus loss, not on a window or buffer switch, and semantic +sessions are not panel-capable yet (`panel_capable_for` is false for them — +`src/daemon.rs:1893-1898`), so 848 never fires either. + +Consequence of shipping revision 1's guard as written: a GPU frontend that +switches away from its terminal **holds the controller indefinitely**. Because +another frontend's grid sync early-returns on a +`controller_view_for_frontend` mismatch, that peer then cannot resize the PTY +until it explicitly re-claims. This is why Q#GT1 splits the function instead +of gating it. + +**Q#GT7 — The per-tick defensive panel reconcile stays for semantic +frontends.** It is the only per-tick pre-paint reconcile the Q#BP2b contract +names (`src/editor.rs:822-830`), and today the grid arm supplies it for GPU +sessions too. Putting it in the neutral half of the split preserves that +exactly. It is harmless-either-way today — semantic sessions have unknown +frame geometry until the bottom-panel GPU band lands — but "harmless today" +is not a reason to remove a contract's only per-tick enforcement point in a +PR about something else. + +**Q#GT5 — Typed characters not echoing by an interactive shell is a +*separate* question and is deliberately out of scope.** Measured: with `bash +--norc -i` on a `TerminalMode::Raw` PTY, typed characters are not echoed to +the screen — **and this reproduces identically in-process**, i.e. on the TUI's +own path, where the user reports the terminal works. Because it is not +frontend-specific it cannot be the GUI/TUI asymmetry, and folding it in would +make this PR two features. It gets its own scout: whether `TerminalMode::Raw` +is the right mode for a `pmacs.terminal.open` child, and what pmacs owes a +child that expects to own its termios. Named, not silently dropped. + +**Q#GT6 — Protocol impact: none.** No wire shape, no negotiation, no version +change. Stays v20. + +## Bets + +- **B1 — SCORED HALF-FALSE before implementation (revision 2).** "Removing the + grid arm for semantic frontends removes the storm without removing any + behavior a GPU session relies on." The first clause holds (measured). The + second is **false**: it also removes the only controller-liveness release + and the only per-tick Q#BP2b reconcile a GPU session gets (Q#GT4, Q#GT7). + Its stated contingency — "the semantic arm grows the release" — is false + too, for a structural reason (`terminal_viewport` is cleared by the very + snapshot that signals the switch-away). Hence the split in Q#GT1. Recorded + rather than deleted: the failure mode is one a reviewer or a future + simplification will re-propose. +- **B2.** The user's reported symptom is this defect. *Partially scored: the + storm is proven and GUI-only, and its shape (line editor unusable, output + still flowing) matches the report. Not fully scored until the user, or an + acceptance running the **user's own shell**, confirms typing works after the + fix. Q#GT5 is the reason this bet is stated rather than assumed.* +- **B3.** No other pair of per-frontend-kind daemon operations is applied as + siblings rather than alternatives. *Scored by an explicit audit of the + dispatcher's per-frontend loop during implementation — this defect's shape + is "twins applied as siblings", and it would be negligent to fix one + instance without looking for others.* + +## Deferred (named) + +- Interactive-shell echo on a raw-mode PTY (Q#GT5) — its own scout. +- `TerminalFrame` suppression including `screen_generation` in its equality: + correct today and load-bearing for correctness, but it means any future + content-neutral generation bump re-emits a frame. Recorded, not changed. +- The `a37` probe's structural weaknesses beyond what the acceptance below + fixes (it still cannot exercise `App::window_event`'s routing, because that + logic is inline in the winit handler with no extractable seam). Making GPU + key routing testable is a real refactor and belongs to its own lane. + +## Acceptance criteria + +**The observation seam (revision 2).** Criteria 2, 3 and 6 assert daemon-side +state, and `TestDaemon` runs the daemon as a **subprocess** +(`tests/common/daemon.rs:90`), so nothing in-process can see it and the +scouting instrumentation does not land. The seam that does land: **extract the +dispatcher loop's per-frontend terminal-layout step into a named function** +that takes `(&mut EditorState, &[FrontendId], &term_sizes, &semantic_states)`. +That is required by Q#GT1's split anyway, it makes the grid/semantic +exclusivity structural rather than two adjacent `if`s, and it lets an +in-process test in the style of the existing `src/daemon.rs` unit tests +(3375ff) drive **the real loop body** rather than a re-implementation — which +is the a37 lesson applied to this PR's own tests. + +The observable is `TerminalScreen::generation`, reachable through +`TerminalManager::snapshot(..).generation`. It advances once per screen +mutation (`src/terminal/screen.rs:1467`), so with a quiet child it is a +**state predicate**, not a readout: "the geometry settled" is exactly +"generation stopped advancing". + +1. On a real daemon + real PTY + real GPU attach, a terminal session that + receives no child output produces a **bounded** number of terminal frames + (settling to zero new frames once the screen is static) — not one per tick. + Fails on `main` with ~730 frames in 20 s; passes with ≤ a small constant. +2. Driving the extracted loop body N times against a semantic frontend with a + fixed declaration and a quiet child: `TerminalManager::resize` takes effect + **exactly once** (generation advances once, then is constant for the + remaining N-1 iterations). Fails on `main`, where generation advances by + two per iteration. +3. After a declaration, `screen_size(buffer)` **equals the declared content + rectangle and stays equal** across subsequent iterations — the state + predicate, not the "a frame at this width arrived" readout that + `observed_resized_frame` provides today. +4. A character sent through the real GPU attach client reaches the child and + its echo appears in a rendered frame. **This is a keep-working pin, not a + fix discriminator: it already passes on today's broken `main`** (falsified + hypothesis 2 measured `echoed_typed_char=true`). Pinned with a `cat` child + — not because `cat` echoes (termios `ECHO` is off in raw mode; nothing + echoes) but because `cat` *copies stdin to stdout*, so the byte comes back + exactly once, with no line discipline and no double echo to disambiguate. +5. A grid (TUI) session's terminal resize behavior is **unchanged** — pinned + against the existing Stage 2 real-TUI PTY smoke, which must stay green + without modification. +6. A semantic frontend whose window stops showing the terminal **releases its + controller** (Q#GT4), pinned through the extracted loop body — driven by an + actual buffer switch, not by calling the release directly. This one bites + against revision 1's guard as well as against `main`. +7. End-to-end SIGWINCH count through the real PTY: a child trapping `WINCH` + and printing a **fresh distinct breadcrumb per signal** (`WINCH 1`, + `WINCH 2`, …) shows a bounded count. The distinctness is load-bearing — + the established PTY-paint trap is that cell diffing skips both spaces and + already-matching cells, so a repeated identical marker can assert nothing. +8. Bite-verified: reverting the split fails 2, 3, 6 and 7 specifically. + Criteria 1 and 7 fail against `main`; criterion 6 fails against **both** + `main` and revision 1's guard, which is the point of keeping B1's + half-false score on the record. + +Criteria 1, 2 and 7 are deliberately expressed as **quiet-child** assertions, +because the existing acceptance's chatty child is exactly what hid this. + +## Coherence impact (`COHERENCE.md` §20) + +- **§2 golden journey, step 8 ("Open a terminal")** — currently graded *"Works + but undiscoverable"*. On the GPU frontend it does not work; this restores + the step for the frontend the document calls the more capable one. Priority + 1 explicitly treats journey regressions as release blockers. +- **§16 Productize the Semantic Frontend Architecture** — graded *strong*, + with "graceful per-frontend degradation is practiced, not aspirational" as + its evidence, citing per-frontend fold projection. This defect is the + counter-example: a per-frontend-kind operation applied to both kinds at + once. The section's claim survives, but the audit should record that the + practice is enforced by convention, not by structure — two arms that must be + alternatives are currently just two adjacent `if`s. Q#GT1's extracted loop + body makes this one structural; the audit note should say the *pattern* is + still convention-enforced everywhere else (B3). +- **§6 Eliminate Hardcoded Interaction Islands** — the audit's row 6 note that + the GPU optimistic classifier "is kept honest by `dispatch_idle_for`" is + **confirmed correct** by this investigation (falsified hypothesis 1), and the + §6 citation `crate::optimistic::classify_key` should be corrected: that + symbol is `src/optimistic.rs`, the **`pmacs --attach` TUI replica's** + classifier. The GPU's separate, unrelated classifier is + `optimistic_insert_text` / `optimistic_crdt_insert` in + `pmacs-gpu/src/main.rs`. Two replica frontends, two classifiers; the audit + conflates them. +- **§19 Product Coherence Acceptance Tests** — this is a concrete instance of + the section's thesis. Every subsystem test passed; the defect lives in how + two correct subsystems compose per frontend kind. Criterion 1's quiet-child + shape is the transferable technique. +- No interaction island added, no config registry surface, no background-work + attribution change. + +## Verification plan + +Full gate suite per `CLAUDE.md`, plus: + +- `cargo test --features crdt --test vterm_stage3_acceptance` (the suite this + repairs) and `--test vterm_stage2_acceptance` (the TUI no-regression pin). +- `PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu`. +- The scouting harness is preserved and should be re-run against the branch: + a quiet-child variant of the `a37` probe plus daemon-side resize tracing, + saved as `scratch_gui_terminal_input.rs`, `scratch_inproc_input.rs`, and + `gui-terminal-probe-instrumentation.patch`. The instrumentation is scratch; + the acceptance criteria above are what lands. +- Manual confirmation with the user's own shell (fish) in a real GPU window, + since B2 is not fully scored by any automated test (Q#GT5). + +**Ops.** This doc is currently untracked in a detached-HEAD worktree +(`../pmacs-gui-term-input`), so it does not travel. On approval it becomes the +branch's first commit before any implementation, per the standing workflow — +no cross-machine expectation should attach to it until then. From b775f1703a46db84c6036db8bc5b3e987a133c43 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 15:22:04 -0400 Subject: [PATCH 2/3] fix(daemon): stop resizing a semantic frontend's PTY twice per tick The dispatcher loop applied BOTH terminal-layout syncs to EVERY attached frontend. A semantic session satisfies both conditions, because it has a term_sizes entry from AttachRequest and a semantic terminal declaration, so its PTY was resized twice on every tick forever: the grid arm installed the TUI placement size, the semantic arm installed the declared content rectangle, and each arm's own idempotence guard only ever saw the size the other had just written. The child took a SIGWINCH storm at tick cadence and the screen reflowed continuously, which is what made typing into a GPU terminal impossible while output kept flowing. The grid arm is also the only per-tick controller-liveness release a semantic frontend gets, so simply skipping it for those frontends trades one defect for another: a GPU window that switches away from its terminal would hold the controller forever, and no peer could resize that PTY again. The semantic arm cannot take over that job, because the buffer-follow snapshot clears the viewport declaration that would drive it. sync_terminal_layout is therefore split into a frontend-kind-neutral half (panel reconciliation plus controller liveness, which read only views, windows and the controller) and a grid-only geometry half (TUI placement plus the resize). The dispatcher runs the neutral half for every attached frontend once per tick, then exactly one geometry arm per frontend kind. sync_terminal_layout survives as the composition of both halves, so the in-process editor loop and LOCAL are unchanged. The loop body is extracted into sync_terminal_layouts_for_tick, which makes the grid/semantic exclusivity structural rather than two adjacent ifs, and lets the tests drive the real loop body instead of a re-implementation. The release that fires when a window has no placement stays in the grid half deliberately: a semantic frontend has no window_placements entry at all, so moving it into the neutral half would release a GPU session's controller on every tick. Bite-verified against two pre-images, because one is not enough here -- the naive guard fixes the storm and introduces the controller leak, so a single revert would score the fix complete when it is not: pin main naive guard split settle (acc 2+3) FAIL pass pass controller release (acc 6) pass FAIL pass grid still resizes (acc 5) pass pass pass Real-path acceptance: a quiet child that counts SIGWINCH reports 144 frames in 4 s and WINCH 1..12 on screen against the pre-fix tree, versus a settled screen with the fix. Acceptance 4 (input reaches the child and returns) is a keep-working pin and passes on both sides -- key transport was never the defect. No protocol change; stays v20. --- docs/gpu-terminal-input-framing.md | 33 +++- pmacs-gpu/src/main.rs | 41 +++- src/daemon.rs | 297 +++++++++++++++++++++++++++-- src/editor.rs | 92 +++++++-- tests/vterm_stage3_acceptance.rs | 225 ++++++++++++++++++++++ 5 files changed, 644 insertions(+), 44 deletions(-) diff --git a/docs/gpu-terminal-input-framing.md b/docs/gpu-terminal-input-framing.md index a383c14..0bbccef 100644 --- a/docs/gpu-terminal-input-framing.md +++ b/docs/gpu-terminal-input-framing.md @@ -295,6 +295,16 @@ change. Stays v20. ## Deferred (named) - Interactive-shell echo on a raw-mode PTY (Q#GT5) — its own scout. +- **A geometry change appears to clear the visible screen.** Observed while + building acceptance 4: after the probe's deliberate 25×92 → 20×71 resize, + the next frame's visible grid is entirely blank even though the content + (two short lines near the top) should survive a shrink of that size. It + reproduces on the pre-fix tree, so it is neither caused nor fixed here, and + it is why acceptance 4 latches its observation across frames instead of + reading the final one. Not investigated: it could be correct reflow + behaviour given where the child leaves its cursor (frames show the cursor + on the bottom row), or a real reflow defect. Named because the next person + to write a resize assertion will hit it. - `TerminalFrame` suppression including `screen_generation` in its equality: correct today and load-bearing for correctness, but it means any future content-neutral generation bump re-emits a frame. Recorded, not changed. @@ -349,16 +359,29 @@ mutation (`src/terminal/screen.rs:1467`), so with a quiet child it is a 6. A semantic frontend whose window stops showing the terminal **releases its controller** (Q#GT4), pinned through the extracted loop body — driven by an actual buffer switch, not by calling the release directly. This one bites - against revision 1's guard as well as against `main`. + against **revision 1's naive guard**, and deliberately **passes on `main`**: + today's sibling arms do supply the release, by the accident of the grid arm + running for a frontend it should never have run for. It is the pin that + stops the fix from trading one defect for another. 7. End-to-end SIGWINCH count through the real PTY: a child trapping `WINCH` and printing a **fresh distinct breadcrumb per signal** (`WINCH 1`, `WINCH 2`, …) shows a bounded count. The distinctness is load-bearing — the established PTY-paint trap is that cell diffing skips both spaces and already-matching cells, so a repeated identical marker can assert nothing. -8. Bite-verified: reverting the split fails 2, 3, 6 and 7 specifically. - Criteria 1 and 7 fail against `main`; criterion 6 fails against **both** - `main` and revision 1's guard, which is the point of keeping B1's - half-false score on the record. +8. Bite-verified against **two** pre-images, because one is not enough here — + the naive guard fixes the storm and introduces a different defect, so a + single revert would score the fix complete when it is not. Measured + (`cargo test --lib`, manual revert since these tests share `src/daemon.rs` + with the production code): + + | pin | `main` (sibling arms) | rev-1 naive guard | the split | + |---|---|---|---| + | acc 2+3 settle | **FAIL** | pass | pass | + | acc 6 controller release | pass | **FAIL** | pass | + | acc 5 grid still resizes | pass | pass | pass | + + The middle column is B1's half-false score made executable: the naive + guard's first clause holds (the storm stops) and its second does not. Criteria 1, 2 and 7 are deliberately expressed as **quiet-child** assertions, because the existing acceptance's chatty child is exactly what hid this. diff --git a/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index 6372189..665194c 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -728,7 +728,22 @@ fn run_headless_probe(socket: &Path, report: &Path) -> i32 { let _ = client.send_key(ProtocolKey::Char(chord), Modifiers::CTRL | Modifiers::ALT); } - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(20); + // Quiet-observation mode. `PMACS_GPU_PROBE_OBSERVE_MS` makes the probe + // send NO input and request NO resize, and observe for exactly that long + // instead of stopping at its usual condition. + // + // This exists because the ordinary probe cannot see a frame storm: it + // stops as soon as it has watched a resize land, so a session emitting a + // frame every tick and one emitting three in total both satisfy it. A + // fixed window over a child that produces no output turns "how many + // frames did the daemon send?" into a number worth asserting on. + let observe_window = std::env::var("PMACS_GPU_PROBE_OBSERVE_MS") + .ok() + .and_then(|value| value.parse::().ok()) + .map(std::time::Duration::from_millis); + let quiet = observe_window.is_some(); + let deadline = std::time::Instant::now() + + observe_window.unwrap_or_else(|| std::time::Duration::from_secs(20)); let mut sent_input = false; let mut sent_resize = false; while std::time::Instant::now() < deadline { @@ -778,13 +793,17 @@ fn run_headless_probe(socket: &Path, report: &Path) -> i32 { if pixels.iter().any(|&b| b != first) { facts.rendered_nonuniform_frames += 1; } - if !sent_input && facts.frames >= 1 { + if facts.last_frame_text.contains(PROBE_INPUT_CHAR) { + facts.input_echo_observed = true; + } + if !quiet && !sent_input && facts.frames >= 1 { sent_input = true; // Real child input over the real wire. - let _ = client.send_key(ProtocolKey::Char('x'), Modifiers::NONE); + let _ = + client.send_key(ProtocolKey::Char(PROBE_INPUT_CHAR), Modifiers::NONE); let _ = client.send_key(ProtocolKey::Enter, Modifiers::NONE); } - if !sent_resize && facts.frames >= 2 { + if !quiet && !sent_resize && facts.frames >= 2 { sent_resize = true; state.resize(700, 500); if let Some((buffer_id, size)) = state.terminal_declaration_if_changed() @@ -800,7 +819,7 @@ fn run_headless_probe(socket: &Path, report: &Path) -> i32 { facts.observed_resized_frame = true; } } - if facts.observed_resized_frame && facts.rendered_nonuniform_frames >= 2 { + if !quiet && facts.observed_resized_frame && facts.rendered_nonuniform_frames >= 2 { break; } } @@ -832,6 +851,7 @@ fn run_headless_probe(socket: &Path, report: &Path) -> i32 { let _ = writeln!(out, "resized_cols={}", facts.resized_cols); let _ = writeln!(out, "last_title={}", facts.last_title.unwrap_or_default()); let _ = writeln!(out, "last_frame_text={}", facts.last_frame_text); + let _ = writeln!(out, "input_echo_observed={}", facts.input_echo_observed); let _ = writeln!(out, "disconnect={}", facts.disconnect.unwrap_or_default()); if let Err(error) = std::fs::write(report, out) { eprintln!( @@ -1037,9 +1057,20 @@ struct ProbeFacts { resized_cols: u32, last_title: Option, last_frame_text: String, + /// Whether any frame carried the probe's own typed character back. + /// + /// Latched ACROSS frames, not read off the final one: a later geometry + /// change reflows the screen, so "the echo arrived" and "the echo is + /// still on the last frame" are different questions and only the first + /// one is about input reaching the child. + input_echo_observed: bool, disconnect: Option, } +/// The character the probe types into the child. Distinct from anything the +/// acceptance children print themselves, so its appearance is unambiguous. +const PROBE_INPUT_CHAR: char = 'x'; + /// One-line printable text of a terminal frame, for probe reporting. fn frame_probe_text(frame: &TerminalFrame) -> String { let mut text = String::new(); diff --git a/src/daemon.rs b/src/daemon.rs index 5af71d0..9ac8256 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1536,23 +1536,7 @@ fn dispatcher_loop( // Accepted terminal context controls PTY size. Apply any focus, // window, or resize changes before consuming another child-output // batch so screen reflow and subsequent bytes share one geometry. - for frontend_id in &attached_fids { - if let Some(size) = term_sizes.get(frontend_id).copied() { - editor.sync_terminal_layout(*frontend_id, size); - } - // Vterm Stage 3 — the semantic twin, right beside the grid - // sync so both frontend kinds resize the screen before the - // next child-output drain. The frontend declared a CONTENT - // rectangle, so this consumes the size directly instead of - // running the TUI placement helper, which would subtract a - // modeline the GPU never drew. - if let Some((buffer_id, size)) = semantic_states - .get(frontend_id) - .and_then(crate::semantic_render::SemanticRenderState::terminal_viewport) - { - editor.sync_semantic_terminal_layout(*frontend_id, buffer_id, size); - } - } + sync_terminal_layouts_for_tick(editor, &attached_fids, &term_sizes, &semantic_states); // `tick_async` last: the M4.5 async bridge settles awaiters // inside `tick_lsp` (via the message bus); draining + resuming @@ -3064,6 +3048,56 @@ fn build_presence_snapshot(editor: &EditorState, frontend_id: FrontendId) -> Pre } } +/// One dispatcher tick's terminal-layout step, for every attached frontend. +/// +/// Extracted from the dispatcher loop so the grid/semantic exclusivity is +/// **structural** rather than two adjacent `if`s, and so acceptance tests can +/// drive the real loop body instead of re-implementing it (Q#GT1). +/// +/// The shape that matters: liveness is frontend-kind NEUTRAL and runs for +/// everyone, exactly once; the geometry arms are EXCLUSIVE alternatives keyed +/// on the same `semantic_states` membership that session establishment uses, +/// so a session can never be caught by both. +/// +/// Before this existed, both arms ran for every frontend. A semantic session +/// has a `term_sizes` entry (from `AttachRequest`) *and* a terminal +/// declaration, so its PTY was resized twice per tick, forever: the grid arm +/// installed the TUI placement size, the semantic arm installed the declared +/// content rectangle, and each arm's own idempotence guard saw only the size +/// the other had just written. The child got a `SIGWINCH` storm at tick +/// cadence, which is what made typing into a GPU terminal impossible while +/// output kept flowing. +fn sync_terminal_layouts_for_tick( + editor: &mut EditorState, + attached_fids: &[FrontendId], + term_sizes: &HashMap, + semantic_states: &HashMap, +) { + for frontend_id in attached_fids { + // Neutral half: panel reconciliation (Q#BP2b's only per-tick + // enforcement point) and the release of a controller whose window + // moved away. A semantic frontend gets this from nowhere else — + // its own arm stops running the moment the buffer-follow snapshot + // clears the declaration (Q#GT4/Q#GT7). + editor.sync_terminal_controller_liveness(*frontend_id); + + // Geometry: exactly one arm per frontend kind. + if let Some(state) = semantic_states.get(frontend_id) { + // Vterm Stage 3 — the frontend declared a CONTENT rectangle, + // so this consumes the size directly instead of running the + // TUI placement helper, which would subtract a modeline the + // GPU never drew. A semantic frontend with no declaration yet + // gets NO resize at all, which is correct: the terminal keeps + // the geometry it was opened with until one arrives. + if let Some((buffer_id, size)) = state.terminal_viewport() { + editor.sync_semantic_terminal_layout(*frontend_id, buffer_id, size); + } + } else if let Some(size) = term_sizes.get(frontend_id).copied() { + editor.sync_terminal_grid_geometry(*frontend_id, size); + } + } +} + /// Dispatch a semantic (grid-less) frontend's input event into the /// shared editor core (Phase B, session B1). Mirrors the `Key` / `Mouse` /// arms of [`apply_event`] but takes no `RenderState` — a semantic @@ -3365,6 +3399,235 @@ mod tests { ); } + // ---- GPU terminal input: the double terminal-layout sync ------------- + // + // These drive `sync_terminal_layouts_for_tick` — the REAL dispatcher loop + // body, not a re-implementation of it. That distinction is the whole + // point: the Stage 3 acceptance sent input through `client.send_key` + // directly and therefore pinned transport rather than routing, which is + // how the defect these pin shipped. + // + // The observable is `TerminalScreen::generation`. It advances once per + // screen mutation, so with a child that produces no output and no + // `tick_processes` call, "generation stopped advancing" is exactly "the + // geometry settled" — a state predicate, not a readout. + + /// Open a quiet terminal and give `frontend_id` a view that shows it, + /// holding its controller — the state the dispatcher loop runs against. + fn quiet_terminal_for( + editor: &EditorState, + frontend_id: FrontendId, + ) -> (crate::buffer::BufferId, crate::window::WindowId) { + let mut spec = crate::terminal::TerminalSpec::new("/bin/sh"); + spec.args = vec!["-c".into(), "sleep 30".into()]; + spec.rows = 24; + spec.cols = 80; + let buffer_id = editor + .terminal_manager + .borrow_mut() + .open( + spec, + &mut editor.core.borrow_mut(), + &mut editor.process_supervisor.borrow_mut(), + ) + .expect("open terminal"); + + let window_id = crate::window::WindowId::next(); + { + let mut core = editor.core.borrow_mut(); + let text_view = { + let registry = core.registry.clone(); + let registry = registry.borrow(); + let buffer = registry.get(buffer_id).expect("terminal buffer"); + crate::text_view::TextView::new(buffer) + }; + core.windows.insert( + window_id, + crate::window::Window::new(window_id, buffer_id, text_view), + ); + core.register_frontend_view( + frontend_id, + crate::window::FrontendView { + layout: crate::window::Layout::single(window_id), + active: window_id, + fold_projection: true, + panel_capable: false, + frame_geometry: None, + panel_hidden: false, + }, + ); + } + let key = crate::terminal::TerminalViewKey::new(frontend_id, window_id, buffer_id); + let mut manager = editor.terminal_manager.borrow_mut(); + manager.register_view(key); + manager.claim_controller(key); + (buffer_id, window_id) + } + + fn screen_generation(editor: &EditorState, buffer_id: crate::buffer::BufferId) -> u64 { + editor + .terminal_manager + .borrow() + .snapshot(buffer_id) + .expect("terminal snapshot") + .screen_generation + } + + /// Acceptance 2 and 3: one declaration produces exactly one resize, and + /// the screen then STAYS at the declared content rectangle. + /// + /// Against the pre-split tree both arms ran for the semantic frontend and + /// generation advanced by two per iteration forever, because each arm's + /// idempotence guard only ever saw the size the other had just written. + #[test] + fn semantic_terminal_geometry_settles_after_one_declaration() { + let fid = FrontendId(41); + let mut editor = EditorState::new(); + let (buffer_id, _window) = quiet_terminal_for(&editor, fid); + + // The GPU declares a CONTENT rectangle; the grid size it also + // reported at attach is deliberately DIFFERENT, which is the + // collision the defect fed on. + let declared = CellSize::new(25, 92); + let mut semantic = crate::semantic_render::SemanticRenderState::for_peer(fid, 20); + semantic.set_terminal_viewport(buffer_id, declared); + let semantic_states = HashMap::from([(fid, semantic)]); + let term_sizes = HashMap::from([(fid, CellSize::new(24, 80))]); + let attached = vec![fid]; + + sync_terminal_layouts_for_tick(&mut editor, &attached, &term_sizes, &semantic_states); + let after_first = screen_generation(&editor, buffer_id); + assert_eq!( + editor.terminal_manager.borrow().screen_size(buffer_id), + Some(declared), + "the declared content rectangle must win" + ); + + // Acceptance 2: every further tick is a no-op. + for _ in 0..8 { + sync_terminal_layouts_for_tick(&mut editor, &attached, &term_sizes, &semantic_states); + } + assert_eq!( + screen_generation(&editor, buffer_id), + after_first, + "an unchanged declaration must not mutate the screen again \ + (pre-split: +2 per tick, forever)" + ); + // Acceptance 3: the state predicate, not "a frame at this width + // arrived at some point". + assert_eq!( + editor.terminal_manager.borrow().screen_size(buffer_id), + Some(declared), + "the geometry must SETTLE at the declared rectangle" + ); + + editor.process_supervisor.borrow_mut().shutdown(); + } + + /// Acceptance 6: a semantic frontend whose window switches away releases + /// its terminal controller. + /// + /// This bites against BOTH the pre-split tree's sibling arms and against + /// the naive "skip the grid arm for semantic frontends" guard, which is + /// why B1 is recorded as half-false. The release cannot live in + /// `sync_semantic_terminal_layout`: the buffer-follow snapshot clears the + /// viewport declaration, so that arm stops running in exactly this case — + /// modelled here by dropping the declaration alongside the switch. + #[test] + fn semantic_frontend_releases_its_terminal_controller_when_its_window_switches_away() { + let fid = FrontendId(42); + let mut editor = EditorState::new(); + let (buffer_id, window_id) = quiet_terminal_for(&editor, fid); + + let declared = CellSize::new(25, 92); + let mut semantic = crate::semantic_render::SemanticRenderState::for_peer(fid, 20); + semantic.set_terminal_viewport(buffer_id, declared); + let mut semantic_states = HashMap::from([(fid, semantic)]); + let term_sizes = HashMap::from([(fid, CellSize::new(24, 80))]); + let attached = vec![fid]; + + sync_terminal_layouts_for_tick(&mut editor, &attached, &term_sizes, &semantic_states); + assert_eq!( + editor + .terminal_manager + .borrow() + .controller_view_for_frontend(fid), + Some(crate::terminal::TerminalViewKey::new( + fid, window_id, buffer_id + )), + "precondition: the frontend holds the controller" + ); + + // The window switches to a document, and the snapshot that announces + // it clears the semantic declaration — `on_buffer_snapshot_sent`. + let document = editor.core.borrow().registry.borrow_mut().create("doc"); + { + let mut core = editor.core.borrow_mut(); + let text_view = { + let registry = core.registry.clone(); + let registry = registry.borrow(); + let buffer = registry.get(document).expect("document buffer"); + crate::text_view::TextView::new(buffer) + }; + let window = core.windows.get_mut(&window_id).expect("window"); + *window = crate::window::Window::new(window_id, document, text_view); + } + semantic_states + .get_mut(&fid) + .expect("semantic state") + .on_buffer_snapshot_sent(document); + + sync_terminal_layouts_for_tick(&mut editor, &attached, &term_sizes, &semantic_states); + assert_eq!( + editor + .terminal_manager + .borrow() + .controller_view_for_frontend(fid), + None, + "a semantic frontend that left its terminal must release the \ + controller, or no peer can resize that PTY again" + ); + + editor.process_supervisor.borrow_mut().shutdown(); + } + + /// Acceptance 5 at the unit seam: a GRID frontend still gets its + /// placement-derived resize. The split must not turn the storm fix into + /// "semantic frontends win everywhere". + #[test] + fn grid_terminal_geometry_still_syncs_for_a_grid_frontend() { + let fid = FrontendId(43); + let mut editor = EditorState::new(); + let (buffer_id, _window) = quiet_terminal_for(&editor, fid); + + let semantic_states = HashMap::new(); + let term_sizes = HashMap::from([(fid, CellSize::new(40, 100))]); + let attached = vec![fid]; + + let before = editor.terminal_manager.borrow().screen_size(buffer_id); + sync_terminal_layouts_for_tick(&mut editor, &attached, &term_sizes, &semantic_states); + let after = editor.terminal_manager.borrow().screen_size(buffer_id); + + assert_ne!(before, after, "a grid frontend must still resize its PTY"); + assert_eq!( + after.map(|size| size.cols), + Some(100), + "the grid arm supplies the full declared width" + ); + // And it too settles. + let settled = screen_generation(&editor, buffer_id); + for _ in 0..4 { + sync_terminal_layouts_for_tick(&mut editor, &attached, &term_sizes, &semantic_states); + } + assert_eq!( + screen_generation(&editor, buffer_id), + settled, + "an unchanged grid size must not mutate the screen again" + ); + + editor.process_supervisor.borrow_mut().shutdown(); + } + #[test] fn frontend_events_from_uninstalled_sessions_are_dropped_without_state_access() { let source = FrontendId(77); diff --git a/src/editor.rs b/src/editor.rs index 79f1225..d4321b5 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -1189,14 +1189,84 @@ impl EditorState { let _ = self.terminal_manager.borrow_mut().release_controller(key); } + /// Reconcile panels and release a controller whose window moved away. + /// + /// **Frontend-kind neutral, and deliberately so** (Q#GT1/Q#GT4): this + /// half reads only `core.views`, `core.windows`, and the controller — + /// never a grid size — so it is the half the dispatcher runs for EVERY + /// attached frontend once per tick. It was previously fused into + /// [`Self::sync_terminal_layout`], which meant a semantic frontend got + /// its controller-liveness release only as a side effect of a grid + /// resize it should never have received. + /// + /// [`Self::sync_semantic_terminal_layout`] cannot substitute for this: + /// when a GPU window switches away from its terminal, the buffer-follow + /// snapshot clears the viewport declaration + /// (`SemanticRenderState::on_buffer_snapshot_sent`), so the semantic arm + /// stops running entirely in exactly the case that needs the release. + /// + /// Returns `true` while `frontend_id` still holds a live controller. + pub fn sync_terminal_controller_liveness(&mut self, frontend_id: FrontendId) -> bool { + // Bottom-panel arc (Q#BP2b): a panel that just became + // unsatisfiable must have released its controller before any + // resize runs, or the child would be resized against a dead rect. + // This is the contract's only per-tick enforcement point, and it + // stays neutral so semantic frontends keep it (Q#GT7). + self.reconcile_panel_layout(frontend_id); + let Some(key) = self + .terminal_manager + .borrow() + .controller_view_for_frontend(frontend_id) + else { + return false; + }; + let core = self.core.borrow(); + let Some(view) = core.views.get(&frontend_id) else { + drop(core); + let _ = self.terminal_manager.borrow_mut().release_controller(key); + return false; + }; + if view.active != key.window_id + || core + .windows + .get(&key.window_id) + .is_none_or(|window| window.buffer_id != key.buffer_id) + { + drop(core); + let _ = self.terminal_manager.borrow_mut().release_controller(key); + return false; + } + true + } + /// Resize the one session durably controlled by `frontend_id`. /// /// This is called before process drain and paint, never from rendering. + /// + /// Composition of the two halves, preserved verbatim for the in-process + /// `editor::run` loop and `LOCAL`. The daemon dispatcher calls the halves + /// separately, because only the geometry half is grid-specific. pub fn sync_terminal_layout(&mut self, frontend_id: FrontendId, term_size: CellSize) -> bool { - // Bottom-panel arc (Q#BP2b): a panel that just became - // unsatisfiable must have released its controller before this - // runs, or the child would be resized against a dead rect. - self.reconcile_panel_layout(frontend_id); + self.sync_terminal_controller_liveness(frontend_id) + && self.sync_terminal_grid_geometry(frontend_id, term_size) + } + + /// The grid half: TUI placement plus the resize it implies. + /// + /// **Grid frontends only** (Q#GT1). The placement lookup below is why: + /// a semantic frontend has no `window_placements` entry at all, so the + /// "no placement" arm would release its controller on EVERY tick. That + /// release reads like liveness and is not — it is grid geometry, and + /// moving it into [`Self::sync_terminal_controller_liveness`] would + /// reintroduce this framing's own defect in a new place. + /// + /// Assumes liveness already ran: the controller is live and its window + /// still shows the terminal. + pub fn sync_terminal_grid_geometry( + &mut self, + frontend_id: FrontendId, + term_size: CellSize, + ) -> bool { let Some(key) = self .terminal_manager .borrow() @@ -1206,23 +1276,11 @@ impl EditorState { }; let content = { let core = self.core.borrow(); - let Some(view) = core.views.get(&frontend_id) else { - let _ = self.terminal_manager.borrow_mut().release_controller(key); - return false; - }; - if view.active != key.window_id - || core - .windows - .get(&key.window_id) - .is_none_or(|window| window.buffer_id != key.buffer_id) - { - let _ = self.terminal_manager.borrow_mut().release_controller(key); - return false; - } let Some(placement) = window_placements(&core, frontend_id, term_size) .get(&key.window_id) .copied() else { + drop(core); let _ = self.terminal_manager.borrow_mut().release_controller(key); return false; }; diff --git a/tests/vterm_stage3_acceptance.rs b/tests/vterm_stage3_acceptance.rs index 04b2c27..b7c2e9c 100644 --- a/tests/vterm_stage3_acceptance.rs +++ b/tests/vterm_stage3_acceptance.rs @@ -1087,3 +1087,228 @@ fn a28_a30_a_v18_semantic_peer_has_no_terminal_surface() { .terminate(terminal_buffer, &mut state.process_supervisor.borrow_mut()) .expect("terminate child"); } + +// ---- GPU terminal input: the double terminal-layout sync ----------------- +// +// Acceptance 1, 4 and 7 of `docs/gpu-terminal-input-framing.md`, on the real +// path: real daemon, real PTY child, real `pmacs-gpu` attach client. +// +// `a37` above passes on the broken tree, and these are shaped around exactly +// why. Its child prints 400 rows on a timer, so a frame storm hides inside +// legitimate output; its only frame-count assertion is `frames >= 2`; and its +// resize assertion is satisfied by a geometry that oscillates THROUGH the +// asserted width. The children below are therefore deliberately QUIET, and +// the assertions are upper bounds. + +/// A terminal child that produces nothing on its own and prints one fresh, +/// DISTINCT breadcrumb per `SIGWINCH`. +/// +/// Distinctness is load-bearing: `cell::diff` skips both spaces and +/// already-matching cells, so a repeated identical marker can never be +/// asserted on — the second and later copies would paint nothing. +#[cfg(feature = "crdt")] +const WINCH_PROBE_INIT_LUA: &str = r#" +pmacs.command.define { + name = "vterm-probe.open", + description = "Open a quiet terminal that counts SIGWINCH.", + fn = function() + return pmacs.terminal.open { + command = "/bin/sh", + args = { "-c", + "n=0; trap 'n=$((n+1)); printf \"WINCH %d\r\n\" \"$n\"' WINCH; " .. + "printf 'READY\r\n'; while :; do sleep 0.2; done" }, + } + end, +} +pmacs.keymap.bind { scope = "global", sequence = "C-M-t", command = "vterm-probe.open" } +"#; + +/// A terminal child that echoes input by copying stdin to stdout. +/// +/// `cat` is the right instrument precisely because it does NOT echo: termios +/// `ECHO` is off on a `TerminalMode::Raw` PTY, so nothing in the kernel line +/// discipline reflects the byte. `cat` copies it exactly once, which makes a +/// single typed character produce a single unambiguous cell. +#[cfg(feature = "crdt")] +const CAT_PROBE_INIT_LUA: &str = r#" +pmacs.command.define { + name = "vterm-probe.open", + description = "Open a terminal child that copies stdin to stdout.", + fn = function() + return pmacs.terminal.open { + command = "/bin/sh", + args = { "-c", "printf 'READY\r\n'; exec cat" }, + } + end, +} +pmacs.keymap.bind { scope = "global", sequence = "C-M-t", command = "vterm-probe.open" } +"#; + +/// Run the headless GPU probe against a daemon built from `init_lua`, and +/// return its parsed report. `observe_ms` selects quiet-observation mode. +#[cfg(feature = "crdt")] +fn run_gpu_probe( + init_lua: &str, + observe_ms: Option, +) -> Option> { + use std::path::{Path, PathBuf}; + + fn gpu_binary() -> PathBuf { + Path::new(env!("CARGO_BIN_EXE_pmacs")) + .parent() + .expect("test binary directory") + .join("pmacs-gpu") + } + + let required = std::env::var_os("PMACS_REQUIRE_GPU").is_some(); + let binary = gpu_binary(); + if !binary.exists() { + assert!( + !required, + "PMACS_REQUIRE_GPU is set but {} is not built", + binary.display() + ); + eprintln!("skipping: {} is not built", binary.display()); + return None; + } + + let daemon = common::daemon::TestDaemon::spawn_with_env_and_init( + &[ + ("PMACS_INSTANCE_SEMANTIC_RENDER", "1"), + ("PMACS_INSTANCE_MULTI_FRONTEND", "1"), + ], + init_lua, + ); + let report = daemon + .socket_path() + .parent() + .expect("socket parent") + .join("gpu-probe.txt"); + let mut command = std::process::Command::new(&binary); + command + .arg("--headless-probe") + .arg(daemon.socket_path()) + .arg(&report) + .env("PMACS_GPU_PROBE_OPEN_KEY", "t"); + if let Some(ms) = observe_ms { + command.env("PMACS_GPU_PROBE_OBSERVE_MS", ms.to_string()); + } + let output = command.output().expect("run the headless GPU probe"); + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + let no_adapter = output.status.code() == Some(3); + assert!( + no_adapter && !required, + "headless GPU probe failed (status {:?}):\n{stderr}", + output.status.code() + ); + eprintln!("skipping: no wgpu adapter available"); + return None; + } + let text = std::fs::read_to_string(&report).expect("probe report"); + Some( + text.lines() + .filter_map(|line| line.split_once('=')) + .map(|(key, value)| (key.to_owned(), value.to_owned())) + .collect(), + ) +} + +/// Acceptance 1 and 7: a GPU session showing a quiet terminal must settle. +/// +/// Both assertions are upper bounds over a fixed observation window, which is +/// the only shape that can see this defect. On the pre-fix tree the dispatcher +/// resized the PTY twice per tick forever, so the child took a `SIGWINCH` +/// storm and the daemon emitted a terminal frame per tick — measured at ~730 +/// frames in 20 s against a child that printed one line and then slept. +#[cfg(feature = "crdt")] +#[test] +fn gpu_terminal_geometry_settles_and_stops_signalling_the_child() { + const OBSERVE_MS: u64 = 4_000; + let Some(facts) = run_gpu_probe(WINCH_PROBE_INIT_LUA, Some(OBSERVE_MS)) else { + return; + }; + let report = || format!("{facts:#?}"); + + assert_eq!( + facts.get("entered_terminal_mode").map(String::as_str), + Some("true"), + "precondition: the GPU entered terminal mode from a real frame: {}", + report() + ); + // Non-vacuity for the whole test: the child really did run, and the + // breadcrumb mechanism really does paint. + let screen = facts.get("last_frame_text").cloned().unwrap_or_default(); + assert!( + screen.contains("READY"), + "precondition: the child's own output must reach the frame: {}", + report() + ); + + // Acceptance 1 — a quiet child must not produce a frame per tick. The + // bound is generous: the session legitimately emits a first frame, plus a + // frame for the geometry it settles at, plus the WINCH breadcrumb. + let frames: u32 = facts + .get("frames") + .and_then(|value| value.parse().ok()) + .unwrap_or_default(); + assert!( + (1..=12).contains(&frames), + "a quiet terminal must settle, got {frames} frames in {OBSERVE_MS} ms \ + (pre-fix: one per dispatcher tick): {}", + report() + ); + + // Acceptance 7 — bounded SIGWINCH, counted by the child itself through + // the real PTY. At most one resize is legitimate here (the frontend's + // first declaration); the probe requests none in quiet mode. + assert!( + !screen.contains("WINCH 3"), + "the child must not be signalled repeatedly: {}", + report() + ); +} + +/// Acceptance 4: a character typed through the real GPU attach client reaches +/// the child and its copy comes back in a rendered frame. +/// +/// **This is a keep-working pin, not a fix discriminator** — it passes on the +/// pre-fix tree too. Key transport was never the defect (falsified hypothesis +/// 2 in the framing), and this exists so that a future change to the routing +/// or transport cannot quietly break what the resize fix was not about. +#[cfg(feature = "crdt")] +#[test] +fn gpu_terminal_input_reaches_the_child_and_returns_in_a_frame() { + let Some(facts) = run_gpu_probe(CAT_PROBE_INIT_LUA, None) else { + return; + }; + let report = || format!("{facts:#?}"); + + assert_eq!( + facts.get("entered_terminal_mode").map(String::as_str), + Some("true"), + "precondition: terminal mode: {}", + report() + ); + let frames: u32 = facts + .get("frames") + .and_then(|value| value.parse().ok()) + .unwrap_or_default(); + assert!( + frames >= 1, + "precondition: the child ran and painted: {}", + report() + ); + // The probe types `x`; `cat` copies it back exactly once. The observation + // is LATCHED across frames rather than read off the last one: the probe + // also requests a geometry change, and a reflow rewrites the visible grid. + // "did the byte come back" and "is it still on screen at the end" are + // different questions, and only the first is about input reaching the + // child. + assert_eq!( + facts.get("input_echo_observed").map(String::as_str), + Some("true"), + "the typed character must reach the child and return: {}", + report() + ); +} From adf90e535185b5920846e0a3fba3effe629bf9fa Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 15:28:47 -0400 Subject: [PATCH 3/3] docs: record the GPU terminal input fix in the three required docs COHERENCE.md section 25 and the handoff/ledger update protocols make these ride the PR. COHERENCE.md: - Section 6 named one optimistic key classifier and attributed it to the GPU. There are two, one per replica frontend: crate::optimistic::classify_key belongs to the pmacs --attach TUI replica, and pmacs-gpu has its own unrelated optimistic_insert_text / optimistic_crdt_insert. The section's "kept honest by dispatch_idle_for" claim is confirmed for both, which this investigation verified rather than assumed. - Section 16 graded per-frontend degradation strong on the evidence of per-frontend fold projection. That grade stands, but the practice is enforced by convention rather than structure, and this defect is the counter-example; the note says so and points at what is now structural. - Section 2 step 8 records that the terminal was broken outright on the GPU frontend, not merely undiscoverable. docs/agent-handoff.md section 5 gains four lessons: adjacency does not make two operations alternatives (and two individually sound idempotence guards can be jointly useless); bite against every pre-image the fix could have taken, since the obvious guard here fixes the storm and introduces a controller leak; a quiet child is an instrument, because a frame storm hides inside a chatty fixture and a geometric readout is satisfied by an oscillating geometry; and TerminalMode::Raw makes sh-based input fixtures useless because there is no ICRNL. docs/active-work.md gains the lane entry with the branch, the bite matrix, the named out-of-scope items, and the gate results. --- COHERENCE.md | 25 +++++++++++++++++---- docs/active-work.md | 52 +++++++++++++++++++++++++++++++++++++++++++ docs/agent-handoff.md | 36 ++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 4 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index 954e621..dfe9cfd 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -363,7 +363,7 @@ Full verdict table: | 5 | Edit | **Works** | Full CUA + Emacs keymap in 161 lines (`builtin/keymaps/default.lua`); isearch, query-replace, kill ring, undo/redo, auto-indent/pair/comment, atomic save. Genuinely excellent zero-config | | 6 | Language intelligence | **Partial** | Rust grammar bundled and auto-attaches; rust-analyzer preconfigured (`builtin/runtime/lsp.lua:44-52`) — but a missing binary fails silently (§1.2) and highlighting masks it. No LSP status command exists to diagnose | | 7 | Find symbol / file | **File: missing → in flight (PR #162). Symbol: works but undiscoverable** | No find-file/dired/picker existed at audit; `M-.`/`M-?`/`C-c o` bound but advertised nowhere and server-gated; no workspace-symbol command; `pmacs.index.*` has no UI | -| 8 | Open terminal | **Works but undiscoverable** | Full PTY with scrollback + modeline segment — reachable only as `M-x terminal`, no keybinding | +| 8 | Open terminal | **Works but undiscoverable** | Full PTY with scrollback + modeline segment — reachable only as `M-x terminal`, no keybinding. *Was broken outright on the GPU frontend until the double terminal-layout sync was fixed: the child took a `SIGWINCH` storm at tick cadence, so typing into it was impossible while output still flowed.* | | 9 | Build / test | **Partial** | `M-x compile.run` works, defaults cwd to detected project root, parses Rust `-->` errors — but no keybinding, an **empty first prompt** (`initial = last and last.cmdline or ""`, `builtin/runtime/compile.lua:1134-1138`), and no `cargo build`/`cargo test` suggestion despite `ProjectKind::Cargo` existing (`src/project.rs:77`) | | 10 | Inspect error | **Partial (good once reached)** | `E:n W:n` modeline counts, underlines, `M-g n/p` + ``C-x ` `` walking a unified compile/grep/diag source, message echo, `RET` visits. Gated entirely on step 6 or 9 succeeding first | | 11 | See background work | **Works but undiscoverable** | `*workers*` view via `M-x editor.list-workers`; `C-c C-k` cancel-at-point. No keybinding, no statusline spinner/progress indicator anywhere (§9) | @@ -671,9 +671,18 @@ Facts that define the gap: optimistic-apply correctness), and `dispatch_paste` (`editor.rs:1129-1140`). - Off-path hardcodes: client-side **F12 detach** (`is_detach_key`, - `src/attach.rs:997-1006`) and the GPU **optimistic key classifier** - (`crate::optimistic::classify_key`) — the latter is classification, - not routing, and is kept honest by `dispatch_idle_for`. + `src/attach.rs:997-1006`) and the replica frontends' **optimistic key + classifiers** — classification, not routing, and kept honest by + `dispatch_idle_for`. There are **two, one per replica frontend**, and the + original audit named only one: `crate::optimistic::classify_key` belongs to + the **`pmacs --attach` TUI** replica (`src/attach.rs:843` is its only + consumer), while `pmacs-gpu` has its own, unrelated + `optimistic_insert_text` / `optimistic_crdt_insert` + (`pmacs-gpu/src/main.rs:2694`/`3306`). The "kept honest by + `dispatch_idle_for`" claim was **verified for both** while investigating the + GPU terminal input defect: a focused terminal buffer is in + `round_trip_buffers` (`src/terminal/session.rs:338`), so `dispatch_idle_for` + reports false and neither classifier can fire there. **The counter-example that proves the idiom:** the entire picker/panel family — listview (references, outline), project-search, buffer-list, @@ -1245,6 +1254,14 @@ its asks are already practiced.** selected from the negotiated `semantic_render` bit) so a grid frontend collapses folds while a simultaneous GPU session does not skip lines (#149/#148). + - **But it is enforced by convention, not by structure.** The GPU terminal + input defect was a per-frontend-kind operation applied to *both* kinds: + the dispatcher's grid and semantic terminal-layout syncs were written as + twins and executed as siblings, so a GPU session's PTY was resized twice + per tick forever. `sync_terminal_layouts_for_tick` now makes that one + exclusive by construction; every other per-frontend-kind pair in the + dispatcher remains two adjacent `if`s that a reader must notice are + alternatives. - The GPU frontend exceeds the TUI (minimap, squiggles, typography) without the TUI losing the model — the "no privileged frontend" rule is holding under real divergence pressure. diff --git a/docs/active-work.md b/docs/active-work.md index b426f38..1cff0f2 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -227,6 +227,58 @@ If it does not, stop and repair the remote/fetch configuration. buffer a directory should resolve *to*, and `pmacs .` should route into it rather than growing a second directory surface. +## GPU terminal input lane — IN REVIEW + +- Portable branch: `githubsucks/gpu-terminal-input`, worktree + `../pmacs-gui-term-input`, based on `githubsucks/main` @ `46a1b8f`. +- Approved framing: `docs/gpu-terminal-input-framing.md` revision 2, + committed as the branch's first commit (`9a0df21`). Bug fix, not a + feature; **no protocol change (stays v20)**. +- Reported as "text input within the terminal doesn't work on GUI, this is + fine in TUI". Root cause: the dispatcher applied **both** terminal-layout + syncs to **every** attached frontend, and a semantic session satisfies both + conditions (a `term_sizes` entry from `AttachRequest` *and* a terminal + declaration). Its PTY was resized twice per tick forever — grid arm installs + the TUI placement size, semantic arm installs the declared content + rectangle, each arm's idempotence guard seeing only what the other just + wrote — so the child took a `SIGWINCH` storm at tick cadence. +- **The fix is a split, not a guard.** The grid arm is also the only per-tick + controller-liveness release a semantic frontend gets, and + `sync_semantic_terminal_layout` cannot take that over: the buffer-follow + snapshot clears the viewport declaration (`on_buffer_snapshot_sent`), so + that arm stops running in exactly the switch-away case that needs the + release. `sync_terminal_layout` is therefore split into a + frontend-kind-neutral half (panel reconcile + liveness) and a grid-only + geometry half, with the loop body extracted to + `sync_terminal_layouts_for_tick` so the exclusivity is structural and tests + drive the real thing. +- **Trap for anyone touching this again:** the release at the "no + `window_placements` entry" arm reads like liveness and is grid geometry. A + semantic frontend has no placement entry at all, so moving it into the + neutral half releases a GPU controller every tick. +- Bite-verified against **two** pre-images, because the naive guard fixes the + storm and introduces the leak: + + | pin | `main` | naive guard | the split | + |---|---|---|---| + | settle (acc 2+3) | FAIL | pass | pass | + | controller release (acc 6) | pass | FAIL | pass | + | grid still resizes (acc 5) | pass | pass | pass | + +- Real-path evidence: a quiet child trapping `SIGWINCH` reports **144 frames + in 4 s and `WINCH 1..12` on screen** against the pre-fix tree, versus a + settled screen with the fix. +- **Deliberately out of scope, named:** interactive-shell echo on a raw-mode + PTY (Q#GT5 — reproduces in-process too, so it is not the GUI/TUI + asymmetry), and a geometry change appearing to clear the visible screen + (reproduces pre-fix; why acceptance 4 latches its observation across + frames). +- Verification on this branch: `cargo fmt --check` clean; strict workspace + Clippy clean; 1,829 default + 2,006 CRDT library tests; vterm Stage 1/2/3 + 10 / 6 / 9 CRDT; bottom-panel Stage 1 46; M4 121; required GPU 155; + **isolated-config workspace sweep 3,177 across 92 suites, zero failures**; + `git diff --check` clean. Gates were run against the committed tree. + ## Bottom-panel lane (window placement + side windows) — Stage 1 IN REVIEW - Portable branch: `githubsucks/bottom-panel`, worktree diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index a844230..d1f4943 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -704,6 +704,42 @@ final variant — its own round-trip cannot detect a discriminant shift. ## 5. Hard-won ops lessons +- **Two operations that must be alternatives are not made alternatives by + being adjacent.** The dispatcher applied its grid and semantic + terminal-layout syncs to every attached frontend; a semantic session + satisfies both conditions, so its PTY was resized twice per tick forever + and the child took a `SIGWINCH` storm that made a GPU terminal untypable + while output still flowed. Each arm had a correct `old_size == size` + idempotence guard — **individually sound, jointly useless**, because each + saw only the size the other had just written. Write mutually exclusive + per-frontend-kind work as one `if`/`else` keyed on the same fact session + establishment uses, and extract the loop body so a test can drive the real + thing. +- **Bite against every pre-image the fix could plausibly have taken, not just + `main`.** For the same defect, the obvious one-line guard (skip the grid arm + for semantic frontends) *does* fix the storm — and silently introduces a + controller leak, because that arm was also the only per-tick + controller-liveness release a semantic frontend got. A single revert would + have scored the fix complete. The pin that catches it (`acc 6`) deliberately + **passes on `main`** and fails only against the naive guard: today's defect + supplies the release by the accident of running an arm it should not. +- **A quiet child is an instrument.** A frame storm is invisible against a + fixture that legitimately emits hundreds of frames, and an assertion like + `frames >= 2` cannot see one. The same applies to geometry: a + "did a frame at the new width arrive" readout is satisfied by a geometry + *oscillating through* that width. Assert upper bounds over a fixed window + against a child that produces nothing, and let the child self-report the + signal you care about (a `SIGWINCH` trap printing a **fresh distinct** + breadcrumb per signal — repeated identical markers paint nothing, because + `cell::diff` skips already-matching cells). +- **`TerminalMode::Raw` makes `sh`-based input fixtures useless.** There is no + `ICRNL`, so Enter delivers CR and a `read -r` loop waits forever for a LF + that never comes — the test then "proves" input never arrived. Use + `exec cat`, which copies stdin to stdout byte by byte. It is also the right + echo instrument for the opposite reason people assume: termios `ECHO` is + *off* in raw mode, so nothing double-echoes and one keystroke yields exactly + one cell. + - **The checkout may be shared with the user.** Check `git status` for foreign uncommitted work before any stash/checkout/branch surgery; never assume dirty files are yours. (Their uncommitted fix was nearly