From 27a27a78160237b32529cf3b650fbd264ed5ebc4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 20:58:28 +0000 Subject: [PATCH 1/3] fix(ci): make the Dependabot merge gate able to merge, and gate it on real CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `merge` job in dependabot-auto-merge.yml had two independent defects. Defect 1 — it could never merge. The job read the semver impact from `GET /pulls/{n}` behind a `dorian` media-type preview: metadata.data?.dependency?.update_type The pulls schema carries no dependency metadata, and `dorian` gated draft PRs, not Dependabot fields. `updateType` was always undefined, so every PR hit the "could not determine update type" branch. Read it instead from the `updated-dependencies` trailer Dependabot writes into the head commit message — the same source `dependabot/fetch-metadata` uses, which exists precisely because there is no such API field. All entries are checked, so a grouped update containing one major is blocked. Defect 2 — the readiness gate read a surface with no CI in it. `getCombinedStatusForRef` returns only legacy commit statuses; every gate in MERGE_POLICY.md gate 2 is a check run. On a typical Dependabot head the combined status is `success` off two Vercel statuses while build/test/guards are still queued, so this would merge to protected main with CI unfinished — or red. Now scans check runs too: unfinished blocks, non-success/skipped/ neutral blocks, commit statuses still checked as before. Defect 1 was masking defect 2. Fixing the update-type lookup alone would have armed unsafe merges, so both are fixed together. Adds the `checks: read` permission the readiness scan needs, without which `checks.listForRef` 403s, and excludes this workflow's own approve/merge check runs so the gate cannot deadlock against itself. The existing tests asserted the script *contained* "pulls.merge" and "semver-major" — both true of the no-op version. Replaced with behavioural tests that extract the script and execute it against a stubbed octokit: 8 of them fail against the previous workflow and pass against this one. No behaviour change today: `vars.DEPENDABOT_AUTO_MERGE_ENABLED` is not 'true', and this commit deliberately does not set it. Closes #1476 --- .github/workflows/dependabot-auto-merge.yml | 93 ++++-- .../fixtures/dependabot_merge_gate_driver.js | 76 +++++ .../test_dependabot_automation_workflow.py | 264 ++++++++++++++++++ 3 files changed, 417 insertions(+), 16 deletions(-) create mode 100644 tests/fixtures/dependabot_merge_gate_driver.js diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml index acb002814..f9fe97308 100644 --- a/.github/workflows/dependabot-auto-merge.yml +++ b/.github/workflows/dependabot-auto-merge.yml @@ -15,6 +15,10 @@ permissions: contents: write pull-requests: write statuses: read + # Required by the merge job's check-run readiness scan. Commit statuses and + # check runs are separate surfaces with separate scopes; without this the + # `checks.listForRef` call 403s. + checks: read jobs: approve: @@ -116,33 +120,52 @@ jobs: continue; } - const metadata = await github.request( - 'GET /repos/{owner}/{repo}/pulls/{pull_number}', - { - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: pullRequest.number, - mediaType: { previews: ['dorian'] }, - } - ); - - const updateType = - metadata.data?.dependency?.update_type ?? - metadata.data?.update_type ?? - metadata.data?.dependency_update_type; + // Dependabot records the update metadata as a YAML block in the + // commit message; the pulls REST API carries no dependency + // metadata at all. The previous lookup here read + // `dependency.update_type` off `GET /pulls/{n}` behind a + // `dorian` preview -- fields that do not exist on that schema + // (and a preview that gated draft PRs, not Dependabot data). So + // `updateType` was always undefined, every PR hit the + // "could not determine update type" branch, and this job merged + // nothing it was meant to merge. + const { data: headCommit } = await github.rest.repos.getCommit({ + owner: context.repo.owner, + repo: context.repo.repo, + ref: pullRequest.head.sha, + }); - if (!updateType) { + const updateTypes = [ + ...(headCommit.commit?.message ?? '').matchAll( + /^\s*update-type:\s*(\S+)\s*$/gm, + ), + ].map(match => match[1]); + + // Absent for indirect/transitive bumps, where Dependabot emits + // dependency-name/version/type and no update-type. Skipping is + // the conservative read: never merge a bump whose semver impact + // we cannot establish. + if (updateTypes.length === 0) { core.info( `Could not determine update type for PR #${pullRequest.number}; skipping.` ); continue; } - if (updateType.includes('semver-major')) { + // Grouped updates carry one entry per dependency; a single major + // in the group disqualifies the whole PR. + if (updateTypes.some(type => type.includes('semver-major'))) { core.info(`Skipping major update PR #${pullRequest.number}.`); continue; } + // `getCombinedStatusForRef` reports only legacy commit statuses. + // Every GitHub Actions gate on this repo is a *check run*, which + // that endpoint does not see -- on a typical Dependabot head it + // returns `success` off two Vercel statuses alone while `test`, + // `build` and `guards` are still queued. Gating on it merged to + // a protected branch on a green that meant nothing. Require both + // surfaces, and require checks to have actually finished. const { data: combined } = await github.rest.repos.getCombinedStatusForRef({ owner: context.repo.owner, repo: context.repo.repo, @@ -156,6 +179,44 @@ jobs: continue; } + const checkRuns = await github.paginate( + github.rest.checks.listForRef, + { + owner: context.repo.owner, + repo: context.repo.repo, + ref: pullRequest.head.sha, + per_page: 100, + }, + ); + + // This workflow's own jobs are check runs on the same head. They + // are still in progress while this step runs, so counting them + // would deadlock the gate against itself. + const SELF_JOBS = new Set(['approve', 'merge']); + const ACCEPTABLE = new Set(['success', 'skipped', 'neutral']); + + const relevant = checkRuns.filter(run => !SELF_JOBS.has(run.name)); + const unfinished = relevant.filter(run => run.status !== 'completed'); + const failed = relevant.filter( + run => run.status === 'completed' && !ACCEPTABLE.has(run.conclusion), + ); + + if (unfinished.length > 0) { + core.info( + `Skipping PR #${pullRequest.number}; ${unfinished.length} check run(s) still running: ` + + unfinished.map(run => run.name).join(', '), + ); + continue; + } + + if (failed.length > 0) { + core.info( + `Skipping PR #${pullRequest.number}; failing check run(s): ` + + failed.map(run => `${run.name}=${run.conclusion}`).join(', '), + ); + continue; + } + try { await github.rest.pulls.merge({ owner: context.repo.owner, diff --git a/tests/fixtures/dependabot_merge_gate_driver.js b/tests/fixtures/dependabot_merge_gate_driver.js new file mode 100644 index 000000000..06cb6177d --- /dev/null +++ b/tests/fixtures/dependabot_merge_gate_driver.js @@ -0,0 +1,76 @@ +// Test driver for the Dependabot auto-merge `merge` job. +// +// The job body is inline JavaScript inside a workflow YAML file, so the only +// way to test its *behaviour* (rather than assert that it contains certain +// words) is to extract the script and run it against a stubbed octokit. This +// driver does that: it takes the extracted script and a JSON scenario, and +// reports whether the gate merged, plus the reason it logged. +// +// Usage: node dependabot_merge_gate_driver.js +// Output (stdout, JSON): {"merged": bool, "log": [string, ...]} + +const fs = require("fs"); + +const script = fs.readFileSync(process.argv[2], "utf8"); +const scenario = JSON.parse(fs.readFileSync(process.argv[3], "utf8")); + +const HEAD_SHA = "0000000000000000000000000000000000000000"; +const log = []; +let merged = false; + +const github = { + // Only the pre-fix script calls `github.request`; keep it present so that + // version fails on its own logic rather than on a missing stub. + request: async () => ({ data: { number: 1, ...(scenario.pullsGetExtra || {}) } }), + rest: { + pulls: { + get: async () => ({ + data: { + number: 1, + head: { sha: HEAD_SHA }, + user: { login: scenario.author || "dependabot[bot]" }, + draft: scenario.draft === true, + }, + }), + merge: async () => { + merged = true; + }, + }, + repos: { + getCommit: async () => ({ + data: { commit: { message: scenario.commitMessage || "" } }, + }), + getCombinedStatusForRef: async () => ({ + data: { state: scenario.combinedState || "success" }, + }), + }, + checks: { listForRef: "listForRef" }, + }, + paginate: async () => scenario.checkRuns || [], +}; + +const context = { + repo: { owner: "groupthinking", repo: "EventRelay" }, + payload: { + check_suite: { head_sha: HEAD_SHA, pull_requests: [{ number: 1 }] }, + }, +}; + +const core = { + info: (message) => log.push(String(message)), + notice: (message) => log.push(String(message)), +}; + +const run = new Function( + "github", + "context", + "core", + `return (async () => {${script}})()`, +); + +run(github, context, core) + .then(() => process.stdout.write(JSON.stringify({ merged, log }))) + .catch((error) => { + process.stderr.write(String((error && error.stack) || error)); + process.exit(1); + }); diff --git a/tests/unit/test_dependabot_automation_workflow.py b/tests/unit/test_dependabot_automation_workflow.py index e7081dedc..ec2b260bc 100644 --- a/tests/unit/test_dependabot_automation_workflow.py +++ b/tests/unit/test_dependabot_automation_workflow.py @@ -1,7 +1,11 @@ from __future__ import annotations +import json +import shutil +import subprocess from pathlib import Path +import pytest import yaml WORKFLOW_PATH = ( @@ -31,10 +35,14 @@ def test_dependabot_workflow_uses_safe_triggers_and_permissions() -> None: "ready_for_review", ] assert workflow["on"]["check_suite"]["types"] == ["completed"] + # `checks: read` is load-bearing, not decorative: the merge job's readiness + # scan calls `checks.listForRef`, which 403s without it. Commit statuses and + # check runs are separate surfaces behind separate scopes. assert workflow["permissions"] == { "contents": "write", "pull-requests": "write", "statuses": "read", + "checks": "read", } # The auto-merge feature flag is controlled by a repository variable # (vars context), which — unlike env — is available in job-level `if` @@ -114,3 +122,259 @@ def test_dependabot_ignores_eslint_v10() -> None: "dependency-name": "eslint", "versions": [">=10"], } in update.get("ignore", []) + + +# --------------------------------------------------------------------------- +# Behavioural coverage for the merge gate (#1476). +# +# The assertions above pin vocabulary: they check that the merge script +# *contains* "pulls.merge" and "semver-major". Both substrings were present in +# the version of this job that could never merge anything, so those tests +# passed against a no-op. The tests below extract the script and run it, so +# they fail if the gate's decisions are wrong regardless of how it is worded. +# --------------------------------------------------------------------------- + +DRIVER_PATH = ( + Path(__file__).resolve().parents[1] / "fixtures/dependabot_merge_gate_driver.js" +) + +DIRECT_PATCH_COMMIT = """build(deps): bump js-yaml from 4.3.0 to 4.3.1 + +--- +updated-dependencies: +- dependency-name: js-yaml + dependency-version: 4.3.1 + dependency-type: direct:development + update-type: version-update:semver-patch +... + +Signed-off-by: dependabot[bot] +""" + +# Dependabot omits `update-type` for some indirect bumps -- the hono bump on +# PR #1459 is a real example. +INDIRECT_COMMIT = """build(deps): bump hono from 4.12.32 to 4.13.1 + +--- +updated-dependencies: +- dependency-name: hono + dependency-version: 4.13.1 + dependency-type: indirect +... +""" + +GROUPED_WITH_MAJOR_COMMIT = """build(deps): bump the npm-minor-patch group + +--- +updated-dependencies: +- dependency-name: safe-dep + update-type: version-update:semver-patch +- dependency-name: breaking-dep + update-type: version-update:semver-major +... +""" + + +def _green(*names: str) -> list[dict]: + return [ + {"name": name, "status": "completed", "conclusion": "success"} for name in names + ] + + +def _run_merge_gate(tmp_path: Path, scenario: dict) -> dict: + """Execute the workflow's real merge script against a stubbed octokit.""" + node = shutil.which("node") + if node is None: # pragma: no cover - depends on the runner image + pytest.skip("node is required to execute the workflow's inline script") + + workflow = _load_workflow() + script = workflow["jobs"]["merge"]["steps"][0]["with"]["script"] + + script_path = tmp_path / "merge_script.js" + script_path.write_text(script) + scenario_path = tmp_path / "scenario.json" + scenario_path.write_text(json.dumps(scenario)) + + result = subprocess.run( + [node, str(DRIVER_PATH), str(script_path), str(scenario_path)], + capture_output=True, + text=True, + timeout=60, + check=False, + ) + assert result.returncode == 0, f"driver failed: {result.stderr}" + return json.loads(result.stdout) + + +def test_merge_gate_merges_a_green_patch_update(tmp_path: Path) -> None: + """The whole point of the job. This fails against the pre-#1476 script, + which read `update_type` off a pulls-API field that does not exist and so + skipped every pull request it was meant to merge.""" + outcome = _run_merge_gate( + tmp_path, + { + "commitMessage": DIRECT_PATCH_COMMIT, + "combinedState": "success", + "checkRuns": _green("build", "test", "guards", "lint-python"), + }, + ) + + assert outcome["merged"] is True, outcome["log"] + + +def test_merge_gate_blocks_while_check_runs_are_unfinished(tmp_path: Path) -> None: + """Commit statuses carry no CI on this repo -- on a typical Dependabot head + the combined status is `success` off two Vercel statuses while `build` and + `test` are still queued. Gating on that alone merged with CI unfinished.""" + outcome = _run_merge_gate( + tmp_path, + { + "commitMessage": DIRECT_PATCH_COMMIT, + "combinedState": "success", + "checkRuns": [ + *_green("lint-python"), + {"name": "build", "status": "queued", "conclusion": None}, + {"name": "test", "status": "in_progress", "conclusion": None}, + ], + }, + ) + + assert outcome["merged"] is False + assert "still running" in outcome["log"][-1] + + +def test_merge_gate_blocks_on_a_failing_check_run(tmp_path: Path) -> None: + outcome = _run_merge_gate( + tmp_path, + { + "commitMessage": DIRECT_PATCH_COMMIT, + "combinedState": "success", + "checkRuns": [ + *_green("build", "guards"), + {"name": "test", "status": "completed", "conclusion": "failure"}, + ], + }, + ) + + assert outcome["merged"] is False + assert "test=failure" in outcome["log"][-1] + + +def test_merge_gate_treats_skipped_and_neutral_as_satisfied(tmp_path: Path) -> None: + """`MERGE_POLICY.md` gate 2 lists conditional checks; `E2E Pipeline Tests` + is routinely `skipped` and `PR Governance` `neutral`. Neither should + deadlock a merge.""" + outcome = _run_merge_gate( + tmp_path, + { + "commitMessage": DIRECT_PATCH_COMMIT, + "combinedState": "success", + "checkRuns": [ + *_green("build", "test"), + { + "name": "E2E Pipeline Tests", + "status": "completed", + "conclusion": "skipped", + }, + { + "name": "PR Governance", + "status": "completed", + "conclusion": "neutral", + }, + ], + }, + ) + + assert outcome["merged"] is True, outcome["log"] + + +def test_merge_gate_ignores_its_own_check_runs(tmp_path: Path) -> None: + """`approve` and `merge` are check runs on the same head, and `merge` is + necessarily in progress while it evaluates this. Counting them would + deadlock the gate against itself.""" + outcome = _run_merge_gate( + tmp_path, + { + "commitMessage": DIRECT_PATCH_COMMIT, + "combinedState": "success", + "checkRuns": [ + *_green("build", "test"), + {"name": "approve", "status": "completed", "conclusion": "skipped"}, + {"name": "merge", "status": "in_progress", "conclusion": None}, + ], + }, + ) + + assert outcome["merged"] is True, outcome["log"] + + +def test_merge_gate_skips_updates_it_cannot_classify(tmp_path: Path) -> None: + """No `update-type` trailer means the semver impact is unknown. Skipping is + the conservative read; guessing minor would let a major through.""" + outcome = _run_merge_gate( + tmp_path, + { + "commitMessage": INDIRECT_COMMIT, + "combinedState": "success", + "checkRuns": _green("build", "test"), + }, + ) + + assert outcome["merged"] is False + assert "update type" in outcome["log"][-1] + + +def test_merge_gate_blocks_a_group_containing_a_major(tmp_path: Path) -> None: + """Grouped updates carry one trailer entry per dependency. A single major + in the group disqualifies the PR, so the check cannot stop at the first.""" + outcome = _run_merge_gate( + tmp_path, + { + "commitMessage": GROUPED_WITH_MAJOR_COMMIT, + "combinedState": "success", + "checkRuns": _green("build", "test"), + }, + ) + + assert outcome["merged"] is False + assert "major" in outcome["log"][-1] + + +def test_merge_gate_still_respects_commit_statuses(tmp_path: Path) -> None: + """Check runs are an addition, not a replacement -- a red commit status + (Vercel, CodeRabbit) must still block.""" + outcome = _run_merge_gate( + tmp_path, + { + "commitMessage": DIRECT_PATCH_COMMIT, + "combinedState": "failure", + "checkRuns": _green("build", "test"), + }, + ) + + assert outcome["merged"] is False + assert "combined status" in outcome["log"][-1] + + +def test_merge_gate_skips_non_dependabot_and_draft_pull_requests( + tmp_path: Path, +) -> None: + human = _run_merge_gate( + tmp_path, + { + "author": "groupthinking", + "commitMessage": DIRECT_PATCH_COMMIT, + "checkRuns": _green("build", "test"), + }, + ) + assert human["merged"] is False + + draft = _run_merge_gate( + tmp_path, + { + "draft": True, + "commitMessage": DIRECT_PATCH_COMMIT, + "checkRuns": _green("build", "test"), + }, + ) + assert draft["merged"] is False From bdca4c524dfde990f1f27f725313a6dece8f30d4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 21:03:09 +0000 Subject: [PATCH 2/3] fix(ci): fail closed on unreported checks, unknown update types, name collisions Three fail-open holes in the merge gate from the previous commit. Two were raised by CodeRabbit; the required-checks one I raised against my own diff and fixed more strictly than the minimum. 1. An empty check-run set merged. `unfinished` and `failed` are both empty when no relevant check runs exist, so the gate merged on no CI at all. This is reachable: the job fires on a *completed check suite*, and the first suite to complete can be this workflow's own, before other workflows have created their check runs. Rather than only blocking the empty case, require every check MERGE_POLICY.md gate 2 lists as "required for every pull request" to be present -- absence is not success, and a partial list was just as unsafe as an empty one. Also passes `filter: 'latest'` so a superseded failure from a re-run cannot block a head that is now green. 2. The update-type gate denied major rather than allowing patch/minor, so any value it did not recognise passed -- a new Dependabot update kind, or a value mangled by future parser drift. Now an explicit allowlist of semver-patch/semver-minor, with every entry of a grouped update required to qualify. 3. The self-exclusion matched the bare names `approve` and `merge`, which would also swallow a failing job of that name from an unrelated workflow. Renamed this workflow's jobs to `dependabot-auto-merge-approve` and `dependabot-auto-merge-merge` and narrowed the exclusion to those. Nothing outside the tests referenced the old names, and neither is in branch protection. Drops the test assertion that the merge script contains "semver-major". It was true of the version that could merge nothing, and stopped being true when the policy got stricter -- it tracked wording, not behaviour, which is the defect #1476 calls out. The behavioural tests cover the semantics. +4 behavioural tests (16 total). Against the previous workflow: 15 fail. --- .github/workflows/dependabot-auto-merge.yml | 83 +++++++- .../test_dependabot_automation_workflow.py | 200 +++++++++++++++--- 2 files changed, 245 insertions(+), 38 deletions(-) diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml index f9fe97308..4221e5543 100644 --- a/.github/workflows/dependabot-auto-merge.yml +++ b/.github/workflows/dependabot-auto-merge.yml @@ -21,7 +21,7 @@ permissions: checks: read jobs: - approve: + dependabot-auto-merge-approve: if: >- vars.DEPENDABOT_AUTO_MERGE_ENABLED == 'true' && github.event_name == 'pull_request_target' && @@ -83,7 +83,7 @@ jobs: core.info(`Auto-merge could not be enabled for PR #${pull_number}: ${error.message}`); } - merge: + dependabot-auto-merge-merge: if: vars.DEPENDABOT_AUTO_MERGE_ENABLED == 'true' && github.event_name == 'check_suite' && github.event.check_suite.conclusion == 'success' runs-on: ubuntu-latest steps: @@ -152,10 +152,23 @@ jobs: continue; } - // Grouped updates carry one entry per dependency; a single major - // in the group disqualifies the whole PR. - if (updateTypes.some(type => type.includes('semver-major'))) { - core.info(`Skipping major update PR #${pullRequest.number}.`); + // Allowlist rather than deny-major: policy is patch/minor only, so + // anything we do not positively recognise -- a major, a new + // Dependabot update kind, or a value mangled by parser drift -- + // must fail closed. Grouped updates carry one entry per + // dependency, so every entry has to qualify. + const ALLOWED_UPDATE_TYPES = new Set([ + 'version-update:semver-patch', + 'version-update:semver-minor', + ]); + const disallowed = updateTypes.filter( + type => !ALLOWED_UPDATE_TYPES.has(type), + ); + if (disallowed.length > 0) { + core.info( + `Skipping PR #${pullRequest.number}; update type(s) outside the ` + + `patch/minor policy: ${disallowed.join(', ')}`, + ); continue; } @@ -186,16 +199,66 @@ jobs: repo: context.repo.repo, ref: pullRequest.head.sha, per_page: 100, + // Re-running a job creates an additional check run under the + // same name; `latest` keeps only the most recent per name, so + // a superseded failure cannot block a head that is now green. + filter: 'latest', }, ); - // This workflow's own jobs are check runs on the same head. They - // are still in progress while this step runs, so counting them - // would deadlock the gate against itself. - const SELF_JOBS = new Set(['approve', 'merge']); + // This workflow's own jobs are check runs on the same head, and + // this one is necessarily in progress while it evaluates them, so + // counting them would deadlock the gate against itself. The job + // names are workflow-qualified precisely so that this exclusion + // cannot silently swallow an unrelated workflow's `merge` job. + const SELF_JOBS = new Set([ + 'dependabot-auto-merge-approve', + 'dependabot-auto-merge-merge', + ]); const ACCEPTABLE = new Set(['success', 'skipped', 'neutral']); + // MERGE_POLICY.md gate 2, "Required for every pull request". + // `trivy` is lowercase deliberately: a separate `Trivy` check run + // exists on the same head and reports `neutral` forever, so + // requiring the capitalised one would never pass. + const REQUIRED_CHECKS = [ + 'validate', + 'guards', + 'lint-python', + 'lint-frontend', + 'build', + 'test', + 'CodeQL', + 'gitleaks (working tree)', + 'dependency-review', + 'PR Governance', + 'Canonical issue and evidence', + 'Security Scan - python', + 'Security Scan - javascript', + 'bandit', + 'python-safety', + 'npm-audit', + 'trivy', + ]; + const relevant = checkRuns.filter(run => !SELF_JOBS.has(run.name)); + const present = new Set(relevant.map(run => run.name)); + + // Absence is not success. This job is triggered by a completed + // check suite, and the first suite to complete may well be this + // workflow's own -- at which point the other workflows' check + // runs do not exist yet, `unfinished` and `failed` are both + // empty, and a bare "nothing is failing" test would merge a PR + // that has had no CI run against it at all. + const missing = REQUIRED_CHECKS.filter(name => !present.has(name)); + if (missing.length > 0) { + core.info( + `Skipping PR #${pullRequest.number}; required check run(s) not reported yet: ` + + missing.join(', '), + ); + continue; + } + const unfinished = relevant.filter(run => run.status !== 'completed'); const failed = relevant.filter( run => run.status === 'completed' && !ACCEPTABLE.has(run.conclusion), diff --git a/tests/unit/test_dependabot_automation_workflow.py b/tests/unit/test_dependabot_automation_workflow.py index ec2b260bc..69d289f16 100644 --- a/tests/unit/test_dependabot_automation_workflow.py +++ b/tests/unit/test_dependabot_automation_workflow.py @@ -57,8 +57,8 @@ def test_dependabot_workflow_approves_and_merges_without_checkout() -> None: workflow = _load_workflow() jobs = workflow["jobs"] - approve_job = jobs["approve"] - merge_job = jobs["merge"] + approve_job = jobs["dependabot-auto-merge-approve"] + merge_job = jobs["dependabot-auto-merge-merge"] assert "vars.DEPENDABOT_AUTO_MERGE_ENABLED == 'true'" in approve_job["if"] assert "dependabot[bot]" in approve_job["if"] @@ -95,10 +95,14 @@ def test_dependabot_workflow_approves_and_merges_without_checkout() -> None: and "enablePullRequestAutoMerge" in step.get("with", {}).get("script", "") for step in approve_steps ) + # This used to also assert the script contained "semver-major", which was + # true of the version that could never merge anything -- and stopped being + # true once deny-major became a patch/minor allowlist, even though the + # policy got *stricter*. The substring tracked wording, not behaviour. What + # the gate actually decides is covered by the behavioural tests below. assert any( "dependabot[bot]" in step.get("with", {}).get("script", "") and "pulls.merge" in step.get("with", {}).get("script", "") - and "semver-major" in step.get("with", {}).get("script", "") for step in merge_steps ) @@ -175,12 +179,44 @@ def test_dependabot_ignores_eslint_v10() -> None: """ +# MERGE_POLICY.md gate 2, "Required for every pull request". `trivy` is +# lowercase: the capitalised `Trivy` check run reports `neutral` forever. +REQUIRED_CHECKS = [ + "validate", + "guards", + "lint-python", + "lint-frontend", + "build", + "test", + "CodeQL", + "gitleaks (working tree)", + "dependency-review", + "PR Governance", + "Canonical issue and evidence", + "Security Scan - python", + "Security Scan - javascript", + "bandit", + "python-safety", + "npm-audit", + "trivy", +] + + def _green(*names: str) -> list[dict]: return [ {"name": name, "status": "completed", "conclusion": "success"} for name in names ] +def _all_required_green(**overrides: dict) -> list[dict]: + """Every gate-2 check reporting success, with named ones overridden.""" + runs = _green(*REQUIRED_CHECKS) + for run in runs: + if run["name"] in overrides: + run.update(overrides[run["name"]]) + return runs + + def _run_merge_gate(tmp_path: Path, scenario: dict) -> dict: """Execute the workflow's real merge script against a stubbed octokit.""" node = shutil.which("node") @@ -188,7 +224,7 @@ def _run_merge_gate(tmp_path: Path, scenario: dict) -> dict: pytest.skip("node is required to execute the workflow's inline script") workflow = _load_workflow() - script = workflow["jobs"]["merge"]["steps"][0]["with"]["script"] + script = workflow["jobs"]["dependabot-auto-merge-merge"]["steps"][0]["with"]["script"] script_path = tmp_path / "merge_script.js" script_path.write_text(script) @@ -215,7 +251,7 @@ def test_merge_gate_merges_a_green_patch_update(tmp_path: Path) -> None: { "commitMessage": DIRECT_PATCH_COMMIT, "combinedState": "success", - "checkRuns": _green("build", "test", "guards", "lint-python"), + "checkRuns": _all_required_green(), }, ) @@ -231,11 +267,10 @@ def test_merge_gate_blocks_while_check_runs_are_unfinished(tmp_path: Path) -> No { "commitMessage": DIRECT_PATCH_COMMIT, "combinedState": "success", - "checkRuns": [ - *_green("lint-python"), - {"name": "build", "status": "queued", "conclusion": None}, - {"name": "test", "status": "in_progress", "conclusion": None}, - ], + "checkRuns": _all_required_green( + build={"status": "queued", "conclusion": None}, + test={"status": "in_progress", "conclusion": None}, + ), }, ) @@ -249,10 +284,7 @@ def test_merge_gate_blocks_on_a_failing_check_run(tmp_path: Path) -> None: { "commitMessage": DIRECT_PATCH_COMMIT, "combinedState": "success", - "checkRuns": [ - *_green("build", "guards"), - {"name": "test", "status": "completed", "conclusion": "failure"}, - ], + "checkRuns": _all_required_green(test={"conclusion": "failure"}), }, ) @@ -270,17 +302,14 @@ def test_merge_gate_treats_skipped_and_neutral_as_satisfied(tmp_path: Path) -> N "commitMessage": DIRECT_PATCH_COMMIT, "combinedState": "success", "checkRuns": [ - *_green("build", "test"), + *_all_required_green( + **{"PR Governance": {"conclusion": "neutral"}} + ), { "name": "E2E Pipeline Tests", "status": "completed", "conclusion": "skipped", }, - { - "name": "PR Governance", - "status": "completed", - "conclusion": "neutral", - }, ], }, ) @@ -298,9 +327,17 @@ def test_merge_gate_ignores_its_own_check_runs(tmp_path: Path) -> None: "commitMessage": DIRECT_PATCH_COMMIT, "combinedState": "success", "checkRuns": [ - *_green("build", "test"), - {"name": "approve", "status": "completed", "conclusion": "skipped"}, - {"name": "merge", "status": "in_progress", "conclusion": None}, + *_all_required_green(), + { + "name": "dependabot-auto-merge-approve", + "status": "completed", + "conclusion": "skipped", + }, + { + "name": "dependabot-auto-merge-merge", + "status": "in_progress", + "conclusion": None, + }, ], }, ) @@ -316,7 +353,7 @@ def test_merge_gate_skips_updates_it_cannot_classify(tmp_path: Path) -> None: { "commitMessage": INDIRECT_COMMIT, "combinedState": "success", - "checkRuns": _green("build", "test"), + "checkRuns": _all_required_green(), }, ) @@ -332,7 +369,7 @@ def test_merge_gate_blocks_a_group_containing_a_major(tmp_path: Path) -> None: { "commitMessage": GROUPED_WITH_MAJOR_COMMIT, "combinedState": "success", - "checkRuns": _green("build", "test"), + "checkRuns": _all_required_green(), }, ) @@ -348,7 +385,7 @@ def test_merge_gate_still_respects_commit_statuses(tmp_path: Path) -> None: { "commitMessage": DIRECT_PATCH_COMMIT, "combinedState": "failure", - "checkRuns": _green("build", "test"), + "checkRuns": _all_required_green(), }, ) @@ -364,7 +401,7 @@ def test_merge_gate_skips_non_dependabot_and_draft_pull_requests( { "author": "groupthinking", "commitMessage": DIRECT_PATCH_COMMIT, - "checkRuns": _green("build", "test"), + "checkRuns": _all_required_green(), }, ) assert human["merged"] is False @@ -374,7 +411,114 @@ def test_merge_gate_skips_non_dependabot_and_draft_pull_requests( { "draft": True, "commitMessage": DIRECT_PATCH_COMMIT, - "checkRuns": _green("build", "test"), + "checkRuns": _all_required_green(), }, ) assert draft["merged"] is False + + +def test_merge_gate_blocks_when_required_checks_have_not_reported( + tmp_path: Path, +) -> None: + """Absence is not success. This job fires on a *completed check suite* -- + and the first suite to complete can be this workflow's own, at which point + the other workflows' check runs do not exist yet. A bare "nothing is + failing" test merges a PR that has had no CI run against it at all.""" + outcome = _run_merge_gate( + tmp_path, + { + "commitMessage": DIRECT_PATCH_COMMIT, + "combinedState": "success", + "checkRuns": [], + }, + ) + + assert outcome["merged"] is False + assert "not reported yet" in outcome["log"][-1] + + partial = _run_merge_gate( + tmp_path, + { + "commitMessage": DIRECT_PATCH_COMMIT, + "combinedState": "success", + "checkRuns": _green("build", "test"), + }, + ) + + assert partial["merged"] is False + assert "CodeQL" in partial["log"][-1] + + +def test_merge_gate_allowlists_update_types_rather_than_denying_major( + tmp_path: Path, +) -> None: + """Policy is patch/minor only, so an unrecognised value must fail closed. + A deny-major test lets anything it does not recognise through -- including + a value mangled by future parser drift.""" + unknown_commit = DIRECT_PATCH_COMMIT.replace( + "version-update:semver-patch", "version-update:semver-unknown" + ) + outcome = _run_merge_gate( + tmp_path, + { + "commitMessage": unknown_commit, + "combinedState": "success", + "checkRuns": _all_required_green(), + }, + ) + + assert outcome["merged"] is False + assert "outside the patch/minor policy" in outcome["log"][-1] + + # A quoted value is equally unrecognised, and equally must not merge. + quoted_commit = DIRECT_PATCH_COMMIT.replace( + "version-update:semver-patch", '"version-update:semver-patch"' + ) + quoted = _run_merge_gate( + tmp_path, + { + "commitMessage": quoted_commit, + "combinedState": "success", + "checkRuns": _all_required_green(), + }, + ) + + assert quoted["merged"] is False + + +def test_merge_gate_does_not_swallow_an_unrelated_job_named_merge( + tmp_path: Path, +) -> None: + """The self-exclusion is a name match, so this workflow's own jobs are + named `dependabot-auto-merge-*`. A failing `merge` job belonging to some + other workflow must still block.""" + outcome = _run_merge_gate( + tmp_path, + { + "commitMessage": DIRECT_PATCH_COMMIT, + "combinedState": "success", + "checkRuns": [ + *_all_required_green(), + {"name": "merge", "status": "completed", "conclusion": "failure"}, + ], + }, + ) + + assert outcome["merged"] is False + assert "merge=failure" in outcome["log"][-1] + + +def test_merge_gate_accepts_a_minor_update(tmp_path: Path) -> None: + minor_commit = DIRECT_PATCH_COMMIT.replace( + "version-update:semver-patch", "version-update:semver-minor" + ) + outcome = _run_merge_gate( + tmp_path, + { + "commitMessage": minor_commit, + "combinedState": "success", + "checkRuns": _all_required_green(), + }, + ) + + assert outcome["merged"] is True, outcome["log"] From 877da4c5e068beefef15082a42080038968900bd Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 06:43:42 +0000 Subject: [PATCH 3/3] fix(ci): require test-frontend, and pin the gate to MERGE_POLICY The open risk recorded when this PR was written -- that the merge gate hard-codes gate 2's check list and will drift from the policy it claims to enforce -- materialised in three weeks. #1449/#1480 made `test-frontend` a required check and updated MERGE_POLICY.md, but nothing pointed at this gate. Its list still ended at `test`, which runs Python pytest only, so it would have merged a Dependabot PR with the apps/web vitest suite unrun -- including the CWE-209 and billing-disclosure regressions that suite carries. That is the same fail-open class this PR exists to close, arriving through drift rather than through a coding error. Adds `test-frontend` to REQUIRED_CHECKS, and adds the test I previously declined to write. I argued then that parsing the policy prose was "its own small parser with its own failure mode"; having now seen the drift happen, that trade is clearly wrong -- the parse is one regex over a stable sentence, and the alternative is a gate that silently under-enforces. The test fails with the exact missing names, so the next addition to gate 2 lands here instead of in production. Also merges origin/main (56 commits), which brings the concurrency group added to this workflow by #1510. Verified present after the merge. 17 tests pass. Removing `test-frontend` from the gate fails the new test with `missing from gate: ['test-frontend']`. --- .github/workflows/dependabot-auto-merge.yml | 4 +++ .../test_dependabot_automation_workflow.py | 31 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml index 4f35f0aa0..dc674bc21 100644 --- a/.github/workflows/dependabot-auto-merge.yml +++ b/.github/workflows/dependabot-auto-merge.yml @@ -248,6 +248,10 @@ jobs: 'lint-frontend', 'build', 'test', + // Python pytest only. The apps/web vitest suite is the separate + // required check `test-frontend` (#1449) -- a green `test` is + // not evidence that web unit tests ran. + 'test-frontend', 'CodeQL', 'gitleaks (working tree)', 'dependency-review', diff --git a/tests/unit/test_dependabot_automation_workflow.py b/tests/unit/test_dependabot_automation_workflow.py index 69d289f16..d1179d6ff 100644 --- a/tests/unit/test_dependabot_automation_workflow.py +++ b/tests/unit/test_dependabot_automation_workflow.py @@ -1,6 +1,7 @@ from __future__ import annotations import json +import re import shutil import subprocess from pathlib import Path @@ -188,6 +189,9 @@ def test_dependabot_ignores_eslint_v10() -> None: "lint-frontend", "build", "test", + # Python pytest only; apps/web vitest is the separate `test-frontend` + # required check (#1449). + "test-frontend", "CodeQL", "gitleaks (working tree)", "dependency-review", @@ -522,3 +526,30 @@ def test_merge_gate_accepts_a_minor_update(tmp_path: Path) -> None: ) assert outcome["merged"] is True, outcome["log"] + + +def test_required_checks_match_merge_policy() -> None: + """The gate hard-codes gate 2's list, so it can drift from the policy it + claims to enforce. It already did: `test-frontend` became required in + #1449/#1480 three weeks after this gate was written, and nothing objected — + the gate would have merged a Dependabot PR with the apps/web vitest suite + unrun. Pin the two together so the next addition fails here instead.""" + policy_path = Path(__file__).resolve().parents[2] / "MERGE_POLICY.md" + policy = policy_path.read_text() + + match = re.search(r"Required for every pull request:(.*?)\.\n", policy, re.S) + assert match, "MERGE_POLICY.md no longer states a 'Required for every pull request' list" + policy_checks = set(re.findall(r"`([^`]+)`", match.group(1))) + + script = _load_workflow()["jobs"]["dependabot-auto-merge-merge"]["steps"][0][ + "with" + ]["script"] + block = re.search(r"const REQUIRED_CHECKS = \[(.*?)\];", script, re.S) + assert block, "merge job no longer declares REQUIRED_CHECKS" + gate_checks = set(re.findall(r"'([^']+)'", block.group(1))) + + assert gate_checks == policy_checks, ( + "Dependabot merge gate is out of sync with MERGE_POLICY.md gate 2.\n" + f" missing from gate: {sorted(policy_checks - gate_checks)}\n" + f" extra in gate : {sorted(gate_checks - policy_checks)}" + )