diff --git a/crates/idx-core/src/ownership/snapshot.rs b/crates/idx-core/src/ownership/snapshot.rs index d23af4d..3b26a18 100644 --- a/crates/idx-core/src/ownership/snapshot.rs +++ b/crates/idx-core/src/ownership/snapshot.rs @@ -93,7 +93,9 @@ pub struct OwnershipSyncResult { struct LocalSnapshotState { /// SHA-256 of the database file. Queries leave a synced file /// byte-identical, so this identifies an unmodified published snapshot. - file_sha256: String, + /// `None` when committed changes could not be checkpointed out of the + /// WAL, so the file alone does not show the database's contents. + file_sha256: Option, latest_release: Option, release_count: usize, } @@ -338,11 +340,14 @@ fn inspect_local_db(db_path: &Path) -> Result, IdxErr return Ok(None); } - // Hash before opening: schema setup may write to an empty database. - let bytes = fs::read(db_path) - .map_err(|e| IdxError::Io(format!("failed to read {}: {e}", db_path.display())))?; - let file_sha256 = sha256_hex(&bytes); - drop(bytes); + let file_sha256 = if checkpoint_wal(db_path) { + // Hash before opening: schema setup may write to an empty database. + let bytes = fs::read(db_path) + .map_err(|e| IdxError::Io(format!("failed to read {}: {e}", db_path.display())))?; + Some(sha256_hex(&bytes)) + } else { + None + }; let conn = Connection::open(db_path).map_err(|e| { IdxError::DatabaseError(format!("failed to open {}: {e}", db_path.display())) @@ -357,6 +362,20 @@ fn inspect_local_db(db_path: &Path) -> Result, IdxErr })) } +/// Moves committed WAL frames into the database file, so its hash covers local +/// edits too. Returns false when changes remain in the WAL. +fn checkpoint_wal(db_path: &Path) -> bool { + let wal_path = sidecar_path(db_path, "-wal"); + let wal_is_empty = || fs::metadata(&wal_path).map_or(true, |meta| meta.len() == 0); + if wal_is_empty() { + return true; + } + if let Ok(conn) = Connection::open(db_path) { + let _ = conn.query_row("PRAGMA wal_checkpoint(TRUNCATE)", [], |_| Ok(())); + } + wal_is_empty() +} + fn build_sync_decision( local: Option<&LocalSnapshotState>, snapshot: &OwnershipSnapshotArtifact, @@ -414,7 +433,8 @@ fn build_sync_decision( }; } - if local.file_sha256 == snapshot.sqlite_sha256 { + let file_sha256 = local.file_sha256.as_deref(); + if file_sha256 == Some(snapshot.sqlite_sha256.as_str()) { return SyncDecision { action: OwnershipSyncAction::NoChange, should_download: false, @@ -429,7 +449,7 @@ fn build_sync_decision( if snapshot .replaces_sqlite_sha256 .iter() - .any(|sha256| sha256.eq_ignore_ascii_case(&local.file_sha256)) + .any(|sha256| file_sha256.is_some_and(|local| sha256.eq_ignore_ascii_case(local))) { return SyncDecision { action: OwnershipSyncAction::Updated, @@ -657,19 +677,23 @@ fn build_backup_path(db_path: &Path) -> PathBuf { } } +fn sidecar_path(db_path: &Path, suffix: &str) -> PathBuf { + let file_name = format!( + "{}{suffix}", + db_path + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or("ownership.db") + ); + match db_path.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent.join(&file_name), + _ => PathBuf::from(&file_name), + } +} + fn remove_sqlite_sidecars(db_path: &Path) -> Result<(), IdxError> { for suffix in ["-wal", "-shm"] { - let file_name = format!( - "{}{suffix}", - db_path - .file_name() - .and_then(|name| name.to_str()) - .unwrap_or("ownership.db") - ); - let sidecar = match db_path.parent() { - Some(parent) if !parent.as_os_str().is_empty() => parent.join(&file_name), - _ => PathBuf::from(&file_name), - }; + let sidecar = sidecar_path(db_path, suffix); if sidecar.exists() { fs::remove_file(&sidecar).map_err(|e| { @@ -807,8 +831,7 @@ mod tests { replaces_sqlite_sha256: Vec::new(), }; let local = super::LocalSnapshotState { - file_sha256: "dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd" - .to_string(), + file_sha256: Some("d".repeat(64)), latest_release: Some(OwnershipRelease { id: 1, source_url: None, @@ -849,7 +872,7 @@ mod tests { file_sha256: &str, ) -> super::LocalSnapshotState { super::LocalSnapshotState { - file_sha256: file_sha256.to_string(), + file_sha256: Some(file_sha256.to_string()), latest_release: Some(OwnershipRelease { id: 3, source_url: None, @@ -1036,4 +1059,49 @@ mod tests { .unwrap(); assert_eq!(release_count, 1); } + + #[test] + fn local_hash_covers_committed_changes_still_in_the_wal() { + let dir = std::env::temp_dir().join(format!( + "idx-snapshot-wal-{}-{}", + std::process::id(), + fastrand::u64(..) + )); + std::fs::create_dir_all(&dir).unwrap(); + let db_path = dir.join("ownership.db"); + let release = |sha256: &str| OwnershipRelease { + id: 0, + source_url: None, + sha256: sha256.to_string(), + as_of_date: NaiveDate::from_ymd_opt(2026, 9, 30).unwrap(), + row_count: 1, + imported_at: 1, + }; + { + let conn = Connection::open(&db_path).unwrap(); + ensure_schema(&conn).unwrap(); + insert_release(&conn, &release("published")).unwrap(); + } + let published = super::sha256_hex(&std::fs::read(&db_path).unwrap()); + + // A committed edit that stays in the WAL: the main file still hashes + // as the published build while the connection is open. + let editor = Connection::open(&db_path).unwrap(); + ensure_schema(&editor).unwrap(); + editor + .query_row("PRAGMA wal_autocheckpoint = 0", [], |_| Ok(())) + .unwrap(); + insert_release(&editor, &release("local edit")).unwrap(); + assert_eq!( + super::sha256_hex(&std::fs::read(&db_path).unwrap()), + published + ); + + let local = super::inspect_local_db(&db_path).unwrap().unwrap(); + assert_ne!(local.file_sha256.as_deref(), Some(published.as_str())); + assert_eq!(local.release_count, 2); + + drop(editor); + let _ = std::fs::remove_dir_all(&dir); + } } diff --git a/crates/idx-mcp/src/server.rs b/crates/idx-mcp/src/server.rs index e9412d2..ac817b2 100644 --- a/crates/idx-mcp/src/server.rs +++ b/crates/idx-mcp/src/server.rs @@ -99,6 +99,9 @@ for one or more IDX tickers." )] async fn get_quotes(&self, params: Parameters) -> ToolResult { let SymbolsParams { symbols } = params.0; + if let Some(err) = empty_input("symbols", &symbols) { + return err; + } self.live(move |idx| { let quotes = idx.market().quotes(&symbols, CachePolicy::Default)?; Ok(json!({ "quotes": quotes })) @@ -155,6 +158,9 @@ earnings, analyst sentiment, insights, and news. Sections that fail are listed u async fn get_company(&self, params: Parameters) -> ToolResult { let CompanyParams { symbol, sections } = params.0; let sections = sections.unwrap_or_else(|| CompanySection::ALL.to_vec()); + if let Some(err) = empty_input("sections", §ions) { + return err; + } self.live(move |idx| { let financial = idx.financial(); let policy = CachePolicy::Default; @@ -195,6 +201,9 @@ are listed under `errors`; the call fails only if every analysis fails." async fn analyze_stock(&self, params: Parameters) -> ToolResult { let AnalyzeParams { symbol, kinds } = params.0; let kinds = kinds.unwrap_or_else(|| AnalysisKind::ALL.to_vec()); + if let Some(err) = empty_input("kinds", &kinds) { + return err; + } self.live(move |idx| { let analysis = idx.analysis(); let policy = CachePolicy::Default; @@ -224,6 +233,9 @@ tickers. Tickers that fail are listed under `failures`." )] async fn compare_stocks(&self, params: Parameters) -> ToolResult { let SymbolsParams { symbols } = params.0; + if let Some(err) = empty_input("symbols", &symbols) { + return err; + } self.live(move |idx| { let comparison = idx.analysis().compare(&symbols, CachePolicy::Default)?; let failures: Vec = comparison @@ -377,6 +389,17 @@ where }) } +/// An explicitly empty list is an input error, as in the CLI, rather than an +/// empty success an agent could mistake for data. +fn empty_input(field: &str, values: &[T]) -> Option { + values.is_empty().then(|| { + let err = IdxError::InvalidInput(format!("{field} must not be empty")); + Ok(CallToolResult::structured_error(Value::Object(error_body( + &err, + )))) + }) +} + fn to_json(value: T) -> Result { serde_json::to_value(value).map_err(|err| IdxError::ParseError(err.to_string())) } diff --git a/crates/idx-mcp/tests/mcp.rs b/crates/idx-mcp/tests/mcp.rs index 8698570..9581448 100644 --- a/crates/idx-mcp/tests/mcp.rs +++ b/crates/idx-mcp/tests/mcp.rs @@ -243,6 +243,17 @@ fn errors_carry_cli_codes() { assert!(is_error); assert_eq!(body["code"], "INVALIDINPUT"); + for (tool, arguments) in [ + ("get_quotes", json!({"symbols": []})), + ("compare_stocks", json!({"symbols": []})), + ("get_company", json!({"symbol": "BBCA", "sections": []})), + ("analyze_stock", json!({"symbol": "BBCA", "kinds": []})), + ] { + let (is_error, body) = client.call(tool, arguments); + assert!(is_error, "{tool}: {body}"); + assert_eq!(body["code"], "INVALIDINPUT", "{tool}"); + } + // Empty ownership database: investor tools point at the sync path. let status = client.ok("ownership_status", json!({})); assert_eq!(status["populated"], false); diff --git a/docs/OWNERSHIP_SYNC.md b/docs/OWNERSHIP_SYNC.md index e589ce4..92215e8 100644 --- a/docs/OWNERSHIP_SYNC.md +++ b/docs/OWNERSHIP_SYNC.md @@ -76,7 +76,8 @@ Without `--force`, sync behaves conservatively: - local latest release older than snapshot: replace with snapshot - local DB file is byte-identical to `sqlite_sha256`: no-op - local DB file is byte-identical to a build in `replaces_sqlite_sha256`: replace - (queries never write to a synced file, so this means it was not modified) + (queries never write to a synced file, so this means it was not modified; + committed changes still in the `-wal` file are checkpointed before hashing) - local latest release/date/count matches snapshot: no-op - local latest release matches but has fewer releases than snapshot: replace to fill missing history - local DB newer than snapshot: no-op and keep local data