feat(tools): manual-priority supersede for memory_store

[gorkem2020]

What Problem This Solves

Two manual-lane gaps let the store drift out of sync with what the agent explicitly decided to remember:

1. The duplicate pre-check (similarity > 0.98) rejects a manual memory_store outright. The agent believes the fact was saved (and tells the user so), but nothing new landed; when the near-duplicate carried an outdated value, the outdated value stays authoritative.
2. A contradicting update at ordinary similarity, for example a new value for a stored preference scoring around 0.8 against the old row, lands as a second live row beside the old one. Recall then serves two conflicting "truths" for the same fact.

Why This Change Was Made

With manualStoreSupersede: true, a manual store always takes priority: its text lands verbatim (never mutated, never dropped), and a similar existing row yields to it. Supersede targets are deterministic, with no LLM on this lane, in order:

• the near-identical neighbor the duplicate check used to reject;
• an active neighbor holding the same fact key at any similarity (the update/contradiction shape, fact-key aware so unrelated but vector-close facts are never invalidated);
• the existing 0.95-0.98 same-category versioned band, unchanged.

Anything else stores alongside, on the principle that a wrong supersede destroys a real fact while a duplicate is fixable noise. force keeps bypassing the pre-check entirely, and a manual-priority supersede writes a fresh overview instead of inheriting the superseded value's stale one. Superseded rows keep the full supersedes/superseded_by relation chain and stay in history.

User Impact

Default off: behavior is byte-for-byte identical unless the knob is set. With the knob on, "remember X" always results in X being stored, updates supersede old values instead of piling up beside them, and the tool result reports exactly what happened (created vs superseded with both ids).

Evidence

• New test/manual-store-supersede.test.mjs (6 tests, red-first): near-identical supersede, fact-key contradiction supersede at low similarity, unrelated-neighbor create-alongside, force bypass, knob-off compat pin, and the pre-existing band regression pin.
• Full test chain green locally; dist rebuilt; plugin manifest schema updated for the new knob.

[app3apps]

Reviewed head a5b5430. The opt-in behavior is valuable, and npm run build, the 6 focused supersede regressions, and GitHub CI pass. One correctness gap blocks the advertised fact-key update behavior.

The implementation searches for factKeyCandidate only inside vectorSearch(vector, 3, 0.1, ...). Therefore an active row with the same fact key is invisible when it ranks fourth behind unrelated but closer neighbors, or when its similarity is below 0.1. In either case the new value is stored alongside the old one, leaving the contradiction this option promises to remove. The low-similarity test does not catch this because its mock returns the supplied neighbors without honoring the production limit or minimum score.

Please resolve fact-key collisions through a complete metadata/key lookup (or otherwise make the contract explicitly bounded), and add regressions with three closer unrelated neighbors ahead of the same-key row plus a same-key row below the vector threshold. Requesting changes.

[gorkem2020]

Rebased onto the current master after the #950 merge. Test-registration files union-resolved (package.json test chain and scripts/ci-test-manifest.mjs pick up both sides' new entries), dist rebuilt from the rebased source. Local checks green, including 6/6 on the new manual-store-supersede suite.

[rwmjhb]

Re-reviewed head 0ca86e3 after the rebase. The test-registration merge is resolved and the focused/full test suites pass, but the prior correctness blocker is unchanged.

factKeyCandidate is still selected only from vectorSearch(vector, 3, 0.1, ...). A live row with the same fact key is therefore invisible when it ranks fourth behind unrelated neighbors or falls below the similarity threshold, so the new value is stored alongside the stale one. The added test double still does not enforce the production limit or minimum score. The implementation also uses find(), so only one active same-key collision can be reconciled.

Please use a complete, scope-aware lookup of active rows for fact-key supersession and add production-shaped regressions for a same-key row ranked fourth, below the score threshold, and multiple active same-key rows. Requesting changes.

[gorkem2020]

Addressed in 54d8aab. Fact-key collisions now resolve through a complete, scope-aware paginated lookup of active rows (same pagination shape as the temporal-fact query path), independent of vector ranking, and every active same-key row is superseded: one supersedes relation per target on the new row, each old row invalidated with a superseded_by link, invalidated history skipped, lookup fail-open like the duplicate pre-check. The test double is production-shaped now (vectorSearch honors limit and minScore, list() pages real rows), with the four requested regressions: same-key row ranked fourth behind closer unrelated neighbors, below the similarity floor, multiple active same-key rows, and already-invalidated history.

[rwmjhb]

