refactor(memory): shared curation evaluator for both write pipelines (memory-core-redesign slice 1)#1575
Merged
Aaronontheweb merged 4 commits intoJul 4, 2026
Conversation
opsx: memory-core-redesign slice 1 (task 1.1) Extracts the full curation decision flow (record bypass -> fuzzy anchor candidates -> content-term search -> CurationRulesEvaluator.Evaluate -> LLM tier + GuardDestructiveUpdate -> TryAutoResolveAmbiguous -> Create fallback) out of MemoryCurationActor.EvaluateSingleAsync into a new MemoryCurationEvaluator, and wires the actor through it. The evaluator also owns ApplyDecisionAsync, the decision -> write-operation mapping (including Consolidate's re-anchor/tombstone side effects) that used to live inline in the actor's write phase. Constructor dependencies are SQLiteMemoryStore (required) and IChatClient? (genuinely optional: absent LLM is a real permanent operating mode for a session with no compaction-role model, not a compat shim). Logging seam: the actor logs via Akka ILoggingAdapter, the daemon engine (next commit) via Microsoft.Extensions.Logging ILogger. A minimal internal ICurationLog delegate (AkkaCurationLog / MicrosoftCurationLog) lets one evaluator body emit identical markers (curation_llm_decision, curation_llm_no_decision, curation_llm_timeout, curation_llm_error, curation_dual_search, curation_ambiguous_auto_resolved, curation_ambiguous_create_fallback, curation_skip/update/consolidate/ create, curation_reanchor, curation_tombstone_anchor) through either logging stack, since the July 2026 audit tooling greps daemon logs for these exact marker strings. SidecarSessionCorrelationTests updated to call MemoryCurationEvaluator.TryLlmEvaluationAsync (moved from the actor).
…ator opsx: memory-core-redesign slice 1 (task 1.2) Structural finding: MemoryCurationEngine.CurateAsync (the daemon checkpoint-worker path) performed NO per-candidate relationship evaluation before this change — only MemoryRulesFirstExtractor extraction (with its own fingerprint-based literal-duplicate check) plus a direct write via ApplyCurationBatchAsync. It never called CurationRulesEvaluator, never ran the LLM tier, and never guarded against destructive updates. Unification therefore means routing extracted candidates through MemoryCurationEvaluator for the first time, not deleting a parallel copy of the actor's logic (none existed). MemoryCurationEngine now constructs a MemoryCurationEvaluator with the store and its own ILogger, and WITHOUT an LLM client (the daemon worker has none to give it) — every Ambiguous decision on this path resolves via CurationRulesEvaluator.TryAutoResolveAmbiguous. CurateAsync's extraction step (MemoryRulesFirstExtractor, fingerprint pre-check) is unchanged; after building operations from extracted candidates, each is evaluated then applied via the shared EvaluateAsync/ApplyDecisionAsync before being returned to the checkpoint worker's ApplyCurationBatchAsync call. THE ONE INTENDED BEHAVIORAL DELTA (audit finding D14): the daemon path now applies CurationRulesEvaluator.GuardDestructiveUpdate, closing the gap where only the inline actor path guarded against an Update decision overwriting existing content it does not preserve.
opsx: memory-core-redesign slice 1 (task 1.3, 1.4) MemoryCurationEvaluatorParityTests seeds one SQLiteMemoryStore and constructs two evaluators against it — one via the ILoggingAdapter constructor (as MemoryCurationActor does), one via the ILogger constructor (as MemoryCurationEngine does), both without an LLM client — and asserts EvaluateAsync returns identical CurationDecision values for the same operation across the full decision matrix: - record bypass -> Create - exact anchor, high content overlap -> Skip - exact anchor, newer + guard-clean superset -> Update - fuzzy anchor, high overlap -> Consolidate - gray zone (0.4-0.8), no LLM -> deterministic auto-resolve (Skip) - fuzzy anchor, low overlap -> Create - guard downgrade: an Update whose proposal drops the existing body's content -> Skip on BOTH paths (audit finding D14 closed) Plus two LLM-tier cases via a scripted IChatClient: a parseable decision (SKIP) and an empty response, which must fall back to the same deterministic auto-resolve decision as the no-LLM case. Gates run clean: dotnet test Netclaw.Actors.Tests (2540/2540) and Netclaw.Daemon.Tests (825/825), dotnet slopwatch analyze (0 issues), Add-FileHeaders.ps1 -Verify. Marks tasks 1.1-1.4 done in openspec/changes/memory-core-redesign/tasks.md.
Aaronontheweb
commented
Jul 4, 2026
Aaronontheweb
left a comment
Aaronontheweb
left a comment
Collaborator
Author
There was a problem hiding this comment.
LGTM - basically standardizes the daemon's memory curation system + the session one to use the same standards and decision tree
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Slice 1 (tasks 1.1–1.4) of the
memory-core-redesignOpenSpec change (#1570): extracts a sharedMemoryCurationEvaluatorused by both memory write pipelines, so the inline per-session actor path and the daemon checkpoint-worker path can never diverge again.What unified
MemoryCurationActor.EvaluateSingleAsync(record bypass → fuzzy anchor candidates → content-term candidate search →CurationRulesEvaluator.Evaluate→ LLM tier +GuardDestructiveUpdate→TryAutoResolveAmbiguous→ Create fallback) and the actor's decision→write-operation mapping (including Consolidate's re-anchor/tombstone side effects) now live in oneMemoryCurationEvaluator(src/Netclaw.Actors/Memory/MemoryCurationEvaluator.cs). The actor delegates to it;MemoryCurationEngine.CurateAsync(daemon) routes every extracted candidate through the sameEvaluateAsync→ApplyDecisionAsyncsequence beforeApplyCurationBatchAsyncpersists the result.Structural finding: the daemon path previously performed no per-candidate relationship evaluation at all — only
MemoryRulesFirstExtractorextraction (with its fingerprint literal-duplicate check) followed by a direct write. It never calledCurationRulesEvaluatorand never guarded updates. Unification here means routing its candidates through the evaluator for the first time, exactly as anticipated by the change's design doc (D4).The ONE intended behavioral delta
The daemon path gains
GuardDestructiveUpdate— closing audit finding D14 (the lossless-update guard existed only on the inline actor path). An Update decision whose proposal does not preserve the existing body is downgraded to Skip on both paths now. The daemon engine constructs the evaluator without an LLM client (its worker has none), preserving today's deterministic-only behavior there; Ambiguous decisions resolve viaTryAutoResolveAmbiguous.Marker-stability guarantee
The evaluator logs through a minimal internal
ICurationLogseam with two adapters (AkkaCurationLogfor the actor'sILoggingAdapter,MicrosoftCurationLogfor the engine's MELILogger). Message templates and argument order are byte-identical to the pre-refactor actor code, so the July 2026 audit tooling's grep targets keep firing unchanged from both paths:curation_dual_search,curation_llm_decision,curation_llm_no_decision,curation_llm_timeout,curation_llm_error,curation_ambiguous_auto_resolved,curation_ambiguous_create_fallback,curation_skip/_update/_consolidate/_create,curation_reanchor,curation_tombstone_anchor.Characterization matrix (
MemoryCurationEvaluatorParityTests, 9 tests, all green)Both constructions (actor-style
ILoggingAdapter/ engine-styleILogger, no LLM) driven against one seeded store, asserting decision equality:SKIPValidation
dotnet test src/Netclaw.Actors.Tests— 2540/2540 passeddotnet test src/Netclaw.Daemon.Tests— 825/825 passeddotnet slopwatch analyze— 0 issuesAdd-FileHeaders.ps1 -Verify— all files have headersnvidia/Qwen3.6-27B-NVFP4, 5 runs/case):The single failing case is an instrumentation false negative, not a regression from this PR: in both "failing" runs the model behaved exactly as the eval's own comment defines correct ("already saved — I can see 'Favorite Color — Chartreuse' in my durable memories", no SOUL.md edit), but the assertion greps for the literal substring
memory, which does not match the word "memories". A first eval attempt scored 2/5 because the vLLM endpoint restarted mid-run (15/25 prompts gotConnection refused); the retry above is the clean run. This case is untouched by this refactor (which only affects the post-proposal curation write pipeline, not model tool routing or response text) and the same code scored 5/5 on it in the earlier partial run.OpenSpec
memory-core-redesign(merged in opsx: memory-core-redesign — proposal, design, spec deltas, tasks #1570), design D4 + Migration Plan item 1openspec/changes/memory-core-redesign/tasks.md