chore(deps): sweep Dependabot alerts into board-tracked issues

[cliffhall]

Closes #2233

The alert-consuming half of #2229, and the last piece of it. #2235 removed .github/dependabot.yml, ending Dependabot's version-update PRs; this ends its security-update PRs and replaces them with a daily sweep that turns the alerts they used to act on into ordinary board items.

alert → scheduled sweep → issue (labeled, milestoned, boarded) → maintainer PR → v2/main

Stacked on #2239. It edits files #2232 introduced and #2239 modified — scripts/dependency-refresh.mjs and .github/workflows/dependency-refresh.yml, whose headers currently say security updates are still on — so its base is v2/chore/2235-remove-dependabot-config. GitHub retargets it to v2/main automatically as the stack merges.

What lands

File
scripts/dependabot-alerts.mjs	reads the alerts, groups them, probes the lockfile, files/edits the issue, boards it
.github/workflows/dependabot-alerts.yml	daily cron + workflow_dispatch, vulnerability-alerts: read
scripts/dependabot-alerts.test.mjs	18 tests over the pure halves
AGENTS.md	a new Dependency updates are issue-driven, like everything else subsection covering both halves
scripts/dependency-refresh.{mjs,yml}	the "security updates are unaffected" comments, now stale, corrected
package.json	semver, declared at the root

One issue per bump, not per advisory

Alerts are per-advisory; a fix is per-bump. Grouping by (package, manifest_path, first_patched_version) is what keeps today's 7 open alerts from becoming 7 issues for what is really 3 overrides entries. Dry-run against the live alert feed:

chore(deps): bump `browserslist` to `4.28.7` in `clients/tui/package-lock.json` (1 advisory)
chore(deps): bump `fast-uri` to `3.1.6` in `package-lock.json` (4 advisories)
chore(deps): bump `qs` to `6.16.0` in `package-lock.json` (2 advisories)

Two advisories on the same package needing different patched versions are different bumps and stay apart — that is why first_patched_version is in the key and not just (package, manifest).

The issue body's first line is a marker naming the package, the manifest and every GHSA covered. That is the idempotency key: a second run the same day is a complete no-op, and a new advisory for a package with an open issue lands as a comment and rewrites the marker rather than filing a second issue.

Alerts are computed from main; we ship from v2/main

This is the one thing the design rests on that isn't obvious. GitHub builds the dependency graph — and therefore every alert — from the default branch. So an alert is not trusted on its face: the sweep checks out v2/main and re-tests the vulnerable range against that branch's own lockfile before filing. Already-fixed-there means the alert is only waiting on a milestone merge to close, and it is skipped silently.

⚠️ The converse is a real blind spot, and no approach that consumes GitHub's alerts avoids it: a vulnerable dependency introduced on v2/main and not yet merged to main produces no alert at all. The release-time npm audit fix from #2232 is a partial second signal. Closing it fully wants a scheduled npm audit --audit-level=high over v2/main's lockfiles, which is a separable follow-up rather than something to bolt on here.

toSemverRange — a silent failure worth its own function

GitHub writes a vulnerable range as >= 3.1.3, < 3.1.6. node-semver reads that comma as nothing at all and returns false for a version that is squarely in range — so a naive semver.satisfies(installed, alert.range) would skip every multi-conjunct advisory and the sweep would file nothing while looking perfectly healthy. Space is semver's AND, so the fix is a split/join; it is exported and tested rather than inlined precisely because the failure is invisible.

Two things GITHUB_TOKEN cannot do, and neither is a prerequisite

vulnerability-alerts: read is the one non-default permission, and GITHUB_TOKEN supports it — reading alerts needs no PAT. Two side steps are outside its reach, and PROJECT_TOKEN covers them when present:

• Writing the board card. Board Add tab and approval flow for server -> client sampling #28 is an org project. Absent the secret, the issue is still created labeled (v2 + chore + dependabot) and milestoned, and the next /issue-triage sweep boards it — its documented exception moves an unboarded-but-milestoned issue straight into Todo.
• Reading back automated-security-fixes. ⚠️ This endpoint needs administration: read, and permissions: has no key for it, so GITHUB_TOKEN can never have it. The issue asked the sweep to "fail loudly if security PRs have been re-enabled"; taken literally that would make every scheduled run red on a 403 for a reason unrelated to the alerts. So it is reported as UNVERIFIED when the token can't see the setting, and only an explicit enabled: true throws. Give PROJECT_TOKEN the extra scope and it becomes the real assertion the issue wanted.

