Skip to content

feat: add a consolidate command to reconcile duplicate and superseded memories across write lanes - #946

Open
gorkem2020 wants to merge 3 commits into
CortexReach:masterfrom
gorkem2020:feat/memory-consolidate-command
Open

feat: add a consolidate command to reconcile duplicate and superseded memories across write lanes#946
gorkem2020 wants to merge 3 commits into
CortexReach:masterfrom
gorkem2020:feat/memory-consolidate-command

Conversation

@gorkem2020

@gorkem2020 gorkem2020 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

The store has no cross-lane consolidation. The same fact can accumulate rows via manual tool writes, reflection writer-1 mapped rows, and the full extraction pipeline, and two existing rows saying the same thing are never reconciled. A preference reversal ("user quit doing X") also never supersedes the rows it contradicts, since plain vector similarity puts a reversal too far from what it contradicts.

This adds a consolidate CLI command, registered under the memory-pro command group, that scans a scope's existing rows and reconciles duplicates, near-duplicates, and reversals across all three write lanes:

memory-pro consolidate --scope <scope> [--apply] [--yes] [--category <cat>] [--since <ISO>] [--include-reflection-slices] [--agent <agentId>]

What changed

Clustering. Seed-based single-hop grouping (every row must be directly linked to the cluster's seed, never merely linked through an intermediate member, so a chain of only-moderately-similar pairs can't bridge unrelated topics) combining embedding cosine similarity, a shared fact_key, and two gated topic-overlap fallbacks: one for reversal-shaped text ("no longer", "stopped", "quit", "doesn't", ...) linking a reversal to what it contradicts, and one for cross-lane near-duplicates that aren't reversal-shaped (a majority token-overlap ratio, not just one shared word, so unrelated short statements don't bridge).

Decision. One batched LLM call per run, not one per cluster, reusing the existing skip / merge / supersede / contradict vocabulary from src/memory-categories.ts. Verdicts are deterministic across repeat runs: temperature 0 on the decide call, candidates and cluster members sorted by row id before both clustering and prompt assembly, and a tightened rubric with an explicit merge-vs-supersede tiebreak. A malformed or out-of-range verdict for one cluster is dropped without discarding the others; a fully malformed response degrades to every cluster skipped.

Non-destructive execution. merge reuses the existing merge-writer prompt (buildMergePrompt). Both merge and supersede now soft-invalidate the absorbed rows (invalidated_at / superseded_by, plus a consolidation_audit trail pointing back at the survivor) rather than deleting. delete was removed from ConsolidateWriteDeps entirely, so no LLM verdict path can hard-delete a row even by future accident; hard delete stays reachable only through the operator-only CLI delete commands, a separate code path.

Append-only shield. events/cases rows are refined from blanket merge-immunity to invalidation-protection: a merge is allowed when every acted-upon row shares the identical append-only category (for example two near-duplicate events rows describing the same occurrence), but a merge mixing an append-only row with a non-append-only row, or with a different append-only category, and any supersede/contradict touching an append-only row, stays blocked unconditionally.

Store-layer invisibility for invalidated rows. excludeInactive now defaults to true across vectorSearch, bm25Search, lexicalFallbackSearch, list, and the compactor's fetchForCompaction path, so invalidated/superseded rows are invisible everywhere by default instead of only where a caller remembered to filter. stats() gains a liveCount alongside the existing totalCount, surfaced in both --json and the human-readable CLI output. The existing export and vault-export paths explicitly opt out (excludeInactive: false) to keep their full-dump, forensic-backup semantics; list and vault-export also gain an --include-invalidated flag for callers who want the historical rows back.

Cost gate. Clustering is free and runs first. Before any LLM call fires, dry-run or --apply alike, a mandatory cost-preview reports real numbers (N clusters -> 1 batched decider call, plus up to M merge-content generations). --yes bypasses the prompt for automation; a declined or missing confirm is a safe abort, never assumed consent.

Two-phase apply. Merge-content generation moved from apply time to plan-build time, so a dry run now builds the complete plan (every verdict plus precomputed merge content) before a single "apply these now?" prompt; confirming executes it as pure store writes with zero further LLM calls. A staleness guard snapshots each member row's metadata at plan-build time and skips, never partially applies, any cluster whose rows changed or disappeared by execution time.

