[Audit] KB store, search, federation — 6 verified findings (5 medium, 1 low)

[cuttlefisch]

Batched findings for KB store, search, federation from the pre-v0.15 codebase audit (branch audit/pre-v015-review).

Each was produced by an end-to-end capability trace, then independently re-verified by a reviewer whose
brief was to refute it. Only findings that survived refutation appear here, at the verifier's corrected
severity — across the audit, 102 claims became 89 confirmed and 29 were downgraded.

6 findings — 5 medium, 1 low. Tick them off individually; split any one
out into its own issue if it needs real design work.

---

1. MCP kb_health reports false orphans: issue #474's cross-instance reconciliation landed only on the human's :kb-health path

bug · medium · already tracked in #118 (thin-primary half only)

Impact. With a primary KB plus any kb-register'd instance (the shipped default already registers MaePractices/DevPractices), a node whose only inbound link lives in a sibling instance is listed under orphan_nodes by the AI's kb_health and NOT by the user's :kb-health. An agent asked to "clean up orphans" acts on a list the human cannot reproduce — and kb-cleanup-orphans exists as a command. Secondarily, because this path reads editor.kb.primary (the in-memory mirror) rather than the store, under a thin primary (primary_thin(), daemon-hosted) the AI's kb_health reports total_nodes: 0 as a successful result while :kb-health reports the real figures.

Evidence

Human path — crates/core/src/editor/option_ops.rs:2009-2013 routes through the query layer:

let store_report = self
    .kb
    .query_layer()
    .and_then(|q| q.health_report())
    .or_else(|| self.kb.store.as_ref().and_then(|s| s.health_report().ok()));

which lands in FederatedQuery::health_report, shared/kb/src/query.rs:628-630:

merged
    .orphan_ids
    .retain(|id| !global_in_degree.contains_key(id));

(comment at query.rs:617-627: "a node whose only real incoming link comes from a sibling instance was wrongly reported orphaned. Fixed below...").

AI path — crates/ai/src/tool_impls/kb.rs:617-621 does NOT use the query layer at all:

let report = if include_local {
    Some(editor.kb.primary.health_report_with_visibility(
        |id| editor.kb.instances.values().any(|kb| kb.contains(id)),
        is_visible(primary_restricted),
    ))

and in health_report_with_visibility (shared/kb/src/lib.rs:2370-2379) the external_contains closure is consumed ONLY by the broken-link check at lib.rs:2359; the orphan branch never sees it:

if node.kind != NodeKind::Index {
    let has_outgoing = !link_pairs.is_empty();
    let has_incoming = self
        .links_in
        .get(id.as_str())
        .is_some_and(|v| !v.is_empty());
    if !has_outgoing && !has_incoming {
        acc.orphan_ids.push(id.clone());
    }
}

The per-instance loop at kb.rs:645-661 has the same shape and the same omission.

Verification

The two cited code paths are quoted accurately. Human path, crates/core/src/editor/option_ops.rs:2009-2013, is verbatim let store_report = self.kb.query_layer().and_then(|q| q.health_report()).or_else(...), and FederatedQuery::health_report (shared/kb/src/query.rs:628-630) does merged.orphan_ids.retain(|id| !global_in_degree.contains_key(id)); with the #474 comment above it. AI path, crates/ai/src/tool_impls/kb.rs:617-621, is verbatim editor.kb.primary.health_report_with_visibility(|id| editor.kb.instances.values().any(|kb| kb.contains(id)), is_visible(primary_restricted)). And in shared/kb/src/lib.rs:2330-2385 the external_contains closure is consumed ONLY in the broken-link arm (if !all_ids.contains(target.as_str()) && !external_contains(target)); the orphan arm reads only self.links_in.get(id.as_str()), i.e. the owning KB's own reverse index, which is built solely from that KB's own nodes (lib.rs:815-816, 951). So the divergence is real: MCP kb_health's orphan_nodes is NOT federation-reconciled, :kb-health's is. Issue #474 is CLOSED and the ADR-065 addendum (docs/adr/065-kb-daemon-drift-corrections.md:193-234) describes the fix as landing in FederatedQuery only. WHY THE SEVERITY IS INFLATED: the claimed data-loss consequence does not follow. Editor::kb_cleanup_orphans (crates/core/src/editor/kb_ops/watchers.rs:373-379) computes its own list from self.kb.query_layer() — let orphan_ids: Vec<String> = if let Some(q) = self.kb.query_layer() { q.health_report().map(|r| r.orphan_ids)... } — i.e. the RECONCILED list, with an explicit #485 doc comment saying so. An agent that reads kb_health and then runs kb-cleanup-orphans deletes from the correct list; only the reported list is wrong. The secondary thin-primary claim (total_nodes: 0) is separately already tracked: issue #118 explicitly names health_report_with / 'cross-federation health' as one of the non-query_layer-first read sites under a thin mirror, and marks it 'lower impact'.

Scope correction from verification: MCP kb_health's orphan_nodes list (and each instance's orphan_count) is computed from the in-memory KnowledgeBase without #474's federation-wide in-degree reconciliation, so it can report false orphans that :kb-health does not — a report-only parity divergence (principle #3/#8). It is NOT a data-loss vector: kb_cleanup_orphans independently re-derives its deletion list from query_layer().health_report(). The thin-primary total_nodes: 0 half is already tracked by #118.