Board Status is Todo, not Incoming — arriving through this pipeline is the approval — and Priority is High as a standing rubric override, recorded in the body so it doesn't read as a mis-scored routine bump.

Option ids are resolved by name at run time rather than hardcoded, because a single-select field's option ids are regenerated whenever its option list is edited. A hardcoded id would turn an unrelated board edit into a silently mis-set field.

semver is declared at the root

#2233 observed semver is already resolvable at the repo root and called it "no new dependency". It is resolvable only transitively, and per Dependency placement scripts/ is root-owned code with no manifest of its own, so what it imports is declared at the root. It is a devDependency: nothing consumed at runtime by the published package, and the tarball ships only each client's build/. The lockfile grew by one line — the copy was already in the tree.

The workflow installs with npm ci --ignore-scripts rather than npm install: the sweep reads every lockfile as JSON and never needs a client's tree on disk, so the postinstall cascade dependency-refresh.yml genuinely needs would be minutes of nothing here.

Verification

• scripts/dependabot-alerts.test.mjs — 18 tests. Beyond the happy paths they pin the three behaviors that would fail silently: toSemverRange on real GitHub ranges; parseMarker rejecting a marker that is not the first line (anything else is not an idempotency key); and groupAlerts dropping an alert with no patched version, since an issue asking for an unavailable upgrade is noise.
• Dry-run against this repo's live alert feed — the 7 → 3 grouping above, each with the right installed version read out of v2/main's lockfile (fast-uri@3.1.5, qs@6.15.3, browserslist@4.28.2) and all three correctly classified transitive.
• Both workflow files parse, and permissions/jobs resolve as intended.
• npm run local:gate green.

⚠️ What this PR cannot do

Turning the security-update PRs off is a repo setting, not a file — DELETE /repos/modelcontextprotocol/inspector/automated-security-fixes, or Settings → Code security. No PR can make that change, so it is a maintainer step at merge time. It currently reports {"enabled":true,"paused":false}. vulnerability-alerts must stay on (it is, and returns 204) — turning alerts off would blind the sweep that replaces the PRs.

Two consequences of the same "settings and default branch" theme:

• Scheduled workflows only run from the default branch. Merging this to v2/main does not start the sweep; it starts at the next milestone merge into main. Same caveat chore(deps): remove dependabot.yml, fold action bumps into the monthly sweep #2239 carries for dependabot.yml.
• Because the sweep will not have run before then, the acceptance items that depend on a real run — the no-op second run, the new-advisory-becomes-a-comment path — are exercised here by unit tests and a dry run, and confirmable in production only via workflow_dispatch after that merge.

🤖 Generated with Claude Code

https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq

[Copilot]

🟡 Changes recommended

Alert identity, pagination, board-state, and failure-handling defects can produce incorrect or incomplete tracking.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds scheduled, issue-driven dependency security tracking to replace Dependabot-generated PRs.

Changes:

• Adds a daily Dependabot-alert sweep with lockfile validation and board integration.
• Extends monthly dependency monitoring to npm packages and GitHub Actions.
• Adds tests, documentation, release guidance, and the semver dependency.

File summaries

File	Description
scripts/dependency-refresh.test.mjs	Tests monthly sweep behavior.
scripts/dependency-refresh.mjs	Implements monthly dependency and action checks.
scripts/dependabot-alerts.test.mjs	Tests alert grouping and issue formatting.
scripts/dependabot-alerts.mjs	Implements the security-alert sweep.
package.json	Declares semver.
package-lock.json	Locks semver.
AGENTS.md	Documents issue-driven dependency updates.
.github/workflows/dependency-refresh.yml	Schedules the monthly sweep.
.github/workflows/dependabot-alerts.yml	Schedules the daily alert sweep.
.github/dependabot.yml	Removes Dependabot update configuration.
.claude/skills/release/SKILL.md	Adds release-time audit guidance.

