From ce8cc030d5789c789b9e4c95423a66f2d6d8eade Mon Sep 17 00:00:00 2001 From: Tim Stranske Date: Sat, 1 Aug 2026 12:27:27 -0500 Subject: [PATCH] fix(branch-protection): assert the reviewed policy instead of an aspirational one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/config/required-contexts.json | 4 +- .../workflows/health-40-repo-selfcheck.yml | 7 +-- .../health-44-gate-branch-protection.yml | 4 +- .../test_enforce_gate_branch_protection.py | 52 +++++++++++++++++++ tools/enforce_gate_branch_protection.py | 37 +++++++++---- 5 files changed, 87 insertions(+), 17 deletions(-) diff --git a/.github/config/required-contexts.json b/.github/config/required-contexts.json index 5b6ceeaad..83ccc7669 100644 --- a/.github/config/required-contexts.json +++ b/.github/config/required-contexts.json @@ -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." } diff --git a/.github/workflows/health-40-repo-selfcheck.yml b/.github/workflows/health-40-repo-selfcheck.yml index 6287f6cb0..94aa1b3e9 100644 --- a/.github/workflows/health-40-repo-selfcheck.yml +++ b/.github/workflows/health-40-repo-selfcheck.yml @@ -118,7 +118,8 @@ jobs: python tools/enforce_gate_branch_protection.py \ --apply \ --branch "${DEFAULT_BRANCH}" \ - --context "Gate / gate" \ + --context "summary" \ + --allow-non-strict \ --no-clean - name: Snapshot branch protection state @@ -130,9 +131,9 @@ jobs: run: | python tools/enforce_gate_branch_protection.py \ --check \ - --require-strict \ --branch "${DEFAULT_BRANCH}" \ - --context "Gate / gate" \ + --context "summary" \ + --allow-non-strict \ --no-clean \ --snapshot repo-health-branch-protection.json diff --git a/.github/workflows/health-44-gate-branch-protection.yml b/.github/workflows/health-44-gate-branch-protection.yml index 2a1a85023..0a3706dfe 100644 --- a/.github/workflows/health-44-gate-branch-protection.yml +++ b/.github/workflows/health-44-gate-branch-protection.yml @@ -220,6 +220,7 @@ jobs: python tools/enforce_gate_branch_protection.py \ --apply \ --config .github/config/required-contexts.json \ + --allow-non-strict \ --no-clean \ --snapshot "${SNAPSHOT_DIR}/enforcement.json" @@ -236,8 +237,8 @@ jobs: run: | timeout 5m python tools/enforce_gate_branch_protection.py \ --check \ - --require-strict \ --config .github/config/required-contexts.json \ + --allow-non-strict \ --no-clean \ --snapshot "${SNAPSHOT_DIR}/verification.json" @@ -255,6 +256,7 @@ jobs: python tools/enforce_gate_branch_protection.py \ --check \ --config .github/config/required-contexts.json \ + --allow-non-strict \ --no-clean \ --snapshot "${SNAPSHOT_DIR}/verification.json" diff --git a/tests/tools/test_enforce_gate_branch_protection.py b/tests/tools/test_enforce_gate_branch_protection.py index d8eb91f06..54c76e93a 100644 --- a/tests/tools/test_enforce_gate_branch_protection.py +++ b/tests/tools/test_enforce_gate_branch_protection.py @@ -1,5 +1,6 @@ from __future__ import annotations +from types import SimpleNamespace from typing import Any import pytest @@ -256,3 +257,54 @@ def test_state_from_branch_payload_preserves_explicit_strict_value() -> None: ) assert state == gate.StatusCheckState(strict=False, contexts=["Gate / gate"]) + + +def test_allow_non_strict_accepts_a_deliberately_non_strict_policy(monkeypatch, capsys): + """A non-strict ruleset is not drift when the reviewed policy is non-strict.""" + monkeypatch.setenv("GITHUB_TOKEN", "token") + monkeypatch.setattr( + gate, + "fetch_status_checks", + lambda *a, **k: gate.StatusCheckState(strict=False, contexts=["summary"]), + ) + monkeypatch.setattr(gate, "_build_session", lambda token: SimpleNamespace()) + + exit_code = gate.main( + [ + "--repo", + "octo/repo", + "--check", + "--allow-non-strict", + "--no-clean", + "--context", + "summary", + ] + ) + + out = capsys.readouterr().out + assert exit_code == 0 + assert "No changes required." in out + assert "Desired 'require up to date': False" in out + + +def test_without_allow_non_strict_a_non_strict_policy_is_still_drift(monkeypatch, capsys): + """The default is unchanged: non-strict counts as drift unless opted out.""" + monkeypatch.setenv("GITHUB_TOKEN", "token") + monkeypatch.setattr( + gate, + "fetch_status_checks", + lambda *a, **k: gate.StatusCheckState(strict=False, contexts=["summary"]), + ) + monkeypatch.setattr(gate, "_build_session", lambda token: SimpleNamespace()) + + exit_code = gate.main(["--repo", "octo/repo", "--check", "--no-clean", "--context", "summary"]) + + out = capsys.readouterr().out + assert exit_code == 1 + assert "Would enable 'require branches to be up to date'." in out + + +def test_allow_non_strict_conflicts_with_require_strict(monkeypatch): + monkeypatch.setenv("GITHUB_TOKEN", "token") + with pytest.raises(SystemExit): + gate.main(["--repo", "octo/repo", "--check", "--allow-non-strict", "--require-strict"]) diff --git a/tools/enforce_gate_branch_protection.py b/tools/enforce_gate_branch_protection.py index 5ed589fa5..bcc46dca1 100755 --- a/tools/enforce_gate_branch_protection.py +++ b/tools/enforce_gate_branch_protection.py @@ -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)} @@ -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: