fix(memory): address PR #27 verification findings (off-lock rebuild + hardening)

AI Verification Protocol pass (agents B/C/D) on PR #27 surfaced one HIGH
finding plus defensive gaps. Repairs:

- HIGH (2.1/2.5 network-I/O-under-lock): episodeVectorIndex.ensureFresh now
  fits + embeds the corpus on a FRESH embedder instance OFF the write lock,
  then swaps the store/embedder in atomically. A single-flight guard collapses
  concurrent rebuilds and a dirty-sequence check folds in a write that lands
  mid-rebuild. A slow/remote embedding backend can no longer serialize every
  concurrent recall behind one rebuild under odek serve. New test
  TestRebuildDoesNotSerializeRecall fails against the old lock-held-on-network
  code; whole package stays race-clean.
- cosineVector now guards NaN/Inf (hostile/buggy embedding backend) so ranking
  order stays well-defined, mirroring MergeDetector.Classify.
- MergeDetector.ReplaceEntry guards a desynced vecs/corpus (post-failed-Fit)
  so it can never index-panic.
- Tests closing the verification coverage gaps: config-loader end-to-end parse
  of memory.embedding (C2), api_key ${ENV_VAR} expansion (C10), and the
  safety-critical failure-mode net — ranker recency fallback + merge add-without-
  merge (C12) and dedup-never-deletes-on-embed-error (C13).
- Docs: corrected the rebuild-cache wording (one batch over the corpus, off
  lock), added the base_url egress/SSRF caveat, and the dims:0 model-change note.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: jkyberneees

docs/CONFIG.md

@@ -262,16 +262,25 @@ that matches by meaning.
 
 Operational notes:
 
-- **Per-turn recall stays cheap.** Episode vectors are cached in a persisted
-  index; a loop turn costs at most one embedding call (the query), bounded by
+- **Per-turn recall stays cheap.** Episode vectors live in a persisted index; a
+  loop turn costs at most one embedding call (the query), bounded by
   `timeout_seconds`. If the backend is down, recall degrades to "no context"
-  and rebuilds back off for 30s — the agent loop is never blocked.
+  and rebuilds back off for 30s — the agent loop is never blocked. The index
+  rebuild that follows a new episode (session-end) embeds the corpus on a fresh
+  client *off* the index lock, so a slow backend never serializes concurrent
+  recall; it is one batch call over the episode summaries.
 - **Switching backends is safe.** The persisted index records which embedding
   space it was built in; changing `provider`/`model`/`dims` automatically
-  invalidates it and rebuilds on next use (one batch embedding call).
-- **Episode summaries leave the machine.** With `"http"`, episode summaries and
-  fact entries are sent to the configured endpoint for embedding. Point it at a
-  local server (Ollama/llama.cpp) if that must not happen.
+  invalidates it and rebuilds on next use (one batch embedding call). Note: with
+  `dims: 0`, if a server silently changes a model's output dimensionality (e.g.
+  a model upgrade under the same name) the fingerprint cannot detect it; recall
+  self-heals to "no context" on the dimension mismatch and rebuilds on the next
+  write. Pin `dims` if you want such a change to force an explicit rebuild.
+- **`base_url` is an egress target — point it only at a server you trust.** Every
+  episode summary and fact entry is POSTed there for embedding. The URL is used
+  verbatim with no allowlist, so do not point it at internal/metadata endpoints
+  (e.g. cloud metadata services) you would not otherwise expose. Prefer a local
+  server (Ollama/llama.cpp) when episode/fact text must not leave the machine.
 
 ### ⚠️ `extract_facts` — automatic fact learning (opt-in, off by default)
 

docs/MEMORY.md

@@ -248,11 +248,14 @@ Mechanics (`internal/memory/embedder.go`):
   `episodes_index_meta.json`; changing provider/model/dims invalidates and
   rebuilds it automatically. Pre-existing RP indexes (no meta file) keep
   loading without a rebuild.
-- The HTTP embedder caches text→vector, so an index rebuild after a new
-  episode only embeds texts it hasn't seen — one batch call.
+- The HTTP embedder caches text→vector within an instance, so per-turn query
+  embeds are not re-sent. An index rebuild (after a new episode) runs on a
+  fresh client *off the index lock* — one batch call over the corpus — so a
+  slow backend never serializes concurrent recall.
 - Failure mode: embedding errors degrade to "no recall context" / "add fact
