chore(deps): npm audit fix before release bump; monthly npm-outdated issue - #2232
chore(deps): npm audit fix before release bump; monthly npm-outdated issue#2232cliffhall wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Command failures can be silently reported as success, orchestration is untested, and the release procedure conflicts with dependency-management safeguards.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds release-time dependency auditing and replaces npm Dependabot PRs with a monthly tracking issue.
Changes:
- Adds a monthly
npm outdatedworkflow and issue-management script. - Removes npm entries from Dependabot configuration.
- Adds
npm audit fixto release instructions and tests formatting helpers.
File summaries
| File | Description |
|---|---|
scripts/dependency-refresh.mjs |
Collects outdated packages and manages the tracking issue. |
scripts/dependency-refresh.test.mjs |
Tests parsing and issue-body formatting. |
.github/workflows/dependency-refresh.yml |
Schedules the monthly dependency sweep. |
.github/dependabot.yml |
Disables npm version-update PRs. |
.claude/skills/release/SKILL.md |
Adds dependency auditing to releases. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 5
- Review effort level: Balanced
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>
Copilot review round 1 — all five addressed (
|
Copilot rounds 2 and 3 — cleanBoth came back 🟢 Approval recommended with 0 new comments against Round 3 was requested deliberately rather than stopping at round 2: Copilot's inline comments can land after its review body, so a single zero-comment round is not on its own evidence that a round produced nothing. Two consecutive clean rounds is the stop condition.
Review is done. One open item for the maintainer, carried over from round 1 rather than resolved by it: finding #4 narrowed this PR from "add |
Closes #2235. Switches the old dependency flow off now that #2232 landed the replacement. Removes the five npm ecosystem entries (root plus each client under clients/*) and, resolving the question #2229 left open, the github-actions entry as well — so .github/dependabot.yml goes away outright rather than being emptied, which its schema does not allow. The github-actions entry had exactly the property #2229 exists to remove: it opened a grouped monthly PR carrying no `Closes #N` and no board card, the one standing exception to "every PR references an issue". Deleting it unreplaced would have left 9 actions unwatched, and `npm outdated` says nothing about actions, so the monthly sweep now also checks every `uses:` ref under .github/workflows and renders the stale ones as one more section of the same tracking issue. Ranking comes from the release LIST, not `releases/latest`. That endpoint returns the release GitHub designates most recent, not the greatest version, so an action publishing a maintenance release for an older major (a v6.9.1 cut after v8.0.0) would make a workflow pinned to v7 compare against v6 and read as current — silently missing a whole major upgrade, the one thing this check exists to catch. Staleness is compared only to the precision the ref specifies. `v7` is a moving major tag that GitHub repoints at every v7.x release, so `v7` against a highest of `v7.0.1` is current and only `v8` makes it stale; an exactly-pinned `v7.0.0` is behind `v7.0.1`; a SHA pin is deliberately immovable and is never reported. A release lookup suppresses only a 404 — the legitimate "this action has never cut a release" answer — and throws on anything else. Treating a rate limit or an expired token as "no release" is indistinguishable from "not stale", and since every action here already sits on its latest major, the resulting empty section is byte-identical to a healthy run. `buildClearedBody` now speaks for both halves: once actions are in scope, its npm-only wording would assert a clean bill of health the sweep never checked. Dependabot security updates are unaffected — they are configured in repo settings, not in this file, and kept working while it was missing entirely (#1833, #1840). That note moves into the workflow header rather than dying with the file; #2233 is where they are turned off deliberately. The script header, the workflow header and the generated issue body all say version-update PRs rather than claiming Dependabot is replaced wholesale. Also renames the workflow's npm-outdated job to dependency-sweep now that the sweep is no longer npm-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGtzPg3UxMLszysQXXfqax Signed-off-by: cliffhall <cliff@futurescale.com>
Closes #2235. Switches the old dependency flow off now that #2232 landed the replacement. Removes the five npm ecosystem entries (root plus each client under clients/*) and, resolving the question #2229 left open, the github-actions entry as well — so .github/dependabot.yml goes away outright rather than being emptied, which its schema does not allow. The github-actions entry had exactly the property #2229 exists to remove: it opened a grouped monthly PR carrying no `Closes #N` and no board card, the one standing exception to "every PR references an issue". Deleting it unreplaced would have left 9 actions unwatched, and `npm outdated` says nothing about actions, so the monthly sweep now also checks every `uses:` ref under .github/workflows and renders the stale ones as one more section of the same tracking issue. Ranking comes from the release LIST, not `releases/latest`. That endpoint returns the release GitHub designates most recent, not the greatest version, so an action publishing a maintenance release for an older major (a v6.9.1 cut after v8.0.0) would make a workflow pinned to v7 compare against v6 and read as current — silently missing a whole major upgrade, the one thing this check exists to catch. Staleness is compared only to the precision the ref specifies. `v7` is a moving major tag that GitHub repoints at every v7.x release, so `v7` against a highest of `v7.0.1` is current and only `v8` makes it stale; an exactly-pinned `v7.0.0` is behind `v7.0.1`; a SHA pin is deliberately immovable and is never reported. A release lookup suppresses only a 404 — the legitimate "this action has never cut a release" answer — and throws on anything else. Treating a rate limit or an expired token as "no release" is indistinguishable from "not stale", and since every action here already sits on its latest major, the resulting empty section is byte-identical to a healthy run. `buildClearedBody` now speaks for both halves: once actions are in scope, its npm-only wording would assert a clean bill of health the sweep never checked. Dependabot security updates are unaffected — they are configured in repo settings, not in this file, and kept working while it was missing entirely (#1833, #1840). That note moves into the workflow header rather than dying with the file; #2233 is where they are turned off deliberately. The script header, the workflow header and the generated issue body all say version-update PRs rather than claiming Dependabot is replaced wholesale. Also renames the workflow's npm-outdated job to dependency-sweep now that the sweep is no longer npm-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGtzPg3UxMLszysQXXfqax Signed-off-by: cliffhall <cliff@futurescale.com>
…issue Adds npm audit fix (root + every client) as the first sub-step of the release skill's bump step, before npm version, so a release is never gated on remembering to check separately. Never --force; anything audit fix can't resolve is left to the dependabot-alert pipeline (#2229) or a follow-up issue. Disables Dependabot npm version updates in dependabot.yml (a version-update PR carries no issue and no board card) and replaces them with a monthly scheduled sweep that runs npm outdated across the root install and every client, filing or updating one idempotent tracking issue instead of an auto-generated PR. Closes #2231 Part of #2229 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017N9ha27Qg6LHpBQP7G7zPY Signed-off-by: cliffhall <cliff@futurescale.com>
The npm-entry removal in `.github/dependabot.yml` is split out of this PR so that every change to that one file lands in a single reviewable piece — alongside the `github-actions` entry, which #2229 had declared out of scope but which opens the same issue-less, board-cardless PR the decision exists to remove. Reverts `.github/dependabot.yml` to its `v2/main` state, leaving this PR with the release-time `npm audit fix` and the monthly `npm outdated` sweep: the replacement flow, not the switch-off. Both new files' header comments claimed the switch-off had already happened, which is false until #2235 lands. They now say #2235 does it, and that until then Dependabot's npm PRs and this sweep overlap — duplicate signal rather than conflicting action. 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>
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>
`main refuses to run without a repo` called `main(undefined, …)` so the default parameter would fire, then asserted it throws. The default reads `process.env.GITHUB_REPOSITORY` — which GitHub Actions sets on every run — so the assertion held only where the variable happened to be unset. It passed locally and failed CI, the one environment where the default is always populated. The test now clears the variable and restores it, and a second test covers the other half of the default: with it set, `main(undefined, …)` uses it instead of throwing. Between them the default's behavior is pinned in both environments rather than inherited from whichever one is running. Verified both ways: 14 pass with the variable unset and with it set, and the full `test:scripts` suite is 411/411 under the CI environment shape. 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>
Closes #2235. Switches the old dependency flow off now that #2232 landed the replacement. Removes the five npm ecosystem entries (root plus each client under clients/*) and, resolving the question #2229 left open, the github-actions entry as well — so .github/dependabot.yml goes away outright rather than being emptied, which its schema does not allow. The github-actions entry had exactly the property #2229 exists to remove: it opened a grouped monthly PR carrying no `Closes #N` and no board card, the one standing exception to "every PR references an issue". Deleting it unreplaced would have left 9 actions unwatched, and `npm outdated` says nothing about actions, so the monthly sweep now also checks every `uses:` ref under .github/workflows and renders the stale ones as one more section of the same tracking issue. Ranking comes from the release LIST, not `releases/latest`. That endpoint returns the release GitHub designates most recent, not the greatest version, so an action publishing a maintenance release for an older major (a v6.9.1 cut after v8.0.0) would make a workflow pinned to v7 compare against v6 and read as current — silently missing a whole major upgrade, the one thing this check exists to catch. Staleness is compared only to the precision the ref specifies. `v7` is a moving major tag that GitHub repoints at every v7.x release, so `v7` against a highest of `v7.0.1` is current and only `v8` makes it stale; an exactly-pinned `v7.0.0` is behind `v7.0.1`; a SHA pin is deliberately immovable and is never reported. A release lookup suppresses only a 404 — the legitimate "this action has never cut a release" answer — and throws on anything else. Treating a rate limit or an expired token as "no release" is indistinguishable from "not stale", and since every action here already sits on its latest major, the resulting empty section is byte-identical to a healthy run. `buildClearedBody` now speaks for both halves: once actions are in scope, its npm-only wording would assert a clean bill of health the sweep never checked. Dependabot security updates are unaffected — they are configured in repo settings, not in this file, and kept working while it was missing entirely (#1833, #1840). That note moves into the workflow header rather than dying with the file; #2233 is where they are turned off deliberately. The script header, the workflow header and the generated issue body all say version-update PRs rather than claiming Dependabot is replaced wholesale. Also renames the workflow's npm-outdated job to dependency-sweep now that the sweep is no longer npm-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGtzPg3UxMLszysQXXfqax Signed-off-by: cliffhall <cliff@futurescale.com>
c1b150f to
7186b44
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The workflow introduces persistent repository automation and modifies release-critical procedures, warranting final human validation despite no additional code findings.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Copilot round 4 — clean on the rebased head (
|
| Round | Commit | Verdict | New comments |
|---|---|---|---|
1 (5102950044) |
e32a1be |
🟡 Changes recommended | 5, all valid, all fixed |
2 (5108387664) |
bb9af94 |
🟢 Approval recommended | 0 |
3 (5108396254) |
bb9af94 |
🟢 Approval recommended | 0 |
4 (5108727454) |
7186b441 |
🔵 Needs a closer look | 0 |
CI
Green on 7186b441 — build pass (27m25s), coverage pass (20m5s).
The earlier red was mine and is fixed here: main refuses to run without a repo exercised the repo = process.env.GITHUB_REPOSITORY default, and Actions sets that variable on every run, so the assertion held only where it happened to be unset. local:gate cannot catch that class — CI is the one environment where the default is always populated. Both halves of the default are now pinned explicitly.
Two items for the maintainer
- Add npm audit fix to the release flow and a monthly npm-outdated tracking issue #2231's framing narrowed. Round 1 finding SSE transport support #4 turned this from "add
npm audit fixto the release flow" into an audit-and-act step, becauseAGENTS.md:99forbidsnpm audit fix— plain, not just--force— and High-severity nanoid advisory in the production tree (via vite → postcss), on both branches #2058 is the reverted incident behind that rule. That is a change to what the issue asked for, not only how it was built. - Round 4's 🔵 stands unaddressed by design. It asks for human validation of the automation and the release-procedure change. Nothing for me to fix; it wants eyes.
Closes #2231
Two independent pieces, both scoped out of #2229 so they land before the bigger alert-driven pipeline that issue proposes.
Note
.github/dependabot.ymlis no longer touched here. This PR originally also removed the fivenpmecosystem entries; that edit moved to #2235, so every change to that one file lands in a single reviewable piece — along with thegithub-actionsentry, which #2229 had left out of scope but which opens the same issue-less, board-cardless PR the decision exists to remove.Consequence: until #2235 lands, Dependabot's npm PRs and the monthly tracking issue below both exist. That is duplicate signal rather than conflicting action, and the header comments in both new files say so explicitly rather than claiming a switch-off that has not happened.
1.
npm audit fixin the release flow/release's step 1 (bump onv2/main) now runsnpm audit fix— root and every client, since each has its own lockfile — beforenpm version, gated bynpm run local:gate. Never--force; whateverfixcan't resolve on its own (typically a transitive dependency needing anoverridespin) is left to the dependabot-alert pipeline or a follow-up issue, called out in the release notes instead of forced through.2. A monthly
npm outdatedtracking issue.github/workflows/dependency-refresh.yml(monthly cron +workflow_dispatch) andscripts/dependency-refresh.mjs, which runsnpm outdatedacross the root install and every client and files or updates one idempotent tracking issue (marker-based, so a re-run before the issue closes edits it in place rather than duplicating it). No PR is opened automatically — a maintainer reviews the issue and opens a normal PR againstv2/main.GITHUB_TOKENis sufficient (issues: write+ public milestone reads); board placement is intentionally not attempted, so an unboarded issue is picked up by the next/issue-triagesweep, same as any other maintainer-filed issue.parseOutdated,buildIssueBody) is unit-tested inscripts/dependency-refresh.test.mjs.Testing
Re-run after removing the
dependabot.ymledit:npm run test:scripts— 402 pass, including the 5 new tests.npm run format:check:scripts— clean.npm run verify:skills— OK (release skill frontmatter still parses, listing budget unaffected sincereleaseisdisable-model-invocation: true).npm run local:gate— full pass in an isolated worktree with a realnpm install.🤖 Generated with Claude Code
https://claude.ai/code/session_017N9ha27Qg6LHpBQP7G7zPY