diff --git a/.gitignore b/.gitignore index f0273a85..05074126 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,4 @@ /install_onpush.ps1 /run_onpush.bat /serve_docs.py +.worktrees/ diff --git a/crates/agentflare-backend/src/claim.rs b/crates/agentflare-backend/src/claim.rs index 99242ca2..d7aa2301 100644 --- a/crates/agentflare-backend/src/claim.rs +++ b/crates/agentflare-backend/src/claim.rs @@ -35,3 +35,19 @@ pub fn release(conn: &Connection, item_id: &str, owner: &str) -> rusqlite::Resul pub fn done(conn: &Connection, item_id: &str, owner: &str, now: i64) -> rusqlite::Result { LEDGER.done(conn, &[item_id], owner, now) } + +/// Returns true if there is an active (live, non-stale) claim on this item +/// whose owner differs from `owner`. Used by the comment edit/delete gates +/// to prevent modifying a comment when another agent has started work. +pub fn has_active_claim_by_other( + conn: &Connection, + item_id: &str, + owner: &str, + now: i64, + ttl_secs: i64, +) -> rusqlite::Result { + let claims = LEDGER.list(conn, false, now, ttl_secs)?; + Ok(claims + .iter() + .any(|c| c.key == [item_id] && c.owner != owner)) +} diff --git a/crates/agentflare-backend/src/comment.rs b/crates/agentflare-backend/src/comment.rs new file mode 100644 index 00000000..f7473d83 --- /dev/null +++ b/crates/agentflare-backend/src/comment.rs @@ -0,0 +1,116 @@ +use rusqlite::{Connection, OptionalExtension}; +use serde::{Deserialize, Serialize}; + +use crate::error::Result; + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ItemComment { + pub id: String, + pub item_id: String, + pub author_agent: String, + pub body: String, + pub created_at: i64, + pub updated_at: i64, +} + +fn row_to_comment(row: &rusqlite::Row) -> rusqlite::Result { + Ok(ItemComment { + id: row.get(0)?, + item_id: row.get(1)?, + author_agent: row.get(2)?, + body: row.get(3)?, + created_at: row.get(4)?, + updated_at: row.get(5)?, + }) +} + +fn now() -> i64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs() as i64) + .unwrap_or(0) +} + +/// Create a comment on an item. `author_agent` is the identity of the caller. +pub fn create( + conn: &Connection, + item_id: &str, + author_agent: &str, + body: &str, +) -> Result { + let id = uuid::Uuid::now_v7().to_string(); + let ts = now(); + conn.execute( + "INSERT INTO item_comments (id, item_id, author_agent, body, created_at, updated_at) + VALUES (?1, ?2, ?3, ?4, ?5, ?6)", + rusqlite::params![id, item_id, author_agent, body, ts, ts], + )?; + get(conn, &id) +} + +/// Get a single comment by id. +pub fn get(conn: &Connection, id: &str) -> Result { + conn.query_row( + "SELECT id, item_id, author_agent, body, created_at, updated_at + FROM item_comments WHERE id = ?1", + rusqlite::params![id], + row_to_comment, + ) + .map_err(|e| match e { + rusqlite::Error::QueryReturnedNoRows => crate::error::Error::NotFound(id.to_string()), + other => other.into(), + }) +} + +/// Update the body of a comment. Returns the updated comment. +pub fn update(conn: &Connection, id: &str, body: &str) -> Result { + let ts = now(); + let changed = conn.execute( + "UPDATE item_comments SET body = ?2, updated_at = ?3 WHERE id = ?1", + rusqlite::params![id, body, ts], + )?; + if changed == 0 { + return Err(crate::error::Error::NotFound(id.to_string())); + } + get(conn, id) +} + +/// Delete a comment by id. +pub fn delete(conn: &Connection, id: &str) -> Result<()> { + let changed = conn.execute( + "DELETE FROM item_comments WHERE id = ?1", + rusqlite::params![id], + )?; + if changed == 0 { + return Err(crate::error::Error::NotFound(id.to_string())); + } + Ok(()) +} + +/// List all comments for an item, oldest first. +pub fn list_by_item(conn: &Connection, item_id: &str) -> Result> { + let mut stmt = conn.prepare( + "SELECT id, item_id, author_agent, body, created_at, updated_at + FROM item_comments WHERE item_id = ?1 ORDER BY created_at ASC, id ASC", + )?; + let rows = stmt.query_map(rusqlite::params![item_id], row_to_comment)?; + Ok(rows.collect::>()?) +} + +/// Check if this comment is the latest (most recent) on its item. +pub fn is_latest(conn: &Connection, comment: &ItemComment) -> Result { + // `created_at` is second-resolution, so two comments posted in the same + // second tie on MAX(created_at) — comparing timestamps alone would treat + // both as "latest". Break ties with `id` (UUIDv7, time-ordered), which + // reflects true insertion order even within one second. + let latest_id: Option = conn + .query_row( + "SELECT id FROM item_comments WHERE item_id = ?1 + ORDER BY created_at DESC, id DESC LIMIT 1", + rusqlite::params![comment.item_id], + |row| row.get(0), + ) + .optional() + .map_err(crate::error::Error::Database)?; + Ok(latest_id.is_none_or(|id| id == comment.id)) +} diff --git a/crates/agentflare-backend/src/db.rs b/crates/agentflare-backend/src/db.rs index 86b39190..bcd36893 100644 --- a/crates/agentflare-backend/src/db.rs +++ b/crates/agentflare-backend/src/db.rs @@ -12,6 +12,7 @@ const MIGRATION_LIST: &[M<'static>] = &[ M::up(include_str!("migrations/0001_initial.sql")), M::up(include_str!("migrations/0002_schema_constraints.sql")), M::up(include_str!("migrations/0003_asset_versioning.sql")), + M::up(include_str!("migrations/0004_item_comments.sql")), ]; const MIGRATIONS: Migrations = Migrations::from_slice(MIGRATION_LIST); diff --git a/crates/agentflare-backend/src/lib.rs b/crates/agentflare-backend/src/lib.rs index db57ebac..7b3dd9b7 100644 --- a/crates/agentflare-backend/src/lib.rs +++ b/crates/agentflare-backend/src/lib.rs @@ -1,5 +1,6 @@ pub mod asset; pub mod claim; +pub mod comment; pub mod db; pub mod error; pub mod events; diff --git a/crates/agentflare-backend/src/migrations/0004_item_comments.sql b/crates/agentflare-backend/src/migrations/0004_item_comments.sql new file mode 100644 index 00000000..f591916a --- /dev/null +++ b/crates/agentflare-backend/src/migrations/0004_item_comments.sql @@ -0,0 +1,9 @@ +CREATE TABLE IF NOT EXISTS item_comments ( + id TEXT PRIMARY KEY, + item_id TEXT NOT NULL REFERENCES items(id), + author_agent TEXT NOT NULL, + body TEXT NOT NULL, + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL +); +CREATE INDEX IF NOT EXISTS idx_item_comments_item ON item_comments(item_id, created_at); diff --git a/src/claims.rs b/src/claims.rs index cf3a9749..c1201ad8 100644 --- a/src/claims.rs +++ b/src/claims.rs @@ -172,6 +172,15 @@ pub fn owner_id() -> String { format!("{agent}:{instance}") } +/// Strips the `:` suffix off an owner id, leaving the stable agent +/// identity. Unlike claim ownership (deliberately instance-scoped, see +/// `owner_id` above), authorship of a comment should survive across +/// sessions — an agent restarting shouldn't lose the ability to edit its own +/// words just because its instance suffix changed. +pub fn agent_of(owner_id: &str) -> &str { + owner_id.split(':').next().unwrap_or(owner_id) +} + pub fn ttl_secs() -> i64 { std::env::var("AGENTFLARE_CLAIM_TTL_SECS") .ok() diff --git a/src/main.rs b/src/main.rs index 1387e37d..d7335068 100644 --- a/src/main.rs +++ b/src/main.rs @@ -37,6 +37,7 @@ mod state; mod tool_install; mod uninstall; mod update; +mod worktree; use clap::Parser; diff --git a/src/mcp_server.rs b/src/mcp_server.rs index 8753813b..f821caff 100644 --- a/src/mcp_server.rs +++ b/src/mcp_server.rs @@ -484,6 +484,10 @@ pub struct AgentflareMcp { /// to `git`/reads cwd, both process-global and unsafe to fake by /// mutating cwd across parallel test threads. backend_repo_key_override: Option, + /// Tests inject a temp repo root here so the worktree-on-claim feature + /// never runs real git worktree/branch operations against this actual + /// repository (worktree add, force-remove, branch -D). + worktree_repo_root_override: Option, } /// All local artifact backends (flared, another session, or our own @@ -651,6 +655,21 @@ struct ItemRequest { state_group: Option, } +#[derive(Debug, Default, Deserialize, schemars::JsonSchema)] +struct CommentRequest { + #[schemars(description = "Action: create|edit|delete|list")] + action: String, + #[schemars(description = "Item ID to comment on (required for create, list)")] + #[serde(default)] + item_id: Option, + #[schemars(description = "Comment ID (required for edit, delete)")] + #[serde(default)] + id: Option, + #[schemars(description = "Comment body text (required for create, edit)")] + #[serde(default)] + body: Option, +} + #[derive(Debug, Default, Deserialize, schemars::JsonSchema)] struct LabelRequest { #[schemars(description = "Action: create")] @@ -1249,6 +1268,15 @@ impl AgentflareMcp { Self::find_root_from(&cwd, &crate::paths::home()) } + /// `repo_root()`, but honoring `worktree_repo_root_override` — used only + /// by the worktree-on-claim feature so tests never run real `git + /// worktree`/branch operations against this actual repository. + fn worktree_repo_root(&self) -> std::path::PathBuf { + self.worktree_repo_root_override + .clone() + .unwrap_or_else(Self::repo_root) + } + /// Pure walk-up so the non-git fallback path is unit-testable without /// touching process-global state: neither this process's real cwd nor /// `crate::paths::home()` (which itself reads the `AGENTFLARE_HOME_OVERRIDE` @@ -2430,18 +2458,50 @@ impl AgentflareMcp { let owner = crate::claims::owner_id(); let now = crate::claims::now(); let ttl = backend_claim_ttl_secs(); - self.with_backend_db(|conn| { + let repo_root = self.worktree_repo_root(); + // Only resolve the item + target branch (DB reads) under the + // backend lock; `git worktree add` below is a blocking + // filesystem+subprocess operation that has no business + // running while the shared DB mutex is held. + let (outcome, item, target_branch) = self.with_backend_db(|conn| { let outcome = agentflare_backend::item::claim(conn, &item_id, &owner, now, ttl) .map_err(map_backend_err)?; - Ok(match outcome { - agentflare_backend::claim::Acquire::Acquired => { - serde_json::json!({"status": "acquired", "item_id": item_id, "owner": owner}) - } - agentflare_backend::claim::Acquire::Held { owner: holder, age_secs } => { - serde_json::json!({"status": "held", "item_id": item_id, "owner": holder, "age_secs": age_secs}) + 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) + }; + Ok((outcome, item, target_branch)) + })??; + let worktree_path = match (&item, &target_branch) { + (Some(item), Some(target)) => { + crate::worktree::create_worktree(item, &repo_root, target) + } + _ => None, + }; + Ok(match outcome { + agentflare_backend::claim::Acquire::Acquired => { + let mut resp = serde_json::json!({ + "status": "acquired", + "item_id": item_id, + "owner": owner, + }); + if let Some(ref path) = worktree_path { + resp["worktree_path"] = + serde_json::Value::String(path.to_string_lossy().to_string()); } - }.to_string()) - })? + resp.to_string() + } + agentflare_backend::claim::Acquire::Held { + owner: holder, + age_secs, + } => serde_json::json!({"status": "held", "item_id": item_id, "owner": holder, "age_secs": age_secs}).to_string(), + }) } "heartbeat" => { let item_id = req.id.ok_or_else(|| { @@ -2535,12 +2595,174 @@ impl AgentflareMcp { } #[tool( - description = "Create, get, list, update, update_state, delete, claim, heartbeat, release, done, add_label, or remove_label work items in the repo's linked project. The `action` field selects the operation; 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|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) } + #[tool( + description = "Create, edit, delete, or list threaded comments on an item. Single consolidated tool with `action` field (create|edit|delete|list). Only the author of a comment may edit/delete it, only the latest comment on an item is editable/deletable, and edit/delete are blocked while another agent holds an active claim on the item." + )] + fn comment(&self, Parameters(req): Parameters) -> Result { + match req.action.as_str() { + "create" => { + let item_id = req.item_id.ok_or_else(|| { + ErrorData::invalid_params("item_id is required for create", None) + })?; + let body = req.body.ok_or_else(|| { + ErrorData::invalid_params("body is required for create", None) + })?; + if item_id.trim().is_empty() || body.trim().is_empty() { + return Err(ErrorData::invalid_params( + "item_id and body are required", + None, + )); + } + let author = crate::claims::owner_id(); + self.with_backend_db(|conn| { + let comment = + agentflare_backend::comment::create(conn, &item_id, &author, &body) + .map_err(map_backend_err)?; + Ok(serde_json::to_string_pretty(&comment).unwrap_or_default()) + })? + } + "edit" => { + let comment_id = req + .id + .ok_or_else(|| ErrorData::invalid_params("id is required for edit", None))?; + let body = req + .body + .ok_or_else(|| ErrorData::invalid_params("body is required for edit", None))?; + if comment_id.trim().is_empty() || body.trim().is_empty() { + return Err(ErrorData::invalid_params("id and body are required", None)); + } + let owner = crate::claims::owner_id(); + let now = crate::claims::now(); + let ttl = backend_claim_ttl_secs(); + self.with_backend_db(|conn| { + // The author/latest/claim checks and the write must be one + // transaction — otherwise a comment landing between the + // is_latest check and the write (routine under concurrent + // multi-agent access) can silently violate the + // "only the latest comment is editable" invariant. + let tx = conn + .unchecked_transaction() + .map_err(|e| ErrorData::internal_error(e.to_string(), None))?; + let comment = agentflare_backend::comment::get(&tx, &comment_id) + .map_err(map_backend_err)?; + if crate::claims::agent_of(&comment.author_agent) + != crate::claims::agent_of(&owner) + { + return Err(ErrorData::invalid_params( + "can only edit your own comments", + None, + )); + } + if !agentflare_backend::comment::is_latest(&tx, &comment) + .map_err(map_backend_err)? + { + return Err(ErrorData::invalid_params( + "comment is not the latest on this item — cannot edit", + None, + )); + } + if agentflare_backend::claim::has_active_claim_by_other( + &tx, + &comment.item_id, + &owner, + now, + ttl, + ) + .map_err(|e| ErrorData::internal_error(e.to_string(), None))? + { + return Err(ErrorData::invalid_params( + "another agent has started work on this item — cannot edit", + None, + )); + } + let updated = agentflare_backend::comment::update(&tx, &comment_id, &body) + .map_err(map_backend_err)?; + tx.commit() + .map_err(|e| ErrorData::internal_error(e.to_string(), None))?; + Ok(serde_json::to_string_pretty(&updated).unwrap_or_default()) + })? + } + "delete" => { + let comment_id = req + .id + .ok_or_else(|| ErrorData::invalid_params("id is required for delete", None))?; + if comment_id.trim().is_empty() { + return Err(ErrorData::invalid_params("id is required", None)); + } + let owner = crate::claims::owner_id(); + let now = crate::claims::now(); + let ttl = backend_claim_ttl_secs(); + self.with_backend_db(|conn| { + // See "edit" above: checks + write must be one transaction + // to close the same TOCTOU window. + let tx = conn + .unchecked_transaction() + .map_err(|e| ErrorData::internal_error(e.to_string(), None))?; + let comment = agentflare_backend::comment::get(&tx, &comment_id) + .map_err(map_backend_err)?; + if crate::claims::agent_of(&comment.author_agent) + != crate::claims::agent_of(&owner) + { + return Err(ErrorData::invalid_params( + "can only delete your own comments", + None, + )); + } + if !agentflare_backend::comment::is_latest(&tx, &comment) + .map_err(map_backend_err)? + { + return Err(ErrorData::invalid_params( + "comment is not the latest on this item — cannot delete", + None, + )); + } + if agentflare_backend::claim::has_active_claim_by_other( + &tx, + &comment.item_id, + &owner, + now, + ttl, + ) + .map_err(|e| ErrorData::internal_error(e.to_string(), None))? + { + return Err(ErrorData::invalid_params( + "another agent has started work on this item — cannot delete", + None, + )); + } + agentflare_backend::comment::delete(&tx, &comment_id) + .map_err(map_backend_err)?; + tx.commit() + .map_err(|e| ErrorData::internal_error(e.to_string(), None))?; + Ok(serde_json::json!({"deleted": true, "id": comment_id}).to_string()) + })? + } + "list" => { + let item_id = req.item_id.ok_or_else(|| { + ErrorData::invalid_params("item_id is required for list", None) + })?; + if item_id.trim().is_empty() { + return Err(ErrorData::invalid_params("item_id is required", None)); + } + self.with_backend_db(|conn| { + let comments = agentflare_backend::comment::list_by_item(conn, &item_id) + .map_err(map_backend_err)?; + Ok(serde_json::to_string_pretty(&comments).unwrap_or_default()) + })? + } + other => Err(ErrorData::invalid_params( + format!("unknown comment action: '{other}' — expected create|edit|delete|list"), + None, + )), + } + } + fn label_inner(&self, req: LabelRequest) -> Result { match req.action.as_str() { "create" => { @@ -4663,6 +4885,308 @@ mod tests { }); } + #[test] + fn item_comment_create_and_list_roundtrip() { + 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 comment: serde_json::Value = serde_json::from_str( + &s.comment(Parameters(CommentRequest { + action: "create".into(), + item_id: Some(item_id.clone()), + body: Some("Hello, world!".into()), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + assert_eq!(comment["body"], "Hello, world!"); + assert!(comment["author_agent"].as_str().unwrap().contains(':')); + + let comments: serde_json::Value = serde_json::from_str( + &s.comment(Parameters(CommentRequest { + action: "list".into(), + item_id: Some(item_id), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let arr = comments.as_array().unwrap(); + assert_eq!(arr.len(), 1); + assert_eq!(arr[0]["body"], "Hello, world!"); + } + + #[test] + fn item_comment_rejects_empty_body() { + let (_tmp, s) = harness(); + let err = s + .comment(Parameters(CommentRequest { + action: "create".into(), + item_id: Some("item-1".into()), + body: Some("".into()), + ..Default::default() + })) + .unwrap_err(); + assert_eq!(err.code, rmcp::model::ErrorCode::INVALID_PARAMS); + } + + #[test] + fn item_comment_edit_succeeds_when_latest_and_own_and_unclaimed_by_other() { + 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 comment: serde_json::Value = serde_json::from_str( + &s.comment(Parameters(CommentRequest { + action: "create".into(), + item_id: Some(item_id.clone()), + body: Some("original".into()), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let comment_id = comment["id"].as_str().unwrap().to_string(); + + let updated: serde_json::Value = serde_json::from_str( + &s.comment(Parameters(CommentRequest { + action: "edit".into(), + id: Some(comment_id.clone()), + body: Some("edited".into()), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + assert_eq!(updated["body"], "edited"); + } + + #[test] + fn item_comment_edit_rejected_when_comment_not_found() { + let (_tmp, s) = harness(); + let err = s + .comment(Parameters(CommentRequest { + action: "edit".into(), + id: Some("nonexistent".into()), + body: Some("edited".into()), + ..Default::default() + })) + .unwrap_err(); + assert_eq!(err.code, rmcp::model::ErrorCode::INVALID_PARAMS); + } + + #[test] + fn item_comment_edit_rejected_when_different_agent() { + 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 comment_id = s + .with_backend_db(|conn| { + agentflare_backend::comment::create(conn, &item_id, "someone-else:1", "not mine") + .unwrap() + .id + }) + .unwrap(); + + let err = s + .comment(Parameters(CommentRequest { + action: "edit".into(), + id: Some(comment_id), + body: Some("edited".into()), + ..Default::default() + })) + .unwrap_err(); + assert_eq!(err.code, rmcp::model::ErrorCode::INVALID_PARAMS); + assert!(err.message.contains("own comments")); + } + + #[test] + fn item_comment_edit_succeeds_across_sessions_of_same_agent() { + 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(); + + // Same agent, different session instance — e.g. a prior CLI + // invocation, or an MCP server process that has since restarted. + let agent = crate::claims::agent_of(&crate::claims::owner_id()).to_string(); + let earlier_session_author = format!("{agent}:some-earlier-session"); + + let comment_id = s + .with_backend_db(|conn| { + agentflare_backend::comment::create( + conn, + &item_id, + &earlier_session_author, + "mine, from an earlier session", + ) + .unwrap() + .id + }) + .unwrap(); + + let updated: serde_json::Value = serde_json::from_str( + &s.comment(Parameters(CommentRequest { + action: "edit".into(), + id: Some(comment_id), + body: Some("edited".into()), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + assert_eq!(updated["body"], "edited"); + } + + #[test] + fn item_comment_edit_uses_id_tiebreak_when_timestamps_collide() { + 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 first: serde_json::Value = serde_json::from_str( + &s.comment(Parameters(CommentRequest { + action: "create".into(), + item_id: Some(item_id.clone()), + body: Some("first".into()), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let first_id = first["id"].as_str().unwrap().to_string(); + + let second: serde_json::Value = serde_json::from_str( + &s.comment(Parameters(CommentRequest { + action: "create".into(), + item_id: Some(item_id), + body: Some("second".into()), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let second_id = second["id"].as_str().unwrap().to_string(); + + // Force both comments onto the same second-resolution timestamp, as + // happens routinely under real multi-agent traffic. Only the comment + // with the higher (later) UUIDv7 id should still count as latest. + s.with_backend_db(|conn| { + conn.execute( + "UPDATE item_comments SET created_at = 1000, updated_at = 1000", + [], + ) + .unwrap(); + }) + .unwrap(); + + let err = s + .comment(Parameters(CommentRequest { + action: "edit".into(), + id: Some(first_id), + body: Some("edited".into()), + ..Default::default() + })) + .unwrap_err(); + assert_eq!(err.code, rmcp::model::ErrorCode::INVALID_PARAMS); + + let updated: serde_json::Value = serde_json::from_str( + &s.comment(Parameters(CommentRequest { + action: "edit".into(), + id: Some(second_id), + body: Some("edited".into()), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + assert_eq!(updated["body"], "edited"); + } + + #[test] + fn item_comment_delete_succeeds_when_latest_and_own() { + 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 comment: serde_json::Value = serde_json::from_str( + &s.comment(Parameters(CommentRequest { + action: "create".into(), + item_id: Some(item_id), + body: Some("delete-me".into()), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let comment_id = comment["id"].as_str().unwrap().to_string(); + + let result: serde_json::Value = serde_json::from_str( + &s.comment(Parameters(CommentRequest { + action: "delete".into(), + id: Some(comment_id), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + assert_eq!(result["deleted"], true); + } + + #[test] + fn item_claim_response_includes_worktree_path() { + let tmp = tempfile::tempdir().unwrap(); + // Isolated temp repo — this test must never run real `git + // worktree`/branch operations against the actual repository running + // the test suite. + 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(); + + let result: serde_json::Value = serde_json::from_str( + &s.item(Parameters(ItemRequest { + action: "claim".into(), + id: Some(item_id), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + assert_eq!(result["status"], "acquired"); + assert!(result.get("worktree_path").is_some()); + let path = result["worktree_path"].as_str().unwrap(); + assert!(std::path::Path::new(path).exists()); + } + #[test] fn item_rejects_unknown_action() { let (_tmp, s) = harness(); diff --git a/src/worktree.rs b/src/worktree.rs new file mode 100644 index 00000000..3ed5f39c --- /dev/null +++ b/src/worktree.rs @@ -0,0 +1,296 @@ +use std::path::Path; +use std::path::PathBuf; +use std::process::Command; + +fn run_git_in(repo_root: &Path, args: &[&str]) -> Result { + let out = Command::new("git") + .args(args) + .current_dir(repo_root) + .output() + .map_err(|e| format!("git not available: {e}"))?; + if !out.status.success() { + let stderr = String::from_utf8_lossy(&out.stderr).trim().to_string(); + return Err(stderr); + } + let stdout = String::from_utf8_lossy(&out.stdout).trim().to_string(); + Ok(stdout) +} + +fn run_git_in_ok(repo_root: &Path, args: &[&str]) -> bool { + Command::new("git") + .args(args) + .current_dir(repo_root) + .output() + .ok() + .is_some_and(|o| o.status.success()) +} + +pub fn resolve_target_branch( + conn: &rusqlite::Connection, + item: &agentflare_backend::item::Item, + repo_root: &Path, +) -> String { + if let Some(ref parent_id) = item.parent_id + && let Ok(parent) = agentflare_backend::item::get(conn, parent_id) + && let Ok(meta) = serde_json::from_str::(&parent.metadata) + && let Some(branch) = meta.get("branch").and_then(|v| v.as_str()) + { + return branch.to_string(); + } + resolve_default_branch(repo_root) +} + +fn resolve_default_branch(repo_root: &Path) -> String { + if let Ok(out) = run_git_in( + repo_root, + &["symbolic-ref", "--short", "refs/remotes/origin/HEAD"], + ) && let Some(stripped) = out.strip_prefix("origin/") + { + return stripped.to_string(); + } + if run_git_in_ok(repo_root, &["rev-parse", "--verify", "main"]) { + return "main".to_string(); + } + if run_git_in_ok(repo_root, &["rev-parse", "--verify", "master"]) { + return "master".to_string(); + } + // Last resort: whatever branch is actually checked out here, so repos + // using trunk/develop/anything else still get a real branch instead of + // a hardcoded guess that may not exist. + run_git_in(repo_root, &["symbolic-ref", "--short", "HEAD"]) + .unwrap_or_else(|_| "master".to_string()) +} + +pub fn already_isolated_for(branch: &str, repo_root: &Path) -> bool { + let git_dir = match run_git_in(repo_root, &["rev-parse", "--git-dir"]) { + Ok(d) => d, + Err(_) => return false, + }; + let common_dir = match run_git_in(repo_root, &["rev-parse", "--git-common-dir"]) { + Ok(d) => d, + Err(_) => return false, + }; + if git_dir == common_dir { + return false; + } + // Exits 0 with EMPTY stdout in a plain linked worktree (not a git + // submodule) — only a non-empty path means we're actually inside a + // submodule's own superproject relationship, which is the case this + // guard exists to rule out. + if let Ok(out) = run_git_in( + repo_root, + &["rev-parse", "--show-superproject-working-tree"], + ) && !out.is_empty() + { + return false; + } + match run_git_in(repo_root, &["branch", "--show-current"]) { + Ok(b) => b == branch, + Err(_) => false, + } +} + +/// Adds `.worktrees/` to this repo's LOCAL, untracked ignore rules +/// (`.git/info/exclude`) rather than the tracked `.gitignore` — a claim +/// should never create a commit in the caller's repository (would sweep up +/// any unrelated staged files, and any pre-existing uncommitted `.gitignore` +/// edits, into a commit the agent didn't ask for). +pub fn ensure_worktrees_ignored(repo_root: &Path) { + let Ok(common_dir) = run_git_in(repo_root, &["rev-parse", "--git-common-dir"]) else { + return; + }; + let exclude_path = repo_root.join(common_dir).join("info").join("exclude"); + if let Ok(existing) = std::fs::read_to_string(&exclude_path) + && existing + .lines() + .any(|l| l.trim() == ".worktrees/" || l.trim() == ".worktrees") + { + return; + } + let mut content = String::new(); + if let Ok(existing) = std::fs::read_to_string(&exclude_path) { + content = existing; + } + if !content.ends_with('\n') && !content.is_empty() { + content.push('\n'); + } + content.push_str(".worktrees/\n"); + if let Some(parent) = exclude_path.parent() { + let _ = std::fs::create_dir_all(parent); + } + if std::fs::write(&exclude_path, content).is_err() { + eprintln!("worktree: failed to write .git/info/exclude"); + } +} + +/// Creates an isolated git worktree for `item` against `target_branch`. +/// +/// Deliberately takes an already-resolved `target_branch` instead of a +/// database connection: callers should resolve the branch (`resolve_target_branch`, +/// above) while still holding whatever lock guards the database, then call +/// this *after* releasing it. `git worktree add` is a blocking +/// filesystem+subprocess operation with no business running while a shared +/// DB lock is held. +pub fn create_worktree( + item: &agentflare_backend::item::Item, + repo_root: &Path, + target_branch: &str, +) -> Option { + let branch = format!("task/{}", item.sequence_id); + let worktree_path = repo_root + .join(".worktrees") + .join("task") + .join(item.sequence_id.to_string()); + if already_isolated_for(&branch, repo_root) { + return Some(worktree_path); + } + ensure_worktrees_ignored(repo_root); + if let Some(parent) = worktree_path.parent() { + let _ = std::fs::create_dir_all(parent); + } + match run_git_in( + repo_root, + &[ + "worktree", + "add", + &worktree_path.to_string_lossy(), + "-b", + &branch, + target_branch, + ], + ) { + Ok(_) => Some(worktree_path), + Err(e) => { + eprintln!("worktree: creation skipped for item {}: {}", item.id, e); + None + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + struct Repo { + _dir: TempDir, + path: PathBuf, + } + + fn init_repo() -> Repo { + init_repo_with_branch("master") + } + + fn init_repo_with_branch(branch: &str) -> Repo { + let dir = TempDir::new().unwrap(); + let path = dir.path().to_path_buf(); + run_git_in(&path, &["init", "-b", branch]).unwrap(); + run_git_in(&path, &["config", "user.email", "test@test.com"]).unwrap(); + run_git_in(&path, &["config", "user.name", "Test"]).unwrap(); + run_git_in(&path, &["commit", "--allow-empty", "-m", "initial"]).unwrap(); + Repo { _dir: dir, path } + } + + fn test_item(sequence_id: i64) -> agentflare_backend::item::Item { + agentflare_backend::item::Item { + id: "test-id".into(), + project_id: "proj".into(), + state_id: "state".into(), + name: "test".into(), + description: String::new(), + priority: "none".into(), + parent_id: None, + assignee_agent: None, + sequence_id, + sort_order: 0.0, + started_at: None, + completed_at: None, + archived_at: None, + external_source: None, + external_id: None, + metadata: "{}".into(), + created_at: 0, + updated_at: 0, + deleted_at: None, + } + } + + #[test] + fn resolve_default_branch_resolves_from_origin_head() { + let repo = init_repo(); + assert_eq!(resolve_default_branch(&repo.path), "master"); + } + + #[test] + fn resolve_default_branch_falls_back_to_actual_head_for_nonstandard_names() { + // No origin, no "main", no "master" — must not guess "master" when + // the repo's real default branch is named something else entirely. + let repo = init_repo_with_branch("trunk"); + assert_eq!(resolve_default_branch(&repo.path), "trunk"); + } + + #[test] + fn ensure_worktrees_ignored_is_noop_when_already_ignored() { + let repo = init_repo(); + let exclude_path = repo.path.join(".git").join("info").join("exclude"); + std::fs::create_dir_all(exclude_path.parent().unwrap()).unwrap(); + std::fs::write(&exclude_path, ".worktrees/\n").unwrap(); + let before = std::fs::read_to_string(&exclude_path).unwrap(); + ensure_worktrees_ignored(&repo.path); + let after = std::fs::read_to_string(&exclude_path).unwrap(); + assert_eq!(before, after); + } + + #[test] + fn ensure_worktrees_ignored_adds_to_local_exclude_without_committing() { + let repo = init_repo(); + ensure_worktrees_ignored(&repo.path); + let exclude_path = repo.path.join(".git").join("info").join("exclude"); + let content = std::fs::read_to_string(&exclude_path).unwrap(); + assert!(content.contains(".worktrees/")); + // Must never touch the tracked .gitignore or create a commit. + assert!(!repo.path.join(".gitignore").exists()); + let log = run_git_in(&repo.path, &["log", "--oneline"]).unwrap(); + assert_eq!( + log.lines().count(), + 1, + "no new commit should have been made" + ); + } + + #[test] + fn already_isolated_for_false_in_regular_repo() { + let repo = init_repo(); + assert!(!already_isolated_for("task/1", &repo.path)); + } + + #[test] + fn already_isolated_for_true_inside_the_worktree_it_created() { + let repo = init_repo(); + let item = test_item(1); + let target = resolve_default_branch(&repo.path); + let worktree_path = create_worktree(&item, &repo.path, &target).unwrap(); + assert!(already_isolated_for("task/1", &worktree_path)); + } + + #[test] + fn create_worktree_creates_worktree_and_branch() { + let repo = init_repo(); + let worktree_path = repo.path.join(".worktrees").join("task").join("1"); + let item = test_item(1); + let target = resolve_default_branch(&repo.path); + let result = create_worktree(&item, &repo.path, &target); + assert!(result.is_some()); + assert!(worktree_path.exists()); + } + + #[test] + fn create_worktree_soft_fails_on_bad_git() { + let tmp = TempDir::new().unwrap(); + let bad_root = tmp.path().join("not-a-repo"); + std::fs::create_dir_all(&bad_root).unwrap(); + let item = test_item(1); + let result = create_worktree(&item, &bad_root, "master"); + assert!(result.is_none()); + } +}