From a39998a50274697a5bbef379404e843b236440ac Mon Sep 17 00:00:00 2001 From: Will Anderson Date: Mon, 22 Jun 2026 12:52:31 -0500 Subject: [PATCH] feat(recall): recall-reliability improvements - Q1: engram_numeric_valid() guard against non-numeric timestamps in bell scoring - Q2: soul-agnostic cold-start fallback in engram_compile (drops genesis-specific hardcoded node IDs) - Q3: partial-write guard and failure logging in conv_history_persist/load - Q4: document circuit-breaker limitation requiring C runtime support - Q5: println warnings on empty activation/search paths - Q6: load_identity_context warns when all identity fetches return empty - Q7: recall_status state tracking (ok/empty/unavailable) surfaced to LLM via MEMORY STATUS block - Q8: document shared-state race conditions in engram_recall_status and safety_system_addendum - CRITICAL BUG: conv_node_id empty check moved outside is_bell block so silent Conversation node loss is always logged --- chat.el | 195 ++++++++++++++++++++++++++++++++++++++++++++++---------- soul.el | 23 +++++++ 2 files changed, 185 insertions(+), 33 deletions(-) diff --git a/chat.el b/chat.el index c101aa8..595a5ad 100644 --- a/chat.el +++ b/chat.el @@ -12,34 +12,59 @@ fn chat_default_model() -> String { return "claude-sonnet-4-5" } +// engram_numeric_valid — guard for str_to_int: returns true only when s is a valid +// decimal number (integer or single-decimal-point float, optional leading minus). +// Q1 fix: rejects "", "null", "N/A", multi-dot strings ("1.2.3"), pure-letter strings. +// Prevents engram_score_node from passing malformed JSON field values to str_to_int +// which has undefined behaviour on non-numeric input and can corrupt score arithmetic. +fn engram_numeric_valid(s: String) -> Bool { + if str_eq(s, "") { return false } + if str_eq(s, "null") { return false } + if str_eq(s, "N/A") { return false } + if str_eq(s, "-") { return false } + let body: String = if str_starts_with(s, "-") { str_slice(s, 1, str_len(s)) } else { s } + if str_eq(body, "") { return false } + // Count dots: remove all, compare lengths. Allow at most one dot (float). + let no_dot: String = str_replace(body, ".", "") + let dot_count: Int = str_len(body) - str_len(no_dot) + if dot_count > 1 { return false } + if str_eq(no_dot, "") { return false } + // str_to_int on a letter-containing string returns 0; "0" is a valid zero. + let parsed: Int = str_to_int(no_dot) + if parsed == 0 && !str_eq(no_dot, "0") { return false } + return true +} + // engram_score_node — compute a recency x relevance score for a single engram // node JSON object. Higher is better. Score = salience * importance * recency_factor. // recency_factor decays linearly over 30 days: nodes updated today score 1.0, // nodes 30+ days old score 0.1 (floor). Nodes with no created_at score 0.5. // This keeps fresh, high-salience nodes at the top and pushes stale low-signal // nodes to the bottom so they get trimmed when we cap context size. +// Q1 fix: all three numeric fields validated with engram_numeric_valid before str_to_int. fn engram_score_node(node_json: String) -> Int { let salience_str: String = json_get(node_json, "salience") let importance_str: String = json_get(node_json, "importance") let created_str: String = json_get(node_json, "created_at") - // Parse as floats via * 100 integer arithmetic (el has no float math) - let salience_100: Int = if str_eq(salience_str, "") { 70 } else { + // Q1 fix: validate before str_to_int. Non-numeric values fall back to safe defaults. + // Parse as floats via * 100 integer arithmetic (el has no float math). + let salience_100: Int = if !engram_numeric_valid(salience_str) { 70 } else { let s: Int = str_to_int(str_replace(salience_str, ".", "")) - // Clamp to 0-100 range (value was e.g. "0.85" -> parsed "085" = 85) if s > 100 { 100 } else { if s < 0 { 0 } else { s } } } - let importance_100: Int = if str_eq(importance_str, "") { 70 } else { + let importance_100: Int = if !engram_numeric_valid(importance_str) { 70 } else { let v: Int = str_to_int(str_replace(importance_str, ".", "")) if v > 100 { 100 } else { if v < 0 { 0 } else { v } } } // Recency: decay from 100 (today) to 10 (30+ days). created_at is Unix seconds. let now_ts: Int = time_now() - let recency_100: Int = if str_eq(created_str, "") { 50 } else { + let recency_100: Int = if !engram_numeric_valid(created_str) { 50 } else { let created_ts: Int = str_to_int(created_str) let age_secs: Int = now_ts - created_ts - let age_days: Int = age_secs / 86400 + // Q1 fix: guard against clock skew / future timestamps — treat as fresh. + let age_days: Int = if age_secs < 0 { 0 } else { age_secs / 86400 } let decay: Int = if age_days >= 30 { 10 } else { 100 - (age_days * 3) } if decay < 10 { 10 } else { decay } } @@ -116,13 +141,23 @@ fn engram_compile_ranked(nodes_json: String, max_nodes: Int) -> String { return c9 } +// Q4 note: engram_compile has no cache or circuit-breaker at the EL layer. +// Every handle_chat call invokes engram_activate_json + engram_search_json unconditionally. +// If the engram backend is repeatedly unreachable (e.g., during startup or after a crash), +// every turn pays two failed RPC round-trips before reaching the cold-start fallback. +// A proper cache/circuit-breaker requires C runtime support (e.g., a shared "engram_healthy" +// flag set by the runtime, or a time-bucketed result cache in el_runtime.c). At the EL +// layer we can only detect failure after the fact (empty string return) and log it. fn engram_compile(intent: String) -> String { let activate_json: String = engram_activate_json(intent, 5) // Fetch more search results than we'll use so ranking has a real pool to pick from. let search_json: String = engram_search_json(intent, 20) - let act_ok: Bool = !str_eq(activate_json, "") && !str_eq(activate_json, "[]") - let srch_ok: Bool = !str_eq(search_json, "") && !str_eq(search_json, "[]") + // Q6/Q7 fix: track raw "" (engram down) vs "[]" (empty graph) to surface different warnings. + let act_failed: Bool = str_eq(activate_json, "") + let srch_failed: Bool = str_eq(search_json, "") + let act_ok: Bool = !act_failed && !str_eq(activate_json, "[]") + let srch_ok: Bool = !srch_failed && !str_eq(search_json, "[]") // Activation nodes (spreading activation) are already high-signal — keep all 5. let act_part: String = if act_ok { activate_json } else { "" } @@ -132,23 +167,31 @@ fn engram_compile(intent: String) -> String { let srch_ranked: String = if srch_ok { engram_compile_ranked(search_json, 8) } else { "" } let srch_part: String = srch_ranked - // Fallback: when vector search returns nothing (no embeddings), fetch pinned - // high-salience nodes by their known IDs. These are the canonical identity - // and biography nodes that should always be in context. - // engram_get_node_json(id) returns a single node as JSON or "" if missing. + // Q2 fix: soul-agnostic cold-start fallback. The previous code used two genesis-specific + // hardcoded node IDs ("knw-35940684..." and "knw-729fc901..."). Cultivated souls with a + // cold or empty vector index received zero episodic context with no error and no log. + // New fallback: search for Persona/Identity nodes seeded by seed_persona_from_env() + // which works for any soul regardless of which specific node IDs were created at seeding. + // Q6 fix: log a warning so the empty-recall path is visible in operator logs. let scan_part: String = if !act_ok && !srch_ok { - let family_node: String = engram_get_node_json("knw-35940684-abc4-42f0-b942-818f66b1f69a") - let origin_node: String = engram_get_node_json("knw-729fc901-8335-44c4-9f3a-b150b4aa0915") - let fam_ok: Bool = !str_eq(family_node, "") && !str_eq(family_node, "null") - let orig_ok: Bool = !str_eq(origin_node, "") && !str_eq(origin_node, "null") - let fam_str: String = if fam_ok { family_node } else { "" } - let orig_str: String = if orig_ok { origin_node } else { "" } - let sep: String = if fam_ok && orig_ok { "\n" } else { "" } - let combined: String = fam_str + sep + orig_str - if str_eq(combined, "") { "" } else { combined } + let engram_down: Bool = act_failed && srch_failed + if engram_down { + println("[chat] engram_compile: WARN engram_down — all calls returned empty string for intent=" + str_slice(intent, 0, 60)) + } else { + println("[chat] engram_compile: WARN cold-index — activation and search returned no results for intent=" + str_slice(intent, 0, 60)) + } + // Soul-agnostic fallback: search for Persona/Identity nodes by label. + let persona_fallback: String = engram_search_json("soul:persona Persona identity", 5) + let pf_ok: Bool = !str_eq(persona_fallback, "") && !str_eq(persona_fallback, "[]") + let combined: String = if pf_ok { engram_compile_ranked(persona_fallback, 3) } else { "" } + if str_eq(combined, "") { + println("[chat] engram_compile: WARN cold-start fallback also empty — LLM has no episodic context") + } + combined } else { "" } + let scan_ok: Bool = !str_eq(scan_part, "") // Affective context: always include the most recent high-emotion memory if one // exists within 72 hours. This ensures continuity of care across turns — when @@ -178,17 +221,31 @@ fn engram_compile(intent: String) -> String { let ca: String = json_get(bn0, "created_at") if str_eq(ca, "") { json_get(bn0, "updated_at") } else { ca } } - let bn_ts: Int = if str_eq(bn_ts_raw, "") { 0 } else { str_to_int(bn_ts_raw) } + // Q1 fix: validate bell timestamp before str_to_int. + let bn_ts: Int = if !engram_numeric_valid(bn_ts_raw) { 0 } else { str_to_int(bn_ts_raw) } if bn_ts > cutoff_ts { bn0 } else { "" } } else { "" } let affective_part: String = if !str_eq(recent_bell, "") { recent_bell } else { "" } + let affective_ok: Bool = !str_eq(affective_part, "") let sep1: String = if !str_eq(act_part, "") && !str_eq(srch_part, "") { "\n" } else { "" } let sep2: String = if (!str_eq(act_part, "") || !str_eq(srch_part, "")) && !str_eq(scan_part, "") { "\n" } else { "" } let sep3: String = if (!str_eq(act_part, "") || !str_eq(srch_part, "") || !str_eq(scan_part, "")) && !str_eq(affective_part, "") { "\n" } else { "" } let ctx: String = act_part + sep1 + srch_part + sep2 + scan_part + sep3 + affective_part - if str_eq(ctx, "") { return "" } + // Q7 fix: store recall status so build_system_prompt can include a hint to the LLM + // distinguishing "no memories yet" (cold start) from "memory system unreachable". + // Values: "ok" | "empty" | "unavailable" + let any_ok: Bool = act_ok || srch_ok || scan_ok || affective_ok + let all_failed: Bool = act_failed && srch_failed + let recall_status: String = if any_ok { "ok" } else { if all_failed { "unavailable" } else { "empty" } } + state_set("engram_recall_status", recall_status) + + if str_eq(ctx, "") { + // Q6 fix: log when ctx is empty after all recall paths so cold-start is visible. + println("[chat] engram_compile: all paths empty — recall_status=" + recall_status + " intent=" + str_slice(intent, 0, 60)) + return "" + } // Raise the cap slightly to match the ranked (higher-signal) output. if str_len(ctx) > 6000 { @@ -227,12 +284,33 @@ fn build_system_prompt(ctx: String) -> String { "\n\n[IDENTITY GRAPH — who you are, loaded from your engram]\n" + id_ctx } + // Q7 fix: if recall produced no results, include a hint so the LLM can respond + // authentically ("I seem to be starting fresh" vs "memory system may be down") + // rather than silently acting as if it has context it doesn't have. + // Q8 note: "engram_recall_status" is a shared state key under http_serve_async. + // Concurrent requests can overwrite each other's status. This is best-effort: + // a full fix requires per-request scoping (not feasible at EL layer without C support). + let recall_status: String = state_get("engram_recall_status") let engram_block: String = if str_eq(ctx, "") { - "" + let status_hint: String = if str_eq(recall_status, "unavailable") { + "\n\n[MEMORY STATUS]\nYour episodic memory system appears to be temporarily unreachable. You may not have access to memories from previous sessions. If asked about past conversations, acknowledge this honestly rather than confabulating." + } else if str_eq(recall_status, "empty") { + "\n\n[MEMORY STATUS]\nNo episodic memories were found for this topic. This may be a new soul or a new area of conversation. Respond naturally from your identity without fabricating memories." + } else { + "" + } + status_hint } else { "\n\n[ENGRAM CONTEXT — compiled from your graph]\n" + ctx } + // Q8 note: layered_cycle_safety_system_addendum is a shared mutable state key. + // Two concurrent requests can both read it (state_get), both see the same value, + // and one clears it (state_set("", "")) while the other uses the value — or both + // clear it and one request gets "" while expecting real content. The race is benign + // in practice (the addendum is only written by layered_cycle and read here once + // per turn; concurrent chat turns are rare in the current deployment), but a full + // fix requires per-session or per-request key scoping at the C runtime level. let safety_addendum: String = state_get("layered_cycle_safety_system_addendum") let safety_block: String = if str_eq(safety_addendum, "") { "" @@ -347,41 +425,79 @@ fn clean_llm_response(s: String) -> String { } // conv_history_persist — save conversation history to engram for cross-restart continuity. -// Stores as a Conversation node. Overwrites by using consistent label "conv:history". +// Stores as a Conversation node with consistent label "conv:history" (upsert by label). +// Q3/Q6 fix: added partial-write guard and failure logging. fn conv_history_persist(hist: String) -> Void { if str_eq(hist, "") { return "" } if str_eq(hist, "[]") { return "" } - let ts: Int = time_now() + // Partial-write guard: refuse to persist a blob that is not a complete JSON array. + // A truncated write starting with '[' but missing ']' would overwrite a good node. + if !str_starts_with(hist, "[") { return "" } + if !str_contains(hist, "]") { return "" } let tags: String = "[\"conv-history\",\"persistent\"]" - let discard: String = engram_node_full( + let node_id: String = engram_node_full( hist, "Conversation", "conv:history", el_from_float(0.7), el_from_float(0.8), el_from_float(0.9), "Episodic", tags ) + // Q6 fix: log write failure — silent history loss is now visible. + if str_eq(node_id, "") { + println("[chat] conv_history_persist: engram_node_full returned empty — history node may be lost") + } } // conv_history_load — restore conversation history from engram on first access. -// Returns the most recent "conv:history" node content, or "" if none found. +// Q3/Q6 fix: added partial-write guard, log on invalid content, and state flag for +// callers to distinguish genuine first-turn from a load failure. fn conv_history_load() -> String { + // Primary: label-based fetch — symmetric with persist, immune to vector index drift. + let label_node: String = engram_get_node_by_label("conv:history") + let label_ok: Bool = !str_eq(label_node, "") && !str_eq(label_node, "null") + if label_ok { + let label_content: String = json_get(label_node, "content") + let label_valid: Bool = str_starts_with(label_content, "[") && str_contains(label_content, "]") + if label_valid { + return label_content + } + println("[chat] conv_history_load: label node found but content invalid — falling back to vector search") + } + // Fallback: vector search. let results: String = engram_search_json("conv:history", 3) - if str_eq(results, "") { return "" } + if str_eq(results, "") { + // Q3 fix: set a state flag so callers can distinguish load failure from first turn. + state_set("conv_history_load_failed", "1") + return "" + } if str_eq(results, "[]") { return "" } let node: String = json_array_get(results, 0) let content: String = json_get(node, "content") - // Validate it looks like a JSON array - if !str_starts_with(content, "[") { return "" } + // Partial-write guard: require both '[' prefix AND ']' presence. + if !str_starts_with(content, "[") || !str_contains(content, "]") { + println("[chat] conv_history_load: vector search result content invalid — treating as first turn") + state_set("conv_history_load_failed", "1") + return "" + } return content } fn handle_chat(body: String) -> String { let message: String = json_get(body, "message") if str_eq(message, "") { - return "{\"error\":\"message is required\",\"response\":\"\"}" + return "{\"__status__\":400,\"error\":\"message is required\",\"response\":\"\"}" } // Load history BEFORE compiling context so we can anchor activation to the thread. + // Q3 fix: clear the load-failure flag before loading so it accurately reflects this call. + state_set("conv_history_load_failed", "") + // Q8 note: "conv_history" is a process-global state key. Concurrent /api/chat requests + // all read the same key, append their exchange, and write it back. Because _state_mu + // serializes individual state_get/state_set calls but NOT the read-append-write sequence, + // two concurrent requests can read the same base history and the last writer wins — one + // turn is silently dropped. A full fix requires per-session history keys (session_hist_) + // and deprecating the global "conv_history" path. Callers using session_id are not affected. let state_hist: String = state_get("conv_history") let stored_hist: String = if str_eq(state_hist, "") { conv_history_load() } else { state_hist } + let hist_load_failed: Bool = str_eq(state_get("conv_history_load_failed"), "1") let hist_len: Int = if str_eq(stored_hist, "") { 0 } else { json_array_len(stored_hist) } // Thread-aware activation: short/ambiguous messages (continuations like "go on", @@ -531,7 +647,13 @@ fn handle_chat(body: String) -> String { let act_out: String = if act_ok { activation_nodes } else { "[]" } strengthen_chat_nodes(act_out) - return "{\"response\":\"" + safe_response + "\",\"model\":\"" + model + "\",\"activation_nodes\":" + act_out + "}" + // Q3 fix: surface history load failure in the response envelope so callers can + // show a "starting fresh — could not load previous conversation" indicator. + let hist_warning: String = if hist_load_failed { + ",\"history_load_failed\":true" + } else { "" } + + return "{\"response\":\"" + safe_response + "\",\"model\":\"" + model + "\",\"activation_nodes\":" + act_out + hist_warning + "}" } fn handle_see(body: String) -> String { @@ -1536,6 +1658,13 @@ fn auto_persist(req: String, resp: String) -> Void { "Episodic", tags ) + // CRITICAL BUG fix: log conv_node_id failure OUTSIDE the is_bell block. + // The original code had this check inside the is_bell block (or missing entirely), + // making the log unreachable on every non-bell turn (the common case). This meant + // silent failure of the Conversation node write went unlogged on most turns. + if str_eq(conv_node_id, "") { + println("[chat] auto_persist: engram_node_full returned empty — conversation node lost (ts=" + ts_str + ")") + } // When a bell fires, write a dedicated BellEvent node in addition to the // Conversation node. This makes distress moments directly findable by label diff --git a/soul.el b/soul.el index c58b03d..2946ee8 100644 --- a/soul.el +++ b/soul.el @@ -148,6 +148,14 @@ fn load_identity_context() -> Void { println("[soul] identity context loaded (" + int_to_str(str_len(ctx)) + " chars, " + int_to_str(parts_count) + " nodes)") } + // Q6 fix: warn when all three identity node fetches return empty. For genesis this + // indicates a corrupted or missing graph. For cultivated souls it is expected on first + // boot (nodes are seeded by seed_persona_from_env, not these genesis-specific IDs). + // The log makes the silent-empty case visible instead of indistinguishable from success. + if parts_count == 0 { + println("[soul] load_identity_context: WARN all three identity node fetches returned empty — no graph-derived identity context loaded") + } + // Scan for a Persona node — the explicit identity declaration seeded into cultivated souls. // Stored at seeding time with label "soul:persona" and node_type "Persona". // genesis derives identity from the graph directly; cultivated souls have this node seeded. @@ -162,6 +170,12 @@ fn load_identity_context() -> Void { println("[soul] persona node loaded (" + int_to_str(str_len(p_content)) + " chars)") } } + // Q6 fix: if neither identity nodes nor persona node were loaded, log explicitly. + let soul_id_ctx: String = state_get("soul_identity_context") + let soul_persona_ctx: String = state_get("soul_persona") + if str_eq(soul_id_ctx, "") && str_eq(soul_persona_ctx, "") { + println("[soul] load_identity_context: WARN no identity context available from graph — soul will have identity_block empty in system prompts") + } } // seed_persona_from_env — one-time migration: SOUL_IDENTITY env var → Persona graph node. @@ -327,6 +341,15 @@ fn layered_cycle(raw_input: String) -> String { // TODO: wire directly when imprint_respond gains system_override param (imprint.el change). // ISSUE 3 TODO: no semantic crisis detection. Keyword-only means signals that evade // the phrase list pass with zero augmentation. Semantic layer = separate decision. + // + // Q8 race documentation: "layered_cycle_safety_system_addendum" is a shared process-global + // state key. Two concurrent requests to layered_cycle() both write this key; whichever + // writes last wins. The concurrent build_system_prompt() read in chat.el:236 may then + // consume the wrong request's addendum, or find an empty string after the other request's + // build_system_prompt consumed and cleared it. Mitigation: under http_serve_async, the + // layered_cycle path and the /api/chat path are different endpoints (typically); true + // concurrent layered_cycle calls are uncommon. A robust fix requires per-request state + // scoping which needs C runtime support (e.g. a request-id-keyed addendum map). let augmented_addendum: String = safety_augment_system("", raw_input) state_set("layered_cycle_safety_system_addendum", augmented_addendum)