From 26d3a9941b9375ff269c01a5a5e29c9657baf207 Mon Sep 17 00:00:00 2001 From: Stephen Waits Date: Mon, 26 Oct 2026 22:45:17 -0600 Subject: [PATCH] fix(package): books.start runs the real integrity check (U0b) books.start passed Integrity::default() to the attention engine, so it asserted "database matches, no file failures". On a book whose database differs from the journal, or whose stored file is missing or corrupt, the viewer showed the database/stored-files items and the server did not: the two lists diverged exactly where they matter most. Call find, which computes the check: every distinct stored blob is read and hash-checked once (a damaged blob is still reported per attachment filed from it), and Store::disk_check now makes the viewer's comparison through a snapshot of its data directory instead of reporting a match. The parity test now damages its own fixture (removes the receipt's blob, then changes a projected account row) and asserts both lists still agree item for item and both name the damage. On the demo books (138 attachments, 138 distinct blobs, debug build) books.start takes about 0.26 s warm, 0.29 s cold, so the real check costs no decision. --- crates/ledger-cli/tests/attention_parity.rs | 106 ++++++++++++++++++-- crates/ledger-mcp/src/tools/kept.rs | 7 +- crates/ledger-package/src/attention.rs | 15 ++- crates/ledger-package/src/books.rs | 16 ++- docs/testing.md | 18 +++- plan/served.md | 11 +- 6 files changed, 146 insertions(+), 27 deletions(-) diff --git a/crates/ledger-cli/tests/attention_parity.rs b/crates/ledger-cli/tests/attention_parity.rs index 9ae3c8d..0a14ec4 100644 --- a/crates/ledger-cli/tests/attention_parity.rs +++ b/crates/ledger-cli/tests/attention_parity.rs @@ -1,14 +1,19 @@ //! The viewer's "Needs attention" and `books.start`'s next steps are one //! list: both come from `ledger_package::attention`, the viewer over its //! `Snapshot`, the server over its `Store`. This drives both on the same -//! books (the demo company, and a book written entry by entry) and compares -//! them item for item. +//! books (the demo company, a book written entry by entry, and that book +//! after its stored file and its database are damaged) and compares them +//! item for item. #![cfg(feature = "gui")] +use std::collections::HashSet; +use std::time::Instant; + use chrono::NaiveDate; use ledger_core::{AccountType, Currency, Money, Side}; use ledger_gui::{Source, data}; use rust_decimal::Decimal; +use serde_json::Value; use serde_json::json; mod common; @@ -20,13 +25,18 @@ fn usd(cents: i64) -> Money { Money::new(Decimal::new(cents, 2), Currency::USD) } +fn start(wire: &mut common::Wire) -> Value { + common::answer_of(&wire.call("books.start", json!({"focus": "daily"}))) + .unwrap_or_else(|e| panic!("books.start: {e}")) +} + /// The items the viewer draws and the next steps the server lists: the /// server's attention steps are the ones whose `why` is one of the viewer's /// titles, and they must be the same titles, in the same order. The viewer -/// has at most 12 (books.start lists the top 12). -fn compare(place: &common::Place, wire: &mut common::Wire, today: NaiveDate) { - let answer = common::answer_of(&wire.call("books.start", json!({"focus": "daily"}))) - .unwrap_or_else(|e| panic!("books.start: {e}")); +/// has at most 12 (books.start lists the top 12). Returns the viewer's +/// titles. +fn compare(place: &common::Place, wire: &mut common::Wire, today: NaiveDate) -> Vec { + let answer = start(wire); let books = data::load_once(&Source { data_dir: place.books_data(), today: Some(today), @@ -53,11 +63,13 @@ fn compare(place: &common::Place, wire: &mut common::Wire, today: NaiveDate) { assert_eq!(&a.title, title); assert!(a.prompt.contains("wanderbooks"), "{}", a.prompt); } + viewer } /// A book written entry by entry, pseudo-randomly but deterministically (a /// fixed seed, so a failure reproduces): a chart, three drafts, posted -/// entries without receipts, and one that names no counterparty. +/// entries without receipts, one that names no counterparty, and one with +/// a filed receipt (the blob the damage test removes). fn write_random(dir: &std::path::Path, today: NaiveDate) { let _ = std::fs::remove_dir_all(dir); let mut st = ledger_store::Store::create(dir, Currency::USD).unwrap(); @@ -132,6 +144,29 @@ fn write_random(dir: &std::path::Path, today: NaiveDate) { drop(st); } +/// A blob's file in the content-addressed store (`//`). +fn blob_path(data_dir: &std::path::Path, sha: &str) -> std::path::PathBuf { + data_dir + .join("blobs") + .join(&sha[0..2]) + .join(&sha[2..4]) + .join(sha) +} + +/// The viewer's items and the server's steps, and both name `expected`. +fn compare_and_expect( + place: &common::Place, + wire: &mut common::Wire, + today: NaiveDate, + expected: &str, +) { + let viewer = compare(place, wire, today); + assert!( + viewer.iter().any(|t| t.contains(expected)), + "the viewer should say {expected:?}: {viewer:?}" + ); +} + #[test] fn the_viewer_and_books_start_see_the_same_attention() { let today = ledger_core::settings::today_in(None); @@ -150,13 +185,68 @@ fn the_viewer_and_books_start_see_the_same_attention() { let mut wire = place.start(None); wire.initialize("attention-parity"); compare(&place, &mut wire, today); + // What the real integrity check costs books.start: the cold call + // (which also opens the store) and a warm one, and how many stored + // files it hashes (one per distinct blob, as the viewer does). + let books = data::load_once(&Source { + data_dir: place.books_data(), + today: Some(today), + }) + .unwrap(); + let blobs: HashSet<&str> = books + .attachments + .iter() + .map(|a| a.blob_sha.as_str()) + .collect(); + let cold = Instant::now(); + let _ = start(&mut wire); + let cold = cold.elapsed(); + let warm = Instant::now(); + let _ = start(&mut wire); + let warm = warm.elapsed(); + println!( + "books.start on the demo: {} attachments, {} distinct blobs; cold {cold:?}, warm {warm:?}", + books.attachments.len(), + blobs.len() + ); } - // A book written entry by entry. + // A book written entry by entry, then damaged: a stored file removed + // and the database changed from outside. Both lists must still agree, + // and both must name the damage (a clean-books-only test missed this). { let place = common::Place::new("attention-parity-random"); write_random(&place.books_data(), today); let mut wire = place.start(Some(SETTINGS)); wire.initialize("attention-parity"); compare(&place, &mut wire, today); + + // The stored file: remove its bytes (the blob store never deletes; + // the test does, on its own books). + let books = data::load_once(&Source { + data_dir: place.books_data(), + today: Some(today), + }) + .unwrap(); + let sha = books + .attachments + .first() + .expect("the fixture filed a receipt") + .blob_sha + .clone(); + let path = blob_path(&place.books_data(), &sha); + std::fs::remove_file(&path).unwrap(); + compare_and_expect(&place, &mut wire, today, "damaged or missing"); + + // The database: change a projected row the journal doesn't say. + let db = ledger_store::database_in_use(&place.books_data()); + let conn = rusqlite::Connection::open(&db).unwrap(); + conn.execute( + "UPDATE accounts SET name = name || ' (changed outside the journal)' \ + WHERE id = (SELECT MIN(id) FROM accounts)", + [], + ) + .unwrap(); + drop(conn); + compare_and_expect(&place, &mut wire, today, "doesn't match the journal"); } } diff --git a/crates/ledger-mcp/src/tools/kept.rs b/crates/ledger-mcp/src/tools/kept.rs index fffd0fd..96c622e 100644 --- a/crates/ledger-mcp/src/tools/kept.rs +++ b/crates/ledger-mcp/src/tools/kept.rs @@ -834,12 +834,7 @@ impl BooksServer { financing_accounts: &cfg.roles.financing, investing_accounts: &cfg.roles.investing, }; - let attention = ledger_package::find_with( - store, - today, - &attention_settings, - &ledger_package::Integrity::default(), - ); + let attention = ledger_package::find(store, today, &attention_settings); for a in attention.iter().filter(|a| a.tool.is_some()).take(12) { let Some((tool, args)) = &a.tool else { continue; diff --git a/crates/ledger-package/src/attention.rs b/crates/ledger-package/src/attention.rs index 2c6fb73..ab7f33f 100644 --- a/crates/ledger-package/src/attention.rs +++ b/crates/ledger-package/src/attention.rs @@ -192,12 +192,21 @@ impl Default for Integrity { impl Integrity { /// Check the database and every stored file: the attachment's bytes - /// are read (and hash-checked) through the books. + /// are read (and hash-checked) through the books, once per blob that + /// is intact; a damaged blob is reported for each attachment filed + /// from it. pub fn check(books: &impl PackageBooks) -> Integrity { + let mut intact: std::collections::HashSet = std::collections::HashSet::new(); let mut file_failures = Vec::new(); for a in books.all_attachments().unwrap_or_default() { - if let Err(e) = books.attachment_bytes(a.id) { - file_failures.push((a.id, e.to_string())); + if intact.contains(&a.blob_sha) { + continue; + } + match books.attachment_bytes(a.id) { + Ok(_) => { + intact.insert(a.blob_sha.clone()); + } + Err(e) => file_failures.push((a.id, e.to_string())), } } Integrity { diff --git a/crates/ledger-package/src/books.rs b/crates/ledger-package/src/books.rs index 839971e..deb0d4b 100644 --- a/crates/ledger-package/src/books.rs +++ b/crates/ledger-package/src/books.rs @@ -62,9 +62,8 @@ pub trait PackageBooks { fn reconciliations(&self) -> Result, StoreError>; /// How the writer's database compares with the journal. A snapshot - /// compares them; the store is the only writer and applies each line - /// before it returns (a failed apply heals on its next open), so it - /// reports a match. + /// compares them; the store makes the same comparison (see its + /// implementation). fn disk_check(&self) -> DiskCheck { DiskCheck::Matches } @@ -104,6 +103,17 @@ impl PackageBooks for Store { fn reconciliations(&self) -> Result, StoreError> { Store::reconciliations(self, None) } + + /// The same comparison the viewer makes, not an assumption: the + /// store's database is the file the viewer checks, and it can be + /// damaged or changed from outside. Loading a snapshot of the data + /// directory compares it against a fresh projection of the journal. + fn disk_check(&self) -> DiskCheck { + match Snapshot::load(Store::data_dir(self)) { + Ok(s) => Snapshot::check_disk(&s), + Err(e) => DiskCheck::Unreadable(e.to_string()), + } + } } impl PackageBooks for Snapshot { diff --git a/docs/testing.md b/docs/testing.md index 125c099..840fee7 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -2927,7 +2927,9 @@ steps, `why` equal to the title, at most 12. `PackageBooks` gained `settings`, `journal`, `statements`, `reconciliations` and `disk_check`, so the engine can read an entry's origin from the journal's event types (as the viewer's builder did) and the same statements, reconciliations and -database check through either view. +database check through either view. `Store::disk_check` makes the viewer's +comparison (a snapshot of the data directory) rather than assuming a +match; `books.start` calls `find`, which runs the real check. - **ledger-package (unit tests in `src/attention.rs`):** none of its own (the engine's conditions are exercised end to end); the crate's tests @@ -2941,9 +2943,13 @@ database check through either view. `tests/edges.rs`, `tests/old_books.rs`, `tests/parties.rs`, `tests/spec_gui.rs` exercise the items through the viewer's wrapper. - **ledger-cli:** `tests/attention_parity.rs` - `the_viewer_and_books_start_see_the_same_attention` (the demo books and a - book written entry by entry: the viewer's titles, in order, are exactly - `books.start`'s attention steps). + `the_viewer_and_books_start_see_the_same_attention` (the demo books, a + book written entry by entry, and that book after its stored file is + removed and its database is changed from outside: the viewer's titles, + in order, are exactly `books.start`'s attention steps, and both name the + `stored_files` and `database` items). The demo half measures + `books.start` with the real check (138 attachments and blobs: about + 0.26 s warm on a debug build). - **unchanged:** `crates/ledger-cli/tests/spec_kept.rs`'s `books.start` tests (the fixed steps keep their order; attention items sit between the backup step and the focus). @@ -2960,5 +2966,7 @@ not marked on file) and `files_1099`'s `any`; `paid_this_year`'s `max` `period_month`'s split; the severity arms (undetermined 1099, refuse tax category, parties require); `dollars`' thousands grouping and its negative-zero test; `filter(|a| a.tool.is_some()).take(12)` and -`step_title`'s 160-character cut in `start_out`; the viewer wrapper's +`step_title`'s 160-character cut in `start_out`; `Integrity::check`'s +intact-blob set (a damaged blob reported per attachment) and `Store:: +disk_check`'s snapshot load and `Unreadable` arm; the viewer wrapper's `Target` arms. diff --git a/plan/served.md b/plan/served.md index c285b80..fa6a457 100644 --- a/plan/served.md +++ b/plan/served.md @@ -1246,8 +1246,15 @@ Each unit follows the 8.0.4 template. "A-n" is an owner answer numbered in 8.3.1 - `books.start` lists at most 12 attention steps (the eventual `needs_attention` top 12; counts by kind come with P5), each step's `why` cut to 160 characters (a free-text description could otherwise - push the result past 8 KB), and passes `Integrity::default()`: the - server's own damage check is `books.verify`. + push the result past 8 KB), and calls `find` — the real integrity + check, so a damaged database or stored file produces the same items + for the server as for the viewer. `Store::disk_check` makes the + viewer's comparison (a snapshot of its data directory) rather than + assuming a match; the check hashes each distinct stored blob once and + replays the journal once. On the demo books (138 attachments, 138 + distinct blobs, debug build) `books.start` takes about 0.26 s warm, + 0.29 s cold; a real book's cost scales with its stored bytes, and a + cache would be the next step if that ever exceeds a second. - The W-9 and reconciliation prompts and the 1099 predicate moved to `ledger-package`; `pages/parties.rs`, `pages/reconciliation.rs` and `Books::files_1099` delegate to them (two files beyond the Build's