fix(handoff): address PR #365 review findings

crates/agentflare-backend/src/item.rs

@@ -630,10 +630,13 @@ pub enum ClaimOutcome {
     BlockedByAssignee { assignee: String },
 }
 
-/// Agent identity part of an owner id (`<agent>:<instance>` -> `<agent>`),
-/// matching `assignee_agent`'s canonical (instance-less) form.
-fn agent_part(owner: &str) -> &str {
-    owner.split(':').next().unwrap_or(owner)
+/// Canonical agent identity of an owner id (`<agent>:<instance>` ->
+/// canonical `<agent>`), matching `assignee_agent`'s canonical form —
+/// `assignee_agent` is canonicalized on write (see `create`/`update`), but
+/// `owner` is the raw caller-supplied id, so an alias like `claude:1` must
+/// be canonicalized here too or it won't match `claude-code`.
+fn agent_part(owner: &str) -> String {
+    agent_registry::canonicalize(owner.split(':').next().unwrap_or(owner))
 }
 
 /// Claims an item so other agents don't duplicate the work: on a fresh
@@ -1484,6 +1487,31 @@ mod tests {
         assert_eq!(outcome, ClaimOutcome::Acquired);
     }
 
+    #[test]
+    fn claim_by_the_handoff_assignee_via_an_alias_succeeds() {
+        // assignee_agent is canonicalized on write ("claude" -> "claude-code"),
+        // but `owner` is the raw caller-supplied id — an alias owner must
+        // still be recognized as the assignee, not blocked as an impostor.
+        let conn = db::open_in_memory().unwrap();
+        let (pid, sid) = seed_project(&conn, "");
+        let item = make_item(&conn, &pid, &sid);
+        update(
+            &conn,
+            &item.id,
+            UpdateItem {
+                assignee_agent: Some("claude".into()),
+                ..Default::default()
+            },
+        )
+        .unwrap();
+        assert_eq!(
+            get(&conn, &item.id).unwrap().assignee_agent.as_deref(),
+            Some("claude-code")
+        );
+        let outcome = claim(&conn, &item.id, "claude:1", 1000, TTL).unwrap();
+        assert_eq!(outcome, ClaimOutcome::Acquired);
+    }
+
     #[test]
     fn current_owner_returns_the_claim_owner() {
         let conn = db::open_in_memory().unwrap();

src/mcp_prompts.rs

@@ -206,6 +206,8 @@ fn get_handoff_command(request: &GetPromptRequestParams, agent: Option<&str>) ->
          - `<recipient> <brief>` → call the `handoff` tool with recipient=<recipient>, \
          name from the brief, content = the work product the brief points at (the preceding \
          conversation content, diff, review, or document — ask only if genuinely ambiguous), \
+         completed and remaining (both required — what's done, what's left), blockers if any, \
+         last_commit=<continuation commit oid> when handing off in-progress work, \
          and a thread_id when continuing an exchange. This assigns/creates an item for the \
          recipient and attaches the content to it as a versioned asset — prepend the brief to \
          the content so the recipient knows what is being asked (sender is set to your \

src/mcp_server/handoff.rs

@@ -36,6 +36,12 @@ impl AgentflareMcp {
         if content.is_empty() {
             return Err(ErrorData::invalid_params("content is required", None));
         }
+        if completed.trim().is_empty() || remaining.trim().is_empty() {
+            return Err(ErrorData::invalid_params(
+                "completed and remaining are required — an empty structured payload tells the recipient nothing",
+                None,
+            ));
+        }
         let recipient = recipient.trim().to_string();
         let name = name.trim().to_string();
         let ext = match r#type.as_deref() {
@@ -45,14 +51,27 @@ impl AgentflareMcp {
             _ => "md",
         };
 
+        // Resolve just the target branch (a DB read) under the backend
+        // lock, then run the blocking git subprocess checks below it —
+        // `verify_continuation_commit` has no business running while the
+        // shared DB mutex is held (same reasoning as `item_claim`'s split
+        // of DB resolution from `git worktree add`).
+        if let Some(oid) = &last_commit {
+            let branch = match &item_id {
+                Some(id) => self.with_backend_db(|conn| {
+                    agentflare_backend::item::get(conn, id)
+                        .ok()
+                        .map(|item| format!("task/{}", item.sequence_id))
+                })?,
+                None => None,
+            };
+            self.verify_continuation_commit(oid, branch.as_deref())?;
+        }
+
         self.with_backend_db(|conn| {
             let project = self.resolve_project(conn)?;
             let ws_id = Self::resolve_workspace_id(conn)?;
 
-            if let Some(oid) = &last_commit {
-                self.verify_continuation_commit(conn, oid, item_id.as_deref())?;
-            }
-
             let item = match &item_id {
                 Some(id) => {
                     let input = agentflare_backend::item::UpdateItem {
@@ -82,9 +101,7 @@ impl AgentflareMcp {
                                 serde_json::from_str::<serde_json::Value>(&i.metadata)
                                     .ok()
                                     .and_then(|m| {
-                                        m.get("thread")
-                                            .and_then(|v| v.as_str())
-                                            .map(str::to_string)
+                                        m.get("thread").and_then(|v| v.as_str()).map(str::to_string)
                                     })
                                     .as_deref()
                                     == Some(t.as_str())
@@ -250,36 +267,48 @@ impl AgentflareMcp {
     }
 
     /// Verified, not trusted: rejects a fabricated or typo'd continuation
-    /// OID rather than recording it as-is. `oid` must exist in the repo;
-    /// when the target item's own `task/<seq>` branch already exists
-    /// locally, `oid` must additionally be reachable from it — a handoff
-    /// can't claim to continue from a commit that branch never saw.
-    fn verify_continuation_commit(
-        &self,
-        conn: &rusqlite::Connection,
-        oid: &str,
-        item_id: Option<&str>,
-    ) -> Result<(), ErrorData> {
+    /// OID rather than recording it as-is. `oid` must exist in the repo as
+    /// a commit (not just any object); when `branch` is given and exists,
+    /// `oid` must additionally be reachable from it — a handoff can't claim
+    /// to continue from a commit that branch never saw. `branch` must be
+    /// resolved by the caller *before* calling this and outside
+    /// `with_backend_db` — this is pure git subprocess work (up to three
+    /// blocking calls) that has no business running while the shared
+    /// backend DB mutex is held.
+    fn verify_continuation_commit(&self, oid: &str, branch: Option<&str>) -> Result<(), ErrorData> {
+        // Reject anything that isn't a plain hex OID before it ever reaches
+        // git: a leading '-' would otherwise be parsed as an option by
+        // `cat-file`/`merge-base` rather than as a rev.
+        if oid.len() < 7 || !oid.chars().all(|c| c.is_ascii_hexdigit()) {
+            return Err(ErrorData::invalid_params(
+                format!("last_commit '{oid}' is not a valid object id"),
+                None,
+            ));
+        }
         let repo_root = self.worktree_repo_root();
-        if !flare_git_core::shell::run_in_ok(&repo_root, &["cat-file", "-e", oid]) {
+        // `^{commit}` forces commit-type resolution — plain `cat-file -e`
+        // also succeeds for blobs/trees/tags, which aren't valid
+        // continuation points.
+        let commit_ref = format!("{oid}^{{commit}}");
+        if !flare_git_core::shell::run_in_ok(&repo_root, &["cat-file", "-e", &commit_ref]) {
             return Err(ErrorData::invalid_params(
                 format!("last_commit '{oid}' does not exist in this repo — verified, not trusted"),
                 None,
             ));
         }
-        let Some(id) = item_id else {
+        let Some(branch) = branch else {
             return Ok(());
         };
-        let Ok(item) = agentflare_backend::item::get(conn, id) else {
-            return Ok(()); // item resolution/existence is checked separately below
-        };
-        let branch = format!("task/{}", item.sequence_id);
+        let branch_ref = format!("refs/heads/{branch}");
         let branch_exists = flare_git_core::shell::run_in_ok(
             &repo_root,
-            &["show-ref", "--verify", "--quiet", &format!("refs/heads/{branch}")],
+            &["show-ref", "--verify", "--quiet", &branch_ref],
         );
         if branch_exists
-            && !flare_git_core::shell::run_in_ok(&repo_root, &["merge-base", "--is-ancestor", oid, &branch])
+            && !flare_git_core::shell::run_in_ok(
+                &repo_root,
+                &["merge-base", "--is-ancestor", oid, branch],
+            )
         {
             return Err(ErrorData::invalid_params(
                 format!(

src/mcp_server/item.rs

@@ -394,15 +394,16 @@ impl AgentflareMcp {
             let item_id = self.resolve_item_id(conn, &raw)?;
             let outcome = agentflare_backend::item::claim(conn, &item_id, &owner, now, ttl)
                 .map_err(map_backend_err)?;
-            let (item, target_branch) = if outcome == agentflare_backend::item::ClaimOutcome::Acquired {
-                let item = agentflare_backend::item::get(conn, &item_id).ok();
-                let target_branch = item
-                    .as_ref()
-                    .map(|i| crate::worktree::resolve_target_branch(conn, i, &repo_root));
-                (item, target_branch)
-            } else {
-                (None, None)
-            };
+            let (item, target_branch) =
+                if outcome == agentflare_backend::item::ClaimOutcome::Acquired {
+                    let item = agentflare_backend::item::get(conn, &item_id).ok();
+                    let target_branch = item
+                        .as_ref()
+                        .map(|i| crate::worktree::resolve_target_branch(conn, i, &repo_root));
+                    (item, target_branch)
+                } else {
+                    (None, None)
+                };
             Ok::<_, ErrorData>((outcome, item_id, item, target_branch))
         })??;
         let worktree_result = match (&item, &target_branch) {

src/mcp_server/tests/artifact_tests.rs

@@ -316,6 +316,8 @@ fn handoff_tool_requires_recipient_and_assigns_item() {
                 recipient: "opencode".into(),
                 name: "review-packet".into(),
                 content: "please review".into(),
+                completed: "wrote the parser".into(),
+                remaining: "wire up the CLI".into(),
                 ..Default::default()
             }))
             .unwrap(),
@@ -364,6 +366,8 @@ fn handoff_trims_whitespace_padded_recipient() {
                 recipient: "  opencode  ".into(),
                 name: "review-packet".into(),
                 content: "please review".into(),
+                completed: "wrote the parser".into(),
+                remaining: "wire up the CLI".into(),
                 ..Default::default()
             }))
             .unwrap(),
@@ -402,6 +406,8 @@ fn handoff_with_item_id_assigns_existing_item_and_versions_the_asset() {
                 name: "Existing task".into(),
                 content: "v1 content".into(),
                 item_id: Some(item_id.clone()),
+                completed: "wrote v1".into(),
+                remaining: "get feedback".into(),
                 ..Default::default()
             }))
             .unwrap(),
@@ -418,6 +424,8 @@ fn handoff_with_item_id_assigns_existing_item_and_versions_the_asset() {
                 name: "Addressed feedback".into(),
                 content: "v2 content".into(),
                 item_id: Some(item_id.clone()),
+                completed: "addressed feedback".into(),
+                remaining: "ship it".into(),
                 ..Default::default()
             }))
             .unwrap(),
@@ -450,6 +458,8 @@ fn handoff_without_item_id_reuses_an_existing_open_item_with_matching_name() {
                 recipient: "opencode".into(),
                 name: "Feature X".into(),
                 content: "v1 content".into(),
+                completed: "wrote v1".into(),
+                remaining: "get feedback".into(),
                 ..Default::default()
             }))
             .unwrap(),
@@ -465,6 +475,8 @@ fn handoff_without_item_id_reuses_an_existing_open_item_with_matching_name() {
                 recipient: "opencode".into(),
                 name: "Feature X".into(),
                 content: "v2 content".into(),
+                completed: "addressed feedback".into(),
+                remaining: "ship it".into(),
                 ..Default::default()
             }))
             .unwrap(),
@@ -487,6 +499,8 @@ fn handoff_without_item_id_reuses_by_matching_thread_id() {
                 name: "Initial brief".into(),
                 content: "v1 content".into(),
                 thread_id: Some("t-abc".into()),
+                completed: "wrote v1".into(),
+                remaining: "get feedback".into(),
                 ..Default::default()
             }))
             .unwrap(),
@@ -501,6 +515,8 @@ fn handoff_without_item_id_reuses_by_matching_thread_id() {
                 name: "Follow-up".into(),
                 content: "v2 content".into(),
                 thread_id: Some("t-abc".into()),
+                completed: "addressed feedback".into(),
+                remaining: "ship it".into(),
                 ..Default::default()
             }))
             .unwrap(),
@@ -520,6 +536,8 @@ fn handoff_without_item_id_still_creates_when_no_existing_item_matches() {
                 recipient: "opencode".into(),
                 name: "Feature X".into(),
                 content: "v1 content".into(),
+                completed: "wrote v1".into(),
+                remaining: "get feedback".into(),
                 ..Default::default()
             }))
             .unwrap(),
