Skip to content

Commit 6034ba0

Browse files
jkyberneeesclaude
andcommitted
fix(memory): close verification findings on async-merge PR (D-02/D-03/D-06/D-08/D-09)
D-06 (critical): consolidate_on_end was gated behind the LLMExtract early-return in OnSessionEndWithProvenance, silently disabling consolidation whenever a user set llm_extract=false (to disable episode extraction). These features are conceptually independent. Fix: move the consolidation goroutine launch before the LLMExtract guard, with its own independent gate (llm != nil, turns >= min, ConsolidateOnEnd=true, LLMConsolidate=true). Verified: consolidation fires with llm_extract=false; added regression test. D-03: Consolidate had no upper-bound guard on LLM-returned entries; a hallucinating LLM could expand the entry count. Fix: cap newEntries to len(entries) before writeEntries. D-02: writeEntries doc comment said "Caller must hold f.mu" but Consolidate calls it holding factsDirLock instead of f.mu (safely, but undocumented). Fix: clarify the locking contract — either f.mu (normal paths) or factsDirLock (Consolidate) is sufficient; the two are never acquired together. D-08: CHEATSHEET.md still documented the old judge behavior ("LLM judges → merge or add"). Updated to reflect the new behavior (auto-add, deferred to session-end consolidation). D-09: CONFIG.md noted consolidate_on_end as the complement to merge-on-write, but did not warn that disabling it causes near-duplicates to accumulate permanently. Added an explicit note. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 09582a1 commit 6034ba0

5 files changed

Lines changed: 65 additions & 20 deletions

File tree

