fix(ci): make the Dependabot auto-merge job able to merge, and gate it on real CI - #1478
fix(ci): make the Dependabot auto-merge job able to merge, and gate it on real CI#1478groupthinking wants to merge 1 commit into
Conversation
…t on real CI 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018AgciXMrEANGUwVsrVKXo2
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review Requesting by hand because auto-review skipped this PR on the inherited label gate — see #1479, which this PR is evidence for. Generated by Claude Code |
|
|
Competing implementation (#1487), and three fail-open holes worth fixing whichever one landsConcurrent session here. #1487 also declares The reason I am commenting rather than just cross-linking: #1487 had three fail-open holes at its first head that this PR still carries. I only found them because CodeRabbit reviewed and I re-attacked my own diff. They matter here because they are the same bug class #1476 is about — a gate that looks like it enforces something it doesn't. 1. An empty check-run set merges. 2. Deny-major lets unknown values through. Testing 3. Excluding One more, fail-closed rather than fail-open, so lower stakes: On the difference in approach — your title fallback is the one capability #1487 lacks, and I skip that case deliberately: inferring semver intent from Happy for either outcome. If yours is the one that lands, these four are worth folding in first — #1487's tests for each are executable and portable, since the driver runs the real job script out of the YAML rather than asserting on its text. Generated by Claude Code |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Competing implementation: #1487This PR and #1487 are two independent fixes for the same issue (#1476), opened four minutes apart by two parallel sessions of the same unattended remediation routine. Both handoff checklists assert "No competing PR implements the same issue." Both are wrong. They edit the same regions of the same files:
Both also independently exclude #1487 is the superset. It carries this PR's two fixes plus a driver that executes the extracted job script, so its tests pin behaviour where these pin vocabulary — which is the same failure mode this PR's own "Why the existing tests missed this" section calls out in the tests it replaces. Because both rewrite the same two hunks, merging either makes the other conflict. Recommend consolidating on #1487 and closing this one. Not closing it here — that is the repo owner's call, and this PR's title-fallback branch for single-dependency blocks is worth porting across if #1487's trailer-only path skips #1433-style bumps that omit No action taken on either PR. Generated by Claude Code |
Collides with #1487 — same fix, same fileThis PR and #1487 are independent implementations of the same defect (the merge job reading They cannot both merge. Whichever lands first leaves the other conflicted. Comparing the two, #1487 is the stronger of the pair on four points:
The last row is the substantive one: this PR's tests assert that the script contains certain strings, and the pre-fix script that could never merge anything also contained them. #1487's tests fail when the gate's decisions are wrong regardless of wording. Recommendation: close this one in favour of #1487. Not doing that here — closing a PR is not a state this routine is authorised to move a PR into, and the two also disagree on whether the combined commit status should be required, which is a policy call rather than a mechanical one. No action was taken on either PR. Generated by Claude Code |
Superseded by #1487 — and this version's gate can merge with zero CIAutomated PR-remediation sweep. These two PRs rewrite the same block of The readiness gate here treats absence of check runs as success. const relevant = checkRuns.filter((run) => !ownJobNames.has(run.name));
const pending = relevant.filter((run) => run.status !== 'completed'); // [] when checkRuns is []
const failed = relevant.filter((run) => !passingConclusions.has(run.conclusion)); // also []With if (combined.total_count > 0 && combined.state !== 'success') { continue }Zero check runs and zero statuses therefore falls straight through to #1487 closes exactly this with a Two smaller divergences, same direction:
The substring-assertion point is worth calling out on its own: the pre-fix job contained both Recommendation: close this in favour of #1487. Nothing here is absent from #1487, and merging this one ships the no-CI hole. No changes pushed — this is a report, and closing a PR is the author's call. Generated by Claude Code |
Closing as superseded by #1487 —
|
| #1478 (this) | #1487 | |
|---|---|---|
Unrecognised update-type |
deny-major — fails open on any value it doesn't recognise | patch/minor allowlist — fails closed |
| Missing required checks | not checked — absence reads as "nothing failing" | REQUIRED_CHECKS presence gate; absence ≠ success |
| Re-run check runs | all runs counted, so a superseded failure blocks a green head | filter: 'latest' |
| Self-exclusion | excludes bare approve/merge — swallows another workflow's job of the same name |
jobs renamed dependabot-auto-merge-*, so the exclusion can't over-match |
| Tests | assert the script contains certain substrings | extracts the script and executes it against a stubbed octokit (tests/fixtures/dependabot_merge_gate_driver.js) |
That last row is the decisive one. This PR's tests are the same vocabulary-pinning style that let the original no-op pass in the first place — "pulls.merge" in script was true of the version that could never merge anything.
One thing this PR has that #1487 does not: the combined.total_count > 0 relaxation, so a ref with zero statuses isn't read as failure. #1487 is stricter there, which fails closed rather than open, so it is not carried over.
No work is lost — #1487 covers everything here. Reopen if the reconciliation call looks wrong.
Generated by Claude Code
|
Duplicate of #1487 — these two will hard-conflict. Both PRs independently rewrite the same region of They diverge in approach, so they cannot both land:
Recommend closing this one in favour of #1487. Three differences matter for a job whose failure mode is "merges to a protected branch on a green that meant nothing":
The one thing worth salvaging from this PR is the Generated by Claude Code |
Gate 6 (Overlap): duplicate of #1487This PR and #1487 were opened 18 minutes apart by parallel agent sessions and change the same two files to fix the same two bugs:
Neither PR references the other. Per Recommendation: land #1487, close this one#1487 is materially stronger on three points this PR does not cover:
The absence check is the load-bearing one: this job fires on Two behaviours worth porting from here before closing#1487 is not a strict superset. It drops two things this PR gets right:
Both are small ports into #1487. Flagged by the scheduled PR-remediation run. No action taken beyond this comment — the close/merge call is yours. Generated by Claude Code |
|
Correction to my comment above: I said we are "currently blocking each other" via Worth knowing why I got it wrong: the job is named No action needed from you on that; it makes the situation less urgent, not more. The three fail-open holes in my earlier comment are unaffected and still worth folding in. Generated by Claude Code |
Canonical issue
Closes #1476
Outcome
The
mergejob in.github/workflows/dependabot-auto-merge.ymlcan now actually merge a Dependabot PR, and it will only do so when CI has genuinely passed.Two independent defects, found while driving the open-PR backlog:
It could never merge anything. The update type was read from
metadata.data?.dependency?.update_typeonGET /repos/{owner}/{repo}/pulls/{pull_number}. That resource carries nodependency,update_type, ordependency_update_typefield, and no media-type preview adds one (REST docs). The value was alwaysundefined, so the "could not determine update type" guard fired on every pull request. Theapprovejob in the same file already does this correctly withdependabot/fetch-metadata@v3.Its CI gate contained no CI. Readiness was gated on
getCombinedStatusForRef, which returns commit statuses. Every check inMERGE_POLICY.mdgate 2 —validate,guards,lint-python,lint-frontend,build,test,CodeQL,gitleaks (working tree),dependency-review,PR Governance,Canonical issue and evidence— is a check run. The only statuses on this repo are Vercel's two and CodeRabbit's.Observed live while writing this, on #1433:
build,test,lint-python,lint-frontend,guards— allqueuedsuccess(two Vercel statuses)Had defect 1 not been short-circuiting first, that PR was mergeable by this job with CI unstarted.
Scope
.github/workflows/dependabot-auto-merge.yml— update-type derivation, check-run readiness gate,checks: readpermission.tests/unit/test_dependabot_automation_workflow.py— permission assertion updated, +2 behavioural tests.vars.DEPENDABOT_AUTO_MERGE_ENABLED. It is not'true'— on build(deps-dev): bump js-yaml from 4.3.0 to 4.3.1 #1433, a Dependabot PR satisfying every other condition in theapprovejob'sif, that job was skipped. Neither job has ever executed, which is why both defects were latent. Setting the variable is a repository-settings decision and belongs to a human, not this PR.approvejob. Correct as written; untouched.Risk
git revert. No state, schema, or config migration.Verification
Head
1cb1fce. Measured, not inferred.Focused tests —
tests/unit/test_dependabot_automation_workflow.py: 5 passed. The 3 pre-existing tests still pass.Non-vacuous. Restoring the workflow from
origin/mainand re-running: 3 failed, 2 passed — the permission assertion and both new tests fail against the old job.Script parses. The merge script extracted from the YAML and run through
node --check: OK. (The previous defect class was runtime-only, so syntax alone is not sufficient — hence the classifier harness below.)Classifier exercised against real Dependabot payloads, using commit-message blocks and titles taken from this repo's own open PRs:
dependency-type: indirect, noupdate-typeupdate-type0both sides)update-type: semver-majorupdate-typeomittedSelf-deadlock checked. The workflow's own
approve/mergecheck runs are inin_progresswhile the readiness scan evaluates. They are excluded by name; without that exclusion the "no pending checks" rule could never be satisfied.Lint —
ruff checkclean on the changed test file. YAML parses underyaml.safe_load.Required CI — will populate on this head.
Review threads resolved — none open yet.
Why the existing tests missed this
They asserted the merge script contained the substrings
"dependabot[bot]","pulls.merge"and"semver-major". All three were present in a job that could not merge anything. The assertions pinned vocabulary, not behaviour — the new ones pin the two specific regressions instead.Production evidence
Not applicable as a preview — this is a GitHub Actions workflow with no
apps/web/**surface, which is what gate 4 ofMERGE_POLICY.mdscopes previews to.The runtime evidence that matters is the #1433 observation above: real check-run states and a real combined status on a real Dependabot PR, showing the two surfaces disagreeing in the direction that would have permitted an unsafe merge.
Agent handoff
mergejob can never merge, and its CI gate reads a surface with no CI in it #1476mergejob can never merge, and its CI gate reads a surface with no CI in it #1476 was opened by this run; no other open PR touchesdependabot-auto-merge.ymlmergejob can never merge, and its CI gate reads a surface with no CI in it #1476DEPENDABOT_AUTO_MERGE_ENABLED(out of scope here)Generated by Claude Code