fix(mcp): get_active_theme round-trips commas in theme values

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.
This commit is contained in:
Kayshen-X 2026-05-14 19:49:21 +08:00
parent 7a4b06f5ad
commit e56869c13d

View file

@ -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 `\<delim>` is unescaped
/// to a literal `<delim>` 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<String> {
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