From e56869c13d01e35fd906b0f6e510e683070f2b11 Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Thu, 14 May 2026 19:49:21 +0800 Subject: [PATCH] fix(mcp): get_active_theme round-trips commas in theme values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex stop-gate: `get_active_theme.options` joins per-axis value lists with `,` but theme values can legitimately contain commas (e.g. "red, white, and blue"). The previous escape set (`\;|`) left `,` unescaped, so the comma joiner would mis-split a single value into multiple fake ones on decode. `escape_record_field` + `unescape_record_field` extended to also escape `,` (and accept `\,` on decode). The Rust-side helpers stay backward-compatible because no existing encoder previously emitted a literal `\,`. `list_variables` is unaffected because that wire format doesn't use `,` as a delimiter; the escaped form is harmless there. New layered-decoder pattern in the test: `get_active_theme.options` is a TWO-LEVEL split (outer `|`, inner `,`), so the decoder must unescape only the delimiter for the current level — leaving `\,` intact during the outer `|` split, then unescaping `\,` during the inner `,` split. The previous test had an over-eager walker that unescaped every delimiter at once, which broke the inner split. Helper `layered_split(s, delim)` makes this contract explicit: let outer = layered_split(&opts, '|'); // unescape only \| let inner = layered_split(&outer[1], ','); // unescape only \, Tests (1 added, 304 shell-core total): - `get_active_theme_round_trips_comma_in_value` builds an axis with value `"a,b,c"` plus a plain second value. After encode + layered decode, asserts the values vec is exactly `["a,b,c", "plain"]`. Pre-fix the inner comma split saw 4 values; with the layered decoder + comma escape it sees 2. The wire format is now safe for every char a `.op` theme value can legally carry. --- crates/openpencil-shell-core/src/mcp/tools.rs | 82 ++++++++++++++++++- 1 file changed, 78 insertions(+), 4 deletions(-) diff --git a/crates/openpencil-shell-core/src/mcp/tools.rs b/crates/openpencil-shell-core/src/mcp/tools.rs index 06b91b848..b608c184a 100644 --- a/crates/openpencil-shell-core/src/mcp/tools.rs +++ b/crates/openpencil-shell-core/src/mcp/tools.rs @@ -350,10 +350,17 @@ impl McpTool for ListVariables { } } -/// Escape `\` / `;` / `|` so the encoded `name|kind|value` triplets -/// stay unambiguous regardless of variable content. Backslash-prefix -/// is the standard pattern — clients invert it by walking bytes and +/// Escape every delimiter the MCP wire format uses, so encoded +/// fields stay unambiguous regardless of payload content. +/// Escaped chars: `\`, `;`, `|`, `,`. Backslash-prefix is the +/// standard pattern — clients invert it by walking bytes and /// promoting `\X` → `X` whenever they see an escape introducer. +/// +/// Comma is in the set because `get_active_theme.options` joins +/// per-axis value lists with `,` (codex stop-gate: a theme value +/// like `"red, white, and blue"` would otherwise mis-split into 3 +/// fake values on decode). `list_variables` doesn't use `,` as a +/// delimiter so the escaped form is harmless there. fn escape_record_field(s: &str) -> String { let mut out = String::with_capacity(s.len()); for c in s.chars() { @@ -361,6 +368,7 @@ fn escape_record_field(s: &str) -> String { '\\' => out.push_str("\\\\"), ';' => out.push_str("\\;"), '|' => out.push_str("\\|"), + ',' => out.push_str("\\,"), c => out.push(c), } } @@ -376,7 +384,7 @@ pub fn unescape_record_field(s: &str) -> String { while let Some(c) = chars.next() { if c == '\\' { match chars.next() { - Some(esc @ ('\\' | ';' | '|')) => out.push(esc), + Some(esc @ ('\\' | ';' | '|' | ',')) => out.push(esc), Some(other) => { out.push('\\'); out.push(other); @@ -588,6 +596,72 @@ mod tests { } } + #[test] + fn get_active_theme_round_trips_comma_in_value() { + // Codex stop-gate: theme values can legitimately contain + // commas (e.g. a description-style value like "red, white, + // and blue"). The comma joiner inside `options` would + // otherwise mis-split into multiple fake values. + // + // Decoding strategy is LAYERED: each split level only + // unescapes the delimiter for THAT level, leaving other + // escapes intact for inner splits. A walker that unescapes + // every delimiter at once over-decodes the comma escapes + // before the inner split sees them. + use crate::document::{Document, ThemeAxis}; + let mut doc = Document::empty(); + doc.var_table.themes.push(ThemeAxis { + name: "palette".into(), + values: vec!["a,b,c".into(), "plain".into()], + }); + let tool = get_active_theme_snapshot(&doc); + let opts = match tool.call(&BTreeMap::new()) { + ToolOutcome::Ok(o) => o.get("options").unwrap().clone(), + _ => panic!(), + }; + // Split on `|` — only `\|` is unescaped at this level; + // `\,` and `\;` pass through verbatim into the inner blob. + let fields = layered_split(&opts, '|'); + assert_eq!(fields.len(), 2); + assert_eq!(fields[0], "palette"); + // Split fields[1] on `,` — only `\,` is unescaped here. + let values = layered_split(&fields[1], ','); + assert_eq!( + values, + vec!["a,b,c".to_string(), "plain".to_string()], + "comma must round-trip through escape" + ); + } + + /// Split `s` on unescaped `delim`. Only `\` is unescaped + /// to a literal `` byte; every other backslash sequence + /// passes through verbatim so inner splits can decode their own + /// delimiters. Decoder-side counterpart to `escape_record_field` + /// for hierarchical wire formats like `get_active_theme.options` + /// (`axis|v1,v2,v3;axis2|v1,v2`). + fn layered_split(s: &str, delim: char) -> Vec { + let mut out = Vec::new(); + let mut cur = String::new(); + let mut chars = s.chars().peekable(); + while let Some(c) = chars.next() { + if c == '\\' { + match chars.peek() { + Some(&n) if n == delim => { + chars.next(); + cur.push(delim); + } + _ => cur.push(c), + } + } else if c == delim { + out.push(std::mem::take(&mut cur)); + } else { + cur.push(c); + } + } + out.push(cur); + out + } + #[test] fn get_active_theme_escapes_pipe_and_semicolon_in_values() { // Theme axis values with weird payloads (unusual but valid