From 1e5e0b556e54d02995c4c53f827035f2427d4181 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 19:36:38 +0000 Subject: [PATCH] fix(ci): run PR quality gates on every base branch, not just main A `pull_request.branches:` allowlist filters on the PR's *base* ref. Six gates carried `branches: [main]` (or `[main, develop]`), so a PR stacked onto another PR's branch matched none of them and ran with no CI, no CodeQL, no security scan, no coverage, no e2e and no dependency review. This is not hypothetical. #1440 -- "fix(web): stop /api/transcribe leaking probe status and key config", a non-draft security fix -- targets #1381's branch and has zero CI runs. Its only green checks are the Vercel deployment and CodeRabbit. Every test gate in the repo silently vanished because of the base it happens to target. Dropping the allowlist from the `pull_request` trigger makes each gate fire for every PR regardless of base. The `push` triggers are untouched and stay pinned to main, so branch pushes do not start running the full suite twice. e2e-tests.yml keeps its existing `vars.E2E_BASE_URL` job-level guard, so its PR behaviour is unchanged apart from now being reachable at all. Non-vacuous: tests/unit/test_workflow_pr_gate_coverage.py run against the pre-fix workflows from origin/main gives 6 failed, 12 passed -- one failure per gate, exactly the allowlist assertion. Against the fix: 18 passed. The push-trigger test is what fails if a future edit widens the `push` scope while removing the PR filter. Verified: 27 passed across the two workflow-YAML test modules (test_pr_governance_workflow.py unchanged at 9); all 32 workflow files still parse as YAML. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014ubMueWZScQy2KgsQ4uHQi --- .github/workflows/ci.yml | 5 +- .github/workflows/codeql-analysis.yml | 3 +- .github/workflows/coverage.yml | 2 +- .github/workflows/dependency-review.yml | 2 +- .github/workflows/e2e-tests.yml | 2 +- .github/workflows/security.yml | 3 +- tests/unit/test_workflow_pr_gate_coverage.py | 77 ++++++++++++++++++++ 7 files changed, 88 insertions(+), 6 deletions(-) create mode 100644 tests/unit/test_workflow_pr_gate_coverage.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6c04f5338..ee6803955 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,8 +3,11 @@ name: CI on: push: branches: [main] + # No base-branch filter. A `branches:` allowlist here gates on the PR's *base*, + # so a PR stacked onto another PR's branch ran no CI at all (see #1440, which + # targets #1381's branch and has zero CI runs). Every PR gets the gate; the + # push trigger above stays scoped to main. pull_request: - branches: [main] permissions: contents: read diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 4948f9539..bdad9e215 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -3,8 +3,9 @@ name: "CodeQL Analysis" on: push: branches: [ "main" ] + # No base-branch filter — see the note in ci.yml. CodeQL is a security gate; + # a stacked PR must not be able to skip it by targeting a non-main base. pull_request: - branches: [ "main" ] schedule: - cron: '0 6 * * 1' diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index 521c11795..417bb83d7 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -6,8 +6,8 @@ on: paths-ignore: - "**/*.md" - "**/*.txt" + # No base-branch filter — see the note in ci.yml. pull_request: - branches: [main, develop] paths-ignore: - "**/*.md" - "**/*.txt" diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml index a7efcad8c..1ff5a5eb5 100644 --- a/.github/workflows/dependency-review.yml +++ b/.github/workflows/dependency-review.yml @@ -1,8 +1,8 @@ name: 🔍 Dependency Review on: + # No base-branch filter — see the note in ci.yml. pull_request: - branches: [ main, develop ] types: [opened, synchronize, reopened, ready_for_review] permissions: diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 547c4b356..3bc529fd7 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -3,8 +3,8 @@ name: E2E Tests on: push: branches: [main] + # No base-branch filter — see the note in ci.yml. pull_request: - branches: [main] permissions: contents: read diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 9f622ad5f..dee5f5ce8 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -3,8 +3,9 @@ name: Security Scan on: push: branches: [main] + # No base-branch filter — see the note in ci.yml. A stacked PR must not be + # able to skip the security scan by targeting a branch other than main. pull_request: - branches: [main] schedule: - cron: '0 0 * * 0' # Weekly on Sunday diff --git a/tests/unit/test_workflow_pr_gate_coverage.py b/tests/unit/test_workflow_pr_gate_coverage.py new file mode 100644 index 000000000..64ebc39d1 --- /dev/null +++ b/tests/unit/test_workflow_pr_gate_coverage.py @@ -0,0 +1,77 @@ +"""Every PR quality gate must run on every PR, whatever its base branch. + +A `pull_request.branches:` allowlist filters on the PR's *base*, not its head. +Six gates carried `branches: [main]`, so a PR stacked onto another PR's branch +matched none of them and ran with no CI, no CodeQL, no security scan, no +coverage, no e2e and no dependency review. #1440 -- a security fix targeting +#1381's branch -- reached "ready for review" with zero CI runs that way. + +These tests pin the trigger shape so the allowlist cannot come back. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +import yaml + +WORKFLOWS = Path(__file__).resolve().parents[2] / ".github/workflows" + +# PR-triggered quality gates. Each must fire regardless of the PR's base branch. +GATED_WORKFLOWS = [ + "ci.yml", + "security.yml", + "coverage.yml", + "e2e-tests.yml", + "codeql-analysis.yml", + "dependency-review.yml", +] + + +def _triggers(filename: str) -> dict: + path = WORKFLOWS / filename + assert path.exists(), f"{filename} should exist" + workflow = yaml.safe_load(path.read_text()) + # PyYAML parses the YAML 'on' key as the Python bool True. + return workflow[True] + + +@pytest.mark.parametrize("filename", GATED_WORKFLOWS) +def test_gate_triggers_on_pull_request(filename: str) -> None: + assert "pull_request" in _triggers(filename), ( + f"{filename} must run on pull_request to gate PRs at all" + ) + + +@pytest.mark.parametrize("filename", GATED_WORKFLOWS) +def test_gate_has_no_base_branch_allowlist(filename: str) -> None: + """The defect itself: a base-branch allowlist lets stacked PRs skip the gate.""" + config = _triggers(filename)["pull_request"] + # A bare `pull_request:` parses to None, which is the shape we want. + if config is None: + return + assert "branches" not in config, ( + f"{filename} filters pull_request on base branch {config['branches']!r}; " + "a PR stacked onto a non-main branch would skip this gate entirely" + ) + assert "branches-ignore" not in config, ( + f"{filename} uses branches-ignore on pull_request, which has the same " + "stacked-PR bypass as a branches allowlist" + ) + + +@pytest.mark.parametrize("filename", GATED_WORKFLOWS) +def test_push_trigger_stays_scoped_to_main(filename: str) -> None: + """Widening the PR trigger must not widen the push trigger with it. + + Un-scoping `push` would run the full suite twice on every branch push. + """ + triggers = _triggers(filename) + if "push" not in triggers: + return + branches = triggers["push"].get("branches") + assert branches, f"{filename} push trigger should stay pinned to a branch list" + assert set(branches) <= {"main", "develop"}, ( + f"{filename} push trigger widened to {branches!r}" + )