Re-reviewed head 54d8aab. The complete scope-aware lookup resolves the prior top-K blocker, and the focused/full suites pass. Three correctness paths still block the advertised supersession guarantee.

1. Collision discovery, replacement insertion, and old-row invalidation are separate operations with no lock spanning the full read-modify-write sequence. Two concurrent same-key stores can both scan the original row, both return superseded, and leave both replacements active. Please move this into a store-level operation that rechecks, inserts, and invalidates under one cross-process lock/transaction, with a concurrent two-writer regression.

2. patchMetadata() null returns are ignored and thrown errors are only logged, after which every intended ID is returned in supersededIds. The current test double itself returns null for every patch, so the suite proves calls were attempted rather than that rows were invalidated. Treat null/throw as partial failure, report only confirmed invalidations, and add null/throw/partial-success tests.

3. An unrestricted near-duplicate becomes primaryTarget, and the new row inherits its memory_category and fact_key even when the requested storage category and same-key targets belong elsewhere. This can invalidate the correctly categorized fact and store its replacement under a different fact key. Build canonical metadata from the requested category/new fact key, inheriting only from a verified same-key/same-category target.

Please also preserve the new value temporal expiry in the supersede branch and exact-filter legacy NULL/global rows during agent-scoped scans. Requesting changes.

[rwmjhb]

Thanks for the latest iteration. The targeted verification and an independent npm ci && npm run build are green, and rebuilding leaves dist/ clean. One concurrency blocker remains before merge:

• storeSuperseding() acquires the cross-process write lock, but its locked recheck calls discoverTargets() through the already-open LanceDB table handle. With two MemoryStore instances using a nonzero readConsistencyInterval, the second writer can still read a stale snapshot after the first writer commits. On this head, a two-instance reproduction left both replacement rows active even though the writes were serialized. The lock orders writers, but it does not make that reused handle observe the preceding commit.

Please refresh/reopen the table snapshot after acquiring the cross-process lock, or otherwise force a strong-consistency read for this read-modify-write path. A regression should use two independent store instances/processes with a nonzero consistency interval and assert that exactly one active same-key row remains. Once that invariant is covered, the core direction looks solid.

[gorkem2020]

Fixed in 8051f17. The locked section now forces the open table handle onto the latest committed version (checkoutLatest) before the re-discovery, so the read-modify-write path observes every commit that preceded its lock acquisition. A sync failure propagates instead of proceeding on a possibly-stale snapshot, since continuing would silently reintroduce the staleness the guard exists to close.

Regression added per your shape: two independent MemoryStore instances on the same database with a 30 second readConsistencyInterval, the second instance's snapshot armed before the first writer commits. Without the re-sync it reproduces your result (both replacement rows active); with it, exactly one active same-key row remains and it is the last writer's replacement.

One note for the record: the re-discovery necessarily scans under the lock, which is what makes the committed target set authoritative; narrowing the locked recheck to the advisory targets could miss a concurrent writer's replacement, the exact class this change closes, so the scan intentionally stays whole.

[rwmjhb]

Re-reviewed head 53fdde5. Refreshing the table under the supersession lock closes the previously reported stale-handle path, and the focused/full suites pass. One default-path compatibility regression still blocks merge.

The existing 0.95-0.98 auto-supersede band now runs through buildSupersedeMetadata(), which prefers the replacement text's derived newFactKey over the verified target's explicit meta.fact_key. The pre-PR path preserved oldMeta.fact_key first. This occurs even with manualStoreSupersede=false: an exact-head reproduction changed the established key preferences:theme to preferences:i prefer light mode. The active row and its predecessor are then split across different canonical identities, so later fact-key history and updates can miss the replacement. Please preserve the verified target's explicit key on the legacy/default-off band, deriving a new key only when it has none; keep requested-key precedence limited to the opt-in manual-priority path. Add a knob-off regression.

The opt-in concurrency invariant also remains incomplete: storeSuperseding() is called only when the unlocked advisory discovery already found a target. Two first-time same-key writers can both see an empty result and fall through to ordinary store(), leaving both rows active. For manual-priority writes, enter the lock-scoped operation even when advisory discovery is empty and perform authoritative rediscovery before deciding between create and supersede. Add an initially empty, two-store interleaving regression.

Please also reconsider category-unchecked invalidation and the direct table.add() insertion boundary, but the canonical-key regression above is the default-path blocker. Requesting changes.

[gorkem2020]

Head 9be640e addresses both blockers.

