Skip to content

fix(janitor): classify grouped PRs per-dependency, fail-closed (supersedes #23) #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
asachs01 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(janitor): classify grouped PRs per-dependency, fail-closed (supersedes #23) #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 52 additions & 6 deletions .github/scripts/dependabot-janitor.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,58 @@ echo "Scanning ${#REPOS[@]} repositories in scope..."
# Return the leading integer (major version) of a semver-ish string.
major_of() { sed -E 's/^[^0-9]*([0-9]+).*/\1/' <<<"$1"; }

# Classify a PR title as ELIGIBLE (patch/minor) or MAJOR.
# Grouped Dependabot PRs are configured to contain only minor/patch updates.
# Classify a PR as ELIGIBLE (patch/minor) or MAJOR.
#
# GAP-1 hardening (opened as #23 2026-05-26, rebased 2026-08-17): a "group" PR is
# ELIGIBLE only if EVERY dep inside it is same-major. The TITLE does not list the
# deps -- "bump the dev-dependencies group with 12 updates" says nothing about what
# is in it -- so parse the PR BODY's per-dep "from A to B" markers and major_of each.
# FAIL-CLOSED: any cross-major, any unparseable marker, zero markers, or a failed
# body fetch -> MAJOR (report, never merge), matching classify()'s posture elsewhere.
#
# The shortcut this replaces (`group in title -> ELIGIBLE, unconditionally`) is how
# node-datto-rmm#46 auto-merged a hidden typescript major and broke main on
# 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 title shortcut with no per-dep
# check at all. This closes the classifier itself.
#
# Update-type-scoped groups (e.g. npm-minor-patch) list only minor/patch bumps and
# stay ELIGIBLE -- no behaviour change. Only a PATTERN-group bundling a major flips.
# Measured 2026-08-17 against the live backlog: of 118 would-merge PRs, 108 are
# grouped, and all 108 parse clean same-major. This is defence in depth against a
# future group config, not a fix for a currently-firing break.
classify() {
local title="$1"
if grep -qiE '\bgroup\b' <<<"$title"; then echo ELIGIBLE; return; fi
# "... from A.B.C to D.E.F"
local title="$1" num="$2" repo="$3"
if grep -qiE '\bgroup\b' <<<"$title"; then
# Fetch the body over REST, not `gh pr view` (which is GraphQL). Fail-closed
# is correct for a corrupt body, but during a GitHub GraphQL outage EVERY
# grouped PR would fail-closed and the whole backlog would stall behind a
# dependency this classifier does not actually need. Observed live
# 2026-08-17: GraphQL 503 for hours while REST stayed healthy, which made
# 40 of 108 grouped PRs unclassifiable in a scan using `gh pr view`.
local body line deps=0 ok=0
body="$(gh api "repos/$ORG/$repo/pulls/$num" --jq '.body // ""' 2>/dev/null </dev/null)" || { echo MAJOR; return; }
[[ -n "$body" ]] || { echo MAJOR; return; } # empty/absent body -> fail-closed
while IFS= read -r line; do
# dependabot per-dep marker lines only: "Updates `pkg` from A to B"
[[ "$line" =~ (Updates|Bumps)[[:space:]]+\` ]] || continue
deps=$((deps+1))
# Deliberately NOT `[^\`[:space:]]`: inside a bracket expression that is the
# literal set { ` [ : s p a c e ] }, which does NOT exclude whitespace, so the
# capture runs greedy across " to " and yields nothing usable. Real Dependabot
# bodies write bare versions ("from 9.39.4 to 10.0.1"), so the simple class is
# both correct and sufficient. Verified against real bodies under bash 5.
if [[ "$line" =~ from[[:space:]]+([0-9][^[:space:]]*)[[:space:]]+to[[:space:]]+([0-9][^[:space:]]*) ]] &&
[[ "$(major_of "${BASH_REMATCH[1]}")" == "$(major_of "${BASH_REMATCH[2]}")" ]]; then
ok=$((ok+1))
else
echo MAJOR; return # marker line with no parseable same-major pair -> fail-closed
fi
done <<<"$body"
[[ "$deps" -gt 0 && "$ok" -eq "$deps" ]] && echo ELIGIBLE || echo MAJOR
return
fi
# single-update PR: "... from A.B.C to D.E.F"
if [[ "$title" =~ from[[:space:]]+([0-9][^[:space:]]*)[[:space:]]+to[[:space:]]+([0-9][^[:space:]]*) ]]; then
local from="${BASH_REMATCH[1]}" to="${BASH_REMATCH[2]}"
if [[ "$(major_of "$from")" == "$(major_of "$to")" ]]; then echo ELIGIBLE; else echo MAJOR; fi
Expand Down Expand Up @@ -74,7 +120,7 @@ for repo in "${REPOS[@]}"; do
label="$repo #$num — $title"

devmajor=0
if [[ "$(classify "$title")" == "MAJOR" ]]; then
if [[ "$(classify "$title" "$num" "$repo")" == "MAJOR" ]]; then
if is_dev_major "$title"; then
devmajor=1 # dev/CI tooling major — eligible for auto-merge on green CI
else
Expand Down
123 changes: 123 additions & 0 deletions .github/scripts/dependabot-janitor.test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
#!/usr/bin/env bash
# Unit tests for the pure classifiers in dependabot-janitor.sh.
#
# Run with: bash .github/scripts/dependabot-janitor.test.sh
# Requires bash >= 4 (see the BASH_REMATCH self-check below).
#
# The functions are extracted with sed rather than sourced, so the production
# script needs no test-only guard and the sweep never runs here. `gh` is stubbed
# as a shell function, which bash resolves ahead of PATH.
#
# Fixtures are REAL Dependabot bodies captured 2026-08-17, not invented shapes.
# The CHANGELOG records what invented fixtures cost last time: a check "tested
# only against invented shapes that encoded the same wrong assumption."

set -uo pipefail
SCRIPT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/dependabot-janitor.sh"
ORG="wyre-technology"

# --- self-check ------------------------------------------------------------
# macOS /bin/bash is 3.2, where BASH_REMATCH does not populate here. A scan run
# under it silently compared "" to "" and reported every PR clean -- a false
# negative that hid 42 cross-major PRs on 2026-08-17. Refuse to run rather than
# report a comforting wrong answer.
_probe='Updates `x` from 9.1.0 to 10.0.0'
if [[ "$_probe" =~ from[[:space:]]+([0-9][^[:space:]]*)[[:space:]]+to[[:space:]]+([0-9][^[:space:]]*) ]]; then
if [[ -z "${BASH_REMATCH[1]:-}" ]]; then
echo "FATAL: BASH_REMATCH not populated under bash $BASH_VERSION — tests would false-pass." >&2
exit 1
fi
else
echo "FATAL: probe regex did not match under bash $BASH_VERSION." >&2
exit 1
fi

# major_of() is a one-liner (its closing brace is not at column 0), so it gets a
# single-line print. Using a /,/^}/ range for it would run on and swallow
# classify() as well, then the second range would emit classify() a second time —
# producing duplicated, syntactically broken input to eval.
eval "$(sed -n '/^major_of()/p; /^classify()/,/^}/p' "$SCRIPT")"
declare -F classify >/dev/null || { echo "FATAL: could not extract classify() from $SCRIPT" >&2; exit 1; }

pass=0; fail=0
BODY=""
# Stub: classify() fetches the PR body via `gh api ... /pulls/N`.
gh() { printf '%s' "$BODY"; }

check() { # check <desc> <expected> <title> [num] [repo]
local desc="$1" want="$2" title="$3" num="${4:-1}" repo="${5:-test-mcp}"
local got; got="$(classify "$title" "$num" "$repo")"
if [[ "$got" == "$want" ]]; then
pass=$((pass+1)); printf ' ok %s\n' "$desc"
else
fail=$((fail+1)); printf ' FAIL %s — want %s, got %s\n' "$desc" "$want" "$got"
fi
}

echo "major_of()"
[[ "$(major_of 9.39.4)" == "9" ]] && { pass=$((pass+1)); echo " ok 9.39.4 -> 9"; } || { fail=$((fail+1)); echo " FAIL 9.39.4"; }
[[ "$(major_of 10.0.1)" == "10" ]] && { pass=$((pass+1)); echo " ok 10.0.1 -> 10"; } || { fail=$((fail+1)); echo " FAIL 10.0.1"; }
[[ "$(major_of ^6.0.3)" == "6" ]] && { pass=$((pass+1)); echo " ok ^6.0.3 -> 6"; } || { fail=$((fail+1)); echo " FAIL ^6.0.3"; }

echo
echo "classify() — single-package PRs (unchanged behaviour)"
check "patch bump -> ELIGIBLE" ELIGIBLE 'chore(deps): bump foo from 1.2.3 to 1.2.4'
check "minor bump -> ELIGIBLE" ELIGIBLE 'chore(deps): bump foo from 1.2.3 to 1.3.0'
check "major bump -> MAJOR" MAJOR 'chore(deps): bump foo from 1.2.3 to 2.0.0'
check "unparseable -> MAJOR" MAJOR 'chore(deps): update everything'

echo
echo "REGRESSION: grouped PRs with a hidden cross-major (the #23 hole)"
# Real body, abnormal-mcp#50, captured 2026-08-17. main's blanket
# `group in title -> ELIGIBLE` shortcut would auto-merge this untouched.
BODY='Bumps the dev-dependencies group with 10 updates:

Updates `@eslint/js` from 9.39.4 to 10.0.1
Updates `@modelcontextprotocol/ext-apps` from 1.7.4 to 1.7.5
Updates `@semantic-release/changelog` from 6.0.3 to 7.0.0
'
check "grouped w/ @eslint/js 9->10 + changelog 6->7 -> MAJOR" MAJOR \
'deps-dev(deps-dev): bump the dev-dependencies group with 10 updates'

# Real body shape, salesbuildr-mcp#55 — a Docker base-image major, and the one
# offender NOT covered by is_dev_major's dev/CI allowlist.
BODY='Bumps node from 22-alpine to 26-alpine.

Updates `node` from 22-alpine to 26-alpine
'
check "grouped w/ runtime node 22->26 base image -> MAJOR" MAJOR \
'chore(deps): bump the docker group with 1 update'

echo
echo "classify() — grouped PRs that are genuinely same-major stay ELIGIBLE"
BODY='Bumps the dev-dependencies group with 3 updates:

Updates `vitest` from 3.1.0 to 3.2.4
Updates `typescript` from 5.6.2 to 5.7.0
Updates `@types/node` from 22.1.0 to 22.9.0
'
check "all same-major -> ELIGIBLE (no behaviour change)" ELIGIBLE \
'deps-dev(deps-dev): bump the dev-dependencies group with 3 updates'

echo
echo "classify() — fail-closed paths"
BODY=''
check "empty body -> MAJOR" MAJOR 'bump the x group with 2 updates'
BODY='Some prose with no dependabot markers at all.'
check "zero parseable markers -> MAJOR" MAJOR 'bump the x group with 2 updates'
BODY='Bumps the group.

Updates `weird-pkg` from latest to newest
'
check "unparseable marker -> MAJOR" MAJOR 'bump the x group with 1 update'
BODY='Bumps the group.

Updates `ok-pkg` from 1.0.0 to 1.1.0
Updates `bad-pkg` from notasemver to 2.0.0
'
check "partial parse does not pass as all-clean -> MAJOR" MAJOR \
'bump the x group with 2 updates'

echo
printf 'passed=%d failed=%d\n' "$pass" "$fail"
[[ "$fail" -eq 0 ]]
44 changes: 44 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,50 @@ here. The format is based on

### Fixed

- **`dependabot-janitor.sh`**: `classify()` no longer treats the word "group" in
a PR title as proof that the PR is minor/patch. It now parses the body's
per-dependency `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. Supersedes #23, rebased onto post-#36/#38 `main`.

**This was live, not theoretical.** Measured against the current backlog on
2026-08-17: of the 118 PRs a dry run would merge, 108 are grouped, and **69 of
those 108 (64%) contain at least one cross-major bump**, across 62 repos. The
blanket shortcut would have merged every one of them without inspecting a
single dependency.

`is_dev_major`'s allowlist is **not** a defence here — `classify()` returns
`ELIGIBLE` for grouped PRs *before* the dev-major check runs, so the grouped
path was strictly more permissive than the single-package path it sits beside.
Two of the 69 carry a genuinely runtime, non-allowlisted major:
`ironscales-mcp#38` and `salesbuildr-mcp#55`, both bumping the Docker base
image `node` from `22-alpine` to `26-alpine` — four majors, straight to
production on merge.

This is the same hole that broke `main` via `node-datto-rmm#46` on 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 title shortcut untouched.

Two changes beyond #23 as authored:
- **Body fetch moved from `gh pr view` (GraphQL) to `gh api` (REST).**
Fail-closed is right for a corrupt body, but during a GraphQL outage *every*
grouped PR would fail closed and the whole backlog would stall behind a
dependency the classifier does not need. Observed live on 2026-08-17: GraphQL
returned 503 for hours while REST stayed healthy, making 40 of 108 PRs
unclassifiable under `gh pr view` and 0 of 108 under `gh api`.
- **Version-capture regex uses `[^[:space:]]`, not `[^\`[:space:]]`.** Inside a
bracket expression the latter is the literal set `` { ` [ : s p a c e ] } ``,
which does not exclude whitespace, so the capture runs greedy across `" to "`
and yields nothing. A scan built on that construct — run under macOS
`/bin/bash` 3.2, where `BASH_REMATCH` additionally never populated —
reported all 108 grouped PRs clean. That false negative is what initially
mis-classified this hole as latent. `dependabot-janitor.test.sh` now refuses
to run unless a known-cross-major probe parses first.

Adds `.github/scripts/dependabot-janitor.test.sh` — 14 assertions, fixtures
taken from real Dependabot bodies (`abnormal-mcp#50`, `salesbuildr-mcp#55`)
rather than invented shapes.

- **`mcp-server-release.yml`**: the digest-verification check tested the wrong
key casing and rejected every legitimate single-platform image. The payload is
a marshalled OCI image config, whose top-level key is lowercase `config` (with
Expand Down
Loading