diff --git a/crates/khive-mcp/src/server.rs b/crates/khive-mcp/src/server.rs index 67750e9f0..241c9cf68 100644 --- a/crates/khive-mcp/src/server.rs +++ b/crates/khive-mcp/src/server.rs @@ -2363,7 +2363,7 @@ mod tests { // `ops` argument as a JSON string using the standard JSON `\n` // escape. Deserializing `RequestParams` decodes that escape into an // actual raw LF byte inside the DSL source — the exact shape - // `escape_literal_control_chars` (crates/khive-request/src/parser/scan.rs) + // `normalize_quoted_string` (crates/khive-request/src/parser/scan.rs) // exists to accept. This confirms the decoded raw newline survives // parsing and dispatch all the way to the pack handler's result. let wire = "{\"ops\":\"create(kind=\\\"entity\\\", entity_kind=\\\"concept\\\", name=\\\"line1\\nline2\\\")\"}"; diff --git a/crates/khive-request/src/parser/parser_impl.rs b/crates/khive-request/src/parser/parser_impl.rs index c3a724f94..e20a17a8c 100644 --- a/crates/khive-request/src/parser/parser_impl.rs +++ b/crates/khive-request/src/parser/parser_impl.rs @@ -4,7 +4,7 @@ use serde_json::Value; use crate::types::{ArgValue, DslError, ParsedOp, NESTING_DEPTH_LIMIT}; -use super::scan::{char_label, escape_literal_control_chars, scan_string_end}; +use super::scan::{char_label, normalize_quoted_string, scan_string_end, NormalizedQuotedString}; /// Byte-slice cursor for the DSL input. pub(crate) struct Parser<'a> { @@ -248,11 +248,7 @@ impl<'a> Parser<'a> { let start = self.pos; let end = scan_string_end(self.src, start)?; let raw = std::str::from_utf8(&self.src[start..end]).expect("utf8 key literal"); - let s: String = - serde_json::from_str(raw).map_err(|e| DslError::InvalidValue { - pos: start, - error: e.to_string(), - })?; + let s = decode_quoted_json_key(raw, start)?; self.pos = end; s } @@ -389,20 +385,18 @@ impl<'a> Parser<'a> { let trimmed = slice.trim(); // A quoted string literal may contain raw control bytes (newline, CR, // tab) verbatim in the DSL source; JSON proper forbids that, so - // rewrite them to JSON escapes before handing the slice to - // `serde_json`. Non-string values (numbers, bool, null) never - // legitimately contain such bytes, so this only touches strings. - let normalized; - let json_src: &str = if trimmed.starts_with('"') { - normalized = escape_literal_control_chars(trimmed); - &normalized + // `decode_quoted_json_string` rewrites them to JSON escapes before + // handing the slice to `serde_json`. Non-string values (numbers, + // bool, null) never legitimately contain such bytes, so this only + // touches strings. + let value = if trimmed.starts_with('"') { + Value::String(decode_quoted_json_string(trimmed, start)?) } else { - trimmed + serde_json::from_str(trimmed).map_err(|e| DslError::InvalidValue { + pos: start, + error: e.to_string(), + })? }; - let value: Value = serde_json::from_str(json_src).map_err(|e| DslError::InvalidValue { - pos: start, - error: e.to_string(), - })?; self.pos = end; Ok(value) } @@ -501,6 +495,126 @@ impl<'a> Parser<'a> { } } +/// Maps a `serde_json::Error`'s 1-indexed `(line, column)` to a 0-indexed +/// byte offset into `text` — the exact string `serde_json` was parsing — +/// pointing AT the offending byte itself. `line`/`column` count raw bytes +/// (including any literal control byte a [`NormalizedQuotedString`] left +/// unrewritten inside a would-be escape pair, which is the failure case a +/// `line > 1` report handles: consuming that raw `\n` itself advances the +/// tracker to `(line + 1, column 0)`, so `column` can legitimately be `0` +/// — it is not an error sentinel). A `line > 1` failure is resolved by +/// walking `text` for the `line - 1`-th `\n` byte at index `i`; the +/// offending byte sits at `i + column` in both the "it's the newline +/// itself" case (`column == 0`) and the "N more bytes were consumed on the +/// new line first" case. Returns `None` only if `text` has fewer `\n` bytes +/// than the error claims (never observed from `serde_json` in practice) or +/// `line <= 1` with `column == 0` (no valid 0-indexed byte before column 1 +/// on the first line), in which case the caller falls back to the plain +/// serde message. +fn serde_error_byte_offset(e: &serde_json::Error, text: &str) -> Option { + let line = e.line(); + let column = e.column(); + if line <= 1 { + return column.checked_sub(1); + } + let mut newlines_seen = 0usize; + for (i, b) in text.bytes().enumerate() { + if b == b'\n' { + newlines_seen += 1; + if newlines_seen == line - 1 { + return Some(i + column); + } + } + } + None +} + +/// Enriches a `serde_json` string-decode failure with the ADR-016 escape +/// grammar and the MCP double-escape gotcha (ADR-084 §3c). Enrichment is +/// gated on the failure being AT the recorded [`ControlByteHit`]: the serde +/// error's `(line, column)` is mapped to a byte offset in `normalized.text` +/// via [`serde_error_byte_offset`], then compared against +/// `normalized.first_control_byte` (collected during the same pass that +/// built `text` — no re-scan of the span). A failure whose offset lands +/// elsewhere (e.g. an invalid `\q` escape, even with an unrelated control +/// byte later in the span) falls through to the plain serde message +/// unchanged, so a control byte is never misattributed as the cause of a +/// different failure. +/// +/// Offset alone is not sufficient when the hit is *not* +/// `preceded_by_backslash`: a malformed `\u` escape (e.g. `\u123` where the +/// 4th hex-digit slot is consumed by a following, unrelated raw control +/// byte) fails with the same "invalid escape" kind and can land at that +/// byte's exact offset too — indistinguishable from a genuine standalone +/// control-character failure by offset alone. Only a hit whose recorded +/// origin was a broken `\` pair (`preceded_by_backslash`) is known — +/// by construction, not by re-deriving it from the error text — to be an +/// actual invalid-escape failure over that byte; a non-backslash hit is only +/// enriched when the failure is the plain control-character kind serde +/// reports for a standalone raw control byte, checked via the error's +/// `Display` text since `ErrorCode` itself is private and `classify()` only +/// returns the coarse `Category::Syntax` shared by every one of these kinds. +fn describe_quoted_string_parse_error( + e: &serde_json::Error, + normalized: &NormalizedQuotedString<'_>, +) -> String { + let base = e.to_string(); + let Some(offset) = serde_error_byte_offset(e, normalized.text.as_ref()) else { + return base; + }; + let Some(hit) = normalized + .first_control_byte + .as_ref() + .filter(|h| h.normalized_pos == offset) + else { + return base; + }; + if !hit.preceded_by_backslash && !base.starts_with("control character") { + return base; + } + // `raw_pos` counts from the span's opening quote; report the index relative + // to the value itself (the saturating guard covers the unreachable pos-0 hit). + let idx = hit.raw_pos.saturating_sub(1); + let c = hit.byte as char; + format!( + "{base} — byte {idx} of the value is {c:?} (U+{:04X}). DSL string escapes follow JSON: \ + \\n, \\t, \\\", \\\\ (raw newline/CR/tab are also accepted literally; other control \ + bytes must be escaped). If `ops` is sent through a JSON transport, the transport \ + decodes one escape level before the DSL parser runs, so a literal \ + backslash-escape must be doubled on the wire — e.g. send \\\\n to produce \\n here.", + c as u32 + ) +} + +/// Decodes a quoted-string DSL literal (`raw`, the exact quoted span +/// including its surrounding `"`) into its `String` value, normalizing raw +/// literal newline/CR/tab bytes to JSON escapes first (ADR-016) and +/// enriching any remaining decode failure via +/// [`describe_quoted_string_parse_error`]. Used by quoted string values +/// (`parse_value`) only — the literal-newline/CR/tab carve-out is scoped to +/// argument values, not object keys (see [`decode_quoted_json_key`]). +fn decode_quoted_json_string(raw: &str, pos: usize) -> Result { + let normalized = normalize_quoted_string(raw); + serde_json::from_str(normalized.text.as_ref()).map_err(|e| DslError::InvalidValue { + pos, + error: describe_quoted_string_parse_error(&e, &normalized), + }) +} + +/// Decodes a quoted-string DSL literal that is an object KEY (`raw`, the +/// exact quoted span including its surrounding `"`) into its `String` value +/// using strict `serde_json` semantics. Unlike [`decode_quoted_json_string`], +/// no raw control byte is tolerated here: per ADR-016 / PR #957, the +/// literal-newline/CR/tab carve-out applies to quoted argument VALUES only — +/// a key containing one of those bytes must use its JSON escape form +/// (`\n`, `\r`, `\t`) like any other control byte. +fn decode_quoted_json_key(raw: &str, pos: usize) -> Result { + serde_json::from_str(raw).map_err(|e| DslError::InvalidValue { + pos, + error: e.to_string(), + }) +} + /// Validates quoted-reference brackets before promoting a string to `PrevRef`. /// A malformed segment keeps the entire value literal. fn quoted_prev_path_is_valid(path: &str) -> bool { diff --git a/crates/khive-request/src/parser/scan.rs b/crates/khive-request/src/parser/scan.rs index 7415ad3f8..423a568f1 100644 --- a/crates/khive-request/src/parser/scan.rs +++ b/crates/khive-request/src/parser/scan.rs @@ -1,5 +1,7 @@ //! Low-level scanner helpers: string scanning, `$prev` detection, char labels. +use std::borrow::Cow; + use serde_json::Value; use crate::types::{ArgValue, DslError, ParsedOp, NESTING_DEPTH_LIMIT}; @@ -20,29 +22,148 @@ pub(crate) fn scan_string_end(src: &[u8], start: usize) -> Result` pair as an invalid-escape failure at the control byte's own +/// offset, while a genuinely standalone control byte is reported as a +/// control-character failure at that same kind of offset — and a malformed +/// `\u` escape whose short hex run is padded out by a *later*, unrelated +/// standalone control byte also fails as an invalid escape at that byte's +/// offset. Offset alone cannot tell the legitimate backslash-pair case +/// apart from that last, spurious one; `preceded_by_backslash` is the signal +/// the caller uses to do so. A control byte that lands inside a `\u` +/// escape's own 4-hex-digit slot run is never recorded at all, even when +/// the byte immediately preceding it happens to be a backslash — see +/// [`normalize_quoted_string`] for why that adjacency is not real. +pub(crate) struct ControlByteHit { + pub(crate) normalized_pos: usize, + pub(crate) raw_pos: usize, + pub(crate) byte: u8, + pub(crate) preceded_by_backslash: bool, +} + +/// Result of [`normalize_quoted_string`]: the text to hand to `serde_json`, +/// plus the first literal control byte it still contains, if any. A +/// subsequent `serde_json` parse failure always occurs at the *earliest* +/// invalid byte, so the first hit in scan order is the only one whose +/// offset can ever match — retaining just that one hit (instead of every +/// occurrence) bounds diagnostic metadata to O(1) regardless of how many +/// control bytes a malformed span contains, while still letting a parse +/// failure be attributed without re-scanning the span. +pub(crate) struct NormalizedQuotedString<'a> { + pub(crate) text: Cow<'a, str>, + pub(crate) first_control_byte: Option, +} + +/// Rewrites raw literal newline, carriage return, and tab bytes inside a /// double-quoted string literal into their JSON escape form, so a value /// containing one of those three bytes verbatim (as opposed to a /// `\n`/`\r`/`\t` escape sequence) still parses as valid JSON. Existing /// backslash-escape pairs are copied through untouched: this walks the same /// `\` + next-byte pairing [`scan_string_end`] uses, so an already-escaped -/// sequence is never reinterpreted. +/// sequence is never reinterpreted — EXCEPT that a backslash directly +/// followed by any raw U+0000-U+001F control byte is not a valid two-byte +/// JSON escape (a valid escape is backslash plus an ASCII escape letter, +/// never backslash plus a literal control byte), so that byte is recorded +/// as [`NormalizedQuotedString::first_control_byte`] (when no earlier hit +/// was already recorded) even though it is copied through unrewritten; the +/// resulting `serde_json` failure is the "real control-char cause" for that +/// case (#491 round-2). /// -/// Per ADR-016, this exception is limited to exactly those three +/// Per ADR-016, the standalone rewrite is limited to exactly those three /// characters. Every other raw U+0000-U+001F control byte is left as-is and /// falls through to `serde_json`, which rejects it as invalid JSON — the -/// same behavior as before this exception existed. -pub(crate) fn escape_literal_control_chars(s: &str) -> String { - if !s.contains(['\n', '\r', '\t']) { - return s.to_owned(); +/// same behavior as before this exception existed. When the span has no +/// control byte at all (the common case), `text` borrows `raw` directly — +/// no allocation. +/// +/// A `\u` escape is handled separately from the general backslash-pair walk: +/// once the `u` is seen, the following 4 characters are copied through +/// verbatim as a single unit, exactly mirroring how `serde_json`'s +/// `decode_hex_escape` consumes its 4-byte candidate-digit slice — as raw +/// bytes, never reinterpreted as introducing a new escape, even when one of +/// them is itself a backslash. Without this, a short `\u` run (e.g. 2 hex +/// digits followed by a backslash and then a raw control byte) would have +/// its 3rd slot — the backslash — re-enter the general branch above and get +/// misread as a fresh, genuine `\` pair, even though `serde_json` never +/// treats it as one; the resulting hit would carry `preceded_by_backslash: +/// true` for a failure that is actually a malformed unicode escape, not a +/// broken control-byte pair. Bytes inside the 4-slot window are therefore +/// never recorded as a [`ControlByteHit`], matching the fact that +/// `decode_hex_escape` never reports a control-character-style failure for +/// them — only ever `ErrorCode::InvalidEscape`, the same code (and `Display` +/// text) a genuine broken pair produces, which is why scan-time origin, not +/// the error text, has to be the source of truth. +/// +/// A completed `\u` escape whose value is a high surrogate (U+D800-U+DBFF) +/// must be followed by a `\u` low-surrogate escape; `serde_json` consumes the +/// next `\` as that continuation and fails on the malformed pair when a `\u` +/// does not follow. The control byte it lands on there belongs to that +/// surrogate failure, not to a fresh broken `\` pair, so it is likewise +/// not recorded as a [`ControlByteHit`]: recording it would attach the +/// double-escape teaching to a malformed-surrogate error it does not explain. +pub(crate) fn normalize_quoted_string(raw: &str) -> NormalizedQuotedString<'_> { + if !raw.bytes().any(|b| b < 0x20) { + return NormalizedQuotedString { + text: Cow::Borrowed(raw), + first_control_byte: None, + }; } - let mut out = String::with_capacity(s.len() + 8); - let mut chars = s.chars(); - while let Some(c) = chars.next() { + let mut out = String::with_capacity(raw.len() + 8); + let mut first_control_byte = None; + let mut chars = raw.char_indices().peekable(); + let mut hex_slots_remaining = 0u32; + let mut hex_acc = 0u32; + let mut hex_valid = true; + let mut after_high_surrogate = false; + while let Some((pos, c)) = chars.next() { + if hex_slots_remaining > 0 { + hex_slots_remaining -= 1; + match c.to_digit(16) { + Some(d) => hex_acc = (hex_acc << 4) | d, + None => hex_valid = false, + } + out.push(c); + if hex_slots_remaining == 0 { + after_high_surrogate = hex_valid && (0xD800..=0xDBFF).contains(&hex_acc); + } + continue; + } + // True for exactly the one position after a completed high-surrogate + // `\u` escape; consumed here so it gates only the immediate surrogate + // continuation, never anything later. + let prev_was_high_surrogate = after_high_surrogate; + after_high_surrogate = false; if c == '\\' { out.push(c); - if let Some(next) = chars.next() { - out.push(next); + if let Some(&(next_pos, next_c)) = chars.peek() { + chars.next(); + if next_c == 'u' { + out.push(next_c); + hex_slots_remaining = 4; + hex_acc = 0; + hex_valid = true; + continue; + } + if (next_c as u32) < 0x20 + && !prev_was_high_surrogate + && first_control_byte.is_none() + { + first_control_byte = Some(ControlByteHit { + normalized_pos: out.len(), + raw_pos: next_pos, + byte: next_c as u8, + preceded_by_backslash: true, + }); + } + out.push(next_c); } continue; } @@ -50,10 +171,24 @@ pub(crate) fn escape_literal_control_chars(s: &str) -> String { '\n' => out.push_str("\\n"), '\r' => out.push_str("\\r"), '\t' => out.push_str("\\t"), + c if (c as u32) < 0x20 => { + if first_control_byte.is_none() { + first_control_byte = Some(ControlByteHit { + normalized_pos: out.len(), + raw_pos: pos, + byte: c as u8, + preceded_by_backslash: false, + }); + } + out.push(c); + } c => out.push(c), } } - out + NormalizedQuotedString { + text: Cow::Owned(out), + first_control_byte, + } } /// Returns a stable delimiter label for diagnostics. @@ -145,3 +280,22 @@ fn arg_value_has_prev_ref(av: &ArgValue) -> bool { ArgValue::Value(_) => false, } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn many_control_bytes_retain_only_the_first_hit() { + let raw = "\u{0}".repeat(4096); + let normalized = normalize_quoted_string(&raw); + let hit = normalized + .first_control_byte + .expect("first control byte must be recorded"); + assert_eq!( + hit.raw_pos, 0, + "must record the earliest hit, not a later one" + ); + assert_eq!(hit.byte, 0); + } +} diff --git a/crates/khive-request/tests/parser.rs b/crates/khive-request/tests/parser.rs index 0c3971af5..14bfe1fd6 100644 --- a/crates/khive-request/tests/parser.rs +++ b/crates/khive-request/tests/parser.rs @@ -193,7 +193,7 @@ fn raw_newline_immediately_after_escaped_quote_boundary() { fn raw_control_char_in_bareword_position_rejected() { // The literal-control-char exception only applies inside a // double-quoted string value (`parse_value` only calls - // `escape_literal_control_chars` when the trimmed slice starts with + // `normalize_quoted_string` when the trimmed slice starts with // `"`). A raw NUL embedded in an unquoted (bareword) numeric value must // still fail to parse as JSON, exactly as before this PR. let src = format!("gtd.assign(weight=1{}0)", '\u{0}'); @@ -220,6 +220,83 @@ fn other_raw_control_chars_in_quoted_string_still_rejected() { } } +#[test] +fn control_char_error_teaches_escape_syntax_and_mcp_double_escape() { + // #491: the bare serde message ("control character ... found while + // parsing a string") teaches nothing. The wrapped error must name the + // JSON escape grammar and the MCP-transport double-escape gotcha, so a + // caller can actually fix the `ops` string instead of landing on the + // wrong "switch to JSON op form" workaround. + let src = format!("gtd.assign(title=\"a{}b\")", '\u{c}'); + let err = parse_request(&src).unwrap_err(); + let msg = err.to_string(); + assert!( + matches!(err, DslError::InvalidValue { .. }), + "expected InvalidValue, got {err:?}" + ); + assert!( + msg.contains(r#"\n"#) && msg.contains(r#"\t"#) && msg.contains(r#"\""#), + "error should name the JSON escape grammar, got: {msg}" + ); + assert!( + msg.to_lowercase().contains("double"), + "error should call out the MCP double-escape requirement, got: {msg}" + ); +} + +#[test] +fn control_char_error_reports_value_relative_byte_index() { + // #491: the diagnostic byte index is relative to the value, not the raw + // quoted span; it must not count the opening quote. A control byte that is + // the first byte of the value is reported as "byte 0 of the value", not + // "byte 1". + let src = format!("gtd.assign(title=\"{}b\")", '\u{c}'); + let err = parse_request(&src).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("byte 0 of the value"), + "byte index must be value-relative (exclude the opening quote), got: {msg}" + ); +} + +#[test] +fn control_char_in_object_key_rejected_with_plain_serde_message() { + // #491: quoted object KEYS decode through a separate, strict path from + // quoted values (`parse_object_arg_body` vs `parse_value`) — the + // value-path escape-grammar teaching diagnostic is scoped to values only + // (ADR-016, PR #957). A raw control byte in a key still fails to parse, + // but with the plain serde message rather than the value-path + // enrichment. + let src = format!("gtd.assign(properties={{\"a{}b\":\"x\"}})", '\u{c}'); + let err = parse_request(&src).unwrap_err(); + let msg = err.to_string(); + assert!( + matches!(err, DslError::InvalidValue { .. }), + "expected InvalidValue, got {err:?}" + ); + assert!( + !msg.to_lowercase().contains("double"), + "key-path error should not carry the value-path MCP double-escape teaching, got: {msg}" + ); +} + +#[test] +fn raw_control_byte_in_object_key_rejected() { + // #491: the literal-newline/CR/tab carve-out (ADR-016, PR #957) is scoped + // to quoted argument VALUES only. A quoted object KEY containing one of + // these bytes raw must still be rejected — only the escaped form (`\n`, + // `\r`, `\t`) is legal inside a key. + for raw in ['\n', '\r', '\t'] { + let src = format!("gtd.assign(properties={{\"a{raw}b\":\"x\"}})"); + let err = parse_request(&src).unwrap_err(); + assert!( + matches!(err, DslError::InvalidValue { .. }), + "expected InvalidValue for raw control byte {:#04x} in an object key, got {err:?}", + raw as u32 + ); + } +} + #[test] fn invalid_backslash_escape_in_quoted_string_rejected() { // A negative invalid-escape case: `\q` is not a JSON escape sequence. @@ -229,6 +306,184 @@ fn invalid_backslash_escape_in_quoted_string_rejected() { assert!(matches!(err, DslError::InvalidValue { .. })); } +#[test] +fn invalid_escape_with_unrelated_control_byte_not_misattributed() { + // #491 round-2 blocking fix 1(a): a `\q` invalid escape occurs before an + // unrelated form-feed later in the same span. The failure is the `\q`, + // not the control byte, so the control-char teaching diagnostic must NOT + // fire — misattributing the error to the wrong byte would send a caller + // chasing the wrong fix. + let src = format!("gtd.assign(title=\"bad \\q escape{}tail\")", '\u{c}'); + let err = parse_request(&src).unwrap_err(); + let msg = err.to_string(); + assert!(matches!(err, DslError::InvalidValue { .. })); + assert!( + !msg.to_lowercase().contains("double"), + "an invalid \\q escape must not be enriched with the control-char diagnostic, got: {msg}" + ); +} + +#[test] +fn malformed_unicode_escape_adjacent_to_control_byte_not_misattributed() { + // A malformed `\u` escape (only 3 hex digits) followed immediately by a + // raw control byte fails with serde's `InvalidEscape`, not a + // control-character error — but the failure's byte offset can coincide + // with the recorded control-byte hit (the 4th hex-digit slot serde reads + // is the control byte itself). Offset alone is not enough to gate the + // enrichment; the error's kind must also be checked, or this lands the + // control-char/double-escape guidance on an invalid-escape failure. + let src = format!("gtd.assign(title=\"bad \\u123{}tail\")", '\u{c}'); + let err = parse_request(&src).unwrap_err(); + let msg = err.to_string(); + assert!(matches!(err, DslError::InvalidValue { .. })); + assert!( + msg.contains("invalid escape"), + "expected the plain serde invalid-escape message, got: {msg}" + ); + assert!( + !msg.to_lowercase().contains("double"), + "a malformed \\u escape must not be enriched with the control-char diagnostic, got: {msg}" + ); +} + +#[test] +fn short_unicode_escape_backslash_adjacent_control_byte_not_misattributed() { + // A `\u` escape short by exactly 2 hex digits has its 3rd slot land on a + // raw backslash and its 4th slot land on a raw control byte — so the + // control byte IS physically adjacent to a backslash, unlike the + // 3-hex-digit case above. That adjacency is spurious: both bytes are + // consumed as `\u`'s own hex-digit slots, never reinterpreted as a fresh + // `\` escape pair. The failure must stay the plain malformed + // unicode-escape message with no double-escape guidance. + let src = format!("gtd.assign(title=\"bad \\u12\\{}tail\")", '\u{c}'); + let err = parse_request(&src).unwrap_err(); + let msg = err.to_string(); + assert!(matches!(err, DslError::InvalidValue { .. })); + assert!( + msg.contains("invalid escape"), + "expected the plain serde invalid-escape message, got: {msg}" + ); + assert!( + !msg.to_lowercase().contains("double"), + "a short \\u escape landing on a backslash+control-byte pair must not be \ + enriched with the control-char diagnostic, got: {msg}" + ); + + // A genuine broken `\` pair with no preceding `\u` run still gets + // the teaching diagnostic — the fix must not blind the guard to the + // legitimate case. + let genuine_src = format!("gtd.assign(title=\"before\\{}after\")", '\u{c}'); + let genuine_err = parse_request(&genuine_src).unwrap_err(); + let genuine_msg = genuine_err.to_string(); + assert!(matches!(genuine_err, DslError::InvalidValue { .. })); + assert!( + genuine_msg.to_lowercase().contains("double"), + "a genuine broken \\ pair must still get the double-escape guidance, got: {genuine_msg}" + ); +} + +#[test] +fn leading_surrogate_escape_followed_by_control_byte_not_misattributed() { + // #491 round-5: a well-formed high-surrogate `\u` escape (U+D800-U+DBFF) + // must be followed by a `\u` low-surrogate escape. When the byte after the + // continuation backslash is a raw control byte instead, `serde_json` fails + // on the malformed surrogate continuation, not on a broken `\` + // escape. Scan's general backslash-pair walk resets after the high + // surrogate's 4 hex slots, so without the surrogate-continuation guard it + // would record that `\` as a genuine broken pair + // (`preceded_by_backslash: true`) and land the double-escape teaching on a + // failure it does not explain. The continuation control byte must not be + // recorded, so the error stays serde's own surrogate message. + let src = format!("gtd.assign(title=\"bad \\uD800\\{}tail\")", '\u{c}'); + let err = parse_request(&src).unwrap_err(); + let msg = err.to_string(); + assert!(matches!(err, DslError::InvalidValue { .. })); + assert!( + !msg.to_lowercase().contains("double"), + "a control byte on a surrogate-continuation path must not be enriched \ + with the control-char double-escape diagnostic, got: {msg}" + ); + + // A broken `\` pair AFTER a complete surrogate pair (high + low) is a + // real broken escape and still gets the teaching; the suppression is scoped + // to the immediate continuation, not to everything after any `\u`. + let after_pair = format!("gtd.assign(title=\"ok \\uD800\\uDC00\\{}tail\")", '\u{c}'); + let after_pair_err = parse_request(&after_pair).unwrap_err(); + let after_pair_msg = after_pair_err.to_string(); + assert!(matches!(after_pair_err, DslError::InvalidValue { .. })); + assert!( + after_pair_msg.to_lowercase().contains("double"), + "a broken \\ pair after a complete surrogate pair must still get \ + the double-escape guidance, got: {after_pair_msg}" + ); +} + +#[test] +fn raw_newline_immediately_after_backslash_caught_as_control_char_cause() { + // #491 round-2 blocking fix 1(b): a raw LF directly following a + // backslash is not a valid two-byte JSON escape (valid escapes are + // backslash + an ASCII letter, never backslash + a literal control + // byte). This must be caught as a control-char cause and receive the + // teaching diagnostic, not silently fall through to the bare serde + // message. + let src = "gtd.assign(title=\"before\\\nafter\")"; + let err = parse_request(src).unwrap_err(); + let msg = err.to_string(); + assert!(matches!(err, DslError::InvalidValue { .. })); + assert!( + msg.contains(r#"\n"#) && msg.contains(r#"\t"#) && msg.contains(r#"\""#), + "error should name the JSON escape grammar, got: {msg}" + ); + assert!( + msg.to_lowercase().contains("double"), + "error should call out the MCP double-escape requirement, got: {msg}" + ); +} + +#[test] +fn raw_carriage_return_and_tab_after_backslash_also_caught() { + // Same defect as the LF case above, for CR and TAB. + for raw in ['\r', '\t'] { + let src = format!("gtd.assign(title=\"before\\{raw}after\")"); + let err = parse_request(&src).unwrap_err(); + assert!( + matches!(err, DslError::InvalidValue { .. }), + "expected InvalidValue for backslash + raw {:#04x}, got {err:?}", + raw as u32 + ); + let msg = err.to_string(); + assert!( + msg.to_lowercase().contains("double"), + "error should call out the MCP double-escape requirement for {:#04x}, got: {msg}", + raw as u32 + ); + } +} + +#[test] +fn backslash_followed_by_other_raw_control_bytes_also_caught() { + // #491 round-2 blocking fix 2: the post-backslash branch previously + // recorded a hit only for `\n`/`\r`/`\t`. A backslash followed by any + // OTHER raw U+0000-U+001F byte — NUL, form feed, and ESC here — is + // equally not a valid JSON escape and must reach the same teaching + // diagnostic instead of the bare serde message. + for raw in ['\u{0}', '\u{c}', '\u{1b}'] { + let src = format!("gtd.assign(title=\"before\\{raw}after\")"); + let err = parse_request(&src).unwrap_err(); + assert!( + matches!(err, DslError::InvalidValue { .. }), + "expected InvalidValue for backslash + raw {:#04x}, got {err:?}", + raw as u32 + ); + let msg = err.to_string(); + assert!( + msg.to_lowercase().contains("double"), + "error should call out the MCP double-escape requirement for {:#04x}, got: {msg}", + raw as u32 + ); + } +} + #[test] fn batch_ops_separated_by_raw_newlines() { // The top-level batch `,` separator may be split across literal