Journal mirroring. --agent <agentId> threads through to the existing mdMirror journal writer's meta.agentId, so applied verdicts land in the invoking agent's own workspace instead of the fallback mirror directory. Omitting it preserves the existing fallback behavior.

admissionControl independence. Verified rather than changed: consolidate.ts and cli.ts never reference admission-control.ts, and admissionControl.enabled has no bearing on whether consolidate's own LLM client gets constructed. Pinned with a regression test using a poison-pill AdmissionController whose every method throws if invoked.

Reflection writer-2 slice rows (category reflection) stay excluded from the scan by default, since they're a separate instructions-to-self lane; --include-reflection-slices opts in. Already-invalidated rows are excluded from future scans, which is what makes --apply idempotent.

Tests

Red-first throughout. test/memory-consolidate.test.mjs covers clustering (cosine, fact_key, both topic-overlap fallbacks), chunking, verdict parsing, both prompt builders, and the full orchestrator end to end. test/invalidated-rows-visibility.test.mjs and test/store-excludeinactive-default.test.mjs cover the store-layer default flip and its fallout across existing call sites (with matching updates to test/store-empty-scope-filter.test.mjs, test/migrate-legacy-schema.test.mjs, and test/temporal-facts.test.mjs). test/memory-consolidate-cost-gate.test.mjs, test/memory-consolidate-two-phase-apply.test.mjs, and test/memory-consolidate-admission-independence.test.mjs cover the cost gate, two-phase apply plus staleness, and admission independence. All new files are registered in both package.json's test chain and scripts/ci-test-manifest.mjs. npm run build (tsc) and the full local npm test chain are green, with one pre-existing, environment-specific test skipped (a host-side port 11434 conflict unrelated to this change). A structural regression test confirms consolidate is attached under the memory-pro group, not the root program.

Notes for reviewers

Fixture rows are entirely synthesized, never copied from a live system. Fixtures modeling real dry-run cluster shapes are paraphrased with all identifying details replaced by a generic "User".

Depends on #945: fetchForCompaction, the row-fetch path this command's clustering scan uses, still drops LanceDB's typed-array vector columns to an empty array on a real store (this branch's own tests use in-memory fixtures with plain-array vectors, so they pass either way). This branch does not rebase onto that fix, to keep the two independently reviewable, but this PR should not be merged before, or without, it.

Batched merge-content writer (2026-07-17)

Plan build now generates merge content for all merge verdicts with one consolidate-merge-batch call per chunk of up to CONSOLIDATE_MERGE_BATCH_MAX_SIZE (10) verdicts, instead of one consolidate-merge call per absorbed member. Each numbered job folds a verdict's survivor plus every absorbed member in one output; the merge requirements text is carried over verbatim, only the call topology changes.

  • Per-item fail-closed matches the sequential fold's failure semantics exactly: a missing or malformed response entry keeps only that job's survivor content unchanged (what the old fold produced when its per-member completions returned null), and a failed chunk call degrades every job in that chunk the same way, never crashing and never fanning back out into per-member calls.
  • Zero merge verdicts make zero writer calls; a single verdict still uses the batch shape.
  • The cost preview now reports the real batched call count: "up to N batched merge-content call(s) covering up to M merge job(s)", where N is ceil(M/10), replacing the per-absorbed-member generation count.

Update: single-source prompt architecture (2026-07-17)

Added this branch's own copy of src/prompt-blocks.ts (the extraction/dedup/merge/consolidate prompt builders live in a duplicated extraction-prompts.ts across sibling branches, so the shared module follows the same convention) and converged every prompt builder in the file onto it:

  • Identity openers: the consolidate decider and consolidate merge writer already opened with their identity; that text is now sourced from the shared module instead of being duplicated inline. The capture-path extraction agent, dedup decider, and merge writer prompts (present in this file but not on the consolidate call path) gain the same identity openers as the sibling admission-batch-utility branch, for consistency at the next assembly.
  • Shared category taxonomy: composed into the consolidate decider and consolidate merge-writer prompts (both singular and batched variants).
  • Markdown payload standard: cluster/member and merge-job payloads move from indentation-only numbered lists to ## Cluster N / ### N. category / plain Label: value field lines / #### Existing memory and #### New information nested subsections. Provenance fields (source, timestamp, valid_from) keep their existing snake_case labels verbatim as plain lines under the new heading. Every JSON output contract is now fenced as a ```json code block.
  • buildDedupPrompt and buildMergePrompt now take the full candidate object instead of separate positional strings, matching the sibling branch's shape.

