feat(sync): reconcile generated delivery handoffs - #2898
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: 42 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 (6)
📝 WalkthroughWalkthroughThe PR extends generated delivery handling to dev-tool sync branches. It centralizes classification, review blocking, cleanup, result metadata, and handoff records in Maint 71. Tests and operations documentation cover the updated contract. ChangesGenerated delivery reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Maint71Workflow
participant GeneratedPR
participant DeliveryContract
participant MergeReport
Maint71Workflow->>GeneratedPR: read branch, review threads, and checks
Maint71Workflow->>DeliveryContract: classify delivery state
DeliveryContract-->>Maint71Workflow: disposition and next command
Maint71Workflow->>MergeReport: record result and handoff
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
Runner dispatch state for codex on PR #2898. Do not edit. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/sync_pr_merge_contract.js:
- Around line 70-71: Update the active review-thread decision in the
merge-contract logic to treat negative and non-finite activeReviewThreadCount
values as blocking evidence failures, rather than allowing the ready path to
return current. Use the review-query retry command for this blocker, preserve
the existing resolve-active-review-threads behavior for positive counts, and add
a regression test covering activeReviewThreadCount: -1.
- Around line 269-282: Update buildDeliveryHandoff so it only returns a handoff
when the PR result includes valid head_sha and delivery_generation values,
rather than emitting records with empty normalized restart data. Ensure all PR
result paths populate these delivery fields before calling buildDeliveryHandoff,
or reject and avoid publishing incomplete records for stale_closed,
stale_close_failed, and delivery_contract_blocked outcomes.
In @.github/workflows/maint-71-merge-sync-prs.yml:
- Around line 137-140: Update the workflow’s open-PR filtering to use
isTrustedGeneratedDeliveryPr instead of isTrustedSyncPr, allowing trusted
deps/sync-dev-versions- PRs to reach classifyGeneratedPr and the merge flow.
Adjust the active-PR selection logic to recognize the generated delivery lane
and avoid treating valid dev-tool PRs as missing or stale.
In `@docs/ops/CONSUMER_REPO_MAINTENANCE.md`:
- Around line 444-451: Update the “Generated delivery ownership” heading in the
document to use level-two Markdown syntax and ensure there is a blank line
before it, preserving the section’s existing content.
In `@docs/ops/SYNC_DEPENDENCY_CAMPAIGN.md`:
- Around line 21-24: Update the campaign-state and local-watcher documentation
in SYNC_DEPENDENCY_CAMPAIGN.md so local agent work is requested only when an
actionable exception fingerprint materially changes. Add the actionable
predicate to the handoff condition and use identical wording wherever Maint 82
and watcher behavior is described.
- Around line 9-19: Update classifyGeneratedPr so the shared review-thread
predicate treats activeReviewThreadCount values indicating unknown state,
including -1 from pagination or API errors, as blocking alongside positive
counts. Apply this consistently to both sync/workflows-* and
deps/sync-dev-versions-* lanes, and add a regression test covering unavailable
review-thread evidence.
🪄 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: c61e1863-3df0-4c25-9f0d-220781167798
📒 Files selected for processing (6)
.github/scripts/__tests__/sync-pr-merge-contract.test.js.github/scripts/sync_dependency_campaign.js.github/scripts/sync_pr_merge_contract.js.github/workflows/maint-71-merge-sync-prs.ymldocs/ops/CONSUMER_REPO_MAINTENANCE.mddocs/ops/SYNC_DEPENDENCY_CAMPAIGN.md
There was a problem hiding this comment.
Pull request overview
This PR aims to unify the “generated delivery” reconciliation contract for both consumer-sync (sync/workflows-*) and dev-tool-sync (deps/sync-dev-versions-*) PRs by centralizing classification/handoff fields and documenting the intended ownership model (Maint 71 as the sole merge/close reconciler; Maint 82 as durable campaign state).
Changes:
- Adds a documented contract for generated delivery handoffs and clarifies operator expectations in ops docs.
- Extends
.github/scripts/sync_pr_merge_contract.jswith generated-delivery lane helpers, aclassifyGeneratedPrdisposition helper, and a machine-readablehandoff_recordssurface. - Adds initial Maint 71 wiring to record delivery context fields and a review-blocked short-circuit, plus test coverage for new helpers.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/ops/SYNC_DEPENDENCY_CAMPAIGN.md | Introduces the intended campaign/lease contract and Maint 71/Maint 82 ownership model. |
| docs/ops/CONSUMER_REPO_MAINTENANCE.md | Adds operator-facing guidance referencing the new campaign contract doc. |
| .github/workflows/maint-71-merge-sync-prs.yml | Starts emitting normalized delivery context fields and review-blocked handling in Maint 71 results. |
| .github/scripts/sync_pr_merge_contract.js | Adds generated-delivery lane detection, PR classification, and handoff record building. |
| .github/scripts/sync_dependency_campaign.js | Expands sync PR detection to include dev-tool-sync branch prefix. |
| .github/scripts/tests/sync-pr-merge-contract.test.js | Adds tests for lane detection, generated delivery classification, and handoff record shape. |
Suppressed comments (3)
.github/workflows/maint-71-merge-sync-prs.yml:146
- Maint 71 still filters open PRs with
isTrustedSyncPr(sync/workflows-* only), and the selection helpers are still sync-specific (selectMergeEligibleSyncPr,syncBranchForHash). As a result, dev-tool-sync branches (deps/sync-dev-versions-*) will not be reconciled/merged/closed by this workflow despite the new delivery-lane plumbing.
const {
buildMarkdownSummary,
buildMergeReport,
classifyGeneratedPr,
classifySyncPrChecks,
collectDeletableSyncBranches,
generatedDeliveryLane,
normalizeSyncHash,
parseBooleanInput,
isTrustedSyncPr,
selectMergeEligibleSyncPr,
selectSyncPrGatingChecks,
} = require('./.github/scripts/sync_pr_merge_contract.js');
docs/ops/SYNC_DEPENDENCY_CAMPAIGN.md:12
- This doc states Maint 71 applies the same merge/close reconciliation contract to
deps/sync-dev-versions-*PRs, butmaint-71-merge-sync-prs.ymlstill selects open PRs viaisTrustedSyncPr(sync/workflows-* only). Either the workflow needs to be updated to actually reconcile dev-tool-sync PRs, or this doc should be softened until that support lands to avoid an operationally misleading contract.
Maint 71 is the sole merge/close reconciler. It applies the same contract to
both `sync/workflows-*` consumer-sync branches and
`deps/sync-dev-versions-*` shared dev-tool propagation branches:
.github/scripts/sync_dependency_campaign.js:100
isSyncPullRequest()now classifiesdeps/sync-dev-versions-*PRs as sync PRs for campaign purposes, but Maint 71 still only reconcilessync/workflows-*PRs. This mismatch can cause dev-tool-sync deliveries to enter campaign tracking without a corresponding merge/close authority, reintroducing repeated handoffs / non-terminal exception churn.
function isSyncPullRequest(pr = {}) {
const headRef = cleanString(pr.head?.ref || pr.headRefName || pr.headRef || pr.head_ref);
const title = cleanString(pr.title).toLowerCase();
const body = cleanString(pr.body);
const labels = labelsForPullRequest(pr).map((label) => label.toLowerCase());
return (
headRef.startsWith(SYNC_BRANCH_PREFIX) ||
headRef.startsWith(DEV_TOOL_SYNC_BRANCH_PREFIX) ||
title.startsWith('chore: sync workflow templates') ||
labels.includes('sync') ||
body.includes('workflows-sync-lifecycle')
);
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 692bb59633
ℹ️ 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".
|
Addressed the current inline review set in |
🤖 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. |
Provider Comparison ReportProvider Summary
📋 Full Provider Details (click to expand)openai
anthropic
Agreement
DisagreementNo major disagreements detected. Unique Insights
🔍 LangSmith Traces |
Closes #2881
Automated Status Summary
Scope
Generated PR ownership is split across overlapping controllers. Maint 71 filters only
sync/workflows-heads today (.github/workflows/maint-71-merge-sync-prs.yml:284), while Maint 82 and.github/scripts/sync_dependency_campaign.js:493-530separately decide which exceptions are claimable by Codex. Dev-tool-sync PRs therefore follow a different merge path, and unchanged exception state can continue to produce automation work.This is a current efficiency defect: repeated observation of the same blocker consumes agent runs without changing repository state. One repository-owned reconciliation contract should decide merge/close disposition, and agent handoff should occur only when the exception fingerprint changes.
Tasks
.github/workflows/maint-71-merge-sync-prs.ymlto reconcile bothsync/workflows-anddeps/sync-dev-versions-PRs through the same contract..github/scripts/sync_pr_merge_contract.js: current, awaiting-checks, review-blocked, repo-local-failure, shared-source-failure, superseded, expired, and owner-decision..github/workflows/maint-82-sync-dependency-auto-pilot.ymlconsume those records and own durable campaign state rather than recomputing merge disposition independently..github/scripts/sync_dependency_campaign.jsto include only state that should trigger new work; preserveupdated_at, claim generation, and lease history.campaign:needs-local-codexonly for new or materially changed actionable exception fingerprints, and remove it after terminal disposition or successful claim completion.docs/ops/SYNC_DEPENDENCY_CAMPAIGN.md; mark local watcher changes as an operator follow-up, not repository implementation.Acceptance criteria
.github/scripts/__tests__/sync_dependency_campaign.test.jsproves the same exception observed on three consecutive runs creates one claim generation and one agent handoff..github/scripts/__tests__/sync_pr_merge_contract.test.jsapplies identical merge/review/check rules to sync and dev-tool-sync fixtures.updated_at, verify the campaign test detects duplicate handoffs, then revert and verify it passes.python scripts/dev_check.py --action testand workflow validation successfully.Summary by CodeRabbit
New Features
Documentation