fix(request): teach JSON escape + MCP double-escape on control-char parse error (#491)#1098
Merged
Conversation
…arse error (closes #491)
…he helper, derive from source byte (#491) 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 <noreply@anthropic.com>
…re + borrow-fast-path the decoder (#491) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… and cover all post-backslash control bytes (#491) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
oceanwaves630
approved these changes
Jul 18, 2026
oceanwaves630
left a comment
oceanwaves630
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed against the verdict at head 3e555b9: zero blocking items, minors filed as follow-up. Approving.
added 4 commits
July 18, 2026 07:28
…y 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 `\<ctrl>` 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.
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.
…e 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.
…inuation (#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 `\<ctrl>` 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 `\<ctrl>` 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.
ohdearquant
marked this pull request as ready for review
July 18, 2026 15:02
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A raw control-character byte inside a quoted string in the function-call DSL surfaced the bare
serde_jsonmessage ("control character ... found while parsing a string"), which teaches nothingand sends callers toward the wrong workaround. This wraps the
serde_jsonstring-decode error at theparse_valuecall site so the message names the offending character and position, states that DSLstring escapes follow JSON (
\n,\t,\",\\), and notes that callers passingopsthrough aJSON transport (every MCP client) must double-escape (
\\nin the wire form, because the transportdecodes one level before the parser runs). Grammar unchanged (ADR-016 / ADR-084 §3c basis).
Scope note
PR #957 already normalizes raw
\n/\r/\tbytes before they reachserde_json, so those nolonger error at all. The bare-serde-message problem remained for the other raw control bytes (NUL,
form feed, ESC, ...); this addresses the error message for that remaining path.
Tests
control_char_error_teaches_escape_syntax_and_mcp_double_escapefeeds a raw form-feed byte in aquoted value and asserts the error is
DslError::InvalidValuewith a message carrying the escapegrammar and the double-escape guidance. Non-control-character parse errors pass through unchanged.
Gates:
cargo test -p khive-request(122 passed),cargo fmt --all -- --check,cargo clippy -p khive-request --all-targets -- -D warningsall clean.Closes #491.