feat(extraction): drop candidates that echo a recent manual store - #963
feat(extraction): drop candidates that echo a recent manual store#963gorkem2020 wants to merge 4 commits into
Conversation
…o-guard, PR 963 delta)
2b40a6a to
dfe94e9
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Reviewed head dfe94e9. The focused tests, full suite, and CI are green, but the guard currently trades duplicate rows for silent loss of legitimate memory updates.
-
Cross-turn fuzzy matching can discard corrections and qualified supersets. Matching is bidirectional and uses unordered token subsets without negation or qualifier awareness, while ledger entries have no TTL or consume-on-match rule. Both
Alice no longer works at AcmeandAlice works at Acme until Fridaymatch a recordedAlice works at Acmeand are dropped before admission. Ordinary expansions are also vulnerable because stopwords remain in the token set and a candidate containing 90% of the manual tokens is discarded wholesale. Please scope entries to a short, explicit lifetime/origin and make matching conservative for negation, changed values, temporal qualifiers, and candidates carrying additional facts; add regressions for each. -
The successful temporal
memory_updatepath is not recorded. Preference/entity supersedes return atsrc/tools.ts:2179-2192, before the only update-sidemanualEchoLedger.record()call. Record the new text after the superseding write succeeds and before that return, with handler-level coverage. -
Echo-only batches are treated as barren and retried. After all candidates are filtered, the result reports zero created/merged/skipped and no settled outcome. The auto-capture caller therefore defers the ingress text after already charging the extraction limiter, so later flushes can repeat the same model call. Count echo drops or otherwise mark an echo-only successful run as settled, and test the full auto-capture path.
-
Wrapped CJK echoes bypass matching.
tokenSet()splits only on whitespace, and the fewer-than-three-token guard runs before containment. A wrapped Chinese or Japanese sentence therefore fails unless it is byte-for-byte exact. Use a Unicode character/grapheme fallback or a conservative CJK containment path and add focused cases. -
memory_forgetleaves stale suppression state.ManualEchoLedger.clear()has no production caller, so a deleted manual fact can still suppress a later re-statement until write-count eviction or restart. Invalidate the relevant ledger entry on deletion, or make lifetime/consumption semantics remove this stale state safely.
Requesting changes on this head.
… settled echo batches, CJK fallback, forget invalidation Round-1 review: the guard must never trade duplicate rows for silent loss of legitimate updates. 1. Matching is now one-sided: a candidate is an echo only when it adds NOTHING beyond the manual text (exact, manual-contains-candidate, or full content-token containment after glue-word stripping). Negation and temporal markers on either side refuse the match, so corrections, changed values, qualified statements, and added facts always survive. The bidirectional Jaccard/subset fuzz is gone. 2. Ledger entries carry a 10-minute TTL and are consumed on match: one manual store suppresses at most one echo, so later identical statements are deliberate re-assertions and never dropped. 3. The successful temporal memory_update supersede path records the new text before its early return (handler-level regression included). 4. An echo-only batch counts its drops as skipped and reports settledOutcomes, so the auto-capture caller consumes the input instead of deferring a retry that re-runs the same extraction. 5. CJK fallback: whitespace-stripped containment with a marker-guarded wrapper budget, so wrapped CJK echoes drop while qualified or negated CJK statements survive. 6. memory_forget invalidates the deleted row's ledger entry on both the id and query paths. test/manual-echo-guard.test.mjs rewritten around the new contract (31 tests incl. full auto-capture path + handler-level supersede coverage).
|
Round 1 addressed on head 9bc03dd, all five accepted. The premise of your review was right: the old matcher optimized for catching echoes at the cost of eating legitimate updates, and the redesign inverts that priority (the worst case of the guard staying quiet is one duplicate row for dedup, which was the pre-guard status quo).
Full suite and manifest verifier green; the echo-guard file runs 31 tests. |
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed head 9bc03dd. The previous temporal-update, echo-only settlement, TTL, and tool-level forget fixes are present, and the focused/full suites plus CI pass. The revised matcher still has correctness blockers:
-
Consume-on-hit fails once an agent has multiple live entries.
match()createslivewithfilter(), but writes that array back tobyAgentonly when its length differs fromring. In the normal no-expiry case,live.splice(i, 1)mutates a detached copy while the Map still retains the matched entry. The single-entry test passes only because the empty-copy branch deletes the whole key. With two recent manual writes, a hit can therefore suppress repeated re-statements for the entire TTL. Always persist the post-splice array, and add a two-live-entry regression that matches one entry twice. -
The CJK branches still silently drop changed or additional facts. The shortened branch checks only
man.includes(cand)and never inspects the removed residual, so喜欢喝茶和咖啡is treated as an echo of the opposite manual statement用户不喜欢喝茶和咖啡. The wrapped branch treats any marker-free residual of at most eight characters as glue: on the current runtime,isNearIdenticalEcho("我住在北京市海淀区并养猫", "我住在北京市海淀区")returnstrue, dropping the new fact并养猫. Match only known wrapper forms or otherwise prove the residual is non-substantive; cover both exact cases. -
English token-set containment loses direction. The final rule ignores order and grammatical roles after stopword removal, so
Alice reports to BobandBob reports to Alicereduce to the same set and the reversed relationship is discarded. Require order-preserving/structural containment rather than bag-of-words subset membership, with directional relationship regressions.
Also route every deletion lane through targeted ledger invalidation (CLI delete/delete-bulk currently only invalidates reflection caches), and avoid the extra getById on memory_forget when smart extraction is disabled.
Requesting changes on this head.
…ive-residual CJK checks, order-preserving containment, deletion-lane invalidation Round-2 review: 1. match() now persists the post-splice array in every outcome: with two or more live entries the consume mutated a detached filter() copy while the Map kept the matched entry, so one manual store could suppress repeated re-statements for its whole TTL. Regression: two live entries, the same candidate matched twice consumes exactly one entry. 2. CJK residuals must be proven non-substantive: the shortened branch rejects a match when the REMOVED manual residual carries a marker (stripping the negated wrapper yields the opposite claim), and the wrapped branch accepts only residuals composed entirely of known reporting-glue fragments (short and marker-free was not enough: a three-character residual can be a new fact). Both review cases covered exactly. 3. English containment is order-preserving: candidate content tokens must appear in the manual text as an ordered subsequence, so reversed relationships (alice reports to bob vs bob reports to alice) survive. 4. Every deletion lane invalidates the ledger: CLI delete fetches the row pre-delete and invalidates its text across all agent buckets, CLI delete-bulk clears the ledger wholesale (fail-open), and the ledger is wired into contexts only while smart extraction is enabled, which also spares memory_forget its pre-delete getById fetch when disabled.
|
Round 2 addressed on head 72290ed, all three blockers plus both tail items:
Tail items: every deletion lane now invalidates the ledger — CLI delete fetches the row before deleting and invalidates its text across all agent buckets (the CLI cannot name the writing agent), CLI delete-bulk clears the ledger wholesale (fail-open: worst case is one uncaught echo for dedup to handle), and memory_forget keeps its targeted invalidation. The ledger is wired into the tool and CLI contexts only while smart extraction is enabled, which also removes memory_forget's pre-delete getById fetch in the disabled configuration. Focused file 37/37, full suite, typecheck, and manifest verifier green. |
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed head 72290ed. The consume persistence, CJK residual, ordered-direction, deletion wiring, and disabled-extraction fixes from the previous round are present, and the focused/full suites plus CI pass. The matcher still has a silent-loss blocker:
- Semantic verbs are stripped, allowing different assertions to match as echoes.
ECHO_STOPWORDSremoveshas,wants,likes, andprefers, and the ordered-subsequence path applies no minimum to the candidate content tokens. On this head, all of these are classified as echoes and dropped:- candidate
User wants a golden retriever named Maxvs manualUser has a golden retriever named Max; - candidate
User prefers Pythonvs manualUser prefers Go over Python for backend services; - candidate
User likes teavs manualUser prefers coffee over tea in the morning.
- candidate
These candidates assert a different relation or preference, yet the guard increments skipped and settles the input, so the memory is silently lost. Keep only true reporting glue in the stopword set, preserve semantic predicates, and require a meaningful candidate-side content floor before subsequence matching. Add the exact three regressions above while retaining the legitimate same-order wrapper cases.
- Supersede/update leaves the replaced text in the ledger. The new text is recorded, but the prior row text is not invalidated. A quick reversal back to the old statement can therefore consume that stale entry and be dropped even though the store no longer contains the old fact. Invalidate the replaced text on every successful update/supersede path.
The CLI invalidation additions are process-local and cannot clear the running gateway's in-memory ledger; the extra pre-delete read and clearAll() only affect the short-lived CLI process. Please remove that ineffective work or introduce an actual cross-process invalidation mechanism, and update the stale _initPluginState comment to match the conditional wiring.
Requesting changes on this head.
…ontent equality for wrap echoes, invalidate replaced text on update and supersede
Review round 3 follow-ups:
- ECHO_STOPWORDS is reporting glue only; has/have/had, wants/want,
likes/like, prefers/prefer are content tokens again, so a candidate that
swaps a predicate is never an echo
- the wrap-echo path requires the candidate's content tokens to EQUAL the
manual text's, in order, instead of forming an ordered subsequence: a
candidate that drops distinguishing content ("User prefers Python" against
"User prefers Go over Python for backend services") is a different
assertion and survives; the manual-side minimum doubles as the candidate
floor since equal sequences have equal length
- memory_store supersede, memory_update temporal supersede, and the plain
memory_update path invalidate the replaced row text in the ledger before
recording the new text, so a reversal back to a replaced statement is
never treated as an echo of a fact the store no longer holds
- the CLI ledger wiring (pre-delete getById, invalidateEverywhere, clearAll)
is removed: the CLI runs in its own process and could never reach the
gateway's in-memory ledger; the class documents the per-process scope and
the TTL-bounded window it implies
- the ledger construction comment in index.ts matches the conditional
wiring (recording happens only when a smart extractor exists)
- regressions: the three predicate/subsequence cases, a predicate-carrying
wrap echo that must still collapse, and replaced-text invalidation on all
three write paths
|
Round 3 addressed on head 1855fdc.
Gates: tsc, full suite, cli-smoke group, and build (dist committed) are green. |
Problem
When a user stores a fact manually with
memory_store(ormemory_update), the same conversation turn usually still flows through auto-capture extraction. The extractor then re-emits the just-stored fact as a candidate, and the pipeline spends judge, dedup, and merge LLM calls deciding what to do with a row that already exists verbatim. In the noisiest case the near-duplicate survives as a second row.Fix
A small deterministic echo guard:
memory_store/memory_updatetexts are recorded in an in-memory per-agent ring (8 entries, no persistence, no config surface).Paraphrase-level echoes (same meaning, different words) are deliberately out of scope: covering them would need embedding lookups and would risk dropping genuinely new memories. The word-level tiers plus the existing dedup layer cover the observed failure shape.
Notes
tools.tsregion; this diff is the last commit on the branch. Best reviewed after feat(tools): manual-priority supersede for memory_store #960 merges.test/manual-echo-guard.test.mjs(17 cases: tier matches, short-text guard, per-agent isolation, ring eviction); registered in the test chain and CI manifest.🤖 Generated with Claude Code