fix: four dogfooding defects — note tags, neighbors soft-deleted notes, prefix dedup, ANN warm-cache race (#747 #748 #749 #750)#753
Merged
Conversation
create(kind=note, tags=[...]) whitelisted `tags` as an accepted param but never read it in the note branch, silently dropping the value. Notes have no dedicated tags column (schema: khive-db/sql/notes-ddl.sql) so, matching the existing convention already used by memory.remember and honored by this pack's own search/list note-tag filters, tags now merge into properties["tags"]. Precedence when the caller supplies both a top-level tags param and a conflicting properties.tags: the top-level tags param wins (documented in merge_note_tags and asserted by a regression test). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
deleted_entity_ids only queried the entities table, so a soft-deleted note-kind neighbor (e.g. reached via an annotates edge) leaked through neighbors()/neighbors_with_query_directed()/traverse() and hydrated as a blank/missing hit. Extended the screen to UNION the notes table using an independent numbered-placeholder block per UNION half (matching the existing batch_neighbors convention in khive-db/src/stores/graph.rs — SQLite numbered params bind by index, so reusing the same numbers across UNION halves would collapse to one shared block instead of binding the id list twice). View-layer only: no edges are touched, delete() semantics are unchanged. Updated annotated_note_soft_delete_preserves_annotate_edge, which had been asserting 'edge not cascaded' by reading it back through neighbors() — that assertion happened to pass only because neighbors() was, itself, exhibiting the #748 bug (failing to filter the soft-deleted note target). It now checks edge survival directly via get_edge() and separately asserts neighbors() correctly omits the soft-deleted note. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
resolve_prefix_inner scanned entities/notes/events/graph_edges in
sequence, pushing every hit into matches with no cross-table dedup. A UUID
legitimately present in more than one table produced duplicate entries,
so matches.len() > 1 fired AmbiguousPrefix even though exactly one record
was addressed — the error message named the same UUID twice.
Added a seen: HashSet<String> so matches (and every length check derived
from it, including the mid-scan early-exit 'if matches.len() > 1 { break
}') reflects distinct UUIDs only. Two genuinely distinct UUIDs sharing a
prefix still trip AmbiguousPrefix (regression-covered by the pre-existing
resolve_prefix_ambiguous_same_namespace test, reconfirmed green).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ensure_ann_for_model_inner installed a finished build via entry(key).or_insert(bridge), which always kept whichever build reached the install call first and was a permanent no-op once the key was occupied - so a slow build that snapshotted the corpus before a still-in-flight write committed could install after a newer write's invalidate+re-warm cycle and silently clobber freshness, with no later rebuild able to correct it (the double-fingerprint check only bounds the scan window, not the gap between computing fp_after and the eventual or_insert, e.g. during persist_snapshot's I/O). Added a per-model monotonic write-generation counter (AnnState.generations, bumped by memory.remember alongside the existing cache invalidation - no existing sequence/timestamp signal on the write path was suitable: the notes table's updated_at is wall-clock and not collision-free under concurrent writes, and the vec0 virtual vector table exposes no reliable rowid). ensure_ann_for_model now captures this counter's value BEFORE its own fast-path check and before the corpus scan, stamps it on the resulting AnnBridge, and installs via install_if_fresher: a real compare-and-replace that only overwrites an existing entry when the candidate's generation is >= the installed entry's. ensure_ann_background's own fast path is generation-aware the same way, so a caller triggered by a write that just bumped the counter always spawns a rebuild rather than trusting a stale present entry. Closed the residual recall-side gap too: search_loaded's cache-hit check was presence-only, so a stale-but-installed entry (one that lost the install race for freshness but still reached an empty slot first) would still be served directly. handlers/common.rs's recall path now gates on the new ann::is_current instead, treating "present but stale" the same as a genuine miss and falling through to the existing synchronous ensure_ann_for_model fallback. ensure_ann_background keeps its fire-and-forget shape; nothing is serialized onto the request path except this pre-existing on-demand-warm fallback. Removed the bounded fresh-runtime + poll retry workarounds from the three tests exercising this (ns733_recall_ann_overfetch_retry_loop_*, ns733b_recall_verbose_multi_model_breakdown_*, ns733b_recall_candidates_multi_model_*) - all three now assert directly in a single attempt. Stress-verified via the compiled test binary run standalone 50 consecutive times each (150 runs total, each a fresh process with a fresh in-memory KhiveRuntime): 150/150 passed, 0 failures. Added direct regression coverage for install_if_fresher's compare-and- replace invariant and is_current's stale-is-a-miss semantics (ann.rs::tests). A true end-to-end interleaving test (pause a build mid-scan, land a write, assert the newer generation wins) was not attempted: no test-only pause hook exists in ensure_ann_for_model_inner, and adding one solely to force a schedule would test the hook rather than the production path - the unit-level invariant tests plus the 150-run stress evidence stand in for it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The #750 freshness gate (ann::is_current) only holds if every memory-corpus mutation advances AnnState::generations. The only production bump was memory.remember's own write path; memory.prune, KG update() reaching a kind="memory" note's reindex, and KG delete() on a kind="memory" note all left the warm ANN cache trusted as current after mutating the corpus. Extends khive-runtime's existing pack-installed extension-point pattern (EntityTypeValidatorFn / install_entity_type_validator / PackRuntime:: register_entity_type_validator / call_register_entity_type_validators) with a parallel NoteMutationHookFn seam: update_note (on text_changed reindex) and delete_note (soft or hard) now fire a pack-installed hook after the mutation is durably applied, with zero cost to packs that don't install one. khive-pack-memory installs a hook that invalidates + bumps the generation for kind="memory" notes, the same two calls memory.remember already makes. khive-mcp's serve.rs/server.rs wire call_register_note_mutation_hooks at boot, alongside the existing call_register_entity_type_validators call -- load-bearing: without it the hook mechanism is entirely inert in the running daemon. memory.prune (same crate as ann, no inversion needed) invalidates and bumps inline, mirroring remember.rs's pattern directly. Added three regression tests (pack.rs's note_mutation_hook_tests): warm the ANN cache, mutate a memory-kind note via each of the three paths with no subsequent memory.remember, assert ann::is_current is false afterward. These assert the white-box is_current signal rather than a black-box recall-result check -- a throwaway experiment (temporarily disabling all three fix-round-1 call sites) proved the black-box form of the delete-path test is NOT discriminating: recall's post-search hydration filters soft-deleted notes regardless of ANN cache freshness, so "the deleted note is absent from results" passed even with the bug reintroduced. is_current does not have that blind spot; the disable/re-enable cycle confirmed all three corrected tests genuinely fail without the fix and pass with it restored. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…round 2)
Codex r2 REJECT, 2 Highs:
High 1: merge_note's non-dry-run success path reindexed the surviving
note but never fired the note-mutation hook (unlike update_note's
identical text-changed branch). Fixed by firing it after reindex_note,
same shape as update_note. New regression test
fr2_kg_merge_invalidates_warm_ann_without_subsequent_remember, verified
to discriminate via disable/re-enable.
High 2 (verified AnnState::generations is process-local in-memory
state, per source read of khive-pack-memory/src/ann.rs -- confirms the
coordinator's hypothesis):
in-process portion (fixed here):
- kkernel exec --atomic's registry build never called
call_register_note_mutation_hooks, so the hook slot was always None
for the whole --atomic process.
- atomic_prepare.rs's ReindexNote post-commit handler called
reindex_note directly, bypassing update_note (and its hook-fire)
entirely.
- DeletePlan carried NO post_commit field at all -- a deeper gap than
ReindexNote's bypass. Added a post_commit field, a new
PostCommitEffect::NoteDeleted variant, and wired it through all five
DeletePlan construction sites plus apply_post_commit_effects.
New regression test in khive-runtime's own atomic_prepare.rs tests
module: atomic_note_update_and_delete_post_commit_fire_the_note_mutation_hook,
verified to discriminate for BOTH paths independently via disable/re-enable.
cross-process portion (NOT fixed here, documented for a follow-up
issue): kkernel exec --atomic and kkernel reindex run in separate OS
processes from any daemon; a hook fired inside them can only mutate
their own transient AnnState, never a running daemon's warm cache.
kkernel reindex additionally has no in-process AnnState concept at
all -- it invalidates only persisted Vamana snapshot rows, a
different mechanism. Full evidence in impl_report_quadfix.md.
Gates: cargo fmt --all -- --check; cargo clippy -p khive-pack-kg -p
khive-runtime -p khive-pack-memory -p kkernel --all-targets -- -D
warnings; cargo test -p khive-pack-kg -p khive-runtime -p
khive-pack-memory. All green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ohdearquant
approved these changes
Jul 9, 2026
ohdearquant
left a comment
ohdearquant
left a comment
Owner
There was a problem hiding this comment.
Verified at content level across all four fixes: (1) note tags via merge_note_tags into properties — consistent with memory.remember's existing path, no new storage shape; (2) neighbors' deleted-screen now consults both tables, correctly scoped as a view-layer read with the edge-survival test proving data-vs-view is honored (the edge row untouched, only the view filtered); (3) prefix dedup with the extra test driving the duplicate through the early-exit path — that test is what makes the fix trustworthy rather than coincidental; (4) the ANN generation design is the right shape: snapshot-before-scan, install-only-if-fresher replacing or_insert, and is_current on the recall gate so a stale winner is a cache miss, not a silent serve. The codex r2 REJECT (merge_note + atomic/reindex bypassing the hook) becoming the NoteMutationHookFn/PostCommitEffect seam is exactly how a fix round should widen. #752 correctly deferred — a persisted generation counter is an architectural change, not a rider.
DESIGN FORK RULING (short-prefix namespace scoping): KEEP the asymmetry — it is principled, not accidental. A full UUID is a demonstration that the caller knows the exact record, which is why ADR-007 makes by-ID ops namespace-agnostic. A short prefix is a RESOLUTION — a search — and searches default to namespace scoping. Making prefix resolution namespace-agnostic would let a prefix typed in one seat silently bind to another namespace's record, which is worse than the current behavior in every case that matters. Disposition: the known-bugs line converts to documented behavior — 'short-prefix resolution is namespace-scoped; cross-namespace targets require the full UUID' — in root CLAUDE.md and the verb help text, not a code change. Approved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four dogfooding fixes plus two codex fix-round commits, one review lane (6 commits):
create(kind=<note>)silently droppedtags. Note create now merges tags intoproperties["tags"](thememory.rememberconvention; notes have no native tags column). Top-leveltagswins over a conflictingproperties.tags.neighbors()missed soft-deleted note targets:deleted_entity_idsqueried onlyentities. Now UNIONs entities+notes with independently-numbered param blocks per UNION half. View-layer only; edges untouched.AmbiguousPrefixwhen the same UUID appears in two scanned tables.resolve_prefix_innernow dedups by UUID (including the mid-scan early-exit). Genuine distinct-UUID ambiguity still errors.entry().or_insert()could permanently install a mid-write corpus snapshot. Fix: generation captured before the corpus scan,install_if_fresherreplaces only on strictly-newer generation, read-sideis_currentgate on recall cache hits, monotonic per-model write counter. Thens733/ns733bbounded-retry test harnesses are removed (deterministic now; 150 stress runs clean).memory.remember. Added a pack-installedNoteMutationHookFnseam in khive-runtime (parallel to the existing entity-type-validator pattern);update_note/delete_notefire it post-mutation, the memory pack installs a hook invalidating onkind="memory",memory.prunebumps inline. Wired at both server boot paths. Three white-box regressions assertann::is_currentdirectly.merge_notenow fires the hook (same pattern asupdate_note) with a discriminating regression;kkernel --atomicregisters the hooks, theReindexNotepost-commit handler fires it, andDeletePlangained apost_commitseam (PostCommitEffect::NoteDeleted) covering atomic note deletes.Deliberately out of scope: the cross-process warm-cache coherence gap (an external
kkernel --atomic/kkernel reindexprocess can never invalidate a running daemon's in-memory generation counter) — filed as #752 with full analysis.Note for reviewer: brain.feedback full-UUID gotcha is NOT retired by #749
Verified at source:
brain.feedbackdoes route throughresolve_prefix_inner, but its full-UUID requirement is a separate namespace-scoping asymmetry — full-UUID lookups bypass namespace filtering (ADR-007 by-ID rule) while short-prefix resolution is namespace-scoped. The #749 dedup does not change that. Design fork worth a ruling: should short-prefix resolution be namespace-agnostic like by-ID ops?Test plan
cargo fmt --all -- --check,cargo clippy -p khive-pack-kg -p khive-runtime -p khive-pack-memory -p kkernel --all-targets -- -D warnings,cargo test -p khive-pack-kg -p khive-runtime -p khive-pack-memory— all exit 0.Closes #747, closes #748, closes #749, closes #750.