feat(memory): add supersedes links and recall demotion - #140
Merged
Merged
Conversation
Recall scores an insight from keyword overlap, cosine similarity, entity
overlap and graph centrality. None of those can express "this fact
replaced that one", so a corrected insight competes with its own
correction on similarity alone -- and usually wins.
The mechanism is worth stating because it is counter-intuitive. A
correction is normally written as a diff ("X is wrong, use Y"), so it
contains the wrong wording verbatim. A query phrased with the wrong
wording matches the stale row at least as well as the correction. The
stale row is also older, so it has accumulated more edges and a higher
access count, which lifts its graph signal. Recording a correction
therefore does not stop the error being served.
Add 'supersedes' as a fifth edge type. It differs in kind from the
existing four: those are similarity or co-occurrence signals, this is an
authority claim. An insight targeted by a supersedes edge has its score
multiplied by supersededScoreFactor and is flagged Superseded in the
result, including through the compact recall projection -- an agent
reading only that shape would otherwise be handed corrected content with
no signal.
The factor is 0.25, which is to say a superseded row must be a four
times better match than its replacement to still outrank it. Happy to
make it configurable if preferred.
This is a demotion, not a filter. The row stays retrievable so lineage
and audit still work.
Nothing changes for existing databases until a supersedes edge exists,
since the lookup returns an empty set and every score is untouched.
The schema migration follows the pattern established for the narrative
migration, including running its probe with foreign key enforcement off:
the probe inserts a sentinel edge whose endpoints do not exist, so with
enforcement on it fails on the foreign key rather than the CHECK,
reports "not yet migrated" on every open, and rebuilds the whole edges
table each time. The added test pins that by comparing the table's
rootpage across two opens.
The existing idempotence test only reopens a store that already admits 'supersedes', so it never exercises migrateAddSupersedesEdgeType's table rebuild. Rewrite sqlite_master to the four-type CHECK, keep that handle open so its schema cache stays wide, and let a second Open read the on-disk schema and run the migration. Fails without the rebuild on "supersedes edge type must be admitted after migration". Also asserts the pre-existing semantic edge is copied and that no __probe_ row survives.
The two migration tests asserted that no edge id begins with `__`, which is broader than the thing being tested: `__test` is a legitimate insight id in this same file, so a real store could fail the assertion with no probe row in sight. Match the actual `__probe_` sentinel instead, on both endpoints, through one helper the two tests share. Still catches what it is for: committing the probe instead of rolling it back fails TestMigrateAddSupersedesEdgeType_IsIdempotent with "probe rows leaked into edges: 2".
GetSupersededIDs read every 'supersedes' edge in the store on every recall, then used the result to answer a question about the candidate set alone. That cost grows with the store's whole supersession history on a hot path, while the answer needed is bounded by the candidates in hand. Take the ids as an argument and let idx_edges_target_type serve them from the index, batched so no IN clause can approach SQLite's host-parameter ceiling. TestGetSupersededIDs_ScopesToRequestedIDs pins both halves: dropping the IN clause fails it with "lookup answered about ids the caller never asked for", and a match placed past the first batch must still be reported.
The comment described a shape this function never had: it spoke of the probe running in autocommit and of a cleanup delete "below", but the probe is always rolled back and nothing here deletes a sentinel. State the hazard as the counterfactual it is, and name what the rollback actually buys.
`link` writes every edge in both directions, which is right for similarity and co-occurrence but wrong for the type this branch adds. A reversed supersedes edge says the correction is itself superseded, so recall demotes both insights by the same factor, their order is unchanged, and the stale insight keeps its lead — the exact ordering the type exists to fix. Mark supersedes directed on the model and write the reverse edge only for mutual types. Derive the CLI's valid-type help and error text from ValidEdgeTypes as well: the flag listed supersedes while the error beside it still named four types. Found by smoke-testing the built binary against a scratch store, where a linked pair came back with superseded=true on both rows. Restoring the unconditional reverse write fails the new test with "supersedes edges by source = [fresh stale], want [fresh]".
related --edge and the import draft schema still listed the four MAGMA types after the link path started accepting supersedes. A filter or draft that used the new type would then fail validation even though ValidEdgeTypes already contained it. Derive both error strings from EdgeTypeNames, document the type in the usage and import tables, and stop hard-coding the MAGMA quartet in GetNeighborhood's comment. Validated with go test ./cmd/memory ./internal/memory/importdraft ./internal/memory/model ./internal/memory/search ./internal/memory/store ./internal/memory/graph.
Preserve all seven contributor commits while applying them to current master. Resolve the single test insertion conflict by retaining both stored_at migration and supersedes coverage.
Carry the superseded marker through the brief discovery projection so agents can distinguish corrected memories in every smart recall output. Current results continue omitting the optional field. Validated all three projections against a real SQLite store with embeddings disabled; focused recall and brief tests pass.
Return a contextual error when recall cannot read supersession authority, instead of silently returning stale facts at full score with no marker. Preserve the underlying SQLite error for callers. Validated with real SQLite schema failure injection and the intent-aware recall regression tests.
Require distinct endpoints when writing supersedes edges. This makes direct self links fail and lets import warn and skip an edge whose draft endpoints deduplicate to the same memory. Existing self edges remain stored but cannot demote a fact. Validated the link command, deduplicating draft import, and scoped lookup against real SQLite stores, including an existing invalid self edge.
Only a SQLite CHECK constraint failure may authorize the supersedes table rebuild. Preserve existing schema and return trigger, lock, and other probe failures instead of treating them as an old edge constraint. Validated a real closed legacy database, repeated reopen, orphan retention, restored foreign keys, and a trigger failure that previously caused an unnecessary rebuild and removed its guard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a directed
supersedesedge so callers can explicitly mark a new memory as replacing an older one. Smart recall multiplies the replaced memory's score by 0.25 and exposessupersededin compact, verbose, and brief output while retaining the original memory for inspection.Integrates #98 with all seven original author commits preserved, plus four focused corrections: retain the marker in brief recall; report failed supersession lookups; reject self-supersession, including draft indices collapsed by deduplication; and permit a schema rebuild only after a SQLite CHECK-constraint probe failure. Migration tests cover real legacy databases, reopen stability, record preservation, and a trigger failure that must preserve the existing schema.
The author's fork does not allow maintainer edits, so this branch provides the integration path. Current mainline behavior from #138 and #139 is preserved: multilingual recall intent remains available, and different facts remain separate during remember/import. Supersession is explicit and applies to smart recall; existing WHY causal ordering and basic/search behavior remain intact.
Validation: product build,
make test, and the fullmake test-integrationsuite passed one788ce115563d66e32a298d17dd71ada97d1b1a1, including 258 CLI E2E assertions, Go and race suites, Docker scenarios, the Pi runtime oracle, and the domain-operations world. Additional checks passed for six Memory packages under the race detector, 27 combined CLI assertions, and 20 current-mainline compatibility assertions. All nine original commits across the three accepted contributions and all six reviewed follow-up fixes are preserved as ancestors. Tests use scratch stores and do not call a paid provider.