From 91bcd1e1336a05137505e5a9963416cc6e0c7063 Mon Sep 17 00:00:00 2001 From: shiva Date: Sun, 9 Aug 2026 14:41:44 +0530 Subject: [PATCH 1/4] fix: resolve an assignee_agent that carries a claim's instance suffix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit item::claim() deliberately stores the raw claim owner (:) into assignee_agent on acquire, not just the bare agent name — intentional, pinned by existing tests. But resolve_confirmed_agent (supervisor.rs's discovery tick) and resolve_agent's assignee fallback (cli/work.rs) matched that field against the agent registry with an exact string comparison, so once an item had been claimed at least once, its own assignee could never be recognized again on a later dispatch attempt — it would silently get skipped or fall through to other routing rules. Both now reuse item::agent_part (already used internally by claim()'s own handoff-freeze comparison) to strip the instance suffix before matching, instead of duplicating that logic or changing the write side. --- crates/agentflare-backend/src/item.rs | 9 ++++++++- src/cli/work.rs | 23 +++++++++++++++++++++++ src/supervisor.rs | 21 ++++++++++++++++++++- 3 files changed, 51 insertions(+), 2 deletions(-) diff --git a/crates/agentflare-backend/src/item.rs b/crates/agentflare-backend/src/item.rs index c982ed3d..dc783d28 100644 --- a/crates/agentflare-backend/src/item.rs +++ b/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)) } diff --git a/src/cli/work.rs b/src/cli/work.rs index ee073c58..57e2f527 100644 --- a/src/cli/work.rs +++ b/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 (`:`) + // 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 + // `:` (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(); diff --git a/src/supervisor.rs b/src/supervisor.rs index c4ff6e59..e9a2e859 100644 --- a/src/supervisor.rs +++ b/src/supervisor.rs @@ -30,10 +30,18 @@ const WORK_JOB_TIMEOUT_SECS: u64 = 21_900; /// Returns the matching `Agent` only if `agent_registry::autonomous_args` /// confirms it has a headless permission-bypass flag — the same gate /// `agentflare work` itself uses (`src/cli/work.rs`'s `run_work`). +/// +/// `assignee` may carry an instance suffix (`:`) once an +/// item has been claimed at least once — `item::claim` deliberately stores +/// the raw claim owner there (see its doc comment). Strip it via the same +/// `agent_part` the claim/handoff-freeze logic itself uses internally, +/// rather than matching the raw string and silently failing to recognize a +/// previously-claimed item's own assignee. pub(crate) fn resolve_confirmed_agent(assignee: &str) -> Option { + let canonical = agentflare_backend::item::agent_part(assignee); let agent = agent_registry::REGISTRY .iter() - .find(|s| s.id.as_str() == assignee) + .find(|s| s.id.as_str() == canonical) .map(|s| s.id)?; agent_registry::autonomous_args(agent).map(|_| agent) } @@ -237,6 +245,17 @@ mod tests { ); } + #[test] + fn resolve_confirmed_agent_recognizes_an_instance_suffixed_assignee() { + // A previously-claimed item's assignee_agent carries `:` + // (see item::claim's doc comment) — this must still resolve, or a + // once-claimed item can never be redispatched. + assert_eq!( + resolve_confirmed_agent("claude-code:some-job-id"), + Some(agent_registry::Agent::ClaudeCode) + ); + } + #[test] fn resolve_confirmed_agent_rejects_opencode() { assert_eq!(resolve_confirmed_agent("opencode"), None); From 4895ad842e0e7614f9f2adccbbc66db3595ba8ce Mon Sep 17 00:00:00 2001 From: shiva Date: Sun, 9 Aug 2026 15:04:00 +0530 Subject: [PATCH 2/4] fix: item done no longer marks completed when nothing was ever committed item_done unconditionally called mark_completed whenever a claim owner called done and no PR resulted, regardless of whether the claimed branch had any real commits on it. A headless work run that only replies with text (no tool use, no commits) previously exited 0 and sailed straight through to being marked "completed" with zero code changed. Reuses branch_diverged (extracted from push_branch's existing squash-merge guard, now shared between flare-git-core and the main binary) to tell a genuinely empty run apart from one whose push/PR failed for another reason. A genuinely empty run now releases the claim and cleans up the (by-definition clean) worktree so the item is available for retry, but is left unchanged rather than falsely marked completed. Agentflare-Agent: claude-code_2-1-226_agent Agentflare-Branch: task/48 Agentflare-Item: 48 --- crates/flare-git-core/src/worktree.rs | 40 ++++++++++------- src/mcp_server/item.rs | 64 ++++++++++++++++++++------- src/mcp_server/tests/action_tests.rs | 8 ++-- src/worktree.rs | 11 +++++ 4 files changed, 88 insertions(+), 35 deletions(-) diff --git a/crates/flare-git-core/src/worktree.rs b/crates/flare-git-core/src/worktree.rs index 2e9ee356..4d3b4fcf 100644 --- a/crates/flare-git-core/src/worktree.rs +++ b/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 { diff --git a/src/mcp_server/item.rs b/src/mcp_server/item.rs index 4b3ded34..0eefdc97 100644 --- a/src/mcp_server/item.rs +++ b/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| { + 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 }; diff --git a/src/mcp_server/tests/action_tests.rs b/src/mcp_server/tests/action_tests.rs index c2de6159..420c0252 100644 --- a/src/mcp_server/tests/action_tests.rs +++ b/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()); } diff --git a/src/worktree.rs b/src/worktree.rs index 8db169c8..86e8a109 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -22,6 +22,17 @@ fn as_progress(p: Option<&ProgressSender>) -> Option<&dyn flare_git_core::worktr pub use flare_git_core::worktree::resolve_target_branch; +/// Whether `item`'s branch has any committed content `target_branch` +/// doesn't already have. See `flare_git_core::worktree::branch_diverged`. +pub fn branch_diverged( + item: &agentflare_backend::item::Item, + repo_root: &Path, + target_branch: &str, +) -> bool { + let branch = format!("task/{}", item.sequence_id); + flare_git_core::worktree::branch_diverged(repo_root, &branch, target_branch) +} + pub fn create_worktree( item: &agentflare_backend::item::Item, repo_root: &Path, From 2447bfd260ee4bb18f11f430f83af4dc7309ee36 Mon Sep 17 00:00:00 2001 From: shiva Date: Sun, 9 Aug 2026 18:22:44 +0530 Subject: [PATCH 3/4] handoff: auto-queue a fresh, unclaimed existing item for dispatch too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously handoff(item_id=X) only ever set assignee_agent — queuing an existing item for the supervisor's discovery tick required a separate item(action=add_label) call every time, unlike a brand-new handed-off item which already got labeled ready-for-work automatically. Extends that to the explicit item_id path, but only when it's actually safe: the item has never been claimed and is still in a genuinely fresh state group (backlog/unstarted/triage). An item that's already claimed, in progress, in review, or completed is left alone, same as the existing reply/continuation path already protects against — re-queuing live work would be wrong regardless of which code path reached it. --- src/mcp_server/handoff.rs | 120 +++++++++++++++++++++++++++++++++----- 1 file changed, 105 insertions(+), 15 deletions(-) diff --git a/src/mcp_server/handoff.rs b/src/mcp_server/handoff.rs index d48a3806..a6e15128 100644 --- a/src/mcp_server/handoff.rs +++ b/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 { + 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, @@ -78,7 +91,30 @@ 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 @@ -136,15 +172,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, @@ -449,7 +477,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); @@ -459,8 +487,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 { @@ -469,8 +521,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(), @@ -481,4 +533,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::(&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()) + ); + } } From 0d91bb0672d537c4424870acc19c1179ed60926a Mon Sep 17 00:00:00 2001 From: shiva Date: Sun, 9 Aug 2026 18:39:32 +0530 Subject: [PATCH 4/4] fmt: apply cargo fmt to handoff.rs --- src/mcp_server/handoff.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/mcp_server/handoff.rs b/src/mcp_server/handoff.rs index a6e15128..4d043f7a 100644 --- a/src/mcp_server/handoff.rs +++ b/src/mcp_server/handoff.rs @@ -91,8 +91,8 @@ impl AgentflareMcp { assignee_agent: Some(recipient.clone()), ..Default::default() }; - let item = - 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 @@ -107,9 +107,13 @@ impl AgentflareMcp { // 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(); + let never_claimed = + agentflare_backend::claim::current_owner(conn, id).is_none(); if never_claimed - && matches!(state.group_name.as_str(), "backlog" | "unstarted" | "triage") + && 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);