feat(lineage): enable soft-delete + tombstone GC by default - #201
Merged
Conversation
- LineageGCConfig.enabled: False → True (GC on by default; tombstones now reclaimed after the 90-day grace window without explicit config) - is_dedup_soft_delete_enabled + is_aggregation_soft_delete_enabled: fail-closed → default-open; absent key returns True, not False (mechanism: new _is_default_open_flag_enabled helper; explicit disable via enabled=False still works; strict-bool + strict-list guards kept) - Test fallout: 5 tests that asserted off-by-default now assert on-by-default; 4 hard-delete path tests pinned is_aggregation_soft_delete_enabled=False - New tests: TestSoftDeleteDefaultOn (6), test_lineage_gc_can_be_explicitly_disabled, TestDefaultOnEndToEnd (3 e2e: supersede → tombstone, GC reclaim aged, GC retain fresh)
📝 WalkthroughWalkthroughTwo features are flipped to default-ON: ChangesSoft-delete and lineage GC default-ON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (2)
tests/server/services/profile/test_dedup_soft_delete_integration.py (1)
405-411: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMake the SQLite fixture deterministic regardless of expansion-mode settings.
Line 405 currently patches only
_get_embedding. If expansion mode is enabled,add_user_profilecan take the_expand_documentbranch and make this test path environment-dependent. Force_should_expand_documentstoFalsein the fixture.Proposed fixture update
`@pytest.fixture` def db(self): with ( tempfile.TemporaryDirectory() as tmp, + patch.object(SQLiteStorage, "_should_expand_documents", return_value=False), patch.object(SQLiteStorage, "_get_embedding", return_value=[0.0] * 512), ): yield SQLiteStorage(org_id="e2e_default_org", db_path=f"{tmp}/e2e.db")🤖 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/profile/test_dedup_soft_delete_integration.py` around lines 405 - 411, The db fixture currently only patches _get_embedding on SQLiteStorage, which makes the test non-deterministic if expansion mode is enabled since add_user_profile may take different code paths. Add an additional patch for the _should_expand_documents method on SQLiteStorage in the same patch.object context manager, setting it to return False to ensure the test behavior is deterministic and independent of expansion-mode settings.tests/server/services/playbook/test_playbook_aggregator.py (1)
671-680: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winStrengthen flag-OFF tests with negative assertions for soft-delete calls.
On Line 690, Line 768, and Line 834 you assert hard-delete calls, but these tests would still pass if a regression invoked both soft-delete and hard-delete paths. Add
assert_not_called()checks to lock exclusivity.Proposed test hardening
@@ def test_rerun_deletes_archived_playbooks_after_success( self, mock_gen, mock_clust, _mock_flag ): @@ agg.storage.delete_archived_agent_playbooks_by_playbook_name.assert_has_calls( [ call(SINGLETON_USER_PLAYBOOK_NAME, agent_version="v1"), call("test_fb", agent_version="v1"), ], any_order=True, ) + agg.storage.supersede_agent_playbooks_by_playbook_name.assert_not_called() @@ def test_incremental_with_changes_archives_selectively( self, mock_gen, mock_clust, _mock_flag ): @@ agg.storage.archive_agent_playbooks_by_ids.assert_called_once_with([50]) agg.storage.delete_agent_playbooks_by_ids.assert_called_once_with([50]) + agg.storage.supersede_agent_playbooks_by_ids.assert_not_called() @@ def test_change_log_exception_is_caught(self, mock_gen, mock_clust, _mock_flag): @@ # Despite the exception, hard-delete should still proceed (flag OFF path) agg.storage.delete_archived_agent_playbooks_by_playbook_name.assert_called() + agg.storage.supersede_agent_playbooks_by_playbook_name.assert_not_called()Also applies to: 740-749, 811-818, 833-834
🤖 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/playbook/test_playbook_aggregator.py` around lines 671 - 680, The test test_rerun_deletes_archived_playbooks_after_success mocks is_aggregation_soft_delete_enabled to return False to test the hard-delete path, but it only asserts that hard-delete methods are called without asserting that soft-delete methods are NOT called. This creates a gap where a regression that invokes both soft-delete and hard-delete paths would still pass the test. Add assert_not_called() assertions for the soft-delete related mock objects to ensure exclusivity between the two deletion paths when the flag is OFF. Apply the same pattern to the other affected tests around lines 740-749, 811-818, and 833-834 where the soft-delete flag is mocked to False.
🤖 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.
Nitpick comments:
In `@tests/server/services/playbook/test_playbook_aggregator.py`:
- Around line 671-680: The test
test_rerun_deletes_archived_playbooks_after_success mocks
is_aggregation_soft_delete_enabled to return False to test the hard-delete path,
but it only asserts that hard-delete methods are called without asserting that
soft-delete methods are NOT called. This creates a gap where a regression that
invokes both soft-delete and hard-delete paths would still pass the test. Add
assert_not_called() assertions for the soft-delete related mock objects to
ensure exclusivity between the two deletion paths when the flag is OFF. Apply
the same pattern to the other affected tests around lines 740-749, 811-818, and
833-834 where the soft-delete flag is mocked to False.
In `@tests/server/services/profile/test_dedup_soft_delete_integration.py`:
- Around line 405-411: The db fixture currently only patches _get_embedding on
SQLiteStorage, which makes the test non-deterministic if expansion mode is
enabled since add_user_profile may take different code paths. Add an additional
patch for the _should_expand_documents method on SQLiteStorage in the same
patch.object context manager, setting it to return False to ensure the test
behavior is deterministic and independent of expansion-mode settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: feee6b11-ec7d-4adb-9d7f-a9a4eab19064
📒 Files selected for processing (7)
reflexio/models/config_schema.pyreflexio/server/site_var/feature_flags.pytests/models/test_lineage_gc_config.pytests/server/services/playbook/test_cluster_change_detection.pytests/server/services/playbook/test_playbook_aggregator.pytests/server/services/profile/test_dedup_soft_delete_integration.pytests/server/site_var/test_feature_flags.py
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
Makes soft-delete + tombstone GC the default for lineage — fulfilling the design's rev-3 intent ("soft-delete is the DEFAULT"). The mechanism has been merged and tested for several phases but shipped OFF; this flips the defaults ON so the next deployment activates it (the deploy applies the
retired_atmigration first, then picks up the new defaults — migration coverage is automatic).Changes
LineageGCConfig.enabled:False → True(GC runs by default; 90-day grace window unchanged). Docstring rewritten to document enabled-by-default + how to disable for an extended audit hold.is_dedup_soft_delete_enabled/is_aggregation_soft_delete_enabled: now default-open via a new_is_default_open_flag_enabledhelper — absent key → ON; malformed config → OFF (safe fallback); explicitenabled:false+ per-orgenabled_org_idsoverride preserved; strict-bool/strict-list guards intact. Both flip together with GC so tombstones are always reclaimed (no unbounded growth).Test Plan
TestSoftDeleteDefaultOn+ an end-to-end test proving soft-supersede → tombstone → GC reclaims aged / retains fresh.test_lineage_gc/reconstruct_enterprise_integrationare unrelated — A/B-verified identical with this change reverted; CI uses a fresh DB.)Activation note
Merging lands this on
main; production activates on the next deployment (GC + soft-delete on for all orgs). Irreversible deletes begin after the 90-day grace window. Disable per-deployment viaLineageGCConfig.enabled=falseif needed.Summary by CodeRabbit
Release Notes
New Features
Tests