Default-band canonical identity. buildSupersedeMetadata now keeps requested-key precedence only on the opt-in manual-priority lane; the 0.95-0.98 band preserves the verified target's established key, so an identity like preferences:theme survives a rewording and later fact-key history stays on one canonical identity. A knob-off regression pins the explicit-key case (it reproduced your preferences:theme split on the previous head), and a companion case pins keyless targets: the metadata parser derives a key from the target's own abstract, the replacement inherits it, and the replacement's wording never mints the identity on this band.

Empty-advisory first writers. A manual-priority write now enters the lock-scoped storeSuperseding even when the unlocked advisory discovery finds nothing. The authoritative rediscovery under the lock decides between create and supersede: the first writer's rediscovery is empty and commits a plain create, reported as created rather than as a superseded-nothing, and the second writer's rediscovery sees that committed row and supersedes it. The new interleaving regression starts from an empty table, barriers both advisory discoveries before either commit, and asserts exactly one active row plus the created/superseded action split; it failed both assertions on the previous head. One deliberate side effect: a first manual-priority create now stamps its derived fact key, which is what makes it discoverable to the second writer's key lookup under the lock.

The category-unchecked invalidation and the direct table.add() insertion boundary from your note are tracked as follow-ups. Full suite, typecheck, and a fresh dist are green on the new head.

[rwmjhb]

Thanks for the update. Current head 9be640e has merge conflicts with the latest base, so the new changes cannot be reliably re-reviewed yet. Please rebase onto the latest master, resolve the conflicts, rerun the relevant tests, and push the resolved head for another review.

[rwmjhb]

Re-reviewed head 6ad6a31. The repository CI is green, the orchestrator full suite passes, and independent reruns of all four targeted gates pass; the aggregate targeted timeout was a review-harness false positive rather than a reproducible PR failure.

The updated implementation addresses the prior full-discovery, stale-snapshot, and first-writer correctness blockers. The remaining cross-category similarity policy, full-scope scan cost, and partial-invalidation metadata concerns are worthwhile follow-ups, but none rises to a merge blocker on this head. Approving.

[rwmjhb]

Thanks for the update. After the recent merges, current head 6ad6a31 now has merge conflicts with the latest master, so the approved revision cannot be merged as-is. Please rebase onto the latest master, resolve the conflicts, rerun the relevant targeted tests and full CI suite, and push the resolved head for a quick re-check.

[gorkem2020]

