-
Notifications
You must be signed in to change notification settings - Fork 1
fix(branch-protection): assert the reviewed policy instead of an aspirational one #2888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "required_contexts": [ | ||
| "Gate / gate" | ||
| "summary" | ||
| ], | ||
| "_note": "Only universally-posted contexts belong here: health-44 can pass this file to enforce_gate_branch_protection.py --apply, so any context listed becomes a REQUIRED status check. 'Health 45 Agents Guard / guard' is deliberately absent: agents-guard.yml posts that status only when the PR carries an agent label (agent:codex, agents:auto-pilot, ...), so requiring it would leave every other PR permanently un-mergeable. See issue #2858." | ||
| "_note": "`summary` is the Gate workflow's own aggregate job (pr-00-gate.yml): it `needs` every other Gate job with `if: always()`, so requiring it gates all of Gate. Only universally-posted, self-healing contexts belong here, because health-44 can pass this file to enforce_gate_branch_protection.py --apply and anything listed becomes a REQUIRED check. Deliberately absent: 'Gate / gate' (a commit STATUS posted by that same summary job — a duplicate verdict that can be left `pending` or stale-`failure` after cancelled concurrent runs, blocking merges with no real defect) and 'Health 45 Agents Guard / guard' (posted only for agent-labelled PRs). See issue #2858." | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,10 +30,13 @@ def resolve_api_root(explicit: str | None = None) -> str: | |
|
|
||
|
|
||
| DEFAULT_CONTEXTS = ( | ||
| # Only universally-posted contexts may be required. "Health 45 Agents Guard / | ||
| # guard" is posted by agents-guard.yml ONLY for agent-labelled PRs, so requiring | ||
| # it would block every other PR forever (issue #2858). | ||
| "Gate / gate", | ||
| # `summary` is the Gate workflow's aggregate job: it needs every other Gate | ||
| # job, so requiring it gates all of Gate. Do NOT add "Gate / gate" — that is a | ||
| # commit status posted by the same job (duplicate verdict) which can be left | ||
| # pending or stale-failure after cancelled runs, and "Health 45 Agents Guard / | ||
| # guard" is only posted for agent-labelled PRs. Both would block PRs with no | ||
| # real defect (issue #2858). | ||
| "summary", | ||
| ) | ||
|
|
||
| DEFAULT_CONFIG_PATH = Path(".github/config/required-contexts.json") | ||
|
|
@@ -675,6 +678,15 @@ def main(argv: Sequence[str] | None = None) -> int: | |
| action="store_true", | ||
| help="Exit with a non-zero status if changes would be required without applying them.", | ||
| ) | ||
| parser.add_argument( | ||
| "--allow-non-strict", | ||
| action="store_true", | ||
| help=( | ||
| "Do not treat a disabled 'require branches to be up to date' setting as" | ||
| " drift. Use when the reviewed policy is deliberately non-strict (e.g. a" | ||
| " repo with no merge queue, where strict would strand PRs behind base)." | ||
| ), | ||
| ) | ||
| parser.add_argument( | ||
| "--require-strict", | ||
| action="store_true", | ||
|
|
@@ -694,6 +706,8 @@ def main(argv: Sequence[str] | None = None) -> int: | |
| ) | ||
|
|
||
| args = parser.parse_args(argv) | ||
| if args.allow_non_strict and args.require_strict: | ||
| parser.error("--allow-non-strict and --require-strict are contradictory") | ||
|
|
||
| if args.apply and args.check: | ||
| parser.error("--check cannot be combined with --apply.") | ||
|
|
@@ -719,6 +733,7 @@ def main(argv: Sequence[str] | None = None) -> int: | |
| api_root = resolve_api_root(args.api_url) | ||
| token = require_token(args.token) | ||
| session = _build_session(token) | ||
| desired_strict = not args.allow_non_strict | ||
|
|
||
| try: | ||
| current_state = fetch_status_checks(session, args.repo, args.branch, api_root=api_root) | ||
|
|
@@ -735,13 +750,13 @@ def main(argv: Sequence[str] | None = None) -> int: | |
| label = "Target contexts" if args.no_clean else "Desired contexts" | ||
| print(f"{label}: {format_contexts(desired_contexts)}") | ||
| print("Current 'require up to date': False") | ||
| print("Desired 'require up to date': True") | ||
| print(f"Desired 'require up to date': {desired_strict}") | ||
|
|
||
| if snapshot is not None: | ||
| snapshot.update( | ||
| { | ||
| "current": None, | ||
| "desired": {"strict": True, "contexts": list(desired_contexts)}, | ||
| "desired": {"strict": desired_strict, "contexts": list(desired_contexts)}, | ||
| "changes_required": True, | ||
| "require_strict": bool(args.require_strict), | ||
| "strict_unknown": False, | ||
|
|
@@ -757,7 +772,7 @@ def main(argv: Sequence[str] | None = None) -> int: | |
| args.repo, | ||
| args.branch, | ||
| contexts=desired_contexts, | ||
| strict=True, | ||
| strict=desired_strict, | ||
| api_root=api_root, | ||
| ) | ||
| except BranchProtectionError as exc: | ||
|
|
@@ -803,7 +818,7 @@ def main(argv: Sequence[str] | None = None) -> int: | |
|
|
||
| to_add, to_remove = diff_contexts(current_state.contexts, target_contexts) | ||
| strict_is_unknown = current_state.strict is None | ||
| strict_change = current_state.strict is False | ||
| strict_change = desired_strict and current_state.strict is False | ||
|
|
||
| if snapshot is not None: | ||
| snapshot["desired"] = {"strict": True, "contexts": list(target_contexts)} | ||
|
Comment on lines
+821
to
824
Comment on lines
+821
to
824
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
|
|
@@ -821,7 +836,7 @@ def main(argv: Sequence[str] | None = None) -> int: | |
| print("Current 'require up to date': (unknown - supply BRANCH_PROTECTION_TOKEN to verify)") | ||
| else: | ||
| print(f"Current 'require up to date': {current_state.strict}") | ||
| print("Desired 'require up to date': True") | ||
| print(f"Desired 'require up to date': {desired_strict}") | ||
|
|
||
| if strict_is_unknown: | ||
| if args.require_strict: | ||
|
|
@@ -836,7 +851,7 @@ def main(argv: Sequence[str] | None = None) -> int: | |
| "The check will pass, but rerun with BRANCH_PROTECTION_TOKEN to audit." | ||
| ) | ||
|
|
||
| if args.require_strict and strict_is_unknown: | ||
| if args.require_strict and strict_is_unknown and desired_strict: | ||
| strict_change = True | ||
|
|
||
| no_changes_required = not to_add and (args.no_clean or not to_remove) and not strict_change | ||
|
|
@@ -874,7 +889,7 @@ def main(argv: Sequence[str] | None = None) -> int: | |
| args.repo, | ||
| args.branch, | ||
| contexts=target_contexts, | ||
| strict=True, | ||
| strict=desired_strict, | ||
| api_root=api_root, | ||
| ) | ||
| except BranchProtectionError as exc: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:411and fails on missing or unexpected contexts at lines 485-530. Consequently, either the intendedsummary-only rule or a transitional--no-cleanrule 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 👍 / 👎.