diff --git a/.github/scripts/dependabot-janitor.sh b/.github/scripts/dependabot-janitor.sh index 5b5f512..70354c9 100755 --- a/.github/scripts/dependabot-janitor.sh +++ b/.github/scripts/dependabot-janitor.sh @@ -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 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 @@ -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 diff --git a/.github/scripts/dependabot-janitor.test.sh b/.github/scripts/dependabot-janitor.test.sh new file mode 100755 index 0000000..17bd4da --- /dev/null +++ b/.github/scripts/dependabot-janitor.test.sh @@ -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 [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 ]] diff --git a/CHANGELOG.md b/CHANGELOG.md index 8eb665b..b2618b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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