Introduce obikalgorithm crate to unify pipeline algorithms

Define a shared Algorithm trait with an associated Output type and a parameterless run(&mut self) method. Refactor PartitionRouter, Dereplicator, Counter, and LayerBuilder to implement the trait, standardizing receivers to &mut self and moving configuration and progress callbacks to dedicated builder setters. Decouple error handling using a generic boxed error type and update workspace dependencies accordingly.

DevDocMD/implementation/partition_layer_cache.md

@@ -62,7 +62,10 @@ layer_builder}`. (8) design agreed, item 1 done in (9) —
 IndexBuilder`, public, the four maintenance methods
 (`clear_output_for_create`/`create_skeleton`/`finalize_indexed`/`state`)
 shared with `merge`/`select`/`rebuild`/`reindex`. Item 2 from (8)
-(`obikalgorithm::Algorithm`) still not started. Note: `Layer` renamed
+(`obikalgorithm::Algorithm`) done in (12) — new crate, `type Output` +
+`fn run(&mut self) -> SKResult<Self::Output>`, `on_progress` moved off
+`run()`'s signature entirely into a per-algorithm `.on_progress(...)`
+setter. Note: `Layer` renamed
 `KmerLayer` (2026-08-21, outside this conversation). (5) itself still not
 implemented, still first on the "order of remaining work" list. Earlier
 mix-up, for