This is purely a prompt-formatting change; clusterConsolidateCandidates, verdict parsing, and the apply/dry-run execution paths are untouched. Expect an extraction-prompts.ts merge conflict against the admission-batch-utility branch at the next assembly (both branches now carry the identical shared-module content); keep one copy.

Update (2026-07-18)

Rebased onto current master, plus an operator-driven polish round, all live-tested on our fleet:

  • The CLI is now --agent-only: scope auto-derives as agent:<id>, --scope is gone, and journal-mirror writes always route to that agent's workspace.
  • Convergence to zero: clusters decided skip/contradict, and verdicts withheld by the append-only shield, are recorded as member-set fingerprints in dbPath/consolidate-settled.json and dropped before the cost gate on later runs; any member change re-opens its cluster. A fully converged store prints "0 candidates".
  • Shield-blocked verdicts are labeled in both the cluster listing and the apply-prompt plan instead of silently losing their action.
  • Honest failure classing: a decide call that returns no response is one aggregate log line and its own counter, no longer per-cluster "missing or malformed verdict" spam.

@gorkem2020
gorkem2020 force-pushed the feat/memory-consolidate-command branch 4 times, most recently from b47d619 to 16be783 Compare July 18, 2026 15:53
@gorkem2020
gorkem2020 marked this pull request as ready for review August 29, 2026 12:27
@gorkem2020

Copy link
Copy Markdown
Contributor Author

Recomposed against current master as a single commit now that its dependency landed: the consolidate CLI adapter needs #947's four-argument completeJson (deterministic verdict temperature), which is on master as of today. Includes the supersede-verification read adaptation for this family's excludeInactive-by-default store reads. Full suite green locally. Ready for review.

…nsolidate module, excludeInactive-by-default store reads, live/total stats split, CLI wiring)
@gorkem2020
gorkem2020 force-pushed the feat/memory-consolidate-command branch from 0184bd1 to 8209f55 Compare August 29, 2026 12:28

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head 8209f55. The focused tests, full suite, and current CI are green, but the apply path is not failure-atomic and can leave the store in a state that the command neither reports nor reliably repairs.

  1. A merge or supersede can be partially applied. writeMergeVerdict rewrites the survivor first and then invalidates absorbed rows one at a time; applySupersedeVerdict likewise performs independent row updates. If update k of N fails, executePlan catches the error, logs it, and continues without rollback, retry state, an applied entry, or an audit event. The survivor may already contain all merged content while some source rows remain active, recreating duplicate retrieval with an underreported Applied N action(s) result. Please make each cluster atomic or explicitly recoverable/idempotent, surface partial failure to the operator, and add fault-injection tests at every write position for both actions.

  2. Staleness and settled fingerprints ignore text/vector changes. Both mechanisms compare/hash only row id plus metadata. A concurrent text-only update can therefore pass the freshness check and be overwritten by a plan built from stale text; a previously settled cluster can also remain permanently skipped after a material text correction. Include every plan-relevant field, preferably a stable content/version hash, in both checks and cover concurrent text-only changes.

  3. The scan can become pathologically expensive before the first LLM call. fetchForCompaction materializes all matching rows/vectors before slicing, the command permits up to 100,000 candidates, and clustering is O(n^2) with repeated tokenization and dimension-wide cosine work per pair. Add a user-visible/query-level bound and scalable candidate blocking/indexing, memoize per-row tokens, and exercise a realistically large scope.

  4. consolidate-settled.json is not crash-safe or concurrency-safe. Runs read once and overwrite the whole file directly, with no lock, re-read/merge, temp-file rename, or pruning. Concurrent agents can lose each other's fingerprints; a truncated file is silently treated as an empty ledger, repeating all settled LLM work. Use locked atomic replacement, report corruption instead of silently resetting, and define bounded cleanup semantics.

