diff --git a/crates/agentflare-backend/src/item.rs b/crates/agentflare-backend/src/item.rs index 43de7897..6c7abb9d 100644 --- a/crates/agentflare-backend/src/item.rs +++ b/crates/agentflare-backend/src/item.rs @@ -617,37 +617,79 @@ pub fn search( Ok(like_rows.collect::>()?) } +/// Outcome of a claim attempt — the raw lease `Acquire` plus the handoff +/// freeze rule: while an item carries an `assignee_agent` that nobody has +/// claimed yet (a handoff sitting unaccepted), only that assignee may +/// acquire it. Once any claim has ever been taken (even a since-stale one), +/// the ordinary `Acquired`/`Held` staleness rules take back over — this +/// variant only covers the fresh, never-claimed window. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ClaimOutcome { + Acquired, + Held { owner: String, age_secs: i64 }, + BlockedByAssignee { assignee: String }, +} + +/// 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 /// acquire, sets the assignee and moves state into the project's "started" /// group (which sets `started_at`, via `update_state`). A live claim held by -/// someone else returns `Held` and leaves the item untouched. Acquisition, -/// the state transition, and the assignee update are one transaction — a -/// mid-sequence failure can't leave `item_claims` saying "claimed" while the -/// item itself never reflects it. +/// someone else returns `Held` and leaves the item untouched. An item +/// freshly handed off (assignee set, never yet claimed) to a *different* +/// agent than the caller returns `BlockedByAssignee` instead of letting the +/// caller silently steal it. Acquisition, the state transition, and the +/// assignee update are one transaction — a mid-sequence failure can't leave +/// `item_claims` saying "claimed" while the item itself never reflects it. pub fn claim( conn: &Connection, item_id: &str, owner: &str, now: i64, ttl_secs: i64, -) -> Result { +) -> Result { let tx = conn.unchecked_transaction()?; - let outcome = crate::claim::acquire(&tx, item_id, owner, now, ttl_secs)?; - if outcome == crate::claim::Acquire::Acquired { - let item = get(&tx, item_id)?; - let started_state = crate::state::first_in_group(&tx, &item.project_id, "started")?; - update_state(&tx, item_id, &started_state.id)?; - update( - &tx, - item_id, - UpdateItem { - assignee_agent: Some(owner.to_string()), - ..Default::default() - }, - )?; + let item = get(&tx, item_id)?; + if let Some(assignee) = &item.assignee_agent + && agent_part(assignee) != agent_part(owner) + && crate::claim::current_owner(&tx, item_id).is_none() + { + // Excludes completed/cancelled items: a done-and-released item is + // fair game for anyone to re-claim (e.g. reopened follow-up work) — + // the freeze only protects a handoff that's still open. + let state = crate::state::get(&tx, &item.state_id)?; + if !matches!(state.group_name.as_str(), "completed" | "cancelled") { + return Ok(ClaimOutcome::BlockedByAssignee { + assignee: assignee.clone(), + }); + } } + let outcome = crate::claim::acquire(&tx, item_id, owner, now, ttl_secs)?; + let result = match outcome { + crate::claim::Acquire::Acquired => { + let started_state = crate::state::first_in_group(&tx, &item.project_id, "started")?; + update_state(&tx, item_id, &started_state.id)?; + update( + &tx, + item_id, + UpdateItem { + assignee_agent: Some(owner.to_string()), + ..Default::default() + }, + )?; + ClaimOutcome::Acquired + } + crate::claim::Acquire::Held { owner, age_secs } => ClaimOutcome::Held { owner, age_secs }, + }; tx.commit()?; - Ok(outcome) + Ok(result) } /// Moves a claimed item into the project's "completed" group WITHOUT @@ -1366,7 +1408,7 @@ mod tests { let (pid, sid) = seed_project(&conn, ""); let item = make_item(&conn, &pid, &sid); let outcome = claim(&conn, &item.id, "agent:1", 1000, TTL).unwrap(); - assert_eq!(outcome, crate::claim::Acquire::Acquired); + assert_eq!(outcome, ClaimOutcome::Acquired); let updated = get(&conn, &item.id).unwrap(); assert_eq!(updated.assignee_agent.as_deref(), Some("agent:1")); assert_eq!(updated.state_id, state_in_group(&conn, &pid, "started")); @@ -1382,7 +1424,7 @@ mod tests { let outcome = claim(&conn, &item.id, "agent:2", 1001, TTL).unwrap(); assert!(matches!( outcome, - crate::claim::Acquire::Held { ref owner, .. } if owner == "agent:1" + ClaimOutcome::Held { ref owner, .. } if owner == "agent:1" )); let unchanged = get(&conn, &item.id).unwrap(); assert_eq!(unchanged.assignee_agent.as_deref(), Some("agent:1")); @@ -1395,11 +1437,81 @@ mod tests { let item = make_item(&conn, &pid, &sid); claim(&conn, &item.id, "agent:1", 1000, TTL).unwrap(); let outcome = claim(&conn, &item.id, "agent:2", 1000 + TTL + 1, TTL).unwrap(); - assert_eq!(outcome, crate::claim::Acquire::Acquired); + assert_eq!(outcome, ClaimOutcome::Acquired); let updated = get(&conn, &item.id).unwrap(); assert_eq!(updated.assignee_agent.as_deref(), Some("agent:2")); } + #[test] + fn claim_by_a_different_agent_than_the_handoff_assignee_is_blocked() { + let conn = db::open_in_memory().unwrap(); + let (pid, sid) = seed_project(&conn, ""); + let item = make_item(&conn, &pid, &sid); + // Simulate a handoff: assignee set, never claimed yet. + update( + &conn, + &item.id, + UpdateItem { + assignee_agent: Some("opencode".into()), + ..Default::default() + }, + ) + .unwrap(); + let outcome = claim(&conn, &item.id, "claude-code:1", 1000, TTL).unwrap(); + assert_eq!( + outcome, + ClaimOutcome::BlockedByAssignee { + assignee: "opencode".to_string() + } + ); + let unchanged = get(&conn, &item.id).unwrap(); + assert_eq!(unchanged.assignee_agent.as_deref(), Some("opencode")); + assert!(crate::claim::current_owner(&conn, &item.id).is_none()); + } + + #[test] + fn claim_by_the_handoff_assignee_itself_succeeds() { + 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("opencode".into()), + ..Default::default() + }, + ) + .unwrap(); + let outcome = claim(&conn, &item.id, "opencode:1", 1000, TTL).unwrap(); + 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(); @@ -1436,14 +1548,14 @@ mod tests { // Lease is still held — concurrent claim must be rejected. match claim(&conn, &item.id, "agent:2", 1200, TTL).unwrap() { - crate::claim::Acquire::Held { .. } => {} + ClaimOutcome::Held { .. } => {} other => panic!("expected Held after mark_completed, got {other:?}"), } // Release the lease, now re-acquirable. assert!(crate::claim::done(&conn, &item.id, "agent:1", 1300).unwrap()); let outcome = claim(&conn, &item.id, "agent:2", 1400, TTL).unwrap(); - assert_eq!(outcome, crate::claim::Acquire::Acquired); + assert_eq!(outcome, ClaimOutcome::Acquired); } #[test] diff --git a/src/mcp_prompts.rs b/src/mcp_prompts.rs index 4021f531..d3b8121b 100644 --- a/src/mcp_prompts.rs +++ b/src/mcp_prompts.rs @@ -216,6 +216,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 b795982c..f092ef1a 100644 --- a/src/mcp_server/handoff.rs +++ b/src/mcp_server/handoff.rs @@ -18,6 +18,10 @@ impl AgentflareMcp { decisions, files_touched, evidence, + last_commit, + completed, + remaining, + blockers, }: HandoffRequest, ) -> Result { if recipient.trim().is_empty() { @@ -32,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() { @@ -41,6 +51,23 @@ 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)?; @@ -54,34 +81,65 @@ impl AgentflareMcp { agentflare_backend::item::update(conn, id, input).map_err(map_backend_err)? } None => { - let state_id = agentflare_backend::state::list_by_project(conn, &project.id) - .map_err(map_backend_err)? - .into_iter() - .find(|s| s.is_default) - .ok_or_else(|| { - ErrorData::internal_error("project has no default state", None) - })? - .id; - let metadata = thread_id - .as_ref() - .map(|t| serde_json::json!({ "thread": t }).to_string()); - let input = agentflare_backend::item::CreateItem { - project_id: project.id.clone(), - state_id, - name: name.clone(), - description: description.clone().or_else(|| Some(content.clone())), - priority: None, - parent_id: None, - assignee_agent: Some(recipient.clone()), - sort_order: None, - external_source: None, - external_id: None, - metadata, - label_ids: vec![], - assignee_ids: vec![], - dependency_ids: vec![], - }; - agentflare_backend::item::create(conn, input).map_err(map_backend_err)? + // Reuse an existing open item already assigned to the + // recipient with a matching name or thread, instead of + // blindly creating a duplicate — the actual fix for the + // "handoff creates a duplicate item" bug on the + // reply/continuation path. Genuinely new work (no match) + // still auto-creates, unchanged. + let canonical_recipient = agent_registry::canonicalize(&recipient); + let reusable = agentflare_backend::item::list_by_assignee_agent( + conn, + &project.id, + &canonical_recipient, + ) + .map_err(map_backend_err)? + .into_iter() + .find(|i| { + i.name == name + || thread_id.as_ref().is_some_and(|t| { + serde_json::from_str::(&i.metadata) + .ok() + .and_then(|m| { + m.get("thread").and_then(|v| v.as_str()).map(str::to_string) + }) + .as_deref() + == Some(t.as_str()) + }) + }); + if let Some(item) = reusable { + item + } else { + let state_id = + agentflare_backend::state::list_by_project(conn, &project.id) + .map_err(map_backend_err)? + .into_iter() + .find(|s| s.is_default) + .ok_or_else(|| { + ErrorData::internal_error("project has no default state", None) + })? + .id; + let metadata = thread_id + .as_ref() + .map(|t| serde_json::json!({ "thread": t }).to_string()); + let input = agentflare_backend::item::CreateItem { + project_id: project.id.clone(), + state_id, + name: name.clone(), + description: description.clone().or_else(|| Some(content.clone())), + priority: None, + parent_id: None, + assignee_agent: Some(recipient.clone()), + sort_order: None, + external_source: None, + external_id: None, + metadata, + label_ids: vec![], + assignee_ids: vec![], + dependency_ids: vec![], + }; + agentflare_backend::item::create(conn, input).map_err(map_backend_err)? + } } }; @@ -91,13 +149,24 @@ impl AgentflareMcp { let filename = format!("{safe_stem}-{asset_id}.{ext}"); let entity_path = crate::asset_store::entity_path("item_attachment", &item.id, &filename); - let mut meta = serde_json::json!({ "sender": self.agent, "recipient": recipient }); + let mut meta = serde_json::json!({ + "sender": self.agent, + "recipient": recipient, + "completed": completed, + "remaining": remaining, + }); if let Some(t) = &thread_id { meta["thread_id"] = serde_json::json!(t); } if let Some(r) = &reply_to { meta["reply_to"] = serde_json::json!(r); } + if let Some(oid) = &last_commit { + meta["last_commit"] = serde_json::json!(oid); + } + if let Some(b) = blockers { + meta["blockers"] = serde_json::json!(b); + } if let Some(s) = summary { meta["session_summary"] = serde_json::json!(s); } @@ -196,4 +265,58 @@ impl AgentflareMcp { Ok(serde_json::to_string_pretty(&result).unwrap_or_default()) })? } + + /// Verified, not trusted: rejects a fabricated or typo'd continuation + /// 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(); + // `^{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(branch) = branch else { + return Ok(()); + }; + let branch_ref = format!("refs/heads/{branch}"); + let branch_exists = flare_git_core::shell::run_in_ok( + &repo_root, + &["show-ref", "--verify", "--quiet", &branch_ref], + ); + if branch_exists + && !flare_git_core::shell::run_in_ok( + &repo_root, + &["merge-base", "--is-ancestor", oid, branch], + ) + { + return Err(ErrorData::invalid_params( + format!( + "last_commit '{oid}' is not reachable from '{branch}' — verified, not trusted" + ), + None, + )); + } + Ok(()) + } } diff --git a/src/mcp_server/item.rs b/src/mcp_server/item.rs index 8c6f9bbc..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::claim::Acquire::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) { @@ -418,7 +419,7 @@ impl AgentflareMcp { _ => None, }; Ok(match outcome { - agentflare_backend::claim::Acquire::Acquired => { + agentflare_backend::item::ClaimOutcome::Acquired => { let mut resp = serde_json::json!({ "status": "acquired", "item_id": item_id, @@ -441,10 +442,19 @@ impl AgentflareMcp { } resp.to_string() } - agentflare_backend::claim::Acquire::Held { + agentflare_backend::item::ClaimOutcome::Held { owner: holder, age_secs, } => serde_json::json!({"status": "held", "item_id": item_id, "owner": holder, "age_secs": age_secs}).to_string(), + agentflare_backend::item::ClaimOutcome::BlockedByAssignee { assignee } => { + serde_json::json!({ + "status": "blocked", + "item_id": item_id, + "assignee": assignee, + "reason": format!("this item was handed off to '{assignee}' and hasn't been accepted yet — only {assignee} can claim it until they accept, decline, or the handoff is cancelled"), + }) + .to_string() + } }) } diff --git a/src/mcp_server/tests/artifact_tests.rs b/src/mcp_server/tests/artifact_tests.rs index 3e6d32d9..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(), @@ -440,6 +448,192 @@ fn handoff_with_item_id_assigns_existing_item_and_versions_the_asset() { }); } +#[test] +fn handoff_without_item_id_reuses_an_existing_open_item_with_matching_name() { + crate::paths::test_support::with_temp_home(|| { + let (_tmp, s) = handoff_harness(); + + let first: serde_json::Value = serde_json::from_str( + &s.handoff(Parameters(HandoffRequest { + recipient: "opencode".into(), + name: "Feature X".into(), + content: "v1 content".into(), + completed: "wrote v1".into(), + remaining: "get feedback".into(), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let item_id = first["item_id"].as_str().unwrap().to_string(); + + // Same recipient + name, no item_id — must reuse the existing item + // rather than creating a duplicate (the actual fix for the + // documented "handoff creates a duplicate item" bug). + let second: serde_json::Value = serde_json::from_str( + &s.handoff(Parameters(HandoffRequest { + recipient: "opencode".into(), + name: "Feature X".into(), + content: "v2 content".into(), + completed: "addressed feedback".into(), + remaining: "ship it".into(), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + assert_eq!(second["item_id"], item_id); + assert_eq!(second["asset_version"], 2); + assert_eq!(item_assets(&s, &item_id).as_array().unwrap().len(), 2); + }); +} + +#[test] +fn handoff_without_item_id_reuses_by_matching_thread_id() { + crate::paths::test_support::with_temp_home(|| { + let (_tmp, s) = handoff_harness(); + + let first: serde_json::Value = serde_json::from_str( + &s.handoff(Parameters(HandoffRequest { + recipient: "opencode".into(), + 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(), + ) + .unwrap(); + let item_id = first["item_id"].as_str().unwrap().to_string(); + + // Different name, same thread — still reused, not duplicated. + let second: serde_json::Value = serde_json::from_str( + &s.handoff(Parameters(HandoffRequest { + recipient: "opencode".into(), + 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(), + ) + .unwrap(); + assert_eq!(second["item_id"], item_id); + }); +} + +#[test] +fn handoff_without_item_id_still_creates_when_no_existing_item_matches() { + crate::paths::test_support::with_temp_home(|| { + let (_tmp, s) = handoff_harness(); + + let first: serde_json::Value = serde_json::from_str( + &s.handoff(Parameters(HandoffRequest { + recipient: "opencode".into(), + name: "Feature X".into(), + content: "v1 content".into(), + completed: "wrote v1".into(), + remaining: "get feedback".into(), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + + // Different name, no thread match — genuinely new work still + // auto-creates. + let second: serde_json::Value = serde_json::from_str( + &s.handoff(Parameters(HandoffRequest { + recipient: "opencode".into(), + name: "Unrelated Feature Y".into(), + content: "v1 content".into(), + completed: "wrote v1".into(), + remaining: "get feedback".into(), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + assert_ne!(first["item_id"], second["item_id"]); + }); +} + +#[test] +fn handoff_stores_structured_payload_in_asset_metadata() { + crate::paths::test_support::with_temp_home(|| { + let (_tmp, s) = handoff_harness(); + let result: serde_json::Value = serde_json::from_str( + &s.handoff(Parameters(HandoffRequest { + recipient: "opencode".into(), + name: "review-packet".into(), + content: "please review".into(), + completed: "wrote the parser".into(), + remaining: "wire up the CLI".into(), + blockers: Some(vec!["needs review of #42".into()]), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let item_id = result["item_id"].as_str().unwrap().to_string(); + let assets = item_assets(&s, &item_id); + let meta = &assets[0]["metadata"]; + assert_eq!(meta["completed"], "wrote the parser"); + assert_eq!(meta["remaining"], "wire up the CLI"); + assert_eq!(meta["blockers"][0], "needs review of #42"); + }); +} + +#[test] +fn handoff_rejects_a_fabricated_last_commit_oid() { + crate::paths::test_support::with_temp_home(|| { + let (_tmp, s) = handoff_harness(); + let err = s + .handoff(Parameters(HandoffRequest { + 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() + })) + .unwrap_err(); + assert_eq!(err.code, rmcp::model::ErrorCode::INVALID_PARAMS); + assert!(err.message.contains("does not exist"), "{}", err.message); + }); +} + +#[test] +fn handoff_accepts_an_existing_last_commit_oid() { + crate::paths::test_support::with_temp_home(|| { + let (_tmp, s) = handoff_harness(); + let repo_root = s.worktree_repo_root(); + let head = flare_git_core::shell::run_in(&repo_root, &["rev-parse", "HEAD"]) + .expect("this test must run inside a git checkout"); + let result: serde_json::Value = serde_json::from_str( + &s.handoff(Parameters(HandoffRequest { + 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() + })) + .unwrap(), + ) + .unwrap(); + let item_id = result["item_id"].as_str().unwrap().to_string(); + let assets = item_assets(&s, &item_id); + assert_eq!(assets[0]["metadata"]["last_commit"], head); + }); +} + #[test] fn artifact_diff_tool_returns_unified_diff() { let tmp = tempfile::tempdir().unwrap(); diff --git a/src/mcp_server/types.rs b/src/mcp_server/types.rs index f8d2fe98..5a356cfe 100644 --- a/src/mcp_server/types.rs +++ b/src/mcp_server/types.rs @@ -230,6 +230,18 @@ pub(crate) struct HandoffRequest { #[schemars(description = "Evidence array [{kind, action, detail}] — session snapshot.")] #[serde(default)] pub(crate) evidence: Option>, + #[schemars( + description = "Continuation commit OID the recipient should build on. Verified before being accepted: must exist in the repo, and (when the item's own task/ branch already exists) be reachable from it. A fabricated or unreachable OID is rejected, not silently trusted." + )] + #[serde(default)] + 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.")] + pub(crate) remaining: String, + #[schemars(description = "Known blockers, if any.")] + #[serde(default)] + pub(crate) blockers: Option>, } #[derive(Debug, Default, Deserialize, schemars::JsonSchema)] diff --git a/src/paths.rs b/src/paths.rs index a024219c..b3649181 100644 --- a/src/paths.rs +++ b/src/paths.rs @@ -217,6 +217,15 @@ mod tests { #[test] fn with_temp_home_clears_the_override_env_var_after_returning() { with_temp_home(|| {}); + // Read the var only while holding the same lock with_temp_home uses to + // serialize all env mutation in this binary. Without it, this check + // races a concurrent thread's own (correctly scoped) with_temp_home + // call: that thread may be transiently holding the var set for its own + // closure at the exact moment this assertion reads it, which isn't a + // leak on our part — it's a var we were never entitled to observe. + let _guard = agent_registry::detect::PATH_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); assert!( std::env::var("AGENTFLARE_HOME_OVERRIDE").is_err(), "AGENTFLARE_HOME_OVERRIDE must not remain set once with_temp_home returns"