Skip to content

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

Merged
cliffhall merged 14 commits into
v2/chore/2235-remove-dependabot-configfrom
v2/chore/2233-dependabot-alert-sweep
Sep 4, 2026
Merged

chore(deps): sweep Dependabot alerts into board-tracked issues#2243
cliffhall merged 14 commits into
v2/chore/2235-remove-dependabot-configfrom
v2/chore/2233-dependabot-alert-sweep

Conversation

@cliffhall

Copy link
Copy Markdown
Member

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 fileDELETE /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

@cliffhall cliffhall added the v2 Issues and PRs for v2 label Sep 4, 2026
@cliffhall
cliffhall requested a balanced review from Copilot September 4, 2026 03:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread scripts/dependabot-alerts.mjs Outdated
Comment thread scripts/dependabot-alerts.mjs Outdated
Comment thread scripts/dependabot-alerts.mjs Outdated
Comment thread scripts/dependabot-alerts.mjs Outdated
Comment thread scripts/dependabot-alerts.mjs Outdated
Comment thread .github/workflows/dependabot-alerts.yml Outdated
Comment thread .github/workflows/dependabot-alerts.yml Outdated
Comment thread AGENTS.md Outdated
Comment thread AGENTS.md Outdated
Comment thread scripts/dependency-refresh.mjs Outdated
cliffhall added a commit that referenced this pull request Sep 4, 2026
- `--slurp` the paginated alert feed; a bare `--paginate` emits one JSON
  array per page and `JSON.parse` rejects it past 100 open alerts.
- Put `fixedIn` in the marker and in the existing-issue lookup, so a
  second bump of one package cannot merge into the first one's issue.
- Only an authorization-shaped failure of the `automated-security-fixes`
  read becomes UNVERIFIED; a rate limit or 5xx now throws.
- Distinguish "card never added" (benign, triage picks it up) from
  "card added, field not set" — the latter finishes every group, then
  fails the run.
- Don't board an unmilestoned issue at Todo; `Incoming` <=> no milestone.
- Comment before rewriting the marker, and give the comment its own
  marker, so a failed comment cannot be skipped forever.
- Ask a direct dependency's range to be raised, not widened to `>=`.
- Test `main()` through an injected spawn, as the sibling sweep does.
- Correct the docs that named the removed release-time `npm audit fix`,
  the ones promising an unconditional guard, and the sibling sweep's
  issue body claiming security updates remain enabled.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

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 isPermissionDeniedHTTP 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

#9latestReleaseTag 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 AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread scripts/dependabot-alerts.mjs Outdated
Comment thread scripts/dependabot-alerts.mjs Outdated
Comment thread scripts/dependabot-alerts.mjs Outdated
Comment thread scripts/dependabot-alerts.test.mjs Outdated
cliffhall added a commit that referenced this pull request Sep 4, 2026
- Remove the literal NUL bytes that were separating the grouping key's
  fields; they classified the whole source file as binary, so repo
  searches skipped it. The key is a JSON array now, with no separator
  left to justify.
- De-duplicate advisory comments per GHSA rather than per whole set. A
  run that comments and then fails before the marker edit left the next
  run computing a different set, which matched nothing and announced the
  same advisory twice.
- Refresh the issue title on edit; it carries the advisory count, so it
  went stale as soon as an issue grew past what it was filed with.
- Correct the test file's header, which still claimed main() was left
  to workflow_dispatch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

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 AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread .github/workflows/dependabot-alerts.yml
cliffhall added a commit that referenced this pull request Sep 4, 2026
Derive the remediation from WHICH copies are vulnerable, not from
whether the package is declared. A manifest can declare a safe `pkg@4`
while a dependency drags a vulnerable `pkg@3` into a nested folder; the
old boolean called that "direct" and asked for a range bump that would
have changed nothing, omitting the override the nested copy needs.

- `lockfileEntries` keeps each copy's tree path and whether it is the
  hoisted one; `lockfileVersions` is now derived from it.
