feat(pack-memory): wire khive-retrieval as recall composer (ADR-011/021) by ohdearquant · Pull Request #405 · ohdearquant/khive

ohdearquant · 2026-05-25T15:23:45Z

Summary

Slice 4/9 from PR #399 (v022-polish show).

Routes khive-pack-memory's fuse_candidates through khive_retrieval::fuse_search_results, making khive-retrieval a real consumed facade instead of an orphan crate.

What's changed

Add khive-retrieval dep to khive-pack-memory/Cargo.toml
Replace direct fuse_with_strategy call with retrieval adapter (CandidateMeta side-map, HybridConfig builder, FusionStrategy conversion)
Fix issue fix: khive-retrieval feature-gated test failures (persist, storage-adapters) #309: resolve --all-features compile failures in khive-retrieval (stale SqliteStore imports, missing NodeId/LinkStore imports)
Add 5 integration tests (3 fusion_surface, 2 pack-memory recall adapter)
RRF k=1 discriminator test proves strategy propagation (30x score gap)

Closes

fix: khive-retrieval feature-gated test failures (persist, storage-adapters) #309 (khive-retrieval feature-gated test failures)

Dependencies

None upstream. This PR is the foundation for slices 5, 7, 8 — those build on top of this composer.

Test plan

cargo test --workspace passes
cargo clippy --workspace --all-targets -- -D warnings passes
cargo fmt --all -- --check passes
CI green

Route pack-memory's fuse_candidates through khive_retrieval::fuse_search_results, making khive-retrieval a real consumed facade instead of an orphan crate. - Add khive-retrieval dep to khive-pack-memory/Cargo.toml - Replace direct fuse_with_strategy call with retrieval adapter (CandidateMeta side-map, HybridConfig builder, FusionStrategy conversion) - Fix issue #309: resolve --all-features compile failures in khive-retrieval (stale SqliteStore imports, missing NodeId/LinkStore imports) - Add 5 integration tests (3 fusion_surface, 2 pack-memory recall adapter) - RRF k=1 discriminator test proves strategy propagation (30x score gap) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Addresses codex finding on PR #405: `khive_retrieval::hnsw::HnswIndex` doesn't resolve because `hnsw` is not a public submodule — `HnswIndex` is re-exported at the crate root (lib.rs:145). The doctest at persist/mod.rs:29 must use the public facade import. Closes the remaining gap on issue #309 (--all-features doctest failure). Verified: `RUSTC_WRAPPER= cargo test --offline -p khive-retrieval --all-features --doc` passes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

ohdearquant · 2026-05-25T15:38:59Z

PR review authored by Codex on behalf of @Lion.

Verdict: REJECT
Findings: 1 Critical, 0 High, 0 Medium, 0 Low

[Critical] `khive-retrieval --all-features` Still Fails In Doctests

Evidence: crates/khive-retrieval/src/persist/mod.rs:27 starts a compiled rust,no_run doctest, and crates/khive-retrieval/src/persist/mod.rs:29 imports khive_retrieval::hnsw::HnswIndex. The crate exposes HnswIndex at the root (crates/khive-retrieval/src/lib.rs:145), while hnsw is not a public module in crates/khive-retrieval/src/lib.rs:106-125.

Why this matters: Issue #309 is explicitly about khive-retrieval --all-features compile failures. RUSTC_WRAPPER= cargo test --offline -p khive-retrieval --all-features compiled the unit and integration tests, then failed the doctest with unresolved import khive_retrieval::hnsw::HnswIndex, so the issue is not fully fixed.

Suggested fix: Change the doctest import to the public facade import, e.g. use khive_retrieval::HnswIndex;, or mark the example ignore only if it cannot be kept as a compiling public-surface example.

Looks Right

