fix(playbook): preserve agent playbooks on null aggregation - #241
Conversation
|
Warning Review limit reached
More reviews will be available in 30 minutes and 26 seconds. 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 review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. 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 (2)
📝 WalkthroughWalkthroughPlaybook aggregation now defers archival until after generation. Incremental runs track prior fingerprints for changed clusters, mark replaced playbooks during save, and selectively soft-supersede only when replacements exist. The null-generation test now expects no archive, supersede, or delete actions. ChangesIncremental playbook archival flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/playbook/components/aggregator.py`:
- Around line 939-948: The selective supersede logic in aggregator.py is using
the run-wide new_playbooks condition, which can incorrectly supersede archived
playbooks for clusters that still returned None and can also drop deferred IDs
after update_cluster_fingerprints(...) clears agent_playbook_id. Update the flow
around update_cluster_fingerprints(...) and the supersede branch to track
deferred supersede per cluster, preserving each cluster’s previous
fingerprint/agent_playbook_id until that cluster gets a replacement, and only
call supersede_agent_playbooks_by_ids(...) for IDs tied to clusters that
actually produced new playbooks.
🪄 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: cfce844f-ec85-4f1b-ba18-f05cc705a4cc
📒 Files selected for processing (2)
reflexio/server/services/playbook/components/aggregator.pytests/server/services/playbook/test_playbook_aggregator.py
There was a problem hiding this comment.
Actionable comments posted: 1
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/playbook/components/aggregator.py (1)
837-849: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore only archives performed by this run.
With archive deferred, generation can fail before Lines 837-842 archive anything, but the exception path still restores by
full_archiveorarchived_playbook_ids. In incremental mode noarchive_agent_playbooks_by_idsruns at all, so restoring those candidate IDs can resurrect playbooks archived before this aggregation attempt.Guard restoration with an archive-performed flag
+ full_archive_performed = False + incremental_archive_performed = False @@ if new_playbooks: for name in full_archive_playbook_names: self.storage.archive_agent_playbooks_by_playbook_name( name, agent_version=self.agent_version ) + full_archive_performed = True @@ - if full_archive: + if full_archive and full_archive_performed: for name in full_archive_playbook_names: self.storage.restore_archived_agent_playbooks_by_playbook_name( name, agent_version=self.agent_version ) - elif archived_playbook_ids: + elif incremental_archive_performed and archived_playbook_ids: self.storage.restore_archived_agent_playbooks_by_ids( archived_playbook_ids )Also applies to: 1053-1061
🤖 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/playbook/components/aggregator.py` around lines 837 - 849, The restore logic in the aggregation flow is using candidate archive state even when no archive operation actually ran in this attempt. Add an explicit “archive performed” flag around the archive steps in aggregator.py (where full_archive and archive_agent_playbooks_by_ids are handled), set it only when this run actually archives playbooks, and gate the exception/rollback path on that flag so only archives created by this run are restored, including the later restore block around the corresponding cleanup logic.
🤖 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/playbook/components/aggregator.py`:
- Around line 904-913: Delay marking shared prior fingerprints as replaced until
every overlapping changed cluster has produced a replacement; the current logic
in aggregator.py within the fingerprint-processing flow (the loop over
previous_fingerprints_for_changed_clusters, the null-cluster preservation check,
and the selective supersede selection) can prematurely add prev_fp to
replaced_previous_fingerprints and later supersede the old playbook. Update the
logic around previous_fingerprints_for_changed_clusters,
replaced_previous_fingerprints, and selective_supersede_playbook_ids so a shared
prev_fp is only considered superseded once all clusters that reference it have
non-null replacements, preserving the visible playbook for any split branch that
returns None.
---
Outside diff comments:
In `@reflexio/server/services/playbook/components/aggregator.py`:
- Around line 837-849: The restore logic in the aggregation flow is using
candidate archive state even when no archive operation actually ran in this
attempt. Add an explicit “archive performed” flag around the archive steps in
aggregator.py (where full_archive and archive_agent_playbooks_by_ids are
handled), set it only when this run actually archives playbooks, and gate the
exception/rollback path on that flag so only archives created by this run are
restored, including the later restore block around the corresponding cleanup
logic.
🪄 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: 7ae1740c-282c-4c25-9ddc-270c89ed20cb
📒 Files selected for processing (2)
reflexio/server/services/playbook/components/aggregator.pytests/server/services/playbook/test_playbook_aggregator.py
1 participant
Summary
Changes
Test Plan
uv run ruff format --check reflexio/server/services/playbook/components/aggregator.py tests/server/services/playbook/test_playbook_aggregator.pyuv run ruff check reflexio/server/services/playbook/components/aggregator.py tests/server/services/playbook/test_playbook_aggregator.pyuv run pytest tests/server/services/playbook/test_playbook_aggregator.py -o 'addopts='Summary by CodeRabbit