Review details

• Files reviewed: 10/11 changed files
• Comments generated: 14
• Review effort level: Balanced

[cliffhall]

Copilot review round 1 — all 14 addressed (13 implemented, 1 declined)

Mirrored at PR level because inline replies go hidden once the fix is pushed.

Correctness

#	Finding	Done
1	--paginate emits one JSON array per page; JSON.parse fails past 100 alerts	--slurp + flatten. This was a hard throw, not degraded output.
2	Existing-issue lookup omitted fixedIn, so a second bump could merge into the first bump's issue	marker is now pkg; manifest; fixed; ghsas, and the lookup matches all three
4	Every failed setting lookup became UNVERIFIED, so a rate limit or 5xx bypassed the guard	isPermissionDenied — HTTP 401/403/404 only; everything else throws
5	item-add succeeding then a field edit failing left a card no triage sweep would fix	the two cases are now distinct; a partial placement finishes every group, then fails the run
7	An unmilestoned issue was still boarded at Todo, breaking Incoming ⇔ no milestone	createIssue returns the milestone; no milestone ⇒ no board write
8	Marker rewritten before the comment posted, so a failed comment was skipped forever	comment first, and the comment carries its own idempotency marker

Content and docs

#	Finding	Done
3	Prescribing >=<fixed> discards the manifest's compatibility bound	body asks for the range to be raised, keeping the existing operator
6	main() untested; the sibling sweep injects its spawn function	same injection here, 12 orchestration tests
10, 13	Header and AGENTS.md promised an unconditional guard the token often cannot perform	both now describe a conditional guard that reports UNVERIFIED
11, 12	Named the release-time npm audit fix, which #2232 replaced with a report-only audit	both now name npm audit --audit-level=high
14	The monthly sweep's issue body still said security updates remain enabled	corrected — this PR is what makes it false

Declined

#9 — latestReleaseTag suppressing a 404 in dependency-refresh.mjs. That line comes from the base of this stack (#2239), not from this PR, and its comment states the choice deliberately: a repo with no releases returns 200 [], and the 404 it suppresses is a renamed or deleted action. Whether that is right is a live question, but fixing it here would put a #2239 change in a #2233 diff. Relayed to #2239 instead.

Verification

• scripts/dependabot-alerts.test.mjs: 21 → 33 tests. The 12 new ones drive main() through an injected spawn and cover exactly the paths above — two-page alert feed, out-of-range skip, absent manifest, second-run no-op, distinct-bump isolation, ["comment", "edit"] ordering, comment de-duplication, the enabled: true throw, the 403 and 502 branches, the unmilestoned board skip, and the partial placement (asserting both issues were filed before the run fails).
• Re-ran the live dry run against the real alert feed after the changes: still 7 alerts → 3 issues, markers now carrying fixed=.
• npm run local:gate green.

[Copilot]

🟡 Changes recommended

Advisory retries can duplicate comments, updated issues retain stale titles, and the source contains text-tool-breaking NUL bytes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 10/11 changed files
• Comments generated: 4
• Review effort level: Balanced

[cliffhall]

Copilot review round 2 — all 4 addressed

Finding	Done
Literal NUL bytes in the grouping key classified the source as binary, so repo searches skipped the file	separator dropped entirely — the key is JSON.stringify([pkg, manifestPath, fixedIn]). Zero NULs remain; file now reports UTF-8 text.
Comment de-duplication compared whole GHSA sets, so a comment posted before a failed marker edit was announced twice on the next run	unions the GHSAs across every comment marker and comments only on unclaimed ones
The issue title kept its filed advisory count after growing	gh issue edit now passes --title buildIssueTitle(group, merged.length)
The test file's header still said main() was left to workflow_dispatch	corrected — a leftover from fixing the same claim in the script header last round

The NUL one deserves a note: it was real, and it had already bitten me silently — grep over dependabot-alerts.mjs returned nothing at all while I was working on it, which I worked around rather than investigated. Worth knowing that a stray control character in a source file degrades tooling well before anyone notices.

