From 1cb1fced8a2ca48a1f16ec4aec7ab7ddccbef074 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 20:55:05 +0000 Subject: [PATCH] fix(ci): make the Dependabot auto-merge job able to merge, and gate it on real CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1476 The `merge` job had two independent defects. It read the update type from `metadata.data?.dependency?.update_type` on `GET /repos/{owner}/{repo}/pulls/{pull_number}`. That resource has no such field and no media-type preview adds one, so the value was always undefined, the "could not determine update type" guard always fired, and the job skipped every pull request it was handed. It now reads the `update-type` line from Dependabot's own `updated-dependencies` commit-message block, falling back to comparing the major components in the title when Dependabot omits it — as it does for some indirect bumps such as #1433. The fallback only applies when the block names exactly one dependency, so a grouped update is skipped rather than classified from a single title. It then gated merge readiness on `getCombinedStatusForRef`, which returns commit statuses only. Every check in MERGE_POLICY.md gate 2 is a check run; the only statuses on this repo are Vercel's and CodeRabbit's. On #1433 the combined status read `success` at 20:48 UTC while `build`, `test` and the `lint-*` jobs were still queued, so this gate would have merged with CI unfinished. Readiness now scans check runs: anything not `completed` blocks, and only `success`/`skipped`/`neutral` count as passing, matching gate 2's treatment of conditionally required jobs. The workflow's own `approve` and `merge` runs are excluded, since they are in flight while the gate evaluates and would otherwise deadlock it against itself. Combined status is still consulted, but an empty status list no longer reads as a failure. Reading check runs needs `checks: read`, which the workflow did not request. The existing tests passed on the broken job because they asserted the script contained the substrings "pulls.merge" and "semver-major", which it did. The two new tests pin the behaviour instead, and fail against the previous workflow. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018AgciXMrEANGUwVsrVKXo2 --- .github/workflows/dependabot-auto-merge.yml | 109 +++++++++++++++--- .../test_dependabot_automation_workflow.py | 56 +++++++++ 2 files changed, 148 insertions(+), 17 deletions(-) diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml index acb002814..86ecceff7 100644 --- a/.github/workflows/dependabot-auto-merge.yml +++ b/.github/workflows/dependabot-auto-merge.yml @@ -15,6 +15,9 @@ permissions: contents: write pull-requests: write statuses: read + # Every gate in MERGE_POLICY.md is a check run, not a commit status, so the + # merge job has to read the checks API to see whether CI actually passed. + checks: read jobs: approve: @@ -116,40 +119,112 @@ 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'] }, - } - ); + // Dependabot records what it did in an `updated-dependencies` + // block in its own commit message; that block is the only + // first-party source for the update type available here. The + // pull request REST resource has no `dependency`/`update_type` + // field, and no media-type preview adds one, so reading it from + // there always yielded undefined and this job skipped every PR + // it was ever handed. + const commits = await github.paginate(github.rest.pulls.listCommits, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pullRequest.number, + per_page: 100, + }); - const updateType = - metadata.data?.dependency?.update_type ?? - metadata.data?.update_type ?? - metadata.data?.dependency_update_type; + const dependabotCommit = commits.find((commit) => + /^\s*-\s*dependency-name:/m.test(commit.commit.message) + ); - if (!updateType) { + if (!dependabotCommit) { core.info( - `Could not determine update type for PR #${pullRequest.number}; skipping.` + `No Dependabot metadata block on PR #${pullRequest.number}; skipping.` ); continue; } - if (updateType.includes('semver-major')) { + const message = dependabotCommit.commit.message; + const explicitUpdateType = message.match(/^\s*update-type:\s*(\S+)/m); + + let isMajor; + if (explicitUpdateType) { + isMajor = explicitUpdateType[1].includes('semver-major'); + } else { + // Dependabot omits `update-type` for some indirect bumps (e.g. + // #1433). Fall back to the major components in the title, but + // only when the block names exactly one dependency — a grouped + // update must never be classified from a single title. + const names = message.match(/^\s*-\s*dependency-name:/gm) ?? []; + const bump = pullRequest.title.match( + /\bfrom\s+v?(\d+)\.\S*\s+to\s+v?(\d+)\./i + ); + + if (names.length !== 1 || !bump) { + core.info( + `Could not determine update type for PR #${pullRequest.number}; skipping.` + ); + continue; + } + + isMajor = bump[1] !== bump[2]; + } + + if (isMajor) { core.info(`Skipping major update PR #${pullRequest.number}.`); continue; } + // Commit statuses alone are not evidence that CI passed: on this + // repo the only statuses are Vercel's and CodeRabbit's, so the + // combined state reads `success` while `build`, `test` and + // `lint-*` are still queued. Every check named in MERGE_POLICY.md + // gate 2 is a check run, so read those instead. + 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 still in flight while this step + // runs; counting them would deadlock the gate against itself. + const ownJobNames = new Set(['approve', 'merge']); + // `skipped`/`neutral` are how GitHub reports the conditionally + // required jobs of gate 2 (E2E, coverage on a docs-only diff). + // Gate 2 treats those as satisfied, not as blocking. + const passingConclusions = new Set(['success', 'skipped', 'neutral']); + const relevant = checkRuns.filter((run) => !ownJobNames.has(run.name)); + + const pending = relevant.filter((run) => run.status !== 'completed'); + if (pending.length > 0) { + core.info( + `Skipping PR #${pullRequest.number}; ${pending.length} check(s) still running: ` + + `${pending.map((run) => run.name).join(', ')}.` + ); + continue; + } + + const failed = relevant.filter( + (run) => !passingConclusions.has(run.conclusion) + ); + if (failed.length > 0) { + core.info( + `Skipping PR #${pullRequest.number}; failing check(s): ` + + `${failed.map((run) => `${run.name} (${run.conclusion})`).join(', ')}.` + ); + continue; + } + const { data: combined } = await github.rest.repos.getCombinedStatusForRef({ owner: context.repo.owner, repo: context.repo.repo, ref: pullRequest.head.sha, }); - if (combined.state !== 'success') { + // `pending` with zero statuses just means nothing posts statuses + // on this ref, which is not a failure. + if (combined.total_count > 0 && combined.state !== 'success') { core.info( `Skipping PR #${pullRequest.number} because combined status is ${combined.state}.` ); diff --git a/tests/unit/test_dependabot_automation_workflow.py b/tests/unit/test_dependabot_automation_workflow.py index e7081dedc..c5ae109ea 100644 --- a/tests/unit/test_dependabot_automation_workflow.py +++ b/tests/unit/test_dependabot_automation_workflow.py @@ -35,6 +35,9 @@ def test_dependabot_workflow_uses_safe_triggers_and_permissions() -> None: "contents": "write", "pull-requests": "write", "statuses": "read", + # Without `checks: read` the merge job's checks.listForRef call 403s and + # the readiness gate below can never be evaluated. + "checks": "read", } # The auto-merge feature flag is controlled by a repository variable # (vars context), which — unlike env — is available in job-level `if` @@ -95,6 +98,59 @@ def test_dependabot_workflow_approves_and_merges_without_checkout() -> None: ) +def _merge_script() -> str: + merge_steps = _load_workflow()["jobs"]["merge"]["steps"] + scripts = [step.get("with", {}).get("script", "") for step in merge_steps] + script = "\n".join(scripts) + assert script.strip(), "Merge job should run a github-script step" + return script + + +def test_merge_job_does_not_read_update_type_from_the_pull_request_resource() -> None: + """The pull request REST resource carries no dependency metadata. + + `GET /repos/{owner}/{repo}/pulls/{pull_number}` returns no `dependency`, + `update_type` or `dependency_update_type` field, and there is no `dorian` + preview that adds one. Reading the update type from there yielded + `undefined` on every pull request, so the job hit its + "Could not determine update type" branch and skipped unconditionally — + it could never merge anything. + """ + script = _merge_script() + + assert "dorian" not in script + assert "mediaType" not in script + assert "previews" not in script + assert "dependency?.update_type" not in script + assert "dependency_update_type" not in script + # The update type must come from Dependabot's own commit-message block. + assert "listCommits" in script + assert "update-type" in script + + +def test_merge_job_gates_on_check_runs_not_only_commit_statuses() -> None: + """Commit statuses are not evidence that CI passed on this repo. + + Every check in MERGE_POLICY.md gate 2 (`build`, `test`, `lint-python`, + `CodeQL`, …) is a check run. The only *statuses* are Vercel's and + CodeRabbit's, so `getCombinedStatusForRef` reports `success` while CI is + still queued — observed live on #1433, whose combined status was green at + 20:48 UTC while `build`/`test`/`lint-*` were queued. Gating on the combined + status alone would merge a Dependabot PR with unfinished or failing CI. + """ + script = _merge_script() + + assert "checks.listForRef" in script, ( + "Merge readiness must consult the checks API, not just commit statuses" + ) + # Unfinished checks must block, not be read as passing. + assert "status !== 'completed'" in script + # This workflow's own jobs are in flight while the gate runs; counting them + # would deadlock the check against itself. + assert "ownJobNames" in script + assert "'approve'" in script and "'merge'" in script + + def test_dependabot_ignores_eslint_v10() -> None: config = _load_dependabot_config() npm_updates = [