Rebased onto current master (post #952) as requested; review-round history squashed into a single commit for a clean re-verification. Full suite green, dist rebuilt.

[rwmjhb]

Reviewed head 4139d38. The full suite and CI are green, but two correctness/performance issues still block this revision:

1. findActiveFactKeyEntries() paginates by repeatedly calling store.list(..., 500, offset), while MemoryStore.list() fetches and sorts the entire matching scope before applying .slice(offset, offset + limit). Discovery runs once advisably and again inside storeSuperseding()'s cross-process write lock, so opted-in writes perform roughly quadratic full-scope work and can hold the global writer lock long enough to cause latency or lock timeouts on large stores. Please use a genuinely database-paginated/indexed fact-key lookup (or otherwise make the authoritative locked scan linear and bounded), and add a query-count/large-scope regression.

2. Collision discovery compares entryKey === factKey, but explicit fact_key values are only trimmed while derived keys are lowercase; the existing fact-query path compares keys case-insensitively. A legacy/imported Preferences:Theme row is therefore query-equivalent to preferences:theme but can evade supersession, leaving both values active. Normalize both stored and requested keys with the same trim/lowercase rule and add a mixed-case explicit-key regression.

[gorkem2020]

Both blockers addressed in a631dfe.

1. findActiveFactKeyEntries() no longer paginates. Since MemoryStore.list() materializes the whole matching scope on every call, the scan now issues exactly one bounded call (FACT_KEY_SCAN_MAX_ROWS = 20k), making each discovery pass linear. A scope past the bound throws, and the discovery wrapper's existing catch turns that into the path's established fail-open (store alongside, warn), so lock hold time is bounded and an incomplete collision set is never acted on silently. Query-count regression added: on a 1200-row scope the whole opted-in write performs exactly 2 list calls (advisory pass + locked recheck); on the previous head the same test measures 6.

2. Collision discovery now normalizes both sides with the same trim/lowercase rule the fact-query path uses (shared normalizeFactKeyForComparison() helper, also adopted by factQueryMatches so the rule has one definition). Mixed-case regression added: an explicit Preferences:Favorite Drink row is superseded by a write whose derived key is preferences:favorite drink; on the previous head it evades and both stay active. A third test pins the over-bound fail-open (stores alongside, no partial supersede).

Typecheck, build (dist committed), manifest verifier, and the full suite are green.

[rwmjhb]

Re-reviewed head a631dfe. The previous repeated-pagination and mixed-case blockers are addressed, and the targeted/full suites plus GitHub CI are green. One correctness issue remains in the replacement implementation.

findActiveFactKeyEntries() requests 20,001 scoped rows and throws whenever the scope exceeds 20,000 rows, before filtering for active rows or the requested fact key. runSupersedeDiscovery() catches that error and continues with no fact-key targets. An opted-in manual-priority update can therefore succeed as created while leaving the old same-key value active. This is deterministic rather than hypothetical: the new over-bound regression explicitly expects storage alongside without invalidation, and inactive history created by this feature also counts toward the cutoff.

Please make the collision lookup database-side and scope/fact-key-aware, using the normalized key and active-row predicate with an actual query bound/index; alternatively, fail the write explicitly instead of silently abandoning the supersession invariant. A single list() call does not currently bound the authoritative work either, because MemoryStore.list() materializes and sorts the full matching scope before applying its limit, and this pass runs under the writer lock.

Requesting changes.

[gorkem2020]

Round 2 addressed, and the branch is also rebased onto current master (post #934; registration-file unions were the only rebase conflicts).

The collision lookup is now database-side, scope and fact-key aware, and genuinely bounded: a new MemoryStore.listFactKeyCandidates(scopeFilter, memoryCategory, bound) pushes the scope conditions, a metadata narrowing, and a hard limit(bound + 1) into the LanceDB query with no sort and no full-scope materialization. The narrowing is complete by construction: a colliding row either carries an explicit fact_key (matched case-preserved in metadata, any category) or matches by derived key, and derived keys embed their category, so same-memory_category covers that half. Exact normalized-key comparison and the active-row predicate stay in findActiveFactKeyEntries on the narrowed candidate set.

Past the bound, the write now FAILS EXPLICITLY instead of silently abandoning the supersession invariant: FactKeyScanOverBoundError propagates out of both the advisory pass and the locked recheck (the locked path aborts before the insert, so no partial state), and the tool returns a rejection naming force: true as the escape hatch that stores without superseding. Nothing is written on rejection.

Regressions updated and added, all red-proved against the previous head: the query-count test now asserts two bounded candidate queries and zero list() calls per opted-in write; the over-bound test asserts the explicit rejection (nothing stored, no patches, force named); a force-bypass test pins verbatim store with no supersede; and a real-store test pins the database-side narrowing (in-scope explicit-key and same-category rows in, other categories and scopes out) plus the bound+1 detection contract.

Typecheck, build (dist recommitted), manifest verifier, and the full suite (50 files, 0 failures) are green.

[rwmjhb]

Re-reviewed head 390779e. The previous repeated full-scope scan, mixed-case comparison, and silent over-bound fallback are addressed, and the targeted/full suites plus GitHub CI are green. One correctness blocker remains.

listFactKeyCandidates() narrows rows using raw metadata patterns for fact_key or an exact memory_category field. However, parseSmartMetadata() also derives both the effective category and fact key from the legacy storage category column and row text when metadata is empty. A real-store case with category="preference", metadata="{}", and text="favorite drink: cola" therefore has the effective key preferences:favorite drink but is excluded from the candidate query. Storing favorite drink: tea with manual supersession enabled then returns created and leaves both values active.

Please make the database narrowing complete for legacy/keyless rows as well, for example through a persisted normalized fact-key field/index with backfill or by including every relevant legacy storage-category candidate before exact normalized-key filtering. Add an end-to-end real-store regression using an empty-metadata legacy row.

Requesting changes.

[gorkem2020]

Round 3 addressed in ace745d. listFactKeyCandidates() now includes legacy keyless candidates database-side: a third narrowing leg matches rows whose storage category column reverse-maps to the requested temporal-versioned category (preference/preferences for preferences, entity/entities for entities), computed from the same mapping table reverseMapLegacyCategory() reads, so a category="preference", metadata="{}" row is now a candidate. Exact normalized-key and active-row filtering are unchanged at the caller, and the scan bound applies to the widened set.

Tests: the real-store narrowing regression now seeds an empty-metadata legacy row and asserts it is returned (and that a keyless fact row stays out), plus the requested end-to-end regression: storing favorite drink: tea over a legacy category="preference", metadata="{}", favorite drink: cola row supersedes it and leaves exactly one active row, with the legacy row invalidated. Both assertions were red before the fix (the write returned created), green after. Typecheck and the full suite pass.

[rwmjhb]

Re-reviewed head ace745d. The legacy storage-category widening fixes the empty-metadata case from the previous round, and the targeted/full suites plus GitHub CI are green. One completeness blocker remains in the database narrowing.

listFactKeyCandidates() still relies on formatting-sensitive raw JSON patterns such as metadata LIKE '%"fact_key":%'. Valid metadata like {"fact_key" : "Preferences:Favorite Drink"} is accepted by JSON.parse() and resolves to the normalized key preferences:favorite drink, but it does not contain that exact substring. When stored under the ordinary fact category, it also misses the legacy preference-category branch. A real-store reproduction then stores the replacement as created and leaves both query-equivalent values active.

Please avoid using raw JSON serialization layout as a candidate-index condition. Use database JSON extraction, a persisted/indexed normalized fact-key field with migration/backfill, or another narrowing that cannot exclude valid metadata before exact normalized-key filtering. Add a real-store regression with whitespace-formatted explicit metadata.

Requesting changes.

[gorkem2020]

Round 4 addressed in cf2d82a, by removing the class rather than patching the instance: listFactKeyCandidates() no longer applies any content-based narrowing. Since an effective key can arrive through an explicit field in any valid JSON layout (spaced colons, unicode escapes), a stamped category in the same layouts, or derivation from the storage category column, no serialization-layout pattern can be a sound candidate index, so the query now narrows on scope alone with the same limit(bound + 1) push-down and no sort. Completeness comes from the scope itself; the existing bound with the explicit over-bound rejection stays as the cost control, and exact normalized-key plus active-row filtering stay with the caller. The round-3 legacy-category helper became dead under this shape and is removed.

On cost, since round 1 was a cost finding: this is one unsorted, projection-limited query per discovery pass (no vectors fetched), never repeated pagination or full-scope sort, and scopes past the bound reject explicitly with the force escape. If scan volume ever matters in practice, a persisted normalized-key column with backfill is the natural follow-up index, but it is a migration and feels out of scope for this lane.

Tests: the requested real-store regression seeds '{"fact_key" : "Preferences:Favorite Drink", ...}' (whitespace layout) under storage category fact, end to end: the manual write supersedes it and exactly one active row holds the normalized key. Red before this commit (the write returned created), green after. The database-side narrowing regression now pins scope-only semantics (every in-scope row is a candidate, other scopes stay out, bound + 1 detection). Typecheck and the full suite pass.

[rwmjhb]

Re-reviewed head cf2d82a. The scope-only candidate scan removes the formatting-sensitive metadata narrowing from the previous round, and the whitespace-formatted real-store regression now covers that case. All targeted tests, the full suite, and GitHub CI pass.

The remaining fact-key coverage, partial-invalidation, large-scope, and cross-category-policy concerns are non-blocking follow-ups for this opt-in feature.

Approved.

[rwmjhb]

PR #941 has now merged, and this branch currently has merge conflicts with the latest master. Please rebase onto current master, resolve the conflicts, and push the updated branch. We will re-review the new head after CI completes.

[gorkem2020]

Rebased onto current master (post #941); the round history is squashed into one commit, registration files re-unioned, typecheck and the full suite pass, dist rebuilt in-commit.

[rwmjhb]

Re-reviewed rebased head 714ab0e. The conflict resolution preserves the previously approved supersession behavior; targeted tests, the full suite, source/dist verification, and GitHub CI are green.

The remaining scope-canonicalization, partial-invalidation result, and large-scope scan concerns are non-blocking follow-ups for this opt-in feature.

Approved.

[rwmjhb]

PR #944 has now merged, and this branch currently has merge conflicts with the latest master. Please rebase onto current master, resolve the conflicts, and push the updated branch. We will re-review the new head after CI completes.

[gorkem2020]

Rebased onto current master (post #944 and #986). Real composition in src/store.ts: master's performUpdateLocked rename kept as the single locked body, this branch's supersede commit path now calls it, docstrings merged. Registration union resolved, full gates green. Mergeable again.

[rwmjhb]

Re-reviewed rebased head 1f56e00. The conflict resolution preserves the previously approved manual supersession behavior. The full local suite and all GitHub CI checks pass, and the generated retriever update matches source behavior already present on the parent revision.

Partial-invalidation consistency, cross-category near-duplicate policy, default-off temporal metadata parity, and large-scope scan/lock cost remain non-blocking follow-ups for this opt-in feature.

Approved.

[gorkem2020]

Rebased onto master after #942's merge (registration-chain union only, no source conflicts). Full gates green: typecheck, fresh dist, manifest verifier, full suite.