The comment finding is the sharper version of the ordering fix from round 1: reordering made the failure mode "a repeat instead of a permanent skip", and set-comparison then failed to prevent that repeat in exactly the interleaving where it mattered. Per-advisory is the right granularity — it is what a comment actually claims.

Verification

• scripts/dependabot-alerts.test.mjs: 33 → 35 tests. main announces only the advisories no comment has claimed yet reconstructs the precise state described — comment announcing b, body marker still at a, c newly arrived — and asserts the outgoing comment names c and not b. main refreshes the title when an issue grows another advisory asserts the edited title ends (2 advisories).
• npm run local:gate green.

[Copilot]

🟡 Changes recommended

The workflow uses an invalid permission key, and mixed direct/transitive package copies can produce incorrect remediation guidance.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

scripts/dependabot-alerts.mjs:761

• This classifies the package as direct merely because its name is declared at the lockfile root, rather than checking which installed copy is vulnerable. For a valid lock containing a safe direct pkg@4 and vulnerable nested pkg@3, affected contains only v3 but direct is still true, so the generated issue recommends bumping the already-safe direct range and omits the override needed for the nested copy. Preserve lockfile paths/relationships and derive the fix strategy from the affected entries, including the mixed direct/transitive case.

• Files reviewed: 10/11 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Copilot review round 3 — 1 implemented, 1 declined with evidence

Implemented — the suppressed finding on isDirectDependency (mixed direct/transitive)

This was the better of the two and it was a genuine defect. The fix strategy was derived from whether the package is declared, when the question is which copies are vulnerable. Your example is exactly right: a lock holding a safe declared pkg@4 and a vulnerable nested pkg@3 produced direct: true, so the issue asked for a range bump that would have changed nothing and never mentioned the override the nested copy needs.

• lockfileEntries now keeps each copy's tree path and whether it is the hoisted one; lockfileVersions is derived from it.

• remediation(affected, declared) returns {direct, transitive} — three real cases, not a boolean:

  vulnerable copies	issue asks for
  the declared (hoisted) one	raise the declared range
  nested ones only	an overrides pin
  both	both, stated as "neither alone clears every vulnerable copy"

• The body now lists the vulnerable copies with their paths, so the maintainer can see why they are being asked for a given edit.

One thing the change surfaced that neither of us had named: an undeclared hoisted copy. It got there transitively like any other, and no declared range reaches it — but my first cut of remediation returned {direct: false, transitive: false} for it, i.e. an issue with no fix at all. A test written for this change caught it before it shipped; transitive is now "every affected copy that is not the declared one".

Declined — vulnerability-alerts is not a valid permissions key

It is valid, and this was worth checking rather than taking on either side's word: the workflow-syntax docs list it, several older community threads say the opposite, and getting it wrong either way breaks the workflow.

So I pushed a throwaway branch with exactly this permissions: block and a step calling the alerts API with GITHUB_TOKEN. The run started — an invalid permission key is a startup failure, not a warning — and the runner reported:

##[group]GITHUB_TOKEN Permissions
Contents: read
Metadata: read
VulnerabilityAlerts: read
##[endgroup]

with the step returning 1 and READ_OK. Probe branch deleted.

security-events is the code scanning permission; using it here would be both wrong and a wider grant than the sweep needs. The older sources are not wrong about their own era — GITHUB_TOKEN could not read Dependabot alerts for years, and vulnerability-alerts is the distinct permission GitHub said it was working towards.

Verification

• scripts/dependabot-alerts.test.mjs: 35 → 39 tests. New: remediation reads the vulnerable copies, not the declaration (all three cases plus the undeclared-hoisted one), buildIssueBody asks for BOTH edits when declared and nested copies are vulnerable, buildIssueBody asks only for an override when the declared copy is safe, and lockfileEntries keeps each copy's path and marks the hoisted one.
• Live dry run against the real alert feed: fast-uri resolves to one hoisted-but-undeclared copy at 3.1.5, correctly classified {direct: false, transitive: true}, and the rendered Fix section asks for the overrides pin and names the copy.
• npm run local:gate green.

[Copilot]

🟡 Changes recommended

Per-advisory filtering and issue pagination are incomplete, and disjunctive ranges can corrupt generated tables.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

