fix(branch-protection): assert the reviewed policy instead of an aspirational one - #2888
Conversation
…rational one health-40 and health-44 asserted a branch-protection policy the repo does not have and does not intend to have, so the drift check failed every week and, in health-40, took the Root allowlist guard and the aggregate step down with it. The asserted policy was wrong on both axes: 1. Contexts. It demanded "Gate / gate". That is a commit STATUS posted by the `summary` job of pr-00-gate.yml (:1039-1079) — the same verdict as the `summary` check-run, which is already the required context. `summary` is Gate's aggregate job: it `needs` all ten Gate jobs with `if: always()`, so requiring it gates all of Gate. Verified on db4e091 (a commit with a real test failure): check-run `summary` = failure, i.e. protection does catch genuine Gate failures. Requiring the status too would add no safety and would add false blocks — it was left `pending` on db4e091, and showed stale `failure` on all 13 sync PRs today after cancelled concurrent runs. A required context stuck pending blocks a PR permanently. 2. Strictness. It demanded "require branches up to date", which the tool wanted unconditionally (it was hardcoded, --require-strict only governed the *unknown* case). Workflows merges 11-13 PRs/day with no merge queue and allow_update_branch=false, so strict would strand PRs behind base needing manual updates. That is a deliberate policy choice, not drift. Point the required-contexts config and the tool default at `summary`, and add --allow-non-strict so a deliberately non-strict policy can be asserted. The default is unchanged: without the flag, non-strict is still drift. Result: the same invocation health-44 runs now exits 0 ("No changes required") against the live ruleset, with no admin action. Refs #2858. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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: 41 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 (5)
Comment |
GitHub Actions
Inactive
Workflow source neededPR #2888 needs either a linked GitHub issue or one valid non-issue Workflow Source before PR metadata automation can manage it safely. Please do one of:
Once a valid source is present, this warning will not be reposted. |
There was a problem hiding this comment.
Pull request overview
Aligns the branch-protection enforcement tooling and health workflows with the actual reviewed policy for this repo: require only the Gate aggregate summary context, and allow a deliberately non-strict (“branches up to date” disabled) ruleset without treating it as drift. This reduces false-negative/false-positive drift signals and avoids requiring commit-status contexts that can become stale/pending under churn.
Changes:
- Switch required contexts policy to
["summary"]and document whyGate / gateandHealth 45 Agents Guard / guardmust not be required. - Update
tools/enforce_gate_branch_protection.pydefaults and add--allow-non-strict, threading it through check/apply/bootstrap paths and rejecting contradiction with--require-strict. - Update health-40 / health-44 workflows to assert
summaryand pass--allow-non-strict; add tests for the new flag and default behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/enforce_gate_branch_protection.py | Changes default required context to summary; adds --allow-non-strict and adjusts strict drift/apply behavior accordingly. |
| tests/tools/test_enforce_gate_branch_protection.py | Adds coverage for --allow-non-strict, default drift behavior, and flag conflict. |
| .github/workflows/health-44-gate-branch-protection.yml | Uses --allow-non-strict in enforcement/verification runs and stops requiring strict in verification. |
| .github/workflows/health-40-repo-selfcheck.yml | Updates asserted context to summary and passes --allow-non-strict for apply/check. |
| .github/config/required-contexts.json | Sets required contexts to summary and expands rationale in _note. |
| strict_change = desired_strict and current_state.strict is False | ||
|
|
||
| if snapshot is not None: | ||
| snapshot["desired"] = {"strict": True, "contexts": list(target_contexts)} |
![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: ce8cc030d5
ℹ️ 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".
| { | ||
| "required_contexts": [ | ||
| "Gate / gate" | ||
| "summary" |
There was a problem hiding this comment.
Once Health 44 applies this configuration, the scheduled Health 41 workflow still hard-codes expectedContexts = ['Gate / gate'] in .github/workflows/health-41-repo-health.yml:411 and fails on missing or unexpected contexts at lines 485-530. Consequently, either the intended summary-only rule or a transitional --no-clean rule containing both contexts causes the weekly health workflow to fail and instructs operators to restore the context rejected here. Update that auditor and the branch-protection contract documentation alongside this switch.
AGENTS.md reference: AGENTS.md:L62-L65
Useful? React with 👍 / 👎.
| strict_change = desired_strict and current_state.strict is False | ||
|
|
||
| if snapshot is not None: | ||
| snapshot["desired"] = {"strict": True, "contexts": list(target_contexts)} |
There was a problem hiding this comment.
When --allow-non-strict encounters an existing rule with strict=True, strict_change is false: if contexts already match, the tool reports no changes and preserves strict mode, but if a context also needs updating, update_status_checks(..., strict=False) disables it. The snapshot simultaneously hard-codes the desired value as True. Thus the resulting policy depends on unrelated context drift and the audit artifact contradicts the printed target; compare the known strict state with desired_strict and record that same target in the snapshot.
Useful? React with 👍 / 👎.
Automated Status SummaryHead SHA: c98c740
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
|
…rational one (#2888) health-40 and health-44 asserted a branch-protection policy the repo does not have and does not intend to have, so the drift check failed every week and, in health-40, took the Root allowlist guard and the aggregate step down with it. The asserted policy was wrong on both axes: 1. Contexts. It demanded "Gate / gate". That is a commit STATUS posted by the `summary` job of pr-00-gate.yml (:1039-1079) — the same verdict as the `summary` check-run, which is already the required context. `summary` is Gate's aggregate job: it `needs` all ten Gate jobs with `if: always()`, so requiring it gates all of Gate. Verified on db4e091 (a commit with a real test failure): check-run `summary` = failure, i.e. protection does catch genuine Gate failures. Requiring the status too would add no safety and would add false blocks — it was left `pending` on db4e091, and showed stale `failure` on all 13 sync PRs today after cancelled concurrent runs. A required context stuck pending blocks a PR permanently. 2. Strictness. It demanded "require branches up to date", which the tool wanted unconditionally (it was hardcoded, --require-strict only governed the *unknown* case). Workflows merges 11-13 PRs/day with no merge queue and allow_update_branch=false, so strict would strand PRs behind base needing manual updates. That is a deliberate policy choice, not drift. Point the required-contexts config and the tool default at `summary`, and add --allow-non-strict so a deliberately non-strict policy can be asserted. The default is unchanged: without the flag, non-strict is still drift. Result: the same invocation health-44 runs now exits 0 ("No changes required") against the live ruleset, with no admin action. Refs #2858. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Three CodeRabbit findings arrived at 18:33Z against code that had already merged (#2887, #2888), so they surfaced on an unrelated open PR's diff instead of anywhere actionable. - pr_verifier: a repair reply of only thinking/metadata blocks was truthy, so the serialized envelope was returned as repair text and the parser failed on JSON the provider never sent. The repair path is now stricter than the parse path: no text blocks, or blank text, means no repair. The parse path keeps its json.dumps fallback for diagnostics. - enforce_gate_branch_protection: two of the three snapshot sites hardcoded strict: True, so --snapshot --allow-non-strict wrote a health artifact claiming strict enforcement that --apply would not create. - enforce_gate_branch_protection: the --context help still advertised 'Gate / gate' as the default; DEFAULT_CONTEXTS is 'summary', and #2858 is specifically about not requiring 'Gate / gate'. Co-authored-by: Cursor <cursoragent@cursor.com>
) * fix: close the review findings that landed after their PRs merged Three CodeRabbit findings arrived at 18:33Z against code that had already merged (#2887, #2888), so they surfaced on an unrelated open PR's diff instead of anywhere actionable. - pr_verifier: a repair reply of only thinking/metadata blocks was truthy, so the serialized envelope was returned as repair text and the parser failed on JSON the provider never sent. The repair path is now stricter than the parse path: no text blocks, or blank text, means no repair. The parse path keeps its json.dumps fallback for diagnostics. - enforce_gate_branch_protection: two of the three snapshot sites hardcoded strict: True, so --snapshot --allow-non-strict wrote a health artifact claiming strict enforcement that --apply would not create. - enforce_gate_branch_protection: the --context help still advertised 'Gate / gate' as the default; DEFAULT_CONTEXTS is 'summary', and #2858 is specifically about not requiring 'Gate / gate'. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(gate): keep already-strict snapshot target under allow-non-strict --allow-non-strict accepts a non-strict floor; an already-strict policy is still in sync. Snapshot desired.strict now mirrors that accepted current value so health_summarize does not render "✅ In sync" beside a True → False transition. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(gate): wire already-strict snapshot target under allow-non-strict Implements the snapshot_desired_strict path the prior commit's regression test expects, so health artifacts stay consistent when an already-strict policy is accepted under --allow-non-strict. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(gate): preserve already-strict on apply and cover review gaps Pass the effective strict target into update_status_checks under --allow-non-strict, and add the split-text, fetch-error, and apply context-drift regressions CodeRabbit flagged on #2890. Co-authored-by: Cursor <cursoragent@cursor.com> * chore(autofix): formatting/lint --------- Co-authored-by: Codex Automation <codex-automation@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Resolves the code side of #2858 — with no admin action needed. The investigation changed the conclusion, so the reasoning matters more than the diff.
summaryalready gates all of Gatesummaryis the aggregate job ofpr-00-gate.yml: itneedsall ten Gate jobs withif: always(). It is already the one required context.Verified on
db4e091— a commit where I had genuinely broken a docs test:So required-checks protection does catch real Gate failures. My original claim on #2858 that "a PR can merge with Gate red" was wrong: what I saw was the mirror status going stale, not the gate being absent.
Why
Gate / gatemust not be requiredIt is a commit status posted by that same
summaryjob (pr-00-gate.yml:1039-1079, step "Report Gate commit status", state fromsteps.summarize.outputs.state). Same verdict, second representation — so requiring it adds no safety, and it actively misfires:pendingondb4e091. A required context stuck pending blocks a PR forever.failureon all 13 sync PRs today after cancelled concurrent Gate runs, whilesummarywas green. Requiring it would have blocked every one of them.Across the last 12 PR heads the two agree perfectly — they diverge only under churn, which is exactly when a false block hurts most. This is the second footgun of this shape in #2858, after
Health 45 Agents Guard / guard.Why non-strict is a policy, not drift
The tool wanted "require branches up to date" unconditionally — it was hardcoded, and
--require-strictonly governed the unknown case. But Workflows merges 11-13 PRs/day, has no merge queue, andallow_update_branch=false, so strict would strand PRs behind base needing manual "Update branch". That is a deliberate choice.Changes
.github/config/required-contexts.json→["summary"], with the reasoning inline so neither rejected context gets re-added. (This corrects my own fix(branch-protection): stop requiring a conditionally-posted status context #2872, which set it toGate / gate.)tools/enforce_gate_branch_protection.py→DEFAULT_CONTEXTS = ("summary",); new--allow-non-strictso a deliberately non-strict policy can be asserted. Threaded through the check path, the snapshot, the apply path and the bootstrap path, so--applycan no longer silently enable strict against the stated policy. Contradicting it with--require-strictis aparser.error.health-40andhealth-44→ assertsummary, pass--allow-non-strict, drop--require-strict.Default behaviour is unchanged: without the flag, a non-strict policy is still drift.
Result
The exact invocation health-44 runs, against the live ruleset:
That clears the weekly health-40 failure that had been skipping the Root allowlist guard and Aggregate steps for a month.
Test gate
test_allow_non_strict_accepts_a_deliberately_non_strict_policy,test_without_allow_non_strict_a_non_strict_policy_is_still_drift(pins the default),test_allow_non_strict_conflicts_with_require_strict.Deliberate break → revert: hardcoding
desired_strict = Truemakes the first test FAIL; restoring it returns 18/18.pytest tests/tools/ tests/docs/ + both enforce-gate suites + post_ci_summary→ 442 passed. ruff clean.Not done, deliberately
Enabling strict — worthwhile only alongside a merge queue, which is its own piece of work given how it interacts with the agent fleet's auto-merge and maint-71.
🤖 Generated with Claude Code