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.