From 07bd98712931dc05c00ba8ed93570ef0c9380df6 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 21:37:29 -0400 Subject: [PATCH] chore(deps): address Copilot review round 1 on #2232 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings, all valid. `runOutdated` accepted every exit status and returned `result.stdout ?? ""`. A failed `npm outdated` (exit 2 on a registry or config error) also prints nothing, so the empty string parsed to an empty package list — and across five installs a total outage produced output byte-identical to a clean sweep. It now throws on any status other than the documented 0/1. `main()` was untested, which is how that reached review: the helper-only suite passed while the silent-success path was live. It now takes its spawn function as a parameter and is driven by a fake, covering npm failure, create vs. edit, milestone handling, both no-op paths and the missing-repo guard. The no-op path returned before looking for the marker issue, so once every install caught up, a still-open tracking issue kept its obsolete package table indefinitely — contradicting the body's own promise to update in place. It now rewrites through `buildClearedBody()`. It deliberately does not close the issue: the sweep takes no board actions, and closing one whose card a maintainer may have moved would make the board assert work shipped that this script cannot verify shipped. The release step mandated `npm audit fix`, which AGENTS.md forbids. The reason is not `--force`: plain `audit fix` resolves an advisory with no upward escape in range by silently downgrading, as it did to esbuild across three installs in #2058, and `local:gate` has no version-regression check to catch it. The step is now `npm audit` report-only, with fixes applied deliberately via a direct bump or an `overrides` entry. An unmilestoned issue is swept into Incoming, not Todo — Todo asserts a maintainer signed off. Message corrected and pinned by a test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RWMJENjs1mExXE5XPJTEei Signed-off-by: cliffhall --- .claude/skills/release/SKILL.md | 52 ++++---- scripts/dependency-refresh.mjs | 121 +++++++++++++------ scripts/dependency-refresh.test.mjs | 179 +++++++++++++++++++++++++++- 3 files changed, 293 insertions(+), 59 deletions(-) diff --git a/.claude/skills/release/SKILL.md b/.claude/skills/release/SKILL.md index 22cd4e64..d9f083dc 100644 --- a/.claude/skills/release/SKILL.md +++ b/.claude/skills/release/SKILL.md @@ -1,6 +1,6 @@ --- name: release -description: Cut an Inspector v2 release — npm audit fix and bump the version on v2/main first, merge the milestone into main, tag origin/main with a bare x.y.z, and publish via the GitHub Release. Also covers the v1 line and what the publish jobs gate on. +description: Cut an Inspector v2 release — run npm audit and bump the version on v2/main first, merge the milestone into main, tag origin/main with a bare x.y.z, and publish via the GitHub Release. Also covers the v1 line and what the publish jobs gate on. disable-model-invocation: true --- @@ -27,10 +27,11 @@ job or the coverage gate red: There is **one version number** (only the root `package.json` has one — the clients carry none), so the flow is three steps. -## 1. `npm audit fix`, then bump, on `v2/main` — before the milestone merge +## 1. `npm audit`, then bump, on `v2/main` — before the milestone merge Both are part of the milestone's work, so both belong on the develop branch and -flow into `main` together, in the same PR, audit fix first. +flow into `main` together, in the same PR — audit first, so the bump sits on top +of a tree you have just checked. ```sh # Branch from the REMOTE ref, and read the version only once you are on it. @@ -40,32 +41,43 @@ flow into `main` together, in the same PR, audit fix first. git fetch origin v2/main git checkout -b v2/chore/-bump- origin/v2/main -# Audit + fix every install that has its own lockfile — root and each client. -npm audit fix -for c in web cli tui launcher; do (cd "clients/$c" && npm audit fix); done -npm run local:gate # confirm the fixes didn't break anything before bumping +# Audit every install that has its own lockfile — root and each client. +# REPORT ONLY. Read the output; do not let npm mutate the tree (see below). +npm audit --audit-level=high +for c in web cli tui launcher; do (cd "clients/$c" && npm audit --audit-level=high); done node -p "require('./package.json').version" # what is on v2/main now npm version minor --no-git-tag-version # or major / patch; bump only, no tag node -p "require('./package.json').version" # confirm, then PR → v2/main ``` -**Never `npm audit fix --force`.** It will apply a fix outside a dependency's -declared semver range — a major bump a client's code was never adjusted for — -to close an advisory, silently trading a known vulnerability for an unvetted -breaking change. If `npm audit` still reports something `fix` can't resolve -(typically a transitive dependency needing an `overrides` pin per -[Dependency placement](../../../AGENTS.md#dependency-placement)), commit -whatever `fix` did resolve, leave the rest to the dependabot-alert pipeline -(#2229) or a follow-up issue, and say so in the release notes rather than +Anything it reports is fixed **deliberately** — a direct bump, or an +`overrides` entry — and each fix is its own commit, gated by +`npm run local:gate` before the version bump goes on top. + +⚠️ **Do not run `npm audit fix`, with or without `--force`.** +[Dependency placement](../../../AGENTS.md#dependency-placement) rules it out, +and the reason is not `--force`: plain `audit fix` resolves an advisory that has +no *upward* escape inside a declared range by silently **downgrading**. That is +not hypothetical here — `tsup@8.5.1` declares `esbuild: ^0.27.0` against an +advisory covering `0.27.3 - 0.28.0`, and `audit fix` walked three installs back +to `0.27.2` (~700 lines of lockfile churn for a low-severity dev-only advisory; +tried and reverted in #2058, written up in the `local-dev` skill). `local:gate` +does not detect a version regression, so nothing downstream would have caught +it. `--force` is worse again — it applies fixes *outside* the declared range, +trading a known vulnerability for an unvetted major. + +So the release step is the **report**, and the judgment stays with a person. +Where `audit` names something with no in-range fix, pin it with `overrides`; +where it needs a major, that is its own issue and its own PR, not a release-day +edit. If something can't be resolved before the release ships, say so in the +release notes and leave it to the alert-driven pipeline (#2229) rather than forcing it here. This step is a **backstop, not a substitute** for #2229's alert-driven issues — -those are what surface a transitive vulnerability that needs an `overrides` -entry `audit fix` cannot apply on its own, tracked and fixed as their own PRs -well before a release is cut. This step exists for whatever's left standing -right before a release ships, so a release is never gated on remembering to -check `npm audit` separately. +those are what surface a transitive vulnerability well before a release is cut, +tracked and fixed as their own PRs. This exists so a release is never gated on +remembering to check `npm audit` separately. The branch name carries the version you are bumping **to**, so it is named after that second reading. If you want it before branching: diff --git a/scripts/dependency-refresh.mjs b/scripts/dependency-refresh.mjs index 2f1dfa19..6d6ad5a8 100644 --- a/scripts/dependency-refresh.mjs +++ b/scripts/dependency-refresh.mjs @@ -17,11 +17,13 @@ // (ISSUE_MARKER below), which is how a second run in the same month finds and // updates the existing open issue instead of filing a duplicate. // -// `parseOutdated` and `buildIssueBody` are pure and covered by -// `dependency-refresh.test.mjs`; `main()` is the CLI entry point, exercised -// against the real repo only via `workflow_dispatch` in CI, not by the test -// suite (it shells out to `npm outdated` and `gh`, per the workflow-script -// convention `verify-skills.mjs` and its siblings already use). +// `parseOutdated`, `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 +// lookup and both no-op paths. `workflow_dispatch` is a production trigger, +// not a substitute for that (Copilot): the helper-only tests it replaced let a +// non-zero `npm outdated` exit report a clean sweep. import { spawnSync } from "node:child_process"; @@ -85,19 +87,53 @@ export function buildIssueBody(installs) { ].join("\n"); } -function runOutdated(dir) { - const result = spawnSync("npm", ["outdated", "--json"], { +/** + * The body a still-open tracking issue is rewritten to once every install is + * current again. Without it the issue keeps its last package table forever and + * reads as live work that no longer exists (Copilot). + * + * The sweep rewrites rather than closes: it deliberately takes no board + * actions (see the workflow header), and closing an issue whose card a + * maintainer has already moved would make the board claim work shipped that + * this script cannot verify shipped. A maintainer closes it. + * + * @param {string} isoDate the sweep date, as `YYYY-MM-DD` + * @returns {string} + */ +export function buildClearedBody(isoDate) { + return [ + ISSUE_MARKER, + `Every install is up to date as of ${isoDate} — nothing is outdated at the root or in any client.`, + "", + "This issue was filed by an earlier run of the monthly sweep (#2229) and its package table is gone because the packages it listed are no longer behind. Either they were bumped or their ranges caught up; nothing here is outstanding.", + "", + "Safe to close. A later sweep that finds something outdated will refile this body with a fresh table rather than open a duplicate.", + ].join("\n"); +} + +function runOutdated(dir, spawn) { + const result = spawn("npm", ["outdated", "--json"], { cwd: dir, encoding: "utf8", }); - // `npm outdated` exits 1 when it finds anything outdated — that is not a - // failure of the command, only stderr / a thrown parse is. if (result.error) throw result.error; + // `npm outdated` exits 0 when everything is current and 1 when it finds + // something outdated — both are successful runs. Every other status is a + // real failure (a registry or config error exits 2), and it MUST throw + // rather than fall through: a failed run also prints nothing to stdout, so + // accepting it parses to an empty package list and reports a clean no-op. + // With five installs swept in a loop, that turns a total outage into a + // silent "nothing to do" (Copilot). + if (result.status !== 0 && result.status !== 1) { + throw new Error( + `npm outdated failed in ${dir} (exit ${result.status}): ${(result.stderr ?? "").trim()}`, + ); + } return result.stdout ?? ""; } -function findExistingIssue(repo) { - const result = spawnSync( +function findExistingIssue(repo, spawn) { + const result = spawn( "gh", [ "issue", @@ -122,8 +158,8 @@ function findExistingIssue(repo) { return issues.find((i) => i.body?.startsWith(ISSUE_MARKER)) ?? null; } -function currentMilestone(repo) { - const result = spawnSync( +function currentMilestone(repo, spawn) { + const result = spawn( "gh", [ "api", @@ -139,42 +175,55 @@ function currentMilestone(repo) { return result.stdout.trim() || null; } -export function main(repo = process.env.GITHUB_REPOSITORY) { +function editIssue(repo, number, body, spawn) { + const edit = spawn( + "gh", + ["issue", "edit", String(number), "--repo", repo, "--body", body], + { encoding: "utf8" }, + ); + if (edit.status !== 0) + throw new Error(`gh issue edit failed: ${edit.stderr}`); +} + +export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { if (!repo) throw new Error("repo not specified (GITHUB_REPOSITORY unset)"); const installs = INSTALLS.map(({ dir, label }) => ({ label, - packages: parseOutdated(runOutdated(dir)), + packages: parseOutdated(runOutdated(dir, spawn)), })); + // Look the existing issue up BEFORE branching on `body`: the nothing- + // outdated case still has to reach an open issue to clear it. + const existing = findExistingIssue(repo, spawn); const body = buildIssueBody(installs); + if (body === null) { - console.log("dependency-refresh: nothing outdated in any install — no-op"); + if (!existing) { + console.log( + "dependency-refresh: nothing outdated in any install — no-op", + ); + return; + } + editIssue( + repo, + existing.number, + buildClearedBody(new Date().toISOString().slice(0, 10)), + spawn, + ); + console.log( + `dependency-refresh: nothing outdated — cleared stale list on #${existing.number}`, + ); return; } - const existing = findExistingIssue(repo); if (existing) { - const edit = spawnSync( - "gh", - [ - "issue", - "edit", - String(existing.number), - "--repo", - repo, - "--body", - body, - ], - { encoding: "utf8" }, - ); - if (edit.status !== 0) - throw new Error(`gh issue edit failed: ${edit.stderr}`); + editIssue(repo, existing.number, body, spawn); console.log(`dependency-refresh: updated existing #${existing.number}`); return; } - const milestone = currentMilestone(repo); + const milestone = currentMilestone(repo, spawn); const args = [ "issue", "create", @@ -193,12 +242,14 @@ export function main(repo = process.env.GITHUB_REPOSITORY) { ]; if (milestone) args.push("--milestone", milestone); - const create = spawnSync("gh", args, { encoding: "utf8" }); + const create = spawn("gh", args, { encoding: "utf8" }); if (create.status !== 0) throw new Error(`gh issue create failed: ${create.stderr}`); if (!milestone) { + // Unmilestoned means unapproved, so triage sweeps it into `Incoming` — NOT + // `Todo`, which asserts a maintainer signed off (Copilot). console.log( - "dependency-refresh: no open milestone — issue filed unmilestoned, will be swept into Todo at next triage", + "dependency-refresh: no open milestone — issue filed unmilestoned, will be swept into Incoming at next triage", ); } console.log(`dependency-refresh: filed ${create.stdout.trim()}`); diff --git a/scripts/dependency-refresh.test.mjs b/scripts/dependency-refresh.test.mjs index 02ee339a..d0b94ad8 100644 --- a/scripts/dependency-refresh.test.mjs +++ b/scripts/dependency-refresh.test.mjs @@ -1,16 +1,89 @@ -// Unit tests for the pure parsing/formatting halves of dependency-refresh.mjs -// (#2229). The impure half (`main()`, which shells out to `npm` and `gh`) is -// exercised only via `workflow_dispatch` in CI, per the same split -// `verify-skills.mjs` uses. Run via `npm run test:scripts`. +// Tests for dependency-refresh.mjs (#2229) — both the pure parsing/formatting +// helpers and `main()`'s orchestration, the latter driven through the injected +// spawn function so no `npm` or `gh` process is ever started. +// +// `main()` is covered rather than left to `workflow_dispatch` because a +// production trigger is not a test (Copilot): the helper-only suite this +// replaced passed while a non-zero `npm outdated` exit reported a clean sweep. +// Run via `npm run test:scripts`. import { test } from "node:test"; import assert from "node:assert/strict"; import { + buildClearedBody, buildIssueBody, + main, parseOutdated, + INSTALLS, ISSUE_MARKER, } from "./dependency-refresh.mjs"; +/** + * A fake `spawnSync` that answers by command shape and records every call. + * + * @param {object} opts + * @param {Record} [opts.outdated] parsed `npm outdated` payload, per install dir + * @param {number} [opts.outdatedStatus] exit status for every `npm outdated` + * @param {Array<{number:number,body:string}>} [opts.existing] what `gh issue list` returns + * @param {string|null} [opts.milestone] what the milestone lookup returns + */ +function fakeSpawn({ + outdated = {}, + outdatedStatus, + existing = [], + milestone = "v2.6.0", +} = {}) { + const calls = []; + const fn = (cmd, args, opts) => { + calls.push({ cmd, args, cwd: opts?.cwd }); + if (cmd === "npm") { + const payload = outdated[opts.cwd] ?? {}; + const found = Object.keys(payload).length > 0; + return { + // Real `npm outdated` exits 1 precisely when it found something. + status: outdatedStatus ?? (found ? 1 : 0), + stdout: found ? JSON.stringify(payload) : "", + stderr: outdatedStatus ? "ENOTFOUND registry.npmjs.org" : "", + }; + } + if (args[0] === "issue" && args[1] === "list") + return { status: 0, stdout: JSON.stringify(existing), stderr: "" }; + if (args[0] === "api") + return { + status: 0, + stdout: milestone ? `${milestone}\n` : "", + stderr: "", + }; + if (args[0] === "issue" && args[1] === "create") + return { + status: 0, + stdout: "https://github.com/o/r/issues/9\n", + stderr: "", + }; + if (args[0] === "issue" && args[1] === "edit") + return { status: 0, stdout: "", stderr: "" }; + throw new Error(`unexpected spawn: ${cmd} ${args.join(" ")}`); + }; + fn.calls = calls; + return fn; +} + +const ghCall = (spawn, verb) => + spawn.calls.find((c) => c.cmd === "gh" && c.args[1] === verb); + +/** Silence main()'s progress logging; returns the captured lines. */ +function captureLog(run) { + const lines = []; + const original = console.log; + console.log = (...a) => lines.push(a.join(" ")); + try { + run(); + } finally { + console.log = original; + } + return lines; +} + test("parseOutdated returns [] for empty npm-outdated output", () => { assert.deepEqual(parseOutdated(""), []); assert.deepEqual(parseOutdated("{}"), []); @@ -59,3 +132,101 @@ test("buildIssueBody starts with the idempotency marker and skips empty installs assert.ok(!body.includes("### `root`")); assert.ok(body.includes("| `zod` | 3.0.0 | 3.1.0 | 4.0.0 |")); }); + +test("buildClearedBody keeps the marker so the next sweep still finds the issue", () => { + const body = buildClearedBody("2026-09-03"); + assert.ok(body.startsWith(ISSUE_MARKER)); + assert.ok(body.includes("2026-09-03")); + assert.ok(body.includes("Safe to close")); +}); + +test("main throws when npm outdated exits with an undocumented status", () => { + // The regression that motivated covering main(): exit 2 used to fall through + // to empty stdout and report a clean sweep across all five installs. + const spawn = fakeSpawn({ outdatedStatus: 2 }); + assert.throws( + () => captureLog(() => main("o/r", spawn)), + /npm outdated failed in \.\s*\(exit 2\).*ENOTFOUND/s, + ); + assert.equal(ghCall(spawn, "create"), undefined); +}); + +test("main sweeps every install and files one milestoned issue", () => { + const spawn = fakeSpawn({ + outdated: { + "clients/web": { + zod: { current: "3.0.0", wanted: "3.1.0", latest: "4.0.0" }, + }, + }, + }); + const log = captureLog(() => main("o/r", spawn)); + + assert.deepEqual( + spawn.calls.filter((c) => c.cmd === "npm").map((c) => c.cwd), + INSTALLS.map((i) => i.dir), + ); + const create = ghCall(spawn, "create"); + assert.ok(create, "expected an issue to be created"); + assert.deepEqual(create.args.slice(-2), ["--milestone", "v2.6.0"]); + assert.ok(create.args[create.args.indexOf("--body") + 1].includes("`zod`")); + assert.ok(log.some((l) => l.includes("filed"))); +}); + +test("main edits the existing issue instead of filing a duplicate", () => { + const spawn = fakeSpawn({ + outdated: { + ".": { ajv: { current: "8.0.0", wanted: "8.1.0", latest: "8.1.0" } }, + }, + existing: [{ number: 77, body: `${ISSUE_MARKER}\nstale` }], + }); + const log = captureLog(() => main("o/r", spawn)); + + assert.equal(ghCall(spawn, "create"), undefined); + const edit = ghCall(spawn, "edit"); + assert.equal(edit.args[2], "77"); + assert.ok(edit.args[edit.args.length - 1].includes("`ajv`")); + assert.ok(log.some((l) => l.includes("updated existing #77"))); +}); + +test("main clears a still-open issue once everything is current again", () => { + const spawn = fakeSpawn({ + existing: [{ number: 77, body: `${ISSUE_MARKER}\n| \`zod\` | 3.0.0 |` }], + }); + const log = captureLog(() => main("o/r", spawn)); + + const edit = ghCall(spawn, "edit"); + assert.ok(edit, "an open issue must be cleared, not left with a stale table"); + const body = edit.args[edit.args.length - 1]; + assert.ok(body.startsWith(ISSUE_MARKER)); + assert.ok(!body.includes("`zod`")); + assert.ok(log.some((l) => l.includes("cleared stale list on #77"))); +}); + +test("main is a true no-op when nothing is outdated and no issue is open", () => { + const spawn = fakeSpawn(); + const log = captureLog(() => main("o/r", spawn)); + + assert.equal(ghCall(spawn, "edit"), undefined); + assert.equal(ghCall(spawn, "create"), undefined); + assert.ok(log.some((l) => l.includes("no-op"))); +}); + +test("main says Incoming, not Todo, when it files without a milestone", () => { + const spawn = fakeSpawn({ + outdated: { + ".": { ajv: { current: "8.0.0", wanted: "8.1.0", latest: "8.1.0" } }, + }, + milestone: null, + }); + const log = captureLog(() => main("o/r", spawn)); + + const create = ghCall(spawn, "create"); + assert.ok(!create.args.includes("--milestone")); + // Unmilestoned is unapproved; triage parks it in Incoming. + assert.ok(log.some((l) => l.includes("Incoming"))); + assert.ok(!log.some((l) => l.includes("Todo"))); +}); + +test("main refuses to run without a repo", () => { + assert.throws(() => main(undefined, fakeSpawn()), /GITHUB_REPOSITORY unset/); +});