Memory accountability: retract verb (tombstone + supersession)#19
Memory accountability: retract verb (tombstone + supersession)#19
Conversation
Memory is not immutable; it is accountable. Implements the design from the RFC on issue #12 after three review passes (work-Claude pass-1 gap-find, pass-2 dedup side-channel, pass-3 v1 scope cuts; Fi's accountability framing and final read). Schema (migration #8): adds tombstoned_at, tombstone_reason, superseded_by columns to mem_nodes. Retraction filtering happens in existing read paths; no new indexes for v1. Single primitive — Retraction — with two surface shapes: - Without --superseded-by: tombstone (preserved as marker, hidden from default reads, reason held behind --include-retracted). - With --superseded-by: supersession (same marker plus a forward link, so readers can follow how understanding evolved). Default reads exclude retracted nodes from search/find/tree/show/context- injection. Inspection paths opt in with --include-retracted (CLI) or include_retracted=true (HTTP). Reason content and original L0/L1/L2 are *absent* from the response when not opted-in (not empty, not null, not redacted) — so consumers don't have to disambiguate "no content" from "hidden by contract." Dedup-against-retracted: retracted nodes still participate in similarity matching, or retraction-because-PII is silently broken — the next session writes similar content, hits no match, re-introduces the leak. When a write matches a retracted node, the engine returns a RetractedMatchError with URIs only; reasons stay sequestered behind --include-retracted. Operator- facing dedup logs use SHA-256 URI hashes (16 hex), not raw URIs, since URIs are correlatable to reasons by anyone with --include-retracted access. The agent passes --acknowledge-retracted to bypass the gate after fetching the reason deliberately. Override events are NOT recorded — documented as a deliberate v1 choice; the agent's reasoning lives in conversation context. CLI: `continuity retract <uri> --reason "..." [--superseded-by <uri>]`. HTTP: POST /api/memories/retract. CLI exit code 2 on the dedup gate so scripted callers can branch. Threat model: pre-retraction, an attacker had to touch SQLite or the operator to corrupt memory; post-retraction, social-engineering the in-session agent suffices. The architectural friction was always thin (SQLite is unlocked); this trades a small amount of that thinness for the agent's ability to curate its own substrate. Operator surface stays discouraged-not-enforced — same trust contract that already governs the SQLite store. Test surface: 22 functions / 30 test runs across three new files (internal/store/retract_test.go, internal/engine/retract_test.go, internal/cli/retract_integration_test.go). Coverage focuses on the contract — absence-of-leakage assertions are explicit on the dedup side-channel paths, multi-hop supersession chain integrity is verified, and the PII scenario from the spec runs end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements “memory accountability” by adding a retract primitive (tombstone + optional supersession link) across the storage layer, engine, HTTP API, and CLI, with default read-path filtering and explicit opt-in to inspect retracted content.
Changes:
- Adds schema migration v8 and store-layer support for retraction fields (
tombstoned_at,tombstone_reason,superseded_by) plus read APIs that include/exclude retracted nodes. - Introduces engine + server support for retraction, including a dedup-against-retracted gate and
POST /api/memories/retract. - Adds CLI verb
continuity retract, plus--include-retracted/--acknowledge-retractedflags and supporting documentation/tests.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/store/retract.go | New DB methods for retracting nodes and listing children/leaves including retracted. |
| internal/store/retract_test.go | Store-layer tests for retraction behavior and filtering. |
| internal/store/nodes.go | Extends MemNode with retraction fields; updates queries and scan logic to include new columns and filter live nodes by default. |
| internal/store/migrations.go | Adds migration v8 to extend mem_nodes with retraction columns. |
| internal/store/db_test.go | Bumps expected schema version to 8. |
| internal/engine/retract.go | Implements engine-level retract + dedup-against-retracted primitives and error type. |
| internal/engine/retract_test.go | Adds unit tests for hashing, dedup-against-retracted, URI validation, and override behavior. |
| internal/engine/engine.go | Adds dedup-against-retracted gate + acknowledge override on Remember. |
| internal/engine/search.go | Excludes retracted nodes from default Find search scoring. |
| internal/server/server.go | Registers new POST /api/memories/retract route. |
| internal/server/routes.go | Implements retract handler; updates show/tree/read responses for retraction contract + include flag; adds remember conflict response for retracted matches. |
| internal/cli/retract.go | Adds continuity retract command (HTTP-backed). |
| internal/cli/retract_integration_test.go | Adds end-to-end tests for retract/show behavior and PII loop. |
| internal/cli/remember.go | Adds --acknowledge-retracted flag and structured handling of retracted-match conflicts. |
| internal/cli/show.go | Adds --include-retracted and preserves “absence-not-empty” contract in JSON output. |
| internal/cli/stubs.go | Adds tree --include-retracted and marks retracted nodes in output. |
| internal/cli/root.go | Registers the new retract command. |
| internal/cli/init.go | Updates onboarding text to mention retract/accountability. |
| README.md | Documents retract semantics and API parameters/routes. |
| CLAUDE.md | Adds retract/accountability guidance for agent usage. |
Comments suppressed due to low confidence (1)
internal/server/routes.go:637
- When listing children, directories compute
ChildrenviaCountChildren, which includes retracted nodes even wheninclude_retractedis false. This makes the returned tree inconsistent with the filtering applied to the listing. Use a live-only count wheninclude_retractedis false (or add aCountChildrenIncludingRetractedvariant and pick based on the flag).
if c.NodeType == "dir" {
count, _ := s.db.CountChildren(c.URI)
tn.Children = count
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| n.LastAccess = &lastAccess.Int64 | ||
| } | ||
| if tombstonedAt.Valid { | ||
| n.TombstonedAt = &tombstonedAt.Int64 |
There was a problem hiding this comment.
Pulled on this carefully — I think this is a false positive specific to the var x inside the loop body (vs. the classic for i := range loop-variable pattern fixed in Go 1.22).
Each iteration's var lastAccess, tombstonedAt sql.NullInt64 declaration creates a fresh variable. Taking &lastAccess.Int64 causes the compiler's escape analysis to allocate it on the heap independently per iteration; the pointers in successive nodes are distinct heap addresses, not aliases.
Verified with a focused regression test (TestScanNodes_NoPointerAliasing in 68c30e3). It seeds three retracted nodes, reads them via FindByCategoryIncludingRetracted (which uses scanNodes), and asserts every TombstonedAt pointer is a distinct address — passing test confirms each iteration gets its own allocation. Test would fail loudly if the aliasing this comment describes were happening.
If you want me to refactor anyway (e.g., v := lastAccess.Int64; n.LastAccess = &v for stylistic clarity even though the existing form is correct), happy to — but flagging that the bug as described isn't present.
- Self-supersession: RetractNode now rejects supersededBy == uri before any write.
Adds TestRetractNode_RejectsSelfSupersession to verify no DB mutation on the
bad path.
- CountChildren in default tree displays counted retracted nodes, contradicting
what the listing actually returned. Adds CountLiveChildren and routes default
paths to it (CLI tree x2, HTTP tree x2). --include-retracted toggles back to
the all-children count for inspection.
- Test robustness: three retract tests followed `if err == nil { t.Error }; ...
err.Error()`, which would panic if the error path failed to fire. Switched to
t.Fatal so the test bails before dereferencing.
- CLI remember was silently swallowing json.Unmarshal errors on the success
path; now distinguishes parse-on-error (fall through to transport error) from
parse-on-success (surface the parse error rather than printing empty fields),
mirroring the pattern in show.go.
- Adds TestScanNodes_NoPointerAliasing as a guard against the aliasing class of
bug Copilot flagged on scanNodes; passing test confirms the var-inside-loop
declaration escapes per-iteration as Go's escape analysis intends.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #12. Implements the design from the RFC.
Ships
tombstoned_at,tombstone_reason,superseded_bycolumns onmem_nodes.continuity retract <uri> --reason "..." [--superseded-by <uri>](CLI) andPOST /api/memories/retract(HTTP).search/find/tree/show/ context-injection.--include-retracted(or?include_retracted=true) opts in.RetractedMatchError);--acknowledge-retractedbypass.Test plan
go test ./...— full suite green; 22 new test functions acrossinternal/{store,engine,cli}/retract*_test.go, with absence-of-leakage assertions on dedup paths and end-to-end PII scenario.go vet ./...,go build ./...— clean.retract, confirmshow --include-retractedtoggle and dedup gate fires +--acknowledge-retractedbypasses.Migration
Migration #8 adds nullable columns to existing DBs — no data migration needed. First server restart applies it.