feat(context): add ErasureCoordinator for cross-store entity erasure - #1027
feat(context): add ErasureCoordinator for cross-store entity erasure#1027pravit-amp wants to merge 4 commits into
Conversation
purge_node() is graph-scope by design (semantica-agi#957), so an entity removed from the graph can survive verbatim as an AgentMemory item and as an embedding. The changelog names GDPR Article 17 as purge's motivation, and an Article 17 erasure the vector store can still answer queries from is not an erasure -- it is worse than none, because purge_node() returns True and writes a tombstone attesting the content is gone. ErasureCoordinator composes the existing public APIs to drive the cascade and returns an ErasureReceipt recording what each store reported. Nothing in context_graph.py or agent_memory.py changes behaviorally; ContextGraph keeps its documented graph-scope contract instead of acquiring references that would invert the dependency. Honest partial reporting is the point. Stores report erased / not_found / not_configured / unsupported / failed, and complete is False when any store reports unsupported or failed. FAISS, Milvus and Weaviate expose no delete at all, so erasure genuinely cannot be completed on them today -- the receipt says so rather than reporting a success it did not achieve. Erasure runs outward-in (vectors, memory, graph). The tombstone is the durable attestation, so writing it first would let a crash mid-cascade leave a record claiming more than happened; erasing the graph last leaves a partial failure recoverable and honest. The memory sweep pages until dry and re-queries afterwards rather than trusting one find_by_entity() call, whose limit=10 default silently truncates the very check a caller uses to decide the erasure is done. Unsupported vector backends are detected by probing the wrapped backend, since the VectorStore facade declares delete_vectors() for every backend and only raises NotImplementedError once called. 27 tests against real ContextGraph/AgentMemory instances, including the 25-items-on-one-entity regression that fails against a naive single-call sweep. Full tests/context/ suite: 596 passed. Closes semantica-agi#1018
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoAdd ErasureCoordinator for cross-store entity erasure receipts
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
The vector-leg tests asserted the three backend shapes the coordinator expects -- delete_vectors / delete / neither -- against fakes, which is worth exactly as much as the assumption that a real store looks like one of them. VectorStore(backend="inmemory") runs without external services, so it can hold that assumption to account. Adds the end-to-end case the receipt actually attests to: a real ContextGraph, AgentMemory and VectorStore, where the embedding is written by AgentMemory.store() and has to be gone afterwards. That exercises the memory leg's own delete_memory() vector cascade rather than the coordinator's model of it. The real backend also pins a limit worth knowing before trusting the receipt: it pops the ids and returns True whether or not they were there, and no backend offers a portable existence check, so `erased` on the vectors leg means the store accepted the delete for the ids given -- not that embeddings were really removed. The memory leg re-queries to confirm and so is the stronger claim. Documented on STATUS_ERASED, _erase_vectors(), and both status tables. tests/context/: 599 passed.
Code Review by Qodo
1.
|
|
Pushed two follow-ups ( The vector-leg tests were only as good as my guess about the backendsThe original tests asserted the three backend shapes the coordinator expects —
The real backend surfaced a caveat the fakes structurally could not
So For a feature whose entire purpose is not overstating what was erased, that asymmetry shouldn't be buried. It's now documented on Docs
Worth flagging separately:
|
Timestamp drift (high). erase_entity() resolved erased_at up front but passed
the caller's original `at` down to purge_node(), so on the default at=None
path the coordinator and the graph each took their own now() and the receipt
attested to a different instant than the tombstone it points at -- breaking
the invariant this module states most loudly. The resolved value is now what
the graph receives. The existing test passed only because it supplied an
explicit `at`, which hides the drift; the regression test covers at=None,
which is what callers actually use.
Backend delete results. The vectors leg treated anything other than the
literal False as success, but no in-repo backend returns a bool -- Qdrant
returns {"status": <UpdateStatus>} and Pinecone {"deleted": True}, so every
dict read as success and the backend's own account of the delete was thrown
away. Results are now interpreted by shape and the payload is kept in the
receipt as backend_result, stringified so it stays JSON-serializable as an
audit record. Bool markers match by identity so a 0 count isn't read as
False; string markers match as substrings so an enum rendering as
"UpdateStatus.FAILED" isn't read as success.
Falsey vector store. The "at least one store" guard used `not vector_store`,
rejecting a valid store whose __bool__/__len__ makes an empty instance falsey
and then reporting vector_store=None when an object had been passed. It now
separates None (absent) from False (deliberately disabled) from provided, and
echoes what it received.
`at` annotations. Widened to int/float, matching the ContextGraph normalizer
they delegate to, so the coordinator stops advertising less than the API it
wraps.
tests/context/: 608 passed.
Description
ContextGraph.purge_node()is graph-scope by design (#957), and its changelog entry says so explicitly. That leaves the feature's stated motivation unfinished: the changelog names GDPR Article 17 as the reason purge exists, and an Article 17 erasure that removes the node while the same content survives verbatim as anAgentMemoryitem and as an embedding is not an erasure. It's worse than not offering one, becausepurge_node()returnsTrueand writes a tombstone attesting the content is gone.This adds
ErasureCoordinator, which owns references to the stores, drives erasure across all of them, and returns an auditableErasureReceiptof what was reached and what was not.ContextGraphkeeps its graph-scope contract unchanged — the coordinator composes the existing public APIs rather than modifying them. Nothing incontext_graph.pyoragent_memory.pychanges behaviorally (the only edit tocontext_graph.pyis a docstring pointer), which also keeps this off a file with large diffs pending across #852, #967 and #921.The property that matters is honest partial reporting. Three vector backends cannot delete at all, so the coordinator must never report a success it didn't achieve. A receipt reading
graph: erased, memory: 14 erased, vectors: unsupported on faissis actionable; a silentTrueis a compliance liability.Scope note
Per @Sameer6305's guidance on #1018, this is the standalone follow-up covering the coordinator itself. #1024 by @yzxcj797 separately fixes the
find_by_entity()truncation this issue also cited; the two do not overlap and this PR does not touchagent_memory.py. The memory sweep here is written to be correct either way — see "Deviations from the plan" below.Type of Change
Related Issues
Closes #1018
Builds on #955 / #957 (retraction and purge for
ContextGraph), whose changelog entry documents this exact gap as out of scope. Adjacent to #1024, which fixes thefind_by_entity()limit trap independently.Changes Made
semantica/context/erasure.py, exportingErasureCoordinatorandErasureReceiptfromsemantica.context.erase_entity(entity_id, reason=..., at=..., vector_ids=...)→ErasureReceipterase_entities([...], reason=...)→ one receipt per entity, in order, so one entity's failure doesn't stop the resterased,not_found,not_configured(store never bound — normal),unsupported(store cannot delete at all; retrying won't help),failed.receipt.completeisFalsewhen any store reportsunsupported/failed, andreceipt.incomplete_storesnames them. Noteerasedis a weaker claim on the vectors leg than on the memory leg — see Additional Notes.failed(with the exception type) and the remaining legs still run, rather than aborting into a half-erased state with no record of which half.limit=that's only correct until someone exceeds it, then re-queries once afterwards and reportsfailedwith the residual count if anything survived. It also stops rather than spinning ifbatch_deletereports no progress on a non-empty page.unsupportedvector backends are detected by probing, not by calling and catching — see below. Backends are reached under either supported name,delete_vectors(ids)(pinecone/qdrant) ordelete(ids)(pgvector/sqlite-vec), and the receipt records which was used.vector_storedefaults tomemory.vector_store, stays overridable for deployments binding a store the memory doesn't own, and acceptsFalseto disable the leg. Vectors owned by memory items are removed by the memory leg's owndelete_memory()cascade; the explicit vector leg covers entity-keyed embeddings written by something other thanAgentMemory.purge_node()'s docstring now points at the coordinator, so callers reading the graph-scope caveat find the thing that completes the workflow; new sections indocs/reference/context.mdandsemantica/context/context_usage.md; CHANGELOG entry.Deviations from the plan I posted on #1018
Three things the code turned up that are worth a reviewer's attention:
find_by_entity()returns items keyedmemory_id, notid. The cascade sketched in the issue body (memory.delete_memory(m["id"])) wouldKeyError. Handled, and a memory item carrying no identifier reportsfailedrather than looking erased.unsupportedprobe has to look at the wrapped backend, not the facade.delete_vectors()is declared on theVectorStorefacade (vector_store.py:786) for every backend and only raisesNotImplementedErroronce called, so probing the facade alone cannot tell a deletable backend from a delete-less one. Probing rather than calling-and-catching also keeps a missing method distinguishable from anAttributeErrorraised inside a working one — exactly where guessing wrong produces a false clean bill of health.NotImplementedErrorat call time is still caught and reported asunsupported; a store returningFalseis reported asfailed.erased_atis normalized throughContextGraph's own temporal normalizer. Without it the receipt read...T00:00:00Zwhile the tombstone for the same erasure read...T00:00:00— an audit record contradicting the tombstone it attests to. Normalizing up front also rejects an unparseableatbefore any store is touched, instead of half way through the cascade.I also kept the loop-until-dry memory sweep that my #1018 amendment said #1024 would make unnecessary. #1024 hasn't merged, so
limit=10is still the default onmain; the loop is correct under both the old and new defaults, so nothing here needs to change when #1024 lands.Answers to the open questions from #1018
to_dict()is serialization-ready and safe to store, but I'd rather not guess at a storage contract. Easy follow-up if maintainers want it durable.erase_entityaccept edges? No. Memory and vectors are keyed on entities, so an edge-scoped erasure has only one leg to run; callers should usepurge_edge()directly.semantica/context/erasure.py, as it reads as context-layer today. Happy to move it to a top-levelsemantica/erasure/if the intent is for it to eventually reach graph stores and triplet stores.Testing
python -m build)New
tests/context/test_erasure_coordinator.py— 30 tests, run against realContextGraphandAgentMemoryinstances rather than mocks. The bug this feature exists to prevent lives in the interaction between them, so mocking that interaction away would test nothing. Most vector stores are fakes, because the point of those tests is backend shape — but a fake is worth exactly as much as the assumption that a real store looks like it, so three tests run against a realVectorStore(backend="inmemory")as well.find_by_entity()once with the default fails this test.delete_vectors/delete/ neither →erased/erased/unsupported— plus the facade-over-a-delete-less-backend shape, and a store returningFalse→failed.failed, the graph leg still runs, and nothing raises through to the caller.batch_deletemaking no progress, items with no identifier — allfailed, never silentlyerased.erase_entity()reports nothing left to do rather than raising, matching howpurge_node()already returnsFalseon a repeat.complete/incomplete_stores,to_dict()copying rather than aliasing the store results, anderased_atagreeing with the tombstone'spurged_at.VectorStore: the ids given are really removed (count()2 → 0); the full three-store cascade with a realContextGraph+AgentMemory+VectorStore, where the embedding is written byAgentMemory.store()and has to be gone afterwards — that exercises the memory leg's owndelete_memory()vector cascade rather than the coordinator's model of it; and theerased-means-accepted caveat below.Test Commands
Documentation
docs/reference/context.mdgains anErasureCoordinatorsection (constructor parameters, methods, the status table, and the receipt shape) plus a row in the Exported Classes table;semantica/context/context_usage.mdgains a walkthrough;purge_node()'s docstring points at the coordinator. No cookbook notebook — happy to add one if that's wanted.Breaking Changes
Breaking Changes: No
New module, purely additive. No existing signature, return type, or default changes; the only edit to an existing Python file besides the package
__init__is a docstring addition onpurge_node().Checklist
black,isort --profile black, andflake8 --max-line-length=88 --extend-ignore=E203,W503are all clean on the new files. The new test module emits zero warnings. Annotations usetypinggenerics rather than thecollections.abcsubscripting that needs 3.9, per the declared 3.8 floor.Additional Notes
erasedis a weaker claim on the vectors leg than on the memory leg, and the receipt should be read with that in mind. The memory leg re-queries after its sweep and only claimserasedonce nothing referencing the entity comes back. The vectors leg cannot do the same: backends delete by id and report success whether or not the id was there —VectorStore(backend="inmemory").delete_vectors(["never-embedded"])returnsTruewith the count unchanged — and there is no portable "does this id exist" check to confirm against. Sovectors: {"status": "erased", "vector_ids": 1}means the store accepted the delete for one id, not one embedding was really removed. Documented onSTATUS_ERASEDand_erase_vectors(), and pinned by a test so it can't quietly drift into a stronger-sounding claim. I'd rather state this in the open than have an operator read the receipt as more than it is — but if reviewers would prefer the leg attempt acount()-based confirmation where the backend exposes one, I'm happy to add it.Known limitation, unchanged by this PR and worth its own issue: erasure still cannot be completed on FAISS, Milvus, or Weaviate —
faiss_store.py,milvus_store.py, andweaviate_store.pyexpose no delete method at all, and FAISS in particular can't remove from a flat index without a rebuild.delete_vectors()is declared on theVectorStorefacade but implemented across the backend set under at least three different names. That's the same multi-backend divergence that makes a change tested only against the default pass locally and break everywhere else. The coordinator ships reportingunsupportedfor those, and starts reportingerasedonce the backends gain a delete — no API change needed here. I'm happy to open that issue separately.Reviewers may also want to weigh in on whether
completeshould treatnot_configuredas complete. It does today: a store that was never bound leaves no residue, and onlyunsupported/failedmean data survived. The alternative reading — that an unconfigured vector store means "you don't know whether embeddings exist elsewhere" — is defensible, but it would make every graph-only erasure report incomplete, which I think dulls the signal.