- `remediation(affected, declared)` returns both flags, so the issue can
  ask for a range bump, an overrides pin, or explicitly both. The body
  lists the vulnerable copies and their paths.
- An undeclared hoisted copy counts as transitive: it got there the same
  way any other transitive copy did, and no declared range reaches it.
  (Found by a test written for this change.)

The `vulnerability-alerts: read` finding is declined — verified against
a real runner, which reports `VulnerabilityAlerts: read` and reads the
alerts successfully. See the PR comment for the log.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

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 AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread scripts/dependabot-alerts.mjs Outdated
Comment thread scripts/dependabot-alerts.mjs Outdated
cliffhall added a commit that referenced this pull request Sep 4, 2026
- Narrow each group to the advisories the installed copies are actually
  in range of. Two advisories can share a package, manifest and patched
  version while having different vulnerable ranges, so a group-level
  "does any match?" left the marker, title, severity and table all
  claiming an advisory that does not apply on this branch.
- Escape every free-form Markdown cell, the vulnerable range included.
  A semver range may contain `||`, which is also the column separator.
- Paginate the open-issue lookup instead of capping it at 100. The
  marker lookup is what makes the sweep idempotent, so a truncated list
  would file a duplicate for every issue it could not see — at the same
  scale the alert fetch is built to handle. Pull requests, which the
  issues endpoint also returns, are dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
Comment thread scripts/dependabot-alerts.mjs
@cliffhall

Copy link
Copy Markdown
Member Author

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 criticalmedium; 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 AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 added a commit that referenced this pull request Sep 4, 2026
Both findings were "previously missed" suppressed ones; the two against
scripts/dependency-refresh.mjs and its workflow are relayed to #2239.

- Serialize the sweep with a fixed concurrency group and
  cancel-in-progress: false. The marker check is a read-before-write and
  a workflow_dispatch can land on top of the scheduled run, so two runs
  could both see no open issue and both file one — the duplicate the
  whole idempotency design exists to prevent. The queued run must wait
  and re-read, never be cancelled.
- Pick the milestone in JS, not in jq. jq sorts null before every
  string, so sort_by(.due_on) | .[0] returns an UNDATED open milestone
  in preference to every dated one. An undated bucket has no due date
  and so cannot be the nearest; pickMilestone drops it, and files the
  issue unmilestoned if nothing dated is open.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

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 added a commit that referenced this pull request Sep 4, 2026
Both were suppressed "previously missed" findings, and both were real.

- isPermissionDenied matched on status alone, so a rate limit (also a
  403) and a bad token (401) were waved through as "missing scope" —
  contradicting the comment right above it, which said those must stop
  the sweep. It now excludes rate-limit wording and drops 401.
- The no-op path asked "were advisories ADDED?" when the question is
  "did the issue CHANGE?". An issue filed for A+B whose branch moved so
  only B applies has nothing added, yet its table, severity, affected
  copies and remediation are all stale. The rendered title and body are
  now compared against the issue, and a comment stays reserved for
  genuinely new advisories.
- The title counts the advisories that APPLY, matching the body, so it
  tracks shrinking exposure as well as growth. The marker's GHSA list
  stays monotonic — its job is to remember what has been announced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall added a commit that referenced this pull request Sep 4, 2026
Both findings are consequences of round 6's staleness fix, in the two
places it did not reach.

- Resolve the matching open issue BEFORE the skip paths, and rewrite it
  to a cleared state when the exposure is gone. Yesterday's issue is
  still open today, so a manifest that has since been removed or copies
  that have moved out of range left its body asserting a vulnerability
  that no longer exists and its Todo/High card live indefinitely. The
  marker is retained so the issue is reused if the advisory returns; the
  issue is not auto-closed, because whether the card belongs in Done or
  should be deleted depends on why the exposure went away.
- The body's prose counted the marker's monotonic history rather than
  the advisories that apply, so it could claim two open alerts while the
  title and table correctly showed one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall added a commit that referenced this pull request Sep 4, 2026
Three reviews I missed: my poll called the reviews endpoint without
--paginate, so it only ever read page one — the same pagination bug
this PR was reviewed for.

