From 7f8c100b31b4b686e9f74f68b17b35bf156b09e9 Mon Sep 17 00:00:00 2001 From: Leo Date: Fri, 17 Jul 2026 21:18:44 -0400 Subject: [PATCH 1/9] fix(request): teach JSON escape + MCP double-escape on control-char parse error (closes #491) --- .../khive-request/src/parser/parser_impl.rs | 28 ++++++++++++++++++- crates/khive-request/tests/parser.rs | 24 ++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/crates/khive-request/src/parser/parser_impl.rs b/crates/khive-request/src/parser/parser_impl.rs index c3a724f94..bc8580939 100644 --- a/crates/khive-request/src/parser/parser_impl.rs +++ b/crates/khive-request/src/parser/parser_impl.rs @@ -401,7 +401,7 @@ impl<'a> Parser<'a> { }; let value: Value = serde_json::from_str(json_src).map_err(|e| DslError::InvalidValue { pos: start, - error: e.to_string(), + error: describe_value_parse_error(&e, json_src), })?; self.pos = end; Ok(value) @@ -503,6 +503,32 @@ impl<'a> Parser<'a> { /// Validates quoted-reference brackets before promoting a string to `PrevRef`. /// A malformed segment keeps the entire value literal. +/// Enriches a `serde_json` string-decode failure with the ADR-016 escape +/// grammar and the MCP double-escape gotcha. Raw newline/CR/tab bytes are +/// already normalized before `json_src` reaches `serde_json` (see +/// `escape_literal_control_chars`), so any remaining control-character +/// failure here is a byte outside that carve-out (NUL, form feed, ESC, ...); +/// the bare serde message alone ("control character ... found while parsing +/// a string") does not tell a caller what to do about it. +fn describe_value_parse_error(e: &serde_json::Error, json_src: &str) -> String { + let base = e.to_string(); + if !base.contains("control character") { + return base; + } + let offending = json_src + .char_indices() + .find(|&(_, c)| (c as u32) < 0x20) + .map(|(idx, c)| format!(" — byte {idx} of the value is {c:?} (U+{:04X})", c as u32)) + .unwrap_or_default(); + format!( + "{base}{offending}. 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 (every MCP client), 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." + ) +} + fn quoted_prev_path_is_valid(path: &str) -> bool { let bytes = path.as_bytes(); let mut i = 0; diff --git a/crates/khive-request/tests/parser.rs b/crates/khive-request/tests/parser.rs index 0c3971af5..d12be05fe 100644 --- a/crates/khive-request/tests/parser.rs +++ b/crates/khive-request/tests/parser.rs @@ -220,6 +220,30 @@ 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 invalid_backslash_escape_in_quoted_string_rejected() { // A negative invalid-escape case: `\q` is not a JSON escape sequence. From f7a60871d7f19dc106e72fab16be067a304fba4f Mon Sep 17 00:00:00 2001 From: Leo Date: Fri, 17 Jul 2026 21:59:58 -0400 Subject: [PATCH 2/9] fix(request): enrich control-char diagnostic for quoted keys, share the helper, derive from source byte (#491) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Route quoted object-key JSON string decoding through the same decode_quoted_json_string helper used by quoted values, so the ADR-084 §3c escape-grammar/MCP-double-escape diagnostic applies to both paths instead of only values. Classify and locate the offending control byte by scanning the known quoted source span directly (find_offending_control_byte) rather than matching serde_json::Error's message text, and bound that scan to the already-known quoted span instead of rescanning the full ops source. Fix a misattached rustdoc comment that had described quoted_prev_path_is_valid while sitting above the now-removed describe_value_parse_error. Co-Authored-By: Claude Sonnet 5 --- .../khive-request/src/parser/parser_impl.rs | 99 ++++++++++++------- crates/khive-request/tests/parser.rs | 24 +++++ 2 files changed, 85 insertions(+), 38 deletions(-) diff --git a/crates/khive-request/src/parser/parser_impl.rs b/crates/khive-request/src/parser/parser_impl.rs index bc8580939..205ddb3e7 100644 --- a/crates/khive-request/src/parser/parser_impl.rs +++ b/crates/khive-request/src/parser/parser_impl.rs @@ -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_string(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: describe_value_parse_error(&e, json_src), - })?; self.pos = end; Ok(value) } @@ -501,34 +495,63 @@ impl<'a> Parser<'a> { } } -/// Validates quoted-reference brackets before promoting a string to `PrevRef`. -/// A malformed segment keeps the entire value literal. +/// Finds the first byte in a quoted-string DSL source span that could not +/// have been normalized away by [`escape_literal_control_chars`] and would +/// therefore reach `serde_json` as a literal control byte. Raw `\n`/`\r`/`\t` +/// are rewritten to JSON escapes before parsing (ADR-016), so they are never +/// the cause of a "control character" decode failure; any other byte below +/// 0x20 (NUL, form feed, ESC, ...) is. Scanning `raw` directly bounds the +/// search to the already-known quoted span, rather than the full (up to +/// 1 MiB) `ops` source. +fn find_offending_control_byte(raw: &str) -> Option<(usize, char)> { + raw.bytes().enumerate().find_map(|(idx, b)| { + if b < 0x20 && !matches!(b, b'\n' | b'\r' | b'\t') { + Some((idx, b as char)) + } else { + None + } + }) +} + /// Enriches a `serde_json` string-decode failure with the ADR-016 escape -/// grammar and the MCP double-escape gotcha. Raw newline/CR/tab bytes are -/// already normalized before `json_src` reaches `serde_json` (see -/// `escape_literal_control_chars`), so any remaining control-character -/// failure here is a byte outside that carve-out (NUL, form feed, ESC, ...); -/// the bare serde message alone ("control character ... found while parsing -/// a string") does not tell a caller what to do about it. -fn describe_value_parse_error(e: &serde_json::Error, json_src: &str) -> String { +/// grammar and the MCP double-escape gotcha (ADR-084 §3c). Classification +/// and byte location come from scanning the known quoted source span +/// directly (`raw`) via [`find_offending_control_byte`], never from matching +/// `serde_json::Error`'s message text, which is not a stable contract. +/// Failures with no control byte in `raw` (e.g. an invalid `\q` escape) fall +/// through to the plain serde message unchanged. +fn describe_quoted_string_parse_error(e: &serde_json::Error, raw: &str) -> String { let base = e.to_string(); - if !base.contains("control character") { + let Some((idx, c)) = find_offending_control_byte(raw) else { return base; - } - let offending = json_src - .char_indices() - .find(|&(_, c)| (c as u32) < 0x20) - .map(|(idx, c)| format!(" — byte {idx} of the value is {c:?} (U+{:04X})", c as u32)) - .unwrap_or_default(); + }; format!( - "{base}{offending}. 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 (every MCP client), 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." + "{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 (every MCP client), \ + 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`]. Shared by quoted object keys +/// (`parse_object_arg_body`) and quoted string values (`parse_value`) so the +/// diagnostic cannot drift between the two paths. +fn decode_quoted_json_string(raw: &str, pos: usize) -> Result { + let normalized = escape_literal_control_chars(raw); + serde_json::from_str(&normalized).map_err(|e| DslError::InvalidValue { + pos, + error: describe_quoted_string_parse_error(&e, raw), + }) +} + +/// 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 { let bytes = path.as_bytes(); let mut i = 0; diff --git a/crates/khive-request/tests/parser.rs b/crates/khive-request/tests/parser.rs index d12be05fe..641342da3 100644 --- a/crates/khive-request/tests/parser.rs +++ b/crates/khive-request/tests/parser.rs @@ -244,6 +244,30 @@ fn control_char_error_teaches_escape_syntax_and_mcp_double_escape() { ); } +#[test] +fn control_char_in_object_key_teaches_escape_syntax_and_mcp_double_escape() { + // #491 follow-up: quoted object KEYS decode through a separate path from + // quoted values (`parse_object_arg_body` vs `parse_value`) and used to + // bypass the enrichment entirely, returning the bare serde message. Both + // paths now share `decode_quoted_json_string`, so a raw control byte in + // a key gets the same teaching diagnostic as one in a value. + 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.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 invalid_backslash_escape_in_quoted_string_rejected() { // A negative invalid-escape case: `\q` is not a JSON escape sequence. From c7e54d9beca0a6ae66bf9b2dd25d6c7be7a0d2c2 Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 18 Jul 2026 00:29:27 -0400 Subject: [PATCH 3/9] fix(request): scope control-byte diagnostic to the actual parse failure + borrow-fast-path the decoder (#491) Co-Authored-By: Claude Sonnet 5 --- .../khive-request/src/parser/parser_impl.rs | 85 +++++++++++++------ crates/khive-request/src/parser/scan.rs | 80 ++++++++++++++--- crates/khive-request/tests/parser.rs | 59 +++++++++++++ 3 files changed, 184 insertions(+), 40 deletions(-) diff --git a/crates/khive-request/src/parser/parser_impl.rs b/crates/khive-request/src/parser/parser_impl.rs index 205ddb3e7..785f423b7 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> { @@ -495,36 +495,67 @@ impl<'a> Parser<'a> { } } -/// Finds the first byte in a quoted-string DSL source span that could not -/// have been normalized away by [`escape_literal_control_chars`] and would -/// therefore reach `serde_json` as a literal control byte. Raw `\n`/`\r`/`\t` -/// are rewritten to JSON escapes before parsing (ADR-016), so they are never -/// the cause of a "control character" decode failure; any other byte below -/// 0x20 (NUL, form feed, ESC, ...) is. Scanning `raw` directly bounds the -/// search to the already-known quoted span, rather than the full (up to -/// 1 MiB) `ops` source. -fn find_offending_control_byte(raw: &str) -> Option<(usize, char)> { - raw.bytes().enumerate().find_map(|(idx, b)| { - if b < 0x20 && !matches!(b, b'\n' | b'\r' | b'\t') { - Some((idx, b as char)) - } else { - None +/// 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). Classification -/// and byte location come from scanning the known quoted source span -/// directly (`raw`) via [`find_offending_control_byte`], never from matching -/// `serde_json::Error`'s message text, which is not a stable contract. -/// Failures with no control byte in `raw` (e.g. an invalid `\q` escape) fall -/// through to the plain serde message unchanged. -fn describe_quoted_string_parse_error(e: &serde_json::Error, raw: &str) -> String { +/// grammar and the MCP double-escape gotcha (ADR-084 §3c). Enrichment is +/// gated on the failure being AT a recorded [`ControlByteHit`]: the serde +/// error's `(line, column)` is mapped to a byte offset in `normalized.text` +/// via [`serde_error_byte_offset`], then matched against +/// `normalized.control_bytes` (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. +fn describe_quoted_string_parse_error( + e: &serde_json::Error, + normalized: &NormalizedQuotedString<'_>, +) -> String { let base = e.to_string(); - let Some((idx, c)) = find_offending_control_byte(raw) else { + let Some(offset) = serde_error_byte_offset(e, normalized.text.as_ref()) else { + return base; + }; + let Some(hit) = normalized + .control_bytes + .iter() + .find(|h| h.normalized_pos == offset) + else { return base; }; + let idx = hit.raw_pos; + 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 \ @@ -543,10 +574,10 @@ fn describe_quoted_string_parse_error(e: &serde_json::Error, raw: &str) -> Strin /// (`parse_object_arg_body`) and quoted string values (`parse_value`) so the /// diagnostic cannot drift between the two paths. fn decode_quoted_json_string(raw: &str, pos: usize) -> Result { - let normalized = escape_literal_control_chars(raw); - serde_json::from_str(&normalized).map_err(|e| DslError::InvalidValue { + 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, raw), + error: describe_quoted_string_parse_error(&e, &normalized), }) } diff --git a/crates/khive-request/src/parser/scan.rs b/crates/khive-request/src/parser/scan.rs index 7415ad3f8..ef5a1c052 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,70 @@ pub(crate) fn scan_string_end(src: &[u8], start: usize) -> Result { + pub(crate) text: Cow<'a, str>, + pub(crate) control_bytes: Vec, +} + +/// 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 a raw `\n`/`\r`/`\t` 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 a +/// [`ControlByteHit`] 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. +pub(crate) fn normalize_quoted_string(raw: &str) -> NormalizedQuotedString<'_> { + if !raw.bytes().any(|b| b < 0x20) { + return NormalizedQuotedString { + text: Cow::Borrowed(raw), + control_bytes: Vec::new(), + }; } - 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 control_bytes = Vec::new(); + let mut chars = raw.char_indices().peekable(); + while let Some((pos, c)) = chars.next() { 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 matches!(next_c, '\n' | '\r' | '\t') { + control_bytes.push(ControlByteHit { + normalized_pos: out.len(), + raw_pos: next_pos, + byte: next_c as u8, + }); + } + out.push(next_c); } continue; } @@ -50,10 +93,21 @@ 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 => { + control_bytes.push(ControlByteHit { + normalized_pos: out.len(), + raw_pos: pos, + byte: c as u8, + }); + out.push(c); + } c => out.push(c), } } - out + NormalizedQuotedString { + text: Cow::Owned(out), + control_bytes, + } } /// Returns a stable delimiter label for diagnostics. diff --git a/crates/khive-request/tests/parser.rs b/crates/khive-request/tests/parser.rs index 641342da3..6c132665b 100644 --- a/crates/khive-request/tests/parser.rs +++ b/crates/khive-request/tests/parser.rs @@ -277,6 +277,65 @@ 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 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 batch_ops_separated_by_raw_newlines() { // The top-level batch `,` separator may be split across literal From 3e555b901bc3f4d0d71b7f02bcecda24894685d3 Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 18 Jul 2026 03:12:05 -0400 Subject: [PATCH 4/9] fix(request): bound control-byte diagnostic metadata to the first hit and cover all post-backslash control bytes (#491) Co-Authored-By: Claude Sonnet 5 --- crates/khive-mcp/src/server.rs | 2 +- .../khive-request/src/parser/parser_impl.rs | 21 +++--- crates/khive-request/src/parser/scan.rs | 74 +++++++++++++------ crates/khive-request/tests/parser.rs | 26 ++++++- 4 files changed, 88 insertions(+), 35 deletions(-) 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 785f423b7..4f32219a8 100644 --- a/crates/khive-request/src/parser/parser_impl.rs +++ b/crates/khive-request/src/parser/parser_impl.rs @@ -531,14 +531,15 @@ fn serde_error_byte_offset(e: &serde_json::Error, text: &str) -> Option { /// 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 a recorded [`ControlByteHit`]: the serde +/// 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 matched against -/// `normalized.control_bytes` (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. +/// 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. fn describe_quoted_string_parse_error( e: &serde_json::Error, normalized: &NormalizedQuotedString<'_>, @@ -548,9 +549,9 @@ fn describe_quoted_string_parse_error( return base; }; let Some(hit) = normalized - .control_bytes - .iter() - .find(|h| h.normalized_pos == offset) + .first_control_byte + .as_ref() + .filter(|h| h.normalized_pos == offset) else { return base; }; diff --git a/crates/khive-request/src/parser/scan.rs b/crates/khive-request/src/parser/scan.rs index ef5a1c052..b8cb441ec 100644 --- a/crates/khive-request/src/parser/scan.rs +++ b/crates/khive-request/src/parser/scan.rs @@ -23,12 +23,13 @@ pub(crate) fn scan_string_end(src: &[u8], start: usize) -> Result { pub(crate) text: Cow<'a, str>, - pub(crate) control_bytes: Vec, + pub(crate) first_control_byte: Option, } /// Rewrites raw literal newline, carriage return, and tab bytes inside a @@ -50,10 +56,11 @@ pub(crate) struct NormalizedQuotedString<'a> { /// 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 — EXCEPT that a backslash directly -/// followed by a raw `\n`/`\r`/`\t` 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 a -/// [`ControlByteHit`] even though it is copied through unrewritten; the +/// 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). /// @@ -67,19 +74,19 @@ pub(crate) fn normalize_quoted_string(raw: &str) -> NormalizedQuotedString<'_> { if !raw.bytes().any(|b| b < 0x20) { return NormalizedQuotedString { text: Cow::Borrowed(raw), - control_bytes: Vec::new(), + first_control_byte: None, }; } let mut out = String::with_capacity(raw.len() + 8); - let mut control_bytes = Vec::new(); + let mut first_control_byte = None; let mut chars = raw.char_indices().peekable(); while let Some((pos, c)) = chars.next() { if c == '\\' { out.push(c); if let Some(&(next_pos, next_c)) = chars.peek() { chars.next(); - if matches!(next_c, '\n' | '\r' | '\t') { - control_bytes.push(ControlByteHit { + if (next_c as u32) < 0x20 && first_control_byte.is_none() { + first_control_byte = Some(ControlByteHit { normalized_pos: out.len(), raw_pos: next_pos, byte: next_c as u8, @@ -94,11 +101,13 @@ pub(crate) fn normalize_quoted_string(raw: &str) -> NormalizedQuotedString<'_> { '\r' => out.push_str("\\r"), '\t' => out.push_str("\\t"), c if (c as u32) < 0x20 => { - control_bytes.push(ControlByteHit { - normalized_pos: out.len(), - raw_pos: pos, - byte: c as u8, - }); + if first_control_byte.is_none() { + first_control_byte = Some(ControlByteHit { + normalized_pos: out.len(), + raw_pos: pos, + byte: c as u8, + }); + } out.push(c); } c => out.push(c), @@ -106,7 +115,7 @@ pub(crate) fn normalize_quoted_string(raw: &str) -> NormalizedQuotedString<'_> { } NormalizedQuotedString { text: Cow::Owned(out), - control_bytes, + first_control_byte, } } @@ -199,3 +208,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 6c132665b..431c3d5cb 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}'); @@ -336,6 +336,30 @@ fn raw_carriage_return_and_tab_after_backslash_also_caught() { } } +#[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 From 93a918123060ac62fe85215e550cf5c94e122ce5 Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 18 Jul 2026 05:34:37 -0400 Subject: [PATCH 5/9] fix(request): gate control-char parse enrichment on serde error kind (#491) --- .../khive-request/src/parser/parser_impl.rs | 17 ++++++++++++++ crates/khive-request/src/parser/scan.rs | 14 ++++++++++- crates/khive-request/tests/parser.rs | 23 +++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/crates/khive-request/src/parser/parser_impl.rs b/crates/khive-request/src/parser/parser_impl.rs index 4f32219a8..de2572780 100644 --- a/crates/khive-request/src/parser/parser_impl.rs +++ b/crates/khive-request/src/parser/parser_impl.rs @@ -540,6 +540,20 @@ fn serde_error_byte_offset(e: &serde_json::Error, text: &str) -> Option { /// 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<'_>, @@ -555,6 +569,9 @@ fn describe_quoted_string_parse_error( else { return base; }; + if !hit.preceded_by_backslash && !base.starts_with("control character") { + return base; + } let idx = hit.raw_pos; let c = hit.byte as char; format!( diff --git a/crates/khive-request/src/parser/scan.rs b/crates/khive-request/src/parser/scan.rs index b8cb441ec..a89d0f81c 100644 --- a/crates/khive-request/src/parser/scan.rs +++ b/crates/khive-request/src/parser/scan.rs @@ -29,11 +29,21 @@ 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. 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`, @@ -90,6 +100,7 @@ pub(crate) fn normalize_quoted_string(raw: &str) -> NormalizedQuotedString<'_> { normalized_pos: out.len(), raw_pos: next_pos, byte: next_c as u8, + preceded_by_backslash: true, }); } out.push(next_c); @@ -106,6 +117,7 @@ pub(crate) fn normalize_quoted_string(raw: &str) -> NormalizedQuotedString<'_> { normalized_pos: out.len(), raw_pos: pos, byte: c as u8, + preceded_by_backslash: false, }); } out.push(c); diff --git a/crates/khive-request/tests/parser.rs b/crates/khive-request/tests/parser.rs index 431c3d5cb..7f9c171c5 100644 --- a/crates/khive-request/tests/parser.rs +++ b/crates/khive-request/tests/parser.rs @@ -294,6 +294,29 @@ fn invalid_escape_with_unrelated_control_byte_not_misattributed() { ); } +#[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 raw_newline_immediately_after_backslash_caught_as_control_char_cause() { // #491 round-2 blocking fix 1(b): a raw LF directly following a From 62518f07d3526efcff74dfc84309ff379ca0fbac Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 18 Jul 2026 07:28:42 -0400 Subject: [PATCH 6/9] fix(request): classify a preceding malformed unicode escape distinctly from a broken control-byte pair (#491) A `\u` escape short by exactly 2 hex digits has its 3rd hex-digit slot land on a raw backslash and its 4th slot land on a raw control byte. The control byte is then physically adjacent to a backslash, so the scanner recorded it as a genuine broken `\` pair and the parse-error guard appended the MCP double-escape guidance for what is actually a malformed unicode escape (both fail with serde_json's identical InvalidEscape kind and Display text, so the guard cannot tell them apart by the error alone). normalize_quoted_string now treats the 4 characters following `\u` as an opaque hex-digit-slot window, mirroring serde_json's decode_hex_escape: they are copied through verbatim and never re-enter the general backslash-pair branch, so a byte inside that window is never recorded as a ControlByteHit regardless of what it is. --- crates/khive-request/src/parser/scan.rs | 34 ++++++++++++++++++++++- crates/khive-request/tests/parser.rs | 36 +++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/crates/khive-request/src/parser/scan.rs b/crates/khive-request/src/parser/scan.rs index a89d0f81c..ac7ae6a3d 100644 --- a/crates/khive-request/src/parser/scan.rs +++ b/crates/khive-request/src/parser/scan.rs @@ -38,7 +38,10 @@ pub(crate) fn scan_string_end(src: &[u8], start: usize) -> Result { /// 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. pub(crate) fn normalize_quoted_string(raw: &str) -> NormalizedQuotedString<'_> { if !raw.bytes().any(|b| b < 0x20) { return NormalizedQuotedString { @@ -90,11 +111,22 @@ pub(crate) fn normalize_quoted_string(raw: &str) -> NormalizedQuotedString<'_> { 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; while let Some((pos, c)) = chars.next() { + if hex_slots_remaining > 0 { + hex_slots_remaining -= 1; + out.push(c); + continue; + } if c == '\\' { out.push(c); if let Some(&(next_pos, next_c)) = chars.peek() { chars.next(); + if next_c == 'u' { + out.push(next_c); + hex_slots_remaining = 4; + continue; + } if (next_c as u32) < 0x20 && first_control_byte.is_none() { first_control_byte = Some(ControlByteHit { normalized_pos: out.len(), diff --git a/crates/khive-request/tests/parser.rs b/crates/khive-request/tests/parser.rs index 7f9c171c5..b7bd266ca 100644 --- a/crates/khive-request/tests/parser.rs +++ b/crates/khive-request/tests/parser.rs @@ -317,6 +317,42 @@ fn malformed_unicode_escape_adjacent_to_control_byte_not_misattributed() { ); } +#[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 raw_newline_immediately_after_backslash_caught_as_control_char_cause() { // #491 round-2 blocking fix 1(b): a raw LF directly following a From 12e76f0947a3557de8105bde0485d588a2b95787 Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 18 Jul 2026 08:40:00 -0400 Subject: [PATCH 7/9] fix(request): keep strict JSON decoding for object keys (#491) Object keys decoded through the same normalize_quoted_string path as argument values, so a raw literal newline/CR/tab byte inside a quoted object key parsed successfully instead of being rejected. Per ADR-016 and PR #957, that literal-control-byte carve-out is scoped to quoted argument values only. Keys now decode via strict serde_json semantics, independent of the value-path normalization and its diagnostic enrichment. --- .../khive-request/src/parser/parser_impl.rs | 22 +++++++++-- crates/khive-request/tests/parser.rs | 38 +++++++++++++------ 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/crates/khive-request/src/parser/parser_impl.rs b/crates/khive-request/src/parser/parser_impl.rs index de2572780..4d404091f 100644 --- a/crates/khive-request/src/parser/parser_impl.rs +++ b/crates/khive-request/src/parser/parser_impl.rs @@ -248,7 +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 = decode_quoted_json_string(raw, start)?; + let s = decode_quoted_json_key(raw, start)?; self.pos = end; s } @@ -588,9 +588,9 @@ fn describe_quoted_string_parse_error( /// 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`]. Shared by quoted object keys -/// (`parse_object_arg_body`) and quoted string values (`parse_value`) so the -/// diagnostic cannot drift between the two paths. +/// [`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 { @@ -599,6 +599,20 @@ fn decode_quoted_json_string(raw: &str, pos: usize) -> Result }) } +/// 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/tests/parser.rs b/crates/khive-request/tests/parser.rs index b7bd266ca..e4933013a 100644 --- a/crates/khive-request/tests/parser.rs +++ b/crates/khive-request/tests/parser.rs @@ -245,12 +245,13 @@ fn control_char_error_teaches_escape_syntax_and_mcp_double_escape() { } #[test] -fn control_char_in_object_key_teaches_escape_syntax_and_mcp_double_escape() { - // #491 follow-up: quoted object KEYS decode through a separate path from - // quoted values (`parse_object_arg_body` vs `parse_value`) and used to - // bypass the enrichment entirely, returning the bare serde message. Both - // paths now share `decode_quoted_json_string`, so a raw control byte in - // a key gets the same teaching diagnostic as one in a value. +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(); @@ -259,15 +260,28 @@ fn control_char_in_object_key_teaches_escape_syntax_and_mcp_double_escape() { "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}" + !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. From eee6dbe486ea8e734450c5cd1e6e7924dcf22a8e Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 18 Jul 2026 09:36:31 -0400 Subject: [PATCH 8/9] fix(request): report the control-byte diagnostic index relative to the value (#491) The value-position diagnostic counted the byte offset from the span opening quote, so it reported an index one past the actual position within the value. Report it relative to the value; add a regression test asserting a leading control byte is byte 0 of the value. --- crates/khive-request/src/parser/parser_impl.rs | 4 +++- crates/khive-request/tests/parser.rs | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/crates/khive-request/src/parser/parser_impl.rs b/crates/khive-request/src/parser/parser_impl.rs index 4d404091f..15db4a40f 100644 --- a/crates/khive-request/src/parser/parser_impl.rs +++ b/crates/khive-request/src/parser/parser_impl.rs @@ -572,7 +572,9 @@ fn describe_quoted_string_parse_error( if !hit.preceded_by_backslash && !base.starts_with("control character") { return base; } - let idx = hit.raw_pos; + // `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: \ diff --git a/crates/khive-request/tests/parser.rs b/crates/khive-request/tests/parser.rs index e4933013a..f892da725 100644 --- a/crates/khive-request/tests/parser.rs +++ b/crates/khive-request/tests/parser.rs @@ -244,6 +244,21 @@ fn control_char_error_teaches_escape_syntax_and_mcp_double_escape() { ); } +#[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 From 0d784fc44558bf978a1cec36e383ecc8ee90f81d Mon Sep 17 00:00:00 2001 From: Leo Date: Sat, 18 Jul 2026 10:36:52 -0400 Subject: [PATCH 9/9] fix(request): suppress double-escape hint on malformed surrogate continuation (#491) A 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, serde_json fails on the malformed surrogate pair, but the quoted-string scanner reset its escape state after the four hex slots and recorded that `\` as a genuine broken control-byte pair. The enriched error then attached the JSON double-escape guidance to a failure the control byte does not explain. Track high-surrogate completion in normalize_quoted_string so the immediate continuation control byte is not recorded as a control-byte hit; the error stays serde_json's own surrogate message. A broken `\` pair after a complete surrogate pair is unaffected and still receives the guidance. Also genericize the double-escape hint: it described the JSON transport as "every MCP client", which conflicts with the transport-agnostic contract of ADR-016. The hint now refers to a JSON transport generally. --- .../khive-request/src/parser/parser_impl.rs | 4 +-- crates/khive-request/src/parser/scan.rs | 30 +++++++++++++++- crates/khive-request/tests/parser.rs | 36 +++++++++++++++++++ 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/crates/khive-request/src/parser/parser_impl.rs b/crates/khive-request/src/parser/parser_impl.rs index 15db4a40f..e20a17a8c 100644 --- a/crates/khive-request/src/parser/parser_impl.rs +++ b/crates/khive-request/src/parser/parser_impl.rs @@ -579,8 +579,8 @@ fn describe_quoted_string_parse_error( 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 (every MCP client), \ - the transport decodes one escape level before the DSL parser runs, so a literal \ + 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 ) diff --git a/crates/khive-request/src/parser/scan.rs b/crates/khive-request/src/parser/scan.rs index ac7ae6a3d..423a568f1 100644 --- a/crates/khive-request/src/parser/scan.rs +++ b/crates/khive-request/src/parser/scan.rs @@ -101,6 +101,14 @@ pub(crate) struct NormalizedQuotedString<'a> { /// 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 { @@ -112,12 +120,27 @@ pub(crate) fn normalize_quoted_string(raw: &str) -> NormalizedQuotedString<'_> { 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_pos, next_c)) = chars.peek() { @@ -125,9 +148,14 @@ pub(crate) fn normalize_quoted_string(raw: &str) -> NormalizedQuotedString<'_> { 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 && first_control_byte.is_none() { + 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, diff --git a/crates/khive-request/tests/parser.rs b/crates/khive-request/tests/parser.rs index f892da725..14bfe1fd6 100644 --- a/crates/khive-request/tests/parser.rs +++ b/crates/khive-request/tests/parser.rs @@ -382,6 +382,42 @@ fn short_unicode_escape_backslash_adjacent_control_byte_not_misattributed() { ); } +#[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