From 623bd884afa05973ba93ecb3ce44a67cd2fa5f54 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 3 Jul 2026 14:38:19 -0400 Subject: [PATCH] =?UTF-8?q?fix(gpu):=20F-004=20AltGr=20strip=20=E2=86=92?= =?UTF-8?q?=20Ctrl+Alt=20only;=20run=20pmacs-protocol=20tests=20in=20CI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups on this session's audit work, both on freshly-merged code. F-004 hardening (regression fix). The AltGr text-input strip (`is_layout_text`) gated on *any* command modifier, so it fired on Alt-alone. On macOS the Option key is reported as Alt and emits printable text for most letters (Option+x → "≈"), so every GUI Meta binding (M-x, M-f, M-b, …) was stripped to a self-insert. Tighten the gate to the true AltGr signature — both Ctrl and Alt (the LCtrl+RAlt the OS synthesizes on Windows) — so Alt-alone forwards as a Meta chord again while Windows AltGr still inserts. Strict narrowing of when we strip: no-op on Linux/Windows, unblocks macOS Option-as-Meta. Test flips the Alt-alone € assertion and adds the macOS Option+x case. CI coverage (F-001 residue). `workspace_default_members` is only the root `pmacs` package, so `cargo test` skipped pmacs-protocol — the shared wire format the daemon, TUI, and GPU all depend on. Add `cargo test -p pmacs-protocol --all-targets` to the test job (its ~12 encode/decode + transport-framing tests). All three first-party crates now run in CI (root pmacs, pmacs-gpu via the render job, now pmacs-protocol). Validated: fmt clean; clippy -p pmacs-gpu --all-targets clean; pmacs-gpu tests 42 pass (render tests on the local adapter, PMACS_REQUIRE_GPU=1); pmacs-protocol 12 pass. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_014TXbAwk27agwhrNNrhLi2U --- .github/workflows/ci.yml | 7 ++++ docs/repository-audit-2026-07-03.md | 22 +++++++++++++ pmacs-gpu/src/main.rs | 50 +++++++++++++++++++---------- 3 files changed, 62 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4bce42f..8fbad57 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -92,6 +92,13 @@ jobs: # to the behavior under test. - run: cargo test --all-targets --no-default-features --features ${{ matrix.lua }} -- --test-threads=1 - run: cargo test --doc --no-default-features --features ${{ matrix.lua }} + # The workspace default member is only the root `pmacs` package, so + # the runs above never execute pmacs-protocol's own tests — the + # shared wire format (encode/decode, transport framing) the daemon, + # TUI, and GPU all depend on. Run them explicitly (Lua-flavor + # independent, so once is enough; runs on every matrix leg cheaply + # off the shared cache). + - run: cargo test -p pmacs-protocol --all-targets acceptance: name: M1 Acceptance Gates diff --git a/docs/repository-audit-2026-07-03.md b/docs/repository-audit-2026-07-03.md index 99e9770..e860b52 100644 --- a/docs/repository-audit-2026-07-03.md +++ b/docs/repository-audit-2026-07-03.md @@ -78,6 +78,15 @@ commands, e.g. `cargo clippy --workspace --all-targets -- -D warnings` and `default-members` if the intent is for plain `cargo test` and `cargo clippy` to cover all first-party crates. +Resolution (PR #75, then extended): the three `pmacs-gpu` lints were fixed and +CI now runs `cargo clippy -p pmacs-gpu --all-targets` (README uses `--workspace`). +That closed the *clippy* blind spot but not the symmetric *test* one — the +default-member scope also means `cargo test` skips `pmacs-protocol`. F-014's +GPU-render job later ran `pmacs-gpu`'s tests; this change adds +`cargo test -p pmacs-protocol --all-targets` to the test job so the shared wire +format (its ~12 encode/decode + transport-framing tests) is finally exercised in +CI. All three first-party crates now run in CI. + ### F-002 - Medium - `--all-features` is not buildable because Lua features are mutually exclusive Evidence: @@ -153,6 +162,19 @@ text input from command chords. Add tests for AltGr-style printable input. If th wire protocol needs it, add an AltGraph modifier bit rather than folding it into Ctrl/Alt. +Resolution (PR #76, then hardened): a keypress that produced printable +`key.text` while a command modifier was held is reclassified as text input and +the modifiers are stripped (`is_layout_text`). The first cut gated on *any* +command modifier, which over-reached: on macOS the `Option` key is reported as +`Alt` and emits printable text for most letters, so `Option+x` was stripped to a +plain insert — swallowing every GUI Meta binding (`M-x`, `M-f`, …) on that +platform. The gate is now the true AltGr signature — **both `Ctrl` and `Alt`** +(the LCtrl+RAlt the OS synthesizes for AltGr on Windows) — so `Alt`-alone forwards +as a Meta chord again while Windows AltGr still inserts. A protocol AltGraph bit +was *not* added; the text-presence heuristic is sufficient and layout-agnostic. +Not locally validatable on macOS (no box) — a narrowing of when we strip, so +low-risk on Linux/Windows. + ### F-005 - Medium - Package install paths collide for namespaced packages with the same basename Evidence: diff --git a/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index c1f0188..fb5017d 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -800,13 +800,14 @@ impl ApplicationHandler for App { // AltGr / international text (audit F-004). winit reports // the text a keypress produces; when a keypress yields - // printable text *while* Ctrl/Alt is held — AltGr is - // Ctrl+Alt on Windows and some layouts — it's text input, - // not a command chord. Strip the Ctrl/Alt (keep Shift) so - // it inserts (through the plain-text path, or the daemon's - // SelfInsert while a prompt is open) instead of being - // routed to the keymap. On platforms where AltGr isn't - // Ctrl/Alt this is a no-op (the chord was already plain). + // printable text *while both Ctrl and Alt* are held — the + // AltGr signature on Windows (LCtrl+RAlt) — it's text + // input, not a command chord. Strip those modifiers (keep + // Shift) so it inserts (through the plain-text path, or the + // daemon's SelfInsert while a prompt is open) instead of + // being routed to the keymap. Alt alone is left intact so + // macOS Option-as-Meta still reaches the keymap; on layouts + // where AltGr isn't Ctrl+Alt this is a no-op. if matches!(pkey, ProtocolKey::Char(_)) && is_layout_text(key.text.as_deref(), pmods) { @@ -4982,16 +4983,25 @@ fn is_command_chord(key: ProtocolKey, mods: Modifiers) -> bool { ) && (mods.contains(Modifiers::CTRL) || mods.contains(Modifiers::ALT)) } -/// Whether a keypress is **layout text input carried under a command -/// modifier** — the `AltGr` case (audit F-004): winit produced printable -/// `text` while `Ctrl`/`Alt` is held. On layouts/platforms where `AltGr` -/// is `Ctrl+Alt` (Windows), the produced character would otherwise be -/// misclassified as a command chord; when this is true the caller strips -/// the command modifiers so it inserts. Returns `false` for genuine -/// command chords (which produce no text, or a control char) and for -/// plain text (no command modifier — already handled). +/// Whether a keypress is **`AltGr` layout text** (audit F-004): winit +/// produced printable `text` while **both `Ctrl` and `Alt`** are held. +/// `AltGr` is `Ctrl+Alt` on Windows (the OS synthesizes LCtrl+RAlt), and +/// on such layouts the produced character (`@`, `€`, `{`, …) would +/// otherwise be misclassified as a command chord; when this is true the +/// caller strips the command modifiers so it inserts. +/// +/// The gate is deliberately `Ctrl+Alt`, **not** "any command modifier": +/// `Alt` alone is *not* `AltGr`. On macOS the `Option` key is reported as +/// `Alt` and produces printable text for most letters (`Option+x` → "≈"), +/// but Option-as-Meta is exactly how the GUI reaches `M-x` / `M-f` / … — +/// stripping `Alt`-alone would swallow every macOS Meta chord. Genuine +/// `AltGr` needs both modifiers, so requiring both leaves `Alt`-alone +/// (macOS `Option`, plain `Meta`) to forward as command chords. Returns +/// `false` for genuine command chords (no text, or a control char) and +/// for plain text (no command modifier — already handled). fn is_layout_text(text: Option<&str>, mods: Modifiers) -> bool { - !is_plain_text_modifiers(mods) + mods.contains(Modifiers::CTRL) + && mods.contains(Modifiers::ALT) && text.is_some_and(|t| !t.is_empty() && t.chars().all(|c| !c.is_control())) } @@ -6074,8 +6084,14 @@ mod tests { // text; that's text input, so the caller strips the modifiers. let ctrl_alt = Modifiers::CTRL | Modifiers::ALT; assert!(is_layout_text(Some("@"), ctrl_alt)); - assert!(is_layout_text(Some("€"), Modifiers::ALT)); assert!(is_layout_text(Some("{"), ctrl_alt)); + assert!(is_layout_text(Some("€"), ctrl_alt)); // AltGr+e on many layouts + // Alt ALONE is not AltGr. On macOS the Option key is Alt and emits + // printable text (Option+x → "≈"), but Option-as-Meta is how the + // GUI reaches M-x / M-f — leave it intact so it forwards as a + // command chord instead of self-inserting the symbol. + assert!(!is_layout_text(Some("≈"), Modifiers::ALT)); // macOS Option+x → M-x + assert!(!is_layout_text(Some("€"), Modifiers::ALT)); // Genuine command chords produce no text (or a control char) — // not layout text, so they still route to the keymap. assert!(!is_layout_text(None, Modifiers::CTRL)); // C-a etc.