fix: three dispatch-reliability bugs found dogfooding autonomous work

crates/agentflare-backend/src/item.rs

@@ -647,7 +647,14 @@ pub enum ClaimOutcome {
 /// `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 {
+///
+/// `pub` because `assignee_agent` legitimately carries the instance suffix
+/// after a claim (`claim()` below stores the raw `owner`, on purpose — see
+/// its own doc comment and the tests pinning that), so any caller outside
+/// this module that reads `assignee_agent` back to resolve *which agent
+/// type* it names (not which specific instance) needs the same stripping
+/// this module already does internally, instead of re-deriving it.
+pub fn agent_part(owner: &str) -> String {
     agent_registry::canonicalize(owner.split(':').next().unwrap_or(owner))
 }
 

crates/flare-git-core/src/worktree.rs

@@ -441,6 +441,30 @@ pub(crate) fn run_output_timeout(
     })
 }
 
+/// Whether `branch` has committed content `target_branch` doesn't already
+/// have: new commits (`rev-list --count`) whose *content* isn't already
+/// fully present on the target either (`diff --quiet` content-diff guard,
+/// catching squash-merges — two-dot, not three-dot: we want whether the two
+/// tips are identical, not whether branch differs from merge-base).
+///
+/// Used by `push_branch` below to skip pushing/PR-ing a no-op branch, and by
+/// `item_done` (main binary) to tell a genuinely empty run (no commits at
+/// all) apart from a run whose push/PR failed for some other reason —
+/// only the former should block marking an item done.
+pub fn branch_diverged(repo_root: &Path, branch: &str, target_branch: &str) -> bool {
+    match run_git_in(
+        repo_root,
+        &["rev-list", "--count", &format!("{target_branch}..{branch}")],
+    ) {
+        Ok(count) if count != "0" => {}
+        _ => return false,
+    }
+    !run_git_in_ok(
+        repo_root,
+        &["diff", "--quiet", &format!("{target_branch}..{branch}")],
+    )
+}
+
 /// Pushes `item`'s isolated worktree branch to `target_branch`'s remote, if
 /// the branch exists, has new commits, and its content isn't already fully
 /// present on the target (squash-merge guard). Returns the pushed branch
@@ -467,21 +491,7 @@ pub fn push_branch(
     }
     // Nothing to push (and nothing worth a PR) if the branch never
     // diverged from its target — e.g. `done` called with no commits made.
