fix(request): accept literal newlines in DSL string values#957
Conversation
|
APPROVE-WITH-FIXES Findings: 0 Blocker, 0 High, 2 Medium, 0 Low [Medium] ADR-016 does not state the grammar the lexer actually acceptsEvidence: Why this matters: This is the request DSL's public grammar. The accepted ADR must either specify the full accepted control-character set or the implementation must restrict the exception to the three characters the ADR names; otherwise callers cannot reliably know whether other raw control characters are valid. Suggested fix: Decide the intended set, then align both surfaces: either limit [Medium] The regression tests omit the requested grammar-boundary matrixEvidence: the new tests at Why this matters: These are the boundaries that distinguish literal string content from structural whitespace and ensure the new scanner does not change escaping, batching, chaining, or public MCP behavior. The current assertions also establish only the Suggested fix: Add focused parser tests for each listed boundary (including exact CRLF and terminal-LF values), a negative bareword/invalid-escape case, and newline-formatted function-call batch and chain inputs. Add one Looks Right
Commands Run
What I Did Not Check
Re-Review GuidanceNarrow re-review after the ADR/implementation decision and boundary tests land; rerun the complete required gate sequence. Domain utility: MEDIUM — the khive contract-review rubric helped prioritize ADR-to-lexer and wire-surface consistency. |
Round-1 review (PR #957) flagged that escape_literal_control_chars rewrote every raw U+0000-U+001F byte in a quoted DSL string, but ADR-016 only documents newline/CR/tab as the literal exception. Narrow the rewrite to exactly those three bytes so every other raw control character falls through to serde_json and is rejected, matching pre-PR behavior. Clarify ADR-016 with one sentence stating the same. Add the grammar-boundary test matrix requested in review: CRLF preservation, a literal newline directly before the closing quote, a raw newline immediately after an escaped-quote boundary, rejection of raw controls in bareword position and for every other control byte, an invalid-escape negative case, newline-split batch/chain separators, and an mcp request-boundary test proving a wire-decoded raw newline reaches the pack handler's result. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ky tx-age sweep run_writer_task registers a writer_task_tx handle in the process-wide tx_registry singleton; the checkpoint tx_age_sweep_* tests read that singleton under #[serial(tx_registry)]. The writer_task.rs tests that spawn a writer task were missing from the group, leaking a longer-lived writer_task_tx into the sweep's oldest() read and intermittently failing the assertion on main CI. Join them to the serial group. No prod change.
|
Codex static review: APPROVE at 7c39d85 (0 blocking findings). |
…ng/length bounds Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
oceanwaves630
left a comment
oceanwaves630
left a comment
There was a problem hiding this comment.
Independent full-diff read at head 427113f. Approving.
The core rewrite (escape_literal_control_chars) is correct on the tricky part: it consumes a backslash plus its next byte as a unit, so existing escape pairs (\", \\, and \n/\r/\t written as escapes) are copied through untouched and never reinterpreted, while only raw LF/CR/TAB bytes are rewritten to their JSON-escape form. It iterates Unicode scalars, so multi-byte UTF-8 is passed through verbatim with no byte-slicing hazard, and the fast-path early return keeps the common (no-control-char) case allocation-free.
The carve-out is correctly scoped: parse_value only invokes the rewrite when the trimmed slice starts with ", so numbers, bool, null, and bareword positions never touch it — a raw control byte cannot smuggle into a non-string value. Everything else stays fail-closed: other U+0000–U+001F controls, controls in bareword position, the strict-JSON ops="[...]" form, and invalid \q-style escapes all still reject, matching pre-change behavior. A lone unescaped backslash before a raw newline was already malformed and still rejects — no regression.
Test coverage is thorough and matches the grammar boundary: CRLF byte-for-byte, trailing newline immediately before the closing quote, escape-pair adjacency to a raw newline, batch/chain separators split across newlines, plus the full negative matrix, and a server-level wire-boundary test proving a JSON-RPC \n escape decodes to a raw LF that survives parse and dispatch to the handler result. The ADR-016 update states the exception precisely and matches the implementation.
Additive, fail-closed, well-tested. CI green. Merging on approve.
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.
…arse error (#491) (#1098) * fix(request): teach JSON escape + MCP double-escape on control-char parse error (closes #491) * fix(request): enrich control-char diagnostic for quoted keys, share the 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> * fix(request): scope control-byte diagnostic to the actual parse failure + borrow-fast-path the decoder (#491) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * fix(request): gate control-char parse enrichment on serde error kind (#491) * 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 `\<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. * 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. * 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. * 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 `\<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. --------- Co-authored-by: Leo <noreply@khive.ai> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
2 participants
Summary
Resolves #952. The function-call form of the request DSL rejected literal newline, carriage-return, and tab characters inside double-quoted string values, forcing agents to collapse multi-paragraph content (emails especially) onto one line with backslash escapes.
The string lexer now accepts those literal control characters as part of the string value verbatim. Backslash escapes are unchanged. The JSON form is unchanged (raw control characters remain invalid there per the JSON spec, handled by serde_json).
ADR-016 gets a one-paragraph grammar clarification for the accepted string syntax.
Tests