fix(mcp): resolve sequence_ids in comment and honour parent_id on item update

crates/agentflare-backend/src/item/crud.rs

@@ -163,8 +163,59 @@ pub fn list_by_assignee_agent(
     Ok(rows.collect::<std::result::Result<_, _>>()?)
 }
 
+/// Reads an item's `parent_id` without the `deleted_at IS NULL` filter `get`
+/// applies, and without erroring on a missing row — an ancestor walk must be
+/// able to cross a soft-deleted ancestor and simply stop when the chain runs
+/// out.
+fn parent_of(conn: &Connection, id: &str) -> Result<Option<String>> {
+    use rusqlite::OptionalExtension;
+    Ok(conn
+        .query_row(
+            "SELECT parent_id FROM items WHERE id = ?1",
+            rusqlite::params![id],
+            |row| row.get::<_, Option<String>>(0),
+        )
+        .optional()?
+        .flatten())
+}
+
+/// Rejects a re-parent that would make `id` its own ancestor. Only `update`
+/// can create a cycle — `create` writes a parent onto a brand-new id that
+/// nothing can point at yet — and a cycle would hang every ancestor walk in
+/// the tree (`quota::goal::find_goal_ancestor`, the dashboard hierarchy, ...).
+/// Also proves the parent exists, so an unknown id fails as `NotFound` rather
+/// than a raw "FOREIGN KEY constraint failed".
+fn validate_parent(conn: &Connection, id: &str, parent_id: &str) -> Result<()> {
+    if parent_id == id {
+        return Err(crate::error::Error::Validation(format!(
+            "item {id} cannot be its own parent"
+        )));
+    }
+    get(conn, parent_id)?;
+    let mut cursor = parent_of(conn, parent_id)?;
+    // Bounded so a cycle already present in the data (written before this
+    // check existed) errors out instead of looping forever.
+    for _ in 0..256 {
+        let Some(current) = cursor else {
+            return Ok(());
+        };
+        if current == id {
+            return Err(crate::error::Error::Validation(format!(
+                "item {parent_id} is a descendant of {id} — re-parenting would create a cycle"
+            )));
+        }
+        cursor = parent_of(conn, &current)?;
+    }
+    Err(crate::error::Error::Validation(format!(
+        "parent chain above {parent_id} is deeper than 256 items or already cyclic"
+    )))
+}
+
 pub fn update(conn: &Connection, id: &str, input: UpdateItem) -> Result<Item> {
     let ts = now();
+    if let Some(Some(parent_id)) = input.parent_id.as_ref() {
+        validate_parent(conn, id, parent_id)?;
+    }
     let assignee_agent = input
         .assignee_agent
         .as_deref()
@@ -204,6 +255,10 @@ pub fn update(conn: &Connection, id: &str, input: UpdateItem) -> Result<Item> {
     }
     if input.metadata.is_some() {
         sets.push(format!("metadata = ?{param_idx}"));
+        param_idx += 1;
+    }
+    if input.parent_id.is_some() {
+        sets.push(format!("parent_id = ?{param_idx}"));
     }
     let sql = format!(
         "UPDATE items SET {} WHERE id = ?1 AND deleted_at IS NULL",
@@ -234,6 +289,9 @@ pub fn update(conn: &Connection, id: &str, input: UpdateItem) -> Result<Item> {
     if let Some(ref metadata) = input.metadata {
         param_values.push(Box::new(metadata.clone()));
     }
+    if let Some(ref parent_id) = input.parent_id {
+        param_values.push(Box::new(parent_id.clone()));
+    }
     let changed = stmt.execute(rusqlite::params_from_iter(param_values.iter()))?;
     if changed == 0 {
         return Err(crate::error::Error::NotFound(id.to_string()));

crates/agentflare-backend/src/item/mod.rs

@@ -65,6 +65,11 @@ pub struct UpdateItem {
     pub assignee_agent: Option<String>,
     pub sort_order: Option<f64>,
     pub metadata: Option<String>,
+    /// Re-parenting is three-valued, unlike every other field here:
+    /// `None` leaves the parent alone, `Some(Some(id))` re-parents, and
+    /// `Some(None)` detaches the item from its parent — otherwise a parent
+    /// set by mistake could never be removed again.
+    pub parent_id: Option<Option<String>>,
 }
 
 fn now() -> i64 {

crates/agentflare-backend/src/item/tests.rs

@@ -1354,3 +1354,127 @@ fn list_by_label_returns_only_items_carrying_that_label() {
     assert_eq!(found.len(), 1);
     assert_eq!(found[0].id, labeled.id);
 }
+
+#[test]
+fn update_sets_and_clears_parent_id() {
+    let conn = db::open_in_memory().unwrap();
+    let (pid, sid) = seed_project(&conn, "");
+    let parent = make_item(&conn, &pid, &sid);
+    let child = make_item(&conn, &pid, &sid);
+
+    let reparented = update(
+        &conn,
+        &child.id,
+        UpdateItem {
+            parent_id: Some(Some(parent.id.clone())),
+            ..Default::default()
+        },
+    )
+    .unwrap();
+    assert_eq!(reparented.parent_id.as_deref(), Some(parent.id.as_str()));
+    assert_eq!(
+        get(&conn, &child.id).unwrap().parent_id.as_deref(),
+        Some(parent.id.as_str())
+    );
+
+    // An unrelated update must leave the parent alone...
+    let renamed = update(
+        &conn,
+        &child.id,
+        UpdateItem {
+            name: Some("Renamed".into()),
+            ..Default::default()
+        },
+    )
+    .unwrap();
+    assert_eq!(renamed.parent_id.as_deref(), Some(parent.id.as_str()));
+
+    // ...while an explicit Some(None) detaches it.
+    let detached = update(
+        &conn,
+        &child.id,
+        UpdateItem {
+            parent_id: Some(None),
+            ..Default::default()
+        },
+    )
+    .unwrap();
+    assert_eq!(detached.parent_id, None);
+}
+
+#[test]
+fn update_rejects_self_parent() {
+    let conn = db::open_in_memory().unwrap();
+    let (pid, sid) = seed_project(&conn, "");
+    let item = make_item(&conn, &pid, &sid);
+    let err = update(
+        &conn,
+        &item.id,
+        UpdateItem {
+            parent_id: Some(Some(item.id.clone())),
+            ..Default::default()
+        },
+    )
+    .unwrap_err();
+    assert!(
+        matches!(err, crate::error::Error::Validation(ref m) if m.contains("its own parent")),
+        "{err:?}"
+    );
+    assert_eq!(get(&conn, &item.id).unwrap().parent_id, None);
+}
+
+#[test]
+fn update_rejects_a_parent_that_would_close_a_cycle() {
+    let conn = db::open_in_memory().unwrap();
+    let (pid, sid) = seed_project(&conn, "");
+    let grandparent = make_item(&conn, &pid, &sid);
+    let parent = make_item(&conn, &pid, &sid);
+    let child = make_item(&conn, &pid, &sid);
+    for (item, new_parent) in [(&parent, &grandparent), (&child, &parent)] {
+        update(
+            &conn,
+            &item.id,
+            UpdateItem {
+                parent_id: Some(Some(new_parent.id.clone())),
+                ..Default::default()
+            },
+        )
+        .unwrap();
+    }
+
+    // grandparent -> child would make the chain loop forever.
+    let err = update(
+        &conn,
+        &grandparent.id,
+        UpdateItem {
+            parent_id: Some(Some(child.id.clone())),
+            ..Default::default()
+        },
+    )
+    .unwrap_err();
+    assert!(
+        matches!(err, crate::error::Error::Validation(ref m) if m.contains("cycle")),
+        "{err:?}"
+    );
+    assert_eq!(get(&conn, &grandparent.id).unwrap().parent_id, None);
+}
+
+#[test]
+fn update_rejects_an_unknown_parent_instead_of_leaking_a_foreign_key_error() {
+    let conn = db::open_in_memory().unwrap();
+    let (pid, sid) = seed_project(&conn, "");
+    let item = make_item(&conn, &pid, &sid);
+    let err = update(
+        &conn,
+        &item.id,
+        UpdateItem {
+            parent_id: Some(Some("no-such-item".into())),
+            ..Default::default()
+        },
+    )
+    .unwrap_err();
+    assert!(
+        matches!(err, crate::error::Error::NotFound(ref m) if m.contains("no-such-item")),
+        "{err:?}"
+    );
+}

src/mcp_server/comment.rs

@@ -20,6 +20,10 @@ impl AgentflareMcp {
                 }
                 let author = crate::claims::owner_id();
                 self.with_backend_db(|conn| {
+                    // `327` / `#327` / the UUID all work, same as every `item`
+                    // action — without this the sequence_id went straight into
+                    // the INSERT and came back as a raw FK error (#375).
+                    let item_id = self.resolve_existing_item_id(conn, &item_id)?;
                     let comment =
                         agentflare_backend::comment::create(conn, &item_id, &author, &body)
                             .map_err(map_backend_err)?;
@@ -150,6 +154,9 @@ impl AgentflareMcp {
                     return Err(ErrorData::invalid_params("item_id is required", None));
                 }
                 self.with_backend_db(|conn| {
+                    // Same resolution as `create` — a sequence_id here quietly
+                    // returned an empty list instead of the item's comments.
+                    let item_id = self.resolve_existing_item_id(conn, &item_id)?;
                     let comments = agentflare_backend::comment::list_by_item(conn, &item_id)
                         .map_err(map_backend_err)?;
                     Ok(serde_json::to_string_pretty(&comments).unwrap_or_default())

src/mcp_server/item.rs

@@ -214,6 +214,28 @@ impl AgentflareMcp {
             .map_err(map_backend_err)
     }
 
+    /// [`Self::resolve_item_id`] plus proof that the item actually exists.
+    /// `resolve_id` passes a non-numeric id straight through, so an unknown
+    /// UUID otherwise reaches the write and surfaces as a raw
+    /// "FOREIGN KEY constraint failed" (#375) that names nothing. Use this
+    /// wherever an id is about to be written as a foreign key rather than
+    /// read back through a call that would 404 on its own.
+    pub(crate) fn resolve_existing_item_id(
+        &self,
+        conn: &Connection,
+        raw: &str,
+    ) -> Result<String, ErrorData> {
+        let id = self.resolve_item_id(conn, raw)?;
+        match agentflare_backend::item::get(conn, &id) {
+            Ok(_) => Ok(id),
+            Err(agentflare_backend::Error::NotFound(_)) => Err(ErrorData::invalid_params(
+                format!("no item matches id '{raw}'"),
+                None,
+            )),
+            Err(e) => Err(map_backend_err(e)),
+        }
+    }
+
     /// Resolve a target state from exactly one of `state_id`, `state_name`
     /// (case-insensitive exact match), or `state_group`. Errors if none or
     /// more than one of the three is given, or if a name/group doesn't
@@ -319,13 +341,22 @@ impl AgentflareMcp {
                         .id
                 }
             };
+            // A sequence_id or `#`-prefixed id must resolve the same way
+            // `update`'s parent_id does (#375/#377) — otherwise it reaches
+            // the INSERT's FK column raw and fails as an opaque "FOREIGN
+            // KEY constraint failed" instead of naming the bad id.
+            let parent_id = match req.parent_id.as_deref() {
+                None => None,
+                Some(p) if p.trim().is_empty() => None,
+                Some(p) => Some(self.resolve_existing_item_id(conn, p)?),
+            };
             let input = agentflare_backend::item::CreateItem {
                 project_id: project.id,
                 state_id,
                 name,
                 description: req.description,
                 priority: req.priority,
-                parent_id: req.parent_id,
+                parent_id,
                 assignee_agent: req.assignee_agent,
                 sort_order: None,
                 external_source: None,
@@ -453,6 +484,14 @@ impl AgentflareMcp {
         }
         self.with_backend_db(|conn| {
             let id = self.resolve_item_id(conn, &raw)?;
+            // `parent_id` used to be accepted and silently dropped here (#377).
+            // An explicit empty string detaches the item; anything else is a
+            // sequence_id or UUID resolved the same way `id` is.
+            let parent_id = match req.parent_id.as_deref() {
+                None => None,
+                Some(p) if p.trim().is_empty() => Some(None),
+                Some(p) => Some(Some(self.resolve_existing_item_id(conn, p)?)),
+            };
             let input = agentflare_backend::item::UpdateItem {
                 name: req.name,
                 description: req.description,
@@ -461,6 +500,7 @@ impl AgentflareMcp {
                 assignee_agent: req.assignee_agent.clone(),
                 sort_order: None,
                 metadata: req.metadata.map(metadata_to_json_string),
+                parent_id,
             };
             let item =
                 agentflare_backend::item::update(conn, &id, input).map_err(map_backend_err)?;