---

2. Federated search picks the winning copy of a duplicate node id by std HashMap iteration order — nondeterministic per process

bug · medium · already tracked in #118 (same function, different framing)

Impact. Two federated KBs holding the same node id (routine after kb_join of a shared KB that overlaps a local one, or the ADR-062 duplicate-id class the code explicitly cites) return a different title/body/instance label for the same query on different runs of the same binary against the same data. Result ordering among federated hits is randomized too, so kb_search / kb_search_context / kb_vector_search / :kb-find are all non-reproducible. It also means ADR-062 Phase B's priority field and Phase B's fan-out cap are inert for every user-facing search — there is no test in crates/core/src/editor/kb_ops/tests/kb_ops_search_federation_tests.rs asserting duplicate-id resolution or repeat-run stability (principle #14).

Evidence

crates/core/src/editor/kb_ops/search.rs:486-506 (the body of kb_federated_search_scoped_impl, which every search surface funnels through):

for (uuid, kb) in &self.kb.instances {
    ...
    for id in rank(kb) {
        if let Some(node) = kb.get(&id) {
            if seen_ids.insert(node.id.clone()) {
                results.push((inst_name.clone(), node.clone()));
            }
        }
    }
}

instances is declared at crates/core/src/editor/kb_state.rs:152:

pub instances: HashMap<String, mae_kb::KnowledgeBase>,

(use std::collections::{HashMap, HashSet};, kb_state.rs:5). seen_ids keeps the FIRST occurrence, so which instance's copy wins is whatever the randomized HashMap iteration yields.

The fix for exactly this exists — in a layer this path never calls. shared/kb/src/query.rs:294-304 documents FederatedQuery::priority_ordered_instances as replacing "whichever instance was registered/iterated first" with an explicit rule, naming "the org-roam #1480/#1496 duplicate-id failure class", and query.rs:377-398 implements priority-aware dedup. FederatedQuery is built at kb_state.rs:484 and is reachable via query_layer(), but kb_federated_search_scoped_impl only touches it in the primary_thin() branch (search.rs:459-473). kb_find_candidates (search.rs:176) iterates the same HashMap.

Verification

Every specific in the evidence checks out. crates/core/src/editor/kb_state.rs:5 is use std::collections::{HashMap, HashSet}; and :152 is pub instances: HashMap<String, mae_kb::KnowledgeBase>,. crates/core/src/editor/kb_ops/search.rs:486 is for (uuid, kb) in &self.kb.instances { with if seen_ids.insert(node.id.clone()) keeping the FIRST occurrence, and search.rs:459 shows the primary_thin() branch is indeed the only place query_layer() is touched. kb_find_candidates iterates the same map (for kb in self.kb.instances.values(), search.rs:176). The fix does exist one layer up and is unused here: shared/kb/src/query.rs:294-317 priority_ordered_instances with the documented stable-sort + 'org-roam #1480/#1496 duplicate-id failure class' rationale, and query.rs:377-398's priority-keyed by_id dedup. Default kb_search_sort is "relevance" (crates/core/src/options.rs:461-463), so no re-sort masks the iteration order. I confirmed crates/core/src/editor/kb_ops/tests/kb_ops_search_federation_tests.rs contains no duplicate-id-resolution or repeat-run-stability test (its federation tests are kb_federated_search_finds_across_instances, ..._scope_filters_instances, ..._scope_project_never_leaks_across_projects). NARROWING: (a) the primary is always ranked first and its ids are inserted into seen_ids before any instance, so a primary-vs-instance duplicate is deterministic — only instance-vs-instance duplicates pick a random winner; (b) within a single process the HashMap order is fixed, so the nondeterminism is across restarts, not query-to-query; (c) the same function is already flagged as the highest-impact non-query_layer-routed read site in open issue #118, whose prescribed fix (route ranked search through the query layer) would resolve this as a side effect.

Scope correction from verification: kb_federated_search_scoped_impl and kb_find_candidates iterate kb.instances (a std HashMap) directly instead of FederatedQuery::priority_ordered_instances, so among FEDERATED instances the relative result order — and, when two instances hold the same node id, which copy wins — varies between process runs. Primary-vs-instance collisions are deterministic (primary always first). ADR-062 Phase B's priority and fan-out cap are inert on this path. The function is already named as the top item in #118.

---

3. A storage or daemon failure is returned as an empty result set, and the degradation flag built for exactly this has no production consumer

bug · medium

Impact. A corrupt/locked cozo store, or a dead mae-daemon socket under daemon_mode on-demand/shared, makes :kb-find, kb_list, :kb-agenda, and the help buffer report zero results as a successful query — indistinguishable from an empty KB. Under a thin primary (primary_thin(), where the in-memory mirror is deliberately empty and the daemon is the only source) the entire knowledge base silently vanishes with a debug!-level line the user never sees. This is a failure reported as success on the read path of the subsystem, and the mechanism written to prevent it is dead code in every shipping build.

Evidence

shared/kb/src/query.rs:172-230 — every CozoQueryLayer read except get swallows the error:

fn search(&self, query: &str, limit: usize) -> Vec<SearchHit> {
    self.store.fts_search(query, limit).unwrap_or_default()
}
...
fn list_ids(&self, prefix: Option<&str>) -> Vec<String> {
    self.store.list_ids(prefix).unwrap_or_default()
}
...
fn agenda(&self, filter: &crate::AgendaFilter) -> Vec<Node> {
    self.store.agenda_query(filter).unwrap_or_default()
}

(get at query.rs:158-166 does tracing::warn!; nothing else even logs.)

shared/kb/src/lru_query.rs:193-199 — the daemon-routed layer does the same at tracing::debug! level:

match result {
    Ok(val) => parse_search_hits(&val),
    Err(e) => {
        tracing::debug!(error = %e, "LruQueryLayer: search failed");
        Vec::new()
    }
}

links_from/links_to/list_ids/health/history at lru_query.rs:228, :261, :287, :317, :351, :378, :393, :407, :430, :451 follow the identical Err(e) => { debug!; empty } shape.

The designed signal exists — KbQueryLayer::degraded() (query.rs:121-130, "ADR-062 Phase E ... the 'timeout-and-continue degradation contract'") and FederatedQuery::last_query_was_partial() (query.rs:278). Neither CozoQueryLayer nor LruQueryLayer overrides degraded(). A repo-wide grep for last_query_was_partial|degraded() outside query.rs returns only shared/kb/src/remote_hub.rs:659, :724, :738 — its own doc comment and two of its own unit-test assertions. No editor, MCP, or Scheme surface reads it. The only implementor that sets it (RemoteHubQueryLayer::degraded, remote_hub.rs:277-279) is behind the default-off remote-hub feature (crates/core/Cargo.toml:14).

Verification

The code facts are accurate. shared/kb/src/query.rs:172-230: search = self.store.fts_search(query, limit).unwrap_or_default(), list_ids/links_from/links_to/id_title_pairs/related/todo_nodes/agenda/history all .unwrap_or_default(), health_report/neighborhood .ok(); only get (:158-166) logs, at tracing::warn!. shared/kb/src/lru_query.rs has 12 LruQueryLayer: ... failed sites, all Err(e) => { tracing::debug!(...); Vec::new() }. degraded() exists at query.rs:121-130 and last_query_was_partial() at :278; a repo-wide grep for degraded()|last_query_was_partial outside query.rs returns only shared/kb/src/remote_hub.rs:659 (doc comment), :724, :738 (its own tests) — no editor, MCP or Scheme consumer. Confirmed that neither CozoQueryLayer nor LruQueryLayer overrides degraded().

Scope correction from verification: Two framing errors worth correcting. (a) degraded() was not 'built for exactly this': ADR-062 Phase E (docs/adr/062-...md:177-187, 397-418) scopes it explicitly to a slow/unreachable RemoteHub, and its own doc comment says 'Default false for every layer with nothing to degrade (local Cozo reads don't have a network failure mode)'. (b) It is not entirely unconsumed — FederatedQuery polls it at query.rs:361 and exposes it via FederatedQuery::degraded() (query.rs:702), covered by tests at :1570-1625. The accurate narrower finding: local Cozo read errors and dead-daemon LruQueryLayer RPC failures are returned as successful empty result sets with only warn/debug logging, and ADR-062's partial-result flag has no user- or AI-visible surface anywhere in the editor, MCP or Scheme, so a degraded or failed KB read is indistinguishable from an empty KB at every surface.

---

4. Batch import never retracts a file's nodes when the file stops parsing to any node

bug · medium

Impact. A user deletes the :ID: drawer, comments out the heading, or empties an .org file that previously contributed nodes. :kb-reimport (even in Full mode) leaves those nodes in the cozo store permanently — they keep appearing in kb_search/:kb-find results and in link targets, with a source_file that no longer produces them, and no reimport will ever clear them because the file still exists on disk. Only deleting the file entirely triggers cleanup.

Evidence

shared/kb/src/federation.rs:928-932, inside import_org_dir_to_store:

let parse_result = parse_org_multi_result(&content);
if parse_result.nodes.is_empty() {
    report.nodes_skipped += 1;
    continue;
}

The continue skips store.record_source_file(&file_path_str, &content_hash, mtime, &file_node_ids) at federation.rs:1002 — which is the ONLY retraction mechanism, shared/kb/src/cozo_store/source_files.rs:38-41:

let prev_ids = self.get_source_file_node_ids(file_path)?;
for old_id in prev_ids.iter().filter(|id| !node_ids.contains(id)) {
    self.delete_node(old_id)?;
}

And the file cannot be caught by Full-mode deletion sweep either, because it was inserted into visited_files at federation.rs:894, before the parse, so the check at federation.rs:1008-1018 (if !visited_files.contains(&tracked_path)) skips it.

The editor's single-file path was hardened against exactly this class — crates/core/src/editor/kb_ops/search.rs:301-308 explicitly retracts "ids this path no longer produces" and its comment cites issues #498/#502 and principle #15. The batch importer was not.

Verification

Confirmed by reading the whole loop. shared/kb/src/federation.rs:892-893 inserts file_path_str into visited_files BEFORE the file is read or parsed; :928-932 let parse_result = parse_org_multi_result(&content); if parse_result.nodes.is_empty() { report.nodes_skipped += 1; continue; }. That continue skips store.record_source_file(&file_path_str, &content_hash, mtime, &file_node_ids)? at :1002, and record_source_file is the only retraction mechanism — shared/kb/src/cozo_store/source_files.rs:38-41 let prev_ids = self.get_source_file_node_ids(file_path)?; for old_id in prev_ids.iter().filter(|id| !node_ids.contains(id)) { self.delete_node(old_id)?; }, whose own doc comment (:25-31) says deletion-detection in federation.rs 'only fires on whole-file removal'. The Full-mode sweep at :1007-1019 (if !visited_files.contains(&tracked_path)) cannot catch it because the path was already inserted at :893. The single-file editor path was hardened against exactly this at crates/core/src/editor/kb_ops/search.rs:301-308 ('Retract ids this path no longer produces'), so the two paths genuinely diverge — principle #15.

---

5. kb_vector_search RRF fusion has nondeterministic tie ordering — the exact defect the sibling implementation documents guarding against

bug · medium

Impact. RRF scores collide constantly by construction: a node appearing only in the lexical list at rank k and a node appearing only in the vector list at rank k both score exactly 1/(60+k+1). Every such pair is a tie, so kb_vector_search (crates/ai/src/tool_impls/kb.rs:2377) returns a different ordering for the same query, same KB, same embeddings across runs — and since the caller truncates to limit, the set of returned nodes changes too, not just their order. There is no repeat-run determinism test on this path, unlike the ADR-062 Phase B one the query.rs comment cites.

Evidence

crates/core/src/editor/kb_ops/search.rs:562-592, rrf_blend_with_vector:

let mut entries: std::collections::HashMap<String, (Option<String>, mae_kb::Node)> =
    std::collections::HashMap::new();
...
let mut fused: Vec<(f64, Option<String>, mae_kb::Node)> = entries
    .into_iter()
    .filter_map(|(id, (inst, node))| score.get(&id).map(|s| (*s, inst, node)))
    .collect();
fused.sort_by(|a, b| b.0.partial_cmp(&a.0).unwrap_or(std::cmp::Ordering::Equal));

No secondary key. sort_by is stable, so ties preserve the randomized HashMap::into_iter order.

The sibling implementation spells out why this is wrong — shared/kb/src/query.rs:400-414:

// Deterministic final order: score descending, id ascending as an explicit
// tiebreak. `by_id` is a `HashMap`, whose iteration order is process-randomized —
// without this explicit secondary key, two hits tied on score could come out in a
// different order on different runs even for byte-identical input ...
hits.sort_by(|a, b| {
    b.score
        .partial_cmp(&a.score)
        .unwrap_or(std::cmp::Ordering::Equal)
        .then_with(|| a.id.cmp(&b.id))
});

Verification

Both quotes are exact. crates/core/src/editor/kb_ops/search.rs:556-593 builds entries: HashMap<String, (Option<String>, Node)>, collects fused from entries.into_iter(), and sorts with fused.sort_by(|a, b| b.0.partial_cmp(&a.0).unwrap_or(Ordering::Equal)); — no secondary key, and Rust's sort_by is stable, so ties preserve randomized HashMap iteration order. The sibling at shared/kb/src/query.rs:399-414 carries the explicit counter-comment ('by_id is a HashMap, whose iteration order is process-randomized — without this explicit secondary key ... the exact nondeterminism the ADR-062 Phase B adversarial test — 20 repeated identical queries must produce identical ordering — exists to catch') and does .then_with(|| a.id.cmp(&b.id)). Tie collisions are structural as claimed: a lexical-only node at rank k and a vector-only node at rank k both score 1/(60+k+1). The downstream truncation is real too — crates/ai/src/tool_impls/kb.rs:2377 calls kb_federated_search_scoped_with_vector and then .take(limit), so the returned set, not just the order, can vary run to run. The only stabilizer is the use_alpha branch (search.rs:530-531 results.sort_by(|a, b| a.1.id.cmp(&b.1.id))), which does not apply in the default relevance sort mode.

Scope correction from verification: One scope note: the path is only reachable when embeddings are configured and search_cached_embeddings returns hits (search.rs:514-528), i.e. opt-in Ollama-backed vector search, not the default kb_search/:kb-find path.

---

6. kb_federated_max_fanout_instances is settable at runtime but does not affect the live query layer

missing-config · low

Impact. :set kb_federated_max_fanout_instances=8 reports success and :set echoes the new value, but the already-constructed FederatedQuery keeps the old cap until an unrelated event (a kb-register, a watcher drain, a node write) happens to rebuild it — so the setting silently applies at an arbitrary later time, or never in a session that registers nothing. And the sibling knob the option's own documentation points at for controlling which instances get dropped is unreachable from all four surfaces without hand-editing the registry JSON.

Evidence

crates/core/src/editor/option_ops.rs:967-972 — the whole arm:

"kb_federated_max_fanout_instances" => {
    let v: usize = value
        .parse()
        .map_err(|_| format!("Invalid integer: '{}'", value))?;
    self.kb.federated_max_fanout_instances = v.clamp(1, 100_000);
}

The cap is only pushed into the built layer inside rebuild_query_layer (crates/core/src/editor/kb_state.rs:485):

federated.set_max_fanout_instances(self.federated_max_fanout_instances);

Editor::set_option (option_ops.rs:384) is a flat match with no post-set hook, and a repo-wide grep shows rebuild_query_layer() is called only from kb_ops/nodes.rs:107, kb_ops/watchers.rs:151, kb_ops/registry.rs:231/:420/:557, kb_ops/sync.rs:253, ai/executor/collab_exec.rs:836 and mae/src/bootstrap.rs:2585 — never from the options path.

Related, same option's doc string (crates/core/src/options.rs:469) refers to "the lowest-priority instances (kb_instances priority)", but KbInstance::priority (shared/kb/src/federation.rs:173-180, "replaces the previous implicit ... rule with an explicit, user-controllable one") is assigned priority: 0 at every construction site in the repo and is exposed by no command, option, Scheme primitive or MCP tool.

Verification

Confirmed exactly. crates/core/src/editor/option_ops.rs:967-972 is the whole arm and only assigns self.kb.federated_max_fanout_instances = v.clamp(1, 100_000);. The cap reaches the live layer only at crates/core/src/editor/kb_state.rs:485 federated.set_max_fanout_instances(self.federated_max_fanout_instances); inside rebuild_query_layer, and rebuild_query_layer() is called from kb_ops/nodes.rs:107, kb_ops/watchers.rs:151, kb_ops/registry.rs:231/420/557, kb_ops/sync.rs:253, ai/executor/collab_exec.rs:836, mae/src/bootstrap.rs:2585 (plus tests) — never from set_option. KbState::query_layer() (kb_state.rs:286-288) just returns the stored layer, no lazy rebuild. The priority sub-claim also holds: KbInstance.priority (shared/kb/src/federation.rs:180) is priority: 0 at every construction site (federation.rs:465, 511, 1169, 1689; kb_ops/registry.rs:978, 1069; tool_impls/kb.rs:2427, 4130, ...) and is exposed by no option, command, Scheme primitive or MCP tool, while the option's own doc string (options.rs:469) points at it as the mechanism that decides which instances get dropped.

Scope correction from verification: Real but low impact. The option is an internal worst-case-cost bound (default 128, per ADR-062 Phase B's documented rationale at shared/kb/src/query.rs:231-240), not a behaviour users observe in results; a stale cap only means fan-out stays bounded at the previous value until the next rebuild (any kb-register/unregister, node write, watcher drain, or sync). It never produces wrong results, and the registry would have to exceed 128 instances for the setting to matter at all. The genuinely reachable-only-by-hand-editing KbInstance::priority is the more defensible half of this finding.

[cuttlefisch]

Part of the pre-v0.15 codebase audit — tracking epic #592.