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 9ff342d..c2a597c 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -317,6 +317,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 diff --git a/docs/gpu-terminal-input-framing.md b/docs/gpu-terminal-input-framing.md new file mode 100644 index 0000000..0bbccef --- /dev/null +++ b/docs/gpu-terminal-input-framing.md @@ -0,0 +1,438 @@ +# 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. +- **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. +- 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 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 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. + +## 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. diff --git a/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index eca19e5..a26f340 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -766,7 +766,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 { @@ -816,13 +831,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() @@ -838,7 +857,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; } } @@ -870,6 +889,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!( @@ -1075,9 +1095,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() + ); +}