diff --git a/src/graph/mod.rs b/src/graph/mod.rs index c5f4bfb..4fb5948 100644 --- a/src/graph/mod.rs +++ b/src/graph/mod.rs @@ -206,7 +206,7 @@ impl FromStr for Priority { } /// A node in the work graph -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct GraphNode { pub id: String, pub project_id: String, @@ -226,7 +226,7 @@ pub struct GraphNode { } /// An edge connecting two nodes in the work graph -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct GraphEdge { pub id: String, pub edge_type: EdgeType, diff --git a/src/graph/store.rs b/src/graph/store.rs index 852fb89..a7d17ac 100644 --- a/src/graph/store.rs +++ b/src/graph/store.rs @@ -1,6 +1,6 @@ use crate::db::Database; use crate::graph::{GraphEdge, GraphNode, NodeStatus, NodeType, parent_id, validate_status}; -use anyhow::{Result, anyhow}; +use anyhow::{Context, Result}; use async_trait::async_trait; use chrono::Utc; use std::collections::HashMap; @@ -234,7 +234,8 @@ impl GraphStore for SqliteGraphStore { db.connection() .call(move |conn| { - let tx = conn.transaction()?; + let tx = + conn.transaction_with_behavior(rusqlite::TransactionBehavior::Immediate)?; // Insert the node tx.execute( @@ -317,15 +318,6 @@ impl GraphStore for SqliteGraphStore { description: Option<&str>, metadata: Option<&HashMap>, ) -> Result<()> { - // Validate status if provided - if let Some(s) = status { - // Get the node to determine its type - let node = self.get_node(id).await?; - if let Some(n) = node { - validate_status(&n.node_type, &s)?; - } - } - let id = id.to_string(); let status_str = status.map(|s| s.to_string()); let title_owned = title.map(|t| t.to_string()); @@ -335,7 +327,25 @@ impl GraphStore for SqliteGraphStore { self.db .connection() .call(move |conn| { - let tx = conn.transaction()?; + let tx = conn.transaction_with_behavior(rusqlite::TransactionBehavior::Immediate)?; + + // Validate status if provided (inside transaction to avoid TOCTOU) + if let Some(s) = &status_str { + // Get the node to determine its type + let node_type_str: String = tx.query_row( + "SELECT node_type FROM nodes WHERE id = ?1", + rusqlite::params![&id], + |row| row.get(0), + )?; + + let node_type: NodeType = node_type_str.parse() + .map_err(|_| rusqlite::Error::InvalidParameterName("Invalid node_type".to_string()))?; + let new_status: NodeStatus = s.parse() + .map_err(|_| rusqlite::Error::InvalidParameterName("Invalid status".to_string()))?; + + validate_status(&node_type, &new_status) + .map_err(|e| rusqlite::Error::InvalidParameterName(e.to_string()))?; + } // Build dynamic UPDATE statement let mut updates = Vec::new(); @@ -492,7 +502,8 @@ impl GraphStore for SqliteGraphStore { .db .connection() .call(move |conn| { - let tx = conn.transaction()?; + let tx = + conn.transaction_with_behavior(rusqlite::TransactionBehavior::Immediate)?; tx.execute( "UPDATE nodes SET status = 'claimed', assigned_to = ?1, started_at = ?2 @@ -566,19 +577,9 @@ impl GraphStore for SqliteGraphStore { AND status = 'ready' ), downstream_counts AS ( - SELECT rt.id, COUNT(*) as downstream_count + SELECT rt.id, COUNT(DISTINCT e.from_node) as downstream_count FROM ready_tasks rt - LEFT JOIN ( - WITH RECURSIVE transitive_deps AS ( - SELECT from_node as start_node, to_node FROM edges - WHERE edge_type = 'depends_on' - UNION ALL - SELECT td.start_node, e.to_node FROM transitive_deps td - JOIN edges e ON e.from_node = td.to_node - WHERE e.edge_type = 'depends_on' - ) - SELECT DISTINCT start_node FROM transitive_deps - ) td ON rt.id = td.start_node + LEFT JOIN edges e ON rt.id = e.to_node AND e.edge_type = 'depends_on' GROUP BY rt.id ) SELECT n.id, n.project_id, n.node_type, n.title, n.description, n.status, @@ -609,17 +610,6 @@ impl GraphStore for SqliteGraphStore { } async fn add_edge(&self, edge: &GraphEdge) -> Result<()> { - // Validate that both nodes exist - let from_node = self.get_node(&edge.from_node).await?; - let to_node = self.get_node(&edge.to_node).await?; - - if from_node.is_none() { - return Err(anyhow!("from_node does not exist: {}", edge.from_node)); - } - if to_node.is_none() { - return Err(anyhow!("to_node does not exist: {}", edge.to_node)); - } - let edge_id = edge.id.clone(); let edge_type = edge.edge_type.to_string(); let from_node_id = edge.from_node.clone(); @@ -630,7 +620,45 @@ impl GraphStore for SqliteGraphStore { self.db .connection() .call(move |conn| { - conn.execute( + let tx = + conn.transaction_with_behavior(rusqlite::TransactionBehavior::Immediate)?; + + // Validate that both nodes exist (inside transaction) + let from_exists: bool = tx + .query_row( + "SELECT COUNT(*) > 0 FROM nodes WHERE id = ?1", + rusqlite::params![&from_node_id], + |row| row.get(0), + ) + .map_err(|e| tokio_rusqlite::Error::Rusqlite(e))?; + + if !from_exists { + return Err(tokio_rusqlite::Error::Rusqlite( + rusqlite::Error::InvalidParameterName(format!( + "from_node does not exist: {}", + from_node_id + )), + )); + } + + let to_exists: bool = tx + .query_row( + "SELECT COUNT(*) > 0 FROM nodes WHERE id = ?1", + rusqlite::params![&to_node_id], + |row| row.get(0), + ) + .map_err(|e| tokio_rusqlite::Error::Rusqlite(e))?; + + if !to_exists { + return Err(tokio_rusqlite::Error::Rusqlite( + rusqlite::Error::InvalidParameterName(format!( + "to_node does not exist: {}", + to_node_id + )), + )); + } + + tx.execute( "INSERT INTO edges (id, edge_type, from_node, to_node, label, created_at) VALUES (?1, ?2, ?3, ?4, ?5, ?6)", rusqlite::params![ @@ -642,9 +670,11 @@ impl GraphStore for SqliteGraphStore { &created_at ], )?; + tx.commit()?; Ok(()) }) - .await?; + .await + .context("Failed to add edge")?; Ok(()) } @@ -946,7 +976,8 @@ impl GraphStore for SqliteGraphStore { .db .connection() .call(move |conn| { - let tx = conn.transaction()?; + let tx = + conn.transaction_with_behavior(rusqlite::TransactionBehavior::Immediate)?; // Get current metadata let metadata_json: String = tx.query_row( diff --git a/src/main.rs b/src/main.rs index c933638..f52e9dc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -91,8 +91,6 @@ enum TaskAction { List { #[arg(long)] status: Option, - #[arg(long)] - priority: Option, }, /// Show ready tasks Ready, @@ -300,10 +298,7 @@ async fn main() -> anyhow::Result<()> { let graph_store = rustagent::graph::store::SqliteGraphStore::new(database.clone()); match action { - Some(TaskAction::List { - status, - priority: _, - }) => { + Some(TaskAction::List { status }) => { let query = rustagent::graph::store::NodeQuery { node_type: Some(rustagent::graph::NodeType::Task), status: status.and_then(|s| s.parse().ok()), diff --git a/tests/common/mod.rs b/tests/common/mod.rs new file mode 100644 index 0000000..81c97cd --- /dev/null +++ b/tests/common/mod.rs @@ -0,0 +1,178 @@ +use anyhow::Result; +use chrono::Utc; +use rustagent::db::Database; +use rustagent::graph::store::{GraphStore, SqliteGraphStore}; +use rustagent::graph::*; +use std::collections::HashMap; +use std::sync::Arc; + +/// Helper to create a test goal node +pub fn create_test_goal(id: &str, project_id: &str, title: &str) -> GraphNode { + GraphNode { + id: id.to_string(), + project_id: project_id.to_string(), + node_type: NodeType::Goal, + title: title.to_string(), + description: "Test goal".to_string(), + status: NodeStatus::Pending, + priority: Some(Priority::High), + assigned_to: None, + created_by: None, + labels: vec![], + created_at: Utc::now(), + started_at: None, + completed_at: None, + blocked_reason: None, + metadata: HashMap::new(), + } +} + +/// Helper to create a test task node (can optionally accept priority) +pub fn create_test_task(id: &str, project_id: &str, title: &str, status: NodeStatus) -> GraphNode { + create_test_task_with_priority(id, project_id, title, status, Some(Priority::Medium)) +} + +/// Helper to create a test task node with specific priority +pub fn create_test_task_with_priority( + id: &str, + project_id: &str, + title: &str, + status: NodeStatus, + priority: Option, +) -> GraphNode { + GraphNode { + id: id.to_string(), + project_id: project_id.to_string(), + node_type: NodeType::Task, + title: title.to_string(), + description: "Test task".to_string(), + status, + priority, + assigned_to: None, + created_by: None, + labels: vec![], + created_at: Utc::now(), + started_at: None, + completed_at: None, + blocked_reason: None, + metadata: HashMap::new(), + } +} + +/// Helper to create a test observation node +pub fn create_test_observation( + id: &str, + project_id: &str, + title: &str, + description: &str, +) -> GraphNode { + GraphNode { + id: id.to_string(), + project_id: project_id.to_string(), + node_type: NodeType::Observation, + title: title.to_string(), + description: description.to_string(), + status: NodeStatus::Active, + priority: None, + assigned_to: None, + created_by: None, + labels: vec![], + created_at: Utc::now(), + started_at: None, + completed_at: None, + blocked_reason: None, + metadata: HashMap::new(), + } +} + +/// Helper to set up a test database with a project (graph store only) +pub async fn setup_test_env() -> Result<(Database, SqliteGraphStore)> { + let db = Database::open_in_memory().await?; + let graph_store = SqliteGraphStore::new(db.clone()); + + // Create a test project by directly inserting into the database + let db_for_project = db.clone(); + db_for_project + .connection() + .call(|conn| { + let now = chrono::Utc::now().to_rfc3339(); + conn.execute( + "INSERT INTO projects (id, name, path, registered_at, config_overrides, metadata) + VALUES (?, ?, ?, ?, ?, ?)", + rusqlite::params![ + "proj-1", + "proj-1", + "/tmp/proj-1", + &now, + None::, + "{}" + ], + )?; + Ok(()) + }) + .await?; + + Ok((db, graph_store)) +} + +/// Helper to set up a test database with a project (includes project store) +pub async fn setup_test_env_with_project() +-> Result<(Database, SqliteGraphStore, rustagent::project::ProjectStore)> { + let db = Database::open_in_memory().await?; + let proj_store = rustagent::project::ProjectStore::new(db.clone()); + let graph_store = SqliteGraphStore::new(db.clone()); + + // Create a test project by directly inserting into the database + let db_for_project = db.clone(); + db_for_project + .connection() + .call(|conn| { + let now = chrono::Utc::now().to_rfc3339(); + conn.execute( + "INSERT INTO projects (id, name, path, registered_at, config_overrides, metadata) + VALUES (?, ?, ?, ?, ?, ?)", + rusqlite::params![ + "proj-1", + "proj-1", + "/tmp/proj-1", + &now, + None::, + "{}" + ], + )?; + Ok(()) + }) + .await?; + + Ok((db, graph_store, proj_store)) +} + +/// Helper to set up a test database with a project (wrapped in Arc for concurrency tests) +pub async fn setup_test_env_concurrent() -> Result<(Database, Arc)> { + let db = Database::open_in_memory().await?; + let graph_store = Arc::new(SqliteGraphStore::new(db.clone())); + + // Create a test project by directly inserting into the database + let db_for_project = db.clone(); + db_for_project + .connection() + .call(|conn| { + let now = chrono::Utc::now().to_rfc3339(); + conn.execute( + "INSERT INTO projects (id, name, path, registered_at, config_overrides, metadata) + VALUES (?, ?, ?, ?, ?, ?)", + rusqlite::params![ + "proj-1", + "proj-1", + "/tmp/proj-1", + &now, + None::, + "{}" + ], + )?; + Ok(()) + }) + .await?; + + Ok((db, graph_store)) +} diff --git a/tests/graph_claim_search_test.rs b/tests/graph_claim_search_test.rs index eb49de7..637c7dc 100644 --- a/tests/graph_claim_search_test.rs +++ b/tests/graph_claim_search_test.rs @@ -1,108 +1,12 @@ +mod common; + use anyhow::Result; use chrono::Utc; -use rustagent::db::Database; -use rustagent::graph::store::{GraphStore, SqliteGraphStore}; +use common::{create_test_goal, create_test_observation, create_test_task, setup_test_env}; +use rustagent::graph::store::GraphStore; use rustagent::graph::*; use std::collections::HashMap; -/// Helper to create a test goal node -fn create_test_goal(id: &str, project_id: &str, title: &str) -> GraphNode { - GraphNode { - id: id.to_string(), - project_id: project_id.to_string(), - node_type: NodeType::Goal, - title: title.to_string(), - description: "Test goal".to_string(), - status: NodeStatus::Pending, - priority: Some(Priority::High), - assigned_to: None, - created_by: None, - labels: vec![], - created_at: Utc::now(), - started_at: None, - completed_at: None, - blocked_reason: None, - metadata: HashMap::new(), - } -} - -/// Helper to create a test task node -fn create_test_task(id: &str, project_id: &str, title: &str, status: NodeStatus) -> GraphNode { - GraphNode { - id: id.to_string(), - project_id: project_id.to_string(), - node_type: NodeType::Task, - title: title.to_string(), - description: "Test task".to_string(), - status, - priority: Some(Priority::Medium), - assigned_to: None, - created_by: None, - labels: vec![], - created_at: Utc::now(), - started_at: None, - completed_at: None, - blocked_reason: None, - metadata: HashMap::new(), - } -} - -/// Helper to create a test observation node -fn create_test_observation( - id: &str, - project_id: &str, - title: &str, - description: &str, -) -> GraphNode { - GraphNode { - id: id.to_string(), - project_id: project_id.to_string(), - node_type: NodeType::Observation, - title: title.to_string(), - description: description.to_string(), - status: NodeStatus::Active, - priority: None, - assigned_to: None, - created_by: None, - labels: vec![], - created_at: Utc::now(), - started_at: None, - completed_at: None, - blocked_reason: None, - metadata: HashMap::new(), - } -} - -/// Helper to set up a test database with a project -async fn setup_test_env() -> Result<(Database, SqliteGraphStore)> { - let db = Database::open_in_memory().await?; - let graph_store = SqliteGraphStore::new(db.clone()); - - // Create a test project by directly inserting into the database - let db_for_project = db.clone(); - db_for_project - .connection() - .call(|conn| { - let now = chrono::Utc::now().to_rfc3339(); - conn.execute( - "INSERT INTO projects (id, name, path, registered_at, config_overrides, metadata) - VALUES (?, ?, ?, ?, ?, ?)", - rusqlite::params![ - "proj-1", - "proj-1", - "/tmp/proj-1", - &now, - None::, - "{}" - ], - )?; - Ok(()) - }) - .await?; - - Ok((db, graph_store)) -} - /// P1b.AC5.1: claim_task atomically sets status Ready->Claimed and assigned_to #[tokio::test] async fn test_claim_task_success() -> Result<()> { diff --git a/tests/graph_concurrency_test.rs b/tests/graph_concurrency_test.rs index 50d515e..01b01a0 100644 --- a/tests/graph_concurrency_test.rs +++ b/tests/graph_concurrency_test.rs @@ -1,86 +1,14 @@ +mod common; + use anyhow::Result; -use chrono::Utc; -use rustagent::db::Database; -use rustagent::graph::store::{GraphStore, SqliteGraphStore}; +use common::{create_test_goal, create_test_task, setup_test_env_concurrent}; +use rustagent::graph::store::GraphStore; use rustagent::graph::*; -use std::collections::HashMap; use std::sync::Arc; -/// Helper to create a test task node -fn create_test_task(id: &str, project_id: &str, title: &str, status: NodeStatus) -> GraphNode { - GraphNode { - id: id.to_string(), - project_id: project_id.to_string(), - node_type: NodeType::Task, - title: title.to_string(), - description: "Test task".to_string(), - status, - priority: Some(Priority::Medium), - assigned_to: None, - created_by: None, - labels: vec![], - created_at: Utc::now(), - started_at: None, - completed_at: None, - blocked_reason: None, - metadata: HashMap::new(), - } -} - -/// Helper to create a test goal node -fn create_test_goal(id: &str, project_id: &str, title: &str) -> GraphNode { - GraphNode { - id: id.to_string(), - project_id: project_id.to_string(), - node_type: NodeType::Goal, - title: title.to_string(), - description: "Test goal".to_string(), - status: NodeStatus::Pending, - priority: Some(Priority::High), - assigned_to: None, - created_by: None, - labels: vec![], - created_at: Utc::now(), - started_at: None, - completed_at: None, - blocked_reason: None, - metadata: HashMap::new(), - } -} - -/// Helper to set up a test database with a project -async fn setup_test_env() -> Result<(Database, Arc)> { - let db = Database::open_in_memory().await?; - let graph_store = Arc::new(SqliteGraphStore::new(db.clone())); - - // Create a test project by directly inserting into the database - let db_for_project = db.clone(); - db_for_project - .connection() - .call(|conn| { - let now = chrono::Utc::now().to_rfc3339(); - conn.execute( - "INSERT INTO projects (id, name, path, registered_at, config_overrides, metadata) - VALUES (?, ?, ?, ?, ?, ?)", - rusqlite::params![ - "proj-1", - "proj-1", - "/tmp/proj-1", - &now, - None::, - "{}" - ], - )?; - Ok(()) - }) - .await?; - - Ok((db, graph_store)) -} - #[tokio::test] async fn test_concurrent_task_claiming() -> Result<()> { - let (_db, store) = setup_test_env().await?; + let (_db, store) = setup_test_env_concurrent().await?; // Create a goal and a task in Ready status let goal = create_test_goal("goal-1", "proj-1", "Test Goal"); @@ -152,7 +80,7 @@ async fn test_concurrent_task_claiming() -> Result<()> { #[tokio::test] async fn test_concurrent_claim_different_tasks() -> Result<()> { - let (_db, store) = setup_test_env().await?; + let (_db, store) = setup_test_env_concurrent().await?; // Create a goal and multiple tasks in Ready status let goal = create_test_goal("goal-2", "proj-1", "Test Goal 2"); @@ -228,7 +156,7 @@ async fn test_concurrent_claim_different_tasks() -> Result<()> { #[tokio::test] async fn test_claim_non_ready_task_fails() -> Result<()> { - let (_db, store) = setup_test_env().await?; + let (_db, store) = setup_test_env_concurrent().await?; // Create a goal and a task that is NOT in Ready status let goal = create_test_goal("goal-3", "proj-1", "Test Goal 3"); @@ -258,7 +186,7 @@ async fn test_claim_non_ready_task_fails() -> Result<()> { #[tokio::test] async fn test_concurrent_claim_race_condition() -> Result<()> { - let (_db, store) = setup_test_env().await?; + let (_db, store) = setup_test_env_concurrent().await?; // Create a goal and one Ready task let goal = create_test_goal("goal-4", "proj-1", "Test Goal 4"); diff --git a/tests/graph_dependency_test.rs b/tests/graph_dependency_test.rs index 12d5c1e..847f4fd 100644 --- a/tests/graph_dependency_test.rs +++ b/tests/graph_dependency_test.rs @@ -1,87 +1,12 @@ +mod common; + use anyhow::Result; use chrono::Utc; -use rustagent::db::Database; -use rustagent::graph::store::{GraphStore, SqliteGraphStore}; +use common::{create_test_goal, create_test_task_with_priority, setup_test_env}; +use rustagent::graph::store::GraphStore; use rustagent::graph::*; -use std::collections::HashMap; - -/// Helper to create a test goal node -fn create_test_goal(id: &str, project_id: &str, title: &str) -> GraphNode { - GraphNode { - id: id.to_string(), - project_id: project_id.to_string(), - node_type: NodeType::Goal, - title: title.to_string(), - description: "Test goal".to_string(), - status: NodeStatus::Pending, - priority: Some(Priority::High), - assigned_to: None, - created_by: None, - labels: vec![], - created_at: Utc::now(), - started_at: None, - completed_at: None, - blocked_reason: None, - metadata: HashMap::new(), - } -} - -/// Helper to create a test task node -fn create_test_task( - id: &str, - project_id: &str, - title: &str, - status: NodeStatus, - priority: Option, -) -> GraphNode { - GraphNode { - id: id.to_string(), - project_id: project_id.to_string(), - node_type: NodeType::Task, - title: title.to_string(), - description: "Test task".to_string(), - status, - priority, - assigned_to: None, - created_by: None, - labels: vec![], - created_at: Utc::now(), - started_at: None, - completed_at: None, - blocked_reason: None, - metadata: HashMap::new(), - } -} - -/// Helper to set up a test database with a project -async fn setup_test_env() -> Result<(Database, SqliteGraphStore)> { - let db = Database::open_in_memory().await?; - let graph_store = SqliteGraphStore::new(db.clone()); - - // Create a test project by directly inserting into the database - let db_for_project = db.clone(); - db_for_project - .connection() - .call(|conn| { - let now = chrono::Utc::now().to_rfc3339(); - conn.execute( - "INSERT INTO projects (id, name, path, registered_at, config_overrides, metadata) - VALUES (?, ?, ?, ?, ?, ?)", - rusqlite::params![ - "proj-1", - "proj-1", - "/tmp/proj-1", - &now, - None::, - "{}" - ], - )?; - Ok(()) - }) - .await?; - - Ok((db, graph_store)) -} +use std::time::Duration; +use tokio::time::sleep; /// P1b.AC4.1: Task moves from Pending to Ready when all DependsOn targets are Completed #[tokio::test] @@ -90,14 +15,14 @@ async fn test_task_pending_to_ready_when_deps_complete() -> Result<()> { // Create a goal and two tasks let goal = create_test_goal("ra-a1b2", "proj-1", "Test Goal"); - let task_a = create_test_task( + let task_a = create_test_task_with_priority( "ra-a1b2.1", "proj-1", "Task A", NodeStatus::Pending, Some(Priority::Medium), ); - let task_b = create_test_task( + let task_b = create_test_task_with_priority( "ra-a1b2.2", "proj-1", "Task B", @@ -149,21 +74,21 @@ async fn test_get_ready_tasks_filters_correctly() -> Result<()> { store.create_node(&goal).await?; // Create three tasks: one Ready, one Pending (with unmet deps), one Completed - let task_ready = create_test_task( + let task_ready = create_test_task_with_priority( "ra-a1b2.1", "proj-1", "Task Ready", NodeStatus::Ready, Some(Priority::Medium), ); - let task_pending = create_test_task( + let task_pending = create_test_task_with_priority( "ra-a1b2.2", "proj-1", "Task Pending", NodeStatus::Pending, Some(Priority::Medium), ); - let task_completed = create_test_task( + let task_completed = create_test_task_with_priority( "ra-a1b2.3", "proj-1", "Task Completed", @@ -205,14 +130,14 @@ async fn test_get_next_task_priority_and_downstream() -> Result<()> { store.create_node(&goal).await?; // Create two ready tasks: one High priority (blocking 3 tasks), one Critical priority (blocking 0) - let task_high_priority = create_test_task( + let task_high_priority = create_test_task_with_priority( "ra-a1b2.1", "proj-1", "High Priority", NodeStatus::Ready, Some(Priority::High), ); - let task_critical_priority = create_test_task( + let task_critical_priority = create_test_task_with_priority( "ra-a1b2.2", "proj-1", "Critical Priority", @@ -224,21 +149,21 @@ async fn test_get_next_task_priority_and_downstream() -> Result<()> { store.create_node(&task_critical_priority).await?; // Create 3 more tasks that depend on the High priority task - let dependent1 = create_test_task( + let dependent1 = create_test_task_with_priority( "ra-a1b2.3", "proj-1", "Dependent 1", NodeStatus::Pending, Some(Priority::Medium), ); - let dependent2 = create_test_task( + let dependent2 = create_test_task_with_priority( "ra-a1b2.4", "proj-1", "Dependent 2", NodeStatus::Pending, Some(Priority::Medium), ); - let dependent3 = create_test_task( + let dependent3 = create_test_task_with_priority( "ra-a1b2.5", "proj-1", "Dependent 3", @@ -274,6 +199,8 @@ async fn test_get_next_task_priority_and_downstream() -> Result<()> { } /// P1b.AC4.3 variant: When priorities are equal, downstream count should be the tiebreaker +/// This test creates tasks in reverse order (B before A) to ensure that created_at ordering +/// would pick the wrong task without the downstream count logic. #[tokio::test] async fn test_get_next_task_tiebreak_by_downstream() -> Result<()> { let (_db, store) = setup_test_env().await?; @@ -282,41 +209,46 @@ async fn test_get_next_task_tiebreak_by_downstream() -> Result<()> { let goal = create_test_goal("ra-a1b2", "proj-1", "Test Goal"); store.create_node(&goal).await?; - // Create two ready tasks with the same priority - let task_a = create_test_task( - "ra-a1b2.1", + // Create Task B FIRST (so it has an earlier created_at) + let task_b = create_test_task_with_priority( + "ra-a1b2.2", "proj-1", - "Task A", + "Task B", NodeStatus::Ready, Some(Priority::High), ); - let task_b = create_test_task( - "ra-a1b2.2", + store.create_node(&task_b).await?; + + // Small delay to ensure Task A has a later created_at + sleep(Duration::from_millis(10)).await; + + // Create Task A SECOND (so it has a later created_at) + // Without downstream count logic, ordering by created_at would pick B + let task_a = create_test_task_with_priority( + "ra-a1b2.1", "proj-1", - "Task B", + "Task A", NodeStatus::Ready, Some(Priority::High), ); - store.create_node(&task_a).await?; - store.create_node(&task_b).await?; // Create 3 tasks that depend on Task A (higher downstream count) - let dep_a1 = create_test_task( + let dep_a1 = create_test_task_with_priority( "ra-a1b2.3", "proj-1", "Dep A1", NodeStatus::Pending, Some(Priority::Medium), ); - let dep_a2 = create_test_task( + let dep_a2 = create_test_task_with_priority( "ra-a1b2.4", "proj-1", "Dep A2", NodeStatus::Pending, Some(Priority::Medium), ); - let dep_a3 = create_test_task( + let dep_a3 = create_test_task_with_priority( "ra-a1b2.5", "proj-1", "Dep A3", @@ -341,11 +273,14 @@ async fn test_get_next_task_tiebreak_by_downstream() -> Result<()> { store.add_edge(&edge).await?; } - // get_next_task should return Task A (higher downstream count) + // get_next_task should return Task A (higher downstream count), not Task B (earlier created_at) let next_task = store.get_next_task("ra-a1b2").await?; assert!(next_task.is_some()); let task = next_task.unwrap(); - assert_eq!(task.id, "ra-a1b2.1"); + assert_eq!( + task.id, "ra-a1b2.1", + "Expected Task A with higher downstream count, not Task B with earlier created_at" + ); Ok(()) } diff --git a/tests/graph_store_test.rs b/tests/graph_store_test.rs index cbb79b8..02bed13 100644 --- a/tests/graph_store_test.rs +++ b/tests/graph_store_test.rs @@ -1,90 +1,15 @@ +mod common; + use anyhow::Result; use chrono::Utc; -use rustagent::db::Database; +use common::{create_test_goal, create_test_task, setup_test_env_with_project}; use rustagent::graph::store::GraphStore; use rustagent::graph::*; use std::collections::HashMap; -/// Helper to create a test goal node -fn create_test_goal(id: &str, project_id: &str, title: &str) -> GraphNode { - GraphNode { - id: id.to_string(), - project_id: project_id.to_string(), - node_type: NodeType::Goal, - title: title.to_string(), - description: "Test goal".to_string(), - status: NodeStatus::Pending, - priority: Some(Priority::High), - assigned_to: None, - created_by: None, - labels: vec![], - created_at: Utc::now(), - started_at: None, - completed_at: None, - blocked_reason: None, - metadata: HashMap::new(), - } -} - -/// Helper to create a test task node -fn create_test_task(id: &str, project_id: &str, title: &str, status: NodeStatus) -> GraphNode { - GraphNode { - id: id.to_string(), - project_id: project_id.to_string(), - node_type: NodeType::Task, - title: title.to_string(), - description: "Test task".to_string(), - status, - priority: Some(Priority::Medium), - assigned_to: None, - created_by: None, - labels: vec![], - created_at: Utc::now(), - started_at: None, - completed_at: None, - blocked_reason: None, - metadata: HashMap::new(), - } -} - -/// Helper to set up a test database with a project -async fn setup_test_env() -> Result<( - Database, - rustagent::graph::store::SqliteGraphStore, - rustagent::project::ProjectStore, -)> { - let db = Database::open_in_memory().await?; - let proj_store = rustagent::project::ProjectStore::new(db.clone()); - let graph_store = rustagent::graph::store::SqliteGraphStore::new(db.clone()); - - // Create a test project by directly inserting into the database - let db_for_project = db.clone(); - db_for_project - .connection() - .call(|conn| { - let now = chrono::Utc::now().to_rfc3339(); - conn.execute( - "INSERT INTO projects (id, name, path, registered_at, config_overrides, metadata) - VALUES (?, ?, ?, ?, ?, ?)", - rusqlite::params![ - "proj-1", - "proj-1", - "/tmp/proj-1", - &now, - None::, - "{}" - ], - )?; - Ok(()) - }) - .await?; - - Ok((db, graph_store, proj_store)) -} - #[tokio::test] async fn test_create_and_get_goal_node() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let goal = create_test_goal("ra-a1b2", "proj-1", "Test Goal"); store.create_node(&goal).await?; @@ -102,7 +27,7 @@ async fn test_create_and_get_goal_node() -> Result<()> { #[tokio::test] async fn test_get_nonexistent_node() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let retrieved = store.get_node("nonexistent-id").await?; assert!(retrieved.is_none()); @@ -112,7 +37,7 @@ async fn test_get_nonexistent_node() -> Result<()> { #[tokio::test] async fn test_update_node_status() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let goal = create_test_goal("ra-a1b2", "proj-1", "Goal"); store.create_node(&goal).await?; @@ -134,7 +59,7 @@ async fn test_update_node_status() -> Result<()> { #[tokio::test] async fn test_update_node_title() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let goal = create_test_goal("ra-a1b2", "proj-1", "Goal"); store.create_node(&goal).await?; @@ -156,7 +81,7 @@ async fn test_update_node_title() -> Result<()> { #[tokio::test] async fn test_add_and_get_edge() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let goal = create_test_goal("ra-a1b2", "proj-1", "Goal"); let task = create_test_task("ra-a1b2.1", "proj-1", "Task", NodeStatus::Pending); @@ -186,7 +111,7 @@ async fn test_add_and_get_edge() -> Result<()> { #[tokio::test] async fn test_get_children() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let goal = create_test_goal("ra-a1b2", "proj-1", "Goal"); store.create_node(&goal).await?; @@ -207,7 +132,7 @@ async fn test_get_children() -> Result<()> { #[tokio::test] async fn test_get_subtree() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let goal = create_test_goal("ra-a1b2", "proj-1", "Goal"); store.create_node(&goal).await?; @@ -246,7 +171,7 @@ async fn test_get_subtree() -> Result<()> { #[tokio::test] async fn test_get_active_decisions() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let active_decision = GraphNode { id: generate_goal_id(), @@ -317,7 +242,7 @@ async fn test_get_active_decisions() -> Result<()> { #[tokio::test] async fn test_get_full_graph() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let goal = create_test_goal("ra-goal1", "proj-1", "Goal"); store.create_node(&goal).await?; @@ -347,7 +272,7 @@ async fn test_get_full_graph() -> Result<()> { #[tokio::test] async fn test_search_nodes_by_title() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let goal = GraphNode { id: generate_goal_id(), @@ -380,7 +305,7 @@ async fn test_search_nodes_by_title() -> Result<()> { #[tokio::test] async fn test_search_nodes_with_type_filter() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let goal = GraphNode { id: generate_goal_id(), @@ -438,7 +363,7 @@ async fn test_search_nodes_with_type_filter() -> Result<()> { #[tokio::test] async fn test_claim_task() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let task = create_test_task("ra-task1", "proj-1", "Test Task", NodeStatus::Ready); store.create_node(&task).await?; @@ -457,7 +382,7 @@ async fn test_claim_task() -> Result<()> { #[tokio::test] async fn test_claim_task_already_claimed() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let task = create_test_task("ra-task1", "proj-1", "Test Task", NodeStatus::Ready); store.create_node(&task).await?; @@ -475,7 +400,7 @@ async fn test_claim_task_already_claimed() -> Result<()> { #[tokio::test] async fn test_next_child_seq() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let parent = create_test_goal("ra-parent", "proj-1", "Parent"); store.create_node(&parent).await?; @@ -497,7 +422,7 @@ async fn test_next_child_seq() -> Result<()> { #[tokio::test] async fn test_query_nodes_by_type() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let goal = create_test_goal("ra-g1", "proj-1", "Goal"); let task = create_test_task("ra-t1", "proj-1", "Task", NodeStatus::Pending); @@ -521,7 +446,7 @@ async fn test_query_nodes_by_type() -> Result<()> { #[tokio::test] async fn test_query_nodes_by_status() -> Result<()> { - let (_db, store, _proj_store) = setup_test_env().await?; + let (_db, store, _proj_store) = setup_test_env_with_project().await?; let pending_task = create_test_task("ra-t1", "proj-1", "Pending Task", NodeStatus::Pending); let active_task = create_test_task("ra-t2", "proj-1", "Active Task", NodeStatus::Active);