dabb3d2c79
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) <noreply@anthropic.com>
266 lines
9.5 KiB
Markdown
266 lines
9.5 KiB
Markdown
# 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<PackOutcome, AppError> {
|
||
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<Vec<f32>> = entries
|
||
.iter()
|
||
.map(|e| {
|
||
store
|
||
.get(&e.text)
|
||
.ok_or_else(|| AppError::Io("cached vector vanished during pack".to_string()))
|
||
})
|
||
.collect::<Result<Vec<_>, _>>()?;
|
||
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.
|