feat(sync): gate fleet fan-out on consumer canaries - #2886
Conversation
stranske
added
agent:codex
stranske
added
agent:retry
|
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: 44 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 (10)
📝 WalkthroughWalkthroughMaint 68 now supports plan-bound preview, canary, and promote phases. Maint 71 publishes canary evidence with check and review-thread status. The PR also updates branch protection enforcement and structured response parsing. ChangesConsumer synchronization phases
Branch protection policy controls
Structured verifier responses
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant Maint68
participant PhaseSelector
participant Maint71
participant ConsumerRepos
Operator->>Maint68: dispatch preview or canary phase
Maint68->>PhaseSelector: compile plan and select repositories
PhaseSelector-->>Maint68: return plan-bound selection
Maint68->>ConsumerRepos: create selected sync PRs
Maint71->>ConsumerRepos: inspect checks and review threads
Maint71-->>Operator: publish canary evidence
Operator->>Maint68: dispatch promote with canary evidence
Maint68->>PhaseSelector: validate matching plan and evidence
PhaseSelector-->>Maint68: return non-canary selection
Maint68->>ConsumerRepos: create remaining sync PRs
Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
GitHub Actions
Inactive
|
Runner dispatch state for codex on PR #2886. Do not edit. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7610a0cece
ℹ️ 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.
Pull request overview
Adds a phase-aware, plan-bound gating mechanism to consumer sync fan-out so Maint 68 defaults to canary-only PR creation and requires explicit, plan-matching canary evidence (from Maint 71) before promoting to the rest of the fleet.
Changes:
- Introduces
preview/canary/promotephases and a canary config to gate Maint 68’s repo matrix selection. - Adds
scripts/select_consumer_sync_phase.pyplus tests to enforce plan-bound promotion and canary evidence requirements. - Extends Maint 71 to emit machine-readable canary evidence (including required-check state and review-thread debt), and updates ops/docs to describe the new two-run flow.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/workflows/test_sync_manifest_delivery.py |
Adds workflow-level contract tests for phase defaults/options, gating behavior, and presence of canary config + evidence fields. |
tests/scripts/test_select_consumer_sync_phase.py |
Adds unit coverage for phase selection behavior and promotion rejection conditions. |
scripts/select_consumer_sync_phase.py |
Implements the phase selector that produces the repo matrix plus auditable selection/prospective evidence. |
docs/WORKFLOW_GUIDE.md |
Updates workflow catalog entries to document canary-gated sync + canary evidence emission. |
docs/ops/CONSUMER_REPO_MAINTENANCE.md |
Documents the operational two-run sequence (Maint 68 canary → Maint 71 evidence → Maint 68 promote). |
config/consumer_sync_canaries.json |
Adds the canonical 2–3 repo canary set and capability tags. |
.github/workflows/maint-71-merge-sync-prs.yml |
Emits sync-canary-evidence.json (plan-bound) including required-check state + active review thread count. |
.github/workflows/maint-68-sync-consumer-repos.yml |
Adds phase inputs, uses the selector to build a phase-gated matrix, records plan/selection artifacts, and annotates PR metadata with plan/phase. |
Suppressed comments (1)
scripts/select_consumer_sync_phase.py:157
- In
promote, the selector currently returns every non-canary repo fromregistered_repos(even if the plan results in no changes for that repo). This conflicts with the acceptance criteria in the PR description (unchanged/skipped repos should never enter the matrix). Implementing "affected-only" selection likely requires incorporating per-repo prospective-diff/shadow evidence rather than just the global plan entries/removals.
raise PhaseSelectionError("promotion_rejected:" + ",".join(reasons))
selected = [repo for repo in registered_repos if repo not in canary_repos]
return {
|
Runner dispatch state for autofix on PR #2886. Do not edit. |
|
Autofix updated these files:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/scripts/test_select_consumer_sync_phase.py (1)
1-106: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winFix Black formatting to unblock CI.
The Selftest CI pipeline reports: "Black formatting check failed... Run 'black --line-length 100 tests/scripts/test_select_consumer_sync_phase.py' to format this file." Run Black on this file before merge.
🤖 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/scripts/test_select_consumer_sync_phase.py` around lines 1 - 106, Run Black with a line length of 100 on the test file containing the select_phase tests, ensuring formatting passes CI without changing test behavior.Source: Pipeline failures
.github/workflows/maint-68-sync-consumer-repos.yml (1)
1-1: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winStale artifact name break and missing regression coverage share one root cause. The "prepare" job's upload-artifact step was renamed to
sync-plan-and-prospective-diffs, but the "sync" job's own download step was not updated to match, and no test asserts producer/consumer artifact-name consistency within the workflow.
.github/workflows/maint-68-sync-consumer-repos.yml#L152-159: update the "sync" job's "Download manifest" step (currentlyname: sync-manifest, shown at lines 280-283) toname: sync-plan-and-prospective-diffsso it matches the renamed upload.tests/workflows/test_sync_manifest_delivery.py#L212-230: add an assertion that parsesjobs.sync.steps'download-artifactwith.nameand confirms it equalsjobs.prepare.steps'upload-artifactwith.name, so a future rename desync fails CI immediately.🤖 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 @.github/workflows/maint-68-sync-consumer-repos.yml at line 1, Update the sync job’s “Download manifest” step to use the artifact name produced by the prepare job’s renamed upload step, `sync-plan-and-prospective-diffs`, instead of `sync-manifest`. In `test_sync_manifest_delivery.py`, add regression coverage that compares the sync job’s download-artifact `with.name` against the prepare job’s upload-artifact `with.name` and fails when they differ.
🤖 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/workflows/maint-68-sync-consumer-repos.yml:
- Around line 148-150: Update the “Build phase-gated repo matrix” step to emit
the resolved phase output as phase=$phase, then remove the separate “Record
selected phase” step and its duplicate inputs.phase default expression. Preserve
the existing phase output contract consumed by the sync job and consumer PR
environment.
- Around line 152-159: Update the sync job’s artifact download step to request
the sync-plan-and-prospective-diffs artifact instead of sync-manifest, matching
the artifact name uploaded by the prepare job.
- Around line 122-146: Update the “Build phase-gated repo matrix” step to expose
inputs.repos through a REPOS_INPUT environment variable, then use "$REPOS_INPUT"
for both the non-empty check and repos assignment instead of interpolating
inputs.repos directly in the shell. Leave the constrained inputs.phase handling
unchanged.
In @.github/workflows/maint-71-merge-sync-prs.yml:
- Around line 267-300: Validate the pull request author against the configured
trusted sync-bot identity in both selectActiveSyncPr and classifySyncPrChecks.
Reject any sync/workflows-* PR whose pr.user does not match before selecting it,
merging it, or recording canary evidence, while preserving the existing behavior
for authorized sync PRs.
In `@tests/scripts/test_select_consumer_sync_phase.py`:
- Around line 58-86: Add a promotion test alongside
test_promotion_rejects_stale_canary_evidence that removes one canary row from
otherwise valid green_evidence, calls select_phase with phase="promote" and the
existing registered repositories and canaries, and asserts PhaseSelectionError
matches "missing_canary_evidence".
In `@tests/workflows/test_sync_manifest_delivery.py`:
- Around line 212-230: Extend
test_sync_fanout_is_canary_gated_and_promotion_is_plan_bound to inspect
jobs.sync.steps and jobs.prepare.steps, locate the download-artifact and
upload-artifact steps respectively, and assert the consumer’s with.name equals
the producer’s with.name. Replace reliance on the broad source substring check
with this direct artifact-name consistency assertion while preserving the
existing workflow phase checks.
---
Outside diff comments:
In @.github/workflows/maint-68-sync-consumer-repos.yml:
- Line 1: Update the sync job’s “Download manifest” step to use the artifact
name produced by the prepare job’s renamed upload step,
`sync-plan-and-prospective-diffs`, instead of `sync-manifest`. In
`test_sync_manifest_delivery.py`, add regression coverage that compares the sync
job’s download-artifact `with.name` against the prepare job’s upload-artifact
`with.name` and fails when they differ.
In `@tests/scripts/test_select_consumer_sync_phase.py`:
- Around line 1-106: Run Black with a line length of 100 on the test file
containing the select_phase tests, ensuring formatting passes CI without
changing test behavior.
🪄 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: b7f2d8d9-5b45-4c9e-b1c4-cf18b4968872
📒 Files selected for processing (8)
.github/workflows/maint-68-sync-consumer-repos.yml.github/workflows/maint-71-merge-sync-prs.ymlconfig/consumer_sync_canaries.jsondocs/WORKFLOW_GUIDE.mddocs/ops/CONSUMER_REPO_MAINTENANCE.mdscripts/select_consumer_sync_phase.pytests/scripts/test_select_consumer_sync_phase.pytests/workflows/test_sync_manifest_delivery.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/scripts/test_select_consumer_sync_phase.py (1)
41-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert exact repository coverage for prospective diffs.
len(result["prospective_diffs"]) == len(REGISTERED)can pass when one repository is omitted and another is duplicated. Assert that the prospective-diff repository set equalsREGISTERED.Proposed test assertion
assert len(result["prospective_diffs"]) == len(REGISTERED) + assert { + item["repo"] for item in result["prospective_diffs"] + } == set(REGISTERED)As per path instructions,
**/*.pyfiles must prioritize correctness, error handling, and test coverage.🤖 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/scripts/test_select_consumer_sync_phase.py` around lines 41 - 46, Strengthen the assertions in the select_phase test by verifying that the repository set in result["prospective_diffs"] exactly matches REGISTERED, rather than relying only on equal lengths. Retain the existing assertions and compare repository identifiers from the prospective diffs to REGISTERED so omissions and duplicates cannot pass.Source: Path instructions
🤖 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.
Outside diff comments:
In `@tests/scripts/test_select_consumer_sync_phase.py`:
- Around line 41-46: Strengthen the assertions in the select_phase test by
verifying that the repository set in result["prospective_diffs"] exactly matches
REGISTERED, rather than relying only on equal lengths. Retain the existing
assertions and compare repository identifiers from the prospective diffs to
REGISTERED so omissions and duplicates cannot pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6df960f3-741e-47c6-ab79-36b82fb1105b
📒 Files selected for processing (2)
tests/scripts/test_select_consumer_sync_phase.pytests/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. |
|
Resolved the current review set in 5ad595e. The Maint 68 matrix now routes manual repos through an environment variable, preserves explicit filtered manual runs while validating canaries against the full registry, emits one resolved phase output, and uses one upload/download artifact name. Promotion rejects missing or duplicate canary evidence; Maint 71 fails closed on review-thread pagination and only accepts trusted sync actors. Updated the promotion wording to match all non-canary fan-out. Validation: 17 focused pytest cases, 15 Node contract tests, Ruff check/format, YAML parsing, and git diff --check all passed. |
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/select_consumer_sync_phase.py (1)
131-166: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject repository subsets during
promote.When a manual run supplies
repos, Maint 68 passes it as--selected-reposfor every phase. This code then builds the promotion matrix from that subset. A green promotion can therefore sync only one non-canary repository, while the documented contract requires all registered non-canary repositories.Reject
selected_reposforpromote, unless a separate audited override defines scoped promotion behavior. Add a regression test for this rejection.Proposed fix
canary_repos = [item["repo"] for item in canaries] if selected_repos is not None and not set(selected_repos) <= set(registered_repos): raise PhaseSelectionError("selected_repos_must_be_registered") + if phase == "promote" and selected_repos is not None: + raise PhaseSelectionError("selected_repos_not_allowed_for_promote") target_repos = selected_repos if selected_repos is not None else registered_repos🤖 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 `@scripts/select_consumer_sync_phase.py` around lines 131 - 166, Update the promotion branch in the phase-selection function to reject any non-None selected_repos before constructing the promotion matrix, using the existing PhaseSelectionError mechanism and a clear rejection reason. Preserve subset handling for preview and canary, while ensuring promote always targets all registered non-canary repositories. Add a regression test covering promote with selected_repos.
🤖 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 `@scripts/langchain/pr_verifier.py`:
- Around line 682-692: Update the structured repair normalization around the
content-handling logic and its empty check so a list containing a valid text
block returns the concatenated text even when it is empty, while lists without
text blocks retain existing behavior; then treat the normalized empty string as
None before JSON serialization. Add a regression test covering a repair response
such as [{"type": "text", "text": ""}] and verify it is handled as empty rather
than parsed as JSON.
In `@tests/scripts/test_pr_verifier_structured_output.py`:
- Around line 86-166: Add a test alongside the existing ComparisonRunner
structured-response tests that sets response.content directly to
_valid_payload() rather than a list of content blocks, then runs
ComparisonRunner.run_single and asserts a PASS verdict plus JSON-normalized
raw_content. Ensure this exercises the fallback coercion path in the verifier
and documents the changed behavior.
In `@tests/scripts/test_select_consumer_sync_phase.py`:
- Around line 122-131: Add a test alongside
test_filtered_manual_canary_run_preserves_requested_repositories that passes an
unregistered repository in selected_repos, asserts select_phase raises
PhaseSelectionError, and retains the existing accepted-selection test unchanged.
In `@tools/enforce_gate_branch_protection.py`:
- Around line 32-39: Update the --context argument help text to document
“summary” as the default instead of “Gate / gate”, while preserving the existing
DEFAULT_CONTEXTS value and argument behavior.
- Line 821: The snapshot generation in tools/enforce_gate_branch_protection.py
must use desired_strict for both desired.strict assignments, including the
assignment near line 821, so --allow-non-strict reports strict as false. Add a
--snapshot --allow-non-strict test in
tests/tools/test_enforce_gate_branch_protection.py covering the relevant
snapshot flow and assert snapshot["desired"]["strict"] is False.
---
Outside diff comments:
In `@scripts/select_consumer_sync_phase.py`:
- Around line 131-166: Update the promotion branch in the phase-selection
function to reject any non-None selected_repos before constructing the promotion
matrix, using the existing PhaseSelectionError mechanism and a clear rejection
reason. Preserve subset handling for preview and canary, while ensuring promote
always targets all registered non-canary repositories. Add a regression test
covering promote with selected_repos.
🪄 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: 74e3882f-1151-445e-a12a-a2f6d82cea7e
📒 Files selected for processing (16)
.github/config/required-contexts.json.github/scripts/__tests__/sync-pr-merge-contract.test.js.github/scripts/sync_pr_merge_contract.js.github/workflows/health-40-repo-selfcheck.yml.github/workflows/health-44-gate-branch-protection.yml.github/workflows/maint-68-sync-consumer-repos.yml.github/workflows/maint-71-merge-sync-prs.ymldocs/ops/CONSUMER_REPO_MAINTENANCE.mdscripts/langchain/pr_verifier.pyscripts/select_consumer_sync_phase.pytestgen_gate.pytests/scripts/test_pr_verifier_structured_output.pytests/scripts/test_select_consumer_sync_phase.pytests/tools/test_enforce_gate_branch_protection.pytests/workflows/test_sync_manifest_delivery.pytools/enforce_gate_branch_protection.py
💤 Files with no reviewable changes (1)
- testgen_gate.py
select_phase rejects a --selected-repos set that is not a subset of the registry, but nothing exercised that branch, so a manual filtered run could have become a fan-out escape hatch without failing a test. Co-authored-by: Cursor <cursoragent@cursor.com>
893eadf to
aa42a99
Compare
GitHub Actions
Inactive
Closer review-recovery: the branch was rebased onto a stale main, which is why the review went sidewaysThe structural problem, first. Before this round the branch's merge-base with That is what sent CodeRabbit's 18:33Z review off-target: four of its five findings were against code that had already merged. Rebased onto current The one in-scope finding is fixed, not waved off. CodeRabbit was right that
Deliberate-break demonstration — deleting the two-line guard from Only the new test failed. Source restored → 9 passed. Validation: The four out-of-scope findings were not discarded. They are real defects in merged
Resolving the two still-open threads here on that basis: their files are no longer in this PR's diff, and the fixes are in #2890. |
GitHub Actions
Inactive
Provider Comparison ReportProvider Summary
📋 Full Provider Details (click to expand)openai
anthropic
Agreement
Disagreement
Unique Insights
🔍 LangSmith Traces |
Closer verifier disposition — anthropic CONCERNS audited as truncated-diff false positiveReport: Provider Comparison on merged head ( Anthropic claims checked against live
Disposition: no bounded follow-up from this CONCERNS report. Source issue |
Closes #2879
Automated Status Summary
Scope
maint-68-sync-consumer-repos.yml:103-128compiles one plan and immediately builds the full registered-repo matrix. Health 69 produces a typed, non-authorizing shadow handoff (.github/workflows/health-69-consumer-sync-shadow-evidence.yml:48-88), but no workflow uses representative consumer results as a promotion gate before the remaining fleet receives PRs. During the setup-python v7 delivery, source defects discovered in consumer review caused four 12-repo waves. This is a current sequencing defect: fleet PRs are used as the first compatibility environment.Tasks
config/consumer_sync_canaries.jsondefining 2-3 representative repos and documented capability tags such as lock-heavy, custom-gate, and standard consumer.scripts/select_consumer_sync_phase.pyto consume the compiled plan, requested phase, registered repos, and prior canary evidence and return an affected repo matrix..github/workflows/maint-68-sync-consumer-repos.ymlwithpreview,canary, andpromoteinputs; an unfiltered normal run must start atcanary.scripts/build_consumer_sync_shadow_handoff.pyand Health 69 artifacts to record plan ID, per-repo desired hash, affected paths, and non-authorizing preview evidence.tests/scripts/test_select_consumer_sync_phase.pyand extendtests/workflows/test_sync_manifest_delivery.pyfor phase/matrix contracts.docs/ops/CONSUMER_REPO_MAINTENANCE.mdanddocs/WORKFLOW_GUIDE.mdwith the no-sleep, two-run promotion sequence and emergency override.Acceptance criteria
python -m pytest tests/scripts/test_select_consumer_sync_phase.py tests/workflows/test_sync_manifest_delivery.py tests/scripts/test_consumer_sync_shadow_handoff.py -qpasses.promoterejects canary evidence with a different plan ID, a failed required check, or active non-outdated review debt.promotetargets only affected non-canary repos; unchanged/skipped repos never enter the matrix.tests/scripts/test_select_consumer_sync_phase.py::test_promotion_rejects_stale_canary_evidencemust fail the promotion decision. Restore the fixture before review.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests