Stabilize retraction invariant: read + write guards - #20
Merged
Conversation
Two latent substrate bugs shipped in #19 + four guardrails to prevent the class. The retraction contract — "default reads behave as if retracted nodes do not exist; no write path mutates a retracted row" — is now defended by two invariant tests and enforced at every reader the surface map identified. == Latent bugs fixed == - engine/extractor.go findSimilarNode: didn't filter retracted; LLM extraction could merge new content into a retracted memory's URI, silently overwriting it. Same shape as #11. - server/context.go buildContext: didn't check IsRetracted on the relational profile; a retracted profile was still injected into every SessionStart. Same shape as #10. == Verb guardrails == - store/retract.go: refuses node_type='dir' (tombstoning a dir would dark a subtree). - store/retract.go: refuses system-owned URIs via a small whitelist with the rule named in code ("synthesized by the system + participates in invariants beyond user memory"). v1 entry: mem://user/profile/communication. Operator SQL-edit remains the friction-bearing path for these. == Invariants encoded as tests == - server/invariant_test.go (read invariant): seeds a retracted node with marker strings, iterates every default-read API/route, asserts neither reason nor original L0/L1 surfaces. Includes show-with-flag inverse to confirm the explicit inspection path still reveals. - engine/no_resurrection_test.go (write invariant): seeds retracted A, snapshots full row, runs findSimilarNode + extractMemories + Remember paths against semantically-similar input, asserts every column on A is byte-identical post-call. Resurrection through any column (including metadata like last_access, access_count, relevance) fails the test. == SPA defensive awareness == The /api/tree default path filters retracted at the store layer, so the SPA never received retracted nodes — meaning the SPA was *accidentally correct*. This commit makes correctness intentional: TreeNode/SearchResult/ProfileNode gain an optional `retracted` field, and MemoryCard renders a [retracted] marker plus suppresses content if the field arrives true. No filtering changes, no toggle UI — just defense against upstream changes that would otherwise silently break the SPA. == Smoke == server/smoke_test.go (build tag `smoke`, env CONTINUITY_SMOKE_DB) exercises the migration + retract pipeline against a copy of a real DB. Validated manually against operator's 147MB / 193-leaf production DB: migration applies cleanly, retract+context invariant holds. One note for follow-up (not a bug here): TFIDF embedder is rebuilt against the live (post-retraction) corpus; cosine vs older stored vectors becomes incoherent when vocabulary shifts. Affects dedup-against-retracted recall on real data with the TFIDF fallback. Ollama/nomic-embed users unaffected. Worth a separate issue if it bites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The retracted-state rendering ships inert (no current API path delivers retracted nodes to the SPA) and unverified visually. Comment makes the deferral explicit and points at #6 (Playwright harness) where the visual regression coverage will land. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Defends the “retraction invariant” across the stack: default read paths behave as if retracted nodes don’t exist (or are metadata-only), and non-inspection write paths must not mutate retracted rows.
Changes:
- Add write-side guards: block retraction of
dirnodes and of specific system-owned URIs; prevent similarity-based merge targeting of retracted nodes. - Add read-side guards and UI defenses: context injection skips retracted relational profile; UI accepts
retracted?: booleanand suppresses content when set. - Add invariant + regression tests (HTTP/store read invariants, no-resurrection) and a smoke test (tagged) for real DB validation.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/lib/types.ts | Adds retracted?: boolean to UI types and documents defensive rendering expectations. |
| ui/src/components/TreeBrowser.svelte | Threads retracted through to MemoryCard for tree rendering. |
| ui/src/components/SearchPanel.svelte | Threads retracted through to MemoryCard for search rendering. |
| ui/src/components/ProfilePanel.svelte | Threads retracted through to MemoryCard for profile rendering. |
| ui/src/components/MemoryCard.svelte | Renders a retracted marker and suppresses summary/details when retracted. |
| internal/store/retract.go | Adds non-retractable guardrails for system-owned URIs and non-leaf nodes. |
| internal/store/retract_test.go | Adds tests ensuring retract refuses dir nodes and system-owned URIs without mutating rows. |
| internal/server/context.go | Prevents retracted relational profile from being injected into session context. |
| internal/server/invariant_test.go | Adds invariant tests covering store-layer + HTTP default-read behavior and show-with-flag inverse. |
| internal/server/smoke_test.go | Adds -tags smoke test to exercise migration/retract/context/dedup against a real DB copy. |
| internal/engine/extractor.go | Filters retracted nodes out of findSimilarNode to prevent resurrection via merge. |
| internal/engine/no_resurrection_test.go | Adds no-resurrection regression tests ensuring extract/remember paths don’t mutate retracted rows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
All seven findings were "test could pass for the wrong reason" shapes — exactly the class the invariant tests are meant to defend against, so worth fixing before merge. - invariant_test.go: four routes' tests inspected response bodies for marker substrings without asserting HTTP 200. A 4xx/5xx with an error body that lacks the markers would silently pass the leak assertions (false negative on the invariant). Added w.Code == 200 assertions to the four affected sub-tests. - smoke_test.go: victim-pick ordered by relevance ASC (lowest), but buildContext only injects nodes with relevance >= 0.3 and non-empty L0. Picking a low-relevance node meant the absence assertion could pass trivially because the victim never would have been injected. Now filters l0_abstract != '' AND relevance >= 0.3 and orders DESC, so the victim is one buildContext would have surfaced — the absence proves the invariant rather than coincidence. - no_resurrection_test.go: three setup sites ignored errors from NewTFIDFEmbedder, Embed, and SaveVector. A failure in any of those would leave the test running with broken state; the assertion could pass without exercising the dedup/merge paths. Switched to t.Fatal on each so setup failure is loud. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 6, 2026
4 tasks
lazypower
added a commit
that referenced
this pull request
Jun 19, 2026
The in-process tests in db_test.go build a fresh DB straight to v9 via
OpenMemory. They cannot exercise the real user upgrade path: a DB built
at an old schema version, opened by the current binary, migrated
incrementally. This commit closes that gap.
Five tests in internal/store/migration_e2e_test.go, each booting a real
`continuity serve` subprocess against a programmatically-built old DB:
TestMigrationE2E_UpgradeFromV5_PreservesData
Covers the longest upgrade chain (v5 → v6 → v7 → v8 → v9). v5 is
pre-moments, pre-tone, pre-retraction; this test touches BOTH
full-table rebuilds (v6 + v9). Seeds one row per v5-valid category
with distinguishable values, then verifies each is reachable via
/api/memories after the binary boots and migrates.
TestMigrationE2E_UpgradeFromV7_PreservesToneAndMoments
The most likely real-world upgrade today: a DB built before
retraction (PR #20). Pins that the v7 tone column and a v6 moments
row both survive the v8 ALTER and v9 rebuild.
TestMigrationE2E_UpgradeFromV8_PreservesTombstones
The load-bearing test. v9's INSERT SELECT * relies on column-order
parity between source (v8 mem_nodes: 16 + 3 retraction columns
appended via ALTER) and destination (v9 mem_nodes_new: same 19
columns declared in the same order). If a future migration moves
or inserts columns and SELECT * silently misaligns, this catches
it: we seed a tombstoned row with distinct values per retraction
column and verify each survives the rebuild byte-identical.
Sanity-checked during development by deliberately corrupting the
expected tombstone_reason; the assertion fired with the actual
surviving value visible in the failure message.
TestMigrationE2E_FreshInstallReachesV9
Cold-start path: no pre-existing DB, the binary creates one and
migrates straight to v9. Covers the new-user install case.
TestMigrationE2E_IdempotentSecondBoot
Boots twice against the same DB. Pins the through-binary
idempotency invariant — in-process db_test.go covers migrate()
in isolation; this covers the same invariant through engine init,
embedder probe, server listen. Includes a marker row that
survives the second boot, so a destructive re-migration would be
visible.
Helpers:
- buildDBAtVersion(t, dir, target) — programmatically applies
migrations [1..target] to a fresh DB. Reuses the production
migrations slice (this file is in package store) so the helper
cannot drift from production behavior. Drift would silently
invalidate every test in the file.
- seedV5Data / seedV6Moment / seedV7Tone / seedV8Tombstone —
realistic per-version inserts. Each row has distinguishable
column values so a column-misalignment bug surfaces as garbled
data, not just a missing column.
- startSubprocessAgainstDB / fetchMemoryByURI / assertSchemaV9 /
envGet — shared scaffold so per-test bodies stay on the contract.
Reuses internal/testharness (BuildContinuityBinary, HermeticEnv,
StartServeProcess, WaitForReady). CI inherits the existing e2e job
that runs `-tags noembed -run 'E2E|Subprocess'`; the migration tests
match the filter without further config.
Runtime: ~4s for all five tests (~700-900ms each — dominated by the
binary build, amortizable via TestMain if needed later).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Follow-up to #19. Two latent substrate bugs from the original retraction ship + four guardrails to prevent the class. The contract — default reads behave as if retracted nodes do not exist; no write path mutates a retracted row — is now defended by two invariant tests.
Ships
findSimilarNoderetraction filter (engine/extractor.go) — closed the silent-resurrection path through LLM extraction.buildContextrelational profile guard (server/context.go) — closed the silent-injection path on SessionStart.node_type='dir'and system-owned URIs (whitelist with the rule named in code; v1 entry:mem://user/profile/communication).retracted?: booleanon TreeNode/SearchResult/ProfileNode; MemoryCard renders a marker and suppresses content if the field is true. The SPA was accidentally correct (filtering happened upstream); now it's intentionally correct.-tags smoke) — exercises migration + retract against a copy of a real DB. Validated manually against 147MB / 193-leaf production DB.Test plan
go test ./...— green; both invariant tests pass with the fixes, fail without them (TDD-verified).go vet ./...— clean.npm run build— clean.Note for follow-up (not a bug here)
TFIDF embedder rebuilds against the live corpus; cosine vs older stored vectors becomes incoherent when vocabulary shifts. Affects dedup-against-retracted recall on real data with the TFIDF fallback. Ollama/nomic-embed users unaffected. Separate issue if it bites.
🤖 Generated with Claude Code