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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VoiEyuPjoBhvwACf8HAnLB
This commit is contained in:
Levi Neuwirth 2026-07-14 19:36:31 +01:00
parent c7fca92fe9
commit 3083458cb0
6 changed files with 304 additions and 21 deletions

View File

@ -1,7 +1,35 @@
# Theme faces — framing (Arc 4 stage 1, themes)
**Revision 4 — 2026-07-14. Status: awaiting approval; folds framing
rounds 13.**
**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 14): 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 17):
`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<Vec<(String, Style)>>`, 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<u64>` 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.

View File

@ -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<CellStyle> {
(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

View File

@ -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);

View File

@ -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<Style> {
let fg: mlua::Value = t.get("fg").unwrap_or(mlua::Value::Nil);
let bg: mlua::Value = t.get("bg").unwrap_or(mlua::Value::Nil);
let underline: mlua::Value = t.get("underline").unwrap_or(mlua::Value::Nil);
let underline_color: mlua::Value = t.get("underline_color").unwrap_or(mlua::Value::Nil);
let fg: mlua::Value = t.get("fg")?;
let bg: mlua::Value = t.get("bg")?;
let underline: mlua::Value = t.get("underline")?;
let underline_color: mlua::Value = t.get("underline_color")?;
Ok(Style {
fg: lua_to_color(&fg)?,
bg: lua_to_color(&bg)?,
bold: t.get("bold").unwrap_or(false),
italic: t.get("italic").unwrap_or(false),
bold: t.get::<Option<bool>>("bold")?.unwrap_or(false),
italic: t.get::<Option<bool>>("italic")?.unwrap_or(false),
underline: lua_to_underline(&underline)?,
reverse: t.get("reverse").unwrap_or(false),
reverse: t.get::<Option<bool>>("reverse")?.unwrap_or(false),
underline_color: lua_to_color(&underline_color)?,
})
}

View File

@ -222,6 +222,15 @@ pub struct SemanticRenderState {
/// viewport declaration. A frontend retaining face state across
/// attachments is therefore corrected even by an unthemed daemon.
last_theme_faces: Option<Vec<crate::protocol::ThemeFace>>,
/// Whether the peer negotiated protocol >= 16 (PR #120 round 1
/// finding 3). Faces reach a semantic frontend through TWO
/// channels: `ThemeFacts` (daemon write-loop gated) and the
/// `ui.diag.*` colors folded into `FileStyleSummary` — an OLDER
/// channel the version gate does not filter. A v15 peer must not
/// receive face-derived minimap marks while its squiggles, signs,
/// and counters stay unthemed, so this producer resolves faces
/// only when the peer can apply the whole face table.
peer_knows_theme_facts: bool,
/// Cached byte↔line table for the diagnostics projection, keyed
/// by buffer revision. Building it costs an O(buffer) rope copy
/// plus a full scan; before this cache, that ran on *every tick*
@ -313,8 +322,22 @@ impl StyleGate {
}
impl SemanticRenderState {
/// Fresh session state for a peer that negotiated
/// `negotiated_protocol_version` — the real daemon construction
/// path (PR #120 round 1 finding 3): a `< 16` peer gets no
/// `ThemeFacts` produced at all and, crucially, no face-derived
/// colors folded into its `FileStyleSummary` marks.
#[must_use]
pub fn for_peer(frontend_id: FrontendId, negotiated_protocol_version: u32) -> Self {
let mut s = Self::new(frontend_id);
s.peer_knows_theme_facts = negotiated_protocol_version >= 16;
s
}
/// Fresh session state for frontend `frontend_id`: no viewport
/// declared, nothing sent.
/// declared, nothing sent. Assumes a current-build peer (>= 16);
/// daemon sessions with a real negotiated version use
/// [`Self::for_peer`].
#[must_use]
pub fn new(frontend_id: FrontendId) -> Self {
Self {
@ -341,6 +364,7 @@ impl SemanticRenderState {
// an epoch-0 daemon before that send.
last_face_epoch: None,
last_theme_faces: None,
peer_knows_theme_facts: true,
diag_line_cache: HashMap::new(),
}
}
@ -990,7 +1014,16 @@ impl SemanticRenderState {
// `ui.diag.*` feeds the marks.
let diag_epoch = diagnostics_epoch(state, buffer_id);
let (syntax_epoch, face_epoch) = theme_epochs(state);
let key = (generation, diag_epoch, syntax_epoch, face_epoch);
// A v15 peer's marks never resolve faces (finding 3), so a
// face mutation cannot change its summary either — zero the
// key component rather than recompute a whole-file pass per
// face edit just to payload-suppress it.
let face_key = if self.peer_knows_theme_facts {
face_epoch
} else {
0
};
let key = (generation, diag_epoch, syntax_epoch, face_key);
if self
.last_summary
.get(&buffer_id)
@ -998,7 +1031,7 @@ impl SemanticRenderState {
{
return None;
}
let lines = scoped_file_summary(state, buffer_id);
let lines = scoped_file_summary(state, buffer_id, self.peer_knows_theme_facts);
// Payload-equality suppression (Q#TH6): a face edit that
// leaves the summary unchanged (e.g. `ui.modeline`) emits
// nothing — but the key still advances, on computation rather
@ -1036,6 +1069,12 @@ impl SemanticRenderState {
/// `None`, so every attachment ships exactly one authoritative
/// table — the empty table included — on its first frame.
fn theme_facts_msg(&mut self, state: &EditorState) -> Option<InstanceMessage> {
// PR #120 round 1 finding 3: never even produced for a peer
// below v16 (the daemon write-loop gate remains as the
// belt-and-braces filter).
if !self.peer_knows_theme_facts {
return None;
}
let theme = state.syntax_registry.theme();
let (faces, face_epoch) = {
let th = theme.lock().expect("theme mutex poisoned");
@ -1815,7 +1854,11 @@ fn lsp_scoped_style_spans(state: &EditorState, vp: &DeclaredViewport) -> Vec<Sty
/// `O(spans × lines)` in the worst case; the caller short-circuits on
/// unchanged CRDT generation so this only runs on first sight of a
/// buffer or after an edit, not per frame.
fn scoped_file_summary(state: &EditorState, buffer_id: BufferId) -> Vec<Style> {
fn scoped_file_summary(
state: &EditorState,
buffer_id: BufferId,
resolve_faces: bool,
) -> Vec<Style> {
let source = {
let core = state.core.borrow();
let registry = core.registry.clone();
@ -1846,7 +1889,7 @@ fn scoped_file_summary(state: &EditorState, buffer_id: BufferId) -> Vec<Style> {
if spans.is_empty() {
// No styled runs — but diagnostic marks are independent of
// syntax styling (a plain-text buffer can still have lints).
overlay_diagnostic_marks(state, buffer_id, &mut out);
overlay_diagnostic_marks(state, buffer_id, &mut out, resolve_faces);
return out;
}
@ -1879,7 +1922,7 @@ fn scoped_file_summary(state: &EditorState, buffer_id: BufferId) -> Vec<Style> {
*line_dominant = winner.0;
}
}
overlay_diagnostic_marks(state, buffer_id, &mut out);
overlay_diagnostic_marks(state, buffer_id, &mut out, resolve_faces);
out
}
@ -1891,7 +1934,12 @@ fn scoped_file_summary(state: &EditorState, buffer_id: BufferId) -> Vec<Style> {
/// describe pre-edit text, same discipline as the decorations
/// producer (the marks return on republish, which bumps the diag
/// epoch and recomputes this summary).
fn overlay_diagnostic_marks(state: &EditorState, buffer_id: BufferId, lines: &mut [Style]) {
fn overlay_diagnostic_marks(
state: &EditorState,
buffer_id: BufferId,
lines: &mut [Style],
resolve_faces: bool,
) {
let uri = {
let core = state.core.borrow();
let Some(uri) = buffer_file_uri(&core, buffer_id) else {
@ -1919,14 +1967,18 @@ fn overlay_diagnostic_marks(state: &EditorState, buffer_id: BufferId, lines: &mu
// `ui.diag.*` faces reach the minimap through this summary. The
// diag `Default`-fg policy guarantees a diagnosed line never
// writes `Default` here, which the GPU reads as "no mark".
let theme = {
// `resolve_faces` is false for a peer below v16 (PR #120 round 1
// finding 3): this summary is an ungated pre-v16 channel, and a
// v15 frontend must not get face-derived marks on one surface
// while every other severity surface stays unthemed.
let theme = resolve_faces.then(|| {
let handle = state.syntax_registry.theme();
let t = handle.lock().expect("theme mutex poisoned");
t.clone()
};
});
for (line, severity) in lines.iter_mut().zip(best) {
if let Some(s) = severity {
line.underline_color = crate::diag::severity_color(Some(&theme), s);
line.underline_color = crate::diag::severity_color(theme.as_ref(), s);
}
}
}

View File

@ -735,6 +735,98 @@ fn malformed_merge_is_atomic_from_lua() {
);
}
#[test]
fn raising_index_metamethods_fail_the_merge_transactionally() {
// PR #120 round 1 finding 2: `Table::get` runs __index, so a
// raising metatable must error the whole merge. Pre-fix, the
// trapped lookup silently parsed as an all-default style and the
// merge SUCCEEDED, committing the valid sibling — against the
// Q#TH6 all-or-nothing contract.
let mut state = editor();
type_str(&mut state, "hello");
let mut sem = semantic(&state);
let _ = sem.render_frame(&state);
let err = exec_err(
&state,
r#"
local trap = setmetatable({}, { __index = function() error("trapdoor") end })
pmacs.theme.merge { ["ui.gutter"] = trap, zebra = { fg = 42 } }
"#,
);
assert!(
format!("{err}").contains("trapdoor"),
"the metatable's own error surfaces: {err}"
);
// `zebra` is not in default_dark, so its presence would prove the
// valid sibling leaked through the failed merge.
let (zebra, gutter): (bool, bool) = eval(
&state,
r#"
local t = pmacs.theme.current()
return t["zebra"] ~= nil, t["ui.gutter"] ~= nil
"#,
);
assert!(
!zebra && !gutter,
"nothing from the failed merge landed (zebra={zebra}, gutter={gutter})"
);
let frame = sem.render_frame(&state);
assert!(
theme_facts_of(&frame).is_none() && !has_style_spans(&frame),
"a failed merge bumps nothing and emits nothing"
);
// (Boolean fields deliberately follow Lua truthiness — mlua's
// bool conversion — so `reverse = "yes"` is Some(true), not an
// error; the transactional contract is about lookups that RAISE.)
}
#[test]
fn v15_peers_get_no_face_derived_summary_marks() {
// PR #120 round 1 finding 3: ui.diag.* colors reach the minimap
// through FileStyleSummary — an ungated pre-v16 channel. A v15
// semantic peer must keep built-in marks (its squiggles, signs,
// and counters are unthemed too); a v16 peer gets the face.
use pmacs::diag::DiagnosticSeverity;
let mut state = editor();
type_str(&mut state, "boom\nfine\n");
attach_diags(&state, vec![diag(DiagnosticSeverity::Error)]);
exec(
&state,
r#"pmacs.theme.merge { ["ui.diag.error"] = { fg = 45 } }"#,
);
let buffer_id = active_buffer(&state);
let viewport = ByteRange {
start: 0,
end: 1 << 20,
};
let mut v15 = SemanticRenderState::for_peer(FrontendId::LOCAL, 15);
v15.set_viewport(buffer_id, viewport, 0);
let frame = v15.render_frame(&state);
let lines = summary_of(&frame).expect("v15 still receives the summary");
assert_eq!(
lines[0].underline_color,
Color::Indexed(1),
"a v15 peer's marks keep the built-in severity color"
);
assert!(
theme_facts_of(&frame).is_none(),
"and no ThemeFacts is produced for it at all"
);
let mut v16 = SemanticRenderState::for_peer(FrontendId::LOCAL, 16);
v16.set_viewport(buffer_id, viewport, 0);
let frame = v16.render_frame(&state);
assert_eq!(
summary_of(&frame).expect("summary ships")[0].underline_color,
Color::Indexed(45),
"a v16 peer's marks resolve the face"
);
assert!(theme_facts_of(&frame).is_some());
}
// ---------------------------------------------------------------------------
// 12 + 16 + 19 — ThemeFacts emission discipline; late join; set wipes
// ---------------------------------------------------------------------------