feat: add a consolidate command to reconcile duplicate and superseded memories across write lanes - #946
Conversation
b47d619 to
16be783
Compare
|
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)
0184bd1 to
8209f55
Compare
rwmjhb
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
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.
-
A merge or supersede can be partially applied.
writeMergeVerdictrewrites the survivor first and then invalidates absorbed rows one at a time;applySupersedeVerdictlikewise performs independent row updates. If update k of N fails,executePlancatches the error, logs it, and continues without rollback, retry state, anappliedentry, or an audit event. The survivor may already contain all merged content while some source rows remain active, recreating duplicate retrieval with an underreportedApplied 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. -
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.
-
The scan can become pathologically expensive before the first LLM call.
fetchForCompactionmaterializes 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. -
consolidate-settled.jsonis 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.
|
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.
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
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
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:
-
The global
excludeInactive=truedefault is still an unaudited compatibility break. Existinglist, search, andfetchForCompactioncallers silently change population when they omit options. The new revision fixes MemoryUpgrader and fact-history paths, but existing whole-store callers remain; for examplescripts/migrate-governance-metadata.mjs:39now 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. -
--scan-limitdoes not bound the database scan or vector materialization.fetchForCompaction()still executes.toArray(), converts every matching vector, sorts the full result, and only then slices tolimit. 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. -
contradictis persisted as settled likeskip. 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. -
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
portmatchsupportorreport. 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. -
Audit callback failure is misclassified as a write failure.
applied.push(audit)and all DB mutations happen beforeawait deps.onAudit, but a rejecting callback enters the same catch, addsapplyFailed, and logsnothing 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.
|
Round 2 addressed on head a3ba97e, all five plus the tail items:
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. |
rwmjhb
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed head a3ba97e. The previous store-default, contradiction, token-boundary, audit-classification, and duplicate-index issues are addressed, and the focused/full suites plus CI pass. Two correctness issues remain in the revised design:
-
The new light pass bypasses the store's projection fallback.
fetchForCompaction()directly calls.select(["id", "timestamp", "metadata"]).toArray(), while this same store already usesqueryRowsWithProjectionFallback()because affected LanceDB versions can return an empty projected metadata read even when the table has rows. On those versions, consolidate silently reportsScanned 0 rowsand does nothing. Route this read through the established fallback and add a fetchForCompaction regression for an empty projected result followed by a populated unprojected result. Also re-checkexcludeInactiveon the full rows: a row invalidated between the light and heavy phases is currently returned from the heavy pass and can enter consolidation as live. -
A dry-run durably changes future consolidation results. The CLI saves
newlySettledfor every completed result, including a preview whose apply is declined or never requested. One mistaken LLMskipduring a dry-run then hides that unchanged cluster from all later previews and--applyruns for up to 90 days, with no list, ignore, or reset command. A dry-run should not mutate durable state. Persist settlement only after an explicit commit action, or make this state transition explicit and provide an operator-visible inspect/reset/bypass workflow.
Non-blocking CLI follow-ups: clarify that non-interactive execution needs both --yes and --apply so automation does not repeatedly pay for plans that can never apply; clean up abandoned ledger temp files; and consider avoiding a full metadata JSON parse for every row merely to produce stats.liveCount.
Requesting changes on this head.
…ess on the heavy pass, settlement persisted only on commit Review round 3 follow-ups: - fetchForCompaction routes its light (id/timestamp/metadata) pass through queryRowsWithProjectionFallback, so the LanceDB versions that return an empty projected read no longer make consolidate report "Scanned 0 rows"; the heavy id-IN pass re-judges excludeInactive on the fresh metadata and drops rows invalidated between the two passes - the CLI writes the settled ledger only when the run committed (--apply or an interactive YES); a dry-run reports its skip verdicts as not recorded and never mutates durable state - --yes/--apply help text and the abort messages state that automation needs both flags - saveConsolidateSettledLedger removes its temp file on a failed write and sweeps abandoned .tmp-<pid> files older than the lock-stale window - stats() decides liveness without a metadata parse for rows carrying neither invalidated_at nor valid_from (the parse result is fixed for those) - regressions: empty projected light read followed by a populated unprojected read; between-phase invalidation; the CLI dry-run, re-decide, apply, settled-rerun sequence
|
Round 3 addressed on head 8811bf4.
Non-blocking items taken in the same push: the Gates: tsc, full suite, cli-smoke group, and build (dist committed) are green. Update Sep 7: round 3 has been on this head since Sep 2 with no further changes. If anything in the requested changes is still open, say which item and it gets fixed the same day; otherwise this should be ready for another look. |
rwmjhb
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
Thanks for the update. The projection fallback, dry-run behavior, heavy-pass liveness recheck, settled-ledger persistence guard, stats fast path, and temporary-file cleanup are now addressed, and both the targeted checks and full test suite pass.
One blocker remains:
- The apply path still treats incomplete work as a successful CLI invocation. When
result.partialFailuresis non-empty orresult.applyFailedis true, the command only prints the condition and exits with status 0. As a result, cron jobs, CI, and scripts runningmemory consolidate --apply --yescannot distinguish a fully applied run from one where mutations failed or were only partially applied. Please return a non-zero exit status for every incomplete/failed apply and add a CLI regression test that asserts the exit code.
Non-blocking follow-ups: the single-topic-token candidate shortcut can admit unrelated same-category rows based on one token alone; explicit scan limits should have a reasonable upper bound; and the fallback tokenizer is ASCII-only.
An --apply run that ended with a cluster that failed to apply, or one that applied only partially, printed the condition and still exited 0, so cron jobs, CI and scripts could not tell it from a clean apply. Set the exit status to 1 (via exitCode, so the report is fully flushed) whenever result.applyFailed is non-empty or any applied cluster carries partialFailures, and say so on stderr. The --apply help text documents the status. Regression: a CLI test drives three applies through the command (nothing written, partially written, clean) and asserts the exit status.
|
Round 4 addressed on head 4a49cac: an The three non-blocking follow-ups (single-topic-token candidate shortcut, an upper bound on |
rwmjhb
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
Reviewed the latest fix at 4a49cac. The previous blocker is resolved: an apply with failed or partially applied clusters now prints the complete report and sets process.exitCode = 1, while a clean apply retains status 0. The new CLI regression covers all three outcomes.
I also fetched GitHub's current synthetic merge (20f7d5f, PR head merged into current master at 0f91791) and ran the full npm test suite plus npm run build; both passed and the generated tree remained clean.
The multi-row apply is intentionally recoverable rather than transactional: the survivor is written first, a failed absorbed-row invalidation leaves that row active, partial failures are reported, and applied clusters are not entered into the settled ledger. With the new non-zero process status, I consider that failure model acceptable. The single-topic-token heuristic, Unicode tokenization, and an upper bound for explicit scan limits remain worthwhile follow-ups, but are not blockers for this revision.
2 participants
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
consolidateCLI command, registered under thememory-procommand group, that scans a scope's existing rows and reconciles duplicates, near-duplicates, and reversals across all three write lanes: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 / contradictvocabulary fromsrc/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.
mergereuses the existing merge-writer prompt (buildMergePrompt). Bothmergeandsupersedenow soft-invalidate the absorbed rows (invalidated_at/superseded_by, plus aconsolidation_audittrail pointing back at the survivor) rather than deleting.deletewas removed fromConsolidateWriteDepsentirely, 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/casesrows 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-duplicateeventsrows 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.
excludeInactivenow defaults totrueacrossvectorSearch,bm25Search,lexicalFallbackSearch,list, and the compactor'sfetchForCompactionpath, so invalidated/superseded rows are invisible everywhere by default instead of only where a caller remembered to filter.stats()gains aliveCountalongside the existingtotalCount, surfaced in both--jsonand the human-readable CLI output. The existingexportand vault-export paths explicitly opt out (excludeInactive: false) to keep their full-dump, forensic-backup semantics;listand vault-export also gain an--include-invalidatedflag for callers who want the historical rows back.Cost gate. Clustering is free and runs first. Before any LLM call fires, dry-run or
--applyalike, a mandatory cost-preview reports real numbers (N clusters -> 1 batched decider call, plus up to M merge-content generations).--yesbypasses 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 existingmdMirrorjournal writer'smeta.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.tsandcli.tsnever referenceadmission-control.ts, andadmissionControl.enabledhas no bearing on whether consolidate's own LLM client gets constructed. Pinned with a regression test using a poison-pillAdmissionControllerwhose 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-slicesopts in. Already-invalidated rows are excluded from future scans, which is what makes--applyidempotent.Tests
Red-first throughout.
test/memory-consolidate.test.mjscovers 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.mjsandtest/store-excludeinactive-default.test.mjscover the store-layer default flip and its fallout across existing call sites (with matching updates totest/store-empty-scope-filter.test.mjs,test/migrate-legacy-schema.test.mjs, andtest/temporal-facts.test.mjs).test/memory-consolidate-cost-gate.test.mjs,test/memory-consolidate-two-phase-apply.test.mjs, andtest/memory-consolidate-admission-independence.test.mjscover the cost gate, two-phase apply plus staleness, and admission independence. All new files are registered in bothpackage.json's test chain andscripts/ci-test-manifest.mjs.npm run build(tsc) and the full localnpm testchain are green, with one pre-existing, environment-specific test skipped (a host-side port 11434 conflict unrelated to this change). A structural regression test confirmsconsolidateis attached under thememory-progroup, 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-batchcall per chunk of up toCONSOLIDATE_MERGE_BATCH_MAX_SIZE(10) verdicts, instead of oneconsolidate-mergecall 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.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 duplicatedextraction-prompts.tsacross sibling branches, so the shared module follows the same convention) and converged every prompt builder in the file onto it:## Cluster N/### N. category/ plainLabel: valuefield lines /#### Existing memoryand#### New informationnested 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.buildDedupPromptandbuildMergePromptnow 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 anextraction-prompts.tsmerge 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:
--agent-only: scope auto-derives asagent:<id>,--scopeis gone, and journal-mirror writes always route to that agent's workspace.dbPath/consolidate-settled.jsonand dropped before the cost gate on later runs; any member change re-opens its cluster. A fully converged store prints "0 candidates".