feat: add governance RTBF local parity - #250
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds governance models and config, deterministic reference helpers, storage contracts and SQLite support for purge/audit workflows, export/erase service orchestration, retention GC scheduling, and tests for the new flows and playbook exclusion. ChangesGovernance Subsystem
Sequence Diagram(s)sequenceDiagram
participant Caller
participant GovernanceService
participant SQLiteStorage
Caller->>GovernanceService: export_user(user_id, request_id)
GovernanceService->>SQLiteStorage: load sessions, requests, playbooks
GovernanceService->>SQLiteStorage: append_audit_event(EXPORT)
GovernanceService-->>Caller: UserExportResult
Caller->>GovernanceService: erase_user(user_id, request_id)
GovernanceService->>SQLiteStorage: begin_purge_operation(...)
GovernanceService->>SQLiteStorage: prepare_governance_erase_targets(...)
GovernanceService->>SQLiteStorage: hide_governance_agent_playbooks_for_rebuild(...)
GovernanceService->>SQLiteStorage: apply_governance_user_data_delete(...)
GovernanceService->>SQLiteStorage: apply_governance_agent_playbook_rebuild(...)
GovernanceService->>SQLiteStorage: complete_purge_operation_with_audit(...)
GovernanceService-->>Caller: UserEraseResult
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🧹 Nitpick comments (1)
tests/server/services/lineage/test_governance_retention_gates.py (1)
22-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a config fixture variant without
governance_retention.
_gc_tickandmaybe_start_lineage_gcboth have an explicitgetattr(..., "governance_retention", GovernanceRetentionConfig())fallback, but_make_ctxalways supplies that attribute. That means this file never locks in the legacy-config path the PR summary calls out, so a future regression there would still pass these tests.Suggested change
def _make_ctx( *, lineage_gc_enabled: bool, purge_expired_profiles_enabled: bool = False, row_count_retention_enabled: bool = False, audit_events_retention_enabled: bool = False, + include_governance_retention: bool = True, ): storage = MagicMock() storage.gc_expired_tombstones.return_value = 0 storage.gc_governance_retention.return_value = 0 - config = SimpleNamespace( - lineage_gc=LineageGCConfig(enabled=lineage_gc_enabled), - governance_retention=SimpleNamespace( - purge_expired_profiles_enabled=purge_expired_profiles_enabled, - row_count_retention_enabled=row_count_retention_enabled, - audit_events_retention_enabled=audit_events_retention_enabled, - ), - ) + config = SimpleNamespace( + lineage_gc=LineageGCConfig(enabled=lineage_gc_enabled), + ) + if include_governance_retention: + config.governance_retention = SimpleNamespace( + purge_expired_profiles_enabled=purge_expired_profiles_enabled, + row_count_retention_enabled=row_count_retention_enabled, + audit_events_retention_enabled=audit_events_retention_enabled, + ) return SimpleNamespace( org_id="org_1", storage=storage, configurator=SimpleNamespace(get_config=MagicMock(return_value=config)), )Then add one
_gc_tickcase and onemaybe_start_lineage_gccase withinclude_governance_retention=False.🤖 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 `@tests/server/services/lineage/test_governance_retention_gates.py` around lines 22 - 44, Add a _make_ctx fixture variant that can omit the governance_retention attribute so the legacy fallback path is actually tested. Update _make_ctx to accept an include_governance_retention flag and, when false, build the config without governance_retention; then add one _gc_tick case and one maybe_start_lineage_gc case using that variant to cover the getattr(..., "governance_retention", GovernanceRetentionConfig()) fallback in both code paths.
🤖 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/models/api_schema/domain/governance.py`:
- Around line 48-60: AuditEvent currently allows request_ref to be omitted even
though SQLite governance storage rejects it later. Update the AuditEvent model
in governance.py so request_ref is required (non-optional) and keep the schema
aligned with the persistence contract. Make sure any constructors or validators
that rely on AuditEvent use the required request_ref field consistently.
In `@reflexio/models/config_schema.py`:
- Around line 711-716: The governance retention config exposes flags that are
not actually used by gc_governance_retention(), so enabling
purge_expired_profiles_enabled or row_count_retention_enabled only starts
scheduling without any deletion behavior. Update the retention flow in
config_schema/GovernanceRetentionConfig and the related gating logic (including
_is_governance_retention_enabled and gc_governance_retention) so each exposed
flag has a real implementation path, or remove/defer the unused flags until they
are wired through.
In `@reflexio/server/services/governance/service.py`:
- Around line 51-57: The RTBF export/erase flow in the governance service
currently relies on get_user_playbooks with a hard-coded limit of 1_000_000,
which can silently miss records for large accounts. Update the logic in the
service methods that build the export payload and prepare erase coverage to use
pagination or streaming over get_user_playbooks, or else enforce and clearly
document a storage-level maximum before depending on that bound. Ensure the
affected paths around the user_playbooks collection and the erase preparation
logic both handle all playbooks without truncation.
- Around line 80-129: The erase workflow in the governance service currently
lets exceptions from target preparation, hiding, delete, rebuild, or completion
escape without marking the purge failed. Wrap the post-`begin_purge_operation`
flow in the service method that handles user erasure, and on any exception call
`fail_purge_operation` with the current `purge_id` before re-raising so the
purge state is not left active. Keep the existing completed-purge fast path
unchanged, and make sure the failure handling covers the calls to
`prepare_governance_erase_targets`,
`hide_governance_agent_playbooks_for_rebuild`,
`apply_governance_user_data_delete`, `_rebuild_agent_playbooks`, and
`complete_purge_operation_with_audit`.
- Around line 146-163: The pagination check in the session export loop is
counting only appended requests, so skipped rows with request is None can make
the loop stop early. Update the paging logic in the governance service method
that iterates over grouped_sessions so it bases page exhaustion on the total
rows returned from storage, not just the number of exported requests; keep
skipping None requests when building requests and sessions_by_id, but use a
separate returned-row counter (or the raw grouped rows) to decide when to break
and when to advance offset.
In `@reflexio/server/services/lineage/gc_scheduler.py`:
- Around line 34-41: The scheduler startup gate in
_is_governance_retention_enabled is too broad because it enables governance GC
for flags that the SQLite storage path does not actually handle. Update this
helper (and any related scheduler checks in gc_scheduler) to only return true
for the implemented audit-events retention path, or implement the missing
purge_expired_profiles and row_count retention branches in the storage layer
before keeping those flags wired here. Make sure the scheduler only starts and
calls governance GC through the flags that the storage implementation in this
codebase truly honors.
In `@reflexio/server/services/storage/sqlite_storage/_governance.py`:
- Around line 1868-1881: The empty-windows rebuild path in the agent playbook
deletion branch is hard-deleting rows without creating the matching lineage
record. Update the branch that deletes from agent_playbooks to also emit an
op=hard_delete lineage event, mirroring the behavior in delete_agent_playbook
and delete_agent_playbooks_by_ids, unless this RTBF rebuild path is explicitly
meant to be exempt; use the same agent-playbook deletion flow symbols to locate
and align the implementation.
---
Nitpick comments:
In `@tests/server/services/lineage/test_governance_retention_gates.py`:
- Around line 22-44: Add a _make_ctx fixture variant that can omit the
governance_retention attribute so the legacy fallback path is actually tested.
Update _make_ctx to accept an include_governance_retention flag and, when false,
build the config without governance_retention; then add one _gc_tick case and
one maybe_start_lineage_gc case using that variant to cover the getattr(...,
"governance_retention", GovernanceRetentionConfig()) fallback in both code
paths.
🪄 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: c8c03a57-fdb1-427e-acd8-2dbeec7f9ac1
📒 Files selected for processing (18)
reflexio/models/api_schema/domain/__init__.pyreflexio/models/api_schema/domain/governance.pyreflexio/models/config_schema.pyreflexio/server/services/governance/service.pyreflexio/server/services/governance/subject_refs.pyreflexio/server/services/lineage/gc_scheduler.pyreflexio/server/services/storage/sqlite_storage/__init__.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/_governance.pyreflexio/server/services/storage/sqlite_storage/_lineage.pyreflexio/server/services/storage/sqlite_storage/_playbook.pyreflexio/server/services/storage/storage_base/__init__.pyreflexio/server/services/storage/storage_base/_governance.pytests/server/services/governance/test_governance_local_e2e.pytests/server/services/governance/test_subject_refs.pytests/server/services/lineage/test_governance_retention_gates.pytests/server/services/storage/sqlite_storage/test_governance_storage.pytests/server/services/storage/test_agent_playbook_rebuild_exclusion.py
Contributor
Author
|
Addressed the review-body nitpick as well: the governance retention tests now cover legacy configs without |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/_playbook.py`:
- Around line 321-322: The pagination in the playbook query uses only created_at
DESC, which can produce unstable ordering when multiple rows share the same
timestamp. Update the query-building logic in _playbook.py so the ORDER BY
clause includes a deterministic tie-breaker in addition to created_at, using the
existing query path that appends LIMIT and OFFSET. This should be done in the
storage query used by the playbook export/erase flow so the results stay stable
across pages.
🪄 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: de5399dc-d6d7-4464-90da-489f928c9a14
📒 Files selected for processing (10)
reflexio/models/api_schema/domain/governance.pyreflexio/models/config_schema.pyreflexio/server/services/governance/service.pyreflexio/server/services/lineage/gc_scheduler.pyreflexio/server/services/storage/sqlite_storage/_governance.pyreflexio/server/services/storage/sqlite_storage/_playbook.pyreflexio/server/services/storage/storage_base/_playbook.pytests/server/services/governance/test_governance_local_e2e.pytests/server/services/lineage/test_governance_retention_gates.pytests/server/services/storage/sqlite_storage/test_governance_storage.py
💤 Files with no reviewable changes (1)
- reflexio/models/config_schema.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/server/services/lineage/test_governance_retention_gates.py
- reflexio/models/api_schema/domain/governance.py
- reflexio/server/services/governance/service.py
- reflexio/server/services/lineage/gc_scheduler.py
- reflexio/server/services/storage/sqlite_storage/_governance.py
This was referenced Jun 29, 2026
feat: expiry reclamation — reclaim TTL-expired rows (profiles, share_links, pending_tool_calls)
#265
Merged
This was referenced Jul 28, 2026
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.
Summary
Changes
Test Plan
uv run ruff check reflexio/server/services/lineage/gc_scheduler.py reflexio/server/services/storage/sqlite_storage/_governance.py reflexio/server/services/storage/sqlite_storage/_lineage.py tests/server/services/storage/sqlite_storage/test_governance_storage.pyuv run pyright reflexio/server/services/lineage/gc_scheduler.py reflexio/server/services/storage/sqlite_storage/_governance.py reflexio/server/services/storage/sqlite_storage/_lineage.py tests/server/services/storage/sqlite_storage/test_governance_storage.pyuv run pytest --no-cov tests/server/services/storage/sqlite_storage/test_governance_storage.py -q -o 'addopts='(207 passed)uv run pytest --no-cov tests/server/services/lineage/test_gc_scheduler.py tests/server/services/lineage/test_governance_retention_gates.py -q -o 'addopts='(22 passed)Summary by CodeRabbit