- Reconcile marked issues whose bump has left the open alert feed
  entirely. openAlerts asks for state=open, so a fixed or dismissed
  alert simply vanishes: its group is never built, the loop never
  visits it, and its issue kept asserting a vulnerability with a live
  Todo/High card. Both cases are covered — the zero-alert run, which
  used to return before loading issues at all, and a vanished group
  while other groups remain.
- Ask for a PARENT-SCOPED overrides entry when the package is also
  declared directly. npm rejects an override contradicting a direct
  dependency with EOVERRIDE, so the guidance for the mixed case would
  not have applied. The issue now prints the exact nested JSON.
- Correct the body's NOTE: severity, range, GHSA and CVE are the
  advisory's own. Only the installed versions, their paths and range
  applicability are verified against v2/main.
- Stop the new-advisory comment claiming the body marker "now covers"
  the advisory; it is posted before the edit, so that is false at
  posting time. It speaks for its own marker instead.
- Cover the successful two-field board placement, not only its failure
  modes.
- AGENTS.md: the monthly sweep NEVER boards; only the security sweep
  does, and only with a PAT. The old wording promised a card the
  monthly job cannot create.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall added a commit that referenced this pull request Sep 4, 2026
- Filter to the npm ecosystem. Dependabot alerts are not npm-only and
  this repo has a Dockerfile, so an alert against it would have been
  parsed as a lockfile, thrown, and aborted the whole daily sweep
  before any npm group ran. A non-npm alert is now reported loudly with
  its GHSAs so a human can file it, and readManifest no longer lets one
  unparseable manifest take the run down.
- Distinguish a superseded bump from a closed one. GitHub can revise
  first_patched_version, moving an advisory to a different key while it
  stays open; reporting that as "fixed or dismissed" would stand down a
  live exposure. The reason now reads from the open GHSA set.
- Only claim security-update PRs are off when the run actually read the
  setting. The guard degrades to UNVERIFIED, and an issue asserting
  what the run could not confirm is worse than one that stays quiet.
- README.md and AGENTS.md: scripts/ now holds repo automation run from
  CI, not only build/verify tooling.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall added a commit that referenced this pull request Sep 4, 2026
Build the open-GHSA set from the raw alert feed, not from the grouped
alerts. groupAlerts deliberately drops an alert with no
first_patched_version, so an advisory that stays OPEN but loses its
patched version vanished from both the keys and that set — and
reconciliation would have called it "fixed or dismissed".

Same wrong direction as the superseded case fixed last round, reached a
different way: a still-open advisory must never stand itself down.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall added a commit that referenced this pull request Sep 4, 2026
Three findings, all of them the same invariant reached by new routes: a
still-open advisory must never stand itself down, and a cleared issue
must be written once.

- Never clear an advisory that is open but unpatched. groupAlerts drops
  an alert with no first_patched_version, so it has no replacement
  group and no replacement issue — calling it "superseded" was false
  and clearing it removed the only thing tracking a live exposure. Such
  an issue is now left exactly as it is, and said so in the log.
- Stop re-editing cleared issues daily. A cleared issue stays open, so
  the next run regenerated its body with a new date and edited it
  again, forever. The date is now read back off the existing body, so
  only a real change writes — and a real change takes today's date.
- Distinguish an absent manifest from an unparseable one. Both returned
  null, so a malformed lockfile was treated as "the manifest is gone"
  and cleared the issue. A read error is evidence of nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall and others added 12 commits September 4, 2026 09:06
Closes #2233 — the alert-consuming half of #2229. #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 into ordinary board items.

- `scripts/dependabot-alerts.mjs` groups alerts by
  `(package, manifest_path, first_patched_version)` — one issue per
  BUMP, not per advisory — re-checks each vulnerable range against
  `v2/main`'s own lockfile before filing, and keys idempotency on a
  marker comment naming every GHSA the issue covers.
- `.github/workflows/dependabot-alerts.yml` runs it daily and on
  `workflow_dispatch`, with `vulnerability-alerts: read`.
