From baad7e12fb0b2cf2223a042c32dd2221c77417ff Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Thu, 14 May 2026 18:34:36 +0800 Subject: [PATCH] fix(variables): set_color_hex mirrors resolve's subset matching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex stop-gate caught a real correctness bug in the previous commit (62653659): `set_color_hex` used EQUALITY to match themed entries against `active_theme`, but `Variable::resolve` uses SUBSET matching. The mismatch let writes report success without actually changing the resolved value: Entry: {theme: Some({"mode": "dark"}), value: "#111"} Active: {"mode": "dark", "density": "compact"} Before: equality check fails (entry's theme has fewer keys), so the write pushes a new entry at the end: {theme: Some({"mode": "dark", "density": "compact"}), value: NEW} resolve() walks first-to-last, picks the ORIGINAL entry (mode= dark is a subset of active), returns "#111" — the new entry is shadowed and never read. Fix: `set_color_hex` now mirrors `resolve` exactly. Three-step lookup matching the resolve walk order: 1. First themed entry whose `theme` map is a subset of active_theme → write there. 2. Else, the `theme: None` default entry → write there. 3. Else, push a fresh entry keyed to active_theme (or None when active is empty). The write is now guaranteed to flip the resolved value when it returns true. Tests (2 added — direct repros of the codex BLOCK, 287 shell-core total): - `set_color_hex_changes_resolved_value_under_subset_active_theme` builds the exact pathological case (entry mode-only, active has mode+density), writes, then asserts `resolve_color` returns the NEW value. Pre-fix this test would have read the untouched "#111" and failed. - `set_color_hex_falls_back_to_default_entry_when_no_subset_match` covers the second branch — themed entry mismatch + `theme: None` default + active that matches neither. The write must update the default (which is what resolve falls back to) and not append a never-read dark entry. --- .../src/document/variables.rs | 133 ++++++++++++++++-- 1 file changed, 124 insertions(+), 9 deletions(-) diff --git a/crates/openpencil-shell-core/src/document/variables.rs b/crates/openpencil-shell-core/src/document/variables.rs index efd093566..93651c203 100644 --- a/crates/openpencil-shell-core/src/document/variables.rs +++ b/crates/openpencil-shell-core/src/document/variables.rs @@ -217,16 +217,30 @@ impl VariableTable { true } VariableValue::Themed(entries) => { - // Find the entry whose `theme` matches every k/v in - // the active map (or the default `theme: None` - // entry when no themed match exists). Write through - // — or push a new entry keyed to the active theme - // when no exact match. - let exact_idx = entries.iter().position(|e| match &e.theme { - Some(t) => t == &active, - None => active.is_empty(), + // Codex stop-gate: the write path MUST match the + // resolve path's subset-matching semantics, otherwise + // a successful write can fail to change the resolved + // value (an earlier-in-vec themed entry whose `theme` + // is a subset of active_theme would still win the + // resolve walk, shadowing the new entry we pushed at + // the end). Mirror `Variable::resolve` exactly: + // + // 1. First themed entry whose `theme` is a subset + // of active_theme → write there. + // 2. Else, the `theme: None` default entry → write + // there. + // 3. Else, push a fresh entry keyed to the active + // theme (or None when active is empty). + let subset_idx = entries.iter().position(|e| match &e.theme { + Some(t) => t.iter().all(|(k, v)| active.get(k) == Some(v)), + None => false, }); - if let Some(i) = exact_idx { + if let Some(i) = subset_idx { + entries[i].value = VariableScalar::Str(normalized); + return true; + } + let default_idx = entries.iter().position(|e| e.theme.is_none()); + if let Some(i) = default_idx { entries[i].value = VariableScalar::Str(normalized); return true; } @@ -540,6 +554,107 @@ mod tests { assert!(!tbl.set_color_hex("spacing", "#ffffff")); } + #[test] + fn set_color_hex_changes_resolved_value_under_subset_active_theme() { + // Codex stop-gate regression: when the themed entry's theme + // is a SUBSET of the active theme (e.g. entry only specifies + // `mode: dark`; active is `mode: dark, density: compact`), + // `resolve` still picks that entry — so a write must update + // that entry in place, NOT push a new entry at the end that + // `resolve` will never reach. + let mut tbl = super::VariableTable::default(); + tbl.variables.push(Variable { + name: "bg".into(), + kind: VariableKind::Color, + value: VariableValue::Themed(vec![ThemedValue { + value: VariableScalar::Str("#111111".into()), + theme: Some({ + let mut m = BTreeMap::new(); + m.insert("mode".into(), "dark".into()); + m + }), + }]), + }); + tbl.set_active_theme("mode", "dark"); + tbl.set_active_theme("density", "compact"); + // Sanity: resolves through the subset-matching entry. + assert_eq!( + tbl.resolve_color("bg"), + Some(crate::Color { + r: 0x11 as f32 / 255.0, + g: 0x11 as f32 / 255.0, + b: 0x11 as f32 / 255.0, + a: 1.0, + }) + ); + // Write under the wider active theme. The original entry + // (only mode=dark) is the resolved one — the write MUST go + // through it, not append a new entry that resolve never + // reaches. + assert!(tbl.set_color_hex("bg", "#22ff44")); + assert_eq!( + tbl.resolve_color("bg"), + Some(crate::Color { + r: 0x22 as f32 / 255.0, + g: 1.0, + b: 0x44 as f32 / 255.0, + a: 1.0, + }), + "themed write must update the entry resolve picks" + ); + } + + #[test] + fn set_color_hex_falls_back_to_default_entry_when_no_subset_match() { + // Themed entry that doesn't match + a `theme: None` default. + // Active theme matches NEITHER themed entry → write must + // update the default entry (which is what resolve falls + // back to) and not append a new themed entry. + let mut tbl = super::VariableTable::default(); + tbl.variables.push(Variable { + name: "bg".into(), + kind: VariableKind::Color, + value: VariableValue::Themed(vec![ + ThemedValue { + value: VariableScalar::Str("#ffffff".into()), + theme: Some({ + let mut m = BTreeMap::new(); + m.insert("mode".into(), "light".into()); + m + }), + }, + ThemedValue { + value: VariableScalar::Str("#888888".into()), + theme: None, + }, + ]), + }); + tbl.set_active_theme("mode", "dark"); // matches neither + // Pre-write: resolves to the default. + assert_eq!( + tbl.resolve_color("bg"), + Some(crate::Color { + r: 0x88 as f32 / 255.0, + g: 0x88 as f32 / 255.0, + b: 0x88 as f32 / 255.0, + a: 1.0, + }) + ); + // Write should update the default entry, not push a new dark + // entry (which would still leave resolve walking the default + // for the SAME active=dark theme). + assert!(tbl.set_color_hex("bg", "#aabbcc")); + assert_eq!( + tbl.resolve_color("bg"), + Some(crate::Color { + r: 0xaa as f32 / 255.0, + g: 0xbb as f32 / 255.0, + b: 0xcc as f32 / 255.0, + a: 1.0, + }) + ); + } + #[test] fn set_color_hex_writes_themed_entry_matching_active_axis() { let mut tbl = super::VariableTable::default();