Please also audit callers affected by the new default excludeInactive=true, bound/chunk the single decision prompt and catch provider rejection, and preserve prior consolidation_audit entries instead of overwriting the scalar object. The unrelated extraction/dedup prompt refactor would be easier to validate as a separate change.

Requesting changes on this head.

…ed scan, hardened settled ledger

Round-1 review: the apply path must never trade duplicate rows for
unreported partial state, and the pre-LLM scan must stay bounded.

1. Apply is per-write isolated with safe ordering instead of pseudo-atomic:
   merge writes the survivor's merged SUPERSET first (a later failure leaves
   duplicates active, the pre-consolidate status quo), each absorbed
   invalidation is attempted independently, and failures ride the audit as
   partialFailures. A cluster whose first write failed (merge survivor, or
   every supersede invalidation) throws, lands in the new applyFailed
   result bucket, stays unsettled, and is retried idempotently by a rerun.
   The CLI reports partial and failed clusters explicitly.
2. Staleness snapshots and settled fingerprints now cover the row TEXT
   (vectors follow text through re-embedding), so a concurrent text-only
   update makes a plan stale and re-opens a settled cluster.
3. The scan is bounded (--scan-limit, default 2500, truncation reported),
   clustering memoizes per-row topic tokens/reversal flags and precomputes
   unit vectors so each pair is a dot product, and the decide prompt is
   chunked (10 clusters/call) with per-chunk throw handling so a provider
   rejection strands only its own chunk.
4. The settled ledger is lock-serialized (mkdir lock with stale takeover),
   merges with the CURRENT file before writing, lands via temp-file rename,
   reports corruption and sets the damaged file aside instead of silently
   resetting, and prunes per scope (5000 entries / 90 days). Entries carry
   timestamps; the legacy bare-string format is normalized on load.
5. consolidation_audit is now an append-only array (legacy scalar wrapped,
   never overwritten).
6. excludeInactive default audit: the fact-query history walk and the
   memory-upgrader passes now opt back in to seeing inactive rows (the
   live-only default silently broke includeHistory and legacy migration).

Regression battery in test/memory-consolidate-two-phase-apply.test.mjs:
fault injection at every write position for both actions, concurrent
text-only changes, decide chunking with failure isolation, scan truncation,
a 1200-candidate clustering bound, and ledger merge/corruption/legacy/prune
semantics.
@gorkem2020

Copy link
Copy Markdown
Contributor Author

Round 1 addressed on head 85cb97b. Items 1-4 accepted and reworked; the tail asks are done too, with one probe-backed correction at the end.

  1. Partial apply: rather than faking cluster atomicity over independent row updates, the apply path is now explicitly recoverable/idempotent with safe write ordering. Merge writes the survivor's merged SUPERSET first, so any later failure leaves absorbed rows active as duplicates (the pre-consolidate status quo, safe direction); each absorbed invalidation is attempted independently and failures ride the audit entry as partialFailures. A cluster whose first write fails (merge survivor, or every supersede invalidation) throws, lands in a new applyFailed result bucket, is NOT settled, and a rerun re-clusters and retries it idempotently. The CLI prints partial and failed clusters explicitly instead of the underreported applied count. Fault-injection regressions cover every write position for both actions (survivor-first, absorbed-middle, absorbed-all, survivor-annotation-last).

  2. Staleness and fingerprints now cover the row TEXT alongside id+metadata (the vector follows the text through re-embedding, so a content hash over text+metadata captures every plan-relevant field). Regressions: a concurrent text-only change makes the plan stale at execution, and a text-only change produces a different fingerprint so a settled cluster re-opens. Note: the fingerprint format change re-opens previously settled ledgers once (one extra decide pass), which is the correct direction for a correctness fix.

  3. Scan cost: the row scan is bounded with a user-visible --scan-limit (default 2500, truncation logged and reported in the result), clustering memoizes per-row topic tokens/reversal flags and precomputes unit-normalized vectors so each pair check is a plain dot product, and the decide prompt is now chunked at 10 clusters per call with per-chunk try/catch, so a provider rejection or thrown completion strands only its own chunk (finer than the previous all-or-nothing). A 1200-candidate clustering regression pins interactive-time behavior.

  4. Settled ledger: writes are serialized by a lock (stale takeover after 60s), the CURRENT on-disk ledger is re-read and merged under the lock so concurrent agents never lose each other's fingerprints, the write lands via temp file + atomic rename, corruption is reported and the damaged file is set aside (never silently treated as empty), and entries carry timestamps with bounded pruning (5000 per scope / 90 days). The legacy bare-string format is normalized on load. Covered by merge/corruption/legacy/prune regressions.

