From e27c388ddd3fe71a2befc5ae795c28aaf56b85e5 Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Tue, 21 Apr 2026 19:04:43 +0200 Subject: [PATCH] fix(mcp): scope `set_fields` to domain fields, add top-level `description` setter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related MCP correctness bugs in `rivet_modify`: 1. `set_fields` silently wrote reserved top-level keys (description, title, status, ...) under the `fields:` sub-map — corrupting the artifact's shape. Values containing backticks or newlines additionally broke the YAML emitter, which used unquoted `format!("{key}: {value}")` lines. 2. There was no way to set the top-level `description` (or other top-level metadata) via MCP — `set_fields` was the only "generic setter" and, by design, targets only the domain `fields:` map. Design: keep `set_fields` scoped to the `fields:` sub-map and expose dedicated setters for top-level metadata. `validate_modify` now rejects any `set_fields` key listed in `RESERVED_TOP_LEVEL_KEYS` (id, type, title, description, status, tags, links, fields, provenance, source-file) with a hint pointing at the right parameter. A new `description` parameter on `rivet_modify` routes through `ModifyParams::set_description`, which emits YAML-safe scalars — multi-line values become block-literal (`|-`) scalars, single-line values with YAML-significant characters are double-quoted with proper escapes. `set_field` in the editor was extended to splice multi-line values into the line buffer so block scalars stay well-formed. Tests (added failing first, now green): - `test_set_fields_rejects_reserved_description` / `_all_reserved_top_level_keys` - `test_modify_sets_top_level_description` - `test_modify_description_with_backticks_and_newlines` - `test_validate_modify_rejects_reserved_top_level_in_set_fields` - `test_yaml_quote_inline_scalar_*`, `test_set_field_writes_*_as_*_scalar` Fixes: REQ-002 --- rivet-cli/src/main.rs | 1 + rivet-cli/src/mcp.rs | 24 ++- rivet-cli/tests/mcp_integration.rs | 212 ++++++++++++++++++ rivet-core/src/mutate.rs | 57 +++++ rivet-core/src/yaml_edit.rs | 287 ++++++++++++++++++++++++- rivet-core/tests/mutate_integration.rs | 37 ++++ 6 files changed, 605 insertions(+), 13 deletions(-) diff --git a/rivet-cli/src/main.rs b/rivet-cli/src/main.rs index 14e6ffc..8344706 100644 --- a/rivet-cli/src/main.rs +++ b/rivet-cli/src/main.rs @@ -7653,6 +7653,7 @@ fn cmd_modify( let params = ModifyParams { set_status: set_status.map(|s| s.to_string()), set_title: set_title.map(|s| s.to_string()), + set_description: None, add_tags: add_tags.to_vec(), remove_tags: remove_tags.to_vec(), set_fields: set_fields.to_vec(), diff --git a/rivet-cli/src/mcp.rs b/rivet-cli/src/mcp.rs index e183e37..4a30785 100644 --- a/rivet-cli/src/mcp.rs +++ b/rivet-cli/src/mcp.rs @@ -123,15 +123,25 @@ pub struct LinkParam { pub struct ModifyParams { #[schemars(description = "Artifact ID to modify")] pub id: String, - #[schemars(description = "New status value")] + #[schemars(description = "New top-level status value")] pub status: Option, - #[schemars(description = "New title")] + #[schemars(description = "New top-level title")] pub title: Option, + #[schemars( + description = "New top-level description (YAML-safe quoting is applied; multi-line \ + values become block-literal scalars)" + )] + pub description: Option, #[schemars(description = "Tags to add")] pub add_tags: Option>, #[schemars(description = "Tags to remove")] pub remove_tags: Option>, - #[schemars(description = "Fields to set as key=value pairs")] + #[schemars( + description = "Domain `fields:` sub-map entries as key=value pairs. Reserved top-level \ + keys (id, type, title, description, status, tags, links, fields, \ + provenance, source-file) are rejected — use the dedicated parameters \ + for those (e.g. `description`, `status`, `title`)." + )] pub set_fields: Option>, } @@ -1025,6 +1035,7 @@ fn tool_modify(project_dir: &Path, p: &ModifyParams) -> Result { let params = mutate::ModifyParams { set_status: p.status.clone(), set_title: p.title.clone(), + set_description: p.description.clone(), add_tags: p.add_tags.clone().unwrap_or_default(), remove_tags: p.remove_tags.clone().unwrap_or_default(), set_fields, @@ -1041,7 +1052,12 @@ fn tool_modify(project_dir: &Path, p: &ModifyParams) -> Result { mcp_audit_log( project_dir, "rivet_modify", - &json!({"id": p.id, "status": p.status, "title": p.title}), + &json!({ + "id": p.id, + "status": p.status, + "title": p.title, + "description": p.description.is_some(), + }), ); Ok(result) } diff --git a/rivet-cli/tests/mcp_integration.rs b/rivet-cli/tests/mcp_integration.rs index 87b4f33..73e6e5b 100644 --- a/rivet-cli/tests/mcp_integration.rs +++ b/rivet-cli/tests/mcp_integration.rs @@ -879,3 +879,215 @@ async fn test_remove_artifact() { client.cancel().await.expect("cancel"); } + +// ── Bug: set_fields scope — reserved top-level keys (Fixes: REQ-002) ──── +// +// `set_fields` targets the `fields:` sub-map on an artifact. It must refuse +// to write keys that collide with reserved top-level keys (id, type, title, +// description, status, tags, links, fields, provenance, source-file), +// because otherwise it either silently nests them under `fields:` (breaking +// the artifact's shape) or, worse, emits unquoted scalars that break YAML +// parsing when the value contains backticks or newlines. + +#[tokio::test] +async fn test_set_fields_rejects_reserved_description() { + let tmp = tempfile::tempdir().unwrap(); + create_test_project(tmp.path()); + + let client = spawn_mcp_client(tmp.path()).await; + + // Attempt to smuggle a top-level `description` via `set_fields`. + let mut args = serde_json::Map::new(); + args.insert("id".to_string(), Value::String("REQ-001".to_string())); + args.insert( + "set_fields".to_string(), + Value::Array(vec![Value::String( + "description=Top-level description via set_fields".to_string(), + )]), + ); + + let result = client + .call_tool(CallToolRequestParams::new("rivet_modify").with_arguments(args)) + .await; + + assert!( + result.is_err(), + "set_fields must refuse reserved key 'description'; got: {result:?}" + ); + let err = format!("{:?}", result.unwrap_err()); + assert!( + err.contains("description") && (err.contains("reserved") || err.contains("top-level")), + "error should mention reserved/top-level description; got: {err}" + ); + + // The file must not have been touched with a nested `description:` under `fields:`. + let content = + std::fs::read_to_string(tmp.path().join("artifacts").join("requirements.yaml")).unwrap(); + assert!( + !content.contains("Top-level description via set_fields"), + "set_fields should not have written the value; file:\n{content}" + ); + + client.cancel().await.expect("cancel"); +} + +#[tokio::test] +async fn test_set_fields_rejects_all_reserved_top_level_keys() { + let tmp = tempfile::tempdir().unwrap(); + create_test_project(tmp.path()); + + let client = spawn_mcp_client(tmp.path()).await; + + for reserved in &[ + "id", + "type", + "title", + "description", + "status", + "tags", + "links", + "fields", + "provenance", + "source-file", + ] { + let mut args = serde_json::Map::new(); + args.insert("id".to_string(), Value::String("REQ-001".to_string())); + args.insert( + "set_fields".to_string(), + Value::Array(vec![Value::String(format!("{reserved}=x"))]), + ); + + let result = client + .call_tool(CallToolRequestParams::new("rivet_modify").with_arguments(args)) + .await; + + assert!( + result.is_err(), + "set_fields must refuse reserved key '{reserved}'; got: {result:?}" + ); + } + + client.cancel().await.expect("cancel"); +} + +// ── Bug 2: set_metadata tool for top-level fields (Fixes: REQ-002) ───── +// +// Exposes `description` on `rivet_modify` so that top-level metadata is +// reachable from MCP. Handles YAML-safe scalar quoting for values containing +// backticks, newlines, or other special characters. + +#[tokio::test] +async fn test_modify_sets_top_level_description() { + let tmp = tempfile::tempdir().unwrap(); + create_test_project(tmp.path()); + + let client = spawn_mcp_client(tmp.path()).await; + + // Set a plain top-level description. + let mut args = serde_json::Map::new(); + args.insert("id".to_string(), Value::String("REQ-001".to_string())); + args.insert( + "description".to_string(), + Value::String("A plain description".to_string()), + ); + + let result = client + .call_tool(CallToolRequestParams::new("rivet_modify").with_arguments(args)) + .await + .expect("call_tool rivet_modify with description"); + + let json = parse_result(&result); + assert_eq!(json["modified"].as_str(), Some("REQ-001")); + + // Reload and verify via rivet_get. + client + .call_tool(CallToolRequestParams::new("rivet_reload")) + .await + .expect("reload"); + + let mut args = serde_json::Map::new(); + args.insert("id".to_string(), Value::String("REQ-001".to_string())); + let result = client + .call_tool(CallToolRequestParams::new("rivet_get").with_arguments(args)) + .await + .expect("rivet_get after modify"); + let json = parse_result(&result); + assert_eq!( + json["description"].as_str(), + Some("A plain description"), + "top-level description should round-trip; got: {json}" + ); + + // And it must not have been nested under `fields:` (regression check). + let content = + std::fs::read_to_string(tmp.path().join("artifacts").join("requirements.yaml")).unwrap(); + // The file should still validate: run rivet_validate. + let result = client + .call_tool(CallToolRequestParams::new("rivet_validate")) + .await + .expect("rivet_validate"); + let json = parse_result(&result); + assert_eq!( + json["result"].as_str(), + Some("PASS"), + "validation should still pass after setting top-level description; file:\n{content}\n\ + diagnostics: {json}" + ); + + client.cancel().await.expect("cancel"); +} + +#[tokio::test] +async fn test_modify_description_with_backticks_and_newlines() { + let tmp = tempfile::tempdir().unwrap(); + create_test_project(tmp.path()); + + let client = spawn_mcp_client(tmp.path()).await; + + // Value contains backticks, newlines, and a trailing colon — the old + // unquoted `format!("description: {value}")` path would blow up parsing. + let tricky = "Use `rivet validate` to check.\nSecond line: ok?\n\nWith blank line."; + + let mut args = serde_json::Map::new(); + args.insert("id".to_string(), Value::String("REQ-001".to_string())); + args.insert("description".to_string(), Value::String(tricky.to_string())); + + let result = client + .call_tool(CallToolRequestParams::new("rivet_modify").with_arguments(args)) + .await + .expect("rivet_modify with tricky description"); + let json = parse_result(&result); + assert_eq!(json["modified"].as_str(), Some("REQ-001")); + + // The resulting file must still parse as YAML. + let content = + std::fs::read_to_string(tmp.path().join("artifacts").join("requirements.yaml")).unwrap(); + let _parsed: serde_yaml::Value = serde_yaml::from_str(&content).unwrap_or_else(|e| { + panic!("file should parse as YAML after tricky description: {e}\n{content}") + }); + + // Reload and check round-trip. + client + .call_tool(CallToolRequestParams::new("rivet_reload")) + .await + .expect("reload"); + + let mut args = serde_json::Map::new(); + args.insert("id".to_string(), Value::String("REQ-001".to_string())); + let result = client + .call_tool(CallToolRequestParams::new("rivet_get").with_arguments(args)) + .await + .expect("rivet_get after tricky modify"); + let json = parse_result(&result); + let got = json["description"].as_str().unwrap_or(""); + assert!( + got.contains("`rivet validate`"), + "description should preserve backticks; got: {got:?}" + ); + assert!( + got.contains("Second line"), + "description should preserve newlines; got: {got:?}" + ); + + client.cancel().await.expect("cancel"); +} diff --git a/rivet-core/src/mutate.rs b/rivet-core/src/mutate.rs index b705528..eb1fbac 100644 --- a/rivet-core/src/mutate.rs +++ b/rivet-core/src/mutate.rs @@ -243,13 +243,52 @@ pub fn validate_unlink( Ok(()) } +/// Reserved top-level artifact keys that must never be written via `set_fields`. +/// +/// These live at the top level of an artifact mapping (not under `fields:`) +/// and each has a dedicated path: base struct members (`id`, `type`, `title`, +/// `description`, `status`, `tags`, `links`), the domain `fields:` sub-map +/// itself, `provenance:` metadata, and the `source-file:` loader hint. +/// +/// If a caller passes one of these through `set_fields`, `validate_modify` +/// returns an error rather than silently nesting it under `fields:` (which +/// would corrupt the artifact shape) or emitting an unquoted top-level scalar +/// whose content (backticks, newlines) could break YAML parsing. +pub const RESERVED_TOP_LEVEL_KEYS: &[&str] = &[ + "id", + "type", + "title", + "description", + "status", + "tags", + "links", + "fields", + "provenance", + "source-file", +]; + /// Parameters for a modify operation. +/// +/// `set_fields` targets the **domain `fields:` sub-map only**. Top-level +/// metadata (title, status, description, ...) has dedicated setters to +/// avoid ambiguity and to enable value-type-specific YAML emission. #[derive(Debug, Default)] pub struct ModifyParams { pub set_status: Option, pub set_title: Option, + /// Set the top-level `description:` of the artifact. + /// + /// Values are emitted with YAML-safe quoting — newlines trigger a + /// block-literal scalar, special characters trigger a double-quoted + /// scalar. See [`crate::yaml_edit`] for the emitter. + pub set_description: Option, pub add_tags: Vec, pub remove_tags: Vec, + /// Domain-specific fields under the artifact's `fields:` map. + /// + /// Keys must not collide with [`RESERVED_TOP_LEVEL_KEYS`]; use + /// `set_title`, `set_status`, `set_description`, `add_tags`, etc. for + /// those instead. Validated by [`validate_modify`]. pub set_fields: Vec<(String, String)>, } @@ -273,6 +312,24 @@ pub fn validate_modify( )) })?; + // Reject attempts to write reserved top-level keys through `set_fields`. + // See [`RESERVED_TOP_LEVEL_KEYS`] for the rationale (scope + YAML safety). + for (key, _) in ¶ms.set_fields { + if RESERVED_TOP_LEVEL_KEYS.contains(&key.as_str()) { + let hint = match key.as_str() { + "title" => " — use the `title` / `set_title` parameter", + "status" => " — use the `status` / `set_status` parameter", + "description" => " — use the `description` / `set_description` parameter", + "tags" => " — use the `add_tags` / `remove_tags` parameters", + _ => "", + }; + return Err(Error::Validation(format!( + "'{key}' is a reserved top-level artifact key and cannot be set via \ + `set_fields` (which targets the `fields:` sub-map){hint}" + ))); + } + } + // Validate field allowed values for (key, value) in ¶ms.set_fields { if let Some(field) = type_def.fields.iter().find(|f| f.name == *key) { diff --git a/rivet-core/src/yaml_edit.rs b/rivet-core/src/yaml_edit.rs index 68a9bde..85f95d3 100644 --- a/rivet-core/src/yaml_edit.rs +++ b/rivet-core/src/yaml_edit.rs @@ -165,6 +165,10 @@ impl YamlEditor { /// If the field already exists, its value is replaced (including any /// block-scalar continuation lines). If it does not exist, a new line /// is inserted at the correct indentation. + /// + /// `value` may contain embedded `\n` characters — each becomes its own + /// line. This supports rendering of YAML block-literal scalars where + /// the caller pre-indents continuation lines (see `yaml_render_scalar_value`). pub fn set_field(&mut self, id: &str, key: &str, value: &str) -> Result<(), String> { let (block_start, block_end) = self .find_artifact_block(id) @@ -173,20 +177,26 @@ impl YamlEditor { let field_indent = self.field_indent(block_start); let indent_str = " ".repeat(field_indent); + // Render the field as one or more lines. The first line is + // `: `; subsequent value lines (if the + // value contains '\n') are kept verbatim — callers are responsible + // for their indentation (block-literal continuation lines must be + // indented deeper than the key). + let rendered = format!("{indent_str}{key}: {value}"); + let new_lines: Vec = rendered.split('\n').map(str::to_string).collect(); + if let Some(field_line) = self.find_field_in_block(block_start, block_end, key) { - // Replace existing field (and any block-scalar continuation) + // Replace existing field (and any block-scalar continuation). let scalar_end = self.block_scalar_end(field_line, block_end); - let new_line = format!("{indent_str}{key}: {value}"); - // Replace the range [field_line, scalar_end) with the single new line - self.lines - .splice(field_line..scalar_end, std::iter::once(new_line)); + self.lines.splice(field_line..scalar_end, new_lines); } else { // Insert new field. Place it after the last simple field before // any `links:`, `fields:`, or `tags:` section — or at the end // of the block. let insert_at = self.find_insert_position(block_start, block_end, key); - let new_line = format!("{indent_str}{key}: {value}"); - self.lines.insert(insert_at, new_line); + for (i, line) in new_lines.into_iter().enumerate() { + self.lines.insert(insert_at + i, line); + } } Ok(()) @@ -517,6 +527,147 @@ use std::path::Path; use super::mutate::ModifyParams; +/// Return `true` if a YAML scalar requires explicit quoting to be parsed back +/// as a plain string. +/// +/// This errs on the safe side: any of the YAML "plain scalar" pitfalls +/// (leading indicators, `#`, `:`, `-`, `?`, quotes, commas, brackets, braces, +/// `&`, `*`, `!`, `|`, `>`, `@`, `` ` ``, `%`), values that look like YAML +/// 1.1 reserved words (`true`/`false`/`yes`/`no`/`null`/`~`), leading/trailing +/// whitespace, or things that parse as numbers — all force quoting. +fn yaml_plain_scalar_needs_quoting(s: &str) -> bool { + if s.is_empty() { + return true; + } + // Any control char / newline / tab → quote (caller of inline variant + // should have picked block scalar; defensive). + if s.chars().any(|c| c.is_control()) { + return true; + } + // Leading whitespace is ambiguous. + if s.starts_with(' ') || s.ends_with(' ') { + return true; + } + // YAML indicator chars at the start. + let first = s.chars().next().unwrap(); + if matches!( + first, + '#' | '&' + | '*' + | '!' + | '|' + | '>' + | '\'' + | '"' + | '%' + | '@' + | '`' + | ',' + | '[' + | ']' + | '{' + | '}' + | '?' + | ':' + | '-' + ) { + return true; + } + // `: ` or ` #` anywhere breaks plain scalars. + if s.contains(": ") || s.contains(" #") { + return true; + } + // Trailing colon → mapping key confusion. + if s.ends_with(':') { + return true; + } + // YAML 1.1 boolean / null literals (case-insensitive). + let lower = s.to_ascii_lowercase(); + if matches!( + lower.as_str(), + "true" | "false" | "yes" | "no" | "on" | "off" | "null" | "~" + ) { + return true; + } + // Pure number → would parse as number, not string. + if s.parse::().is_ok() { + return true; + } + false +} + +/// Emit a YAML scalar suitable for use inline on the same line as a key +/// (e.g. `title: `). Never produces a block scalar; multi-line input +/// is double-quoted with `\n` escapes. Callers that want block scalars for +/// multi-line text should use [`yaml_render_scalar_value`] instead. +fn yaml_quote_inline_scalar(s: &str) -> String { + if yaml_plain_scalar_needs_quoting(s) || s.contains('\n') { + yaml_double_quote(s) + } else { + s.to_string() + } +} + +/// Double-quote a YAML scalar, escaping backslash/double-quote/control chars +/// using the YAML double-quoted style. +fn yaml_double_quote(s: &str) -> String { + let mut out = String::with_capacity(s.len() + 2); + out.push('"'); + for c in s.chars() { + match c { + '\\' => out.push_str("\\\\"), + '"' => out.push_str("\\\""), + '\n' => out.push_str("\\n"), + '\r' => out.push_str("\\r"), + '\t' => out.push_str("\\t"), + '\0' => out.push_str("\\0"), + c if (c as u32) < 0x20 => { + out.push_str(&format!("\\x{:02X}", c as u32)); + } + c => out.push(c), + } + } + out.push('"'); + out +} + +/// Render a YAML scalar value for use as the right-hand side of a key at +/// `field_indent`. Multi-line values use a block-literal scalar (`|`) with +/// continuation lines indented by `field_indent + 2`; single-line values +/// fall through to [`yaml_quote_inline_scalar`]. +/// +/// The returned string begins with the value — for multi-line it starts with +/// `|\n`; for single-line it is the value or its quoted +/// form. The caller is expected to concatenate `"{key}: "` + result. +fn yaml_render_scalar_value(s: &str, field_indent: usize) -> String { + if !s.contains('\n') { + return yaml_quote_inline_scalar(s); + } + // Block-literal scalar preserves newlines exactly. + let content_indent = " ".repeat(field_indent + 2); + // Handle trailing newline: YAML's default chomp keeps a single trailing + // newline; use `|-` if there is no trailing newline so round-tripping is + // exact. + let chomped = if s.ends_with('\n') { "|" } else { "|-" }; + let mut out = String::from(chomped); + for line in s.split('\n') { + out.push('\n'); + if line.is_empty() { + // Blank lines must still appear; a literal empty line is fine + // (it's just a newline at content_indent depth — YAML accepts a + // truly empty line between block-scalar content lines). + } else { + out.push_str(&content_indent); + out.push_str(line); + } + } + // Strip the trailing empty-line artifact if the source ended in `\n`: + // `split('\n')` on "a\n" yields ["a", ""], which our loop emits as + // "\n{indent}a\n" — the trailing blank line is intentional because the + // block-literal's default-chomp behaviour keeps exactly one newline. + out +} + /// Modify an artifact in its YAML file using the safe editor. pub fn modify_artifact_in_file( id: &str, @@ -553,15 +704,31 @@ pub fn modify_artifact_yaml( // Set title if let Some(ref new_title) = params.set_title { + let quoted = yaml_quote_inline_scalar(new_title); editor - .set_field(id, "title", new_title) + .set_field(id, "title", "ed) .map_err(Error::Validation)?; } // Set status if let Some(ref new_status) = params.set_status { + let quoted = yaml_quote_inline_scalar(new_status); editor - .set_field(id, "status", new_status) + .set_field(id, "status", "ed) + .map_err(Error::Validation)?; + } + + // Set description (top-level). Multi-line values are emitted as a YAML + // block-literal scalar; single-line values that contain YAML-significant + // characters are double-quoted. + if let Some(ref new_desc) = params.set_description { + let (block_start, _) = editor + .find_artifact_block(id) + .ok_or_else(|| Error::Validation(format!("artifact '{id}' not found")))?; + let field_indent = editor.field_indent(block_start); + let rendered = yaml_render_scalar_value(new_desc, field_indent); + editor + .set_field(id, "description", &rendered) .map_err(Error::Validation)?; } @@ -1207,4 +1374,106 @@ artifacts: assert!(result.is_err()); assert!(result.unwrap_err().contains("not found")); } + + // ── YAML-safe scalar quoting (Fixes: REQ-002) ───────────────────── + + // rivet: verifies REQ-002 + #[test] + fn test_yaml_quote_inline_scalar_plain() { + // Safe plain scalars remain unquoted. + assert_eq!(yaml_quote_inline_scalar("hello world"), "hello world"); + assert_eq!(yaml_quote_inline_scalar("Foo"), "Foo"); + assert_eq!(yaml_quote_inline_scalar("abc123"), "abc123"); + } + + // rivet: verifies REQ-002 + #[test] + fn test_yaml_quote_inline_scalar_needs_quoting() { + // Anything with YAML-significant characters gets double-quoted. + assert_eq!(yaml_quote_inline_scalar("- foo"), "\"- foo\""); + assert_eq!(yaml_quote_inline_scalar("yes"), "\"yes\""); + assert_eq!(yaml_quote_inline_scalar("42"), "\"42\""); + assert_eq!(yaml_quote_inline_scalar(""), "\"\""); + // Backticks at the start trigger quoting; interior backticks are fine. + assert_eq!(yaml_quote_inline_scalar("`x"), "\"`x\""); + assert_eq!( + yaml_quote_inline_scalar("uses `x` inside"), + "uses `x` inside" + ); + // `: ` anywhere is ambiguous. + assert_eq!(yaml_quote_inline_scalar("key: value"), "\"key: value\""); + } + + // rivet: verifies REQ-002 + #[test] + fn test_yaml_render_scalar_value_multiline_block_literal() { + let input = "Line one\nLine two\nLine three"; + let out = yaml_render_scalar_value(input, 4); + // Starts with |- (no trailing newline in input) + assert!( + out.starts_with("|-\n"), + "expected block literal, got: {out:?}" + ); + assert!(out.contains(" Line one")); + assert!(out.contains(" Line two")); + assert!(out.contains(" Line three")); + } + + // rivet: verifies REQ-002 + #[test] + fn test_set_field_writes_multiline_description_as_block_scalar() { + let content = "\ +artifacts: + - id: REQ-001 + type: requirement + title: First + description: old one-liner + status: draft"; + + let mut editor = YamlEditor::parse(content); + let rendered = yaml_render_scalar_value("First line\nSecond line `with backticks`", 4); + editor + .set_field("REQ-001", "description", &rendered) + .unwrap(); + let output = editor.to_string(); + + // Must parse back as YAML — no unquoted-scalar corruption. + let parsed: serde_yaml::Value = serde_yaml::from_str(&output) + .unwrap_or_else(|e| panic!("output must parse as YAML: {e}\n---\n{output}")); + let desc = &parsed["artifacts"][0]["description"]; + let s = desc.as_str().expect("description should be a string"); + assert!(s.contains("First line")); + assert!(s.contains("Second line")); + assert!(s.contains("`with backticks`")); + + // The old one-liner is gone. + assert!(!output.contains("old one-liner")); + } + + // rivet: verifies REQ-002 + #[test] + fn test_set_field_writes_description_with_backticks_as_quoted_scalar() { + // Single-line value with backticks at start → double-quoted. + let content = "\ +artifacts: + - id: REQ-001 + type: requirement + title: First + status: draft"; + + let mut editor = YamlEditor::parse(content); + let rendered = yaml_render_scalar_value("`rivet validate` is handy", 4); + editor + .set_field("REQ-001", "description", &rendered) + .unwrap(); + let output = editor.to_string(); + + // Must parse back cleanly. + let parsed: serde_yaml::Value = serde_yaml::from_str(&output) + .unwrap_or_else(|e| panic!("output must parse as YAML: {e}\n---\n{output}")); + assert_eq!( + parsed["artifacts"][0]["description"].as_str(), + Some("`rivet validate` is handy") + ); + } } diff --git a/rivet-core/tests/mutate_integration.rs b/rivet-core/tests/mutate_integration.rs index 52240e7..963e0f6 100644 --- a/rivet-core/tests/mutate_integration.rs +++ b/rivet-core/tests/mutate_integration.rs @@ -501,6 +501,7 @@ fn test_validate_modify_rejects_invalid_field() { let params = mutate::ModifyParams { set_status: None, set_title: None, + set_description: None, add_tags: vec![], remove_tags: vec![], set_fields: vec![("priority".to_string(), "critical".to_string())], @@ -515,6 +516,42 @@ fn test_validate_modify_rejects_invalid_field() { ); } +// ── Test: set_fields rejects reserved top-level keys (Fixes: REQ-002) ──── + +// rivet: verifies REQ-002 +#[test] +fn test_validate_modify_rejects_reserved_top_level_in_set_fields() { + let schema = load_schema_files(&["common", "dev"]); + let mut store = Store::new(); + + store + .insert(make_artifact( + "REQ-001", + "requirement", + "First", + vec![], + BTreeMap::new(), + )) + .unwrap(); + + for reserved in mutate::RESERVED_TOP_LEVEL_KEYS { + let params = mutate::ModifyParams { + set_fields: vec![((*reserved).to_string(), "x".to_string())], + ..Default::default() + }; + let result = mutate::validate_modify("REQ-001", ¶ms, &store, &schema); + assert!( + result.is_err(), + "set_fields must reject reserved key '{reserved}'" + ); + let err = result.unwrap_err().to_string(); + assert!( + err.contains(reserved) && err.contains("reserved"), + "error for '{reserved}' should mention 'reserved', got: {err}" + ); + } +} + // ── Test: validate_unlink rejects missing link ─────────────────────────── // rivet: verifies REQ-031