fix(request): teach JSON escape + MCP double-escape on control-char parse error (#491)

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\\\")\"}";

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<usize> {
+    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 `\<ctrl>` 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<String, DslError> {
+    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<String, DslError> {
+    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 {