fix(sync): classify covered consumer drift - #2885
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: 23 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 (1)
📝 WalkthroughWalkthroughHealth 68 now classifies consumer drift as converged, covered, or actionable drift. It matches current sync PR branches and leases, updates report data and exit handling, suppresses covered comments, documents the states, and triggers after successful merge-sync runs. ChangesHealth 68 drift classification
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
GitHub Actions
Inactive
|
Runner dispatch state for codex on PR #2885. Do not edit. |
![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: 60a2250f58
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/check_consumer_sync_drift.py (1)
967-977: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDistinguish the
coveredsuccess message fromconverged.A
coveredrun exits 0 and prints"Consumer repos are in sync.". Drift exists in that state, and an open sync PR carries the remediation. The log then hides an actionable-adjacent condition. Print the status and the covered repository count so scheduled-run logs stay diagnostic.♻️ Proposed change to differentiate the success paths
if report["status"] not in {"converged", "covered"}: print("::warning::Consumer repo drift detected") return 1 - print("Consumer repos are in sync.") + if report["status"] == "covered": + print("Consumer repo drift is covered by current sync PRs.") + else: + print("Consumer repos are in sync.") return 0🤖 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/check_consumer_sync_drift.py` around lines 967 - 977, Update the final status handling around report["status"] so the converged and covered success paths use distinct messages. Preserve exit code 0 for covered runs, but include the status and covered repository count in that message; keep the existing drift warning and nonzero return for other statuses.tests/scripts/test_check_consumer_sync_drift.py (1)
180-202: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThree of the five remediation states have no assertions.
build_remediation_statesnow producesconverged,covered,stale,blocked, anduntracked_drift. The supplied tests assert onlyconverged,covered, anduntracked_drift. The unasserted branches are the ones that decide a nonzero exit, so a regression in them would pass CI.
tests/scripts/test_check_consumer_sync_drift.py#L180-L202: add cases for a branch that does not match the plan-derived branch, for anupdated_atolder than the 36-hour lease, and for a repository listed insync_pr_lookup_errors.scripts/check_consumer_sync_drift.py#L448-L484: keep theblockedandstalebranches as written, and confirm the new tests reach both, including the"errors" in categoriespath.As per path instructions: "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_check_consumer_sync_drift.py` around lines 180 - 202, Add tests covering the remaining remediation states in test_build_report_marks_current_sync_pr_as_covered: a non-matching branch must produce stale, an updated_at older than the 36-hour lease must produce stale, and a repository in sync_pr_lookup_errors must produce blocked through the "errors" in categories path. Keep the existing blocked and stale logic in build_remediation_states unchanged in scripts/check_consumer_sync_drift.py; the sibling site requires no direct code change beyond confirming these tests exercise both branches.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.
Inline comments:
In @.github/scripts/__tests__/consumer-sync-drift-issue-body.test.js:
- Around line 108-110: Extend the existing formatIssueComment test coverage
around formatIssueBody to assert rendering for covered status, expected_branch,
and coverage_lease_hours, while also asserting the actionable output remains
unchanged. Reuse the existing report fixture and expected body conventions
rather than introducing unrelated test setup.
In @.github/workflows/health-68-consumer-sync-drift.yml:
- Line 16: Update the Health 68 schedule and coordination so it explicitly waits
for Maint 71 janitor completion rather than relying on the 05:40 cron offset;
use a workflow completion signal if supported, otherwise validate and enforce
that the janitor’s worst-case duration fits within the interval before running
the drift check.
In `@docs/ops/CONSUMER_REPO_MAINTENANCE.md`:
- Around line 16-24: The “Drift coverage states” paragraph omits documented
failure conditions. Update it to explicitly include lookup-error and expired
coverage as actionable failures, clarifying that expired coverage is represented
by stale and lookup-error is a separate failure reason while preserving the
existing state and exit behavior.
In `@scripts/check_consumer_sync_drift.py`:
- Around line 548-554: Update the status calculation around remediation_states
so repo-agnostic errors and an empty repository set produce a failing “drift”
status, while preserving “covered” for repository-attributed covered states and
the existing “converged” result only when all repositories are validly
converged. Use the existing report/error data symbols to detect unattributed
errors before applying the state_values classification.
---
Outside diff comments:
In `@scripts/check_consumer_sync_drift.py`:
- Around line 967-977: Update the final status handling around report["status"]
so the converged and covered success paths use distinct messages. Preserve exit
code 0 for covered runs, but include the status and covered repository count in
that message; keep the existing drift warning and nonzero return for other
statuses.
In `@tests/scripts/test_check_consumer_sync_drift.py`:
- Around line 180-202: Add tests covering the remaining remediation states in
test_build_report_marks_current_sync_pr_as_covered: a non-matching branch must
produce stale, an updated_at older than the 36-hour lease must produce stale,
and a repository in sync_pr_lookup_errors must produce blocked through the
"errors" in categories path. Keep the existing blocked and stale logic in
build_remediation_states unchanged in scripts/check_consumer_sync_drift.py; the
sibling site requires no direct code change beyond confirming these tests
exercise both branches.
🪄 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: 6fc819b9-cd94-42b5-8e94-139ffd594588
📒 Files selected for processing (6)
.github/scripts/__tests__/consumer-sync-drift-issue-body.test.js.github/scripts/consumer_sync_drift_issue_body.js.github/workflows/health-68-consumer-sync-drift.ymldocs/ops/CONSUMER_REPO_MAINTENANCE.mdscripts/check_consumer_sync_drift.pytests/scripts/test_check_consumer_sync_drift.py
There was a problem hiding this comment.
Pull request overview
This PR updates the consumer-sync drift health signal (Health 68) to distinguish actionable drift from drift that is already covered by a current compiler-plan sync PR, reducing false failures and tracker noise.
Changes:
- Add per-repo remediation classification (
converged/covered/blocked/untracked_drift/stale) and compute top-levelstatusasconverged/covered/drift, with exit-0 for fullyconvergedorcovered. - Move the scheduled Health 68 run later (05:40 UTC) to occur after the janitor window.
- Update durable-tracker formatting/tests and maintenance docs to reflect the new “covered drift” semantics.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/check_consumer_sync_drift.py |
Adds sync-plan-aware remediation states and changes status/exit-code behavior to treat fully covered drift as success. |
tests/scripts/test_check_consumer_sync_drift.py |
Updates expectations for new status/sync_remediation shape and adds a covered-state fixture. |
.github/workflows/health-68-consumer-sync-drift.yml |
Reschedules the daily run to 05:40 UTC (post-janitor). |
.github/scripts/consumer_sync_drift_issue_body.js |
Updates issue body wording for covered drift and suppresses covered-state comments. |
.github/scripts/__tests__/consumer-sync-drift-issue-body.test.js |
Adds a regression test for covered-state comment suppression. |
docs/ops/CONSUMER_REPO_MAINTENANCE.md |
Documents remediation states and the coverage lease contract. |
🤖 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. |
GitHub Actions
Inactive
|
Addressed all current inline findings in 4651719: trusted same-repo sync provenance; global/empty-input errors now block rather than report convergence; covered reports skip empty tracker comments; schedule now follows successful Maint 71 completion; docs and focused coverage cover coverage lease, stale/untrusted candidates, naive timestamps, and body rendering. Local validation: 29 focused Python tests, 8 Node tests, Ruff, YAML parse, and Actionlint. |
|
Runner dispatch state for autofix on PR #2885. Do not edit. |
|
Autofix updated these files:
|
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 `@tests/scripts/test_check_consumer_sync_drift.py`:
- Around line 206-225: Add a test case for build_report with a registered
repository, no attributed drift (drift=set()), and a non-empty global errors
set; assert that the repository's sync_remediation repo state is "blocked" while
preserving the existing global error assertion.
🪄 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: 17ad6fa6-49a1-43be-a9e1-ed40f018d682
📒 Files selected for processing (6)
.github/scripts/__tests__/consumer-sync-drift-issue-body.test.js.github/workflows/health-68-consumer-sync-drift.yml.github/workflows/maint-71-merge-sync-prs.ymldocs/ops/CONSUMER_REPO_MAINTENANCE.mdscripts/check_consumer_sync_drift.pytests/scripts/test_check_consumer_sync_drift.py
A global comparison error must outrank the "no attributed gaps" branch in build_remediation_states; without a case that has no local drift, an unattributable failure could silently read as converged. Co-authored-by: Cursor <cursoragent@cursor.com>
Closer review-thread audit (post-
|
GitHub Actions
Inactive
GitHub Actions
Inactive
Provider Comparison ReportProvider Summary
📋 Full Provider Details (click to expand)openai
anthropic
Agreement
Disagreement
Unique Insights
🔍 LangSmith Traces |
Closer verifier disposition — split verdict audited; one claim is real and is now fixedBackground: this PR had no verifier report at all until now. The label-triggered run Verdict: openai The anthropic claim that is correct
Verified against the merged commit, not the PR body. This is not a cosmetic omission.
Leaving that text in place would have preserved exactly the false alarm #2878 set out to remove (249 runs, zero successes, 223 tracker comments in 49 days). Fixed in bounded follow-up #2891, which adds the five-state table with exit codes and tracker reach, notes that a quiet tracker no longer proves the workflow is alive, and adds The anthropic claims that do not survive the audit"The issue-body JS/test changes are quite small relative to the requirement to render five distinct states." That is a diff-size observation, not a behavioural one. The openai PASS specifically confirms the deliberate-break gate, the plan/hash-mismatch behaviour, and the current/superseded/blocked/expired/lookup-error fixtures the acceptance criteria named. DispositionSource issue #2878 stays closed: the implementation criteria are satisfied on |
Closes #2878
Automated Status Summary
Scope
scripts/check_consumer_sync_drift.py:420-456already recordspending_sync_prs, butbuild_report()still sets top-level status todrift;main()then returns 1 for every non-converged state (scripts/check_consumer_sync_drift.py:841-860). The workflow runs on source pushes and at 05:10 UTC (.github/workflows/health-68-consumer-sync-drift.yml:6-17), before the 05:30 janitor, and comments on the durable issue for every failure (health-68-consumer-sync-drift.yml:81-201). Live evidence showed 249 runs with zero successes in 49 days and 223 comments on #2210. This is a current observability break: expected, covered propagation is reported as failure.Context for Agent
Related Issues/PRs
Tasks
scripts/check_consumer_sync_drift.py:358-495to parse current sync provenance and classify repo-level remediation states.scripts/sync_manifest_compiler.pyand require an open PR marker/branch to match it before state becomescovered.scripts/check_consumer_sync_drift.py:841-860soconvergedand fullycoveredreports exit zero;blocked,untracked_drift, lookup errors, and expired coverage exit non-zero..github/workflows/health-68-consumer-sync-drift.ymlto run after Maint 71 and invoke issue mutation only for actionable failure states..github/scripts/consumer_sync_drift_issue_body.jsso body/marker/comment output renders the five states and does not append an unchanged covered-state comment.tests/scripts/test_check_consumer_sync_drift.pywith current, superseded, blocked, expired, and lookup-error fixtures..github/scripts/__tests__/consumer-sync-drift-issue-body.test.jswith comment-suppression and actionable-state cases.docs/ops/CONSUMER_REPO_MAINTENANCE.mdanddocs/ops/DURABLE_TRACKING_ISSUES.mdwith the state and SLO contract.Acceptance criteria
python -m pytest tests/scripts/test_check_consumer_sync_drift.py -qandnode --test .github/scripts/__tests__/consumer-sync-drift-issue-body.test.jspass.coveredand process exit 0.coveredfixture;tests/scripts/test_check_consumer_sync_drift.py::test_current_sync_pr_covers_driftmust fail or change the report tountracked_drift. Restore the fixture before review.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores