SemianiakaVY/openpencil
1
0
Fork 0
Code Issues Pull requests Actions 2 Packages Projects Releases Wiki Activity

fix(variables): set_color_hex mirrors resolve's subset matching

Browse source 
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.
This commit is contained in:
Kayshen-X 2026-05-14 18:34:36 +08:00
parent ff3c79e81b
commit baad7e12fb
1 changed files with 124 additions and 9 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

133
crates/openpencil-shell-core/src/document/variables.rs
View file

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