-  without merge check"; a failed index rebuild backs off for 30s so a down
-  backend is not re-hit every loop turn. The agent loop is never blocked.
+  without merge check" / recency ranking — never a wrong dedup (which would
+  delete the matched episode). A failed index rebuild backs off for 30s so a
+  down backend is not re-hit every loop turn. The agent loop is never blocked.
 
 See `docs/CONFIG.md` → `embedding` for all fields and the privacy note
 (summaries are sent to the configured endpoint — use a local server if that

internal/config/loader_test.go

@@ -893,3 +893,45 @@ func TestLoadConfig_LegacyAPIKeyEnvVarLost(t *testing.T) {
 		t.Error("ODEK_API_KEY should be present in child env after re-injection")
 	}
 }
+
+// TestLoadConfig_MemoryEmbeddingSection verifies the memory.embedding config
+// section is parsed and propagated through LoadConfig/resolveMemory, and that
+// the raw ${ENV_VAR} placeholders survive into ResolvedConfig (expansion is
+// deferred to embedder construction, where both base_url and api_key are run
+// through os.ExpandEnv). Closes the C2 end-to-end config gap surfaced by the
+// PR #27 verification pass.
+func TestLoadConfig_MemoryEmbeddingSection(t *testing.T) {
+	dir := t.TempDir()
+	t.Setenv("HOME", dir)
+	t.Chdir(dir)
+	if err := os.WriteFile(filepath.Join(dir, "odek.json"), []byte(`{
+		"memory": {
+			"embedding": {
+				"provider": "http",
+				"base_url": "${ODEK_EMBED_URL}",
+				"model": "nomic-embed-text",
+				"api_key": "${ODEK_EMBED_KEY}",
+				"dims": 768,
+				"timeout_seconds": 7
+			}
+		}
+	}`), 0644); err != nil {
+		t.Fatal(err)
+	}
+	cfg := LoadConfig(CLIFlags{})
+	emb := cfg.Memory.Embedding
+	if emb == nil {
+		t.Fatal("memory.embedding was not parsed into ResolvedConfig")
+	}
+	if emb.Provider != "http" || emb.Model != "nomic-embed-text" {
+		t.Errorf("provider/model = %q/%q, want http/nomic-embed-text", emb.Provider, emb.Model)
+	}
+	if emb.Dims != 768 || emb.TimeoutSeconds != 7 {
+		t.Errorf("dims/timeout = %d/%d, want 768/7", emb.Dims, emb.TimeoutSeconds)
+	}
+	// Raw config keeps ${VAR} (expansion happens at embedder construction); assert
+	// the literal so a future eager-expand change is caught deliberately.
+	if emb.BaseURL != "${ODEK_EMBED_URL}" || emb.APIKey != "${ODEK_EMBED_KEY}" {
+		t.Errorf("base_url/api_key = %q/%q, want unexpanded ${...} placeholders", emb.BaseURL, emb.APIKey)
+	}
+}

internal/memory/episode_index.go

@@ -52,13 +52,20 @@ type scoredEpisode struct {
 // Thread-safety: all exported methods hold vi.mu as appropriate.
 // Per-directory singleton: see sharedEpisodeIndex.
 type episodeVectorIndex struct {
-	mu       sync.RWMutex
-	store    *vector.Store
-	emb      textEmbedder
-	dir      string
-	ready    bool
-	dirty    bool
-	failedAt time.Time // last failed rebuild; zero = never failed
+	mu    sync.RWMutex
+	store *vector.Store
+	emb   textEmbedder
+	// newEmb builds a FRESH embedder instance for a rebuild. Rebuilds embed on
+	// a detached instance off-lock (see ensureFresh), so the live (emb, store)
+	// pair stays valid and consistent until the atomic swap — a query never
+	// embeds against a half-refitted embedder.
+	newEmb     func() textEmbedder
+	dir        string
+	ready      bool
+	dirty      bool
+	rebuilding bool      // single-flight guard: a rebuild is embedding off-lock
+	dirtySeq   uint64    // bumped by markDirty; reconciled after an off-lock rebuild
+	failedAt   time.Time // last failed rebuild; zero = never failed
 }
 
 // indexMeta is the persisted embedding-space identity (episodeIndexMetaFile).
@@ -104,7 +111,7 @@ func sharedEpisodeIndex(dir string, newEmb func() textEmbedder) *episodeVectorIn
 	if vi, ok := epIdxes[key]; ok {
 		return vi
 	}
-	vi := &episodeVectorIndex{dir: abs, emb: emb}
+	vi := &episodeVectorIndex{dir: abs, emb: emb, newEmb: newEmb}
 	epIdxes[key] = vi
 	return vi
 }
