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 = [