chore(deps): address Copilot review round 1 on #2232

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RWMJENjs1mExXE5XPJTEei
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-03 21:37:29 -04:00 • committed by GitHub
commit 07bd987129
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 293 additions and 59 deletions

View file

@ -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/<ISSUE>-bump-<X-Y-Z> 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:

View file

@ -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()}`);

View file

@ -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<string,unknown>} [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/);
});