@@ -117,6 +124,7 @@ func sharedEpisodeIndex(dir string, newEmb func() textEmbedder) *episodeVectorIn
 func (vi *episodeVectorIndex) markDirty() {
 	vi.mu.Lock()
 	vi.dirty = true
+	vi.dirtySeq++
 	vi.failedAt = time.Time{}
 	vi.mu.Unlock()
 }
@@ -150,34 +158,78 @@ func (vi *episodeVectorIndex) search(query string, k int) []scoredEpisode {
 
 // ensureFresh loads or rebuilds the index as needed. Must NOT be called while
 // holding vi.mu (it acquires it internally).
+//
+// The expensive part of a rebuild — fitting the embedder and embedding the
+// whole corpus, which is a blocking network call for the HTTP backend — runs
+// OFF the lock on a fresh embedder instance, then the result is swapped in
+// atomically under the lock. This keeps a slow embedding backend from
+// serializing every concurrent recall behind one rebuild (search runs per
+// turn; under `odek serve` all connections funnel through this per-dir
+// singleton). A single-flight guard (rebuilding) collapses a thundering herd
+// of concurrent rebuilds into one, and a dirty-sequence check folds in any
+// episode write that lands mid-rebuild.
 func (vi *episodeVectorIndex) ensureFresh() {
 	vi.mu.RLock()
-	if vi.ready && !vi.dirty {
-		vi.mu.RUnlock()
+	ready := vi.ready && !vi.dirty
+	vi.mu.RUnlock()
+	if ready {
 		return
 	}
-	vi.mu.RUnlock()
 
 	vi.mu.Lock()
-	defer vi.mu.Unlock()
 	if vi.ready && !vi.dirty {
-		return // double-checked
+		vi.mu.Unlock()
+		return
 	}
 	// Back off after a failed rebuild so a down embedding backend is not
 	// re-hit on every loop turn (search runs per turn).
 	if !vi.failedAt.IsZero() && time.Since(vi.failedAt) < rebuildRetryInterval {
+		vi.mu.Unlock()
 		return
 	}
-
-	// Cold start without a pending write: try the persisted state first.
+	// Single-flight: if another goroutine is already rebuilding off-lock, serve
+	// the current state for this turn rather than launch a duplicate (and a
+	// duplicate batch embed) — the in-flight rebuild will publish shortly.
+	if vi.rebuilding {
+		vi.mu.Unlock()
+		return
+	}
+	// Cold start without a pending write: try the persisted state first
+	// (disk-only, fast — fine to do under the lock).
 	if !vi.ready && !vi.dirty {
 		if vi.tryLoadLocked() {
+			vi.mu.Unlock()
 			return
 		}
 	}
-	// Either cold-start without usable persisted state, or dirty after a
-	// write — full rebuild.
-	vi.rebuildLocked()
+	// Rebuild needed. Snapshot the dirty sequence, take a fresh embedder, and
+	// release the lock before the network-bound embedding work.
+	vi.rebuilding = true
+	seq := vi.dirtySeq
+	emb := vi.newEmb()
+	vi.mu.Unlock()
+
+	store := buildEpisodeStore(vi.readAllSummaries(), emb)
+
+	vi.mu.Lock()
+	defer vi.mu.Unlock()
+	vi.rebuilding = false
+	if store == nil {
+		// Embedding failed (e.g. backend down) — keep the previous index, if
+		// any, serving and start the retry cool-down.
+		vi.failedAt = time.Now()
+		return
+	}
+	vi.store = store
+	vi.emb = emb
+	vi.ready = true
+	vi.failedAt = time.Time{}
+	// Only clear dirty if no write landed while we were rebuilding off-lock;
+	// otherwise leave it set so the next search rebuilds with the newest data.
+	if vi.dirtySeq == seq {
+		vi.dirty = false
+	}
+	vi.saveLocked()
 }
 
 // tryLoadLocked attempts to load persisted state. Returns true on success.
@@ -219,26 +271,23 @@ func legacyRPFingerprint() string {
 	return newRPTextEmbedder(episodeVectorDim).fingerprint()
 }
 
-// rebuildLocked reads all episode summaries from disk, fits the embedder on
-// the full corpus, and persists the result. On embedding failure (e.g. a
-// remote backend being down) the previous index — if any — is kept serving
-// and a retry cool-down starts. Caller must hold vi.mu (write lock).
-func (vi *episodeVectorIndex) rebuildLocked() {
-	texts := vi.readAllSummaries()
-
+// buildEpisodeStore fits emb on the full corpus and returns a populated vector
+// store, or nil on any embedding failure (e.g. a remote backend being down) so
+// the caller can keep the previous index serving. It takes NO lock and touches
+// no shared index state — it operates entirely on its arguments and a fresh
+// embedder, which is what lets ensureFresh run it off vi.mu.
+func buildEpisodeStore(texts []idText, emb textEmbedder) *vector.Store {
 	corpus := make([]string, len(texts))
 	for i, t := range texts {
 		corpus[i] = t.text
 	}
 
-	if err := vi.emb.fit(corpus); err != nil {
-		vi.failedAt = time.Now()
-		return
+	if err := emb.fit(corpus); err != nil {
+		return nil
 	}
-	vecs, err := vi.emb.embedAll(corpus)
+	vecs, err := emb.embedAll(corpus)
 	if err != nil {
-		vi.failedAt = time.Now()
-		return
+		return nil
 	}
 
 	store := vector.NewStore(vector.CosineDistance)
@@ -248,12 +297,7 @@ func (vi *episodeVectorIndex) rebuildLocked() {
 		}
 		store.Add(t.id, vecs[i])
 	}
-
-	vi.store = store
-	vi.ready = true
-	vi.dirty = false
-	vi.failedAt = time.Time{}
-	vi.saveLocked()
+	return store
 }
 
 type idText struct {

internal/memory/episodes.go

@@ -862,7 +862,15 @@ func cosineVector(a, b vector.Vector) float32 {
 	if normA == 0 || normB == 0 {
 		return 0
 	}
-	return float32(dot / (math.Sqrt(normA) * math.Sqrt(normB)))
+	sim := dot / (math.Sqrt(normA) * math.Sqrt(normB))
+	if math.IsNaN(sim) || math.IsInf(sim, 0) {
+		// A buggy/hostile embedding backend can return NaN/Inf components; a
+		// NaN score breaks sort ordering (non-strict-weak). Treat as "no
+		// similarity" so ranking stays well-defined. Mirrors the NaN guard in
+		// MergeDetector.Classify.
+		return 0
+	}
+	return float32(sim)
 }
 
 // truncateAtRune returns s truncated to at most maxBytes bytes, always

internal/memory/merge.go

@@ -173,14 +173,19 @@ func (m *MergeDetector) ReplaceEntry(idx int, entry string) {
 	}
 	m.corpus[idx] = entry
 	if err := m.emb.fit(m.corpus); err != nil {
-		m.vecs[idx] = nil
+		m.vecs = nil // a failed fit invalidates the whole precomputed set
 		return
 	}
 	vec, err := m.emb.embed(entry)
 	if err != nil {
 		vec = nil
 	}
-	m.vecs[idx] = vec
+	// A prior failed Fit can leave m.vecs nil or shorter than m.corpus; guard
+	// the write so a desynced index never panics here (Classify already treats
+	// an empty/short vecs as "nobody").
+	if idx < len(m.vecs) {
+		m.vecs[idx] = vec
+	}
 }
 
 // Corpus returns the current corpus (for inspection).