From dabb3d2c79b977b948052e0fd035cf14ea76384c Mon Sep 17 00:00:00 2001 From: Brummel Date: Mon, 1 Jun 2026 01:30:19 +0200 Subject: [PATCH] plan: extract the index --pack writer into the packed module (refs #5) Behaviour-preserving refactor (tracker issue #5, no spec): move the inline pack logic from main into packed::pack_from_store, returning a PackOutcome {Packed(PackedHeader), Incomplete{cached,total}} so the header construction is unit-testable without spawning the binary. main's pack branch shrinks to load-entries + call + match (same stderr messages + exit 2). The four existing pack CLI tests are the behaviour-parity guard; two new unit tests cover the complete/incomplete outcomes directly. refs #5 Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/plans/2026-06-01-extract-pack-writer.md | 265 +++++++++++++++++++ 1 file changed, 265 insertions(+) create mode 100644 docs/plans/2026-06-01-extract-pack-writer.md diff --git a/docs/plans/2026-06-01-extract-pack-writer.md b/docs/plans/2026-06-01-extract-pack-writer.md new file mode 100644 index 0000000..b99cd66 --- /dev/null +++ b/docs/plans/2026-06-01-extract-pack-writer.md @@ -0,0 +1,265 @@ +# Extract the `--pack` Writer into the packed Module — Implementation Plan + +> **Parent spec:** none — tracker issue #5 (a behaviour-preserving +> refactor of shipped code; the issue body is the contract). The packed +> format itself is specced in +> `docs/specs/2026-05-31-packed-versioned-index-bundle.md`. +> +> **For agentic workers:** REQUIRED SUB-SKILL: use the `implement` +> skill to run this plan. Steps use `- [ ]` checkboxes for tracking. + +**Goal:** Move the `index --pack` writer logic out of `main` into a +`packed::pack_from_store` library fn so the header construction is +unit-testable without spawning the binary, with zero behaviour change. + +**Architecture:** A new `PackOutcome { Packed(PackedHeader), Incomplete +{ cached, total } }` and `pack_from_store(index_dir, embed_model, +alpha_id_path, entries)` in `src/packed.rs` own the completeness check, row +collection in corpus order, header derivation, and the write. The binary's +pack branch shrinks to: load entries, call the fn, match the outcome to the +same stderr messages and exit code it emits today. + +**Tech Stack:** Rust; `src/packed.rs` (new fn + enum), `src/bin/alpha_id.rs` +(rewire), `tests/packed_tests.rs` (new unit tests); reuses the existing +`EmbeddingStore`, `corpus_sha256`, `write_packed`, `packed_path`. + +**Scope boundary:** Behaviour-preserving refactor only. No change to the +load path, `IndexStatus`, the packed format, or `mmap`. The four existing +pack CLI tests (`pack_refuses_incomplete_store_exit_2`, +`pack_writes_file_from_complete_store`, `cli_packed_file_loads_back_as_ok_index`, +`cli_packed_rows_are_in_corpus_order`) MUST stay green unchanged — they are +the behaviour-parity guard. + +--- + +**Files this plan creates or modifies:** + +- Modify: `src/packed.rs` — add `PackOutcome` enum and `pack_from_store`; + extend the imports to `EmbeddingStore` and `AlphaIdEntry`. +- Modify: `src/bin/alpha_id.rs:87-117` — replace the inline pack body with a + call to `pack_from_store` and an outcome match. +- Test: `tests/packed_tests.rs` — unit tests for the complete and incomplete + outcomes of `pack_from_store`. + +--- + +### Task 1: Extract `pack_from_store` and rewire the binary + +**Files:** +- Modify: `src/packed.rs` +- Modify: `src/bin/alpha_id.rs:87-117` +- Test: `tests/packed_tests.rs` + +- [ ] **Step 1: Write the failing unit tests** + +Append to `tests/packed_tests.rs`. Add these imports at the top of the file +(it currently imports `corpus_sha256, read_packed, write_packed, +PackedHeader` from `alpha_id::packed`): + +```rust +use alpha_id::embed::EmbeddingStore; +use alpha_id::model::AlphaIdEntry; +use alpha_id::packed::{pack_from_store, packed_path, PackOutcome}; +``` + +Then append: + +```rust +fn mk_entry(text: &str) -> AlphaIdEntry { + AlphaIdEntry { + alpha_id: "A0001".to_string(), + valid: true, + icd_primary: "A00".to_string(), + icd_star: String::new(), + icd_addon: String::new(), + icd_primary2: String::new(), + orpha: String::new(), + text: text.to_string(), + } +} + +#[test] +fn pack_from_store_complete_builds_header_and_writes_file() { + let dir = tempfile::tempdir().unwrap(); + let index_dir = dir.path().join("index"); + std::fs::create_dir_all(&index_dir).unwrap(); + let corpus = dir.path().join("corpus.txt"); + std::fs::write(&corpus, b"corpus bytes").unwrap(); + + let store = EmbeddingStore::open(index_dir.to_str().unwrap(), "test/model").unwrap(); + store.put("alpha", &[1.0f32, 0.0, 0.0]).unwrap(); + store.put("beta", &[0.0f32, 1.0, 0.0]).unwrap(); + let entries = vec![mk_entry("alpha"), mk_entry("beta")]; + + let outcome = pack_from_store( + index_dir.to_str().unwrap(), + "test/model", + corpus.to_str().unwrap(), + &entries, + ) + .unwrap(); + + match outcome { + PackOutcome::Packed(h) => { + assert_eq!(h.n_rows, 2); + assert_eq!(h.dim, 3); + assert_eq!(h.embed_model, "test/model"); + assert_eq!( + h.corpus_sha256, + alpha_id::packed::corpus_sha256(corpus.to_str().unwrap()).unwrap() + ); + } + PackOutcome::Incomplete { .. } => panic!("a complete store must pack"), + } + assert!(packed_path(index_dir.to_str().unwrap(), "test/model").exists()); +} + +#[test] +fn pack_from_store_incomplete_reports_counts_and_writes_nothing() { + let dir = tempfile::tempdir().unwrap(); + let index_dir = dir.path().join("index"); + std::fs::create_dir_all(&index_dir).unwrap(); + let corpus = dir.path().join("corpus.txt"); + std::fs::write(&corpus, b"corpus bytes").unwrap(); + + let store = EmbeddingStore::open(index_dir.to_str().unwrap(), "test/model").unwrap(); + store.put("alpha", &[1.0f32, 0.0]).unwrap(); // only 1 of 2 entries cached + let entries = vec![mk_entry("alpha"), mk_entry("beta")]; + + let outcome = pack_from_store( + index_dir.to_str().unwrap(), + "test/model", + corpus.to_str().unwrap(), + &entries, + ) + .unwrap(); + + assert!(matches!(outcome, PackOutcome::Incomplete { cached: 1, total: 2 })); + assert!( + !packed_path(index_dir.to_str().unwrap(), "test/model").exists(), + "an incomplete store must not write a packed file" + ); +} +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cargo test --test packed_tests` +Expected: FAIL — compile error: `pack_from_store` and `PackOutcome` are not +found in `alpha_id::packed` (they do not exist yet), which reddens the test +binary. + +- [ ] **Step 3: Add `PackOutcome` + `pack_from_store` in `src/packed.rs`** + +Extend the model import at the top of `src/packed.rs` — change: + +```rust +use crate::model::AppError; +``` + +to: + +```rust +use crate::embed::EmbeddingStore; +use crate::model::{AlphaIdEntry, AppError}; +``` + +Then add, after the `corpus_sha256` fn (before `write_packed`): + +```rust +/// Result of a pack attempt: either the header that was written, or a report +/// that the per-file store is missing some corpus entry's embedding (in which +/// case nothing is written, and the caller refuses loudly). +pub enum PackOutcome { + Packed(PackedHeader), + Incomplete { cached: usize, total: usize }, +} + +/// Pack a complete per-file store into the single packed matrix file at +/// `packed_path(index_dir, embed_model)`, returning the header written. Rows +/// are collected in `entries` order (row `i` = `entries[i]`), so the packed +/// matrix is corpus-aligned. If any entry's embedding is not cached, returns +/// `Incomplete { cached, total }` and writes nothing. +pub fn pack_from_store( + index_dir: &str, + embed_model: &str, + alpha_id_path: &str, + entries: &[AlphaIdEntry], +) -> Result { + let store = EmbeddingStore::open(index_dir, embed_model)?; + let cached = entries.iter().filter(|e| store.has(&e.text)).count(); + if cached != entries.len() { + return Ok(PackOutcome::Incomplete { cached, total: entries.len() }); + } + let rows: Vec> = entries + .iter() + .map(|e| { + store + .get(&e.text) + .ok_or_else(|| AppError::Io("cached vector vanished during pack".to_string())) + }) + .collect::, _>>()?; + let dim = rows.first().map(|r| r.len()).unwrap_or(0) as u32; + let corpus_name = Path::new(alpha_id_path) + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or(alpha_id_path) + .to_string(); + let header = PackedHeader { + n_rows: entries.len() as u32, + dim, + embed_model: embed_model.to_string(), + corpus_sha256: corpus_sha256(alpha_id_path)?, + corpus_name, + }; + write_packed(&packed_path(index_dir, embed_model), &header, &rows)?; + Ok(PackOutcome::Packed(header)) +} +``` + +- [ ] **Step 4: Rewire the binary's pack branch** + +In `src/bin/alpha_id.rs`, replace the inline pack body (`src/bin/alpha_id.rs:87-117`, +the whole `if pack { … }` block) with: + +```rust + if pack { + use alpha_id::packed::{pack_from_store, packed_path, PackOutcome}; + let entries = alpha_id::corpus::load(&cfg.alpha_id_path).expect("corpus"); + match pack_from_store(&cfg.index_dir, &cfg.embed_model, &cfg.alpha_id_path, &entries) + .expect("pack") + { + PackOutcome::Incomplete { cached, total } => { + eprintln!( + "error: store incomplete: {} of {} entries embedded; \ + run `index --full --confirm` first", + cached, total + ); + std::process::exit(2); + } + PackOutcome::Packed(header) => { + let path = packed_path(&cfg.index_dir, &cfg.embed_model); + eprintln!("packed {} rows × {} dims → {}", header.n_rows, header.dim, path.display()); + } + } + return; + } +``` + +(The `use alpha_id::embed::{EmbeddingStore, build_corpus_embeddings, estimate};` +line at `src/bin/alpha_id.rs:84` stays — the `sample` / `full` branches below +still use it.) + +- [ ] **Step 5: Run the new unit tests to verify they pass** + +Run: `cargo test --test packed_tests` +Expected: PASS — the two new tests plus the existing format tests all pass. + +- [ ] **Step 6: Run the full suite to verify behaviour parity** + +Run: `cargo test` +Expected: PASS — the whole suite is green. In particular the four pack CLI +tests (`pack_refuses_incomplete_store_exit_2`, +`pack_writes_file_from_complete_store`, `cli_packed_file_loads_back_as_ok_index`, +`cli_packed_rows_are_in_corpus_order`) pass unchanged, confirming the +extraction preserved behaviour.