- `semver` is declared at the repo root, per Dependency placement:
  `scripts/` is root-owned code with no manifest of its own.
- The sibling `dependency-refresh` comments no longer claim security
  updates are unaffected, and AGENTS.md gains the flow both halves
  now follow.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
- `--slurp` the paginated alert feed; a bare `--paginate` emits one JSON
  array per page and `JSON.parse` rejects it past 100 open alerts.
- Put `fixedIn` in the marker and in the existing-issue lookup, so a
  second bump of one package cannot merge into the first one's issue.
- Only an authorization-shaped failure of the `automated-security-fixes`
  read becomes UNVERIFIED; a rate limit or 5xx now throws.
- Distinguish "card never added" (benign, triage picks it up) from
  "card added, field not set" — the latter finishes every group, then
  fails the run.
- Don't board an unmilestoned issue at Todo; `Incoming` <=> no milestone.
- Comment before rewriting the marker, and give the comment its own
  marker, so a failed comment cannot be skipped forever.
- Ask a direct dependency's range to be raised, not widened to `>=`.
- Test `main()` through an injected spawn, as the sibling sweep does.
- Correct the docs that named the removed release-time `npm audit fix`,
  the ones promising an unconditional guard, and the sibling sweep's
  issue body claiming security updates remain enabled.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
- Remove the literal NUL bytes that were separating the grouping key's
  fields; they classified the whole source file as binary, so repo
  searches skipped it. The key is a JSON array now, with no separator
  left to justify.
- De-duplicate advisory comments per GHSA rather than per whole set. A
  run that comments and then fails before the marker edit left the next
  run computing a different set, which matched nothing and announced the
  same advisory twice.
- Refresh the issue title on edit; it carries the advisory count, so it
  went stale as soon as an issue grew past what it was filed with.
- Correct the test file's header, which still claimed main() was left
  to workflow_dispatch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
Derive the remediation from WHICH copies are vulnerable, not from
whether the package is declared. A manifest can declare a safe `pkg@4`
while a dependency drags a vulnerable `pkg@3` into a nested folder; the
old boolean called that "direct" and asked for a range bump that would
have changed nothing, omitting the override the nested copy needs.

- `lockfileEntries` keeps each copy's tree path and whether it is the
  hoisted one; `lockfileVersions` is now derived from it.
- `remediation(affected, declared)` returns both flags, so the issue can
  ask for a range bump, an overrides pin, or explicitly both. The body
  lists the vulnerable copies and their paths.
- An undeclared hoisted copy counts as transitive: it got there the same
  way any other transitive copy did, and no declared range reaches it.
  (Found by a test written for this change.)

The `vulnerability-alerts: read` finding is declined — verified against
a real runner, which reports `VulnerabilityAlerts: read` and reads the
alerts successfully. See the PR comment for the log.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
- Narrow each group to the advisories the installed copies are actually
  in range of. Two advisories can share a package, manifest and patched
  version while having different vulnerable ranges, so a group-level
  "does any match?" left the marker, title, severity and table all
  claiming an advisory that does not apply on this branch.
- Escape every free-form Markdown cell, the vulnerable range included.
  A semver range may contain `||`, which is also the column separator.
- Paginate the open-issue lookup instead of capping it at 100. The
  marker lookup is what makes the sweep idempotent, so a truncated list
  would file a duplicate for every issue it could not see — at the same
  scale the alert fetch is built to handle. Pull requests, which the
  issues endpoint also returns, are dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
Both findings were "previously missed" suppressed ones; the two against
scripts/dependency-refresh.mjs and its workflow are relayed to #2239.

- Serialize the sweep with a fixed concurrency group and
  cancel-in-progress: false. The marker check is a read-before-write and
  a workflow_dispatch can land on top of the scheduled run, so two runs
  could both see no open issue and both file one — the duplicate the
  whole idempotency design exists to prevent. The queued run must wait
  and re-read, never be cancelled.
