fix(sync): converge delivery in stable reviewed PRs - #3049
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: 29 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 (21)
📝 WalkthroughWalkthroughThe PR replaces hash-based consumer delivery branches with stable candidate and delivery lanes. It adds staged delivery records, reviewer settlement, exact-head sealing, Gate validation, merge holds, workflow updates, tests, and operational documentation. ChangesStable delivery contracts and review
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d229f2107b
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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__/runtime-ac-merge-guard.test.js:
- Around line 63-83: Add a test case in the mutable generated delivery coverage
that supplies both sync:delivery-staging and a runtime AC label while setting
allowSealedSyncDelivery: true, then assert the runtime AC requirement still
rejects the request. Keep the existing generic-merger rejection and sealed Maint
71 success assertions unchanged.
In @.github/scripts/__tests__/sync_pr_merge_contract.test.js:
- Around line 363-393: Extend the test named “reviewer settlement never requires
every configured reviewer” with an assertion for the interval after the quiet
period but before maxWaitMs, using no responses and available reviewers. Verify
evaluateReviewerSettlement returns ready: false, reason:
'review_quorum_pending', and the expected eligible_at value, covering the path
later mapped to reviewer_settlement_pending.
In @.github/scripts/maint71_merge_sync_prs.js:
- Around line 278-310: Wrap the GitHub GraphQL request in
collectReviewerEvidence with the existing withRetry pattern and a try/catch,
matching activeReviewThreadCount’s failure handling. On any exhausted or caught
API failure, warn and return empty responded/unavailable lists with truncated:
true so evaluateReviewerSettlement remains pending and cannot reach quorum from
missing evidence.
- Around line 844-846: Update the head timestamp assignment near
evaluatePostPushReviewWindow so it uses a server-observed push anchor: retain
the existing pushed timestamp when available, or clamp the selected commit date
to at least pr.head.updated_at. Do not rely solely on
selectedHeadCommit.comitter.date or author.date, ensuring backdated commits
cannot make the post-push review window immediately eligible.
In @.github/scripts/sync_pr_lease_contract.js:
- Around line 95-101: Update replaceDeliveryRecord to pass a replacer function
to String.prototype.replace instead of using marker directly as the replacement
string. Preserve the existing expression matching and return marker unchanged
from the callback so `$&`, `$'`, `$```, and `$<name>` sequences in serialized
delivery-record fields cannot be interpreted.
In @.github/scripts/sync_pr_merge_contract.js:
- Around line 209-256: In .github/scripts/sync_pr_merge_contract.js lines
209-256, update the reviewer settlement logic around the contract function to
normalize quietPeriodMs, maxWaitMs, and minimumResponses to finite defaults
before date arithmetic and eligible_at generation. In
.github/scripts/maint71_merge_sync_prs.js lines 182-194, validate the parsed
policy during load, rejecting or defaulting non-finite minimum_responses,
quiet_period_minutes, and maximum_wait_minutes, and wrap fs.readFileSync plus
JSON.parse to report clear failures for missing or malformed policy files before
the repository loop starts.
In @.github/workflows/maint-68-sync-consumer-repos.yml:
- Around line 924-930: Update the exact no-op handling around the existing PR
matching logic to detect valid legacy records lacking delivery_state, write the
staging lifecycle record as a metadata-only migration, and avoid any commit or
push. Preserve the current review lifecycle and early-exit behavior for already
initialized records, and add coverage for a valid legacy record whose base and
tree are unchanged.
- Line 779: Update both gh pr list lookups at the existing branch-name queries,
including the one assigned to existing_pr, to restrict results to
same-repository pull requests by filtering isCrossRepository == false or
matching the target headRepository identity. Preserve selection of the intended
local branch PR, and add a regression case covering a fork PR named
sync/workflows-delivery.
In @.github/workflows/maint-71-merge-sync-prs.yml:
- Line 183: Update the merge step’s always() condition to also require
successful selector resolution and repository extraction, and ensure an empty
ACTIVE_SYNC_HASH_INPUT cannot permit unscoped processing. Preserve the existing
candidateEvidenceAllowsMutation checks while preventing merges for any
repository when either prerequisite fails.
In @.github/workflows/pr-00-gate.yml:
- Around line 165-212: Require the PR head repository to equal github.repository
in both delivery-seal step conditions in
.github/workflows/pr-00-gate.yml:165-212 and
templates/consumer-repo/.github/workflows/pr-00-gate.yml:174-221. In
.github/scripts/gate_summary.py:55-100 and
templates/consumer-repo/.github/scripts/gate_summary.py:55-100, make stable
branches from forked repositories return (False, True, ""). Extend
tests/workflows/github_scripts/test_gate_summary.py:250-307 with a forked
stable-branch event and assert it is not classified as a generated delivery.
In `@docs/SYNC_WORKFLOW.md`:
- Around line 111-112: Update the Summary Checklist near the legacy merge
guidance to remove instructions for closing duplicate sync PRs and merging clean
PRs. Replace those items with Maint 71 reconciliation steps that preserve the
stable-branch quiet-period and recorded check-timestamp workflow.
In `@scripts/check_consumer_sync_drift.py`:
- Line 661: Add an assertion in
test_build_report_marks_current_sync_pr_as_covered that verifies
sync_remediation.delivery_branch matches the expected delivery branch value,
alongside the existing expected_branch assertion. Keep the test focused on the
new report contract.
🪄 Autofix
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: 85973cac-dabf-45ef-88bb-5e5bbcf0f6e9
📒 Files selected for processing (28)
.github/scripts/__tests__/runtime-ac-merge-guard.test.js.github/scripts/__tests__/sync_pr_lease_contract.test.js.github/scripts/__tests__/sync_pr_merge_contract.test.js.github/scripts/gate_summary.py.github/scripts/maint71_merge_sync_prs.js.github/scripts/runtime_ac_merge_guard.js.github/scripts/sync_pr_lease_contract.js.github/scripts/sync_pr_merge_contract.js.github/workflows/maint-68-sync-consumer-repos.yml.github/workflows/maint-71-merge-sync-prs.yml.github/workflows/pr-00-gate.ymlREADME.mdconfig/consumer_sync_review_policy.jsondocs/INTEGRATION_GUIDE.mddocs/SYNC_WORKFLOW.mddocs/WORKFLOW_GUIDE.mddocs/ci/WORKFLOWS.mddocs/ci/WORKFLOW_SYSTEM.mddocs/ops/CONSUMER_REPO_MAINTENANCE.mddocs/ops/DURABLE_TRACKING_ISSUES.mddocs/ops/SYNC_DEPENDENCY_CAMPAIGN.mdscripts/check_consumer_sync_drift.pytemplates/consumer-repo/.github/scripts/gate_summary.pytemplates/consumer-repo/.github/scripts/runtime_ac_merge_guard.jstemplates/consumer-repo/.github/workflows/pr-00-gate.ymltests/scripts/test_check_consumer_sync_drift.pytests/workflows/github_scripts/test_gate_summary.pytests/workflows/test_sync_manifest_delivery.py
Automated Status SummaryHead SHA: 5e4e627
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Low Coverage Files (<50.0%)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeNo scope information available Tasks
Acceptance criteria
|
Summary
Why
The previous hash-per-wave fan-out turned each source correction into another fleet-sized PR wave. It also had no durable reviewer-settled state, so recurring sync runs could restart the same review cycle and generic merge lanes could consume an in-place PR before delivery was complete.
This change makes PR identity stable while keeping plan and tree identity immutable in the delivery record. Review capacity never requires all configured reviewers, but unresolved live review debt remains a hard stop.
Validation
Rollout
This PR is intentionally draft during the mandatory post-push review window. After an unchanged-head review pass it can be marked ready; merging it starts one new candidate cycle, followed by Maint 71 candidate settlement/evidence and stable delivery promotion.
Summary by CodeRabbit