From 426acc050ba59eb217793b11b29efd8f6a566154 Mon Sep 17 00:00:00 2001 From: David Hagerty Date: Mon, 9 Feb 2026 09:21:19 -0500 Subject: [PATCH] fix: address code review feedback for Phase 1a Critical fixes: - C1: Add test for P1a.AC2.3 - newer database version error test now opens in-memory DB, sets schema_version to 999, calls run_migrations, and asserts error containing 'newer version'. Important fixes: - I1: Wrap schema creation in transaction for atomicity using BEGIN IMMEDIATE/COMMIT - I2: Run cargo fmt to fix formatting in 6 files - I3: Fix 3 clippy warnings (redundant closure, collapsible if statements) Minor fixes: - M1: Add path canonicalization in CLI handler for consistent path matching - M2: Update error message to match spec requirements Verification: All 62 tests pass, fmt check passes, 0 clippy warnings Co-Authored-By: Claude Opus 4.6 --- src/db/migrations.rs | 61 ++++++++++++++++--------------------------- src/main.rs | 54 ++++++++++++++++++++------------------ src/project.rs | 27 +++++++++---------- src/ralph/mod.rs | 1 - tests/db_test.rs | 53 +++++++++++++++++++++++++++++-------- tests/project_test.rs | 4 +-- 6 files changed, 106 insertions(+), 94 deletions(-) diff --git a/src/db/migrations.rs b/src/db/migrations.rs index 5059bb5..c9fb523 100644 --- a/src/db/migrations.rs +++ b/src/db/migrations.rs @@ -5,21 +5,18 @@ const CURRENT_VERSION: u32 = 1; /// Run migrations to set up or upgrade the database schema pub async fn run_migrations(conn: &Connection) -> Result<()> { - conn.call(|c| { - check_and_migrate(c).map_err(|e| tokio_rusqlite::Error::Rusqlite(e)) - }) - .await - .map_err(|e| anyhow::anyhow!(e))?; + conn.call(|c| check_and_migrate(c).map_err(tokio_rusqlite::Error::Rusqlite)) + .await + .map_err(|e| anyhow::anyhow!(e))?; Ok(()) } -/// Check schema version and apply migrations if needed -fn check_and_migrate(conn: &rusqlite::Connection) -> rusqlite::Result<()> { +/// Check schema version and apply migrations if needed (exposed for testing) +pub fn check_and_migrate(conn: &rusqlite::Connection) -> rusqlite::Result<()> { // Check if schema_version table exists - let mut stmt = conn.prepare( - "SELECT name FROM sqlite_master WHERE type='table' AND name='schema_version'", - )?; + let mut stmt = conn + .prepare("SELECT name FROM sqlite_master WHERE type='table' AND name='schema_version'")?; let exists = stmt.exists([])?; @@ -43,7 +40,10 @@ fn check_and_migrate(conn: &rusqlite::Connection) -> rusqlite::Result<()> { // Return an error to signal this condition return Err(rusqlite::Error::SqliteFailure( rusqlite::ffi::Error::new(1), - Some("database newer than binary".to_string()), + Some( + "your database was created by a newer version of rustagent, please upgrade" + .to_string(), + ), )); } @@ -54,6 +54,9 @@ fn check_and_migrate(conn: &rusqlite::Connection) -> rusqlite::Result<()> { /// Create the initial database schema (version 1) fn create_schema_v1(conn: &rusqlite::Connection) -> rusqlite::Result<()> { + // Wrap schema creation in a transaction for atomicity using BEGIN/COMMIT + conn.execute_batch("BEGIN IMMEDIATE")?; + // Schema version table conn.execute( "CREATE TABLE schema_version ( @@ -65,10 +68,7 @@ fn create_schema_v1(conn: &rusqlite::Connection) -> rusqlite::Result<()> { conn.execute( "INSERT INTO schema_version (version, migrated_at) VALUES (?, ?)", - [ - "1", - &chrono::Utc::now().to_rfc3339(), - ], + ["1", &chrono::Utc::now().to_rfc3339()], )?; // Projects table @@ -106,18 +106,9 @@ fn create_schema_v1(conn: &rusqlite::Connection) -> rusqlite::Result<()> { [], )?; - conn.execute( - "CREATE INDEX idx_nodes_project ON nodes(project_id)", - [], - )?; - conn.execute( - "CREATE INDEX idx_nodes_type ON nodes(node_type)", - [], - )?; - conn.execute( - "CREATE INDEX idx_nodes_status ON nodes(status)", - [], - )?; + conn.execute("CREATE INDEX idx_nodes_project ON nodes(project_id)", [])?; + conn.execute("CREATE INDEX idx_nodes_type ON nodes(node_type)", [])?; + conn.execute("CREATE INDEX idx_nodes_status ON nodes(status)", [])?; // Edges table (unified relationships) conn.execute( @@ -132,18 +123,9 @@ fn create_schema_v1(conn: &rusqlite::Connection) -> rusqlite::Result<()> { [], )?; - conn.execute( - "CREATE INDEX idx_edges_from ON edges(from_node)", - [], - )?; - conn.execute( - "CREATE INDEX idx_edges_to ON edges(to_node)", - [], - )?; - conn.execute( - "CREATE INDEX idx_edges_type ON edges(edge_type)", - [], - )?; + conn.execute("CREATE INDEX idx_edges_from ON edges(from_node)", [])?; + conn.execute("CREATE INDEX idx_edges_to ON edges(to_node)", [])?; + conn.execute("CREATE INDEX idx_edges_type ON edges(edge_type)", [])?; // Sessions table (temporal) conn.execute( @@ -214,5 +196,6 @@ fn create_schema_v1(conn: &rusqlite::Connection) -> rusqlite::Result<()> { [], )?; + conn.execute_batch("COMMIT")?; Ok(()) } diff --git a/src/main.rs b/src/main.rs index 2bba51e..74829a1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,6 +1,6 @@ -use rustagent::{config, logging, planning, ralph, project, db}; +use rustagent::{config, db, logging, planning, project, ralph}; -use clap::{Parser, Subcommand, CommandFactory}; +use clap::{CommandFactory, Parser, Subcommand}; use std::path::PathBuf; #[derive(Parser)] @@ -197,9 +197,15 @@ async fn main() -> anyhow::Result<()> { match action { ProjectAction::Add { name, path } => { - let proj = store.add(&name, std::path::Path::new(&path)).await?; - println!("Registered project '{}' ({}) at {}", - proj.name, proj.id, proj.path.display()); + let path_obj = std::path::Path::new(&path); + let canonical_path = path_obj.canonicalize()?; + let proj = store.add(&name, &canonical_path).await?; + println!( + "Registered project '{}' ({}) at {}", + proj.name, + proj.id, + proj.path.display() + ); } ProjectAction::List => { let projects = store.list().await?; @@ -219,29 +225,25 @@ async fn main() -> anyhow::Result<()> { } } } - ProjectAction::Show { name } => { - match store.get_by_name(&name).await? { - Some(proj) => { - println!("Project: {}", proj.name); - println!(" ID: {}", proj.id); - println!(" Path: {}", proj.path.display()); - println!(" Registered: {}", proj.registered_at); - } - None => { - println!("Project '{}' not found", name); - } + ProjectAction::Show { name } => match store.get_by_name(&name).await? { + Some(proj) => { + println!("Project: {}", proj.name); + println!(" ID: {}", proj.id); + println!(" Path: {}", proj.path.display()); + println!(" Registered: {}", proj.registered_at); } - } - ProjectAction::Remove { name } => { - match store.remove(&name).await? { - true => { - println!("Removed project '{}'", name); - } - false => { - println!("Project '{}' not found", name); - } + None => { + println!("Project '{}' not found", name); } - } + }, + ProjectAction::Remove { name } => match store.remove(&name).await? { + true => { + println!("Removed project '{}'", name); + } + false => { + println!("Project '{}' not found", name); + } + }, } } } diff --git a/src/project.rs b/src/project.rs index 2349a16..43e57c3 100644 --- a/src/project.rs +++ b/src/project.rs @@ -40,9 +40,8 @@ impl ProjectStore { let result = db .connection() .call(move |conn| { - let tx = conn.transaction_with_behavior( - rusqlite::TransactionBehavior::Immediate, - )?; + let tx = + conn.transaction_with_behavior(rusqlite::TransactionBehavior::Immediate)?; let id = generate_project_id(); let now = Utc::now(); @@ -187,12 +186,11 @@ impl ProjectStore { if project.path == path_buf { return Ok(Some(project)); } - if let Ok(canonical_stored) = project.path.canonicalize() { - if let Some(ref canonical_query) = canonical_query { - if canonical_stored == *canonical_query { - return Ok(Some(project)); - } - } + if let Ok(canonical_stored) = project.path.canonicalize() + && let Some(ref canonical_query_ref) = canonical_query + && canonical_stored == *canonical_query_ref + { + return Ok(Some(project)); } } Ok(None) @@ -212,12 +210,13 @@ impl ProjectStore { let result = db .connection() .call(move |conn| { - let tx = conn.transaction_with_behavior( - rusqlite::TransactionBehavior::Immediate, - )?; + let tx = + conn.transaction_with_behavior(rusqlite::TransactionBehavior::Immediate)?; - let rows_affected = - tx.execute("DELETE FROM projects WHERE name = ?", rusqlite::params![&name])?; + let rows_affected = tx.execute( + "DELETE FROM projects WHERE name = ?", + rusqlite::params![&name], + )?; tx.commit()?; diff --git a/src/ralph/mod.rs b/src/ralph/mod.rs index e60b056..ab3dae1 100644 --- a/src/ralph/mod.rs +++ b/src/ralph/mod.rs @@ -139,7 +139,6 @@ impl RalphLoop { Ok(()) } - async fn execute_task(&self, task_id: &str) -> Result { let context = self.build_context(task_id)?; let tool_definitions = self.tools.definitions(); diff --git a/tests/db_test.rs b/tests/db_test.rs index ada9db4..42ef2e2 100644 --- a/tests/db_test.rs +++ b/tests/db_test.rs @@ -67,9 +67,8 @@ async fn test_database_schema_tables_exist() { let conn = db.connection(); let tables = conn .call(|c| { - let mut stmt = c.prepare( - "SELECT name FROM sqlite_master WHERE type='table' ORDER BY name", - )?; + let mut stmt = + c.prepare("SELECT name FROM sqlite_master WHERE type='table' ORDER BY name")?; let tables = stmt .query_map([], |row| row.get::<_, String>(0))? .collect::, _>>()?; @@ -124,10 +123,7 @@ async fn test_projects_table_has_correct_schema() { let mut stmt = c.prepare("PRAGMA table_info(projects)")?; let cols = stmt .query_map([], |row| { - Ok(( - row.get::<_, String>(1)?, - row.get::<_, String>(2)?, - )) + Ok((row.get::<_, String>(1)?, row.get::<_, String>(2)?)) })? .collect::, _>>()?; Ok(cols) @@ -135,7 +131,14 @@ async fn test_projects_table_has_correct_schema() { .await .unwrap(); - let expected_cols = vec!["id", "name", "path", "registered_at", "config_overrides", "metadata"]; + let expected_cols = vec![ + "id", + "name", + "path", + "registered_at", + "config_overrides", + "metadata", + ]; for (col_name, _) in columns { assert!( @@ -215,9 +218,8 @@ async fn test_triggers_exist() { let conn = db.connection(); let triggers = conn .call(|c| { - let mut stmt = c.prepare( - "SELECT name FROM sqlite_master WHERE type='trigger' ORDER BY name", - )?; + let mut stmt = + c.prepare("SELECT name FROM sqlite_master WHERE type='trigger' ORDER BY name")?; let triggers = stmt .query_map([], |row| row.get::<_, String>(0))? .collect::, _>>()?; @@ -316,3 +318,32 @@ async fn test_indexes_exist() { ); } } + +#[tokio::test] +async fn test_newer_database_version_error() { + let db = Database::open_in_memory().await.unwrap(); + + // Manually set version to 999 to simulate a newer database + let conn = db.connection(); + conn.call(|c| { + c.execute("UPDATE schema_version SET version = 999", [])?; + Ok(()) + }) + .await + .unwrap(); + + // Now try to run migrations again - should fail with error containing "newer" + let result = conn + .call(|c| { + rustagent::db::migrations::check_and_migrate(c).map_err(tokio_rusqlite::Error::Rusqlite) + }) + .await; + + assert!(result.is_err(), "Expected error for newer database version"); + let error_msg = result.unwrap_err().to_string(); + assert!( + error_msg.contains("newer"), + "Error message should contain 'newer', got: {}", + error_msg + ); +} diff --git a/tests/project_test.rs b/tests/project_test.rs index 818c9bf..e6d91fb 100644 --- a/tests/project_test.rs +++ b/tests/project_test.rs @@ -34,9 +34,7 @@ async fn test_add_duplicate_name_fails() { .await .expect("first add should succeed"); - let result = store - .add("my-api", Path::new("/tmp/test2")) - .await; + let result = store.add("my-api", Path::new("/tmp/test2")).await; assert!(result.is_err(), "adding duplicate name should fail"); } -- 2.51.2