- Pick the milestone in JS, not in jq. jq sorts null before every
  string, so sort_by(.due_on) | .[0] returns an UNDATED open milestone
  in preference to every dated one. An undated bucket has no due date
  and so cannot be the nearest; pickMilestone drops it, and files the
  issue unmilestoned if nothing dated is open.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
Both were suppressed "previously missed" findings, and both were real.

- isPermissionDenied matched on status alone, so a rate limit (also a
  403) and a bad token (401) were waved through as "missing scope" —
  contradicting the comment right above it, which said those must stop
  the sweep. It now excludes rate-limit wording and drops 401.
- The no-op path asked "were advisories ADDED?" when the question is
  "did the issue CHANGE?". An issue filed for A+B whose branch moved so
  only B applies has nothing added, yet its table, severity, affected
  copies and remediation are all stale. The rendered title and body are
  now compared against the issue, and a comment stays reserved for
  genuinely new advisories.
- The title counts the advisories that APPLY, matching the body, so it
  tracks shrinking exposure as well as growth. The marker's GHSA list
  stays monotonic — its job is to remember what has been announced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
Both findings are consequences of round 6's staleness fix, in the two
places it did not reach.

- Resolve the matching open issue BEFORE the skip paths, and rewrite it
  to a cleared state when the exposure is gone. Yesterday's issue is
  still open today, so a manifest that has since been removed or copies
  that have moved out of range left its body asserting a vulnerability
  that no longer exists and its Todo/High card live indefinitely. The
  marker is retained so the issue is reused if the advisory returns; the
  issue is not auto-closed, because whether the card belongs in Done or
  should be deleted depends on why the exposure went away.
- The body's prose counted the marker's monotonic history rather than
  the advisories that apply, so it could claim two open alerts while the
  title and table correctly showed one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
Three reviews I missed: my poll called the reviews endpoint without
--paginate, so it only ever read page one — the same pagination bug
this PR was reviewed for.

- Reconcile marked issues whose bump has left the open alert feed
  entirely. openAlerts asks for state=open, so a fixed or dismissed
  alert simply vanishes: its group is never built, the loop never
  visits it, and its issue kept asserting a vulnerability with a live
  Todo/High card. Both cases are covered — the zero-alert run, which
  used to return before loading issues at all, and a vanished group
  while other groups remain.
- Ask for a PARENT-SCOPED overrides entry when the package is also
  declared directly. npm rejects an override contradicting a direct
  dependency with EOVERRIDE, so the guidance for the mixed case would
  not have applied. The issue now prints the exact nested JSON.
- Correct the body's NOTE: severity, range, GHSA and CVE are the
  advisory's own. Only the installed versions, their paths and range
  applicability are verified against v2/main.
- Stop the new-advisory comment claiming the body marker "now covers"
  the advisory; it is posted before the edit, so that is false at
  posting time. It speaks for its own marker instead.
- Cover the successful two-field board placement, not only its failure
  modes.
- AGENTS.md: the monthly sweep NEVER boards; only the security sweep
  does, and only with a PAT. The old wording promised a card the
  monthly job cannot create.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
- Filter to the npm ecosystem. Dependabot alerts are not npm-only and
  this repo has a Dockerfile, so an alert against it would have been
  parsed as a lockfile, thrown, and aborted the whole daily sweep
  before any npm group ran. A non-npm alert is now reported loudly with
  its GHSAs so a human can file it, and readManifest no longer lets one
  unparseable manifest take the run down.
- Distinguish a superseded bump from a closed one. GitHub can revise
  first_patched_version, moving an advisory to a different key while it
  stays open; reporting that as "fixed or dismissed" would stand down a
  live exposure. The reason now reads from the open GHSA set.
- Only claim security-update PRs are off when the run actually read the
  setting. The guard degrades to UNVERIFIED, and an issue asserting
  what the run could not confirm is worse than one that stays quiet.
- README.md and AGENTS.md: scripts/ now holds repo automation run from
  CI, not only build/verify tooling.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