Tail items: consolidation_audit is now an append-only array (a legacy scalar is wrapped, never overwritten); the decide prompt is bounded/chunked with provider-rejection catch as above; and the excludeInactive default audit found two real callers that needed to opt back in: the fact-query history walk (includeHistory was silently broken by the live-only default) and the memory-upgrader scan passes (legacy rows must be migrated even when invalidated). Both fixed with comments.

On splitting the "unrelated extraction/dedup prompt refactor": the src/extraction-prompts.ts diff in this PR is purely additive (+219/-1) and consists only of the consolidate feature's own prompt builders (buildConsolidatePrompt/buildConsolidateBatchPrompt/buildConsolidateBatchMergePrompt and their formatters); no existing extraction or dedup prompt is modified. They live in that module because it is the repo's prompt home, so there is nothing separable to split.

Full suite, manifest verifier (133 entries), and typecheck green.

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed head 85cb97b. The prior partial-apply reporting, text-aware freshness, decision chunking, and settled-ledger persistence work are substantial improvements; the focused/full suites and CI pass. Remaining issues block this head:

  1. The global excludeInactive=true default is still an unaudited compatibility break. Existing list, search, and fetchForCompaction callers silently change population when they omit options. The new revision fixes MemoryUpgrader and fact-history paths, but existing whole-store callers remain; for example scripts/migrate-governance-metadata.mjs:39 now skips invalidated historical rows during metadata migration. Preserve the previous API defaults and opt the new live-only call sites in explicitly, or audit/update every internal caller and document the external contract change with compatibility tests.

  2. --scan-limit does not bound the database scan or vector materialization. fetchForCompaction() still executes .toArray(), converts every matching vector, sorts the full result, and only then slices to limit. This protects the O(n^2) clustering input count but not the full-table read/heap spike called out previously. Push ordering/limit or paging into the LanceDB query before materialization. The 1200-row performance test uses two-dimensional vectors; add a production-dimension case at the real default boundary before treating 2500 as interactive.

  3. contradict is persisted as settled like skip. A dry-run can therefore hide an unresolved live contradiction for up to 90 days; later runs show only an aggregate settled count, with no list/reset command. Do not settle contradictions, or persist them as explicitly unresolved and give operators a visible retry/reset workflow.

  4. The topic fallback can cluster unrelated short rows. Overlap divides by the smaller token-set size, so one shared token yields 1.0, and substring matching makes tokens such as port match support or report. Same-category mutable rows can then be sent to merge/supersede despite being unrelated. Require stronger two-sided overlap and token-boundary/equality semantics, with these exact false-positive regressions.

  5. Audit callback failure is misclassified as a write failure. applied.push(audit) and all DB mutations happen before await deps.onAudit, but a rejecting callback enters the same catch, adds applyFailed, and logs nothing written. Isolate mirror/audit errors from store-apply classification so one cluster cannot appear in both applied and failed results.

Please also recompute time during settled-lock retries and make stale takeover ownership-safe, and reject duplicate absorbed_indices before issuing duplicate writes.

Requesting changes on this head.

… unsettled contradictions, boundary-token clustering, isolated audit mirror

Round-2 review:

