fix(mcp): set_fields rejects reserved keys; add top-level description

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(),

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<String>,
-    #[schemars(description = "New title")]
+    #[schemars(description = "New top-level title")]
     pub title: Option<String>,
+    #[schemars(
+        description = "New top-level description (YAML-safe quoting is applied; multi-line \
+                       values become block-literal scalars)"
+    )]
+    pub description: Option<String>,
     #[schemars(description = "Tags to add")]
     pub add_tags: Option<Vec<String>>,
     #[schemars(description = "Tags to remove")]
     pub remove_tags: Option<Vec<String>>,
-    #[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<Vec<String>>,
 }
 
@@ -1025,6 +1035,7 @@ fn tool_modify(project_dir: &Path, p: &ModifyParams) -> Result<Value> {
     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<Value> {
     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)
 }

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

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<String>,
     pub set_title: Option<String>,
+    /// 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<String>,
     pub add_tags: Vec<String>,
     pub remove_tags: Vec<String>,
+    /// 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 &params.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 &params.set_fields {
         if let Some(field) = type_def.fields.iter().find(|f| f.name == *key) {