fix(janitor): classify grouped PRs per-dependency, fail-closed (supersedes #23) - #54
fix(janitor): classify grouped PRs per-dependency, fail-closed (supersedes #23)#54asachs01 wants to merge 1 commit into
Conversation
Supersedes #23, rebased onto post-#36/#38 main. classify() no longer treats "group" in a PR title as proof the PR is minor/patch. It parses the body's per-dep "Updates `pkg` from A to B" markers and requires every one to be same-major, failing closed on any cross-major, unparseable marker, zero markers, or failed body fetch. This was live, not theoretical. Of the 118 PRs a dry run would merge, 108 are grouped, and 69 of those 108 (64%) contain a cross-major bump across 62 repos. is_dev_major's allowlist is not a defence: classify() returned ELIGIBLE for grouped PRs before the dev-major check ran, making the grouped path strictly more permissive than the single-package path. Two carry runtime, non-allowlisted majors -- ironscales-mcp#38 and salesbuildr-mcp#55 both bump the Docker base image node 22-alpine -> 26-alpine. Same hole that broke main via node-datto-rmm#46. #36 guarded only grouped PRs with NO CI; grouped + green CI still rode the shortcut. Two changes beyond #23 as authored: - Body fetch over REST (gh api), not GraphQL (gh pr view). Fail-closed is right for a corrupt body, but a GraphQL outage would fail-close every grouped PR and stall the backlog. Observed live 2026-08-17: GraphQL 503 for hours while REST stayed healthy -- 40/108 unclassifiable via gh pr view, 0/108 via gh api. - Version regex uses [^[:space:]], not [^`[:space:]]. The latter is the literal set { ` [ : s p a c e ] } inside a bracket expression and does not exclude whitespace, so the capture runs greedy across " to ". A scan built on it, run under macOS bash 3.2 where BASH_REMATCH also never populated, reported all 108 grouped PRs clean -- the false negative that initially mis-classified this as latent. Adds dependabot-janitor.test.sh: 14 assertions, fixtures from real Dependabot bodies, with a BASH_REMATCH self-check that refuses to run rather than false-pass under bash 3.2.
|
Automated review (forge): This is the highest-stakes diff of the five — it changes Correctness — the fix itself is right:
Process note, not a defect: the "latent vs. live" self-correction in the PR body (macOS bash 3.2 not populating CI: actionlint is green on both runs, but Tests: 14 assertions in Collision risk: this PR and #50 both edit Verdict: safe to merge pending the |
…ent overlap (#69) Three open PRs (#26, #50, #54) all showed mergeable=CONFLICTING against current main. None of their real changes overlap line-for-line — the conflicts are purely stale-diff artifacts of main having moved on since each PR's fork point (#66's dual-org repo-enumeration restructure in particular). This reapplies all three fixes' actual intent by hand onto current main, in dependency order (both #50 and #54 touch dependabot-janitor.sh). #54 (classify() group-PR hardening, supersedes #23): a Dependabot "group" PR title no longer proves minor/patch. classify() now fetches the PR body via REST and requires every per-dependency "from A to B" marker to be same-major, failing closed otherwise. Reapplied verbatim except the body fetch now calls `repos/$repo/pulls/$num` directly (since $repo is already "org/name" post-#66) instead of #54's original `repos/$ORG/$repo/...`. Adds dependabot-janitor.test.sh (14 assertions, real fixtures) — verified byte-identical to #54's original test file aside from that same $ORG-> $repo adaptation; all 14 assertions pass unmodified. #50 (cortextos/conduit scope carve-out): reapplied by hand onto the current multi-org enumeration loop (#66) rather than #50's own stale single-org grep pipeline — added `|^cortextos$|^conduit$` to the still-present grep -E inside the `for _org in $ORGS` loop. #26 (mcp-server-release.yml always() gating): the release job's "Detect released version" step now runs with `if: always()`, and the docker job's gate is `if: always() && needs.release.outputs.released == 'true'`, so a non-fatal failure after semantic-release cuts a release (e.g. a flaky git-notes push) can't silently skip artifact publishing. Investigated the apparent second `id: detect` in the current file: it is an unrelated step in the (post-#26) mcpb job that checks for a pack:mcpb script, not a duplicate release-detection block — #26's fix applies to exactly the one occurrence it always targeted. Flagged in the CHANGELOG that mcpb's own released=='true' gate has an analogous latent exposure, left out of scope here since it predates neither #26 nor this reconciliation. Verified: shellcheck clean on dependabot-janitor.sh (baseline was already clean); dependabot-janitor.test.sh carries only pre-existing info-level notices inherited from #54's original file (one pre-existing SC2034 warning was fixed by dropping the now-unused $ORG var); all 14 tests pass; mcp-server-release.yml parses as valid YAML. Refs: #26, #50, #54
1 participant
Supersedes #23, rebased onto post-#36/#38
main. Opened as a new branch rather than force-pushing #23's branch.This hole is live, not latent
classify()onmainstill treats the word "group" in a PR title as proof the PR is minor/patch:Measured against the current backlog on 2026-08-17 — of the 118 PRs a dry run would merge, 108 are grouped, and:
Every one of those 69 would merge without a single dependency being inspected.
is_dev_major's allowlist is not a defence here.classify()returnsELIGIBLEfor grouped PRs before the dev-major check runs, so the grouped path is strictly more permissive than the single-package path sitting right beside it. Two of the 69 carry a genuinely runtime, non-allowlisted major:Four majors of Docker base image, straight to production on merge.
Sample of the rest:
This is the same hole that broke
mainvianode-datto-rmm#46on 2026-07-21. #36 responded with a downstream guard, but only for grouped PRs with no CI — a grouped PR with green CI still rode the shortcut untouched.Two changes beyond #23 as authored
1. Body fetch over REST (
gh api), not GraphQL (gh pr view).Fail-closed is correct for a corrupt body, but during a GraphQL outage every grouped PR fails closed and the whole backlog stalls behind a dependency this classifier doesn't need. Observed live while building this: GraphQL 503'd for hours while REST stayed healthy — 40 of 108 PRs unclassifiable via
gh pr view, 0 of 108 viagh api.**2. Version regex uses
[^[:space:]], not[^\[:space:]].** Inside a bracket expression the latter is the literal set{ ` [ : s p a c e ] }— it does **not** exclude whitespace, so the capture runs greedy across" to "` and yields nothing usable. Merging #23 as authored would have fail-closed a large share of legitimate grouped PRs.How the "latent" misdiagnosis happened
Worth recording, because it's the same failure shape as the bug being fixed. My first scan reported all 108 grouped PRs clean. It was running under macOS
/bin/bash3.2, whereBASH_REMATCHdoesn't populate in this construct — marker detection worked (deps=10), but every version comparison was"" vs "", which compares equal.A validator that reads nothing and a validator that finds nothing wrong emit identical output. Same shape as #38's vacuous-green checks, and as the 27-day janitor outage that produced zero failed runs.
dependabot-janitor.test.shnow refuses to run unless a known-cross-major probe parses first.Tests
.github/scripts/dependabot-janitor.test.sh— 14 assertions, 0 failures. Fixtures are real Dependabot bodies (abnormal-mcp#50,salesbuildr-mcp#55), not invented shapes, per the CHANGELOG's own lesson about fixtures "tested only against invented shapes that encoded the same wrong assumption."Grouped PRs that are genuinely same-major still classify
ELIGIBLE— covered by a test.Expected effect on re-enable
This will reduce the merge set: 69 of the 118 move from auto-merge to the
majorsbucket for human review. That is the intended outcome, not a regression.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.