@@ -533,6 +551,8 @@ fn handoff_without_item_id_still_creates_when_no_existing_item_matches() {
                 recipient: "opencode".into(),
                 name: "Unrelated Feature Y".into(),
                 content: "v1 content".into(),
+                completed: "wrote v1".into(),
+                remaining: "get feedback".into(),
                 ..Default::default()
             }))
             .unwrap(),
@@ -577,6 +597,8 @@ fn handoff_rejects_a_fabricated_last_commit_oid() {
                 recipient: "opencode".into(),
                 name: "review-packet".into(),
                 content: "please review".into(),
+                completed: "wrote the parser".into(),
+                remaining: "wire up the CLI".into(),
                 last_commit: Some("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef".into()),
                 ..Default::default()
             }))
@@ -598,6 +620,8 @@ fn handoff_accepts_an_existing_last_commit_oid() {
                 recipient: "opencode".into(),
                 name: "review-packet".into(),
                 content: "please review".into(),
+                completed: "wrote the parser".into(),
+                remaining: "wire up the CLI".into(),
                 last_commit: Some(head.clone()),
                 ..Default::default()
             }))

src/mcp_server/types.rs

@@ -237,9 +237,7 @@ pub(crate) struct HandoffRequest {
     pub(crate) last_commit: Option<String>,
     #[schemars(description = "What's done so far — required, part of the structured payload.")]
     pub(crate) completed: String,
-    #[schemars(
-        description = "What's left to do — required, part of the structured payload."
-    )]
+    #[schemars(description = "What's left to do — required, part of the structured payload.")]
     pub(crate) remaining: String,
     #[schemars(description = "Known blockers, if any.")]
     #[serde(default)]