From 08b785cfacd21dac4ac6ffd8147fd80410ab0eff Mon Sep 17 00:00:00 2001 From: Will Anderson Date: Mon, 22 Jun 2026 13:42:33 -0500 Subject: [PATCH] fix(recall): address all five code-review issues in context-dedup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue 1 — cache read-before-write: move engram_compile_multi call to before the affective_prefix block in handle_chat. engram_compile writes "engram_compile_bell_node" to state; the previous ordering meant the first-turn affective prefix always read an empty cache even when a recent bell node existed. Issue 2 — double-write clobber: engram_compile_multi now saves the primary-seed activation ("engram_compile_primary_activation_json") after the first engram_compile call, before the secondary call can overwrite the shared "engram_compile_activation_json" key. strengthen_chat_nodes now prefers the primary key, falling back only when absent. Issue 3 — mid-object truncation in engram_compile_multi: replace the dumb str_slice(merged, 0, 6000) with the same safe JSON boundary-scan (last closing brace before cap) already used in engram_compile, so ctx1+ctx2+ctx3 over 6000 chars never produces a torn JSON object. Issue 4 — heuristic regression in is_genuine_continuation: add explicit question-word prefix detection (what/how/why/when/where/who/which/is/ can/could/does/do/explain/describe/define) that fires before the 50-char length gate. A message starting with a question word is always a new topic, regardless of length, so "what is rust?" (14 chars, all-lowercase, no mid-capitals) correctly returns false instead of true. Issue 5 — unreliable dedup via str_contains: remove the substring duplicate checks in engram_compile_multi. str_contains across multi-KB JSON strings is not a reliable deduplication mechanism — coincidental field-value matches suppress valid context, and truncated ctx1 misses genuine duplicates. We now concatenate ctx1+ctx2+ctx3 unconditionally and accept minor node redundancy in exchange for correctness. --- chat.el | 108 +++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 92 insertions(+), 16 deletions(-) diff --git a/chat.el b/chat.el index 7196af8..86038d1 100644 --- a/chat.el +++ b/chat.el @@ -139,14 +139,40 @@ fn is_followup_phrase(msg: String) -> Bool { } // is_genuine_continuation — returns true when a short message is a contextual -// follow-up rather than a new topic. Fixes Issue 2: the old threshold (str_len < 50) -// conflated new-topic short messages like "explain quantum tunneling" (49 chars) -// with genuine follow-ups like "ok", "yes", or "what do you think?". +// follow-up rather than a new topic. +// Issue 4 fix: the prior heuristic only checked for mid-string capitals, which +// fails for all-lowercase new-topic queries like "what is rust?" (14 chars) or +// "explain quantum computing" (26 chars). Added question-word prefix detection +// that fires BEFORE the length check: any message starting with a question word +// (what/how/why/when/where/who/which/is/can/could/does/do) introduces a new +// topic and is never a continuation, regardless of length. fn is_genuine_continuation(msg: String, hist_len: Int) -> Bool { if hist_len == 0 { return false } if str_len(msg) == 0 { return false } if is_followup_phrase(msg) { return true } + // Question-word prefix: messages starting with these introduce new topics. + // Check before the length heuristic so short new-topic questions escape. + let is_question_start: Bool = str_starts_with(msg, "what ") + || str_starts_with(msg, "What ") + || str_starts_with(msg, "how ") || str_starts_with(msg, "How ") + || str_starts_with(msg, "why ") || str_starts_with(msg, "Why ") + || str_starts_with(msg, "when ") || str_starts_with(msg, "When ") + || str_starts_with(msg, "where ") || str_starts_with(msg, "Where ") + || str_starts_with(msg, "who ") || str_starts_with(msg, "Who ") + || str_starts_with(msg, "which ") || str_starts_with(msg, "Which ") + || str_starts_with(msg, "is ") || str_starts_with(msg, "Is ") + || str_starts_with(msg, "can ") || str_starts_with(msg, "Can ") + || str_starts_with(msg, "could ") || str_starts_with(msg, "Could ") + || str_starts_with(msg, "does ") || str_starts_with(msg, "Does ") + || str_starts_with(msg, "do ") || str_starts_with(msg, "Do ") + || str_starts_with(msg, "explain ") || str_starts_with(msg, "Explain ") + || str_starts_with(msg, "describe ") || str_starts_with(msg, "Describe ") + || str_starts_with(msg, "define ") || str_starts_with(msg, "Define ") + if is_question_start { return false } + // Long messages (50+ chars) typically introduce new topics. if str_len(msg) >= 50 { return false } + // Short messages with a mid-string capital are likely named-concept queries + // (e.g. "tell me about Rust", "what about AWS") — treat as new topic. let rest: String = str_slice(msg, 1, str_len(msg)) let has_mid_capital: Bool = false let has_mid_capital = has_mid_capital || str_contains(rest, " A") @@ -319,9 +345,34 @@ fn build_activation_seed(message: String, hist: String, hist_len: Int) -> String // engram_compile_multi — fan-out activation across multiple query seeds. Fixes Issue 4: // only a single seed was tried per turn, with no entity/emotion/topic diversification. +// +// Issue 2 fix: save the primary-seed activation to a dedicated state key BEFORE calling +// engram_compile(message). Each engram_compile call overwrites "engram_compile_activation_json" +// with its own activation result. Without this save, the secondary compile (bare message, +// lower signal) clobbers the primary (enriched seed, higher signal), and strengthen_chat_nodes +// later reads the lower-signal result for node strengthening. +// +// Issue 3 fix: replace the dumb str_slice(merged, 0, 6000) truncation with the same +// safe JSON boundary-scan used in engram_compile. The old truncation could cut mid-object +// when ctx1+ctx2+ctx3 together exceeded 6000 chars, producing malformed JSON context. +// +// Issue 5 fix: remove str_contains(ctx1, ctx2) / str_contains(merged, ctx3) substring +// duplicate checks. These compared multi-KB JSON strings and were unreliable in both +// directions: a coincidental substring match inside a JSON field value could falsely suppress +// ctx2 entirely; a genuinely duplicate ctx2 was missed when ctx1 was already truncated. +// We now concatenate unconditionally and let engram_compile's own dedup (node-ID based) +// handle within-result duplicates. Slight redundancy across ctx1/ctx2 is acceptable; false +// suppression of valid context is not. fn engram_compile_multi(primary_seed: String, message: String) -> String { let ctx1: String = engram_compile(primary_seed) + // Issue 2 fix: save the primary-seed activation before any secondary compile can + // overwrite the shared "engram_compile_activation_json" state key. + let primary_act: String = state_get("engram_compile_activation_json") + if !str_eq(primary_act, "") && !str_eq(primary_act, "[]") { + state_set("engram_compile_primary_activation_json", primary_act) + } + let entity_seed_differs: Bool = !str_eq(primary_seed, message) let ctx2: String = if entity_seed_differs { let raw_ctx: String = engram_compile(message) @@ -335,19 +386,29 @@ fn engram_compile_multi(primary_seed: String, message: String) -> String { if emo_ok { engram_compile_ranked(emo_results, 3) } else { "" } } else { "" } - let merged: String = ctx1 - let sep2: String = if !str_eq(merged, "") && !str_eq(ctx2, "") { "\n" } else { "" } - let merged = if !str_eq(ctx2, "") && !str_contains(ctx1, ctx2) { - merged + sep2 + ctx2 - } else { merged } + // Issue 5 fix: concatenate unconditionally — no str_contains substring dedup. + let sep2: String = if !str_eq(ctx1, "") && !str_eq(ctx2, "") { "\n" } else { "" } + let merged: String = ctx1 + sep2 + ctx2 let sep3: String = if !str_eq(merged, "") && !str_eq(ctx3, "") { "\n" } else { "" } - let merged = if !str_eq(ctx3, "") && !str_contains(merged, ctx3) { - merged + sep3 + ctx3 - } else { merged } + let merged = if !str_eq(ctx3, "") { merged + sep3 + ctx3 } else { merged } if str_eq(merged, "") { return "" } - if str_len(merged) > 6000 { return str_slice(merged, 0, 6000) } - return merged + + // Issue 3 fix: safe JSON boundary-scan truncation — find the last closing brace + // before the 6000-char cap rather than slicing mid-object. + let cap_len: Int = 6000 + if str_len(merged) <= cap_len { return merged } + let cap_search: Int = cap_len - 1 + let cap_min: Int = if cap_len > 500 { cap_len - 500 } else { 0 } + let cap_pos: Int = -1 + let cap_si: Int = cap_search + while cap_si >= cap_min && cap_pos < 0 { + let cap_ch: String = str_slice(merged, cap_si, cap_si + 1) + let cap_pos = if str_eq(cap_ch, "}") { cap_si } else { cap_pos } + let cap_si = if cap_pos < 0 { cap_si - 1 } else { cap_si } + } + if cap_pos > 0 { return str_slice(merged, 0, cap_pos + 1) } + return str_slice(merged, 0, cap_len) } fn engram_compile(intent: String) -> String { @@ -636,7 +697,15 @@ fn handle_chat(body: String) -> String { // and tail-biased snipping from long assistant replies. let activation_seed: String = build_activation_seed(message, stored_hist, hist_len) + // Issue 1 fix: call engram_compile_multi BEFORE reading the bell-node cache. + // engram_compile (called inside engram_compile_multi) writes "engram_compile_bell_node" + // at line 426. Reading the cache before the compile call means the first session turn + // always sees an empty cache — the very turn where safety continuity matters most. + // Moving compile first ensures the cache is populated before affective_prefix reads it. + let ctx: String = engram_compile_multi(activation_seed, message) + // Fix Issue 2: reuse cached bell result from engram_compile — no second engram query. + // Now runs AFTER engram_compile_multi so the cache is guaranteed to be warm. let affective_prefix: String = if hist_len == 0 { let cached_bell: String = state_get("engram_compile_bell_node") if !str_eq(cached_bell, "") { @@ -644,8 +713,6 @@ fn handle_chat(body: String) -> String { } else { "" } } else { "" } - // Issue 4 fix: engram_compile_multi adds entity + emotion fan-out seeds - let ctx: String = engram_compile_multi(activation_seed, message) let system: String = affective_prefix + build_system_prompt(ctx) // First message of the session: proactively load user profile and active work context. @@ -755,7 +822,16 @@ fn handle_chat(body: String) -> String { conv_history_persist(final_hist) // Fix Issue 7: reuse activation JSON from engram_compile — no third activate query. - let cached_act: String = state_get("engram_compile_activation_json") + // Issue 2 fix: prefer the primary-seed activation (enriched seed, depth 5) saved + // before the secondary compile could overwrite the shared state key. Fall back to + // the final compile activation only when the primary key is absent (e.g. first boot + // before any compile has run or when primary_seed == message and ctx2 was skipped). + let primary_cached: String = state_get("engram_compile_primary_activation_json") + let cached_act: String = if !str_eq(primary_cached, "") && !str_eq(primary_cached, "[]") { + primary_cached + } else { + state_get("engram_compile_activation_json") + } let act_out: String = if !str_eq(cached_act, "") && !str_eq(cached_act, "[]") { cached_act } else { "[]" }