diff --git a/src/mcp_prompts.rs b/src/mcp_prompts.rs index 2d9467d5..0f90a900 100644 --- a/src/mcp_prompts.rs +++ b/src/mcp_prompts.rs @@ -188,16 +188,14 @@ fn get_handoff_command(request: &GetPromptRequestParams, agent: Option<&str>) -> .unwrap_or("") .trim() .to_string(); - - if command.is_empty() { - return assistant_text(format!( - "Handoff — agent-to-agent work exchange via items and assets. Pass as this command's argument:\n\ - — hand the relevant work product to that agent (e.g. `codex review the API design above`)\n\ - inbox [me] — list this project's tasks assigned to (or unclaimed for) an agent (default: {me})\n\ - thread — show a handoff thread's items in order\n\ - Work products only — facts and decisions belong in memory (memory_remember), not items.", - )); - } + // Bare `/handoff` checks your own inbox rather than printing a usage + // card — that's the common case, and the grammar below already covers + // `inbox` alongside the other subcommands. + let command = if command.is_empty() { + "inbox".to_string() + } else { + command + }; assistant_text(format!( "Handoff command: `{command}`\n\n\ @@ -347,10 +345,25 @@ mod tests { } #[test] - fn bare_handoff_usage_names_agent_identity() { + fn bare_handoff_defaults_to_inbox_for_the_calling_agent() { let result = get_prompt(&GetPromptRequestParams::new("handoff"), Some("opencode")).unwrap(); let text = format!("{:?}", result.messages[0].content); - assert!(text.contains("default: opencode"), "{text}"); + assert!(text.contains("Handoff command: `inbox`"), "{text}"); + assert!(text.contains("identity, opencode"), "{text}"); + } + + #[test] + fn bare_handoff_command_matches_explicit_inbox_command() { + use rmcp::model::JsonObject; + let bare = get_prompt(&GetPromptRequestParams::new("handoff"), Some("codex")).unwrap(); + let mut args = JsonObject::new(); + args.insert("command".to_string(), serde_json::json!("inbox")); + let params = GetPromptRequestParams::new("handoff").with_arguments(args); + let explicit = get_prompt(¶ms, Some("codex")).unwrap(); + assert_eq!( + format!("{:?}", bare.messages[0].content), + format!("{:?}", explicit.messages[0].content), + ); } #[test] diff --git a/src/mcp_server.rs b/src/mcp_server.rs index dd43d97d..3531090c 100644 --- a/src/mcp_server.rs +++ b/src/mcp_server.rs @@ -621,7 +621,7 @@ fn base64_encode(bytes: &[u8]) -> String { #[derive(Debug, Default, Deserialize, schemars::JsonSchema)] struct ItemRequest { #[schemars( - description = "Action: create|get|list|update|update_state|delete|claim|heartbeat|release|done|add_label|remove_label" + description = "Action: create|get|list|update|update_state|delete|claim|heartbeat|release|done|cancel|add_label|remove_label" )] action: String, #[schemars( @@ -646,7 +646,9 @@ struct ItemRequest { #[schemars(description = "Parent item ID, for sub-items (create)")] #[serde(default)] parent_id: Option, - #[schemars(description = "Agent ID to assign (create, update)")] + #[schemars( + description = "Agent ID to assign (create, update), or to filter by (list — matches items assigned to this agent plus unassigned ones, sorted open+assigned-to-you first)" + )] #[serde(default)] assignee_agent: Option, #[schemars(description = "Domain-specific fields as a JSON object (create)")] @@ -666,6 +668,28 @@ struct ItemRequest { )] #[serde(default)] state_group: Option, + #[schemars(description = "Max items to return (list); omit for no limit")] + #[serde(default)] + limit: Option, + #[schemars(description = "Items to skip before applying limit (list); default 0")] + #[serde(default)] + offset: Option, +} + +/// Lean per-item projection for `item(list)` — the raw 19-field `Item` (full +/// description/metadata/timestamps) is what `get` returns; `list` only needs +/// enough to triage, and resolves the opaque `state_id` into a readable name. +#[derive(Debug, serde::Serialize)] +struct ItemSummary { + id: String, + name: String, + state: String, + state_group: String, + priority: String, + assignee_agent: Option, + parent_id: Option, + sequence_id: i64, + updated_at: i64, } #[derive(Debug, Default, Deserialize, schemars::JsonSchema)] @@ -2388,22 +2412,78 @@ impl AgentflareMcp { Ok(serde_json::to_string_pretty(&item).unwrap_or_default()) })? } - "list" => self.with_backend_db(|conn| { - let project = self.resolve_project(conn)?; - let mut items = agentflare_backend::item::list_by_project(conn, &project.id) - .map_err(map_backend_err)?; - if let Some(group) = req.state_group { - let matching: std::collections::HashSet = - agentflare_backend::state::list_by_project(conn, &project.id) - .map_err(map_backend_err)? - .into_iter() - .filter(|s| s.group_name == group) - .map(|s| s.id) - .collect(); - items.retain(|i| matching.contains(&i.state_id)); + "list" => { + if req.limit.is_some_and(|l| l < 0) || req.offset.is_some_and(|o| o < 0) { + return Err(ErrorData::invalid_params( + "limit and offset must be non-negative", + None, + )); } - Ok(serde_json::to_string_pretty(&items).unwrap_or_default()) - })?, + self.with_backend_db(|conn| { + let project = self.resolve_project(conn)?; + let mut items = agentflare_backend::item::list_by_project(conn, &project.id) + .map_err(map_backend_err)?; + let states = agentflare_backend::state::list_by_project(conn, &project.id) + .map_err(map_backend_err)?; + let state_by_id: std::collections::HashMap< + &str, + &agentflare_backend::state::State, + > = states.iter().map(|s| (s.id.as_str(), s)).collect(); + + if let Some(group) = &req.state_group { + items.retain(|i| { + state_by_id + .get(i.state_id.as_str()) + .map(|s| &s.group_name == group) + .unwrap_or(false) + }); + } + if let Some(agent) = &req.assignee_agent { + items.retain(|i| { + i.assignee_agent.as_deref() == Some(agent.as_str()) + || i.assignee_agent.is_none() + }); + items.sort_by_key(|i| { + let is_open = state_by_id + .get(i.state_id.as_str()) + .map(|s| { + !matches!(s.group_name.as_str(), "completed" | "cancelled") + }) + .unwrap_or(true); + let is_mine = i.assignee_agent.as_deref() == Some(agent.as_str()); + (!is_open, !is_mine) + }); + } + + let offset = req.offset.unwrap_or(0) as usize; + let items = items.into_iter().skip(offset); + let items: Vec<_> = match req.limit { + Some(limit) => items.take(limit as usize).collect(), + None => items.collect(), + }; + + let summaries: Vec = items + .into_iter() + .map(|i| { + let state = state_by_id.get(i.state_id.as_str()); + ItemSummary { + id: i.id, + name: i.name, + state: state.map(|s| s.name.clone()).unwrap_or_default(), + state_group: state + .map(|s| s.group_name.clone()) + .unwrap_or_default(), + priority: i.priority, + assignee_agent: i.assignee_agent, + parent_id: i.parent_id, + sequence_id: i.sequence_id, + updated_at: i.updated_at, + } + }) + .collect(); + Ok(serde_json::to_string_pretty(&summaries).unwrap_or_default()) + })? + } "update" => { let id = req .id @@ -2555,6 +2635,31 @@ impl AgentflareMcp { Ok(serde_json::json!({"done": done, "item_id": item_id}).to_string()) })? } + "cancel" => { + let item_id = req + .id + .ok_or_else(|| ErrorData::invalid_params("id is required for cancel", None))?; + if item_id.trim().is_empty() { + return Err(ErrorData::invalid_params("id is required", None)); + } + let owner = crate::claims::owner_id(); + self.with_backend_db(|conn| { + let project = self.resolve_project(conn)?; + let cancelled = + agentflare_backend::state::first_in_group(conn, &project.id, "cancelled") + .map_err(map_backend_err)?; + let item = + agentflare_backend::item::update_state(conn, &item_id, &cancelled.id) + .map_err(map_backend_err)?; + // Best-effort: release this caller's own claim lease on + // the item, if any, so a cancelled item isn't stuck + // "held" until the TTL expires (mirrors `done`'s + // claim_done release). No-ops if someone else holds it + // or nobody does — `release` is owner-scoped. + let _ = agentflare_backend::claim::release(conn, &item_id, &owner); + Ok(serde_json::to_string_pretty(&item).unwrap_or_default()) + })? + } "add_label" => { let item_id = req.id.ok_or_else(|| { ErrorData::invalid_params("id is required for add_label", None) @@ -2595,7 +2700,7 @@ impl AgentflareMcp { } other => Err(ErrorData::invalid_params( format!( - "unknown item action: '{other}' — expected create|get|list|update|update_state|delete|claim|heartbeat|release|done|add_label|remove_label" + "unknown item action: '{other}' — expected create|get|list|update|update_state|delete|claim|heartbeat|release|done|cancel|add_label|remove_label" ), None, )), @@ -2603,7 +2708,7 @@ impl AgentflareMcp { } #[tool( - description = "Manage work items in the repo's linked project. Single consolidated tool with `action` field (create|get|list|update|update_state|delete|claim|heartbeat|release|done|add_label|remove_label). See each field's description for when it's required." + description = "Manage work items in the repo's linked project. Single consolidated tool with `action` field (create|get|list|update|update_state|delete|claim|heartbeat|release|done|cancel|add_label|remove_label). See each field's description for when it's required." )] fn item(&self, Parameters(req): Parameters) -> Result { self.item_inner(req) @@ -4239,6 +4344,238 @@ mod tests { assert!(updated["completed_at"].is_null()); } + #[test] + fn item_cancel_moves_to_cancelled_state() { + let (tmp, s) = harness(); + let created: serde_json::Value = + serde_json::from_str(&s.item(Parameters(empty_item_create("Test"))).unwrap()).unwrap(); + let item_id = created["id"].as_str().unwrap().to_string(); + let project_id = created["project_id"].as_str().unwrap().to_string(); + + let cancelled: serde_json::Value = serde_json::from_str( + &s.item(Parameters(ItemRequest { + action: "cancel".into(), + id: Some(item_id), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let state_id = cancelled["state_id"].as_str().unwrap().to_string(); + + let conn = backend_conn(&tmp); + let group = agentflare_backend::state::list_by_project(&conn, &project_id) + .unwrap() + .into_iter() + .find(|st| st.id == state_id) + .unwrap() + .group_name; + assert_eq!(group, "cancelled"); + } + + #[test] + fn item_cancel_releases_the_callers_own_claim() { + // `claim` always resolves a worktree_repo_root and may run real `git + // worktree` commands against it — every test that calls `claim` must + // override this to an isolated throwaway repo, never the repo + // `cargo test` itself is running in. Same scaffolding as + // `item_claim_response_includes_worktree_path`. + let tmp = tempfile::tempdir().unwrap(); + let repo_dir = tempfile::tempdir().unwrap(); + let repo_root = repo_dir.path().to_path_buf(); + let run_git = |args: &[&str]| { + std::process::Command::new("git") + .args(args) + .current_dir(&repo_root) + .output() + .unwrap() + }; + run_git(&["init", "-b", "master"]); + run_git(&["config", "user.email", "test@test.com"]); + run_git(&["config", "user.name", "Test"]); + run_git(&["commit", "--allow-empty", "-m", "initial"]); + + let s = AgentflareMcp { + backend_db_override: Some(tmp.path().join("backend.db")), + backend_project_link_override: Some(tmp.path().join("project.json")), + worktree_repo_root_override: Some(repo_root), + ..Default::default() + }; + + let created: serde_json::Value = + serde_json::from_str(&s.item(Parameters(empty_item_create("Test"))).unwrap()).unwrap(); + let item_id = created["id"].as_str().unwrap().to_string(); + + s.item(Parameters(ItemRequest { + action: "claim".into(), + id: Some(item_id.clone()), + ..Default::default() + })) + .unwrap(); + + s.item(Parameters(ItemRequest { + action: "cancel".into(), + id: Some(item_id.clone()), + ..Default::default() + })) + .unwrap(); + + // The claim must be released — re-claiming should succeed + // immediately instead of coming back "held". + let reclaimed: serde_json::Value = serde_json::from_str( + &s.item(Parameters(ItemRequest { + action: "claim".into(), + id: Some(item_id), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + assert_eq!(reclaimed["status"], "acquired"); + } + + #[test] + fn item_list_rejects_negative_limit_and_offset() { + let (_tmp, s) = harness(); + let err = s + .item(Parameters(ItemRequest { + action: "list".into(), + limit: Some(-1), + ..Default::default() + })) + .unwrap_err(); + assert_eq!(err.code, rmcp::model::ErrorCode::INVALID_PARAMS); + + let err = s + .item(Parameters(ItemRequest { + action: "list".into(), + offset: Some(-1), + ..Default::default() + })) + .unwrap_err(); + assert_eq!(err.code, rmcp::model::ErrorCode::INVALID_PARAMS); + } + + #[test] + fn item_list_filters_by_assignee_or_unassigned_and_sorts_open_first() { + let (tmp, s) = harness(); + let mine_open: serde_json::Value = + serde_json::from_str(&s.item(Parameters(empty_item_create("Mine open"))).unwrap()) + .unwrap(); + let project_id = mine_open["project_id"].as_str().unwrap().to_string(); + s.item(Parameters(ItemRequest { + action: "update".into(), + id: Some(mine_open["id"].as_str().unwrap().to_string()), + assignee_agent: Some("me".into()), + ..Default::default() + })) + .unwrap(); + + serde_json::from_str::( + &s.item(Parameters(empty_item_create("Unassigned"))).unwrap(), + ) + .unwrap(); + + let others: serde_json::Value = + serde_json::from_str(&s.item(Parameters(empty_item_create("Others"))).unwrap()) + .unwrap(); + s.item(Parameters(ItemRequest { + action: "update".into(), + id: Some(others["id"].as_str().unwrap().to_string()), + assignee_agent: Some("someone-else".into()), + ..Default::default() + })) + .unwrap(); + + let mine_done: serde_json::Value = + serde_json::from_str(&s.item(Parameters(empty_item_create("Mine done"))).unwrap()) + .unwrap(); + s.item(Parameters(ItemRequest { + action: "update".into(), + id: Some(mine_done["id"].as_str().unwrap().to_string()), + assignee_agent: Some("me".into()), + ..Default::default() + })) + .unwrap(); + let done_state_id = { + let conn = backend_conn(&tmp); + agentflare_backend::state::list_by_project(&conn, &project_id) + .unwrap() + .into_iter() + .find(|st| st.group_name == "completed") + .unwrap() + .id + }; + s.item(Parameters(ItemRequest { + action: "update_state".into(), + id: Some(mine_done["id"].as_str().unwrap().to_string()), + state_id: Some(done_state_id), + ..Default::default() + })) + .unwrap(); + + let listed: serde_json::Value = serde_json::from_str( + &s.item(Parameters(ItemRequest { + action: "list".into(), + assignee_agent: Some("me".into()), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let names: Vec<&str> = listed + .as_array() + .unwrap() + .iter() + .map(|i| i["name"].as_str().unwrap()) + .collect(); + assert_eq!(names, vec!["Mine open", "Unassigned", "Mine done"]); + } + + #[test] + fn item_list_respects_limit_and_offset() { + let (_tmp, s) = harness(); + for name in ["A", "B", "C"] { + s.item(Parameters(empty_item_create(name))).unwrap(); + } + let listed: serde_json::Value = serde_json::from_str( + &s.item(Parameters(ItemRequest { + action: "list".into(), + limit: Some(1), + offset: Some(1), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let names: Vec<&str> = listed + .as_array() + .unwrap() + .iter() + .map(|i| i["name"].as_str().unwrap()) + .collect(); + assert_eq!(names, vec!["B"]); + } + + #[test] + fn item_list_returns_lean_projection_with_readable_state() { + let (_tmp, s) = harness(); + s.item(Parameters(empty_item_create("Test"))).unwrap(); + let listed: serde_json::Value = serde_json::from_str( + &s.item(Parameters(ItemRequest { + action: "list".into(), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let first = &listed.as_array().unwrap()[0]; + assert_eq!(first["state"], "Backlog"); + assert_eq!(first["state_group"], "backlog"); + assert!(first.get("description").is_none()); + assert!(first.get("metadata").is_none()); + } + #[test] fn resolve_workspace_id_creates_once_and_reuses() { let (tmp, _s) = harness();