fix(sync): safely refresh matching leased deliveries - #2901
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughMaint 68 now preserves an existing sync PR when its base and generated tree match. It records branch metadata, uses lease-protected updates for changed branches, and adds tests for branch refresh and merge eligibility. ChangesSync PR safeguards
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Maint68Workflow
participant ConsumerRepository
participant GitHubPR
Maint68Workflow->>ConsumerRepository: Fetch existing branch metadata
Maint68Workflow->>ConsumerRepository: Compute desired tree
alt Existing base and tree match
Maint68Workflow->>GitHubPR: Refresh existing PR without commit or push
else Existing branch differs
Maint68Workflow->>ConsumerRepository: Force-push with head-specific lease
else Branch does not exist
Maint68Workflow->>ConsumerRepository: Push new branch normally
end
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Runner dispatch state for codex on PR #2901. Do not edit. |
There was a problem hiding this comment.
Pull request overview
Updates the consumer sync delivery contract so Maint 68 can safely keep an existing current-generation sync PR up to date by comparing the remote PR head’s base/tree against the newly staged desired tree, only refreshing the lease when it’s an exact match and otherwise using a --force-with-lease guarded refresh. Adds regression coverage for the refreshed Maint 68 behavior and for merge-eligibility selection of the newest matching generation.
Changes:
- Maint 68 now fetches the existing sync branch head, computes base/tree hashes, and refreshes only a matching delivery attempt (else refreshes via
--force-with-lease). - Adds a workflow-level regression test asserting the new Maint 68 refresh/lease-guard contract is present.
- Extends the lease/merge contract test suite to cover “newest matching generation” selection and terminal/expired ineligibility.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.github/workflows/maint-68-sync-consumer-repos.yml |
Adds base/tree hash comparison + --force-with-lease guarding to safely refresh only matching leased deliveries. |
tests/workflows/test_sync_manifest_delivery.py |
Adds a regression test that asserts the new Maint 68 refresh/lease-guard shell contract exists. |
.github/scripts/__tests__/sync_pr_lease_contract.test.js |
Adds selection coverage ensuring only the newest matching generation is merge-eligible and terminal/expired records are ineligible. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9424cbd960
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Runner dispatch state for autofix on PR #2901. Do not edit. |
|
Autofix updated these files:
|
Require an unexpired non-terminal delivery marker before refreshing an existing same-generation sync PR, configure git credentials before the early fetch, and keep Black formatting on the Maint 68 contract test. Co-authored-by: Cursor <cursoragent@cursor.com>
Closer recovery (cursor) — lint + review threadsHead: Fixes
Validation
Fresh CI on |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/scripts/__tests__/sync_pr_lease_contract.test.js:
- Around line 41-67: Update the test around selectMergeEligibleSyncPr so both
old and newest candidates match syncHash: 'new' while retaining different
created_at values. Keep the candidates otherwise distinguishable, then assert
newest is active and old is stale to verify selection uses recency rather than
generation matching alone.
In `@tests/workflows/test_sync_manifest_delivery.py`:
- Around line 250-263: The test
test_maint68_refreshes_only_a_same_base_and_tree_delivery_attempt must verify
the no-op matching_existing path skips both git commit and git push, while the
mismatch path performs the lease-protected force push. Extend the source
assertions to cover the conditional commit/push structure and distinguish
behavior for matching versus non-matching deliveries.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f7ddd5fd-d2ca-4525-bc44-9553aab62d39
📒 Files selected for processing (3)
.github/scripts/__tests__/sync_pr_lease_contract.test.js.github/workflows/maint-68-sync-consumer-repos.ymltests/workflows/test_sync_manifest_delivery.py
🤖 Bot Comment Handler
The agent has been assigned to this PR to address the bot review comments. Instructions for agent
The bot comment handler workflow has prepared context in the artifacts. |
|
Addressed the two current CodeRabbit test findings and the Maint 52 ShellCheck failure in |
Provider Comparison ReportProvider Summary
📋 Full Provider Details (click to expand)openai
anthropic
Agreement
DisagreementNo major disagreements detected. Unique Insights
🔍 LangSmith Traces |
Closes #2880
Automated Status Summary
Scope
The coordination object must be allowed to persist, but a generated PR must not. The current durable campaign already models queue fingerprints and item leases in
.github/scripts/sync_dependency_campaign.js:178-230,360-456, while Maint 68 stops when an existing consumer PR is found (.github/workflows/maint-68-sync-consumer-repos.yml:681-724). That combination leaves the opener/closer system without a bounded rule for whether an old PR should be refreshed, replaced, closed, or escalated.This is a current productivity defect: generated PRs can survive across source generations, repeatedly consume review and CI attention, and still be indistinguishable from the latest intended delivery. The durable object should be the campaign issue (currently #1836), with each generated PR acting as a leased delivery attempt that has an explicit terminal disposition.
Context for Agent
Related Issues/PRs
Tasks
.github/scripts/sync_dependency_campaign.jswith campaign issue URL, plan ID, generation, repository, desired tree hash, source commit, lease expiry, and predecessor/successor PR references..github/workflows/maint-68-sync-consumer-repos.ymland.github/workflows/maint-52-sync-dev-versions.yml..github/scripts/sync_pr_merge_contract.jsand.github/workflows/maint-71-merge-sync-prs.ymlso only the latest unexpired delivery generation is merge-eligible.merged,superseded,expired, andblocked; blocked transitions must link a durable source or repo-local issue with the exact next action..github/scripts/sync_tracker_state.jsand the campaign state used by Maint 82.docs/ops/SYNC_DEPENDENCY_CAMPAIGN.md,docs/ops/DURABLE_TRACKING_ISSUES.md, anddocs/ops/CONSUMER_REPO_MAINTENANCE.mdwith the rule “durable issue, leased PR.”Acceptance criteria
.github/scripts/__tests__/sync_dependency_campaign.test.jsproves that the durable issue survives multiple delivery generations while each PR reaches exactly one terminal disposition..github/scripts/__tests__/sync_pr_lease_contract.test.jsproves that only the newest unexpired PR whose desired tree hash matches the current plan is merge-eligible.tests/workflows/test_sync_manifest_delivery.pyproves Maint 68 updates the same current-generation PR when safe and creates a replacement only after a generation or terminal-state transition.sync_pr_lease_contract.test.jsfails merge eligibility, then revert the fixture and verify the suite passes.python scripts/dev_check.py --action testand the repository workflow-validation suite successfully.Summary by CodeRabbit
Bug Fixes
Tests