feat(memory): add a supersedes edge type and demote superseded insights - #98
Open
audreyt wants to merge 6 commits into
Open
feat(memory): add a supersedes edge type and demote superseded insights#98audreyt wants to merge 6 commits into
audreyt wants to merge 6 commits into
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.
There was a problem hiding this comment.
Pull request overview
Adds support for a new supersedes edge type to represent “this insight replaces that one”, including schema/migration support and recall-time score demotion so corrected insights outrank the content they supersede.
Changes:
- Extend the
edges.edge_typeCHECK constraint to includesupersedes, with an on-open migration that rebuildsedgesfor legacy stores. - Add store/query support for identifying superseded insights (
GetSupersededIDs). - Demote superseded insights during intent-aware recall and surface a
Supersededflag through both internal and compact CLI recall result shapes; add regression tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/memory/store/store_test.go | Adds migration tests for idempotence and legacy-store upgrade behavior. |
| internal/memory/store/edge.go | Adds a helper to fetch IDs that are targets of supersedes edges. |
| internal/memory/store/db.go | Extends schema CHECK and adds a migration to rebuild edges to admit supersedes. |
| internal/memory/search/recall.go | Demotes superseded insights during scoring and exposes a Superseded field in results. |
| internal/memory/search/integration_test.go | Adds a regression test ensuring corrections outrank superseded stale content. |
| internal/memory/model/edge.go | Introduces the EdgeSupersedes constant and updates valid edge types. |
| cmd/memory/recall.go | Preserves Superseded in the compact recall JSON output. |
| cmd/memory/link.go | Updates CLI help text to include supersedes as a link type. |
Suppressed comments (1)
internal/memory/store/store_test.go:749
- The probe-leak assertion currently matches any source_id beginning with "". That can produce false failures if a legitimate insight/edge id starts with "". Since the sentinel prefix here is "_probe", the test should narrow the check to that prefix (and ideally check both source_id and target_id).
var probes int
if err := reopened.conn.QueryRow(
`SELECT COUNT(*) FROM edges WHERE source_id LIKE '\_\_%' ESCAPE '\'`).Scan(&probes); err != nil {
t.Fatalf("count probes: %v", err)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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]".
Contributor
Author
|
CI on c6ce4e7 is red on Evidence it is unrelated:
The failure message is A re-run should be green. |
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
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.
Adds a fifth
edges.edge_typevalue,supersedes, and uses it to demote superseded insights at recall time.This is the next slice after #96/#97. Those two landed the narrative-edge rebuild and the rolled-back schema probe; this PR reuses that probe shape to widen the CHECK.
What changes
CHECK(edge_type IN (…,'supersedes'))on new databases, plusmigrateAddSupersedesEdgeTypefor existing ones.PRAGMA foreign_keys=OFF, insert'__probe_'||hex(randomblob(8))inside a transaction that is always rolled back, then rebuild. A leftover sentinel cannot strand the store, and a fixed id cannot collide with a caller-supplied insight id.supersedesedge.Tests
TestMigrateAddSupersedesEdgeType_IsIdempotent— second open must not rebuild (rootpage unchanged, zero__probe_%rows).TestMigrateAddSupersedesEdgeType_UpgradesLegacySchema— on-disk four-type CHECK is rewritten, a secondOpenrebuilds:supersedesis then admitted, the pre-existing semantic edge is copied, no probe row survives. Fails without the rebuild onsupersedes edge type must be admitted after migration.TestIntentAwareRecall_SupersededInsightIsDemoted.go test ./internal/memory/store ./internal/memory/search -count=1green on this head (e99c422e).A follow-up branch (
feat/supersedes-citations) records supersession stated in insight text and backfills it; not in this PR.