Build the open-GHSA set from the raw alert feed, not from the grouped
alerts. groupAlerts deliberately drops an alert with no
first_patched_version, so an advisory that stays OPEN but loses its
patched version vanished from both the keys and that set — and
reconciliation would have called it "fixed or dismissed".

Same wrong direction as the superseded case fixed last round, reached a
different way: a still-open advisory must never stand itself down.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
Three findings, all of them the same invariant reached by new routes: a
still-open advisory must never stand itself down, and a cleared issue
must be written once.

- Never clear an advisory that is open but unpatched. groupAlerts drops
  an alert with no first_patched_version, so it has no replacement
  group and no replacement issue — calling it "superseded" was false
  and clearing it removed the only thing tracking a live exposure. Such
  an issue is now left exactly as it is, and said so in the log.
- Stop re-editing cleared issues daily. A cleared issue stays open, so
  the next run regenerated its body with a new date and edited it
  again, forever. The date is now read back off the existing body, so
  only a real change writes — and a real change takes today's date.
- Distinguish an absent manifest from an unparseable one. Both returned
  null, so a malformed lockfile was treated as "the manifest is gone"
  and cleared the issue. A read error is evidence of nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
- Reconcile on (package, manifest, GHSA) tuples, not GHSAs alone.
  Dependabot alerts are per manifest and this repo has five lockfiles,
  so the same advisory legitimately covers the root install and a
  client. Keyed on the GHSA alone, another manifest's still-filable
  alert could vouch for this one and clear an issue whose own alert is
  open with no bump available.
- Rename the cleared-date test: it asserts the NEW date is taken when
  the reason changes, which is the opposite of what its name said.
- AGENTS.md no longer implies both sweeps select a dated milestone.
  Only the security sweep filters undated buckets; the monthly one's
  selection is raised on #2239.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
Comment thread AGENTS.md
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review round 14 — all 3 addressed

Also rebased onto the freshly-rebased stack (thanks — the base moved twice while this was gating).

A GHSA alone is not a key; alerts are per manifest

The real one. This repo has five lockfiles, so the same advisory covering both the root install and a client is the normal case. Keyed on the GHSA alone, the client's still-filable alert would vouch for the root issue — and an issue whose own alert had lost its patched version would be cleared on the strength of a different lockfile's alert.

Both sets are now (package, manifest, GHSA) tuples through a shared advisoryKey, and reconciliation builds its lookups from the marker's own package and manifest, so nothing outside that manifest can speak for it.

This is the same rule as the previous rounds, hit through the identity of an advisory rather than its state:

Clearing needs positive evidence about this advisory in this manifest.

Two smaller ones

  • Test name said the opposite of its assertiona cleared issue keeps its original date when its reason changes asserts the new date is taken. Renamed. Worse than a vague name: the failure message would have sent someone after the wrong bug.
  • AGENTS.md asserted an invariant only one script keeps. Correct. I fixed the documentation rather than the monthly script, for the same scope reason as the earlier dependency-refresh findings — that selection is chore(deps): remove dependabot.yml, fold action bumps into the monthly sweep #2239's code and is raised there with the jq null-ordering demonstration. The sentence now scopes the dated-milestone behavior to the security sweep and says plainly that the monthly one does not yet filter undated buckets, so it cannot be read as a guarantee both scripts keep.

Verification

  • scripts/dependabot-alerts.test.mjs: 71 → 73 tests. another manifest's alert cannot vouch for this one when clearing puts an unpatched root alert and a filable clients/tui alert for the same GHSA in one feed, and asserts the TUI bump is filed while the root issue is left as is. Plus advisoryKey directly.
  • npm run local:gate green on the rebased branch, with GITHUB_REPOSITORY set.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Revised alert versions can incorrectly clear an existing issue when the replacement group cannot be validated.

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

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

Comment thread scripts/dependabot-alerts.mjs Outdated
Record each advisory's disposition during the loop instead of deriving
"can this be filed?" from the groups beforehand.