scripts/dependabot-alerts.mjs:597

• This cap breaks idempotency at the same scale the alert fetch is designed to support. If a run creates 101 distinct bump issues, the next run can retrieve only 100 of them and will file a duplicate for every omitted marker. Fetch all open Dependabot issues (for example with paginated gh api) rather than truncating this lookup.

• Files reviewed: 10/11 changed files
• Comments generated: 2
• Review effort level: Balanced

[cliffhall]

Copilot review round 4 — all 3 addressed

Finding	Done
Range validated at the group level, so one matching advisory kept advisories that don't apply	narrowToApplicable filters advisories individually, then re-derives the affected copies and the severity from what survives
The vulnerable range went into a Markdown table unescaped, and || is legal in semver	a cell() helper escapes every free-form column — range, summary, severity, CVE
The open-issue lookup was capped at --limit 100 while the alert fetch paginates	gh api --paginate --slurp over /issues?state=open&labels=dependabot, with pull requests dropped

The grouping one is the sharpest finding of the four rounds

It undermined the grouping contract from the inside. (package, manifest, first_patched_version) does not imply a shared vulnerable range — >= 3.1.3, < 3.1.6 and >= 3.0.0, < 3.1.6 both patch at 3.1.6, and an installed 3.1.0 is in range of only the second. Asking "does any advisory match?" kept both, so the marker, the title's count, the highest severity, the advisory table and every later comment all overstated what this branch is exposed to. Severity is the part that would have hurt: a critical that does not apply here would have been the number a maintainer triaged on.

main() now works from the narrowed group from that point on, so nothing downstream can see a discarded advisory.

The pagination one is about the asymmetry, not the scale

101 open bump issues is implausible today — but the marker lookup is what makes this sweep idempotent, and a truncated list files a duplicate for every issue it cannot see. It would start doing that at precisely the scale the --slurped alert fetch was added to support, which is what made the cap worth removing rather than justifying.

Verification

• scripts/dependabot-alerts.test.mjs: 39 → 46 tests. narrowToApplicable drops advisories the installed version is out of range of asserts both the dropped GHSA and the severity falling critical → medium; main files an issue naming only the advisories that apply here is the end-to-end version. The escaping test counts unescaped pipes in the rendered row rather than asserting the escape sequence, since the column count is what actually breaks. main reads every page of open dependabot issues puts the matching marker on page two.
• Live dry run: still 7 alerts → 3 issues, all three groups narrowing to themselves (every advisory genuinely applies), and the real paginated issue lookup returns 4 items with 0 PRs.
• npm run local:gate green.

[Copilot]

🔵 Needs a closer look

Concurrent runs can create duplicates, and undated milestones can be selected instead of the current release milestone.

Review details

Suppressed comments (4)

Previously missed (4) — in code that hasn't changed since the last review.

.github/workflows/dependabot-alerts.yml:56

• The marker check is a non-atomic read-before-create/update, but Actions permits the scheduled and manually dispatched runs to overlap. Two runs can both see no issue and file duplicate issues, or both post the same “new advisory” comment. Serialize this workflow with a fixed concurrency group and cancel-in-progress: false so the queued run rechecks state after the first completes.
  .github/workflows/dependency-refresh.yml:32
• The issue marker makes sequential runs idempotent, but the lookup-and-create sequence is not atomic and scheduled/manual runs may overlap. Both executions can observe no open tracking issue and create duplicates. Add a fixed concurrency group with cancellation disabled so later runs wait and then observe the first run’s issue.
  scripts/dependabot-alerts.mjs:690
• An undated open milestone sorts before every dated milestone because jq orders null before strings, so this can select an undated bucket instead of the current release milestone. Exclude milestones without due_on (or sort nulls last) before taking [0]; the issue-creation procedure defines “current” as the open milestone with the nearest due date.
  scripts/dependency-refresh.mjs:388
• An undated open milestone sorts before every dated milestone because jq orders null before strings, so the monthly issue can be assigned to an undated bucket rather than the current release milestone. Filter out null due dates (or sort them last) before selecting [0]; the issue-creation procedure defines “current” as the open milestone with the nearest due date.

