ci: Do not auto-approve cargo minor bumps - #356
Conversation
`auto_approve.yml` approves any PR authored by `dependabot[bot]` with no
filter on update type. `fetch-metadata` is already wired up with
`id: metadata`, but none of its outputs are read.
That is about to matter. Once dependabot watches `cargo`, the highest-risk
update class it can produce is a minor bump of `openjd-expr`,
`openjd-model` or `openjd-sessions`: those crates are 0.x, where cargo
treats a minor as breaking, and they carry the API surface the Rust
bindings wrap. openjd-expr 0.4.0 carried a breaking coercion change and
openjd-model 0.5.2 changed the job-side `StepScript` wire format. Under
the current workflow such a PR arrives pre-approved, so the update most
needing a human to look is the one least likely to get one.
Gate the approval step on the ecosystem and the update type. Because
`update-type` reports the highest semver change in the PR, a grouped cargo
PR containing any minor is held back as well as a solo one.
Scoped to cargo rather than gating every ecosystem on patch. The pip and
github-actions entries group minor with patch into one PR, so a global
patch-only gate would stop auto-approving nearly every PR from them --
a much larger behaviour change than the problem warrants, and not one
this addresses.
Known gap left in place: a pip or github-actions major is still
auto-approved, as it is today. Gating that is a one-line change but a
separate decision, so it is deliberately not bundled here.
Testing: actionlint passes on the modified file, and on the unmodified
file as a control. Mutating the step reference to a name that does not
exist makes actionlint fail at that line, which confirms it is checking
the expression rather than skipping it. actionlint types step outputs as
`{string => string}` and so cannot confirm `package-ecosystem` is a real
output; that was checked against fetch-metadata's README at the pinned v3,
which documents both `package-ecosystem` and `update-type` as
"the highest semver change being made by this PR".
The gate itself was checked by parsing the expression back out of the YAML
and evaluating it over 10 PR shapes: openjd-* minors solo and grouped,
a cargo major, a cargo patch group, and the pip and github-actions cases
that must keep flowing. All 10 behave as intended. GitHub Actions cannot
be run locally, so the live behaviour is unverified until a dependabot PR
opens.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
| # and github-actions groups bundle minor with patch, so gating on patch | ||
| # alone would stop auto-approving nearly every PR from them. | ||
| if: >- | ||
| steps.metadata.outputs.package-ecosystem != 'cargo' || |
There was a problem hiding this comment.
The gate is scoped to cargo, but .github/dependabot.yml only configures pip and github-actions — there is no package-ecosystem: cargo entry, even though Cargo.toml / rust-bindings/Cargo.toml exist. So for Dependabot version updates this condition never fires (no cargo PRs are ever opened), and it only has an effect on Dependabot security update PRs, which are raised independently of dependabot.yml.
Not wrong, but worth being explicit about: if the intent is to actually review cargo 0.x minors, the companion change is adding a cargo ecosystem block to dependabot.yml. Otherwise a reader will assume this gate is doing more than it currently does.
| # alone would stop auto-approving nearly every PR from them. | ||
| if: >- | ||
| steps.metadata.outputs.package-ecosystem != 'cargo' || | ||
| steps.metadata.outputs.update-type == 'version-update:semver-patch' |
There was a problem hiding this comment.
This condition still auto-approves pip and github-actions majors. Per .github/dependabot.yml, the pip-minor-patch / github-actions-minor-patch groups only match minor and patch, so majors fall outside the groups and each get their own PR with update-type == version-update:semver-major — and this if lets those straight through.
The rationale in the comment ("gating on patch alone would stop auto-approving nearly every PR from them") argues against gating on patch for those ecosystems, but it does not argue for auto-approving their majors. A major-level gate costs nothing on the grouped PRs, since those are minor+patch only and so can never report semver-major:
if: >-
steps.metadata.outputs.update-type != 'version-update:semver-major' &&
(steps.metadata.outputs.package-ecosystem != 'cargo' ||
steps.metadata.outputs.update-type == 'version-update:semver-patch')The existing black major ignore rule in dependabot.yml is evidence that pip majors here do break things (Python 3.9 compat), and today a pydantic 2→3 bump would be auto-approved.
Raised from a review finding on #336 / #355 (adding
cargoto dependabot). Independent of those —this stands on its own — but it is what makes them safe.
The problem
auto_approve.ymlapproves any PR authored bydependabot[bot], with no filter on update type:Once dependabot watches
cargo, the highest-risk update class it can produce is a minor bump ofopenjd-expr,openjd-modeloropenjd-sessions. Those crates are 0.x, where cargo treats a minoras breaking, and they carry the API surface the Rust bindings wrap —
openjd-expr0.4.0 carried abreaking coercion change, and
openjd-model0.5.2 changed the job-sideStepScriptwire format. Sothe update that most needs a human to look at it is the one least likely to get one.
To be precise about severity: there is no auto-merge workflow in this repository, so what an
unconditional approval costs today is the "someone looked at this" signal, not the merge itself. A
human still clicks merge. #355 exists specifically to give those bumps their own PR and their own
review, and an unconditional pre-approval undercuts that.
The change
fetch-metadatawas already wired up, so this reads outputs it was collecting and discarding.Because
update-typeis documented as "the highest semver change being made by this PR", agrouped cargo PR containing any minor is held back as well as a solo one.
openjd-expr0.6 -> 0.7Why cargo only, and not a global patch-only gate
The review that raised this suggested
if: steps.metadata.outputs.update-type == 'version-update:semver-patch'unconditionally. That is a bigger change than it looks: thepipandgithub-actionsentries both group minor with patch into a single PR, andupdate-typereportsthe highest change, so those grouped PRs report
semver-minorand would stop being auto-approved.That is nearly every PR from both ecosystems. Their dependencies are also 1.x, where a minor is
additive by semver, so the risk that motivates the cargo gate does not apply to them.
Known gap left in place: a
piporgithub-actionsmajor is still auto-approved, exactlyas it is today.
pydantic3.0 would be waved through, for instance. Gating that is one more clausebut it is a separate decision about a pre-existing behaviour, so it is deliberately not bundled
here. Happy to add it if you would rather it went in now.
Testing
actionlint1.7.12 passes on the modified file, and on the unmodified file frommainlineas acontrol. Mutating the step reference to one that does not exist makes actionlint fail at that line:
so the clean result reflects a check that ran rather than one that skipped the expression. Note
actionlint types step outputs as
{string => string}and therefore cannot confirmpackage-ecosystemis a real output — that was verified againstfetch-metadata's README at thepinned
v3, which documents bothpackage-ecosystemandupdate-type.The gate was then checked by parsing the expression back out of the YAML and evaluating it over the
10 PR shapes in the table above, including the pip and github-actions cases that must keep flowing.
All 10 behave as intended.
GitHub Actions cannot be run locally, so the live behaviour is unverified until a dependabot PR
actually opens.