Membership in a group only means an alert exists. Whether an issue
tracks it is decided by the ecosystem check, the manifest read and the
range probe that follow — so if GitHub revised first_patched_version
while that manifest happened to be unparseable, the replacement group
was skipped and the old-key issue was still cleared as "superseded",
claiming an issue that was never filed.

Each advisory is now noted as tracked, not-exposed or indeterminate as
the loop reaches it, and reconciliation refuses to clear while anything
still open is indeterminate or absent. That also splits the clear reason
honestly: a bump whose copies simply left range now says so, rather than
claiming a replacement issue exists.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot review round 15 — addressed

One finding, and it was asking the right question of the wrong data.

filableAdvisories was built from groups before the loop, so membership meant "an alert exists" — while what reconciliation needs is "does an issue track this after the run", which the ecosystem check, the manifest read and the range probe all get a say in. So a revised first_patched_version landing while that manifest happened to be unparseable would skip the replacement group and still clear the old issue as "superseded, it has its own issue" — an issue that was never filed.

Each advisory is now noted as the loop reaches it:

disposition set when
tracked survived every check; an issue exists for it
not-exposed probed, nothing installed in range (or the manifest is gone)
indeterminate could not be probed — non-npm, or a manifest that would not parse
absent no group carried it, so no bump is available

Reconciliation refuses to clear while anything still open is indeterminate or absent.

It exposed a second inaccuracy in the same block

Following the disposition through made this obvious: when every still-open advisory is not-exposed, the old wording still said "superseded — has their own issue", and there is no issue in that case. The reason is now chosen from the dispositions, so a bump whose copies simply left range says no installed copy is in range of its advisories any more.

Where this leaves the invariant

Five rounds have now hit the same rule from five directions — a vanished key, a removed patched version, a failed read, a cross-manifest GHSA, and now an unvalidated replacement. Stated in its final form:

Clearing requires positive evidence, established this run, about every advisory still open for this package and manifest. "Not in the feed", "not in a group", "could not be read" and "some other manifest has it" are all absence of information.

That is now enforced structurally — by a disposition the loop has to actually set — rather than by each site remembering to check.

Verification

  • scripts/dependabot-alerts.test.mjs: 73 → 75 tests, the pair your last sentence asked for: a revised bump does not clear the old issue when the probe is indeterminate (revised version + truncated lockfile; asserts no edit and no create) and a revised bump that is no longer exposed clears without claiming an issue (probe runs; asserts out-of-range wording and no "superseded").
  • npm run local:gate green.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It introduces stateful security automation involving external repository settings, issue mutation, and project-board writes that warrant final human validation.

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The workflow performs security-alert reconciliation and live issue and board mutations whose operational behavior warrants final human review.

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

@cliffhall

Copy link
Copy Markdown
Member Author

Review loop converged — 17 rounds, 37 findings, all answered

Rounds 16 and 17 both returned zero new comments and zero suppressed findings, with the verdict 🔵 "needs a closer look" — a request for human validation of what the automation does, not a change request. Two consecutive clean rounds is the stop condition.

37 Copilot inline comments, 37 replied to. Nothing outstanding.

Where the findings landed

34 implemented, 2 declined, 4 relayed to #2239 (three of which that PR has since taken).

The declines were both evidence-based rather than judgement calls:

  • vulnerability-alerts is a valid permissions key — settled by pushing a probe branch and reading the runner's own VulnerabilityAlerts: read line plus a successful alerts API call, since the docs and community threads contradict each other. security-events is the code-scanning permission and would be a wider, wrong grant.
  • latestReleaseTag's 404 handling was chore(deps): remove dependabot.yml, fold action bumps into the monthly sweep #2239's code, relayed rather than fixed in this diff.

What the review actually caught

Four defects that would have reached production, none of which the tests or the gate would have found:

A crash a Dockerfile alert parsed as an npm lockfile would abort the whole daily sweep — and this repo has one
A silent no-op gh api --paginate emits one JSON array per page; JSON.parse throws past 100 alerts
Unusable advice a package-wide overrides pin is rejected with EOVERRIDE when the package is also declared directly
A stale security card five separate routes by which an issue could keep asserting a vulnerability that no longer existed — or, worse, be cleared while one still did

