diff --git a/crates/khive-mcp/src/server.rs b/crates/khive-mcp/src/server.rs index 0140f0c2b..2e7822986 100644 --- a/crates/khive-mcp/src/server.rs +++ b/crates/khive-mcp/src/server.rs @@ -2339,6 +2339,60 @@ mod tests { } } + // ── request-boundary regression: raw controls survive wire decoding ───── + + #[tokio::test] + async fn request_boundary_raw_control_bytes_reach_handler() { + // Simulates the actual MCP wire: a JSON-RPC client sends the tool's + // `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) + // 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\\\")\"}"; + let params: RequestParams = serde_json::from_str(wire).expect("wire JSON deserializes"); + assert!( + params.ops.contains('\n'), + "deserialized ops must carry a raw LF, not the two-char escape: {:?}", + params.ops + ); + + let config = RuntimeConfig { + db_path: None, + default_namespace: Namespace::local(), + embedding_model: None, + additional_embedding_models: vec![], + packs: vec!["kg".to_string()], + ..RuntimeConfig::default() + }; + let runtime = KhiveRuntime::new(config).expect("in-memory runtime"); + let server = KhiveMcpServer::new(runtime).expect("server builds with kg"); + + let parsed = parse_request(¶ms.ops).expect("literal newline inside quotes must parse"); + let response = server + .run_parsed( + parsed.ops, + parsed.mode, + PresentationMode::Verbose, + None, + false, + None, + ) + .await; + + let results = response["results"] + .as_array() + .expect("results must be an array"); + assert_eq!(results.len(), 1); + assert_eq!( + results[0]["ok"], + json!(true), + "unexpected result: {response:?}" + ); + assert_eq!(results[0]["result"]["name"], json!("line1\nline2")); + } + // ── MCP-AUD-002 regression: save_to must bypass daemon forwarding ──────── fn make_daemon_save_to_test_server() -> KhiveMcpServer { diff --git a/crates/khive-request/src/parser/parser_impl.rs b/crates/khive-request/src/parser/parser_impl.rs index cee826864..c3a724f94 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, scan_string_end}; +use super::scan::{char_label, escape_literal_control_chars, scan_string_end}; /// Byte-slice cursor for the DSL input. pub(crate) struct Parser<'a> { @@ -386,11 +386,23 @@ impl<'a> Parser<'a> { let end = self.scan_value_end()?; let slice = std::str::from_utf8(&self.src[start..end]) .expect("ascii-or-utf8 maintained by scanner"); - let value: Value = - serde_json::from_str(slice.trim()).map_err(|e| DslError::InvalidValue { - pos: start, - error: e.to_string(), - })?; + 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 + } else { + trimmed + }; + 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) } diff --git a/crates/khive-request/src/parser/scan.rs b/crates/khive-request/src/parser/scan.rs index a58692c1b..7415ad3f8 100644 --- a/crates/khive-request/src/parser/scan.rs +++ b/crates/khive-request/src/parser/scan.rs @@ -20,6 +20,42 @@ pub(crate) fn scan_string_end(src: &[u8], start: usize) -> Result String { + if !s.contains(['\n', '\r', '\t']) { + return s.to_owned(); + } + let mut out = String::with_capacity(s.len() + 8); + let mut chars = s.chars(); + while let Some(c) = chars.next() { + if c == '\\' { + out.push(c); + if let Some(next) = chars.next() { + out.push(next); + } + continue; + } + match c { + '\n' => out.push_str("\\n"), + '\r' => out.push_str("\\r"), + '\t' => out.push_str("\\t"), + c => out.push(c), + } + } + out +} + /// Returns a stable delimiter label for diagnostics. pub(crate) fn char_label(c: char) -> &'static str { match c { diff --git a/crates/khive-request/tests/parser.rs b/crates/khive-request/tests/parser.rs index 7c26a43a0..0c3971af5 100644 --- a/crates/khive-request/tests/parser.rs +++ b/crates/khive-request/tests/parser.rs @@ -109,6 +109,151 @@ fn string_with_escaped_quote() { assert_eq!(val(&v[0].args["title"]), &json!("he said \"hi\"")); } +#[test] +fn multiline_string_value_round_trips() { + // A literal raw newline (0x0A) inside a double-quoted value, as an agent + // client would send it directly rather than escaping it to `\n` text. + let src = "gtd.assign(title=\"line one\nline two\")"; + let v = ops(src); + assert_eq!(val(&v[0].args["title"]), &json!("line one\nline two")); +} + +#[test] +fn tab_and_carriage_return_literal_in_string() { + let src = "gtd.assign(title=\"a\tb\rc\")"; + let v = ops(src); + assert_eq!(val(&v[0].args["title"]), &json!("a\tb\rc")); +} + +#[test] +fn mixed_literal_newline_and_escaped_sequences() { + // A literal raw newline sits next to escaped-quote and escaped-backslash + // pairs in the same value, proving the control-char rewrite does not + // disturb adjacent backslash-escape pairs. + let src = "comm.reply(content=\"line one \\\"quoted\\\"\nline two a\\\\b\")"; + let v = ops(src); + assert_eq!( + val(&v[0].args["content"]), + &json!("line one \"quoted\"\nline two a\\b") + ); +} + +#[test] +fn unterminated_multiline_string_rejected() { + // A raw newline mid-string does not make an unclosed string legal; the + // closing quote is still required somewhere in the input. + let src = "gtd.assign(title=\"line one\nline two)"; + let err = parse_request(src).unwrap_err(); + assert!(matches!(err, DslError::UnclosedString)); +} + +#[test] +fn json_form_rejects_literal_newline_in_string() { + // JSON form stays strict JSON: a raw control character inside a quoted + // string is a parse error there, unlike the function-call form above. + let src = "[{\"tool\":\"gtd.next\",\"args\":{\"note\":\"line one\nline two\"}}]"; + let err = parse_request(src).unwrap_err(); + assert!(matches!(err, DslError::InvalidJson { .. })); +} + +// ── ADR-016 grammar-boundary matrix (round-1 review, PR #957) ──────────────── + +#[test] +fn crlf_preserved_exact_in_quoted_value() { + // A literal CRLF pair (not just LF or CR alone) inside a quoted value + // round-trips byte-for-byte, proving both bytes are rewritten + // independently rather than the pair being collapsed or reordered. + let src = "gtd.assign(title=\"line one\r\nline two\")"; + let v = ops(src); + assert_eq!(val(&v[0].args["title"]), &json!("line one\r\nline two")); +} + +#[test] +fn literal_newline_immediately_before_closing_quote() { + // The raw newline is the very last byte before the closing `"`, with no + // trailing content — the scanner must not mistake it for trailing + // whitespace to trim, nor the rewrite skip the final byte. + let src = "gtd.assign(title=\"abc\n\")"; + let v = ops(src); + assert_eq!(val(&v[0].args["title"]), &json!("abc\n")); +} + +#[test] +fn raw_newline_immediately_after_escaped_quote_boundary() { + // A `\"` escape pair is immediately followed by a raw literal newline. + // `scan_string_end` must consume the escape pair as a unit (not treat + // its second byte as the closing quote), and the rewrite must copy the + // escape pair through untouched before rewriting the adjacent raw LF. + let src = "gtd.assign(title=\"a\\\"\nb\")"; + let v = ops(src); + assert_eq!(val(&v[0].args["title"]), &json!("a\"\nb")); +} + +#[test] +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 + // `"`). 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}'); + let err = parse_request(&src).unwrap_err(); + assert!( + matches!(err, DslError::InvalidValue { .. }), + "expected InvalidValue for a raw control byte in bareword position, got {err:?}" + ); +} + +#[test] +fn other_raw_control_chars_in_quoted_string_still_rejected() { + // Only `\n`, `\r`, and `\t` are exempted (ADR-016). Every other raw + // U+0000-U+001F byte inside a quoted value must remain a parse error, + // matching pre-PR behavior — NUL, form feed, and ESC here. + for raw in ['\u{0}', '\u{c}', '\u{1b}'] { + let src = format!("gtd.assign(title=\"a{raw}b\")"); + let err = parse_request(&src).unwrap_err(); + assert!( + matches!(err, DslError::InvalidValue { .. }), + "expected InvalidValue for raw control byte {:#04x}, 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. + // This must fail regardless of the literal-newline carve-out. + let src = r#"gtd.assign(title="bad \q escape")"#; + let err = parse_request(src).unwrap_err(); + assert!(matches!(err, DslError::InvalidValue { .. })); +} + +#[test] +fn batch_ops_separated_by_raw_newlines() { + // The top-level batch `,` separator may be split across literal + // newlines between ops, since `skip_ws` treats `\n` as ordinary + // whitespace around the separator. + let src = "[create(kind=\"entity\", name=\"A\"),\ncreate(kind=\"entity\", name=\"B\")]"; + let r = req(src); + assert_eq!(r.mode, ExecutionMode::Parallel); + assert_eq!(r.ops.len(), 2); + assert_eq!(val(&r.ops[0].args["name"]), &json!("A")); + assert_eq!(val(&r.ops[1].args["name"]), &json!("B")); +} + +#[test] +fn chain_ops_separated_by_raw_newlines() { + // The chain `|` separator may likewise be split across literal + // newlines between ops. + let src = "create(kind=\"entity\", name=\"A\")\n|\ncreate(kind=\"entity\", name=\"B\")"; + let r = req(src); + assert_eq!(r.mode, ExecutionMode::Chain); + assert_eq!(r.ops.len(), 2); + assert_eq!(val(&r.ops[0].args["name"]), &json!("A")); + assert_eq!(val(&r.ops[1].args["name"]), &json!("B")); +} + #[test] fn null_and_negative_number() { let v = ops(r#"update(id="x", description=null, weight=-0.5)"#); diff --git a/docs/adr/ADR-016-request-dsl.md b/docs/adr/ADR-016-request-dsl.md index 4811c583e..4be32cc06 100644 --- a/docs/adr/ADR-016-request-dsl.md +++ b/docs/adr/ADR-016-request-dsl.md @@ -105,7 +105,16 @@ Argument values use JSON literal syntax inside the function-call form: | Reference (chain only) | `$prev`, `$prev.field.path`, `$prev[N].field` | String escapes follow JSON: `\\`, `\"`, `\n`, `\t`, `\r`. Other backslash -sequences are literal. +sequences are literal. Unlike strict JSON, a double-quoted value in the +function-call form may also contain a literal newline, carriage return, or +tab byte verbatim (not just the backslash-escaped form); the parser rewrites +those bytes to their JSON escape before decoding, so multi-paragraph content +round-trips without requiring callers to pre-escape line breaks. This +exception is limited to exactly those three bytes: every other raw +U+0000-U+001F control character inside a function-call quoted value remains +a parse error, same as strict JSON. The JSON form (`ops="[{\"tool\":...}]"`) +keeps strict JSON string rules: raw control characters there remain a parse +error, per the JSON spec. #### `$prev` reference grammar