docs/CHEATSHEET.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -122,12 +122,13 @@ Settings: `model` (tiny/base/small/medium), `language` (ISO code, empty=auto), `
122122
```
123123
RP.embed(newEntry) → cosine similarity vs each existing entry
124124
125-
cos > 0.7 ─────→ auto-merge (replace)
125+
cos > 0.7 ─────→ simple merge (no LLM — substring or concatenation)
126126
cos < 0.3 ─────→ auto-add
127-
0.3–0.7 ─────→ LLM judges → merge or add
127+
0.3–0.7 ─────→ auto-add (deferred to session-end consolidation)
128128
```
129129

130-
Saves ~80% of LLM calls on memory writes.
130+
AddFact makes zero LLM calls. Near-duplicate dedup happens at session end
131+
via background consolidation (`consolidate_on_end`, default true).
131132

132133
### Memory Tool
133134

docs/CONFIG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ The `memory` section controls the persistent memory system (see [docs/MEMORY.md]
216216
| `buffer_lines` | 20 | Max turn summaries in session buffer |
217217
| `buffer_enabled` | true | Enable the turn-level buffer |
218218
| `merge_on_write` | true | Use go-vector RP similarity to auto-merge related entries (fast, no LLM — uses simple string merge) |
219-
| `consolidate_on_end` | true | At session end, run an LLM consolidation pass over `user.md` and `env.md` in a background goroutine. This is the quality complement to `merge_on_write`: merge-on-write handles obvious duplicates immediately (no LLM), while consolidation handles near-duplicates and paraphrases at session end with full LLM quality. Requires `llm_consolidate: true`. |
219+
| `consolidate_on_end` | true | At session end, run an LLM consolidation pass over `user.md` and `env.md` in a background goroutine. This is the quality complement to `merge_on_write`: merge-on-write handles obvious duplicates immediately (no LLM), while consolidation handles near-duplicates and paraphrases at session end with full LLM quality. Requires `llm_consolidate: true`. **Note:** facts in the borderline similarity band (0.3–0.7 cosine) are now always added immediately and only merged by this consolidation pass — if you set `consolidate_on_end: false`, near-duplicate facts will accumulate rather than being merged. |
220220
| `extract_on_end` | true | At session end (≥3 turns), extract a narrative episode summary via LLM for later recall |
221221
| `extract_facts` | **false** | **Opt-in.** At session end (≥3 turns), auto-extract a few **durable** facts (stable user preferences, project invariants) into `user.md`/`env.md`. Off by default — see the security note below. Independent of `extract_on_end`; to disable *all* end-of-session LLM extraction set `llm_extract: false`. |
222222
| `llm_search` | true | Use LLM to rerank candidates for **explicit** `memory search` calls (the `memory` tool). Per-turn recall (`FormatEpisodeContext`) always uses the cached go-vector index — no LLM call on the hot path regardless of this setting. |

internal/memory/async_merge_test.go

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,36 @@ func TestConsolidateOnEnd_FiresAtSessionEnd(t *testing.T) {
124124
t.Error("session-end consolidation did not reduce fact count within 3 seconds")
125125
}
126126

127+
// TestConsolidateOnEnd_IndependentOfLLMExtract: consolidation must fire even
128+
// when llm_extract=false (episode extraction disabled). D-06 regression guard.
129+
func TestConsolidateOnEnd_IndependentOfLLMExtract(t *testing.T) {
130+
dir := t.TempDir()
131+
llm := &mockLLM{responses: map[string]string{
132+
"memory entri": `["consolidated single fact"]`,
133+
}}
134+
cfg := DefaultMemoryConfig()
135+
cfg.LLMExtract = boolPtr(false) // episodes off — must NOT suppress consolidation
136+
cfg.ConsolidateOnEnd = boolPtr(true)
137+
cfg.MergeOnWrite = boolPtr(false)
138+
mm := NewMemoryManager(dir, llm, cfg)
139+
_ = mm.AddFact("user", "prefers dark mode editors")
140+
_ = mm.AddFact("user", "uses dark theme always")
141+
entries0, _ := mm.facts.Entries("user")
142+
if len(entries0) < 2 {
143+
t.Fatalf("need 2 seeded entries, got %d", len(entries0))
144+
}
145+
msgs := []string{"user: hi", "assistant: ok", "user: more", "assistant: done"}
146+
mm.OnSessionEndWithProvenance("sess-d06", 5, msgs, EpisodeProvenance{})
147+
deadline := time.Now().Add(3 * time.Second)
148+
for time.Now().Before(deadline) {
149+
if e, _ := mm.facts.Entries("user"); len(e) < 2 {
150+
return // consolidation fired
151+
}
152+
time.Sleep(50 * time.Millisecond)
153+
}
154+
t.Error("consolidation should fire even with llm_extract=false")
155+
}
156+
127157
// TestConsolidateOnEnd_FlagOff: with consolidate_on_end=false, fact count must
128158
// remain stable at session end (no consolidation LLM call).
129159
func TestConsolidateOnEnd_FlagOff(t *testing.T) {

internal/memory/facts.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,13 @@ func (f *FactStore) Entries(target string) ([]string, error) {
304304
// writeEntries joins entries and writes them to disk atomically. It writes to a
305305
// UNIQUE temp file in the same directory and renames it into place, so two
306306
// FactStore instances writing the same directory concurrently can never clobber
307-
// a shared temp file. Caller must hold f.mu.
307+
// a shared temp file.
308+
//
309+
// Locking contract: callers must hold EITHER f.mu (normal mutation paths via
310+
// readModifyWrite) OR the process-wide factsDirLock for this directory
311+
// (Consolidate path in memory.go). These two locks are never acquired together,
312+
// so there is no deadlock risk. factsDirLock provides the same cross-instance
313+
// mutual exclusion that f.mu provides per-instance.
308314
func (f *FactStore) writeEntries(target string, entries []string) error {
309315
content := strings.Join(entries, entrySep)
310316
path := f.path(target)

internal/memory/memory.go

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,11 @@ Entries for %s:
417417
if len(newEntries) == 0 || (len(newEntries) == 1 && newEntries[0] == "") {
418418
return nil // LLM returned nothing useful
419419
}
420+
// Guard against a hallucinating LLM expanding the entry count; the system
421+
// prompt says "Never more than the original count" but that is advisory only.
422+
if len(newEntries) > len(entries) {
423+
newEntries = newEntries[:len(entries)]
424+
}
420425

421426
// Security: scan LLM output before persisting
422427
for _, entry := range newEntries {
@@ -514,7 +519,24 @@ func (m *MemoryManager) OnSessionEndWithProvenance(sessionID string, turns int,
514519
if minTurns <= 0 {
515520
minTurns = defaultMinTurnsForExtraction
516521
}
517-
// Shared preconditions for any end-of-session LLM extraction.
522+
523+
// Background consolidation is independent of episode/fact extraction —
524+
// it fires based on its own gate so that llm_extract=false does not
525+
// silently disable it (D-06). Requires an LLM client, llm_consolidate,
526+
// and a minimum session length (same threshold reused for consistency).
527+
if m.llm != nil && turns >= minTurns &&
528+
m.cfg.ConsolidateOnEnd != nil && *m.cfg.ConsolidateOnEnd &&
529+
m.cfg.LLMConsolidate != nil && *m.cfg.LLMConsolidate {
530+
go func() {
531+
for _, target := range []string{"user", "env"} {
532+
// Best-effort: errors (e.g. only 1 entry, nothing to consolidate)
533+
// are silently ignored — consolidation is a quality pass, not critical.
534+
_ = m.Consolidate(target)
535+
}
536+
}()
537+
}
538+
539+
// Preconditions shared by episode summary + fact extraction.
518540
if m.cfg.LLMExtract == nil || !*m.cfg.LLMExtract || m.llm == nil || turns < minTurns || len(messages) == 0 {
519541
return
520542
}
@@ -531,20 +553,6 @@ func (m *MemoryManager) OnSessionEndWithProvenance(sessionID string, turns int,
531553
if m.cfg.ExtractFacts != nil && *m.cfg.ExtractFacts && !prov.Untrusted {
532554
m.extractFactsFromSession(convText)
533555
}
534-
535-
// Background consolidation: recover the LLM-merge quality that AddFact no
536-
// longer applies synchronously. Runs after fact extraction so any newly
537-
// auto-extracted facts are also consolidated in the same pass.
538-
if m.cfg.ConsolidateOnEnd != nil && *m.cfg.ConsolidateOnEnd &&
539-
m.cfg.LLMConsolidate != nil && *m.cfg.LLMConsolidate && m.llm != nil {
540-
go func() {
541-
for _, target := range []string{"user", "env"} {
542-
// Best-effort: errors (e.g. only 1 entry, nothing to consolidate)
543-
// are silently ignored — consolidation is a quality pass, not critical.
544-
_ = m.Consolidate(target)
545-
}
546-
}()
547-
}
548556
}
549557

550558
// buildConvText joins the session's message lines into a single transcript for

0 commit comments

Comments
 (0)