That last one is the substantive outcome. Rounds 6, 7, 11, 12, 13, 14 and 15 each reached the same rule from a different direction — a vanished key, a removed patched version, a failed read, a cross-manifest GHSA, an unvalidated replacement — until it was stated and enforced structurally:

Clearing an issue requires positive evidence, established this run, about every advisory still open for that package and manifest. "Not in the feed", "not in a group", "could not be read" and "another manifest has it" are all absence of information.

It is now a per-advisory disposition (tracked / not-exposed / indeterminate / absent) the loop has to actually set, rather than a check each call site must remember.

State

  • build and coverage green; mergeable.
  • scripts/dependabot-alerts.test.mjs: 18 → 75 tests, 30 of them driving main() through an injected spawn.
  • Rebased onto the current stack after the merges.

The two things still needing a human

  1. DELETE /repos/modelcontextprotocol/inspector/automated-security-fixes — a repo setting, not a file, so no PR can make it. It still reports {"enabled":true,"paused":false}. Alerts must stay on.
  2. Scheduled workflows only run from the default branch, so the sweep starts at the next milestone merge into main, not when this lands on v2/main. The acceptance items that need a real run (workflow_dispatch) are confirmable only after that.

@cliffhall

Copy link
Copy Markdown
Member Author

✅ Repo settings done — automated-security-fixes is off

A maintainer has turned off Dependabot's security-update PRs in the repo settings (Settings → Advanced Security → Dependabot security updates → Disable). This was the one "Done when" item on #2233 that no PR could satisfy, since it is a repo setting rather than a file.

Verified just now against the live repo:

Check Result
GET /repos/…/automated-security-fixes {"enabled":false,"paused":false} ✅ Dependabot opens no security-update PRs
GET /repos/…/vulnerability-alerts HTTP 204 ✅ alerts still on — the sweep's input is intact
GET /repos/…/dependabot/alerts?state=open 7 open alerts ✅ feed readable and unaffected

The sweep's own precondition (checkSecurityPrsStillDisabled) was run against that live response and returns PASS, so the workflow will not trip its guard on the first scheduled run.

Configuration state

With this flipped and #2239 having removed .github/dependabot.yml, Dependabot now opens no pull requests against this repo at all — neither version updates nor security updates. Both halves of #2229 are switched off, and both replacements are in the stack:

switched off by replaced by
Version updates dependabot.yml deleted (#2235 / PR #2239) monthly dependency-refresh sweep
Security updates this settings change daily dependabot-alerts sweep (this PR)

Nothing regresses in the meantime: with the setting off and the sweep not yet running, security alerts simply accumulate — they are not lost, and the first run picks up whatever is open then.

What happens at the next release

⚠️ Scheduled workflows only run from the default branch, so this sweep does not start when the stack merges to v2/main — it starts at the next milestone merge into main. After that merge, the acceptance items that need a real run become checkable:

  1. workflow_dispatch the Dependabot Alert Sweep and confirm it files one issue per bump, not per advisory — today's 7 open alerts should produce exactly 3 issues (fast-uri → 3.1.6 covering 4 advisories, qs → 6.16.0 covering 2, browserslist → 4.28.7 covering 1).
  2. Run it a second time and confirm a complete no-op.
  3. Confirm each issue is labeled v2 + chore + dependabot and milestoned. Board placement at Todo / High happens only if a PROJECT_TOKEN secret with organization projects: write exists; without it the issues are filed unboarded and the next /issue-triage sweep places them — that is expected, not a failure.

Optional, and worth doing while you are in settings: giving PROJECT_TOKEN an additional administration: read scope upgrades the security-PR guard from "reports UNVERIFIED" to a real assertion, so a future re-enable of that setting fails the run loudly instead of going unnoticed. GITHUB_TOKEN can never hold that scope — permissions: has no key for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Workflow: consume Dependabot security alerts into board-tracked issues and disable security-update PRs

2 participants