@@ -1340,10 +1343,138 @@ end to end against the new `Arc<IndexMeta>`/on-disk-`IndexState` shape.
 
 Still not done: (5)'s `KmerPartition`/`Layer` self-naming redesign itself
 (only the `PartitionRouter`-`&mut`-removal piece landed, as a side
-effect); (8)'s `obikalgorithm::Algorithm` trait (paused, not abandoned,
-for this detour — points 2–4 from that discussion are still open); the
+effect); the `distance.rs` → `obikphylo` relocation ((9), explicitly
+deferred); the future cache-manager crate. (8)'s `obikalgorithm::
+Algorithm` trait, resumed and closed in (12) below.
+
+## (12) done (2026-08-21): `obikalgorithm::Algorithm` — the shared trait, resumed and closed in one session
+
+Resumed (8)'s point 2 through a point-by-point discussion of what's
+actually common across the four pipeline algorithms, now that (11) made
+`KmerIndex` itself immutable everywhere. Four sub-points, each closed
+before moving to the next:
+
+**1. Receiver (`&self` vs `&mut self`)** — investigated whether (11)'s
+removal of `&mut KmerIndex` also removed the need for `PartitionRouter::
+run` to take `&mut self`. It didn't: `PartitionRouter` holds real
+per-run state of its own (`writers: Vec<Option<SKFileWriter>>`, open file
+handles, purely in-process RAM — confirmed by checking where `writers` is
+stored, nothing to do with `KmerIndex`/disk truth), unrelated to the
+index. First proposal (wrap `writers` in `RefCell` so all four could
+share a uniform `&self`) was retracted on pushback: manufacturing
+interior mutability with runtime borrow checks to satisfy a cosmetic
+uniformity that Rust doesn't even require is over-engineering — a trait
+method's receiver must match exactly across implementors, but nothing
+stops that shared receiver from being `&mut self` with three of the four
+implementations simply not using the mutability. Settled: trait declares
+`&mut self`; `Dereplicator`/`Counter`/`LayerBuilder` (previously `&self`)
+now also take `&mut self`, unused.
+
+**2. `path_source` as a `PartitionRouter` setter, not a `run()` param** —
+added a `files: Option<Box<dyn Iterator<Item = PathBuf> + Send>>` field +
+`.files(impl Iterator<Item = PathBuf> + Send + 'static) -> Self` setter
+(boxed rather than a generic type parameter on `PartitionRouter<'a>`:
+negligible cost — one `PathBuf` per input *file*, not per k-mer — for a
+much more usable type when passing the builder around). `run` now does
+`self.files.take().ok_or_else(...)`, erroring if `.files(...)` was never
+called, instead of taking `path_source` as a parameter.
+
+**3. Unifying the three progress-callback bound shapes** — reopened, then
+resolved differently than (8) originally framed it. First proposal
+(force everything to `FnMut(Progress) + Send`) was rejected on the same
+principle as point 1: `Dereplicator`/`Counter`'s `Fn(Progress) + Sync`
+isn't arbitrary — their callback is invoked concurrently from multiple
+rayon worker threads, and `FnMut` requires exclusive access, so forcing
+it would mean wrapping the callback in a `Mutex` for zero benefit at the
+one real call site (`pb.inc(1)`, already thread-safe). The actual
+resolution: move `on_progress` off `run()`'s signature entirely, onto a
+per-algorithm `.on_progress(...)` setter — same treatment as point 2's
+`path_source` — so each algorithm keeps its own bound (`PartitionRouter`:
+`FnMut(Progress) + 'a`, sequential, no `Send` needed; `LayerBuilder`:
+`FnMut(Progress) + Send + 'a`, crosses into `PartitionRunner`'s
+`thread::scope`-spawned controller thread once; `Dereplicator`/`Counter`:
+`Fn(Progress) + Sync + 'a`, invoked concurrently from rayon workers).
+This *also* dissolves the original problem `run()` had: once the
+callback isn't part of `run`'s signature at all, there's nothing left to
+unify there, and point 4 (below) becomes trivial.
+
+**4. `Output` as an associated type, `Error` fixed** — trivial once (3)
+moved the callback out: `Error` was already uniform (all four return
+`obiskio::SKResult<T>` = `Result<T, SKError>`), only `Output` varied
+(`()`/`()`/`KmerSpectrum`/`usize`). First cut reused `obiskio::SKResult`
+directly as the trait's return type — **caught and corrected the same
+session**: `SKError` enumerates I/O-specific cases (`BadMagic`/
+`Truncated`/`Compression`/...), meaningless at the level of a generic
+"algorithm" abstraction, and borrowing it made `obikalgorithm` — meant to
+be minimal and neutral — depend on a low-level I/O crate purely to reuse
+its error type. Textbook instance of the "petits pois" failure mode
+(patch around a convenient existing type instead of asking what this
+crate should actually own). Fixed to a genuinely generic, boxed error
+type owned by `obikalgorithm` itself:
+
+```rust
+// obikalgorithm — no dependency on obiskio or any other crate
+pub type Error = Box<dyn std::error::Error + Send + Sync>;
+pub type Result<T> = std::result::Result<T, Error>;
+
+pub trait Algorithm {
+    type Output;
+    fn run(&mut self) -> Result<Self::Output>;
+}
+```
+
+Any concrete error (`SKError`, `std::io::Error`, ...) converts
+automatically via `?`, through `std`'s own blanket `impl<E: Error + Send
++ Sync> From<E> for Box<dyn Error + Send + Sync>` — no custom `From` impl
+needed, no dependency on the crate that defines the concrete error type.
+The four algorithms' `run` bodies needed no change beyond the signature's
+return type (every existing `?` on an `SKError`-returning subcall keeps
+compiling, converting through the same blanket impl at the boundary).
+
+`PartitionRouter`/`Dereplicator`/`Counter`/`LayerBuilder` each `impl
+Algorithm for X<'_> { type Output = ...; fn run(&mut self) -> obikalgorithm::Result<...> { ... } }`
+— the old inherent `run` methods were removed outright (not kept as
+duplicates), so callers now `use obikalgorithm::Algorithm;` to call
+`.run()`. Every field-lifetime-bound boxed callback (`Box<dyn
+FnMut(Progress) + 'a>` etc.) is tied to the algorithm's own `'a` (the
+`&'a KmerIndex` lifetime already on the struct), not `'static` — avoids
+forcing callers' progress closures to `move`-capture (and therefore clone
+or `Arc`-wrap) local state like `TracedBar`/EMA-rate accumulators that
+they'd otherwise want to keep using by reference after `run()` returns.
+
+**Why a new crate, not a submodule of `obikindexer`**: `obikmer::cmd::
+index::mod` and `obikphylo`'s own test helpers both need to call `.run()`
+on these algorithms, so the trait has to be reachable from outside
+`obikindexer` — putting it in `obikindexer` itself would work file-wise
+but conflates "the trait every algorithm implements" with "one crate's
+particular four implementations of it", the same reasoning that already
+separated `obikindex` (data model) from `obikindexer` (algorithms
+operating on it). `obikalgorithm` has **no dependencies at all** (see
+above); `obikindexer`, `obikmer`, and `obikphylo` (dev-dependency, for
+its test helper) all depend on it.
+
+**Blast radius**: `obikindexer`'s four algorithm modules (struct field +
+setter + trait impl each); `obikmer::cmd::index::mod` (three call sites:
+`.on_progress(cb)` before `.run()`, unqualified now that the trait is in
+scope); `obikindexer::algorithms::partitionner::tests` and `obikphylo::
+siblings::tests` (both had direct `.run(None::<fn(Progress)>)`-shaped
+calls needing the same treatment). New `obikalgorithm` crate registered
+in the workspace `Cargo.toml`, depended on by `obikindexer`/`obikmer`
+(regular) and `obikphylo` (dev).
+
+**Verification**: `cargo check --workspace --all-targets` and `cargo test
+--workspace` both green (0 failures), `scripts/smoke_test_index.sh` green
+(870 kmers, same as every prior round) — this round didn't repeat the
+manual `merge`/`select`/`reindex` CLI exercise from (11), since nothing
+in this pass touched those commands' code paths (only the four pipeline
+algorithms and `cmd::index`, already covered by the smoke test). Reverified
+after the `obiskio`-dependency fix above (same three checks, still green,
+`obikalgorithm/Cargo.toml` now has zero `[dependencies]`).
+
+Still not done: (5)'s `KmerPartition`/`Layer` self-naming redesign; the
 `distance.rs` → `obikphylo` relocation ((9), explicitly deferred); the
-future cache-manager crate.
+future cache-manager crate (mentioned in (8) as a later, mirrored
+extension-trait exercise, not started).
 
 ## The problem