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/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/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/mcp_server/handoff.rs b/src/mcp_server/handoff.rs index c9520078..0be79180 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, @@ -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::(&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()) + ); + } } 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/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); 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,