• Files reviewed: 10/11 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Copilot review round 5 — 0 new comments, 4 suppressed findings triaged

Round 5 generated no new comments. It surfaced four "previously missed" findings against unchanged code; two apply here and are fixed, two are dependency-refresh and relayed to #2239.

Fixed here

1. The sweep can race itself. Correct, and it defeats the point of the whole design. The marker check is a read-before-write, and nothing stops a workflow_dispatch landing on top of the scheduled run — two overlapping runs both see no open issue and both file one, which is precisely the duplicate the marker exists to prevent. Added:

concurrency:
  group: dependabot-alert-sweep
  cancel-in-progress: false

cancel-in-progress: false is the load-bearing half, as you noted: the queued run has to wait and then re-read what the first run wrote. Cancelling it would silently drop a sweep instead.

2. sort_by(.due_on) picks an undated milestone. Also correct — jq orders null before every string, so an open bucket with no due date wins over every dated one. Verified directly:

$ echo '[{"t":"dated","due_on":"2026-09-09..."},{"t":"undated","due_on":null}]' | jq -c 'sort_by(.due_on) | .[0]'
{"t":"undated","due_on":null}

No live impact today — both open milestones are dated — but a Backlog-style bucket is exactly the kind of thing that gets opened later, and the failure would be silent milestoning of security issues into the wrong release.

Rather than patching the jq, the selection moved into a pure pickMilestone(milestones): an undated bucket has no due date and so cannot be "the nearest", so it is dropped rather than sorted last. If nothing dated is open the issue is filed unmilestoned — which, per the round-1 fix, also means the board write is skipped and triage places it. That path is now a tested one rather than an incidental consequence. Also made this testable, which the jq expression was not.

Relayed to #2239

The same two findings against scripts/dependency-refresh.mjs (currentMilestone) and .github/workflows/dependency-refresh.yml (concurrency) are that PR's code, arriving in this diff only because this branch stacks on it. Same scope reasoning as the latestReleaseTag finding in round 1. ⚠️ The milestone one is a live bug in the monthly sweep as well — it is not cosmetic, and if #2239 merges before it is picked up it needs a follow-up issue rather than being dropped.

Verification

• scripts/dependabot-alerts.test.mjs: 46 → 48 tests. pickMilestone takes the nearest due date, never an undated bucket uses the exact list jq gets wrong; the companion covers closed milestones, an all-undated list, [] and undefined.
• Checked against the live milestone list: v2.6.0(2026-09-09), v2.7.0(2026-09-16) → picks v2.6.0.
• Workflow YAML parses with the concurrency block as intended.
• npm run local:gate green.

[cliffhall]

Copilot review round 11 — all 4 addressed

A non-npm alert would have crashed the sweep

The most serious defect the whole review found, and the only one that takes the job down rather than degrading it. openAlerts does not filter by ecosystem, and Dependabot alerts are not npm-only — this repo has a Dockerfile at the root, so a docker alert is reachable today. readManifest would have thrown a SyntaxError parsing it, before a single npm group was processed.

Three changes, defence in depth:

• groupAlerts records dependency.package.ecosystem.
• main() skips anything not npm — loudly, naming the package, ecosystem, manifest and GHSAs with "raise it by hand". Silently dropping a security alert is the one outcome worse than crashing.
• readManifest treats an unparseable file as "not an npm lockfile" rather than throwing, so no single manifest can abort a run.

I deliberately did not route non-npm alerts into a generic issue. Filing one usefully means knowing how the fix is expressed — a base-image bump is not an overrides entry — and everything downstream of the group reads npm lockfiles. A loud skip is honest about the scope; a generic issue would look like coverage this sweep does not have.

Superseded ≠ fixed

A vanished grouping key is not a closed advisory: GitHub can revise first_patched_version, moving an advisory to a new key while it stays open. The reconciliation would have called that "fixed or dismissed" — standing down a live exposure, which is the wrong direction to fail in. It now reads the set of GHSAs still open in any group and says superseded, naming them, unless none remain.

Two accuracy fixes