1. The store keeps its pre-CortexReach#946 excludeInactive default (false) at every
   choke point; live-only reads are explicit per-caller opt-ins (retriever
   and dedup prefilter already passed it; the consolidate fetch, the
   admission novelty pool, and nothing else now do too). Existing
   whole-store callers -- migrate-governance-metadata included -- keep
   their population unchanged. The default-contract test file now pins
   default-inclusion plus opt-in exclusion per method.
2. fetchForCompaction is two-phase so the limit bounds materialization:
   a light pass (id/timestamp/metadata columns only, no vectors) ranks and
   filters, and full rows with vectors are fetched only for the limit
   survivors via chunked id lookups. A production-dimension (2560d)
   clustering regression joins the 2d one.
3. Contradictions are never settled: an unresolved live conflict
   re-surfaces in every run's plan output until the rows are fixed --
   leaving it unsettled IS the retry workflow. Regression pins that a
   contradiction is absent from newlySettled and not skipped next run.
4. Topic fallback: token matching is equality-or-compound-boundary (cola
   still matches coca-cola; port no longer matches support/report), and
   multi-token rows need two-sided overlap (>=0.6 of the smaller AND
   >=0.5 of the larger set). Single-token rows keep the motivating
   cross-lane behavior with whole-token anchors. Exact false-positive
   regressions included alongside the preserved motivating case.
5. Audit-mirror isolation: deps.onAudit runs outside the store-apply
   classification with its own catch, so a rejecting mirror is logged but
   can never land an applied cluster in applyFailed.

Tail: the settled-lock stale check recomputes its clock per retry, and a
verdict with duplicate absorbed_indices is rejected as malformed before
any duplicate write can be issued.
@gorkem2020

Copy link
Copy Markdown
Contributor Author

Round 2 addressed on head a3ba97e, all five plus the tail items:

  1. Default preserved: the store now keeps its pre-PR excludeInactive default (false) at every choke point, and live-only reads are explicit per-caller opt-ins, which was your option (a) and is clearly the cleaner contract. The retriever and the dedup prefilter already passed it explicitly; the consolidate fetch and the admission novelty pool now do as well, and no other caller wanted live-only (migrate-governance-metadata and every other whole-store caller keep their population untouched). The default-contract test file was rewritten to pin default-inclusion plus per-method opt-in exclusion.

  2. Bounded materialization: fetchForCompaction is now two-phase. A light pass selects only id/timestamp/metadata (the vector column never crosses the wire), filters and ranks newest-first, and slices to the limit; full rows with vectors are then fetched for just those survivors via chunked id-IN lookups. The heap spike from converting every matching row's vector is gone, and ordering semantics are unchanged. A production-dimension (2560d, 600 candidates) clustering regression joins the 2d case.

  3. Contradictions are no longer settled, at all: an unresolved live conflict re-surfaces in every run's plan output until the underlying rows are fixed, so re-running IS the retry workflow and nothing needs a reset command. Regression pins both halves (absent from newlySettled; not skipped by a subsequent run).

  4. Topic fallback: token matching is now equality-or-compound-boundary (cola still matches the coca-cola compound part; port can no longer reach support or report by raw substring), and multi-token rows require two-sided overlap (>=0.6 of the smaller AND >=0.5 of the larger set), so one incidental shared token cannot bridge them. Your exact false-positive cases are regressions. One deliberate carve-out, pinned by this PR's motivating fixture: a row whose ONLY content token anchors the pair ("Favorite drink: cola" vs the paraphrase) still clusters on a whole-token match, because that cross-lane case is the reason the fallback exists, and clustering only nominates the pair for the decider and the append-only shield to adjudicate.

  5. Confirmed, a bug I introduced in round 1: deps.onAudit ran inside the store-apply try, so a rejecting mirror re-classified an applied cluster as failed. It now runs after classification with its own catch; a mirror failure is logged ("store writes already applied") and can never put one cluster in both applied and applyFailed. Regression covers a throwing onAudit.

Tail: the settled-lock stale check recomputes its clock on every retry attempt (the captured timestamp went stale across the 100ms waits), takeover remains an atomic mkdir race after the stale rm, and a verdict carrying duplicate absorbed_indices is rejected as malformed before any duplicate write can be issued (regression included).

Full suite, typecheck, and manifest verifier green; the apply-path file now runs 39 tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants