From 0e7b25583bb5d6e871067543c2736a8b86d7e1f5 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Wed, 29 Jul 2026 10:24:21 +0530 Subject: [PATCH] fix(handoff): fmt, canonicalize claim owner, validate commit oid/payload off the DB lock - cargo fmt (item.rs claim closure, types.rs schemars doc attr) - item::claim: canonicalize both sides of the assignee/owner comparison so an alias owner (claude:1) isn't wrongly BlockedByAssignee against its own canonical handoff assignee (claude-code) - verify_continuation_commit: resolve the target branch under the backend DB lock, then run all git subprocess checks after releasing it, matching the existing item_claim split; validate oid is a plain hex id and force commit-type resolution (oid^{commit}) so a non-hex/flag-like value or a blob/tree/tag can't pass as a continuation commit - handoff_impl: reject empty completed/remaining instead of silently accepting an empty structured payload - mcp_prompts: document completed/remaining as required handoff fields so generated requests don't fail deserialization --- crates/agentflare-backend/src/item.rs | 36 ++++++++++-- src/mcp_prompts.rs | 2 + src/mcp_server/handoff.rs | 79 ++++++++++++++++++-------- src/mcp_server/item.rs | 19 ++++--- src/mcp_server/tests/artifact_tests.rs | 24 ++++++++ src/mcp_server/types.rs | 4 +- 6 files changed, 123 insertions(+), 41 deletions(-) diff --git a/crates/agentflare-backend/src/item.rs b/crates/agentflare-backend/src/item.rs index 2985ce4f..6c7abb9d 100644 --- a/crates/agentflare-backend/src/item.rs +++ b/crates/agentflare-backend/src/item.rs @@ -630,10 +630,13 @@ pub enum ClaimOutcome { BlockedByAssignee { assignee: String }, } -/// Agent identity part of an owner id (`:` -> ``), -/// 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 (`:` -> +/// canonical ``), 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(); diff --git a/src/mcp_prompts.rs b/src/mcp_prompts.rs index 340449d9..e2d773d1 100644 --- a/src/mcp_prompts.rs +++ b/src/mcp_prompts.rs @@ -206,6 +206,8 @@ fn get_handoff_command(request: &GetPromptRequestParams, agent: Option<&str>) -> - ` ` → call the `handoff` tool with 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= 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 \ diff --git a/src/mcp_server/handoff.rs b/src/mcp_server/handoff.rs index 27828c9d..f092ef1a 100644 --- a/src/mcp_server/handoff.rs +++ b/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::(&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/` 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!( diff --git a/src/mcp_server/item.rs b/src/mcp_server/item.rs index b62aaef6..7db5ef38 100644 --- a/src/mcp_server/item.rs +++ b/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) { diff --git a/src/mcp_server/tests/artifact_tests.rs b/src/mcp_server/tests/artifact_tests.rs index 7ef786a7..6971b1fa 100644 --- a/src/mcp_server/tests/artifact_tests.rs +++ b/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() })) diff --git a/src/mcp_server/types.rs b/src/mcp_server/types.rs index 1d9083f0..5a356cfe 100644 --- a/src/mcp_server/types.rs +++ b/src/mcp_server/types.rs @@ -237,9 +237,7 @@ pub(crate) struct HandoffRequest { pub(crate) last_commit: Option, #[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)]