-    match run_git_in(
-        repo_root,
-        &["rev-list", "--count", &format!("{target_branch}..{branch}")],
-    ) {
-        Ok(count) if count != "0" => {}
-        _ => return None,
-    }
-    // Content-diff guard: even when the branch has new commits, its
-    // *content* may already be on the target (squash-merge). Compares
-    // target→branch tree (two-dot, not three-dot: we want whether the
-    // two tips are identical, not whether branch differs from merge-base).
-    if run_git_in_ok(
-        repo_root,
-        &["diff", "--quiet", &format!("{target_branch}..{branch}")],
-    ) {
+    if !branch_diverged(repo_root, &branch, target_branch) {
         return None;
     }
     if let Some(p) = progress {

src/cli/work.rs

@@ -215,9 +215,18 @@ fn resolve_agent(
             .ok_or_else(|| format!("unknown agent: {name} — use `agentflare agents list`"));
     }
 
+    // `assignee_agent` may carry an instance suffix (`<agent>:<instance>`)
+    // once the item has been claimed at least once — `item::claim` stores
+    // the raw claim owner there deliberately (see its doc comment). Strip it
+    // via the same `agent_part` the claim/handoff-freeze logic already uses
+    // internally, so a previously-claimed item still routes to its own
+    // assignee instead of silently falling through to the router's other
+    // rules.
     let assigned_agent = item
         .assignee_agent
         .as_deref()
+        .map(agentflare_backend::item::agent_part)
+        .as_deref()
         .and_then(agent_registry::agent_by_name);
     let task = agent_registry::TaskContext {
         labels: labels.to_vec(),
@@ -681,6 +690,20 @@ mod tests {
         assert_eq!(reason, "explicit assignment on task");
     }
 
+    #[test]
+    fn resolve_agent_falls_back_to_an_instance_suffixed_assignee() {
+        // A previously-claimed item's assignee_agent carries
+        // `<agent>:<instance>` (see item::claim's doc comment) — this must
+        // still route correctly, or a once-claimed item silently loses its
+        // assignee on the next auto-routed dispatch.
+        let mut item = test_item();
+        item.assignee_agent = Some("claude-code:some-job-id".to_string());
+        let config = agent_registry::RouterConfig::default();
+        let (agent, reason) = resolve_agent(None, &item, &[], &config, &[]).unwrap();
+        assert_eq!(agent, agent_registry::Agent::ClaudeCode);
+        assert_eq!(reason, "explicit assignment on task");
+    }
+
     #[test]
     fn resolve_agent_errors_when_no_flag_and_no_assignment_and_no_rule() {
         let item = test_item();

src/mcp_server/handoff.rs

@@ -1,5 +1,18 @@
 use super::*;
 
+/// The project's `ready-for-work` label id, if the project has that label at
+/// all — skipped (returns `None`) rather than creating it out of nowhere.
+/// Shared by both branches of `handoff_impl` below: a brand-new handed-off
+/// item, and an existing item that's safe to queue (see the `Some(id)`
+/// branch's own comment for what "safe" means there).
+fn ready_label_id(conn: &rusqlite::Connection, project_id: &str) -> Option<String> {
+    agentflare_backend::label::list_by_project(conn, project_id)
+        .ok()?
+        .into_iter()
+        .find(|l| l.name == crate::supervisor::READY_LABEL)
+        .map(|l| l.id)
+}
+
 impl AgentflareMcp {
     pub fn handoff_impl(
         &self,
@@ -101,7 +114,34 @@ impl AgentflareMcp {
                         assignee_agent: Some(recipient.clone()),
                         ..Default::default()
                     };
-                    agentflare_backend::item::update(conn, id, input).map_err(map_backend_err)?
+                    let item = agentflare_backend::item::update(conn, id, input)
+                        .map_err(map_backend_err)?;
+                    // Queue it for autonomous dispatch too, same as the
+                    // brand-new-item path below — but only when it's safe:
+                    // genuinely fresh (backlog/unstarted/triage) and nobody
+                    // has ever claimed it. An explicit `item_id` handoff onto
+                    // something already claimed, in progress, in review, or
+                    // completed must NOT be silently re-queued — same danger
+                    // the reply/continuation branch below already guards
+                    // against, just reached via a different path (a caller
+                    // passing `item_id` directly instead of relying on
+                    // name/thread matching). Without this, `handoff` onto an
+                    // existing item only ever set the assignee — queuing it
+                    // needed a separate `item add_label` call every time.
+                    let state = agentflare_backend::state::get(conn, &item.state_id)
+                        .map_err(map_backend_err)?;
+                    let never_claimed =
+                        agentflare_backend::claim::current_owner(conn, id).is_none();
+                    if never_claimed
+                        && matches!(
+                            state.group_name.as_str(),
+                            "backlog" | "unstarted" | "triage"
+                        )
+                        && let Some(ready_id) = ready_label_id(conn, &project.id)
+                    {
+                        let _ = agentflare_backend::item::add_label(conn, id, &ready_id);
+                    }
+                    item
                 }
                 None => {
                     // Reuse an existing open item already assigned to the
@@ -159,15 +199,7 @@ impl AgentflareMcp {
                         // may already be claimed, in progress, or done, and
                         // silently re-queuing those for dispatch would be
                         // wrong.
-                        let ready_label_id =
-                            agentflare_backend::label::list_by_project(conn, &project.id)
-                                .ok()
-                                .and_then(|labels| {
-                                    labels
-                                        .into_iter()
-                                        .find(|l| l.name == crate::supervisor::READY_LABEL)
-                                })
-                                .map(|l| l.id);
+                        let ready_label_id = ready_label_id(conn, &project.id);
                         let input = agentflare_backend::item::CreateItem {
                             project_id: project.id.clone(),
                             state_id,
@@ -661,7 +693,7 @@ mod tests {
     }
 
     #[test]
-    fn a_reply_to_an_existing_item_is_not_labeled_ready_for_work() {
+    fn a_reply_to_an_already_claimed_item_is_not_relabeled_ready_for_work() {
         let (_tmp, mcp) = test_mcp();
         seed_ready_for_work_label(&mcp);
 
@@ -671,8 +703,32 @@ mod tests {
             .as_str()
             .unwrap()
             .to_string();
-        // A human clears the label after picking it up manually, same as the
-        // supervisor's own discovery tick would once it dispatches the item.
+        // The recipient actually claims it — moves to "started", creates a
+        // claim row — same as the supervisor's own discovery tick dispatch
+        // would once it picks the item up. This is the real condition the
+        // explicit-`item_id` branch must protect against, not merely the
+        // label being absent (see the sibling test below: a still-fresh
+        // item with no label DOES get relabeled now, on purpose). Claims
+        // directly against the backend (not through the MCP `item_claim`
+        // wrapper, whose owner resolves ambiently from process identity —
+        // not controllable here) with an owner matching the assignee
+        // (`claude-code`), since `item::claim`'s handoff-freeze rule blocks
+        // a mismatched owner from acquiring a freshly handed-off item.
+        mcp.with_backend_db(|conn| {
+            agentflare_backend::item::claim(
+                conn,
+                &item_id,
+                "claude-code:test",
+                db_kit::ids::now(),
+                1800,
+            )
+        })
+        .unwrap()
+        .unwrap();
+        // The real dispatch path (`supervisor::dispatch_item`) strips the
+        // ready-for-work label the moment it enqueues a job, well before
+        // the agent ever claims anything — mirror that here so this test
+        // reflects the label state a real in-flight item actually has.
         mcp.with_backend_db(|conn| {
             let label_ids = agentflare_backend::item::list_labels(conn, &item_id).unwrap();
             for id in label_ids {
@@ -681,8 +737,8 @@ mod tests {
         })
         .unwrap();
 
-        // A reply (item_id set) must not silently re-queue an item that may
-        // already be claimed, in progress, or done.
+        // A reply (item_id set) must not silently re-queue an item that's
+        // already claimed, in progress, or done.
         let reply = HandoffRequest {
             item_id: Some(item_id.clone()),
             completed: "more".to_string(),
@@ -693,4 +749,42 @@ mod tests {
 
         assert!(item_label_names(&mcp, &item_id).is_empty());
     }
+
+    #[test]
+    fn an_explicit_item_id_handoff_labels_a_still_fresh_unclaimed_item() {
+        let (_tmp, mcp) = test_mcp();
+        seed_ready_for_work_label(&mcp);
+
+        // First handoff creates the item, then a human clears the label
+        // without actually claiming it (e.g. picked up by hand outside the
+        // autonomous queue, or just never got labeled the first time).
+        let first = mcp.handoff_impl(base_request()).unwrap();
+        let item_id = serde_json::from_str::<serde_json::Value>(&first).unwrap()["item_id"]
+            .as_str()
+            .unwrap()
+            .to_string();
+        mcp.with_backend_db(|conn| {
+            let label_ids = agentflare_backend::item::list_labels(conn, &item_id).unwrap();
+            for id in label_ids {
+                agentflare_backend::item::remove_label(conn, &item_id, &id).unwrap();
+            }
+        })
+        .unwrap();
+
+        // A second handoff onto the same still-fresh, never-claimed item
+        // must queue it for dispatch — the whole point of this change is
+        // that a single `handoff(item_id=...)` call is enough, no separate
+        // `item add_label` call required.
+        let reply = HandoffRequest {
+            item_id: Some(item_id.clone()),
+            completed: "more".to_string(),
+            remaining: "less".to_string(),
+            ..base_request()
+        };
+        mcp.handoff_impl(reply).unwrap();
+
+        assert!(
+            item_label_names(&mcp, &item_id).contains(&crate::supervisor::READY_LABEL.to_string())
+        );
+    }
 }

src/mcp_server/item.rs

@@ -634,8 +634,54 @@ impl AgentflareMcp {
         // no-ops (logging) rather than trusting push/PR success alone as
         // proof nothing would be lost.
         let in_review = owns_claim && pr_url.is_some();
+        // No PR resulted — either nothing was ever committed on the claimed
+        // branch, or a real commit's push/PR failed for some other reason.
+        // Only the former should block completion: marking an item
+        // "completed" when its branch never diverged from target claims
+        // work was delivered when none was (item #48) — a headless run that
+        // merely replies with text, with no tool use, previously exited 0
+        // and sailed straight through to `mark_completed` below with zero
+        // code changed.
+        let nothing_was_ever_committed = !in_review
+            && owns_claim
+            && item
+                .as_ref()
+                .zip(target_branch.as_ref())
+                .is_none_or(|(item, target)| {
+                    !crate::worktree::branch_diverged(item, &repo_root, target)
+                });
+        // Shared by both "nothing was ever committed" (worktree is clean by
+        // definition, safe to remove) and a real completion: release the
+        // lease so the item is genuinely available again — either for a
+        // fresh claim (nothing done) or because the work is done — instead
+        // of leaving it wedged on a claim nobody will ever release.
+        // `cleanup_worktree` itself still verifies the tree is clean before
+        // removing it, so this is safe even if `nothing_was_ever_committed`
+        // somehow raced with an uncommitted local change.
+        let release_claim_and_cleanup = |item: &Option<agentflare_backend::item::Item>| {
+            if let Some(item) = item {
+                crate::worktree::cleanup_worktree(item, &repo_root);
+            }
+            match self.with_backend_db(|conn| {
+                agentflare_backend::claim::done(conn, &item_id, &owner, now)
+            }) {
+                Ok(Ok(true)) => {}
+                Ok(Ok(false)) => eprintln!(
+                    "worktree: releasing claim for item {item_id} affected no rows (owner mismatch or already released)"
+                ),
+                Ok(Err(e)) => {
+                    eprintln!("worktree: failed to release claim for item {item_id}: {e}")
+                }
+                Err(e) => {
+                    eprintln!("worktree: failed to release claim for item {item_id}: {e:?}")
+                }
+            }
+        };
         let done = if !owns_claim {
             false
+        } else if nothing_was_ever_committed {
+            release_claim_and_cleanup(&item);
+            false
         } else if in_review {
             self.with_backend_db(|conn| {
                 agentflare_backend::item::mark_in_review(conn, &item_id, &owner)
@@ -647,23 +693,7 @@ impl AgentflareMcp {
                     .map_err(map_backend_err)
             })??;
             if moved {
-                if let Some(item) = &item {
-                    crate::worktree::cleanup_worktree(item, &repo_root);
-                }
-                match self.with_backend_db(|conn| {
-                    agentflare_backend::claim::done(conn, &item_id, &owner, now)
-                }) {
-                    Ok(Ok(true)) => {}
-                    Ok(Ok(false)) => eprintln!(
-                        "worktree: releasing claim for item {item_id} affected no rows (owner mismatch or already released)"
-                    ),
-                    Ok(Err(e)) => {
-                        eprintln!("worktree: failed to release claim for item {item_id}: {e}")
-                    }
-                    Err(e) => {
-                        eprintln!("worktree: failed to release claim for item {item_id}: {e:?}")
-                    }
-                }
+                release_claim_and_cleanup(&item);
             }
             moved
         };

src/mcp_server/tests/action_tests.rs

@@ -354,7 +354,7 @@ fn item_claim_response_includes_worktree_error_instead_of_silently_omitting_it()
 }
 
 #[test]
-fn item_done_without_new_commits_omits_pr_fields() {
+fn item_done_without_new_commits_leaves_the_item_unchanged() {
     let tmp = tempfile::tempdir().unwrap();
     let repo_dir = tempfile::tempdir().unwrap();
     let repo_root = repo_dir.path().to_path_buf();
@@ -390,7 +390,8 @@ fn item_done_without_new_commits_omits_pr_fields() {
 
     // No commits were made in the claimed worktree, so `done` has
     // nothing to push/PR — must not attempt a real push (no remote
-    // configured on this throwaway repo).
+    // configured on this throwaway repo), and must not mark the item
+    // completed either (item #48): nothing was actually delivered.
     let result: serde_json::Value = serde_json::from_str(
         &s.item(Parameters(ItemRequest {
             action: "done".into(),
@@ -400,7 +401,8 @@ fn item_done_without_new_commits_omits_pr_fields() {
         .unwrap(),
     )
     .unwrap();
-    assert_eq!(result["done"], true);
+    assert_eq!(result["done"], false);
+    assert_eq!(result["status"], "unchanged");
     assert!(result.get("pr_url").is_none());
     assert!(result.get("next").is_none());
 }