fix(mcp): escape \ / ; / | in list_variables wire encoding

Codex stop-gate flagged that the previous commit (a54464ce) made an
unfounded claim about the canonical `.op` schema rejecting `;` and
`|` in variable names + values. There is no such validator —
string-kind variables can legitimately carry payloads like
`"a|b;c\\d"`, and my flat `name|kind|value;...` encoding would
mangle the field boundaries.

`crates/openpencil-shell-core/src/mcp/tools.rs`:

  - `escape_record_field(s)` — `\` → `\\`, `;` → `\;`, `|` → `\|`.
    Applied to every field before joining. Standard
    backslash-escape pattern that any client can invert with a
    one-pass byte walk.

  - `unescape_record_field(s)` — exposed as `pub` so Rust-side MCP
    clients have a canonical decoder reference. Unrecognized
    escape sequences pass through verbatim (`\X` for X ∉ {`\`,
    `;`, `|`} stays as `\X`) so future format extensions don't
    silently corrupt today's data.

  - Doc comment correctly states the wire format + the escape
    rules instead of the previous wrong "schema forbids these
    chars" claim.

Tests (2 added, 5 mcp::tools total, 280 shell-core total):

  - `escape_record_field_round_trips_pipe_semicolon_backslash`
    fuzzes the round-trip across 10 inputs including every
    delimiter alone, mixed payloads (`a|b;c\\d`), empty string,
    and pure-delimiter strings (`;;|||`).

  - `list_variables_encodes_string_value_with_special_chars`
    drives the full end-to-end path: a string variable whose
    value is `a|b;c\\d` is encoded by `list_variables`, then
    decoded by a hand-written walker matching the doc-comment's
    "promote `\X` → `X`" rule. Asserts the split yields exactly
    3 fields + the third field matches the original payload byte
    for byte.

The wire format is now unambiguous for any variable data the
canonical loader will accept.
This commit is contained in:
Kayshen-X 2026-05-14 18:21:53 +08:00
parent e9fc471855
commit 7f73278c6c

View file

@ -304,20 +304,70 @@ impl McpTool for ListVariables {
let mut out = BTreeMap::new();
out.insert("count".into(), self.variables.len().to_string());
// Encode the list as `name|kind|value` triplets joined by
// `;`. Keeps shell-core serde-free; clients split on the
// delimiters. Names + values cannot contain `;` or `|` in
// valid `.op` schemas (the canonical loader rejects those
// chars during variable-name validation).
// `;`. The canonical `.op` schema doesn't actually forbid
// `;` / `|` / `\` in variable names or string values
// (codex stop-gate flagged my earlier claim), so escape
// them: `\` → `\\`, `;` → `\;`, `|` → `\|`. Clients can
// decode unambiguously by walking the bytes and treating
// a `\` as the escape introducer. Empty list → empty
// `variables` field (clients should consult `count`).
let encoded: Vec<String> = self
.variables
.iter()
.map(|v| format!("{}|{}|{}", v.name, v.kind, v.value))
.map(|v| {
format!(
"{}|{}|{}",
escape_record_field(&v.name),
escape_record_field(&v.kind),
escape_record_field(&v.value),
)
})
.collect();
out.insert("variables".into(), encoded.join(";"));
ToolOutcome::Ok(out)
}
}
/// 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
/// promoting `\X` → `X` whenever they see an escape introducer.
fn escape_record_field(s: &str) -> String {
let mut out = String::with_capacity(s.len());
for c in s.chars() {
match c {
'\\' => out.push_str("\\\\"),
';' => out.push_str("\\;"),
'|' => out.push_str("\\|"),
c => out.push(c),
}
}
out
}
/// Inverse of `escape_record_field`. Exposed for clients written in
/// Rust (the TS / Python MCP client side rolls its own decoder; this
/// is the canonical reference impl + the test fixture).
pub fn unescape_record_field(s: &str) -> String {
let mut out = String::with_capacity(s.len());
let mut chars = s.chars();
while let Some(c) = chars.next() {
if c == '\\' {
match chars.next() {
Some(esc @ ('\\' | ';' | '|')) => out.push(esc),
Some(other) => {
out.push('\\');
out.push(other);
}
None => out.push('\\'),
}
} else {
out.push(c);
}
}
out
}
pub fn list_variables_snapshot(doc: &crate::document::Document) -> ListVariables {
use crate::document::{VariableKind, VariableScalar};
let variables = doc
@ -416,4 +466,74 @@ mod tests {
_ => panic!("expected Ok"),
}
}
#[test]
fn escape_record_field_round_trips_pipe_semicolon_backslash() {
// Codex stop-gate caught: the canonical `.op` schema does
// NOT forbid `;`, `|`, or `\` in variable names or string
// values. The encoding must round-trip every such payload.
for raw in &[
"plain",
"a|b",
"a;b",
"a\\b",
"a|b;c\\d",
"\\",
";;|||",
"",
"color/primary",
"label with space",
] {
let escaped = escape_record_field(raw);
let back = unescape_record_field(&escaped);
assert_eq!(&back, raw, "round-trip failed for {raw:?}");
}
}
#[test]
fn list_variables_encodes_string_value_with_special_chars() {
// String variable whose value contains every delimiter.
// The wire format must keep the record boundary clear AND
// recover the original payload on decode.
use crate::document::{Document, Variable, VariableKind, VariableScalar, VariableValue};
let mut doc = Document::empty();
doc.var_table.variables.push(Variable {
name: "msg".into(),
kind: VariableKind::String,
value: VariableValue::Scalar(VariableScalar::Str("a|b;c\\d".into())),
});
let tool = list_variables_snapshot(&doc);
let out = match tool.call(&BTreeMap::new()) {
ToolOutcome::Ok(o) => o,
other => panic!("expected Ok, got {other:?}"),
};
assert_eq!(out.get("count"), Some(&"1".to_string()));
let encoded = out.get("variables").expect("variables field");
// Pipe inside the value is escaped, so splitting on `|`
// still yields exactly 3 fields (name | kind | value).
// Decode each field + verify the original payload.
let mut fields: Vec<String> = Vec::new();
let mut cur = String::new();
let mut chars = encoded.chars().peekable();
while let Some(c) = chars.next() {
if c == '\\' {
if let Some(&n) = chars.peek() {
if matches!(n, '\\' | ';' | '|') {
cur.push(chars.next().unwrap());
continue;
}
}
cur.push(c);
} else if c == '|' {
fields.push(std::mem::take(&mut cur));
} else {
cur.push(c);
}
}
fields.push(cur);
assert_eq!(fields.len(), 3, "expected name|kind|value, got {fields:?}");
assert_eq!(fields[0], "msg");
assert_eq!(fields[1], "string");
assert_eq!(fields[2], "a|b;c\\d");
}
}