feat(lineage): purge_content — content-only erasure that keeps lineage chains - #219
Merged
Conversation
…p skeleton, atomic op=purge
- test_clear_user_data_cross_user_chain_purges_other_users_survivor: add assertion that C's content == "" (body blanked) to prove the row was actually purged, not merely kept un-deleted - clear_user_data docstring: "optimised" -> "optimized" (American spelling)
…se/Postgres path) + shared partition helper
…er_data
The GDPR erasure path in BaseStorage.clear_user_data enumerated
user profiles and user_playbooks with:
status_filter=[None, Status.ARCHIVED, Status.PENDING]
This excluded Status.SUPERSEDED and Status.MERGED (tombstone rows),
so a user whose only rows had been soft-deleted via lineage operations
was never fed to _partition_purge_vs_delete and their rows persisted
after erasure — a GDPR regression vs the old delete_all_profiles_for_user
(which had no filter and removed all rows).
Fix: build _all_statuses = [None, ARCHIVED, PENDING, ARCHIVE_IN_PROGRESS,
SUPERSEDED, MERGED] and use it for both the get_user_playbooks and
get_user_profile enumeration calls. This ensures every row the user
owns, regardless of lifecycle state, is reached and either purged or
hard-deleted by the erasure path.
Regression test: test_clear_user_data_tombstone_only_user seeds a user
whose sole profile is a tombstone (SUPERSEDED) and asserts that
clear_user_data purges it (leaves no intact content).
…ent Task 6) Audit finding: ZERO production call sites currently consume resolve_current's return value or dereference the resolved record's content. The offline-RL / attribution path (playbook_generation_service.py) references resolve_current only in a docstring — no skip-guard is needed anywhere today. Changes (no consumer needed a skip): - RecordRef.is_purged docstring: explicit consumer contract — any future consumer dereferencing resolved content MUST skip when is_purged=True. - resolve_current docstring: Consumer contract paragraph added at the top so the obligation is visible before writing a new consumer. - test_purge_content.py: dedicated guard test test_resolve_current_returns_is_purged_for_purged_survivor — asserts is_purged=False before purge and is_purged=True after, locking in the signal any future consumer will rely on.
|
Warning Review limit reached
More reviews will be available in 20 minutes. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a lineage-aware erasure pathway to ChangesLineage-aware content purge
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BaseStorage
participant SQLiteLineageMixin
participant SQLiteDB
participant SearchIndex
rect rgba(100, 149, 237, 0.5)
Note over Caller,BaseStorage: clear_user_data
Caller->>BaseStorage: clear_user_data(user_id)
BaseStorage->>BaseStorage: Snapshot profile/user_playbook ids (incl. tombstones)
BaseStorage->>BaseStorage: _partition_purge_vs_delete(profiles)
BaseStorage->>BaseStorage: _partition_purge_vs_delete(user_playbooks)
end
rect rgba(255, 165, 0, 0.5)
Note over BaseStorage,SQLiteDB: Phase — hard-delete
BaseStorage->>SQLiteDB: DELETE interactions, requests, delete-eligible rows
SQLiteDB-->>BaseStorage: commit
end
rect rgba(144, 238, 144, 0.5)
Note over BaseStorage,SearchIndex: Phase — content purge (outside lock)
loop each purge-eligible profile/user_playbook
BaseStorage->>SQLiteLineageMixin: purge_content(entity_type, entity_id)
SQLiteLineageMixin->>SQLiteDB: UPDATE blank content WHERE content != ''
SQLiteLineageMixin->>SQLiteDB: INSERT OR IGNORE op=purge event (request_id=purge_entity_id)
SQLiteLineageMixin->>SQLiteDB: COMMIT
SQLiteLineageMixin->>SearchIndex: _purge_search_indexes (delete FTS + vec)
end
end
BaseStorage-->>Caller: {deleted_profiles, purged_profiles, deleted_user_playbooks, purged_user_playbooks, ...}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
yilu331
force-pushed
the
feat/lineage-purge-content
branch
from
June 24, 2026 00:22
2c82426 to
90aa49d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
reflexio/server/services/storage/sqlite_storage/_base.py (1)
1786-1902: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRoll back the phase-1/4 transaction on any pre-commit failure.
If any DML statement or Line 1902’s commit fails, the connection can retain a partial transaction that a later operation may accidentally commit. Mirror the retention delete path and rollback before re-raising.
Proposed transaction guard
with self._lock: + try: # ------------------------------------------------------------------ # Phase 1: snapshot all user-scoped ids before any mutations. # ------------------------------------------------------------------ interaction_ids = [ r["interaction_id"] @@ # purge_content issues its own conn.commit() and nesting it here # would prematurely flush the still-pending deletes. self.conn.commit() + except Exception: + self.conn.rollback() + raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/storage/sqlite_storage/_base.py` around lines 1786 - 1902, The database transaction in the method body needs error handling to prevent partial state retention. Wrap the entire DML operation block (from after the lock acquisition through the self.conn.commit() call at the end) in a try-except block. If any exception occurs during the phases of snapshotting, FTS/vector cleanup, or hard deletes, catch the exception, call self.conn.rollback() to safely revert the partial transaction, and then re-raise the exception. This ensures that failed operations don't leave the connection in an inconsistent state.
🧹 Nitpick comments (1)
reflexio/server/services/storage/storage_base/__init__.py (1)
53-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFail fast on unsupported entity types in
_is_lineage_tombstone.Line 76 silently returns
Falsefor unsupported types, which makes_partition_purge_vs_deletetreat them as hard-delete eligible. RaisingValueErrorhere prevents accidental misrouting in future call sites.Suggested change
if entity_type == "user_playbook": row = self.get_user_playbook_by_id(int(entity_id), include_tombstones=True) if row is None: return False return bool(row.merged_into or row.superseded_by) - return False + raise ValueError(f"unsupported entity_type for purge partitioning: {entity_type!r}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/server/services/storage/storage_base/__init__.py` around lines 53 - 77, The `_is_lineage_tombstone` method currently returns False silently for unsupported entity_type values, which can cause downstream issues in `_partition_purge_vs_delete`. Instead of the final `return False` statement, raise a ValueError to fail fast when an unsupported entity type is passed to the method. This will prevent accidental misrouting of unsupported types as hard-delete eligible cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reflexio/server/services/storage/sqlite_storage/_base.py`:
- Around line 1882-1888: The hard-delete branch is missing cleanup of
source-window dependencies before deleting user_playbooks. Before executing the
DELETE statement from user_playbooks table using the delete_upb_ids, call
_delete_source_windows_for_user_playbook_ids with delete_upb_ids as the argument
to clean up any orphaned agent_playbook_source_user_playbooks rows and ensure
foreign key constraints are satisfied, following the same pattern used in the
retention path.
- Around line 1902-1919: The purge_content method calls for profiles and
user_playbooks (lines 1908-1911) are executed after the lock is released,
leaving sensitive data exposed. Move the purge_content calls before exiting the
outer critical section (before line 1902's commit and lock release) to keep the
erase lock held throughout the purge phase. Additionally, instead of using
len(purge_profile_ids) and len(purge_upb_ids) for the return counts, capture and
count the actual successful returns from each purge_content call to accurately
reflect what was purged.
- Around line 1840-1896: The DELETE statements using IN clauses with large lists
of IDs can exceed SQLite's variable limit for users with many interactions,
profiles, or playbooks. Apply the existing chunked-delete pattern to all
IN-based DELETE operations in this section: convert the single DELETE statements
for interactions_fts, user_playbooks_fts, profiles_fts, and their corresponding
vec and main tables to process IDs in smaller batches instead of all at once.
For the user_playbooks and profiles main table deletions, accumulate the
rowcount values from each chunk iteration into upb_deleted_count and
profile_deleted_count respectively to get the final deleted counts.
In `@reflexio/server/services/storage/sqlite_storage/_lineage.py`:
- Around line 105-109: The purge_content method's documentation advertises
agent_playbook as a supported entity type, but the _PURGE_SQL dictionary does
not include an entry for it, causing the method to raise ValueError at runtime
when called with agent_playbook. Either add the agent_playbook SQL definition to
the _PURGE_SQL dictionary and implement the corresponding purge logic, or update
the documentation to remove agent_playbook from the list of supported entity
types. Since the comment indicates agent_playbook purge is not yet required,
remove it from the advertised supported types in the docstring.
- Around line 88-104: The WHERE clause guard in both _PROFILE_PURGE_SQL and
_USER_PLAYBOOK_PURGE_SQL is checking only if content is non-empty, but this
allows rows with empty content but populated sensitive columns (like user_id) to
skip the purge operation entirely. Modify the WHERE clause guard conditions in
both SQL statements to check whether any of the sensitive/PII columns actually
contain data that needs purging, rather than checking only the content field.
This ensures that all rows with any populated sensitive columns are properly
sanitized and audited, regardless of their content field state.
In `@tests/server/services/storage/sqlite_storage/test_purge_content.py`:
- Around line 326-327: The assertion after the resolve_current call for profile
"A" at line 326-327 is incomplete for validating the cross-user survivor purge
test case. Currently, it only checks ref.id == "C", but it should also assert
that ref.is_purged is True to fully lock the resolve contract and prevent silent
regressions. Add an additional check in the same assertion statement to verify
that ref.is_purged evaluates to True alongside the existing ref.id check.
- Around line 282-290: The test currently permits both purging (content='') and
hard-deletion (a_row is None) as acceptable outcomes for tombstone profile A,
but the storage contract requires purged tombstones to be retained as skeletons
with blanked content, not hard-deleted. Remove the conditional check for a_row
is not None and the subsequent comment allowing hard-deletion as acceptable.
Instead, directly assert that a_row is not None and that a_row["content"] equals
an empty string, ensuring the test enforces that lineage rows are purged
(content-blanked) and retained, not hard-deleted.
---
Outside diff comments:
In `@reflexio/server/services/storage/sqlite_storage/_base.py`:
- Around line 1786-1902: The database transaction in the method body needs error
handling to prevent partial state retention. Wrap the entire DML operation block
(from after the lock acquisition through the self.conn.commit() call at the end)
in a try-except block. If any exception occurs during the phases of
snapshotting, FTS/vector cleanup, or hard deletes, catch the exception, call
self.conn.rollback() to safely revert the partial transaction, and then re-raise
the exception. This ensures that failed operations don't leave the connection in
an inconsistent state.
---
Nitpick comments:
In `@reflexio/server/services/storage/storage_base/__init__.py`:
- Around line 53-77: The `_is_lineage_tombstone` method currently returns False
silently for unsupported entity_type values, which can cause downstream issues
in `_partition_purge_vs_delete`. Instead of the final `return False` statement,
raise a ValueError to fail fast when an unsupported entity type is passed to the
method. This will prevent accidental misrouting of unsupported types as
hard-delete eligible cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c69adea-6ad8-4ea6-a60d-e769e882cd34
📒 Files selected for processing (8)
.superpowers/sdd/task-3-report.mdreflexio/models/api_schema/domain/entities.pyreflexio/server/services/lineage/resolve.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/_lineage.pyreflexio/server/services/storage/storage_base/__init__.pyreflexio/server/services/storage/storage_base/_lineage.pytests/server/services/storage/sqlite_storage/test_purge_content.py
…_data #4 (CRITICAL): remove AND content != '' guard from _PURGE_SQL — rows with content='' but other PII (user_id, embedding, tags) populated were skipped entirely, leaving PII intact and no purge event recorded. Idempotency of the event is guaranteed by INSERT OR IGNORE on the deterministic request_id key. #1: replace inline IN-placeholder SQL in clear_user_data Phase 3 + Phase 4 with _delete_in_chunks() calls to stay under SQLite SQLITE_MAX_VARIABLE_NUMBER on large user datasets. #2: call _delete_source_windows_for_user_playbook_ids before hard-deleting user_playbooks in Phase 4 — without this the agent_playbook_source_user_playbooks join table accumulated orphan rows. #3: wrap Phase 5 purge loop in with self._lock — self._lock is an RLock so purge_content's internal re-acquire is safe; Phase 4 commit already closed the outer transaction so no flush hazard exists. #5: update abstract + concrete purge_content docstrings to explicitly state only 'profile'/'user_playbook' are supported; 'agent_playbook' raises ValueError. #6: drop conditional escape hatch in test_clear_user_data_tombstone_only_user — scenario is deterministic (tombstone → purge set), assert unconditionally. #7: add is_purged assertion to cross-user-chain test. New tests: test_purge_with_empty_content_still_blanks_other_pii and test_purge_idempotent_on_already_purged_row covering the #4 invariant.
…ch-ups
F1: test_purge_user_playbook_blanks_body_user_id_null locks in user_playbook
purge path; user_id is NULLABLE so purge sets it to NULL (not '').
Also caught a production bug: SELECT rowid FROM user_playbooks fails with
IndexError on sqlite3.Row when the table has INTEGER PRIMARY KEY (alias
collision). Fixed by aliasing as _rowid in the SELECT.
F2: test_clear_user_data_chunk_boundary monkeypatches chunked() to size=2,
seeds 5 profiles, asserts all are deleted without error.
F3: test_clear_user_data_no_orphan_source_window_rows seeds a source-window
join row for a hard-deleted user_playbook, asserts the join row is gone
after clear_user_data.
Optional: test_purge_agent_playbook_raises_value_error.
F4: fix misleading comment on if cur.rowcount guard — rowcount is 1 whenever
the row exists (not only when content changed), event idempotency is
guaranteed by INSERT OR IGNORE on the deterministic request_id.
F5: change has_inbound_lineage_refs abstract body from ... to
raise NotImplementedError, matching the file convention.
F6: add docstring note to BaseStorage.clear_user_data that the default
(non-SQLite) path is not wrapped in a single transaction.
…bit #2) Move the Phase-5 content-purge loop inside the Phase-4 lock block so the lock is never released between the hard-delete commit and the purges — erase-eligible rows are never observable with PII intact mid-erasure. RLock-safe; the hard-delete commit already closed the outer transaction so purge_content's self-commits don't flush pending work.
Collaborator
Author
|
Triaged the 4 re-raised CodeRabbit comments against the current head:
|
This was referenced Jun 28, 2026
feat: expiry reclamation — reclaim TTL-expired rows (profiles, share_links, pending_tool_calls)
#265
Merged
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 7, 2026
…rity) (#300) ## Summary **SEC-016** — cross-backend GDPR-erase parity on `lineage_event`. On user-data erase, **SQLite hard-DELETEd** `lineage_event` rows referencing erased entities, while **Supabase retains** the content-free lineage skeleton (its erase path never enumerates `lineage_event` — it's in neither the purge nor the delete set; the #219/#383 content-purge design keeps a content-free skeleton and purges PII on the entity tables). This converges SQLite onto the intended model. ## Change - **Stop deleting `lineage_event` on erase** (`sqlite_storage/governance/_erase_execution.py`). Safe because `lineage_event` has **no PII/content column, no foreign keys, no FTS/vec shadow tables** — retaining the row is exactly the content-free skeleton the design intends, and lineage reconstruction is *helped* (the retained `status_change` signal survives). Removed the delete block + now-dead `erased_entity_ids`/`request_ids`/`import json`. Entity-table deletes + `purge_content` calls are untouched. - **Inverted the test** that codified the old behavior (`test_governance_storage.py`): `test_apply_governance_user_data_delete_retains_lineage_skeleton` now asserts the pre-seeded skeleton row **still exists** (`== 1`) after erase, instead of `== 0`. ## Note on scope The retained values are opaque surrogate IDs (`profile_id`/`request_id`/`entity_id`) in a content-free table — this matches the **already-shipped Supabase behavior**, so this is a convergence, not a new retention decision. (If those internal IDs should ever be treated as PII, that's a separate cross-backend change requiring Supabase to scrub too.) ## Verification `governance/erase/lineage/purge` tests: **473 passed**; full sqlite storage suite: **390 passed**; ruff + pyright clean. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated data-deletion behavior so non-sensitive lineage “skeleton” records are preserved after governance erasure, while content-bearing user data is still removed. * Aligned deletion handling with expected behavior to avoid removing valid lineage context tied to unrelated records. * **Tests** * Revised coverage to verify that retained lineage records remain after user data deletion. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
1 participant
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.
Implements the
purge_contentprimitive (specified but never built in the lineage design §5.1E/§14). Pairs with the enterprise PR that adds the Supabase/Postgres RPC + bumps this submodule pointer.What it does
GDPR erasure (
clear_user_data) now content-purges a user's referenced lineage tombstones instead of hard-deleting them — blanks all PII (incl.user_id), keeps the{id, status, merged_into, superseded_by, retired_at}skeleton, emits anop=purgeevent — soresolve_currentchains stay resolvable (is_purged=True) while the PII is gone. Standalone rows + interactions/requests hard-delete as before.Changes (TDD, per-task reviewed)
has_inbound_lineage_refs(org-scoped, not user-scoped — cross-user chains).purge_content(entity_type, id)— blank = complement of the keep-allowlist;profiles.user_id→''(NOT NULL),user_playbooks.user_id→NULL; atomic blank+event in one commit (rowcount-guarded), FTS/vec cleanup after commit (the codebase's emit-before-commit hazard avoided); deterministicrequest_id="purge_"+id→ idempotent (no duplicate event); PII-free event. Crash-window test proves no phantom event.clear_user_databodies (SQLite override + BaseStorage default for Supabase/Postgres) gain the purge-vs-delete decision (tombstone OR pointed-to → purge; else hard-delete) via a shared_partition_purge_vs_deletehelper. Erasure enumerates all statuses incl. tombstones (SUPERSEDED/MERGED).is_purgedcontract strengthened onresolve_current/RecordRef+ a guard test. Audit found zero consumers dereference resolved content today (so live-pointed-to-survivor purge is safe now; the consumer-skip is a documented gated dependency for any future consumer).Verification
SQLite purge suite 10 passed; broader storage suite 527 passed; ruff + pyright clean. A final whole-branch review verified atomicity, blank-completeness parity with Supabase, tombstone-erasure completeness, and idempotency.
Summary by CodeRabbit
Documentation
New Features
Tests