• The body no longer claims security-update PRs are off when the run logged that check as UNVERIFIED. checkSecurityPrsStillDisabled already returned whether it read the setting; the value was simply unused.
• scripts/ is described in README.md and AGENTS.md as also holding repo automation run from CI. That description had drifted before this PR — chore(deps): npm audit fix before release bump; monthly npm-outdated issue #2232 added dependency-refresh.mjs without updating it — so this corrects both.

Verification

• scripts/dependabot-alerts.test.mjs: 61 → 66 tests. main skips a non-npm alert loudly instead of crashing on its manifest puts a Dockerfile alert and an npm alert in one feed and asserts the npm bump is still filed and the skip line names the GHSA. main says superseded, not fixed, when the GHSA is still open elsewhere revises the patched version 3.1.6 → 3.1.7. Two more cover the UNVERIFIED wording, unit and end-to-end.
• npm run local:gate green.

On the review loop itself

Eleven rounds, and the last three surfaced a production crash, a security-issue mis-statement and a remediation that npm would reject — so the rounds after "0 new comments" were not diminishing returns. My earlier claim that the reviewer had gone quiet was my own pagination bug, not the reviewer stopping.

[Copilot]

🔵 Needs a closer look

Reconciliation can incorrectly mark an unpatched but still-open advisory as fixed or dismissed.

Review details

Suppressed comments (1)

scripts/dependabot-alerts.mjs:1040

• openGhsas is derived from groups, but groupAlerts() deliberately drops an open alert whose first_patched_version is null. If GitHub removes that field for an advisory already tracked, its key and GHSA both disappear here, so reconciliation rewrites the existing issue as “fixed or dismissed” even though the alert remains open. Retain the raw alert feed and build this set from every open alert's security_advisory.ghsa_id; grouping can still omit unpatched advisories from filing. Add a regression for an existing issue whose alert remains open but loses its patched version.

  /** Every GHSA still open, in ANY group — the check a vanished key needs. */
  const openGhsas = new Set(groups.flatMap((g) => g.ghsas));

• Files reviewed: 8/9 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Copilot review round 12 — one finding, and it was a hole in last round's fix

Zero new inline comments; one suppressed finding, and it was correct.

openGhsas was derived from groups, which inherits a filter it should not. groupAlerts deliberately drops an alert with no first_patched_version — there is nothing to bump to, so nothing to file. Building the open-advisory set from the groups silently picked that filter up, so an advisory that stays open but loses its patched version would vanish from both the keys and the set, and reconciliation would report it "fixed or dismissed".

openAlerts is now held and the set built from every open alert's security_advisory.ghsa_id. Grouping still omits unpatched advisories from filing, which is right — "can this be filed as a bump?" and "is this advisory still open?" are different questions, and conflating them is exactly what caused this.

The invariant worth naming

This is the second time the same wrong direction has been reached by a different route — round 11 was a revised first_patched_version moving the key, round 12 is a removed one. Both would have stood down a live exposure. The rule the code now has to hold to:

A still-open advisory must never stand itself down. Every path that writes a cleared body has to be checked against the open-advisory set, not against the absence of a grouping key.

It is stated in the comment on openGhsas so the next person changing that reconciliation sees why it reads the raw feed.

Verification

• scripts/dependabot-alerts.test.mjs: 66 → 67 tests. main will not stand down an open alert that lost its patched version builds precisely that feed and asserts the body does not say "fixed or dismissed" and does say superseded, naming the still-open GHSA.
• npm run local:gate green.

[Copilot]

🟡 Changes recommended

Reconciliation can falsely clear live alerts and repeatedly edit cleared issues on every daily run.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

scripts/dependabot-alerts.mjs:609

• today is part of the rendered cleared body, but cleared issues are intentionally left open. On tomorrow's scheduled run the regenerated body therefore differs solely by date, so every cleared issue is edited again each day; the no-reclear tests miss this by passing the same date twice. Preserve the original clear date or remove the volatile date from the rendered/idempotency state.

scripts/dependabot-alerts.mjs:1251