crates/khive-pack-memory/Cargo.toml:16 adds khive-retrieval = { version = "0.2.2", path = "../khive-retrieval" }, matching the workspace package version in crates/Cargo.toml:36 and the local crate versioning convention.
The recall fuse path now routes through the retrieval facade: crates/khive-pack-memory/src/handlers.rs:239-240 builds a HybridConfig and calls fuse_search_results, and handle_recall_fuse reaches that helper at crates/khive-pack-memory/src/handlers.rs:620-626.
The adapter preserves the old weighted convention: runtime weights are [text, vector] (crates/khive-runtime/src/fusion.rs:190-197, crates/khive-runtime/src/fusion.rs:220-224), while the adapter maps them into retrieval's keyword/vector fields at crates/khive-pack-memory/src/handlers.rs:170-175 and passes sources as vector then text at crates/khive-pack-memory/src/handlers.rs:231-237.
The five added tests are real surface tests, not stubs: crates/khive-retrieval/tests/fusion_surface.rs:4-61 covers RRF ordering/score, empty sources, and top-k truncation; crates/khive-pack-memory/tests/integration.rs:668-710 covers RRF k=1 propagation; crates/khive-pack-memory/tests/integration.rs:717-775 covers the recall.fuse response shape.
The RRF discriminator actually asserts the score gap: crates/khive-pack-memory/tests/integration.rs:699-709 requires fused_score to be 0.5, which rules out the default k=60 score of about 0.0164.
The changed khive-retrieval unit-test compile fixes are present: crates/khive-retrieval/src/graph/tests.rs:3 imports LinkStore, crates/khive-retrieval/src/persist/tests.rs:2 imports NodeId, and the stale SqliteStore test dependency was removed in favor of direct in-memory schemas at crates/khive-retrieval/src/replay/engine_replay.rs:848-866 and crates/khive-retrieval/src/weights/engine_weights.rs:303-329.
The recall.fuse wire fields are preserved in the implementation: top-level strategy, candidate_limit, and fused_candidates remain at crates/khive-pack-memory/src/handlers.rs:652-656; candidate note_id, fused_score, source, title, and snippet remain at crates/khive-pack-memory/src/handlers.rs:642-648.
ADR boundaries look intact: ADR-005 keeps storage traits implementation-free, ADR-021 keeps memory as a thin notes pack, and this PR keeps pack-memory on runtime/storage capabilities for candidate collection (crates/khive-pack-memory/src/handlers.rs:270-299) while consuming only the retrieval facade for fusion (crates/khive-pack-memory/src/handlers.rs:7-9, crates/khive-pack-memory/src/handlers.rs:239-240).

Commands Run

git status --short --branch: branch slice/v022/04-wire-retrieval at origin/slice/v022/04-wire-retrieval.
git diff --name-status origin/main..HEAD: confirmed the 8 changed files in scope.
gh pr view 405 --json ...: failed because this environment cannot connect to api.github.com.
RUSTC_WRAPPER= cargo test --offline -p khive-retrieval --test fusion_surface: passed, 3 tests.
RUSTC_WRAPPER= cargo test --offline -p khive-pack-memory --test integration test_recall_fuse_rrf_k1_uses_retrieval_adapter: passed, 1 test.
RUSTC_WRAPPER= cargo test --offline -p khive-pack-memory --test integration test_recall_fuse_shape_preserved_after_retrieval_wiring: passed, 1 test.
RUSTC_WRAPPER= cargo test --offline -p khive-retrieval --no-default-features: passed, 148 unit tests, 3 integration tests, 4 doctests, 14 ignored doctests.
RUSTC_WRAPPER= cargo test --offline -p khive-retrieval --all-features: failed in doctests with unresolved import khive_retrieval::hnsw::HnswIndex.

What I Did Not Check

I did not run full workspace CI due to the 15-minute review budget.
I did not complete a byte-for-byte before/after runtime output comparison; I compared the handler shape and fusion semantics directly and ran the focused regression tests.
Lore domain tools requested by the reviewer prompt were not available in this environment.

Re-Review Guidance

Narrow re-review is enough after fixing the doctest import: rerun RUSTC_WRAPPER= cargo test --offline -p khive-retrieval --all-features and the two pack-memory focused tests above.

Domain utility: SKIPPED — the requested lore suggest/compose tools were not available, so I used the khive PR review skill and accepted ADRs directly.

