From 3083458cb09866c8bd32f2cefc6c1ef478c2751a Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 14 Jul 2026 19:36:31 +0100 Subject: [PATCH] fix(themes): PR #120 round 1 -- minimap cache, __index holes, v15 face leak Finding 1: accepting a FileStyleSummary drops the GPU minimap vertex cache -- theme recolors and diagnostic republishes arrive at an unchanged generation, and the cache keys only on (generation, dims, scroll), so stale strokes survived until an edit/resize/scroll. The daemon payload-suppresses identical summaries, so the invalidation is precise. GPU test drives two same-generation summaries; hand-bitten by reverting the single invalidation line (script-bite is vacuous here: fix and test share main.rs). Finding 2: lua_to_style propagates every Table::get error -- the lookups run __index, so a raising metatable previously parsed as an all-default style and the merge SUCCEEDED, committing valid siblings against the Q#TH6 all-or-nothing contract. Boolean fields keep Lua truthiness by design (mlua bool), so only raising lookups fail the transaction. Acceptance reproduces the reviewer's trap shape; runtime bite vs pre-fix mod.rs. Finding 3: SemanticRenderState::for_peer records the negotiated version; below v16 no ThemeFacts is produced and no ui.diag.* face folds into the FileStyleSummary marks -- the summary is an ungated pre-v16 channel, and a v15 peer must not get face-derived minimap colors while its other severity surfaces stay unthemed. The summary cache key zeroes its face-epoch component for such peers. Acceptance drives v15/v16 producers side by side; compile-fail bite disclosed (the test needs for_peer, absent pre-fix). Finding 4: framing revision 5 weakens the canonical-severity claim to what is true -- the daemon-RESOLVED color is canonical, while the GPU's built-in squiggle/sign/counter defaults are historical bright RGBs that differ from the minimap's converted Indexed marks, a pre-existing divergence kept because unset faces must render byte-identically to before. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VoiEyuPjoBhvwACf8HAnLB --- docs/theme-faces-framing.md | 76 +++++++++++++++++++++++++-- pmacs-gpu/src/main.rs | 51 ++++++++++++++++++ src/daemon.rs | 12 ++++- src/lua_bindings/mod.rs | 22 +++++--- src/semantic_render.rs | 72 ++++++++++++++++++++++---- tests/theme_faces_acceptance.rs | 92 +++++++++++++++++++++++++++++++++ 6 files changed, 304 insertions(+), 21 deletions(-) diff --git a/docs/theme-faces-framing.md b/docs/theme-faces-framing.md index b1bfef8..7674b55 100644 --- a/docs/theme-faces-framing.md +++ b/docs/theme-faces-framing.md @@ -1,7 +1,35 @@ # Theme faces — framing (Arc 4 stage 1, themes) -**Revision 4 — 2026-07-14. Status: awaiting approval; folds framing -rounds 1–3.** +**Revision 5 — 2026-07-14. Status: implemented on branch +`theme-faces` (PR #120); revision 5 folds PR round 1.** + +Revision 5 (PR #120 round 1, findings 1–4): accepting a +`FileStyleSummary` on the GPU now drops the cached minimap vertices — +theme recolors and diagnostic republishes arrive at an UNCHANGED +generation, and the cache keys only on (generation, dims, scroll), so +the stale strokes survived until an edit/resize/scroll; the daemon's +payload-equality suppression makes the invalidation precise +(finding 1). `lua_to_style` propagates every `Table::get` error: the +lookups run `__index`, so a raising metatable previously parsed as an +all-default style and the merge SUCCEEDED, committing its valid +siblings against the Q#TH6 all-or-nothing contract; boolean fields +deliberately keep Lua truthiness (mlua's `bool`), so only lookups +that RAISE fail the transaction (finding 2). The producer is +peer-version-aware: `SemanticRenderState::for_peer` records whether +the peer negotiated >= 16, and below that no `ThemeFacts` is produced +at all AND no `ui.diag.*` face is folded into the `FileStyleSummary` +marks — that summary is an ungated pre-v16 channel, and a v15 peer +must not get face-derived minimap colors while its squiggles, signs, +and counters stay unthemed; the summary's cache key zeroes its +face-epoch component for such peers so face edits don't trigger +pointless whole-file recomputes (finding 3). And the "canonical +severity color" claim is weakened to what is true: the +daemon-RESOLVED color is canonical (TUI squiggle/marker/minimap and, +when a face is set, all GPU surfaces), but the GPU's BUILT-IN +defaults — bright RGB constants for squiggles/signs/counters vs the +minimap's converted `Indexed(1/3/6/8)` — are a pre-existing +divergence this arc keeps, because unset faces must render +byte-identically to before (finding 4). Revision 4 (framing round 3, findings 1–7): `ui.minibuffer.candidate` now has a real GPU site: its `fg` colors the @@ -410,6 +438,14 @@ counterpart. The unified rule: built-in error color on every surface (squiggle, sign, minimap, band counter). It is not absent: an exact empty child stops Q#TH4's walk, so it overrides an inherited `ui.diag` color with the built-in. + Scope of "built-in" (round 1 finding 4): a SET face with a concrete + color unifies all surfaces on both frontends, but the per-surface + built-in DEFAULTS keep their pre-existing values — the TUI and the + wire summary use `Indexed(1/3/6/8)`, while the GPU's squiggle, + sign, and counter constants are historical bright RGBs that differ + from the minimap's converted `Indexed` marks. That divergence + predates this arc and stays, because unset faces must render + byte-identically to before. Residual divergence, accepted and tested: `Indexed` colors mean the user's terminal palette on the TUI and the `indexed_to_glyphon` @@ -427,7 +463,11 @@ the four Lua bindings — the only mid-session mutation paths: `mlua::Result>`, and only after `Ok(Vec)` acquires the theme mutex and commits in the requested mode. The Lua table iterator maps each raw `(name, style_table)` entry through - `lua_to_style` into that result stream. `default` parses its one + `lua_to_style` into that result stream — whose own lookups + PROPAGATE errors (round 1 finding 2): `Table::get` runs `__index`, + so a raising metatable fails the transaction rather than silently + parsing as an all-default style; boolean fields keep Lua + truthiness by design. `default` parses its one `Style` before locking; `clear` has no input to parse. `set` already parses into a scratch value pre-lock; `merge` today inserts while iterating under the lock (`mod.rs:6925-6931`), so a @@ -497,6 +537,16 @@ pub struct ThemeFace { logic. The daemon owns face semantics; frontends own pixels (Q#UX1). No epoch on the wire — the payload is self-contained and the house facts style carries no counters. +- **The producer is peer-version-aware** (round 1 finding 3): + daemon sessions construct via `SemanticRenderState::for_peer`, and + below v16 no `ThemeFacts` is produced at all — more importantly, + no `ui.diag.*` face is folded into the `FileStyleSummary` marks, + because that summary is an ungated pre-v16 channel and a v15 peer + must not receive face-derived minimap colors while its squiggles, + signs, and counters stay unthemed. The write-loop gate remains as + the belt-and-braces filter. The summary cache key zeroes its + face-epoch component for such peers (no whole-file recompute per + face edit that could never change their marks). - **Producer**: `theme_facts_msg` on `SemanticRenderState`, called from the `render_frame` emission list. Per-tick cheapness comes from a `last_face_epoch: Option` gate **seeded `None`** — @@ -552,6 +602,14 @@ are unchanged (`main.rs:3502-3546`) — without clearing those cached comparison strings, a diag-face change with constant counts keeps stale counter colors indefinitely. +A newly accepted `FileStyleSummary` drops the cached minimap +vertices (round 1 finding 1): theme recolors and diagnostic +republishes ship a new summary at an UNCHANGED generation, and the +minimap cache keys only on (generation, dims, scroll) — without the +explicit invalidation the stale strokes survive until an edit, +resize, or scroll. The daemon payload-suppresses identical +summaries, so every accepted one is genuinely new. + Each themed site resolves per draw with the Q#TH5 rule: face absent → today's site constant; face present → the face's **in-mask** components, with `Default` fg ↦ the buffer-text default, `Default` @@ -816,3 +874,15 @@ Keybinding-driven tests dispatch keys, never `pmacs.command.invoke`. 24. **Lua surface unchanged**: `pmacs.theme.current()` lists both bare `ui` and `ui.*` entries alongside captures; `get("ui.modeline")` returns the set style (existing lookup semantics). +25. **Raising `__index` is transactional** (PR round 1 finding 2): a + merge whose style value carries a raising `__index` metatable + errors with the metatable's own message, commits nothing — + including its valid sibling — and emits nothing. +26. **v15 peers get no face-derived summary marks** (PR round 1 + finding 3): with a diag face set, a `for_peer(…, 15)` producer + ships summary marks at the built-in severity color and produces + no `ThemeFacts`; the `for_peer(…, 16)` twin resolves the face. +27. **Same-generation summary updates repaint the GPU minimap** + (PR round 1 finding 1, `PMACS_REQUIRE_GPU=1`): two summaries at + one generation with different mark colors render different + frames — fails without the cache invalidation on accept. diff --git a/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index 9c8261a..cd274ce 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -4095,6 +4095,15 @@ impl State { } self.current_line_shapes = minimap_line_shapes(&self.current_text); self.current_summary = Some(FileStyleSummaryState { generation, lines }); + // PR #120 round 1 finding 1: a newly accepted summary can + // arrive at an UNCHANGED generation (theme recolor, + // diagnostic republish) and the minimap cache keys only on + // (generation, dims, scroll) — without an explicit drop the + // stale vertices survive until an edit, resize, or scroll. + // The daemon payload-suppresses identical summaries, so every + // summary accepted here is genuinely new and the invalidation + // is precise. + self.minimap_cache = None; self.request_redraw(); } @@ -8659,6 +8668,48 @@ mod tests { ); } + #[test] + fn headless_same_generation_summary_update_repaints_the_minimap() { + // PR #120 round 1 finding 1: theme recolors and diagnostic + // republishes ship a NEW summary at the SAME CRDT generation, + // and the minimap cache keys only on (generation, dims, + // scroll) — accepting a summary must drop the cached vertices + // or the stale strokes survive until an edit/resize/scroll. + let text = "alpha\nbeta\ngamma\ndelta\n"; + let (w, h) = (400u32, 300u32); + let Some(mut state) = headless_or_skip(w, h, text) else { + return; + }; + let bid = BufferId::next(); + state.current_buffer_id = Some(bid); + state.view_range = (0, text.len() as u64); + let summary = |color: CellColor| -> Vec { + (0..4) + .map(|_| CellStyle { + fg: color, + ..CellStyle::default() + }) + .collect() + }; + let _ = state.apply_attach_message(InstanceMessage::FileStyleSummary { + buffer_id: bid, + generation: 7, + lines: summary(CellColor::Rgb(200, 40, 40)), + }); + let first = state.render_offscreen(); + // Same generation, different colors — a theme-recolor twin. + let _ = state.apply_attach_message(InstanceMessage::FileStyleSummary { + buffer_id: bid, + generation: 7, + lines: summary(CellColor::Rgb(40, 200, 40)), + }); + let second = state.render_offscreen(); + assert_ne!( + first, second, + "a same-generation summary recolor must repaint the minimap" + ); + } + #[test] fn own_wash_faces_color_local_rects_peers_keep_the_constant() { // Acceptance 21 + 23: decode the emitted decoration vertex diff --git a/src/daemon.rs b/src/daemon.rs index 4af5da9..b84d3fb 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1375,6 +1375,9 @@ fn handle_session_established( // fires: the semantic frontend holds the rope locally and the // semantic frame ships no text. let semantic_render = session_state.negotiated_capabilities.semantic_render; + // Captured before `register_session` consumes the state: the + // semantic producer needs the peer's version (finding 3 below). + let negotiated_protocol_version = session_state.negotiated_protocol_version; if crdt_replica { send_buffer_snapshots(editor, &mut write_stream); } @@ -1386,7 +1389,14 @@ fn handle_session_established( if semantic_render { semantic_states.insert( frontend_id, - crate::semantic_render::SemanticRenderState::new(frontend_id), + // for_peer, not new (PR #120 round 1 finding 3): a v15 + // peer's producer must not resolve faces into the + // FileStyleSummary marks — that channel predates the v16 + // gate. + crate::semantic_render::SemanticRenderState::for_peer( + frontend_id, + negotiated_protocol_version, + ), ); } else { let mut render_state = RenderState::new(initial_size); diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 974a2b5..b153ffd 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -6862,18 +6862,26 @@ fn underline_to_lua(style: UnderlineStyle) -> &'static str { /// default to the same values as `Style::default()` --- a Lua table /// with `{ bold = true }` produces a style that is otherwise /// terminal-default. +/// +/// Every lookup PROPAGATES its error (PR #120 round 1 finding 2): +/// `Table::get` runs `__index`, so a raising metatable must fail the +/// enclosing transactional mutation (Q#TH6 all-or-nothing), not +/// silently parse as an all-default style and let sibling entries +/// commit. Color/underline fields validate strictly through their +/// converters; the boolean fields follow Lua truthiness (mlua's +/// `bool` conversion), so `reverse = 1` reads as `true` by design. fn lua_to_style(t: &Table) -> mlua::Result