• An open alert that loses first_patched_version is dropped by groupAlerts, so it has no replacement group or issue, but this branch still calls it superseded, claims it has its own issue, and passes it to buildClearedBody, which says the exposure is gone. Distinguish GHSAs represented by another actionable group from still-open unpatched alerts; the latter must not be cleared.

    const stillOpen = issue.marker.ghsas.filter((g) => openGhsas.has(g));
    const reason =
      stillOpen.length > 0
        ? `this bump was superseded — ${stillOpen.map((g) => `\`${g}\``).join(", ")} ${stillOpen.length === 1 ? "is" : "are"} still open under a different patched version, and ${stillOpen.length === 1 ? "has" : "have"} their own issue`
        : "every alert it tracked has been fixed or dismissed";

• Files reviewed: 8/9 changed files
• Comments generated: 1
• Review effort level: Balanced

[cliffhall]

Fixed. You are right, and this is the third route to the same failure — I introduced this null myself last round while hardening against a non-npm manifest, and collapsed two states that mean opposite things.

readManifest now returns {lock}, {absent: true} or {unparseable: true}, and the caller treats them differently:

• absent — real evidence the exposure went away, so the issue is cleared.
• unparseable — evidence of nothing. The group is skipped without clearing, and the log says skipping … WITHOUT clearing its issue so a truncated lockfile in CI is visible rather than quietly standing down an alert.

Test: main does not clear an issue when the lockfile cannot be parsed writes a truncated package-lock.json and asserts no issue edit, no issue create, and the parse-failure line.

[cliffhall]

Copilot review round 13 — all 3 addressed

All three are the same invariant reached by three different routes, which is the useful thing this round exposed. Two of them broke fixes I had made in the two rounds immediately before.

1. An unpatched-but-open alert must not be cleared at all

My round-12 fix was still wrong, just less wrong. groupAlerts drops an alert with no first_patched_version — nothing to bump to — so such an advisory has no replacement group and no replacement issue. Calling it "superseded — has their own issue" was false, and buildClearedBody then asserted the exposure was gone while nothing tracked it.

There are three states, and the code now says so:

marker's GHSAs	action
still open and filable	superseded — a replacement issue exists
still open, unpatched	leave the issue exactly as it is, and log why
not open	fixed or dismissed — clear

2. Cleared issues were re-edited every day, forever

A cleared issue is deliberately left open, so tomorrow's run sees it again — and with today's date rendered into the body, the regenerated body differed by the date alone. Every cleared issue would have taken one edit a day indefinitely.

You also named exactly why my tests missed it: the no-reclear cases passed the same date twice. parseClearedDate now reads the date off the existing body and reuses it, so only a real change writes — and a real change takes the new date, which is the more honest reading of "as of".

3. An unparseable manifest is not an absent one

The one I introduced myself last round, hardening against non-npm manifests: both returned null, so a malformed or truncated lockfile was read as "the manifest is gone" and cleared the issue — standing down a live alert on the strength of a read error. readManifest now returns {lock} / {absent} / {unparseable}, and only absent is evidence of anything.

The pattern worth stating

Three rounds, three routes, one rule:

Clearing an issue requires positive evidence that the exposure went away. An absent grouping key, a missing patched version, and a failed read are all absence of information, and none of them is that evidence.

The only facts that qualify are: the manifest is genuinely gone, every installed copy is out of range, or no tracked advisory is open any more.

Verification

• scripts/dependabot-alerts.test.mjs: 67 → 71 tests. a cleared issue is not re-edited on a LATER day runs the sweep on a different date from the one in the body — the case the same-date tests structurally could not reach. Plus the changed-reason case, parseClearedDate directly, and main does not clear an issue when the lockfile cannot be parsed, which writes a truncated lockfile and asserts no edit and no create.
• npm run local:gate green.

[Copilot]

🟡 Changes recommended

GHSA-only reconciliation can incorrectly clear tracking for an alert that remains open in the same manifest without a patched version.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

AGENTS.md:125

• This guarantee is not currently true for the monthly sweep. scripts/dependency-refresh.mjs:380-388 sorts open milestones without filtering due_on, so an undated milestone sorts first and is assigned instead of the nearest dated release milestone; only the new daily sweep filters this case. Apply the same dated-milestone selection to the monthly script before documenting the invariant for both.

• Files reviewed: 8/9 changed files
• Comments generated: 2
• Review effort level: Balanced