From dde1736702864598bd7a3b848492921d083638e0 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 23:19:58 -0400 Subject: [PATCH] chore(deps): treat every failed release-list response as fatal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Copilot review round 4 on #2239. Both findings are regressions I introduced in round 3 and both are correct. Round 3 moved the lookup from `releases/latest` to the release LIST, to rank by version rather than by GitHub's designated latest — but kept the 404 suppression that only made sense for the old endpoint. The list endpoint answers "this action cuts no releases" with a successful empty array, so a 404 there does not mean that at all: it means the repository is missing or inaccessible, i.e. a `uses:` ref the sweep cannot check. Converting it to null silently dropped a broken or renamed action from the flow that replaced Dependabot — the same silent-success shape this PR has been closing everywhere else, reintroduced one layer down. Every non-zero status is now fatal and `isMissingRelease` is gone entirely; the benign no-releases case is the successful empty array, covered by its own test. `buildClearedBody` also overstated its all-clear. Refs pinned to a commit SHA or a branch return null from `parseVersionRef` and are never ranked, so "no workflow `uses:` ref is behind" asserted a check that had not happened for them. It now says version-pinned refs and names the exclusion explicitly. Both guards are mutation-checked: restoring the 404 suppression, and dropping the SHA/branch qualification, each fail exactly one test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NGtzPg3UxMLszysQXXfqax Signed-off-by: cliffhall --- scripts/dependency-refresh.mjs | 41 +++++++++++------------------ scripts/dependency-refresh.test.mjs | 33 ++++++++++++----------- 2 files changed, 32 insertions(+), 42 deletions(-) diff --git a/scripts/dependency-refresh.mjs b/scripts/dependency-refresh.mjs index 17b540e1..ab54ec1c 100644 --- a/scripts/dependency-refresh.mjs +++ b/scripts/dependency-refresh.mjs @@ -29,8 +29,8 @@ // updates the existing open issue instead of filing a duplicate. // // `parseOutdated`, `parseActionRefs`, `parseVersionRef`, `isActionStale`, -// `staleActions`, `isMissingRelease`, `highestVersionTag`, `buildIssueBody` -// and `buildClearedBody` are pure. `main()` +// `staleActions`, `highestVersionTag`, `buildIssueBody` and +// `buildClearedBody` are pure. `main()` // shells out to `npm outdated` and `gh`, so it takes its spawn function as a // parameter (defaulting to the real one) and `dependency-refresh.test.mjs` // drives it with a fake — covering npm failure, create vs. edit, the milestone @@ -221,7 +221,9 @@ export function buildIssueBody(installs, actions = []) { export function buildClearedBody(isoDate) { return [ ISSUE_MARKER, - `Everything this sweep watches is current as of ${isoDate} — no npm package is outdated at the root or in any client, and no workflow \`uses:\` ref is behind its action's highest release.`, + `Everything this sweep can check is current as of ${isoDate} — no npm package is outdated at the root or in any client, and no version-pinned workflow \`uses:\` ref is behind its action's highest release.`, + "", + "Refs pinned to a commit SHA or a branch are deliberately **not** covered by that statement: neither can be ranked against a release tag, so this sweep says nothing about them either way.", "", "This issue was filed by an earlier run of the monthly sweep (#2229, #2235) and its tables are gone because nothing they listed is behind any more. Either it was bumped or the ranges caught up; nothing here is outstanding.", "", @@ -258,24 +260,6 @@ function collectActionRefs() { ); } -/** - * Is this failed release lookup the expected "publishes no releases" answer? - * - * `repos///releases/latest` 404s when an action has never cut a - * GitHub release, which is a legitimate state and must not fail the sweep. - * Every OTHER failure — a rate limit, an expired token, a transient 5xx — must, - * because treating it as "no release" is indistinguishable from "not stale": - * the sweep would exit green having silently checked nothing, and since this - * repo's actions are all on their latest major the empty section would look - * exactly like a healthy run (Copilot). - * - * @param {string} stderr stderr from a non-zero `gh api` call - * @returns {boolean} - */ -export function isMissingRelease(stderr) { - return /HTTP 404/.test(stderr); -} - /** * The highest parseable version among these tags, or `null` if none parse. * @@ -337,12 +321,17 @@ function latestReleaseTag(action, spawn) { { encoding: "utf8" }, ); if (result.error) throw result.error; + // EVERY non-zero status is fatal, 404 included. This is the release *list* + // endpoint, which answers "no releases" with a successful empty array — so a + // 404 here does not mean "this action cuts no releases", it means the + // repository is missing or inaccessible, i.e. a `uses:` ref this sweep + // cannot check at all. Converting that to `null` would silently drop a + // broken or renamed action from the sweep that replaced Dependabot + // (Copilot). The empty-array case is already handled by the parse below. if (result.status !== 0) { - const stderr = result.stderr ?? ""; - // A repo with no releases returns `[]`, not a 404 — but a renamed or - // deleted action really is gone, and that is not a reason to fail. - if (isMissingRelease(stderr)) return null; - throw new Error(`release lookup for ${repo} failed: ${stderr.trim()}`); + throw new Error( + `release lookup for ${repo} failed (exit ${result.status}): ${(result.stderr ?? "").trim()}`, + ); } const tags = result.stdout .split("\n") diff --git a/scripts/dependency-refresh.test.mjs b/scripts/dependency-refresh.test.mjs index 4cf79226..55af8354 100644 --- a/scripts/dependency-refresh.test.mjs +++ b/scripts/dependency-refresh.test.mjs @@ -14,7 +14,6 @@ import { buildIssueBody, highestVersionTag, isActionStale, - isMissingRelease, main, parseActionRefs, parseOutdated, @@ -220,20 +219,6 @@ test("staleActions dedupes, drops actions with no known release and sorts", () = ); }); -test("isMissingRelease suppresses only a 404, never a real API failure", () => { - // A 404 is the legitimate "this action cuts no GitHub releases" answer. - assert.equal(isMissingRelease("gh: Not Found (HTTP 404)"), true); - // Everything else must fail the sweep rather than read as "not stale" — an - // empty actions section is indistinguishable from a healthy run (Copilot). - assert.equal( - isMissingRelease("gh: API rate limit exceeded (HTTP 403)"), - false, - ); - assert.equal(isMissingRelease("gh: Bad credentials (HTTP 401)"), false); - assert.equal(isMissingRelease("gh: Server Error (HTTP 502)"), false); - assert.equal(isMissingRelease(""), false); -}); - test("buildIssueBody returns null when every install is up to date", () => { assert.equal( buildIssueBody([ @@ -423,6 +408,9 @@ test("buildClearedBody speaks for both halves of the sweep", () => { // npm-only wording here would assert an all-clear the sweep never checked. assert.match(body, /npm package/); assert.match(body, /uses:/); + // Must not claim every ref was verified: SHA- and branch-pinned refs are + // never ranked against a release (Copilot). + assert.match(body, /SHA or a branch/); }); test("main fails the sweep when a release lookup errors, rather than reporting no stale actions", () => { @@ -439,11 +427,24 @@ test("main fails the sweep when a release lookup errors, rather than reporting n ); }); -test("main treats a 404 release lookup as 'this action cuts no releases'", () => { +test("main fails on a 404 release lookup — a missing action repo is not 'no releases'", () => { + // The release LIST endpoint answers "no releases" with a successful empty + // array, so a 404 means the repository is missing or inaccessible. Treating + // it as benign would silently drop a broken or renamed action from the sweep + // that replaced Dependabot (Copilot). const spawn = fakeSpawn({ releasesStatus: 1, releasesStderr: "gh: Not Found (HTTP 404)", }); + assert.throws( + () => captureLog(() => main("o/r", spawn)), + /release lookup for .* failed/, + ); +}); + +test("main treats an empty release list as 'this action cuts no releases'", () => { + // The benign case: status 0 with no tags. The action is simply not ranked. + const spawn = fakeSpawn({ releaseTags: [] }); const log = captureLog(() => main("o/r", spawn)); assert.match(log.join("\n"), /no-op/); });