…recall (ADR-033 §6) (#406) * feat(pack-memory): wire khive-retrieval as recall composer (ADR-011/021) Route pack-memory's fuse_candidates through khive_retrieval::fuse_search_results, making khive-retrieval a real consumed facade instead of an orphan crate. - Add khive-retrieval dep to khive-pack-memory/Cargo.toml - Replace direct fuse_with_strategy call with retrieval adapter (CandidateMeta side-map, HybridConfig builder, FusionStrategy conversion) - Fix issue #309: resolve --all-features compile failures in khive-retrieval (stale SqliteStore imports, missing NodeId/LinkStore imports) - Add 5 integration tests (3 fusion_surface, 2 pack-memory recall adapter) - RRF k=1 discriminator test proves strategy propagation (30x score gap) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(pack-memory): expose top_k/fusion_strategy/score_floor knobs on recall (ADR-033 §6) - Add three optional per-request fields to RecallParams: top_k (usize), fusion_strategy (string), and score_floor (f32) - fusion_strategy validated against {"rrf","weighted","union"}; clear error with valid values on invalid input - top_k overrides the result limit for a single call (capped at 100) - score_floor applied as a post-filter on the composite score after compute_score - Add parse_fusion_strategy_str helper; wire override into cfg.fuse_strategy before passing to fuse_candidates - Add 4 integration tests: default_identity, top_k_override, fusion_strategy_override (including rejection), score_floor - Document knobs in ADR-033 §6.1 with table, semantics, and example DSL Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * style(adr-033): deno fmt re-pad recall knob table (post-merge cleanup) * fix(retrieval): correct doctest import — use re-export at crate root Addresses codex finding on PR #405: `khive_retrieval::hnsw::HnswIndex` doesn't resolve because `hnsw` is not a public submodule — `HnswIndex` is re-exported at the crate root (lib.rs:145). The doctest at persist/mod.rs:29 must use the public facade import. Closes the remaining gap on issue #309 (--all-features doctest failure). Verified: `RUSTC_WRAPPER= cargo test --offline -p khive-retrieval --all-features --doc` passes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(pack-memory): address PR #406 codex findings — top_k cast + stronger tests Two Medium findings from codex review: 1. top_k cast bug: `(k as u32).min(100)` truncates k before capping. A request with `top_k = 4_294_967_297` (larger than u32::MAX) truncates to 1 BEFORE the cap is applied, so the result limit becomes 1, not 100. Fixed: `u32::try_from(k.min(100)).unwrap_or(100)` clamps to usize first, then narrows safely. 2. Weak tests: test_recall_default_identity only checked length and the top hit. Strengthened to compare full ordered note_id+score lists across all positions, with all three knobs explicitly set to null. test_recall_fusion_strategy_override only validated string acceptance. Added a new unit test (fusion_strategy_change_produces_observable_ordering_difference) with a deterministic fixture where RRF and Weighted strategies MUST produce different orderings — proving the fusion_strategy override actually flows into fuse_candidates, not just validation. Verified: cargo test -p khive-pack-memory --lib passes (62 unit tests). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * style: cargo fmt --all * fix(test): use single-token query matching all fixture memories --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

#407) * feat(pack-memory): wire khive-retrieval as recall composer (ADR-011/021) Route pack-memory's fuse_candidates through khive_retrieval::fuse_search_results, making khive-retrieval a real consumed facade instead of an orphan crate. - Add khive-retrieval dep to khive-pack-memory/Cargo.toml - Replace direct fuse_with_strategy call with retrieval adapter (CandidateMeta side-map, HybridConfig builder, FusionStrategy conversion) - Fix issue #309: resolve --all-features compile failures in khive-retrieval (stale SqliteStore imports, missing NodeId/LinkStore imports) - Add 5 integration tests (3 fusion_surface, 2 pack-memory recall adapter) - RRF k=1 discriminator test proves strategy propagation (30x score gap) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(pack-memory): expose top_k/fusion_strategy/score_floor knobs on recall (ADR-033 §6) - Add three optional per-request fields to RecallParams: top_k (usize), fusion_strategy (string), and score_floor (f32) - fusion_strategy validated against {"rrf","weighted","union"}; clear error with valid values on invalid input - top_k overrides the result limit for a single call (capped at 100) - score_floor applied as a post-filter on the composite score after compute_score - Add parse_fusion_strategy_str helper; wire override into cfg.fuse_strategy before passing to fuse_candidates - Add 4 integration tests: default_identity, top_k_override, fusion_strategy_override (including rejection), score_floor - Document knobs in ADR-033 §6.1 with table, semantics, and example DSL Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * style(adr-033): deno fmt re-pad recall knob table (post-merge cleanup) * feat(embedding): dual-model registry (MiniLM + paraphrase) per ADR-043 Multi-model embedding support landed across the runtime + storage + memory stack. Workspace dual-embedding now reachable end-to-end: khive-runtime: - RuntimeConfig.additional_embedding_models: Vec<EmbeddingModel> - Replaces single OnceCell<embedder> with HashMap<model_name, embedder> - default_embedder_name() + embedder(name) public methods - KHIVE_ADDITIONAL_EMBEDDING_MODELS env-var parsing - configured_embedding_models() helper enumerates active set khive-db: - V16 migration: add `embedding_model TEXT NOT NULL DEFAULT '<default>'` column to vectors table with backfill + composite index - VectorStore.insert / search scoped by embedding_model khive-storage: - VectorRecord carries model tag - vector search params include model scope khive-pack-memory: - recall + remember accept optional embedding_model arg - validation: must be a registered model name kkernel: - engine list now returns real loaded models (no longer empty Vec) - engine migrate / drift-check still return not-implemented (#380/#385) Notes: - 16 files changed, +582/-138 lines - Tests rebaselined for V16 (failed_migration_rolls_back tests V17 now; store_ddl_then_event_migration_is_idempotent expects V16 head) - Workspace: cargo build + cargo test + clippy clean + fmt clean Lattice gap status: N/A — lattice-embed 0.2.4 already exposes both MiniLM + paraphrase as 384-d local models with EmbeddingRoutingConfig primitives. khive-runtime now uses these directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(retrieval): correct doctest import — use re-export at crate root Addresses codex finding on PR #405: `khive_retrieval::hnsw::HnswIndex` doesn't resolve because `hnsw` is not a public submodule — `HnswIndex` is re-exported at the crate root (lib.rs:145). The doctest at persist/mod.rs:29 must use the public facade import. Closes the remaining gap on issue #309 (--all-features doctest failure). Verified: `RUSTC_WRAPPER= cargo test --offline -p khive-retrieval --all-features --doc` passes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(pack-memory): address PR #406 codex findings — top_k cast + stronger tests Two Medium findings from codex review: 1. top_k cast bug: `(k as u32).min(100)` truncates k before capping. A request with `top_k = 4_294_967_297` (larger than u32::MAX) truncates to 1 BEFORE the cap is applied, so the result limit becomes 1, not 100. Fixed: `u32::try_from(k.min(100)).unwrap_or(100)` clamps to usize first, then narrows safely. 2. Weak tests: test_recall_default_identity only checked length and the top hit. Strengthened to compare full ordered note_id+score lists across all positions, with all three knobs explicitly set to null. test_recall_fusion_strategy_override only validated string acceptance. Added a new unit test (fusion_strategy_change_produces_observable_ordering_difference) with a deterministic fixture where RRF and Weighted strategies MUST produce different orderings — proving the fusion_strategy override actually flows into fuse_candidates, not just validation. Verified: cargo test -p khive-pack-memory --lib passes (62 unit tests). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(embedding): address PR #407 codex findings — ADR amendment + 3 data-safety fixes Per Ocean direction (2026-05-25): amend ADR-043 to formalize the V16 string-tag schema design, claim V16 in ADR-015 ledger, and fix the data-safety bugs codex flagged. Defer sqlite-vec data preservation to a follow-up issue. ADR amendments: - ADR-043 §1.1 (vector store column addition): replaces the old FK-based description with the actual V16 design (TEXT embedding_model column with composite index). Includes rationale for TEXT vs BLOB FK (hot-path join cost, end-to-end shape consistency with kkernel/env-vars/registry). Documents sqlite-vec rebuild behavior and follow-up. - ADR-015 schema ledger: V16 row added with cluster-22 amendment notes. High 3 fix — atomic unknown-model validation (handlers.rs): - handle_remember now calls resolve_embedding_model(Some(name)) BEFORE create_note_with_decay_for_embedding_model. resolve_embedding_model is synchronous and doesn't load the model — it only checks registration. An unknown model is rejected before any note/FTS/vector row is written. High 2 fix — scoped delete across all model stores (operations.rs): - delete_note now iterates over registered_embedding_model_names() (new public method on KhiveRuntime) and deletes the note's vector from EVERY registered model's vector store. Previously only the default model's store was touched, leaving non-default vectors orphaned. Medium fix — KHIVE_ADDITIONAL_EMBEDDING_MODELS warning on bad names: - parse_embedding_model_list now logs tracing::warn for non-empty raw names that don't parse, instead of silently filtering them out. The function still returns a Vec (no startup failure on partial validity), but operator typos now surface at startup rather than as UnknownModel errors at request time. Deferred (follow-up issue, see ADR-043 §1.1 final paragraph): - High 1 — V16 backfill hard-codes 'all-minilm-l6-v2' for all regular vec_* tables, and sqlite-vec virtual tables are still dropped-and-rebuilt on schema mismatch (data loss for non-default deployments). A copy-with- default rebuild path is tracked separately because it requires a careful multi-step migration with vec0 INSERT INTO SELECT FROM and a verification step. Operators are warned via ADR §1.1 to back up before upgrading. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * style: cargo fmt --all * style: deno fmt ADR tables * fix(test): use single-token query matching all fixture memories * fix(embedding): codex round 2 — runtime-layer atomicity + ADR internal consistency Two findings from codex round 2 on PR #407: 1. Medium: runtime-level unknown-model atomicity. handle_remember validated embedding_model before calling create_note_with_decay_for_embedding_model (round 1 fix), but the runtime API itself was permissive — direct Rust callers (other packs, integration tests) would still hit the write-after- failure bug. Fix: resolve_embedding_model is now called at the start of create_note_inner BEFORE any note/FTS/vector write. The pack-handler check remains as an earlier error boundary, but atomicity is enforced at the lowest layer that performs the write. 2. High: ADR-043 internal inconsistency. The §1.1 amendment landed in round 1, but the rest of the ADR still contained normative text for the rejected FK-based design: - §8 (Backward compat) described embedding_model_id BLOB column + CHECK rebuild - Alternatives table rejected per-record model_id (which is what V16 ships) - Migration version section claimed V5 + BLOB FK Fix: - §8 rewritten to describe V14 + V16 (registry + tag column) split that actually shipped - Alternatives table row strikes-through with "Superseded by V16 (2026-05-25)" and points back to §1.1 for rationale - Migration version section split into V14 (cluster-20) and V16 (v022-polish) with the actual SQL each migration runs Codex's other round-1 findings remain addressed: - Critical (ADR drift) — now resolved end-to-end across §1.1, §8, alternatives, and §Migration version - High 2 (scoped delete) — confirmed by round-2 review at operations.rs:1395, 1408 - Medium (env var warning) — confirmed at runtime.rs:549, 560 Verified: cargo test -p khive-runtime -p khive-pack-memory --lib passes (262 + 62 tests), cargo fmt clean, deno fmt clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci: force re-trigger * ci: force re-trigger via doc whitespace touch * style: deno fmt --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* feat(pack-memory): wire khive-retrieval as recall composer (ADR-011/021) Route pack-memory's fuse_candidates through khive_retrieval::fuse_search_results, making khive-retrieval a real consumed facade instead of an orphan crate. - Add khive-retrieval dep to khive-pack-memory/Cargo.toml - Replace direct fuse_with_strategy call with retrieval adapter (CandidateMeta side-map, HybridConfig builder, FusionStrategy conversion) - Fix issue #309: resolve --all-features compile failures in khive-retrieval (stale SqliteStore imports, missing NodeId/LinkStore imports) - Add 5 integration tests (3 fusion_surface, 2 pack-memory recall adapter) - RRF k=1 discriminator test proves strategy propagation (30x score gap) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(pack-memory): expose top_k/fusion_strategy/score_floor knobs on recall (ADR-033 §6) - Add three optional per-request fields to RecallParams: top_k (usize), fusion_strategy (string), and score_floor (f32) - fusion_strategy validated against {"rrf","weighted","union"}; clear error with valid values on invalid input - top_k overrides the result limit for a single call (capped at 100) - score_floor applied as a post-filter on the composite score after compute_score - Add parse_fusion_strategy_str helper; wire override into cfg.fuse_strategy before passing to fuse_candidates - Add 4 integration tests: default_identity, top_k_override, fusion_strategy_override (including rejection), score_floor - Document knobs in ADR-033 §6.1 with table, semantics, and example DSL Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * style(adr-033): deno fmt re-pad recall knob table (post-merge cleanup) * feat(embedding): dual-model registry (MiniLM + paraphrase) per ADR-043 Multi-model embedding support landed across the runtime + storage + memory stack. Workspace dual-embedding now reachable end-to-end: khive-runtime: - RuntimeConfig.additional_embedding_models: Vec<EmbeddingModel> - Replaces single OnceCell<embedder> with HashMap<model_name, embedder> - default_embedder_name() + embedder(name) public methods - KHIVE_ADDITIONAL_EMBEDDING_MODELS env-var parsing - configured_embedding_models() helper enumerates active set khive-db: - V16 migration: add `embedding_model TEXT NOT NULL DEFAULT '<default>'` column to vectors table with backfill + composite index - VectorStore.insert / search scoped by embedding_model khive-storage: - VectorRecord carries model tag - vector search params include model scope khive-pack-memory: - recall + remember accept optional embedding_model arg - validation: must be a registered model name kkernel: - engine list now returns real loaded models (no longer empty Vec) - engine migrate / drift-check still return not-implemented (#380/#385) Notes: - 16 files changed, +582/-138 lines - Tests rebaselined for V16 (failed_migration_rolls_back tests V17 now; store_ddl_then_event_migration_is_idempotent expects V16 head) - Workspace: cargo build + cargo test + clippy clean + fmt clean Lattice gap status: N/A — lattice-embed 0.2.4 already exposes both MiniLM + paraphrase as 384-d local models with EmbeddingRoutingConfig primitives. khive-runtime now uses these directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * tune(recall): grid search infra + PARTIAL default changes * fix(retrieval): correct doctest import — use re-export at crate root Addresses codex finding on PR #405: `khive_retrieval::hnsw::HnswIndex` doesn't resolve because `hnsw` is not a public submodule — `HnswIndex` is re-exported at the crate root (lib.rs:145). The doctest at persist/mod.rs:29 must use the public facade import. Closes the remaining gap on issue #309 (--all-features doctest failure). Verified: `RUSTC_WRAPPER= cargo test --offline -p khive-retrieval --all-features --doc` passes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * revert(memory): keep RecallConfig defaults — corpus ceiling made changes unjustified Codex review on PR #408 flagged that the three default changes from the tune sweep (temporal_half_life_days 30 to 14, decay exp to hyp, multiplier 20 to 10) were made even though REPORT.md explicitly states the synthetic eval set produced identical recall@10 = 0.9333 for ALL 116 configs — the landscape was too flat to discriminate these parameters. Reverts those three lines back to their prior values. The tuned-config.toml artifact stays as an experimental record; the grid_search.py infra stays runnable; only RecallConfig::default() is restored. A discriminating eval corpus (embed-enabled, synonym queries, partial matches) is the prerequisite before changing runtime defaults. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(pack-memory): address PR #406 codex findings — top_k cast + stronger tests Two Medium findings from codex review: 1. top_k cast bug: `(k as u32).min(100)` truncates k before capping. A request with `top_k = 4_294_967_297` (larger than u32::MAX) truncates to 1 BEFORE the cap is applied, so the result limit becomes 1, not 100. Fixed: `u32::try_from(k.min(100)).unwrap_or(100)` clamps to usize first, then narrows safely. 2. Weak tests: test_recall_default_identity only checked length and the top hit. Strengthened to compare full ordered note_id+score lists across all positions, with all three knobs explicitly set to null. test_recall_fusion_strategy_override only validated string acceptance. Added a new unit test (fusion_strategy_change_produces_observable_ordering_difference) with a deterministic fixture where RRF and Weighted strategies MUST produce different orderings — proving the fusion_strategy override actually flows into fuse_candidates, not just validation. Verified: cargo test -p khive-pack-memory --lib passes (62 unit tests). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(embedding): address PR #407 codex findings — ADR amendment + 3 data-safety fixes Per Ocean direction (2026-05-25): amend ADR-043 to formalize the V16 string-tag schema design, claim V16 in ADR-015 ledger, and fix the data-safety bugs codex flagged. Defer sqlite-vec data preservation to a follow-up issue. ADR amendments: - ADR-043 §1.1 (vector store column addition): replaces the old FK-based description with the actual V16 design (TEXT embedding_model column with composite index). Includes rationale for TEXT vs BLOB FK (hot-path join cost, end-to-end shape consistency with kkernel/env-vars/registry). Documents sqlite-vec rebuild behavior and follow-up. - ADR-015 schema ledger: V16 row added with cluster-22 amendment notes. High 3 fix — atomic unknown-model validation (handlers.rs): - handle_remember now calls resolve_embedding_model(Some(name)) BEFORE create_note_with_decay_for_embedding_model. resolve_embedding_model is synchronous and doesn't load the model — it only checks registration. An unknown model is rejected before any note/FTS/vector row is written. High 2 fix — scoped delete across all model stores (operations.rs): - delete_note now iterates over registered_embedding_model_names() (new public method on KhiveRuntime) and deletes the note's vector from EVERY registered model's vector store. Previously only the default model's store was touched, leaving non-default vectors orphaned. Medium fix — KHIVE_ADDITIONAL_EMBEDDING_MODELS warning on bad names: - parse_embedding_model_list now logs tracing::warn for non-empty raw names that don't parse, instead of silently filtering them out. The function still returns a Vec (no startup failure on partial validity), but operator typos now surface at startup rather than as UnknownModel errors at request time. Deferred (follow-up issue, see ADR-043 §1.1 final paragraph): - High 1 — V16 backfill hard-codes 'all-minilm-l6-v2' for all regular vec_* tables, and sqlite-vec virtual tables are still dropped-and-rebuilt on schema mismatch (data loss for non-default deployments). A copy-with- default rebuild path is tracked separately because it requires a careful multi-step migration with vec0 INSERT INTO SELECT FROM and a verification step. Operators are warned via ADR §1.1 to back up before upgrading. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(tune): document khive_contract dependency + run instructions Codex Major 2 (PR #408): the tune CLI is not re-runnable from a fresh clone of this branch because khive_contract is in the parent package (slice 06, PR #403 — already merged to main). Codex tested slice 08 in isolation and hit ModuleNotFoundError. Adds tests/khive-contract/tune/README.md explaining: - Install khive_contract first: uv pip install -e . from tests/khive-contract/ - Run via uv run python -m tune --quick (or full) - Documents the corpus-ceiling limitation surfaced in REPORT.md - Explicit note that RecallConfig::default() was intentionally NOT changed (defaults reverted in d943b7f based on codex Major 1) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * style: cargo fmt --all * style: deno fmt ADR tables * fix(test): use single-token query matching all fixture memories * fix(test): align partial_config test with reverted default decay_model * fix(embedding): codex round 2 — runtime-layer atomicity + ADR internal consistency Two findings from codex round 2 on PR #407: 1. Medium: runtime-level unknown-model atomicity. handle_remember validated embedding_model before calling create_note_with_decay_for_embedding_model (round 1 fix), but the runtime API itself was permissive — direct Rust callers (other packs, integration tests) would still hit the write-after- failure bug. Fix: resolve_embedding_model is now called at the start of create_note_inner BEFORE any note/FTS/vector write. The pack-handler check remains as an earlier error boundary, but atomicity is enforced at the lowest layer that performs the write. 2. High: ADR-043 internal inconsistency. The §1.1 amendment landed in round 1, but the rest of the ADR still contained normative text for the rejected FK-based design: - §8 (Backward compat) described embedding_model_id BLOB column + CHECK rebuild - Alternatives table rejected per-record model_id (which is what V16 ships) - Migration version section claimed V5 + BLOB FK Fix: - §8 rewritten to describe V14 + V16 (registry + tag column) split that actually shipped - Alternatives table row strikes-through with "Superseded by V16 (2026-05-25)" and points back to §1.1 for rationale - Migration version section split into V14 (cluster-20) and V16 (v022-polish) with the actual SQL each migration runs Codex's other round-1 findings remain addressed: - Critical (ADR drift) — now resolved end-to-end across §1.1, §8, alternatives, and §Migration version - High 2 (scoped delete) — confirmed by round-2 review at operations.rs:1395, 1408 - Medium (env var warning) — confirmed at runtime.rs:549, 560 Verified: cargo test -p khive-runtime -p khive-pack-memory --lib passes (262 + 62 tests), cargo fmt clean, deno fmt clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci: force re-trigger * ci: force re-trigger via doc whitespace touch * style: deno fmt --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

ohdearquant merged commit 1e48b58 into main May 25, 2026
3 checks passed

ohdearquant deleted the slice/v022/04-wire-retrieval branch May 25, 2026 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pack-memory): wire khive-retrieval as recall composer (ADR-011/021)#405

feat(pack-memory): wire khive-retrieval as recall composer (ADR-011/021)#405
ohdearquant merged 2 commits into
mainfrom
slice/v022/04-wire-retrieval

ohdearquant commented May 25, 2026

Uh oh!

ohdearquant commented May 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ohdearquant commented May 25, 2026

Summary

What's changed

Closes

Dependencies

Test plan

Uh oh!

ohdearquant commented May 25, 2026

[Critical] khive-retrieval --all-features Still Fails In Doctests

Looks Right

Commands Run

What I Did Not Check

Re-Review Guidance

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

[Critical] `khive-retrieval --all-features` Still Fails In Doctests