From 66a3332f1d280385e28f7159be72e4cdf94b329c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 21:13:39 -0400 Subject: [PATCH 01/14] chore(deps): sweep Dependabot alerts into board-tracked issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- .github/workflows/dependabot-alerts.yml | 89 +++ .github/workflows/dependency-refresh.yml | 26 +- AGENTS.md | 18 + package-lock.json | 1 + package.json | 1 + scripts/dependabot-alerts.mjs | 691 +++++++++++++++++++++++ scripts/dependabot-alerts.test.mjs | 265 +++++++++ scripts/dependency-refresh.mjs | 11 +- 8 files changed, 1085 insertions(+), 17 deletions(-) create mode 100644 .github/workflows/dependabot-alerts.yml create mode 100644 scripts/dependabot-alerts.mjs create mode 100644 scripts/dependabot-alerts.test.mjs diff --git a/.github/workflows/dependabot-alerts.yml b/.github/workflows/dependabot-alerts.yml new file mode 100644 index 000000000..72bd99d31 --- /dev/null +++ b/.github/workflows/dependabot-alerts.yml @@ -0,0 +1,89 @@ +# Dependabot alert sweep (#2233), the alert-consuming half of #2229. +# +# Dependabot's SECURITY-update PRs are turned off for this repo; its ALERTS are +# left on. Those are two independent settings, and this workflow depends on the +# split: it reads the alerts and turns them into ordinary board-tracked issues, +# so the fix is written by hand against `v2/main` like any other work. +# +# alert -> this sweep -> issue (labeled, milestoned, boarded) -> maintainer PR -> v2/main +# +# It is a SCHEDULE, not an event handler, because there is no `dependabot_alert` +# workflow trigger — that is a webhook event only. Daily is deliberate: with +# security PRs off there is no mergeable-against-`main` artifact and no window to +# race, so the merge guard #2060 needed has no analogue here. What replaces it is +# a precondition inside the script: `automated-security-fixes` is a repo SETTING +# and can be switched back on from the UI without a commit, so the sweep asserts +# it is still disabled and fails loudly if it is not. +# +# ⚠️ GitHub computes the dependency graph, and therefore every alert, from the +# DEFAULT branch (`main`), while we ship from `v2/main`. Two consequences: +# +# * An alert is re-checked against `v2/main`'s own lockfile before an issue is +# filed — hence the `ref: v2/main` checkout below. An alert whose vulnerable +# range no longer matches is already fixed on the branch we ship from and is +# waiting on a milestone merge to close, so it is skipped silently. +# * A vulnerable dependency introduced on `v2/main` and not yet merged to +# `main` produces NO alert at all. No approach that consumes GitHub's alerts +# avoids that. The release-time `npm audit fix` from #2231 is a second signal +# that partially covers it; a scheduled `npm audit --audit-level=high` over +# `v2/main`'s lockfiles would close it fully and is a separable follow-up. +# +# `vulnerability-alerts: read` is the one non-default permission, and +# `GITHUB_TOKEN` supports it — no PAT is needed to read the alerts themselves. +# Two side steps are outside its reach, and `PROJECT_TOKEN` is what covers them +# when it exists: +# +# * Writing the board card, since board #28 is an ORG project +# (`organization projects: write`). Absent, the issue is still filed labeled +# and milestoned and the next `/issue-triage` sweep boards it. +# * Reading back `automated-security-fixes`, which needs `administration: read` +# — a permission `permissions:` has no key for, so `GITHUB_TOKEN` can never +# have it. Absent, that assertion is reported as UNVERIFIED rather than +# failing the run; an explicit `enabled: true` still fails it. +# +# Nothing the sweep exists to do is skipped for want of that secret. +# +# The version-update half of #2229 is the sibling `dependency-refresh.yml`. +name: Dependabot Alert Sweep + +on: + schedule: + - cron: "17 6 * * *" # 06:17 UTC daily; alerts are not minute-sensitive + workflow_dispatch: + +permissions: + contents: read + issues: write + vulnerability-alerts: read + +jobs: + alert-sweep: + runs-on: ubuntu-latest + steps: + - name: Checkout v2/main + uses: actions/checkout@v7 + with: + ref: v2/main + + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version: "22.x" + cache: "npm" + + # Root install only, and no lifecycle scripts. The sweep's one dependency + # is `semver`; it reads every lockfile as JSON and never needs a client's + # tree on disk, so the postinstall cascade into `clients/*` that + # `dependency-refresh.yml` genuinely needs (it shells out to + # `npm outdated` in each) would be minutes of nothing here. + - name: Install root dependencies + run: npm ci --ignore-scripts + + - name: Run the Dependabot alert sweep + run: node scripts/dependabot-alerts.mjs + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITHUB_REPOSITORY: ${{ github.repository }} + # Optional: an org-project PAT with `organization projects: write`. + # Absent, the issue is filed unboarded and triage picks it up. + PROJECT_TOKEN: ${{ secrets.PROJECT_TOKEN }} diff --git a/.github/workflows/dependency-refresh.yml b/.github/workflows/dependency-refresh.yml index 89687b71b..4e5f87a6f 100644 --- a/.github/workflows/dependency-refresh.yml +++ b/.github/workflows/dependency-refresh.yml @@ -2,19 +2,21 @@ # # A Dependabot version-update PR carries no issue and no board card, so # `.github/dependabot.yml` was removed outright in #2235 — npm and -# github-actions alike. This workflow is what replaced those PRs (security -# updates are a separate mechanism and stay on; see below): it runs -# `scripts/dependency-refresh.mjs` against `v2/main` once a month and files or -# updates ONE tracking issue listing every outdated npm package across the root -# install and each client, plus any workflow `uses:` ref behind its action's -# highest released version. No PR is opened automatically. A maintainer -# reviews the -# issue, picks what to bump, and opens a normal PR against `v2/main`. +# github-actions alike. This workflow is what replaced those PRs (the +# security-update half is a separate mechanism, switched off separately; see +# below): it runs `scripts/dependency-refresh.mjs` against `v2/main` once a +# month and files or updates ONE tracking issue listing every outdated npm +# package across the root install and each client, plus any workflow `uses:` +# ref behind its action's highest released version. No PR is opened +# automatically. A maintainer reviews the issue, picks what to bump, and opens +# a normal PR against `v2/main`. # -# Dependabot SECURITY updates are unaffected: they are enabled in repo -# settings, not in a config file, and kept working while `dependabot.yml` was -# missing entirely (see #1833, #1840). They are raised against the default -# branch and still need retargeting by hand. +# Dependabot SECURITY updates are the other half, and #2233 turned their PRs +# off too — they were enabled in repo settings rather than in a config file, +# which is why they kept working while `dependabot.yml` was missing entirely +# (see #1833, #1840) and why deleting that file did not touch them. Its +# ALERTS stay on and are swept into issues daily by the sibling +# `dependabot-alerts.yml`. Between them, Dependabot opens no PRs here at all. # # `GITHUB_TOKEN` is sufficient: it only needs to read milestones and the public # release feeds of the actions we use, and to create/edit an issue diff --git a/AGENTS.md b/AGENTS.md index ca6acc1b1..a5502051b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -98,6 +98,24 @@ The reasoning behind each of these, and what breaks when it is ignored, is the - **One version per install-crossing dependency.** When bumping a dependency the shared sources pull in, bump it in every install that declares it. Consolidating to the root is what makes most of these unbumpable in two places at once, but it does not retire the rule — a client's `devDependencies`, and any package that arrives transitively into a client install, can still skew against the root. Never raise the tsc heap to work around one. `npm run verify:dep-lockstep` enforces this. - **Pin a transitive dependency with an `overrides` entry**, not with `npm audit fix` — which "resolves" an advisory with no upward escape by silently downgrading. +### Dependency updates are issue-driven, like everything else + +**Dependabot opens no pull requests against this repo — neither version updates nor security updates.** A Dependabot PR carries no `Closes #N` and no board card, so it was the one standing exception to [Issue-driven Work Style](#issue-driven-work-style), enforced by nothing. Both halves are now replaced by scheduled workflows that file **issues**, and a maintainer writes the fix by hand against `v2/main`. + +| Half | Switched off by | Replaced by | Cadence | +| --- | --- | --- | --- | +| Version updates | Deleting `.github/dependabot.yml` outright (#2235) — an empty `updates:` list is not valid config | `.github/workflows/dependency-refresh.yml` → `scripts/dependency-refresh.mjs`: `npm outdated` across every install, plus a `uses:` check against each action's latest release, folded into **one** tracking issue | Monthly | +| Security updates | `DELETE /repos/{owner}/{repo}/automated-security-fixes` — a **repo setting**, not a file | `.github/workflows/dependabot-alerts.yml` → `scripts/dependabot-alerts.mjs`: reads the alerts and files one issue **per bump** | Daily | + +Four things about this that are not obvious from the code: + +- **Dependabot *alerts* stay on.** Alerts and security-update PRs are independent settings; only the PRs are off. Turning alerts off would blind the sweep that replaced them. +- **The security half is a schedule, not an event handler**, because there is no `dependabot_alert` workflow trigger — it is a webhook event only. +- **Alerts are computed from the default branch (`main`), and we ship from `v2/main`.** So the sweep re-checks each alert's vulnerable range against `v2/main`'s own lockfile before filing, and skips one that is already fixed there. The converse is a real blind spot with no fix on this path: a vulnerable dependency introduced on `v2/main` and not yet merged to `main` produces **no alert at all**. The release-time `npm audit fix` (#2231) is the partial second signal. +- **`automated-security-fixes` can be re-enabled from the UI without a commit**, so nothing in the repo would record it. The sweep asserts it is still disabled and **fails loudly** if it is not; a red run of that workflow means the setting was flipped, not that the script broke. + +An issue filed by either sweep is an ordinary board item — `v2` + `chore` + `dependabot`, the current milestone, and a card on #28. A security issue lands at **Todo / High**: arriving through this pipeline *is* the approval, and `High` is a standing override of the [priority rubric](.claude/skills/issue-triage/SKILL.md), which would otherwise score a routine bump Medium. Board placement needs an org-project PAT that `GITHUB_TOKEN` cannot have, so it is **best-effort** — without the secret the issue is still created labeled and milestoned, and the next triage sweep boards it. + ## Contributing External contributions are accepted as **issues, not pull requests** — maintainers handle design and implementation through a prompt-driven workflow. diff --git a/package-lock.json b/package-lock.json index 30ffce316..bb0cd36ee 100644 --- a/package-lock.json +++ b/package-lock.json @@ -46,6 +46,7 @@ "express": "^5.2.1", "globals": "^17.7.0", "prettier": "3.8.4", + "semver": "^7.8.5", "typescript": "~5.9.3", "typescript-eslint": "^8.65.0", "vitest": "4.1.10" diff --git a/package.json b/package.json index 3d940a819..0614b5182 100644 --- a/package.json +++ b/package.json @@ -123,6 +123,7 @@ "express": "^5.2.1", "globals": "^17.7.0", "prettier": "3.8.4", + "semver": "^7.8.5", "typescript": "~5.9.3", "typescript-eslint": "^8.65.0", "vitest": "4.1.10" diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs new file mode 100644 index 000000000..d56f5f654 --- /dev/null +++ b/scripts/dependabot-alerts.mjs @@ -0,0 +1,691 @@ +#!/usr/bin/env node +// Dependabot alert sweep (#2233), the alert-consuming half of #2229. +// +// Dependabot's SECURITY-update PRs are turned off in repo settings; its alerts +// stay on. This script is what consumes them: +// +// alert -> scheduled sweep -> issue (labeled, milestoned, boarded) -> maintainer PR -> v2/main +// +// A Dependabot-opened PR carries no `Closes #N` and no board card, which is the +// carve-out from "every PR references an issue" that #2229 exists to remove. +// The version-update half was removed outright in #2235 and replaced by +// `dependency-refresh.mjs`; this is the security half, and it files issues +// rather than PRs for the same reason. +// +// Three things shape the design, each verified against this repo before it was +// written: +// +// 1. There is no `dependabot_alert` WORKFLOW trigger — it is a webhook event +// only — so this is a scheduled sweep, not event-driven. Daily is enough; +// alerts are not minute-sensitive. +// 2. `GITHUB_TOKEN` can read alerts with `vulnerability-alerts: read`, so no +// PAT is needed for the sweep itself. Two side steps DO need one, and both +// are best-effort rather than preconditions: writing the board card (an org +// project is outside `GITHUB_TOKEN`'s reach — an unboarded-but-milestoned +// issue is swept into Todo by the next `/issue-triage` pass), and reading +// back the `automated-security-fixes` setting (`administration: read`, +// which `permissions:` cannot grant at all). +// 3. Alerts are per-ADVISORY but a fix is per-BUMP. Today's seven open alerts +// are three `overrides` entries, so grouping by +// `(package, manifest_path, first_patched_version)` is what keeps this from +// filing seven issues for three pieces of work. +// +// ⚠️ GitHub computes the dependency graph — and therefore every alert — from +// the DEFAULT branch (`main`), while we ship from `v2/main`. So an alert is not +// trusted on its face: the vulnerable range is re-checked against `v2/main`'s +// own lockfile before anything is filed. The blind spot that leaves is stated +// plainly in the workflow header: a vulnerable dependency introduced on +// `v2/main` and not yet merged to `main` produces no alert at all, and no +// approach that consumes GitHub's alerts can see it. +// +// Idempotency key is the marker comment at the top of each issue body, which +// names the package, the manifest and every GHSA the issue covers. A second run +// the same day is a complete no-op; a NEW advisory for a package that already +// has an open issue lands as a comment on it and rewrites the marker, rather +// than filing a second issue. +// +// The pure halves — `toSemverRange`, `lockfileVersions`, `isDirectDependency`, +// `groupAlerts`, `buildMarker`, `parseMarker`, `mergeGhsas`, `buildIssueTitle`, +// `buildIssueBody` and `buildNewAdvisoryComment` — are covered by +// `dependabot-alerts.test.mjs`. `main()` is the CLI entry point, exercised +// against the real repo only via `workflow_dispatch` in CI, per the same split +// `dependency-refresh.mjs` and `verify-skills.mjs` already use. + +import { spawnSync } from "node:child_process"; +import { readFileSync } from "node:fs"; +import semver from "semver"; + +/** Board #28 (v2). The project and field node ids are stable; option ids are not. */ +export const PROJECT_ID = "PVT_kwDOCt2Azc4BJVxt"; +export const STATUS_FIELD_ID = "PVTSSF_lADOCt2Azc4BJVxtzg5iI8c"; +export const PRIORITY_FIELD_ID = "PVTSSF_lADOCt2Azc4BJVxtzg5iJE4"; +/** + * Option ids are regenerated whenever a single-select field's option list is + * edited, so they are resolved by NAME at run time rather than hardcoded here — + * a hardcoded id turns an unrelated board edit into a silently mis-set field. + */ +export const BOARD_STATUS = "Todo"; +export const BOARD_PRIORITY = "High"; + +/** + * The branch this repo actually ships from, and whose lockfiles are probed. + * + * The workflow checks this branch out, so manifests are read from the working + * tree rather than through `git show` — the same shape `dependency-refresh.mjs` + * uses to run `npm outdated` against it. Named here only so the issue body can + * say which branch the versions it quotes came from. + */ +export const TARGET_BRANCH = "v2/main"; + +const MARKER_RE = + /^/; + +/** + * The issue body's first line: the idempotency key. + * + * @param {{package: string, manifestPath: string, ghsas: string[]}} group + * @returns {string} + */ +export function buildMarker({ package: pkg, manifestPath, ghsas }) { + return ``; +} + +/** + * Read a marker back off an issue body. + * + * @param {string | undefined} body + * @returns {{package: string, manifestPath: string, ghsas: string[]} | null} + */ +export function parseMarker(body) { + const match = MARKER_RE.exec(body ?? ""); + if (!match) return null; + return { + package: match[1], + manifestPath: match[2], + ghsas: match[3].split(",").filter(Boolean), + }; +} + +/** + * Which of `group`'s advisories the existing issue does not already name. + * + * @param {string[]} existing the marker's GHSA list + * @param {string[]} incoming the GHSAs the sweep just saw + * @returns {{merged: string[], added: string[]}} both sorted + */ +export function mergeGhsas(existing, incoming) { + const known = new Set(existing); + const added = [...new Set(incoming.filter((g) => !known.has(g)))].sort(); + const merged = [...new Set([...existing, ...incoming])].sort(); + return { merged, added }; +} + +/** + * Translate a GitHub `vulnerable_version_range` into a range npm `semver` + * understands. + * + * ⚠️ GitHub separates conjuncts with a COMMA (`>= 3.1.3, < 3.1.6`); node-semver + * reads a comma as nothing at all and quietly returns `false` for a version + * that is in fact in range. Space is semver's AND, so the fix is a split/join — + * but the failure it prevents is silent, which is why this is its own tested + * function rather than an inline `.replace`. + * + * @param {string} range + * @returns {string} + */ +export function toSemverRange(range) { + return range + .split(",") + .map((part) => part.trim()) + .filter(Boolean) + .join(" "); +} + +/** + * Every version of `pkg` installed anywhere in an npm lockfile. + * + * A transitive package can legitimately appear more than once (a nested + * `node_modules/x/node_modules/y`), and the alert applies if ANY copy is in + * range, so this returns them all rather than picking one. + * + * @param {object} lock parsed `package-lock.json` (lockfileVersion 2 or 3) + * @param {string} pkg + * @returns {string[]} sorted, deduped + */ +export function lockfileVersions(lock, pkg) { + const suffix = `node_modules/${pkg}`; + const versions = new Set(); + for (const [path, entry] of Object.entries(lock.packages ?? {})) { + if (path !== suffix && !path.endsWith(`/${suffix}`)) continue; + if (entry?.version) versions.add(entry.version); + } + return [...versions].sort(semver.compare); +} + +/** + * Is `pkg` declared by the manifest itself, rather than pulled in transitively? + * + * Decides which fix the issue asks for: a direct dependency is a plain version + * bump, a transitive one is an `overrides` entry per AGENTS.md's Dependency + * placement — never `npm audit fix`, which "resolves" an advisory with no + * upward escape by silently downgrading. + * + * @param {object} lock parsed `package-lock.json` + * @param {string} pkg + * @returns {boolean} + */ +export function isDirectDependency(lock, pkg) { + const root = lock.packages?.[""] ?? {}; + return Boolean( + root.dependencies?.[pkg] ?? + root.devDependencies?.[pkg] ?? + root.optionalDependencies?.[pkg] ?? + root.peerDependencies?.[pkg], + ); +} + +const SEVERITY_RANK = { critical: 4, high: 3, medium: 2, moderate: 2, low: 1 }; + +/** + * Collapse per-advisory alerts into one entry per BUMP. + * + * Grouped by `(package, manifest_path, first_patched_version)`: that triple is + * one edit to one manifest, which is the unit a maintainer actually acts on. + * Two advisories on the same package with different patched versions are + * different bumps and stay apart. + * + * @param {object[]} alerts raw `GET /repos/{o}/{r}/dependabot/alerts` entries + * @returns {Array<{key: string, package: string, manifestPath: string, fixedIn: string, scope: string, severity: string, ghsas: string[], advisories: Array<{ghsa: string, cve: string | null, severity: string, summary: string, range: string, url: string}>}>} + */ +export function groupAlerts(alerts) { + const groups = new Map(); + for (const alert of alerts) { + if (alert.state !== "open") continue; + const pkg = alert.dependency?.package?.name; + const manifestPath = alert.dependency?.manifest_path; + const fixedIn = + alert.security_vulnerability?.first_patched_version?.identifier; + // No patched version means there is nothing to bump TO — an issue asking + // for an unavailable upgrade is noise, so it waits for one to be published. + if (!pkg || !manifestPath || !fixedIn) continue; + + const key = `${pkg}${manifestPath}${fixedIn}`; + const advisory = { + ghsa: alert.security_advisory?.ghsa_id ?? "", + cve: alert.security_advisory?.cve_id ?? null, + severity: alert.security_advisory?.severity ?? "unknown", + summary: alert.security_advisory?.summary ?? "", + range: alert.security_vulnerability?.vulnerable_version_range ?? "*", + url: alert.html_url ?? "", + }; + + const existing = groups.get(key); + if (existing) { + existing.advisories.push(advisory); + if ( + (SEVERITY_RANK[advisory.severity] ?? 0) > + (SEVERITY_RANK[existing.severity] ?? 0) + ) { + existing.severity = advisory.severity; + } + continue; + } + groups.set(key, { + key, + package: pkg, + manifestPath, + fixedIn, + scope: alert.dependency?.scope ?? "runtime", + severity: advisory.severity, + advisories: [advisory], + }); + } + + return [...groups.values()] + .map((group) => { + group.advisories.sort((a, b) => a.ghsa.localeCompare(b.ghsa)); + group.ghsas = group.advisories.map((a) => a.ghsa); + return group; + }) + .sort( + (a, b) => + a.package.localeCompare(b.package) || + a.manifestPath.localeCompare(b.manifestPath) || + a.fixedIn.localeCompare(b.fixedIn), + ); +} + +/** + * @param {ReturnType[number]} group + * @returns {string} + */ +export function buildIssueTitle(group) { + const n = group.advisories.length; + return `chore(deps): bump \`${group.package}\` to \`${group.fixedIn}\` in \`${group.manifestPath}\` (${n} ${n === 1 ? "advisory" : "advisories"})`; +} + +const PLACEMENT_DOC = + "https://github.com/modelcontextprotocol/inspector/blob/v2/main/AGENTS.md#dependency-placement"; + +/** + * @param {ReturnType[number]} group + * @param {{installed: string[], direct: boolean, ghsas?: string[]}} probe + * `ghsas` overrides the marker's list when an existing issue is being + * rewritten to cover advisories it did not originally name. + * @returns {string} + */ +export function buildIssueBody(group, { installed, direct, ghsas }) { + const covered = ghsas ?? group.ghsas; + const rows = group.advisories + .map( + (a) => + `| [${a.ghsa}](${a.url}) | ${a.cve ?? "—"} | ${a.severity} | ${a.range} | ${a.summary.replace(/\|/g, "\\|")} |`, + ) + .join("\n"); + + const fix = direct + ? `\`${group.package}\` is a **direct** dependency of \`${group.manifestPath.replace(/package-lock\.json$/, "package.json")}\` — bump its declared range to \`>=${group.fixedIn}\`.` + : `\`${group.package}\` is **transitive**, so the fix is an [\`overrides\`](${PLACEMENT_DOC}) entry pinning it to \`${group.fixedIn}\` — **not** \`npm audit fix\`, which "resolves" an advisory with no upward escape by silently downgrading.`; + + return [ + buildMarker({ ...group, ghsas: covered }), + `Filed automatically from ${covered.length} open Dependabot ${covered.length === 1 ? "alert" : "alerts"} (#2233). Dependabot opens no security-update PRs on this repo; the fix is written by hand against \`v2/main\`.`, + "", + "| | |", + "| --- | --- |", + `| Package | \`${group.package}\` |`, + `| Manifest | \`${group.manifestPath}\` |`, + `| Installed on \`v2/main\` | ${installed.length > 0 ? installed.map((v) => `\`${v}\``).join(", ") : "—"} |`, + `| Fixed in | \`${group.fixedIn}\` |`, + `| Scope | ${group.scope} |`, + `| Highest severity | ${group.severity} |`, + "", + "## Advisories", + "", + "| GHSA | CVE | Severity | Vulnerable range | Summary |", + "| --- | --- | --- | --- | --- |", + rows, + "", + "## Fix", + "", + fix, + "", + "> [!NOTE]", + `> **Priority is a standing rubric override.** A routine bump scores Medium; a security bump is filed **${BOARD_PRIORITY}** so it does not sit. The version and severity above come from \`${TARGET_BRANCH}\`'s own lockfile, not from the alert — GitHub computes alerts from the default branch, so an alert is only filed here after its vulnerable range is re-checked against the branch we ship from.`, + ].join("\n"); +} + +/** + * The comment a NEW advisory for an already-open issue gets, instead of a + * second issue. + * + * @param {ReturnType[number]} group + * @param {string[]} added the GHSAs not previously covered + * @returns {string} + */ +export function buildNewAdvisoryComment(group, added) { + const rows = group.advisories + .filter((a) => added.includes(a.ghsa)) + .map( + (a) => + `| [${a.ghsa}](${a.url}) | ${a.severity} | ${a.summary.replace(/\|/g, "\\|")} |`, + ) + .join("\n"); + return [ + `${added.length} new Dependabot ${added.length === 1 ? "advisory" : "advisories"} for \`${group.package}\`, cleared by the same bump to \`${group.fixedIn}\`. The issue body's marker now covers ${added.length === 1 ? "it" : "them"} too.`, + "", + "| GHSA | Severity | Summary |", + "| --- | --- | --- |", + rows, + ].join("\n"); +} + +// --------------------------------------------------------------------------- +// Impure half: everything below shells out to `gh` or `git`. +// --------------------------------------------------------------------------- + +function gh(args, { token } = {}) { + const env = token ? { ...process.env, GH_TOKEN: token } : process.env; + const result = spawnSync("gh", args, { encoding: "utf8", env }); + if (result.error) throw result.error; + return result; +} + +function ghJson(args) { + const result = gh(args); + if (result.status !== 0) { + throw new Error(`gh ${args[0]} failed: ${(result.stderr ?? "").trim()}`); + } + return JSON.parse(result.stdout || "null"); +} + +/** + * Detect whether Dependabot's security-update PRs have been switched back on. + * + * `automated-security-fixes` is a repo SETTING, so it can be re-enabled from + * the UI without a commit and nothing in this repo would record it. This check + * is this design's analogue of the merge guard #2060 needed: one API call + * standing in for a required status check plus a ruleset change. + * + * ⚠️ **The endpoint needs `administration: read`, which `GITHUB_TOKEN` cannot + * be granted** — `permissions:` has no such key. So with the default token the + * call 403s, and treating that as failure would make every scheduled run red + * for a reason unrelated to the alerts. It is therefore reported and skipped: + * only an explicit `enabled: true` throws. Give `PROJECT_TOKEN` the extra + * `administration: read` scope and the guard becomes a real assertion; without + * it the sweep still does its job, it just cannot see that setting. + */ +function checkSecurityPrsStillDisabled(repo) { + const result = gh(["api", `repos/${repo}/automated-security-fixes`], { + token: process.env.PROJECT_TOKEN, + }); + if (result.status !== 0) { + console.log( + "dependabot-alerts: cannot read automated-security-fixes " + + `(${(result.stderr ?? "").trim()}) — the token lacks \`administration: read\`, ` + + "so whether Dependabot security PRs are still off is UNVERIFIED this run", + ); + return; + } + const state = JSON.parse(result.stdout || "{}"); + if (state.enabled === true) { + throw new Error( + "Dependabot security-update PRs are ENABLED again. This sweep exists to replace them; " + + "an enabled setting means both flows are running and Dependabot is opening PRs with no " + + "issue and no board card. Disable it (Settings -> Code security, or " + + `DELETE /repos/${repo}/automated-security-fixes) and re-run.`, + ); + } +} + +function openAlerts(repo) { + return ghJson([ + "api", + "--paginate", + `repos/${repo}/dependabot/alerts?state=open&per_page=100`, + ]); +} + +/** + * A manifest's contents in the checkout, or `null` when it is absent — an alert + * against a manifest this branch does not have is not actionable. + */ +function readManifest(manifestPath) { + try { + return JSON.parse(readFileSync(manifestPath, "utf8")); + } catch (error) { + if (error.code === "ENOENT") return null; + throw error; + } +} + +function openDependabotIssues(repo) { + return ghJson([ + "issue", + "list", + "--repo", + repo, + "--state", + "open", + "--label", + "dependabot", + "--json", + "number,body", + "--limit", + "100", + ]); +} + +function currentMilestone(repo) { + const result = gh([ + "api", + `repos/${repo}/milestones`, + "--jq", + 'map(select(.state=="open")) | sort_by(.due_on) | .[0].title // empty', + ]); + if (result.status !== 0) { + throw new Error(`milestone lookup failed: ${(result.stderr ?? "").trim()}`); + } + return result.stdout.trim() || null; +} + +/** + * Resolve a single-select option id by NAME. + * + * Option ids are regenerated whenever the field's option list is edited, so + * looking them up each run is what keeps an unrelated board edit from turning + * into a silently mis-set field here. + */ +function optionId(fieldName, optionName, token) { + const result = gh( + [ + "project", + "field-list", + "28", + "--owner", + "modelcontextprotocol", + "--format", + "json", + ], + { token }, + ); + if (result.status !== 0) { + throw new Error(`field-list failed: ${(result.stderr ?? "").trim()}`); + } + const field = JSON.parse(result.stdout).fields.find( + (f) => f.name === fieldName, + ); + const option = field?.options?.find((o) => o.name === optionName); + if (!option) { + throw new Error( + `no ${fieldName} option named "${optionName}" on board #28`, + ); + } + return option.id; +} + +/** + * Put the issue on board #28 at Todo / High. + * + * Best-effort by design: an org project is outside `GITHUB_TOKEN`'s reach, so + * this needs a PAT the workflow may not have. A failure here is logged and the + * run continues — the issue is already labeled and milestoned, which is enough + * for the next `/issue-triage` sweep to board it (its documented exception + * moves an unboarded-but-milestoned issue straight into Todo). + * + * Todo rather than Incoming: arriving through this pipeline IS the approval. + */ +function addToBoard(issueUrl) { + const token = process.env.PROJECT_TOKEN; + if (!token) { + console.log( + "dependabot-alerts: PROJECT_TOKEN unset — issue left unboarded for the next triage sweep", + ); + return; + } + try { + const added = gh( + [ + "project", + "item-add", + "28", + "--owner", + "modelcontextprotocol", + "--url", + issueUrl, + "--format", + "json", + ], + { token }, + ); + if (added.status !== 0) { + throw new Error((added.stderr ?? "").trim()); + } + const itemId = JSON.parse(added.stdout).id; + // Each item-edit sets exactly one field, so Status and Priority are two calls. + for (const [fieldId, fieldName, optionName] of [ + [STATUS_FIELD_ID, "Status", BOARD_STATUS], + [PRIORITY_FIELD_ID, "Priority", BOARD_PRIORITY], + ]) { + const edit = gh( + [ + "project", + "item-edit", + "--project-id", + PROJECT_ID, + "--id", + itemId, + "--field-id", + fieldId, + "--single-select-option-id", + optionId(fieldName, optionName, token), + ], + { token }, + ); + if (edit.status !== 0) throw new Error((edit.stderr ?? "").trim()); + } + console.log( + `dependabot-alerts: boarded ${issueUrl} at ${BOARD_STATUS}/${BOARD_PRIORITY}`, + ); + } catch (error) { + console.log( + `dependabot-alerts: board write failed (${error.message}) — issue is labeled and milestoned, next triage sweep will board it`, + ); + } +} + +function createIssue(repo, group, body) { + const milestone = currentMilestone(repo); + const args = [ + "issue", + "create", + "--repo", + repo, + "--title", + buildIssueTitle(group), + "--label", + "v2", + "--label", + "chore", + "--label", + "dependabot", + "--body", + body, + ]; + if (milestone) args.push("--milestone", milestone); + const result = gh(args); + if (result.status !== 0) { + throw new Error(`gh issue create failed: ${(result.stderr ?? "").trim()}`); + } + const url = result.stdout.trim(); + if (!milestone) { + console.log( + "dependabot-alerts: no open milestone — issue filed unmilestoned", + ); + } + console.log(`dependabot-alerts: filed ${url}`); + return url; +} + +export function main(repo = process.env.GITHUB_REPOSITORY) { + if (!repo) throw new Error("repo not specified (GITHUB_REPOSITORY unset)"); + + checkSecurityPrsStillDisabled(repo); + + const groups = groupAlerts(openAlerts(repo)); + if (groups.length === 0) { + console.log("dependabot-alerts: no open alerts — no-op"); + return; + } + + const existingIssues = openDependabotIssues(repo).map((issue) => ({ + ...issue, + marker: parseMarker(issue.body), + })); + + const manifests = new Map(); + for (const group of groups) { + if (!manifests.has(group.manifestPath)) { + manifests.set(group.manifestPath, readManifest(group.manifestPath)); + } + const lock = manifests.get(group.manifestPath); + if (lock === null) { + console.log( + `dependabot-alerts: ${group.manifestPath} absent on ${TARGET_BRANCH} — skipping ${group.package}`, + ); + continue; + } + + const installed = lockfileVersions(lock, group.package); + const affected = installed.filter((version) => + group.advisories.some((a) => + semver.satisfies(version, toSemverRange(a.range)), + ), + ); + if (affected.length === 0) { + console.log( + `dependabot-alerts: ${group.package}@${installed.join("/") || "(absent)"} is already out of range on ${TARGET_BRANCH} — skipping`, + ); + continue; + } + + const direct = isDirectDependency(lock, group.package); + const existing = existingIssues.find( + (i) => + i.marker?.package === group.package && + i.marker?.manifestPath === group.manifestPath, + ); + + if (!existing) { + const url = createIssue( + repo, + group, + buildIssueBody(group, { installed: affected, direct }), + ); + addToBoard(url); + continue; + } + + const { merged, added } = mergeGhsas(existing.marker.ghsas, group.ghsas); + if (added.length === 0) { + console.log( + `dependabot-alerts: #${existing.number} already covers ${group.package} — no-op`, + ); + continue; + } + + const edit = gh([ + "issue", + "edit", + String(existing.number), + "--repo", + repo, + "--body", + buildIssueBody(group, { installed: affected, direct, ghsas: merged }), + ]); + if (edit.status !== 0) { + throw new Error(`gh issue edit failed: ${(edit.stderr ?? "").trim()}`); + } + const comment = gh([ + "issue", + "comment", + String(existing.number), + "--repo", + repo, + "--body", + buildNewAdvisoryComment(group, added), + ]); + if (comment.status !== 0) { + throw new Error( + `gh issue comment failed: ${(comment.stderr ?? "").trim()}`, + ); + } + console.log( + `dependabot-alerts: added ${added.join(", ")} to #${existing.number}`, + ); + } +} + +if (import.meta.url === `file://${process.argv[1]}`) { + main(); +} diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs new file mode 100644 index 000000000..1bc203717 --- /dev/null +++ b/scripts/dependabot-alerts.test.mjs @@ -0,0 +1,265 @@ +// Unit tests for the pure halves of dependabot-alerts.mjs (#2233). The impure +// half (`main()`, which shells out to `gh`) is exercised only via +// `workflow_dispatch` in CI, per the same split `dependency-refresh.mjs` and +// `verify-skills.mjs` use. Run via `npm run test:scripts`. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + buildIssueBody, + buildIssueTitle, + buildMarker, + buildNewAdvisoryComment, + groupAlerts, + isDirectDependency, + lockfileVersions, + mergeGhsas, + parseMarker, + toSemverRange, +} from "./dependabot-alerts.mjs"; + +/** Shaped like a real `GET /repos/{o}/{r}/dependabot/alerts` entry. */ +function alert({ + ghsa, + pkg = "fast-uri", + manifest = "package-lock.json", + fixed = "3.1.6", + severity = "high", + range = ">= 3.0.0, < 3.1.6", + scope = "runtime", + cve = null, + state = "open", +}) { + return { + state, + html_url: `https://github.com/o/r/security/dependabot/${ghsa}`, + dependency: { + package: { name: pkg }, + manifest_path: manifest, + scope, + }, + security_advisory: { + ghsa_id: ghsa, + cve_id: cve, + severity, + summary: `${pkg} is bad`, + }, + security_vulnerability: { + vulnerable_version_range: range, + first_patched_version: { identifier: fixed }, + }, + }; +} + +test("toSemverRange turns GitHub's comma-separated conjuncts into semver ANDs", () => { + assert.equal(toSemverRange(">= 3.1.3, < 3.1.6"), ">= 3.1.3 < 3.1.6"); + assert.equal(toSemverRange("<= 4.28.6"), "<= 4.28.6"); + assert.equal(toSemverRange(">= 2.2.5, < 6.16.0"), ">= 2.2.5 < 6.16.0"); +}); + +test("toSemverRange tolerates stray whitespace and trailing commas", () => { + assert.equal(toSemverRange(" >= 1.0.0 , < 2.0.0 , "), ">= 1.0.0 < 2.0.0"); +}); + +test("lockfileVersions finds hoisted and nested copies, deduped and sorted", () => { + const lock = { + packages: { + "": { dependencies: { zod: "^3.0.0" } }, + "node_modules/fast-uri": { version: "3.1.5" }, + "node_modules/ajv/node_modules/fast-uri": { version: "3.0.1" }, + "node_modules/other/node_modules/fast-uri": { version: "3.1.5" }, + "node_modules/fast-uri-lookalike": { version: "9.9.9" }, + }, + }; + assert.deepEqual(lockfileVersions(lock, "fast-uri"), ["3.0.1", "3.1.5"]); +}); + +test("lockfileVersions returns [] when the package is absent", () => { + assert.deepEqual(lockfileVersions({ packages: {} }, "qs"), []); + assert.deepEqual(lockfileVersions({}, "qs"), []); +}); + +test("isDirectDependency reads the root manifest entry, not the tree", () => { + const lock = { + packages: { + "": { dependencies: { zod: "^4.0.0" }, devDependencies: { vitest: "1" } }, + "node_modules/fast-uri": { version: "3.1.5" }, + }, + }; + assert.equal(isDirectDependency(lock, "zod"), true); + assert.equal(isDirectDependency(lock, "vitest"), true); + assert.equal(isDirectDependency(lock, "fast-uri"), false); +}); + +test("groupAlerts collapses advisories into one entry per bump", () => { + const grouped = groupAlerts([ + alert({ ghsa: "GHSA-5jgf", range: ">= 3.1.3, < 3.1.6" }), + alert({ ghsa: "GHSA-f65p" }), + alert({ ghsa: "GHSA-jqff" }), + alert({ ghsa: "GHSA-fph4", range: ">= 3.1.2, < 3.1.6" }), + alert({ + ghsa: "GHSA-x5fp", + pkg: "qs", + fixed: "6.16.0", + severity: "medium", + range: ">= 6.14.2, <= 6.15.3", + }), + alert({ + ghsa: "GHSA-73wf", + pkg: "browserslist", + manifest: "clients/tui/package-lock.json", + fixed: "4.28.7", + scope: "development", + range: "<= 4.28.6", + }), + ]); + + assert.deepEqual( + grouped.map((g) => [g.package, g.manifestPath, g.fixedIn, g.ghsas.length]), + [ + ["browserslist", "clients/tui/package-lock.json", "4.28.7", 1], + ["fast-uri", "package-lock.json", "3.1.6", 4], + ["qs", "package-lock.json", "6.16.0", 1], + ], + ); + // GHSAs are sorted within a group so the marker is stable across runs. + assert.deepEqual(grouped[1].ghsas, [ + "GHSA-5jgf", + "GHSA-f65p", + "GHSA-fph4", + "GHSA-jqff", + ]); +}); + +test("groupAlerts keeps the highest severity across a group", () => { + const [group] = groupAlerts([ + alert({ ghsa: "GHSA-a", severity: "low" }), + alert({ ghsa: "GHSA-b", severity: "critical" }), + alert({ ghsa: "GHSA-c", severity: "medium" }), + ]); + assert.equal(group.severity, "critical"); +}); + +test("groupAlerts splits a package whose advisories need different bumps", () => { + const grouped = groupAlerts([ + alert({ ghsa: "GHSA-a", fixed: "3.1.6" }), + alert({ ghsa: "GHSA-b", fixed: "4.0.0" }), + ]); + assert.equal(grouped.length, 2); + assert.deepEqual( + grouped.map((g) => g.fixedIn), + ["3.1.6", "4.0.0"], + ); +}); + +test("groupAlerts drops closed alerts and ones with no patched version", () => { + const unpatched = alert({ ghsa: "GHSA-x" }); + unpatched.security_vulnerability.first_patched_version = null; + assert.deepEqual( + groupAlerts([alert({ ghsa: "GHSA-y", state: "fixed" }), unpatched]), + [], + ); +}); + +test("buildMarker and parseMarker round-trip, sorting the GHSA list", () => { + const marker = buildMarker({ + package: "fast-uri", + manifestPath: "package-lock.json", + ghsas: ["GHSA-b", "GHSA-a"], + }); + assert.equal( + marker, + "", + ); + assert.deepEqual(parseMarker(`${marker}\nbody text`), { + package: "fast-uri", + manifestPath: "package-lock.json", + ghsas: ["GHSA-a", "GHSA-b"], + }); +}); + +test("parseMarker returns null for an unmarked or absent body", () => { + assert.equal(parseMarker(undefined), null); + assert.equal(parseMarker("just an issue someone wrote"), null); + // The marker is the FIRST line or it is not the idempotency key. + assert.equal( + parseMarker( + "preamble\n", + ), + null, + ); +}); + +test("mergeGhsas reports only the advisories the issue does not already name", () => { + assert.deepEqual(mergeGhsas(["GHSA-a", "GHSA-b"], ["GHSA-b", "GHSA-c"]), { + merged: ["GHSA-a", "GHSA-b", "GHSA-c"], + added: ["GHSA-c"], + }); +}); + +test("mergeGhsas reports nothing added when the issue already covers them", () => { + assert.deepEqual(mergeGhsas(["GHSA-a", "GHSA-b"], ["GHSA-a"]), { + merged: ["GHSA-a", "GHSA-b"], + added: [], + }); +}); + +test("buildIssueTitle names the bump and pluralizes the advisory count", () => { + const [many] = groupAlerts([ + alert({ ghsa: "GHSA-a" }), + alert({ ghsa: "GHSA-b" }), + ]); + assert.equal( + buildIssueTitle(many), + "chore(deps): bump `fast-uri` to `3.1.6` in `package-lock.json` (2 advisories)", + ); + const [one] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + assert.equal( + buildIssueTitle(one), + "chore(deps): bump `fast-uri` to `3.1.6` in `package-lock.json` (1 advisory)", + ); +}); + +test("buildIssueBody leads with the marker and asks for an overrides pin when transitive", () => { + const [group] = groupAlerts([ + alert({ ghsa: "GHSA-a", cve: "CVE-2026-1" }), + alert({ ghsa: "GHSA-b" }), + ]); + const body = buildIssueBody(group, { installed: ["3.1.5"], direct: false }); + + assert.ok(body.startsWith(buildMarker(group))); + assert.match(body, /\| Installed on `v2\/main` \| `3\.1\.5` \|/); + assert.match(body, /\| Fixed in \| `3\.1\.6` \|/); + assert.match(body, /GHSA-a/); + assert.match(body, /CVE-2026-1/); + assert.match(body, /`overrides`/); + assert.doesNotMatch(body, /bump its declared range/); +}); + +test("buildIssueBody asks for a plain range bump when the dependency is direct", () => { + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const body = buildIssueBody(group, { installed: ["3.1.5"], direct: true }); + assert.match(body, /\*\*direct\*\* dependency of `package\.json`/); + assert.match(body, /bump its declared range to `>=3\.1\.6`/); +}); + +test("buildIssueBody honors an overridden GHSA list when rewriting an issue", () => { + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const body = buildIssueBody(group, { + installed: ["3.1.5"], + direct: false, + ghsas: ["GHSA-a", "GHSA-old"], + }); + assert.deepEqual(parseMarker(body).ghsas, ["GHSA-a", "GHSA-old"]); +}); + +test("buildNewAdvisoryComment lists only the newly-seen advisories", () => { + const [group] = groupAlerts([ + alert({ ghsa: "GHSA-a" }), + alert({ ghsa: "GHSA-b" }), + ]); + const comment = buildNewAdvisoryComment(group, ["GHSA-b"]); + assert.match(comment, /1 new Dependabot advisory/); + assert.match(comment, /GHSA-b/); + assert.doesNotMatch(comment, /GHSA-a/); +}); diff --git a/scripts/dependency-refresh.mjs b/scripts/dependency-refresh.mjs index be87fba4e..4ce6799e2 100644 --- a/scripts/dependency-refresh.mjs +++ b/scripts/dependency-refresh.mjs @@ -3,12 +3,13 @@ // // A Dependabot version-update PR carries no issue and no board card — the same // carve-out from "every PR references an issue" that the security-update flow -// had (that half is handled separately by the alert-driven pipeline, also -// #2229). #2235 removed `.github/dependabot.yml` outright, so Dependabot opens +// had (that half is `dependabot-alerts.mjs`, #2233, which turned those PRs off +// as well and files an issue per bump from the alerts they leave behind — so +// between them Dependabot opens no PRs against this repo at all, though its +// security ALERTS stay on, since that sweep is what consumes them). +// #2235 removed `.github/dependabot.yml` outright, so Dependabot opens // no version-update PRs against this repo at all and this script is what -// replaced them. Dependabot SECURITY updates are a separate mechanism, enabled -// in repo settings rather than in that file, and are deliberately still on — -// so this replaces the version-update half only, not Dependabot wholesale. +// replaced them. // // Once a month it runs `npm outdated` across the root install and every client // under `clients/*` (each has its own package.json + lockfile — v2 is not a From b53266bc724e1bde878c95d5d224581ee89bf1e1 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 3 Sep 2026 23:56:18 -0400 Subject: [PATCH 02/14] chore(deps): address Copilot review round 1 on #2243 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `--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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- .github/workflows/dependabot-alerts.yml | 14 +- AGENTS.md | 4 +- scripts/dependabot-alerts.mjs | 352 +++++++++++++------ scripts/dependabot-alerts.test.mjs | 440 +++++++++++++++++++++++- scripts/dependency-refresh.mjs | 2 +- 5 files changed, 695 insertions(+), 117 deletions(-) diff --git a/.github/workflows/dependabot-alerts.yml b/.github/workflows/dependabot-alerts.yml index 72bd99d31..4e1f3f388 100644 --- a/.github/workflows/dependabot-alerts.yml +++ b/.github/workflows/dependabot-alerts.yml @@ -12,8 +12,11 @@ # security PRs off there is no mergeable-against-`main` artifact and no window to # race, so the merge guard #2060 needed has no analogue here. What replaces it is # a precondition inside the script: `automated-security-fixes` is a repo SETTING -# and can be switched back on from the UI without a commit, so the sweep asserts -# it is still disabled and fails loudly if it is not. +# and can be switched back on from the UI without a commit, so the sweep reads it +# back and fails loudly on an explicit `enabled: true`. That read needs a +# permission `GITHUB_TOKEN` cannot hold, so with the default token it reports +# UNVERIFIED instead — see the token notes below; it is a conditional guard, not +# an invariant. # # ⚠️ GitHub computes the dependency graph, and therefore every alert, from the # DEFAULT branch (`main`), while we ship from `v2/main`. Two consequences: @@ -24,9 +27,10 @@ # waiting on a milestone merge to close, so it is skipped silently. # * A vulnerable dependency introduced on `v2/main` and not yet merged to # `main` produces NO alert at all. No approach that consumes GitHub's alerts -# avoids that. The release-time `npm audit fix` from #2231 is a second signal -# that partially covers it; a scheduled `npm audit --audit-level=high` over -# `v2/main`'s lockfiles would close it fully and is a separable follow-up. +# avoids that. The release-time `npm audit --audit-level=high` report from +# #2231 is a second signal that partially covers it — at release time only; +# running that same report over `v2/main`'s lockfiles on a schedule would +# close it fully and is a separable follow-up. # # `vulnerability-alerts: read` is the one non-default permission, and # `GITHUB_TOKEN` supports it — no PAT is needed to read the alerts themselves. diff --git a/AGENTS.md b/AGENTS.md index a5502051b..ba08444fe 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -111,8 +111,8 @@ Four things about this that are not obvious from the code: - **Dependabot *alerts* stay on.** Alerts and security-update PRs are independent settings; only the PRs are off. Turning alerts off would blind the sweep that replaced them. - **The security half is a schedule, not an event handler**, because there is no `dependabot_alert` workflow trigger — it is a webhook event only. -- **Alerts are computed from the default branch (`main`), and we ship from `v2/main`.** So the sweep re-checks each alert's vulnerable range against `v2/main`'s own lockfile before filing, and skips one that is already fixed there. The converse is a real blind spot with no fix on this path: a vulnerable dependency introduced on `v2/main` and not yet merged to `main` produces **no alert at all**. The release-time `npm audit fix` (#2231) is the partial second signal. -- **`automated-security-fixes` can be re-enabled from the UI without a commit**, so nothing in the repo would record it. The sweep asserts it is still disabled and **fails loudly** if it is not; a red run of that workflow means the setting was flipped, not that the script broke. +- **Alerts are computed from the default branch (`main`), and we ship from `v2/main`.** So the sweep re-checks each alert's vulnerable range against `v2/main`'s own lockfile before filing, and skips one that is already fixed there. The converse is a real blind spot with no fix on this path: a vulnerable dependency introduced on `v2/main` and not yet merged to `main` produces **no alert at all**. The release-time `npm audit --audit-level=high` report (#2231) is the partial second signal — and only at release time. +- **`automated-security-fixes` can be re-enabled from the UI without a commit**, so nothing in the repo would record it. The sweep reads it back and **fails loudly on an explicit `enabled: true`**. ⚠️ It is a *conditional* guard, not an invariant: the endpoint needs `administration: read`, which `GITHUB_TOKEN` cannot be granted (`permissions:` has no such key), so under the default token the sweep logs **UNVERIFIED** and carries on rather than going red every day for an unrelated reason. Only a token carrying that scope makes it a real assertion. An issue filed by either sweep is an ordinary board item — `v2` + `chore` + `dependabot`, the current milestone, and a card on #28. A security issue lands at **Todo / High**: arriving through this pipeline *is* the approval, and `High` is a standing override of the [priority rubric](.claude/skills/issue-triage/SKILL.md), which would otherwise score a routine bump Medium. Board placement needs an org-project PAT that `GITHUB_TOKEN` cannot have, so it is **best-effort** — without the secret the issue is still created labeled and milestoned, and the next triage sweep boards it. diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index d56f5f654..8ca81e323 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -20,11 +20,12 @@ // alerts are not minute-sensitive. // 2. `GITHUB_TOKEN` can read alerts with `vulnerability-alerts: read`, so no // PAT is needed for the sweep itself. Two side steps DO need one, and both -// are best-effort rather than preconditions: writing the board card (an org -// project is outside `GITHUB_TOKEN`'s reach — an unboarded-but-milestoned -// issue is swept into Todo by the next `/issue-triage` pass), and reading -// back the `automated-security-fixes` setting (`administration: read`, -// which `permissions:` cannot grant at all). +// degrade rather than block: writing the board card (an org project is +// outside `GITHUB_TOKEN`'s reach — an unboarded-but-milestoned issue is +// swept into Todo by the next `/issue-triage` pass), and reading back the +// `automated-security-fixes` setting (`administration: read`, which +// `permissions:` cannot grant at all — so that guard reports UNVERIFIED +// rather than failing when the token cannot see it). // 3. Alerts are per-ADVISORY but a fix is per-BUMP. Today's seven open alerts // are three `overrides` entries, so grouping by // `(package, manifest_path, first_patched_version)` is what keeps this from @@ -36,7 +37,8 @@ // own lockfile before anything is filed. The blind spot that leaves is stated // plainly in the workflow header: a vulnerable dependency introduced on // `v2/main` and not yet merged to `main` produces no alert at all, and no -// approach that consumes GitHub's alerts can see it. +// approach that consumes GitHub's alerts can see it. The release-time +// `npm audit --audit-level=high` report (#2231) is the partial second signal. // // Idempotency key is the marker comment at the top of each issue body, which // names the package, the manifest and every GHSA the issue covers. A second run @@ -44,12 +46,12 @@ // has an open issue lands as a comment on it and rewrites the marker, rather // than filing a second issue. // -// The pure halves — `toSemverRange`, `lockfileVersions`, `isDirectDependency`, -// `groupAlerts`, `buildMarker`, `parseMarker`, `mergeGhsas`, `buildIssueTitle`, -// `buildIssueBody` and `buildNewAdvisoryComment` — are covered by -// `dependabot-alerts.test.mjs`. `main()` is the CLI entry point, exercised -// against the real repo only via `workflow_dispatch` in CI, per the same split -// `dependency-refresh.mjs` and `verify-skills.mjs` already use. +// Everything here is covered by `dependabot-alerts.test.mjs`: the pure halves +// directly, and `main()` through an injected spawn function, the same way +// `dependency-refresh.mjs` does it. `workflow_dispatch` is a production +// trigger, not a test, so the orchestration that handles API failures, lockfile +// filtering, issue idempotency and partial board writes is exercised here +// rather than left to a real run (Copilot). import { spawnSync } from "node:child_process"; import { readFileSync } from "node:fs"; @@ -78,23 +80,32 @@ export const BOARD_PRIORITY = "High"; export const TARGET_BRANCH = "v2/main"; const MARKER_RE = - /^/; + /^/; + +/** Marker on the comment that announces newly-seen advisories, keyed by GHSA. */ +const COMMENT_MARKER_RE = /^/; /** * The issue body's first line: the idempotency key. * - * @param {{package: string, manifestPath: string, ghsas: string[]}} group + * It carries `fixedIn` as well as the package and manifest because that triple + * IS the grouping key — two advisories on one package needing different patched + * versions are different bumps and get different issues. Keyed on the pair + * alone, a second bump would match the first issue and merge its GHSAs and its + * target version into the wrong one (Copilot). + * + * @param {{package: string, manifestPath: string, fixedIn: string, ghsas: string[]}} group * @returns {string} */ -export function buildMarker({ package: pkg, manifestPath, ghsas }) { - return ``; +export function buildMarker({ package: pkg, manifestPath, fixedIn, ghsas }) { + return ``; } /** * Read a marker back off an issue body. * * @param {string | undefined} body - * @returns {{package: string, manifestPath: string, ghsas: string[]} | null} + * @returns {{package: string, manifestPath: string, fixedIn: string, ghsas: string[]} | null} */ export function parseMarker(body) { const match = MARKER_RE.exec(body ?? ""); @@ -102,10 +113,23 @@ export function parseMarker(body) { return { package: match[1], manifestPath: match[2], - ghsas: match[3].split(",").filter(Boolean), + fixedIn: match[3], + ghsas: match[4].split(",").filter(Boolean), }; } +/** + * The GHSAs a previously-posted "new advisories" comment already announced. + * + * @param {string | undefined} body + * @returns {string[] | null} `null` when the comment carries no marker + */ +export function parseCommentMarker(body) { + const match = COMMENT_MARKER_RE.exec(body ?? ""); + if (!match) return null; + return match[1].split(",").filter(Boolean); +} + /** * Which of `group`'s advisories the existing issue does not already name. * @@ -284,7 +308,7 @@ export function buildIssueBody(group, { installed, direct, ghsas }) { .join("\n"); const fix = direct - ? `\`${group.package}\` is a **direct** dependency of \`${group.manifestPath.replace(/package-lock\.json$/, "package.json")}\` — bump its declared range to \`>=${group.fixedIn}\`.` + ? `\`${group.package}\` is a **direct** dependency of \`${group.manifestPath.replace(/package-lock\.json$/, "package.json")}\` — raise its declared range so it can no longer resolve below \`${group.fixedIn}\`, keeping the operator the manifest already uses. Widening it to a bare \`>=\` would drop the compatibility bound with it (Copilot).` : `\`${group.package}\` is **transitive**, so the fix is an [\`overrides\`](${PLACEMENT_DOC}) entry pinning it to \`${group.fixedIn}\` — **not** \`npm audit fix\`, which "resolves" an advisory with no upward escape by silently downgrading.`; return [ @@ -315,6 +339,16 @@ export function buildIssueBody(group, { installed, direct, ghsas }) { ].join("\n"); } +/** + * The marker that makes a "new advisories" comment idempotent on its own. + * + * @param {string[]} added + * @returns {string} + */ +export function buildCommentMarker(added) { + return ``; +} + /** * The comment a NEW advisory for an already-open issue gets, instead of a * second issue. @@ -332,6 +366,7 @@ export function buildNewAdvisoryComment(group, added) { ) .join("\n"); return [ + buildCommentMarker(added), `${added.length} new Dependabot ${added.length === 1 ? "advisory" : "advisories"} for \`${group.package}\`, cleared by the same bump to \`${group.fixedIn}\`. The issue body's marker now covers ${added.length === 1 ? "it" : "them"} too.`, "", "| GHSA | Severity | Summary |", @@ -341,24 +376,43 @@ export function buildNewAdvisoryComment(group, added) { } // --------------------------------------------------------------------------- -// Impure half: everything below shells out to `gh` or `git`. +// Impure half: everything below shells out to `gh`. Each takes its spawn +// function as a parameter, defaulted to `spawnSync`, so `main()` is testable +// with an injected fake rather than left to `workflow_dispatch` — the same +// shape `dependency-refresh.mjs` uses. // --------------------------------------------------------------------------- -function gh(args, { token } = {}) { +function gh(spawn, args, { token } = {}) { const env = token ? { ...process.env, GH_TOKEN: token } : process.env; - const result = spawnSync("gh", args, { encoding: "utf8", env }); + const result = spawn("gh", args, { encoding: "utf8", env }); if (result.error) throw result.error; return result; } -function ghJson(args) { - const result = gh(args); +function ghJson(spawn, args) { + const result = gh(spawn, args); if (result.status !== 0) { throw new Error(`gh ${args[0]} failed: ${(result.stderr ?? "").trim()}`); } return JSON.parse(result.stdout || "null"); } +/** + * Is this failed lookup the "the token may not read this" answer, rather than a + * real API failure? + * + * The distinction is what keeps the security-PR guard honest: a bad token, a + * rate limit or a transient 5xx must NOT be waved through as "unverified", or + * the sweep exits green having silently skipped its own precondition + * (Copilot). Only an authorization-shaped status is tolerated. + * + * @param {string} stderr stderr from a non-zero `gh api` call + * @returns {boolean} + */ +export function isPermissionDenied(stderr) { + return /HTTP (401|403|404)\b/.test(stderr); +} + /** * Detect whether Dependabot's security-update PRs have been switched back on. * @@ -370,22 +424,29 @@ function ghJson(args) { * ⚠️ **The endpoint needs `administration: read`, which `GITHUB_TOKEN` cannot * be granted** — `permissions:` has no such key. So with the default token the * call 403s, and treating that as failure would make every scheduled run red - * for a reason unrelated to the alerts. It is therefore reported and skipped: - * only an explicit `enabled: true` throws. Give `PROJECT_TOKEN` the extra - * `administration: read` scope and the guard becomes a real assertion; without - * it the sweep still does its job, it just cannot see that setting. + * for a reason unrelated to the alerts. An authorization-shaped failure is + * therefore reported and skipped; every other failure throws, and an explicit + * `enabled: true` throws. Give `PROJECT_TOKEN` the extra `administration: read` + * scope and the guard becomes a real assertion; without it the sweep still does + * its job, it just cannot see that setting. + * + * @returns {boolean} whether the setting was actually read */ -function checkSecurityPrsStillDisabled(repo) { - const result = gh(["api", `repos/${repo}/automated-security-fixes`], { +function checkSecurityPrsStillDisabled(repo, spawn) { + const result = gh(spawn, ["api", `repos/${repo}/automated-security-fixes`], { token: process.env.PROJECT_TOKEN, }); if (result.status !== 0) { + const stderr = (result.stderr ?? "").trim(); + if (!isPermissionDenied(stderr)) { + throw new Error(`automated-security-fixes lookup failed: ${stderr}`); + } console.log( - "dependabot-alerts: cannot read automated-security-fixes " + - `(${(result.stderr ?? "").trim()}) — the token lacks \`administration: read\`, ` + - "so whether Dependabot security PRs are still off is UNVERIFIED this run", + `dependabot-alerts: cannot read automated-security-fixes (${stderr}) — ` + + "the token lacks `administration: read`, so whether Dependabot security " + + "PRs are still off is UNVERIFIED this run", ); - return; + return false; } const state = JSON.parse(result.stdout || "{}"); if (state.enabled === true) { @@ -396,14 +457,25 @@ function checkSecurityPrsStillDisabled(repo) { `DELETE /repos/${repo}/automated-security-fixes) and re-run.`, ); } + return true; } -function openAlerts(repo) { - return ghJson([ +/** + * Every open Dependabot alert. + * + * ⚠️ `--slurp` is load-bearing. Without it `gh api --paginate` concatenates one + * top-level JSON array PER PAGE, which `JSON.parse` rejects outright the moment + * open alerts exceed the 100-per-page limit (Copilot). With it the pages arrive + * as an array of arrays, flattened here. + */ +function openAlerts(repo, spawn) { + const pages = ghJson(spawn, [ "api", "--paginate", + "--slurp", `repos/${repo}/dependabot/alerts?state=open&per_page=100`, ]); + return (pages ?? []).flat(); } /** @@ -419,25 +491,43 @@ function readManifest(manifestPath) { } } -function openDependabotIssues(repo) { - return ghJson([ +function openDependabotIssues(repo, spawn) { + return ( + ghJson(spawn, [ + "issue", + "list", + "--repo", + repo, + "--state", + "open", + "--label", + "dependabot", + "--json", + "number,body", + "--limit", + "100", + ]) ?? [] + ); +} + +/** The GHSA sets already announced by comments on an issue. */ +function announcedAdvisories(repo, number, spawn) { + const issue = ghJson(spawn, [ "issue", - "list", + "view", + String(number), "--repo", repo, - "--state", - "open", - "--label", - "dependabot", "--json", - "number,body", - "--limit", - "100", + "comments", ]); + return (issue?.comments ?? []) + .map((comment) => parseCommentMarker(comment.body)) + .filter(Boolean); } -function currentMilestone(repo) { - const result = gh([ +function currentMilestone(repo, spawn) { + const result = gh(spawn, [ "api", `repos/${repo}/milestones`, "--jq", @@ -456,8 +546,9 @@ function currentMilestone(repo) { * looking them up each run is what keeps an unrelated board edit from turning * into a silently mis-set field here. */ -function optionId(fieldName, optionName, token) { +function optionId(fieldName, optionName, token, spawn) { const result = gh( + spawn, [ "project", "field-list", @@ -487,24 +578,36 @@ function optionId(fieldName, optionName, token) { /** * Put the issue on board #28 at Todo / High. * - * Best-effort by design: an org project is outside `GITHUB_TOKEN`'s reach, so - * this needs a PAT the workflow may not have. A failure here is logged and the - * run continues — the issue is already labeled and milestoned, which is enough - * for the next `/issue-triage` sweep to board it (its documented exception - * moves an unboarded-but-milestoned issue straight into Todo). + * Failing to add the card at all is benign — the issue is already labeled and + * milestoned, which is enough for the next `/issue-triage` sweep to board it + * (its documented exception moves an unboarded-but-milestoned issue straight + * into Todo). That is why an org-project PAT is an optimization here rather + * than a prerequisite. + * + * ⚠️ **Failing PART WAY through is not benign**, and the two cases must not be + * reported the same way (Copilot). Once `item-add` succeeds the issue IS + * boarded, so no later triage sweep will look at it — a failed field edit + * leaves a card sitting on the board with no Status or no Priority, in exactly + * the state nothing else will fix. So a partial placement is returned to the + * caller, which finishes every remaining group and then fails the run. * * Todo rather than Incoming: arriving through this pipeline IS the approval. + * + * @returns {string | null} a description of a PARTIAL placement, else `null` */ -function addToBoard(issueUrl) { +function addToBoard(issueUrl, spawn) { const token = process.env.PROJECT_TOKEN; if (!token) { console.log( "dependabot-alerts: PROJECT_TOKEN unset — issue left unboarded for the next triage sweep", ); - return; + return null; } + + let itemId; try { const added = gh( + spawn, [ "project", "item-add", @@ -518,16 +621,24 @@ function addToBoard(issueUrl) { ], { token }, ); - if (added.status !== 0) { - throw new Error((added.stderr ?? "").trim()); - } - const itemId = JSON.parse(added.stdout).id; - // Each item-edit sets exactly one field, so Status and Priority are two calls. - for (const [fieldId, fieldName, optionName] of [ - [STATUS_FIELD_ID, "Status", BOARD_STATUS], - [PRIORITY_FIELD_ID, "Priority", BOARD_PRIORITY], - ]) { + if (added.status !== 0) throw new Error((added.stderr ?? "").trim()); + itemId = JSON.parse(added.stdout).id; + } catch (error) { + // Nothing was added, so the issue is simply unboarded — recoverable. + console.log( + `dependabot-alerts: board add failed (${error.message}) — issue is labeled and milestoned, next triage sweep will board it`, + ); + return null; + } + + // Each item-edit sets exactly one field, so Status and Priority are two calls. + for (const [fieldId, fieldName, optionName] of [ + [STATUS_FIELD_ID, "Status", BOARD_STATUS], + [PRIORITY_FIELD_ID, "Priority", BOARD_PRIORITY], + ]) { + try { const edit = gh( + spawn, [ "project", "item-edit", @@ -538,24 +649,27 @@ function addToBoard(issueUrl) { "--field-id", fieldId, "--single-select-option-id", - optionId(fieldName, optionName, token), + optionId(fieldName, optionName, token, spawn), ], { token }, ); if (edit.status !== 0) throw new Error((edit.stderr ?? "").trim()); + } catch (error) { + return `${issueUrl} is on board #28 but its ${fieldName} was not set (${error.message}) — no triage sweep will fix this, set it by hand`; } - console.log( - `dependabot-alerts: boarded ${issueUrl} at ${BOARD_STATUS}/${BOARD_PRIORITY}`, - ); - } catch (error) { - console.log( - `dependabot-alerts: board write failed (${error.message}) — issue is labeled and milestoned, next triage sweep will board it`, - ); } + + console.log( + `dependabot-alerts: boarded ${issueUrl} at ${BOARD_STATUS}/${BOARD_PRIORITY}`, + ); + return null; } -function createIssue(repo, group, body) { - const milestone = currentMilestone(repo); +/** + * @returns {{url: string, milestone: string | null}} + */ +function createIssue(repo, group, body, spawn) { + const milestone = currentMilestone(repo, spawn); const args = [ "issue", "create", @@ -573,37 +687,34 @@ function createIssue(repo, group, body) { body, ]; if (milestone) args.push("--milestone", milestone); - const result = gh(args); + const result = gh(spawn, args); if (result.status !== 0) { throw new Error(`gh issue create failed: ${(result.stderr ?? "").trim()}`); } const url = result.stdout.trim(); - if (!milestone) { - console.log( - "dependabot-alerts: no open milestone — issue filed unmilestoned", - ); - } console.log(`dependabot-alerts: filed ${url}`); - return url; + return { url, milestone }; } -export function main(repo = process.env.GITHUB_REPOSITORY) { +export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { if (!repo) throw new Error("repo not specified (GITHUB_REPOSITORY unset)"); - checkSecurityPrsStillDisabled(repo); + checkSecurityPrsStillDisabled(repo, spawn); - const groups = groupAlerts(openAlerts(repo)); + const groups = groupAlerts(openAlerts(repo, spawn)); if (groups.length === 0) { console.log("dependabot-alerts: no open alerts — no-op"); return; } - const existingIssues = openDependabotIssues(repo).map((issue) => ({ + const existingIssues = openDependabotIssues(repo, spawn).map((issue) => ({ ...issue, marker: parseMarker(issue.body), })); const manifests = new Map(); + const boardProblems = []; + for (const group of groups) { if (!manifests.has(group.manifestPath)) { manifests.set(group.manifestPath, readManifest(group.manifestPath)); @@ -630,19 +741,34 @@ export function main(repo = process.env.GITHUB_REPOSITORY) { } const direct = isDirectDependency(lock, group.package); + // Matched on the full grouping key, `fixedIn` included: a second bump of + // the same package is a different issue, not an update to this one. const existing = existingIssues.find( (i) => i.marker?.package === group.package && - i.marker?.manifestPath === group.manifestPath, + i.marker?.manifestPath === group.manifestPath && + i.marker?.fixedIn === group.fixedIn, ); if (!existing) { - const url = createIssue( + const { url, milestone } = createIssue( repo, group, buildIssueBody(group, { installed: affected, direct }), + spawn, ); - addToBoard(url); + // `Incoming` <=> no milestone, everything past it <=> milestoned. With no + // open milestone to assign there is nothing to put the card past Incoming + // WITH, so boarding it at Todo would assert an approval the invariant + // reads off the milestone (Copilot). Leave it for triage instead. + if (!milestone) { + console.log( + "dependabot-alerts: no open milestone — issue filed unmilestoned and unboarded, next triage sweep places it", + ); + continue; + } + const problem = addToBoard(url, spawn); + if (problem) boardProblems.push(problem); continue; } @@ -654,7 +780,31 @@ export function main(repo = process.env.GITHUB_REPOSITORY) { continue; } - const edit = gh([ + // ⚠️ Comment FIRST, then rewrite the marker. The marker is the idempotency + // key, so editing it first and failing on the comment would make the next + // run take the no-op branch above and skip the comment permanently + // (Copilot). In this order the worst case is a repeat, and the comment's + // own marker rules that out too. + const alreadyAnnounced = announcedAdvisories(repo, existing.number, spawn); + const marker = buildCommentMarker(added); + if (!alreadyAnnounced.some((set) => buildCommentMarker(set) === marker)) { + const comment = gh(spawn, [ + "issue", + "comment", + String(existing.number), + "--repo", + repo, + "--body", + buildNewAdvisoryComment(group, added), + ]); + if (comment.status !== 0) { + throw new Error( + `gh issue comment failed: ${(comment.stderr ?? "").trim()}`, + ); + } + } + + const edit = gh(spawn, [ "issue", "edit", String(existing.number), @@ -666,24 +816,18 @@ export function main(repo = process.env.GITHUB_REPOSITORY) { if (edit.status !== 0) { throw new Error(`gh issue edit failed: ${(edit.stderr ?? "").trim()}`); } - const comment = gh([ - "issue", - "comment", - String(existing.number), - "--repo", - repo, - "--body", - buildNewAdvisoryComment(group, added), - ]); - if (comment.status !== 0) { - throw new Error( - `gh issue comment failed: ${(comment.stderr ?? "").trim()}`, - ); - } console.log( `dependabot-alerts: added ${added.join(", ")} to #${existing.number}`, ); } + + // Every group is processed before this throws: a half-placed card is worth + // failing the run over, but not at the cost of the issues still unfiled. + if (boardProblems.length > 0) { + throw new Error( + `dependabot-alerts: incomplete board placement —\n ${boardProblems.join("\n ")}`, + ); + } } if (import.meta.url === `file://${process.argv[1]}`) { diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index 1bc203717..a37faf6a8 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -5,15 +5,22 @@ import { test } from "node:test"; import assert from "node:assert/strict"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { tmpdir } from "node:os"; import { + buildCommentMarker, buildIssueBody, buildIssueTitle, buildMarker, buildNewAdvisoryComment, groupAlerts, isDirectDependency, + isPermissionDenied, lockfileVersions, + main, mergeGhsas, + parseCommentMarker, parseMarker, toSemverRange, } from "./dependabot-alerts.mjs"; @@ -165,26 +172,57 @@ test("buildMarker and parseMarker round-trip, sorting the GHSA list", () => { const marker = buildMarker({ package: "fast-uri", manifestPath: "package-lock.json", + fixedIn: "3.1.6", ghsas: ["GHSA-b", "GHSA-a"], }); assert.equal( marker, - "", + "", ); assert.deepEqual(parseMarker(`${marker}\nbody text`), { package: "fast-uri", manifestPath: "package-lock.json", + fixedIn: "3.1.6", ghsas: ["GHSA-a", "GHSA-b"], }); }); +test("the marker carries fixedIn, so two bumps of one package stay distinct", () => { + const [a] = groupAlerts([alert({ ghsa: "GHSA-a", fixed: "3.1.6" })]); + const [b] = groupAlerts([alert({ ghsa: "GHSA-b", fixed: "4.0.0" })]); + assert.notEqual(buildMarker(a), buildMarker(b)); + assert.equal(parseMarker(buildMarker(a)).fixedIn, "3.1.6"); + assert.equal(parseMarker(buildMarker(b)).fixedIn, "4.0.0"); +}); + +test("buildCommentMarker and parseCommentMarker round-trip", () => { + const marker = buildCommentMarker(["GHSA-b", "GHSA-a"]); + assert.equal(marker, ""); + assert.deepEqual(parseCommentMarker(`${marker}\ntext`), ["GHSA-a", "GHSA-b"]); + assert.equal(parseCommentMarker("an ordinary comment"), null); +}); + +test("isPermissionDenied separates an authorization failure from a real one", () => { + assert.equal( + isPermissionDenied("gh: HTTP 403: Resource not accessible"), + true, + ); + assert.equal(isPermissionDenied("gh: HTTP 401: Bad credentials"), true); + assert.equal(isPermissionDenied("gh: HTTP 404: Not Found"), true); + assert.equal( + isPermissionDenied("gh: HTTP 500: Internal Server Error"), + false, + ); + assert.equal(isPermissionDenied("gh: API rate limit exceeded"), false); +}); + test("parseMarker returns null for an unmarked or absent body", () => { assert.equal(parseMarker(undefined), null); assert.equal(parseMarker("just an issue someone wrote"), null); // The marker is the FIRST line or it is not the idempotency key. assert.equal( parseMarker( - "preamble\n", + "preamble\n", ), null, ); @@ -233,14 +271,16 @@ test("buildIssueBody leads with the marker and asks for an overrides pin when tr assert.match(body, /GHSA-a/); assert.match(body, /CVE-2026-1/); assert.match(body, /`overrides`/); - assert.doesNotMatch(body, /bump its declared range/); + assert.doesNotMatch(body, /raise its declared range/); }); -test("buildIssueBody asks for a plain range bump when the dependency is direct", () => { +test("buildIssueBody asks a direct dependency's range to be raised, not widened", () => { const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); const body = buildIssueBody(group, { installed: ["3.1.5"], direct: true }); assert.match(body, /\*\*direct\*\* dependency of `package\.json`/); - assert.match(body, /bump its declared range to `>=3\.1\.6`/); + assert.match(body, /can no longer resolve below `3\.1\.6`/); + // Prescribing `>=3.1.6` would throw away the manifest's compatibility bound. + assert.doesNotMatch(body, /range to `>=/); }); test("buildIssueBody honors an overridden GHSA list when rewriting an issue", () => { @@ -263,3 +303,393 @@ test("buildNewAdvisoryComment lists only the newly-seen advisories", () => { assert.match(comment, /GHSA-b/); assert.doesNotMatch(comment, /GHSA-a/); }); + +// --------------------------------------------------------------------------- +// main() orchestration, driven through the injected spawn function. +// +// `workflow_dispatch` is a production trigger, not a test — and everything that +// can go wrong here goes wrong in production only: a paginated alert feed, a +// manifest that has moved on, a half-written board card, a comment posted twice +// (Copilot). Manifests are read from the working tree, so each test runs in a +// temp directory it populates itself. +// --------------------------------------------------------------------------- + +/** + * A `spawnSync` stand-in. `gh` responses are matched on the argument list, in + * the order the handlers are declared, and every call is recorded. + */ +function fakeSpawn({ + securityFixes = { enabled: false, paused: false }, + securityFixesStatus = 0, + securityFixesStderr = "", + alertPages = [[]], + issues = [], + comments = [], + milestone = "v2.6.0", + boardEditStatus = 0, +} = {}) { + const calls = []; + const ok = (stdout = "") => ({ status: 0, stdout, stderr: "" }); + + const spawn = (cmd, args, opts) => { + calls.push({ cmd, args, opts }); + const joined = args.join(" "); + + if (joined.includes("automated-security-fixes")) { + return securityFixesStatus === 0 + ? ok(JSON.stringify(securityFixes)) + : { + status: securityFixesStatus, + stdout: "", + stderr: securityFixesStderr, + }; + } + if (joined.includes("dependabot/alerts")) { + // `--slurp` yields one array PER PAGE; main() must flatten them. + return ok(JSON.stringify(alertPages)); + } + if (joined.includes("milestones")) return ok(milestone); + if (args[0] === "issue" && args[1] === "list") + return ok(JSON.stringify(issues)); + if (args[0] === "issue" && args[1] === "view") + return ok(JSON.stringify({ comments })); + if (args[0] === "issue" && args[1] === "create") + return ok("https://github.com/o/r/issues/77"); + if (args[0] === "project" && args[1] === "item-add") + return ok(JSON.stringify({ id: "PVTI_fake" })); + if (args[0] === "project" && args[1] === "field-list") + return ok( + JSON.stringify({ + fields: [ + { name: "Status", options: [{ name: "Todo", id: "todo-id" }] }, + { name: "Priority", options: [{ name: "High", id: "high-id" }] }, + ], + }), + ); + if (args[0] === "project" && args[1] === "item-edit") + return boardEditStatus === 0 + ? ok() + : { status: boardEditStatus, stdout: "", stderr: "field edit blew up" }; + return ok(); + }; + spawn.calls = calls; + return spawn; +} + +const ghCall = (spawn, verb) => + spawn.calls.find( + (c) => c.cmd === "gh" && c.args[0] === "issue" && c.args[1] === verb, + ); +const ghCalls = (spawn, verb) => + spawn.calls.filter( + (c) => c.cmd === "gh" && c.args[0] === "issue" && c.args[1] === verb, + ); + +function captureLog(run) { + const lines = []; + const original = console.log; + console.log = (...a) => lines.push(a.join(" ")); + try { + run(); + } finally { + console.log = original; + } + return lines; +} + +/** Run `body` in a temp cwd populated with `files` (path -> JSON value). */ +function inTempRepo(files, body) { + const dir = mkdtempSync(join(tmpdir(), "dependabot-alerts-")); + const cwd = process.cwd(); + try { + for (const [path, value] of Object.entries(files)) { + const full = join(dir, path); + mkdirSync(dirname(full), { recursive: true }); + writeFileSync(full, JSON.stringify(value)); + } + process.chdir(dir); + return body(); + } finally { + process.chdir(cwd); + rmSync(dir, { recursive: true, force: true }); + } +} + +/** A lockfile holding one transitive copy of `pkg` at `version`. */ +const lockWith = (pkg, version) => ({ + lockfileVersion: 3, + packages: { "": { dependencies: {} }, [`node_modules/${pkg}`]: { version } }, +}); + +/** Without a PAT the board is never touched, which most tests want. */ +function withoutProjectToken(body) { + const saved = process.env.PROJECT_TOKEN; + delete process.env.PROJECT_TOKEN; + try { + return body(); + } finally { + if (saved !== undefined) process.env.PROJECT_TOKEN = saved; + } +} + +function withProjectToken(body) { + const saved = process.env.PROJECT_TOKEN; + process.env.PROJECT_TOKEN = "pat"; + try { + return body(); + } finally { + if (saved === undefined) delete process.env.PROJECT_TOKEN; + else process.env.PROJECT_TOKEN = saved; + } +} + +test("main files one issue per bump, flattening a paginated alert feed", () => { + // Two pages, as `--slurp` returns them: a single JSON.parse of concatenated + // pages would have thrown before this ever reached grouping. + const spawn = fakeSpawn({ + alertPages: [ + [ + alert({ ghsa: "GHSA-a", range: ">= 3.1.3, < 3.1.6" }), + alert({ ghsa: "GHSA-b" }), + ], + [ + alert({ + ghsa: "GHSA-c", + pkg: "browserslist", + manifest: "clients/tui/package-lock.json", + fixed: "4.28.7", + range: "<= 4.28.6", + }), + ], + ], + }); + + const log = inTempRepo( + { + "package-lock.json": lockWith("fast-uri", "3.1.5"), + "clients/tui/package-lock.json": lockWith("browserslist", "4.28.2"), + }, + () => withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + + const created = ghCalls(spawn, "create"); + assert.equal(created.length, 2, "3 advisories, 2 bumps, 2 issues"); + const titles = created.map((c) => c.args[c.args.indexOf("--title") + 1]); + assert.ok(titles.some((t) => t.includes("`fast-uri` to `3.1.6`"))); + assert.ok(titles.some((t) => t.includes("`browserslist` to `4.28.7`"))); + // The fast-uri issue names both of its advisories. + const fastUri = created.find((c) => + c.args[c.args.indexOf("--title") + 1].includes("fast-uri"), + ); + const body = fastUri.args[fastUri.args.indexOf("--body") + 1]; + assert.deepEqual(parseMarker(body).ghsas, ["GHSA-a", "GHSA-b"]); + assert.deepEqual(created[0].args.slice(-2), ["--milestone", "v2.6.0"]); + assert.ok(log.some((l) => l.includes("filed"))); +}); + +test("main skips an alert already out of range on the checked-out branch", () => { + const spawn = fakeSpawn({ alertPages: [[alert({ ghsa: "GHSA-a" })]] }); + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.6") }, + () => withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.equal(ghCall(spawn, "create"), undefined); + assert.ok(log.some((l) => l.includes("already out of range"))); +}); + +test("main skips an alert whose manifest is absent from the checkout", () => { + const spawn = fakeSpawn({ + alertPages: [ + [alert({ ghsa: "GHSA-a", manifest: "gone/package-lock.json" })], + ], + }); + const log = inTempRepo({}, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.equal(ghCall(spawn, "create"), undefined); + assert.ok(log.some((l) => l.includes("absent on v2/main"))); +}); + +test("main is a complete no-op on a second run", () => { + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" })]], + issues: [ + { + number: 41, + body: buildIssueBody(group, { installed: ["3.1.5"], direct: false }), + }, + ], + }); + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.5") }, + () => withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.equal(ghCall(spawn, "create"), undefined); + assert.equal(ghCall(spawn, "comment"), undefined); + assert.equal(ghCall(spawn, "edit"), undefined); + assert.ok(log.some((l) => l.includes("#41 already covers"))); +}); + +test("main will not update an issue whose bump differs, even for the same package", () => { + // The marker's `fixed=` is what keeps a 4.0.0 bump off the 3.1.6 issue. + const [old] = groupAlerts([alert({ ghsa: "GHSA-a", fixed: "3.1.6" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-b", fixed: "4.0.0", range: "< 4.0.0" })]], + issues: [ + { + number: 41, + body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }), + }, + ], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.ok(ghCall(spawn, "create"), "a different bump gets its own issue"); + assert.equal(ghCall(spawn, "edit"), undefined); +}); + +test("main comments a new advisory BEFORE rewriting the marker", () => { + const [old] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" }), alert({ ghsa: "GHSA-b" })]], + issues: [ + { + number: 41, + body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }), + }, + ], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + + const order = spawn.calls + .filter( + (c) => c.args[0] === "issue" && ["comment", "edit"].includes(c.args[1]), + ) + .map((c) => c.args[1]); + // Marker-first would let a failed comment be skipped forever by the no-op branch. + assert.deepEqual(order, ["comment", "edit"]); + + const comment = ghCall(spawn, "comment"); + const text = comment.args[comment.args.indexOf("--body") + 1]; + assert.deepEqual(parseCommentMarker(text), ["GHSA-b"]); + assert.ok(!text.includes("GHSA-a"), "only the newly-seen advisory"); + + const edit = ghCall(spawn, "edit"); + assert.deepEqual( + parseMarker(edit.args[edit.args.indexOf("--body") + 1]).ghsas, + ["GHSA-a", "GHSA-b"], + ); +}); + +test("main does not repeat a comment it already posted", () => { + const [old] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" }), alert({ ghsa: "GHSA-b" })]], + issues: [ + { + number: 41, + body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }), + }, + ], + comments: [{ body: `${buildCommentMarker(["GHSA-b"])}\nsaid already` }], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.equal(ghCall(spawn, "comment"), undefined); + // The marker still gets brought up to date. + assert.ok(ghCall(spawn, "edit")); +}); + +test("main fails loudly when Dependabot security PRs are back on", () => { + const spawn = fakeSpawn({ securityFixes: { enabled: true, paused: false } }); + assert.throws( + () => captureLog(() => main("o/r", spawn)), + /security-update PRs are ENABLED again/, + ); + assert.equal(spawn.calls.length, 1, "nothing else runs"); +}); + +test("main continues, reporting UNVERIFIED, when the token cannot read the setting", () => { + const spawn = fakeSpawn({ + securityFixesStatus: 1, + securityFixesStderr: "gh: HTTP 403: Resource not accessible by integration", + alertPages: [[]], + }); + const log = captureLog(() => main("o/r", spawn)); + assert.ok(log.some((l) => l.includes("UNVERIFIED"))); + assert.ok(log.some((l) => l.includes("no open alerts"))); +}); + +test("main throws when the setting lookup fails for a non-permission reason", () => { + const spawn = fakeSpawn({ + securityFixesStatus: 1, + securityFixesStderr: "gh: HTTP 502: Bad Gateway", + }); + // Swallowing this would exit green having skipped the sweep's own precondition. + assert.throws( + () => captureLog(() => main("o/r", spawn)), + /automated-security-fixes lookup failed.*502/s, + ); +}); + +test("main leaves an unmilestoned issue off the board for triage", () => { + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" })]], + milestone: "", + }); + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.5") }, + () => withProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.ok(ghCall(spawn, "create")); + // `Incoming` <=> no milestone: boarding it at Todo would assert an approval + // the invariant reads off the milestone. + assert.equal( + spawn.calls.find( + (c) => c.args[0] === "project" && c.args[1] === "item-add", + ), + undefined, + ); + assert.ok(log.some((l) => l.includes("unmilestoned and unboarded"))); +}); + +test("main fails the run when a card is added but its fields are not set", () => { + const spawn = fakeSpawn({ + alertPages: [ + [ + alert({ ghsa: "GHSA-a" }), + alert({ + ghsa: "GHSA-b", + pkg: "qs", + fixed: "6.16.0", + range: "< 6.16.0", + }), + ], + ], + boardEditStatus: 1, + }); + assert.throws( + () => + inTempRepo( + { + "package-lock.json": { + lockfileVersion: 3, + packages: { + "": { dependencies: {} }, + "node_modules/fast-uri": { version: "3.1.5" }, + "node_modules/qs": { version: "6.15.3" }, + }, + }, + }, + () => withProjectToken(() => captureLog(() => main("o/r", spawn))), + ), + /incomplete board placement/, + ); + // A half-placed card is worth failing over — but not before both issues exist. + assert.equal(ghCalls(spawn, "create").length, 2); +}); diff --git a/scripts/dependency-refresh.mjs b/scripts/dependency-refresh.mjs index 4ce6799e2..348e42367 100644 --- a/scripts/dependency-refresh.mjs +++ b/scripts/dependency-refresh.mjs @@ -191,7 +191,7 @@ export function buildIssueBody(installs, actions = []) { return [ ISSUE_MARKER, - "Routine dependency refresh — `npm outdated` plus a workflow `uses:` check, run against `v2/main` on a monthly schedule. This sweep replaces Dependabot's version-update PRs (#2229, #2235); Dependabot security updates are a separate mechanism and remain enabled.", + "Routine dependency refresh — `npm outdated` plus a workflow `uses:` check, run against `v2/main` on a monthly schedule. This sweep replaces Dependabot's version-update PRs (#2229, #2235); its security-update PRs are off too (#2233), and the alerts they used to act on are swept into their own issues daily.", "", "This is a tracking issue, not a diff: pick what's worth bumping (`wanted` is the safe default; `latest` may cross a major and needs its own judgment call, especially for anything root-declared per [Dependency placement](https://github.com/modelcontextprotocol/inspector/blob/v2/main/AGENTS.md#dependency-placement)) and open a normal PR against `v2/main`.", "", From 761bdd128d9d974348d3ca62b9d8863dd08e3d86 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 00:22:55 -0400 Subject: [PATCH 03/14] chore(deps): address Copilot review round 2 on #2243 - 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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- scripts/dependabot-alerts.mjs | 44 ++++++++++++++----- scripts/dependabot-alerts.test.mjs | 68 ++++++++++++++++++++++++++++-- 2 files changed, 97 insertions(+), 15 deletions(-) diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index 8ca81e323..e4af699de 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -233,7 +233,11 @@ export function groupAlerts(alerts) { // for an unavailable upgrade is noise, so it waits for one to be published. if (!pkg || !manifestPath || !fixedIn) continue; - const key = `${pkg}${manifestPath}${fixedIn}`; + // JSON rather than a delimited string: the three fields are free-form, + // so any separator has to be argued for — and the one that was here was + // a literal NUL, which classified the whole source file as binary and + // made repository searches skip it (Copilot). + const key = JSON.stringify([pkg, manifestPath, fixedIn]); const advisory = { ghsa: alert.security_advisory?.ghsa_id ?? "", cve: alert.security_advisory?.cve_id ?? null, @@ -281,10 +285,12 @@ export function groupAlerts(alerts) { /** * @param {ReturnType[number]} group + * @param {number} [count] advisories the issue covers, when that is more than + * this run saw — an issue grown by a later advisory keeps one title. * @returns {string} */ -export function buildIssueTitle(group) { - const n = group.advisories.length; +export function buildIssueTitle(group, count = group.advisories.length) { + const n = count; return `chore(deps): bump \`${group.package}\` to \`${group.fixedIn}\` in \`${group.manifestPath}\` (${n} ${n === 1 ? "advisory" : "advisories"})`; } @@ -510,7 +516,15 @@ function openDependabotIssues(repo, spawn) { ); } -/** The GHSA sets already announced by comments on an issue. */ +/** + * Every GHSA already announced by a comment on an issue, unioned. + * + * Unioned per ADVISORY, not compared per comment. Comparing whole sets looks + * equivalent and is not: a run that posts `[B]` and then fails before rewriting + * the marker leaves the next run computing `[B, C]`, which matches no existing + * comment, and `B` is announced a second time (Copilot). Individual GHSAs are + * what a comment actually claims to have announced. + */ function announcedAdvisories(repo, number, spawn) { const issue = ghJson(spawn, [ "issue", @@ -521,9 +535,13 @@ function announcedAdvisories(repo, number, spawn) { "--json", "comments", ]); - return (issue?.comments ?? []) - .map((comment) => parseCommentMarker(comment.body)) - .filter(Boolean); + const announced = new Set(); + for (const comment of issue?.comments ?? []) { + for (const ghsa of parseCommentMarker(comment.body) ?? []) { + announced.add(ghsa); + } + } + return announced; } function currentMilestone(repo, spawn) { @@ -785,9 +803,9 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { // run take the no-op branch above and skip the comment permanently // (Copilot). In this order the worst case is a repeat, and the comment's // own marker rules that out too. - const alreadyAnnounced = announcedAdvisories(repo, existing.number, spawn); - const marker = buildCommentMarker(added); - if (!alreadyAnnounced.some((set) => buildCommentMarker(set) === marker)) { + const announced = announcedAdvisories(repo, existing.number, spawn); + const unannounced = added.filter((ghsa) => !announced.has(ghsa)); + if (unannounced.length > 0) { const comment = gh(spawn, [ "issue", "comment", @@ -795,7 +813,7 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { "--repo", repo, "--body", - buildNewAdvisoryComment(group, added), + buildNewAdvisoryComment(group, unannounced), ]); if (comment.status !== 0) { throw new Error( @@ -804,12 +822,16 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { } } + // The title carries the advisory count, so it goes stale the moment the + // issue covers one more than it was filed with (Copilot). const edit = gh(spawn, [ "issue", "edit", String(existing.number), "--repo", repo, + "--title", + buildIssueTitle(group, merged.length), "--body", buildIssueBody(group, { installed: affected, direct, ghsas: merged }), ]); diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index a37faf6a8..8af2c760e 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -1,7 +1,13 @@ -// Unit tests for the pure halves of dependabot-alerts.mjs (#2233). The impure -// half (`main()`, which shells out to `gh`) is exercised only via -// `workflow_dispatch` in CI, per the same split `dependency-refresh.mjs` and -// `verify-skills.mjs` use. Run via `npm run test:scripts`. +// Tests for dependabot-alerts.mjs (#2233) — both the pure grouping/formatting +// helpers and `main()`'s orchestration, the latter driven through the injected +// spawn function so no `gh` process is ever started. +// +// `main()` is covered rather than left to `workflow_dispatch` because a +// production trigger is not a test (Copilot): everything that can go wrong in +// the orchestration — a paginated alert feed, a manifest that has moved on, a +// half-written board card, a comment posted twice — goes wrong only against the +// real API, where nothing would be asserted. +// Run via `npm run test:scripts`. import { test } from "node:test"; import assert from "node:assert/strict"; @@ -605,6 +611,60 @@ test("main does not repeat a comment it already posted", () => { assert.ok(ghCall(spawn, "edit")); }); +test("main announces only the advisories no comment has claimed yet", () => { + // The exact shape a failed marker edit leaves behind: the comment for `b` + // went out, the body edit did not, and a third advisory has since arrived. + // Comparing whole GHSA sets would find no match and announce `b` twice. + const [old] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [ + [ + alert({ ghsa: "GHSA-a" }), + alert({ ghsa: "GHSA-b" }), + alert({ ghsa: "GHSA-c" }), + ], + ], + issues: [ + { + number: 41, + body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }), + }, + ], + comments: [{ body: `${buildCommentMarker(["GHSA-b"])}\nannounced b` }], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + + const comment = ghCall(spawn, "comment"); + assert.ok(comment, "the unannounced advisory still gets a comment"); + const text = comment.args[comment.args.indexOf("--body") + 1]; + assert.deepEqual(parseCommentMarker(text), ["GHSA-c"]); + assert.ok(!text.includes("GHSA-b"), "b was already announced"); +}); + +test("main refreshes the title when an issue grows another advisory", () => { + const [old] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" }), alert({ ghsa: "GHSA-b" })]], + issues: [ + { + number: 41, + body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }), + }, + ], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + const edit = ghCall(spawn, "edit"); + // Filed as "(1 advisory)"; editing only the body would leave it saying so. + assert.match( + edit.args[edit.args.indexOf("--title") + 1], + /\(2 advisories\)$/, + ); +}); + test("main fails loudly when Dependabot security PRs are back on", () => { const spawn = fakeSpawn({ securityFixes: { enabled: true, paused: false } }); assert.throws( From 5c65bd2bfaf3c29fa099cf9c2d26ea5da5a5c71e Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 00:44:48 -0400 Subject: [PATCH 04/14] chore(deps): address Copilot review round 3 on #2243 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- scripts/dependabot-alerts.mjs | 140 +++++++++++++++++++++++------ scripts/dependabot-alerts.test.mjs | 129 +++++++++++++++++++++++--- 2 files changed, 229 insertions(+), 40 deletions(-) diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index e4af699de..c16506021 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -166,33 +166,58 @@ export function toSemverRange(range) { } /** - * Every version of `pkg` installed anywhere in an npm lockfile. + * Every installed copy of `pkg` in an npm lockfile, with its tree path. * - * A transitive package can legitimately appear more than once (a nested - * `node_modules/x/node_modules/y`), and the alert applies if ANY copy is in - * range, so this returns them all rather than picking one. + * A package can legitimately appear more than once — a hoisted + * `node_modules/x` plus one or more nested `node_modules/y/node_modules/x` — + * and the copies can be at DIFFERENT versions. The path is kept rather than + * just the version because it is what distinguishes the copy the manifest + * declares from a copy some dependency dragged in, and the fix for those two + * is not the same (Copilot). * * @param {object} lock parsed `package-lock.json` (lockfileVersion 2 or 3) * @param {string} pkg - * @returns {string[]} sorted, deduped + * @returns {Array<{path: string, version: string, hoisted: boolean}>} sorted by version */ -export function lockfileVersions(lock, pkg) { - const suffix = `node_modules/${pkg}`; - const versions = new Set(); +export function lockfileEntries(lock, pkg) { + const hoistedPath = `node_modules/${pkg}`; + const entries = []; for (const [path, entry] of Object.entries(lock.packages ?? {})) { - if (path !== suffix && !path.endsWith(`/${suffix}`)) continue; - if (entry?.version) versions.add(entry.version); + if (path !== hoistedPath && !path.endsWith(`/${hoistedPath}`)) continue; + if (!entry?.version) continue; + entries.push({ + path, + version: entry.version, + hoisted: path === hoistedPath, + }); } - return [...versions].sort(semver.compare); + return entries.sort( + (a, b) => + semver.compare(a.version, b.version) || a.path.localeCompare(b.path), + ); +} + +/** + * Every version of `pkg` installed anywhere in an npm lockfile. + * + * @param {object} lock parsed `package-lock.json` (lockfileVersion 2 or 3) + * @param {string} pkg + * @returns {string[]} sorted, deduped + */ +export function lockfileVersions(lock, pkg) { + return [...new Set(lockfileEntries(lock, pkg).map((e) => e.version))].sort( + semver.compare, + ); } /** * Is `pkg` declared by the manifest itself, rather than pulled in transitively? * - * Decides which fix the issue asks for: a direct dependency is a plain version - * bump, a transitive one is an `overrides` entry per AGENTS.md's Dependency - * placement — never `npm audit fix`, which "resolves" an advisory with no - * upward escape by silently downgrading. + * ⚠️ Being declared is NOT by itself the question the issue needs answered — + * see `remediation` below. A manifest can declare a safe `pkg@^4` while some + * dependency drags a vulnerable `pkg@3` into a nested folder, and telling the + * maintainer to raise an already-safe range would leave the vulnerable copy + * exactly where it is (Copilot). * * @param {object} lock parsed `package-lock.json` * @param {string} pkg @@ -297,14 +322,46 @@ export function buildIssueTitle(group, count = group.advisories.length) { const PLACEMENT_DOC = "https://github.com/modelcontextprotocol/inspector/blob/v2/main/AGENTS.md#dependency-placement"; +/** + * What the maintainer actually has to change, derived from WHICH copies are + * vulnerable rather than from whether the package is declared. + * + * The three cases are genuinely different edits, and the mixed one is why this + * is not a boolean (Copilot): + * + * | vulnerable copies | fix | + * | --- | --- | + * | the declared (hoisted) one | raise the declared range | + * | nested ones only | an `overrides` pin | + * | both | both, and neither alone is enough | + * + * A manifest declaring a safe `pkg@^4` alongside a dependency that drags in a + * vulnerable nested `pkg@3` lands in the middle row: the declared range is + * already correct, and raising it changes nothing. + * + * @param {Array<{path: string, version: string, hoisted: boolean}>} affected + * @param {boolean} declared whether the manifest declares the package + * @returns {{direct: boolean, transitive: boolean}} + */ +export function remediation(affected, declared) { + const isDeclaredCopy = (entry) => declared && entry.hoisted; + return { + direct: affected.some(isDeclaredCopy), + // Everything that is NOT the declared copy needs the override — nested + // copies, and also a hoisted copy of a package this manifest never + // declared, which got there transitively like any other. + transitive: affected.some((entry) => !isDeclaredCopy(entry)), + }; +} + /** * @param {ReturnType[number]} group - * @param {{installed: string[], direct: boolean, ghsas?: string[]}} probe + * @param {{affected: Array<{path: string, version: string, hoisted: boolean}>, declared: boolean, ghsas?: string[]}} probe * `ghsas` overrides the marker's list when an existing issue is being * rewritten to cover advisories it did not originally name. * @returns {string} */ -export function buildIssueBody(group, { installed, direct, ghsas }) { +export function buildIssueBody(group, { affected, declared, ghsas }) { const covered = ghsas ?? group.ghsas; const rows = group.advisories .map( @@ -313,9 +370,35 @@ export function buildIssueBody(group, { installed, direct, ghsas }) { ) .join("\n"); - const fix = direct - ? `\`${group.package}\` is a **direct** dependency of \`${group.manifestPath.replace(/package-lock\.json$/, "package.json")}\` — raise its declared range so it can no longer resolve below \`${group.fixedIn}\`, keeping the operator the manifest already uses. Widening it to a bare \`>=\` would drop the compatibility bound with it (Copilot).` - : `\`${group.package}\` is **transitive**, so the fix is an [\`overrides\`](${PLACEMENT_DOC}) entry pinning it to \`${group.fixedIn}\` — **not** \`npm audit fix\`, which "resolves" an advisory with no upward escape by silently downgrading.`; + const manifestJson = group.manifestPath.replace( + /package-lock\.json$/, + "package.json", + ); + const { direct, transitive } = remediation(affected, declared); + const steps = []; + if (direct) { + steps.push( + `**Raise the declared range in \`${manifestJson}\`** so \`${group.package}\` can no longer resolve below \`${group.fixedIn}\`, keeping the operator the manifest already uses — widening it to a bare \`>=\` would drop the compatibility bound with it.`, + ); + } + if (transitive) { + steps.push( + `**Add an [\`overrides\`](${PLACEMENT_DOC}) entry** pinning \`${group.package}\` to \`${group.fixedIn}\`, for the ${direct ? "nested copies below, which the declared range does not reach" : "copies below, which no declared range reaches"}. **Not** \`npm audit fix\`, which "resolves" an advisory with no upward escape by silently downgrading.`, + ); + } + const fix = [ + ...(steps.length === 2 + ? [ + "Both edits are needed; neither alone clears every vulnerable copy.", + "", + ] + : []), + ...steps.map((step, i) => (steps.length > 1 ? `${i + 1}. ${step}` : step)), + "", + "| Vulnerable copy | Version |", + "| --- | --- |", + ...affected.map((e) => `| \`${e.path}\` | \`${e.version}\` |`), + ].join("\n"); return [ buildMarker({ ...group, ghsas: covered }), @@ -325,7 +408,7 @@ export function buildIssueBody(group, { installed, direct, ghsas }) { "| --- | --- |", `| Package | \`${group.package}\` |`, `| Manifest | \`${group.manifestPath}\` |`, - `| Installed on \`v2/main\` | ${installed.length > 0 ? installed.map((v) => `\`${v}\``).join(", ") : "—"} |`, + `| Vulnerable on \`${TARGET_BRANCH}\` | ${[...new Set(affected.map((e) => e.version))].map((v) => `\`${v}\``).join(", ") || "—"} |`, `| Fixed in | \`${group.fixedIn}\` |`, `| Scope | ${group.scope} |`, `| Highest severity | ${group.severity} |`, @@ -745,20 +828,21 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { continue; } - const installed = lockfileVersions(lock, group.package); - const affected = installed.filter((version) => + const entries = lockfileEntries(lock, group.package); + const affected = entries.filter((entry) => group.advisories.some((a) => - semver.satisfies(version, toSemverRange(a.range)), + semver.satisfies(entry.version, toSemverRange(a.range)), ), ); if (affected.length === 0) { + const seen = [...new Set(entries.map((e) => e.version))]; console.log( - `dependabot-alerts: ${group.package}@${installed.join("/") || "(absent)"} is already out of range on ${TARGET_BRANCH} — skipping`, + `dependabot-alerts: ${group.package}@${seen.join("/") || "(absent)"} is already out of range on ${TARGET_BRANCH} — skipping`, ); continue; } - const direct = isDirectDependency(lock, group.package); + const declared = isDirectDependency(lock, group.package); // Matched on the full grouping key, `fixedIn` included: a second bump of // the same package is a different issue, not an update to this one. const existing = existingIssues.find( @@ -772,7 +856,7 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { const { url, milestone } = createIssue( repo, group, - buildIssueBody(group, { installed: affected, direct }), + buildIssueBody(group, { affected, declared }), spawn, ); // `Incoming` <=> no milestone, everything past it <=> milestoned. With no @@ -833,7 +917,7 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { "--title", buildIssueTitle(group, merged.length), "--body", - buildIssueBody(group, { installed: affected, direct, ghsas: merged }), + buildIssueBody(group, { affected, declared, ghsas: merged }), ]); if (edit.status !== 0) { throw new Error(`gh issue edit failed: ${(edit.stderr ?? "").trim()}`); diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index 8af2c760e..c1926e354 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -16,6 +16,8 @@ import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; import { buildCommentMarker, + lockfileEntries, + remediation, buildIssueBody, buildIssueTitle, buildMarker, @@ -248,6 +250,19 @@ test("mergeGhsas reports nothing added when the issue already covers them", () = }); }); +/** The probe shape `buildIssueBody` takes: one vulnerable copy, nested by default. */ +const nested = ( + version = "3.1.5", + path = "node_modules/ajv/node_modules/fast-uri", +) => ({ + affected: [{ path, version, hoisted: false }], + declared: false, +}); +const hoisted = (version = "3.1.5") => ({ + affected: [{ path: "node_modules/fast-uri", version, hoisted: true }], + declared: true, +}); + test("buildIssueTitle names the bump and pluralizes the advisory count", () => { const [many] = groupAlerts([ alert({ ghsa: "GHSA-a" }), @@ -269,10 +284,10 @@ test("buildIssueBody leads with the marker and asks for an overrides pin when tr alert({ ghsa: "GHSA-a", cve: "CVE-2026-1" }), alert({ ghsa: "GHSA-b" }), ]); - const body = buildIssueBody(group, { installed: ["3.1.5"], direct: false }); + const body = buildIssueBody(group, nested()); assert.ok(body.startsWith(buildMarker(group))); - assert.match(body, /\| Installed on `v2\/main` \| `3\.1\.5` \|/); + assert.match(body, /\| Vulnerable on `v2\/main` \| `3\.1\.5` \|/); assert.match(body, /\| Fixed in \| `3\.1\.6` \|/); assert.match(body, /GHSA-a/); assert.match(body, /CVE-2026-1/); @@ -282,18 +297,108 @@ test("buildIssueBody leads with the marker and asks for an overrides pin when tr test("buildIssueBody asks a direct dependency's range to be raised, not widened", () => { const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); - const body = buildIssueBody(group, { installed: ["3.1.5"], direct: true }); - assert.match(body, /\*\*direct\*\* dependency of `package\.json`/); + const body = buildIssueBody(group, hoisted()); + assert.match(body, /Raise the declared range in `package\.json`/); assert.match(body, /can no longer resolve below `3\.1\.6`/); // Prescribing `>=3.1.6` would throw away the manifest's compatibility bound. assert.doesNotMatch(body, /range to `>=/); + assert.doesNotMatch(body, /overrides/); +}); + +test("remediation reads the vulnerable copies, not the declaration", () => { + const declaredSafe = [ + { + path: "node_modules/ajv/node_modules/fast-uri", + version: "3.1.5", + hoisted: false, + }, + ]; + // The manifest declares `fast-uri`, but the copy in range is a nested one: + // raising the declared range would change nothing at all. + assert.deepEqual(remediation(declaredSafe, true), { + direct: false, + transitive: true, + }); + assert.deepEqual( + remediation( + [{ path: "node_modules/fast-uri", version: "3.1.5", hoisted: true }], + true, + ), + { direct: true, transitive: false }, + ); + // An undeclared hoisted copy got there transitively like any other, so it + // needs the override — "hoisted" is not a synonym for "declared". + assert.deepEqual( + remediation( + [{ path: "node_modules/fast-uri", version: "3.1.5", hoisted: true }], + false, + ), + { direct: false, transitive: true }, + ); +}); + +test("buildIssueBody asks for BOTH edits when declared and nested copies are vulnerable", () => { + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const body = buildIssueBody(group, { + declared: true, + affected: [ + { path: "node_modules/fast-uri", version: "3.1.5", hoisted: true }, + { + path: "node_modules/ajv/node_modules/fast-uri", + version: "3.0.1", + hoisted: false, + }, + ], + }); + assert.match(body, /Both edits are needed/); + assert.match(body, /1\. \*\*Raise the declared range/); + assert.match(body, /2\. \*\*Add an \[`overrides`\]/); + // The table names the copies, so the maintainer can see why. + assert.match( + body, + /\| `node_modules\/ajv\/node_modules\/fast-uri` \| `3\.0\.1` \|/, + ); +}); + +test("buildIssueBody asks only for an override when the declared copy is safe", () => { + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + // A valid lock: safe declared fast-uri@4, vulnerable nested fast-uri@3.1.5. + const body = buildIssueBody(group, { + declared: true, + affected: [ + { + path: "node_modules/ajv/node_modules/fast-uri", + version: "3.1.5", + hoisted: false, + }, + ], + }); + assert.doesNotMatch(body, /Raise the declared range/); + assert.match(body, /Add an \[`overrides`\]/); +}); + +test("lockfileEntries keeps each copy's path and marks the hoisted one", () => { + const lock = { + packages: { + "": { dependencies: { "fast-uri": "^4.0.0" } }, + "node_modules/fast-uri": { version: "4.0.0" }, + "node_modules/ajv/node_modules/fast-uri": { version: "3.1.5" }, + }, + }; + assert.deepEqual(lockfileEntries(lock, "fast-uri"), [ + { + path: "node_modules/ajv/node_modules/fast-uri", + version: "3.1.5", + hoisted: false, + }, + { path: "node_modules/fast-uri", version: "4.0.0", hoisted: true }, + ]); }); test("buildIssueBody honors an overridden GHSA list when rewriting an issue", () => { const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); const body = buildIssueBody(group, { - installed: ["3.1.5"], - direct: false, + ...nested(), ghsas: ["GHSA-a", "GHSA-old"], }); assert.deepEqual(parseMarker(body).ghsas, ["GHSA-a", "GHSA-old"]); @@ -523,7 +628,7 @@ test("main is a complete no-op on a second run", () => { issues: [ { number: 41, - body: buildIssueBody(group, { installed: ["3.1.5"], direct: false }), + body: buildIssueBody(group, nested()), }, ], }); @@ -545,7 +650,7 @@ test("main will not update an issue whose bump differs, even for the same packag issues: [ { number: 41, - body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }), + body: buildIssueBody(old, nested()), }, ], }); @@ -563,7 +668,7 @@ test("main comments a new advisory BEFORE rewriting the marker", () => { issues: [ { number: 41, - body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }), + body: buildIssueBody(old, nested()), }, ], }); @@ -598,7 +703,7 @@ test("main does not repeat a comment it already posted", () => { issues: [ { number: 41, - body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }), + body: buildIssueBody(old, nested()), }, ], comments: [{ body: `${buildCommentMarker(["GHSA-b"])}\nsaid already` }], @@ -627,7 +732,7 @@ test("main announces only the advisories no comment has claimed yet", () => { issues: [ { number: 41, - body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }), + body: buildIssueBody(old, nested()), }, ], comments: [{ body: `${buildCommentMarker(["GHSA-b"])}\nannounced b` }], @@ -650,7 +755,7 @@ test("main refreshes the title when an issue grows another advisory", () => { issues: [ { number: 41, - body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }), + body: buildIssueBody(old, nested()), }, ], }); From 1e58d2fe8a3bcff2d2044339cfe846b570168d04 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 01:01:02 -0400 Subject: [PATCH 05/14] chore(deps): address Copilot review round 4 on #2243 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- scripts/dependabot-alerts.mjs | 114 ++++++++++++++++++------- scripts/dependabot-alerts.test.mjs | 130 ++++++++++++++++++++++++++++- 2 files changed, 212 insertions(+), 32 deletions(-) diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index c16506021..1cb674118 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -308,6 +308,51 @@ export function groupAlerts(alerts) { ); } +/** + * Narrow a group to the advisories that actually apply to what is installed. + * + * ⚠️ Grouping is by `(package, manifest, first_patched_version)`, and two + * advisories sharing that triple can still have DIFFERENT vulnerable ranges — + * `>= 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` matches only the second. Validating at the group level + * ("does ANY advisory match?") keeps both, and the issue's marker, title, + * severity, advisory table and later comments then all claim an advisory that + * does not apply on this branch (Copilot). + * + * @param {ReturnType[number]} group + * @param {Array<{path: string, version: string, hoisted: boolean}>} entries every installed copy + * @returns {{group: ReturnType[number], affected: Array<{path: string, version: string, hoisted: boolean}>} | null} + * `null` when nothing installed is in range of any of the group's advisories + */ +export function narrowToApplicable(group, entries) { + const applies = (advisory, entry) => + semver.satisfies(entry.version, toSemverRange(advisory.range)); + + const advisories = group.advisories.filter((a) => + entries.some((e) => applies(a, e)), + ); + if (advisories.length === 0) return null; + + const affected = entries.filter((e) => advisories.some((a) => applies(a, e))); + const severity = advisories.reduce( + (worst, a) => + (SEVERITY_RANK[a.severity] ?? 0) > (SEVERITY_RANK[worst] ?? 0) + ? a.severity + : worst, + advisories[0].severity, + ); + + return { + group: { + ...group, + advisories, + ghsas: advisories.map((a) => a.ghsa), + severity, + }, + affected, + }; +} + /** * @param {ReturnType[number]} group * @param {number} [count] advisories the issue covers, when that is more than @@ -319,6 +364,9 @@ export function buildIssueTitle(group, count = group.advisories.length) { return `chore(deps): bump \`${group.package}\` to \`${group.fixedIn}\` in \`${group.manifestPath}\` (${n} ${n === 1 ? "advisory" : "advisories"})`; } +/** Escape a value going into a Markdown table cell. */ +const cell = (value) => String(value).replace(/\|/g, "\\|"); + const PLACEMENT_DOC = "https://github.com/modelcontextprotocol/inspector/blob/v2/main/AGENTS.md#dependency-placement"; @@ -363,10 +411,14 @@ export function remediation(affected, declared) { */ export function buildIssueBody(group, { affected, declared, ghsas }) { const covered = ghsas ?? group.ghsas; + // ⚠️ Every free-form cell is escaped, the RANGE included: a semver range is + // allowed to contain `||`, so a disjoint advisory range like + // `>= 1.0, < 2.0 || >= 3.0, < 3.5` would otherwise inject two extra column + // separators and shear the table apart (Copilot). const rows = group.advisories .map( (a) => - `| [${a.ghsa}](${a.url}) | ${a.cve ?? "—"} | ${a.severity} | ${a.range} | ${a.summary.replace(/\|/g, "\\|")} |`, + `| [${a.ghsa}](${a.url}) | ${cell(a.cve ?? "—")} | ${cell(a.severity)} | ${cell(a.range)} | ${cell(a.summary)} |`, ) .join("\n"); @@ -580,23 +632,26 @@ function readManifest(manifestPath) { } } +/** + * Every open `dependabot`-labeled issue. + * + * Paginated rather than capped: the marker lookup is what makes this sweep + * idempotent, so a truncated list files a duplicate for every issue it could + * not see — at exactly the scale the `--slurp`ed alert fetch is built to handle + * (Copilot). `/issues` also returns pull requests, which carry no marker and + * are dropped. + */ function openDependabotIssues(repo, spawn) { - return ( - ghJson(spawn, [ - "issue", - "list", - "--repo", - repo, - "--state", - "open", - "--label", - "dependabot", - "--json", - "number,body", - "--limit", - "100", - ]) ?? [] - ); + const pages = ghJson(spawn, [ + "api", + "--paginate", + "--slurp", + `repos/${repo}/issues?state=open&labels=dependabot&per_page=100`, + ]); + return (pages ?? []) + .flat() + .filter((issue) => !issue.pull_request) + .map((issue) => ({ number: issue.number, body: issue.body })); } /** @@ -816,31 +871,30 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { const manifests = new Map(); const boardProblems = []; - for (const group of groups) { - if (!manifests.has(group.manifestPath)) { - manifests.set(group.manifestPath, readManifest(group.manifestPath)); + for (const rawGroup of groups) { + if (!manifests.has(rawGroup.manifestPath)) { + manifests.set(rawGroup.manifestPath, readManifest(rawGroup.manifestPath)); } - const lock = manifests.get(group.manifestPath); + const lock = manifests.get(rawGroup.manifestPath); if (lock === null) { console.log( - `dependabot-alerts: ${group.manifestPath} absent on ${TARGET_BRANCH} — skipping ${group.package}`, + `dependabot-alerts: ${rawGroup.manifestPath} absent on ${TARGET_BRANCH} — skipping ${rawGroup.package}`, ); continue; } - const entries = lockfileEntries(lock, group.package); - const affected = entries.filter((entry) => - group.advisories.some((a) => - semver.satisfies(entry.version, toSemverRange(a.range)), - ), - ); - if (affected.length === 0) { + const entries = lockfileEntries(lock, rawGroup.package); + const applicable = narrowToApplicable(rawGroup, entries); + if (applicable === null) { const seen = [...new Set(entries.map((e) => e.version))]; console.log( - `dependabot-alerts: ${group.package}@${seen.join("/") || "(absent)"} is already out of range on ${TARGET_BRANCH} — skipping`, + `dependabot-alerts: ${rawGroup.package}@${seen.join("/") || "(absent)"} is already out of range on ${TARGET_BRANCH} — skipping`, ); continue; } + // From here on `group` carries only the advisories that apply to this + // branch, so the marker, title, severity and table cannot overstate it. + const { group, affected } = applicable; const declared = isDirectDependency(lock, group.package); // Matched on the full grouping key, `fixedIn` included: a second bump of diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index c1926e354..076d4a9d9 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -16,6 +16,7 @@ import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; import { buildCommentMarker, + narrowToApplicable, lockfileEntries, remediation, buildIssueBody, @@ -263,6 +264,76 @@ const hoisted = (version = "3.1.5") => ({ declared: true, }); +test("narrowToApplicable drops advisories the installed version is out of range of", () => { + // Same package, manifest and patched version, so one group — but different + // vulnerable ranges, and 3.1.0 is in range of only one of them. + const [group] = groupAlerts([ + alert({ + ghsa: "GHSA-narrow", + range: ">= 3.1.3, < 3.1.6", + severity: "critical", + }), + alert({ + ghsa: "GHSA-wide", + range: ">= 3.0.0, < 3.1.6", + severity: "medium", + }), + ]); + assert.equal(group.advisories.length, 2); + + const result = narrowToApplicable(group, [ + { path: "node_modules/fast-uri", version: "3.1.0", hoisted: true }, + ]); + assert.deepEqual(result.group.ghsas, ["GHSA-wide"]); + // Severity is re-derived: the critical one does not apply here. + assert.equal(result.group.severity, "medium"); + assert.equal(result.affected.length, 1); + // ...and the marker cannot claim an advisory this branch is not exposed to. + assert.deepEqual(parseMarker(buildMarker(result.group)).ghsas, ["GHSA-wide"]); +}); + +test("narrowToApplicable keeps every advisory that does apply", () => { + const [group] = groupAlerts([ + alert({ ghsa: "GHSA-narrow", range: ">= 3.1.3, < 3.1.6" }), + alert({ ghsa: "GHSA-wide", range: ">= 3.0.0, < 3.1.6" }), + ]); + const result = narrowToApplicable(group, [ + { path: "node_modules/fast-uri", version: "3.1.5", hoisted: true }, + ]); + assert.deepEqual(result.group.ghsas, ["GHSA-narrow", "GHSA-wide"]); +}); + +test("narrowToApplicable returns null when nothing installed is in range", () => { + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + assert.equal( + narrowToApplicable(group, [ + { path: "node_modules/fast-uri", version: "3.1.6", hoisted: true }, + ]), + null, + ); + assert.equal(narrowToApplicable(group, []), null); +}); + +test("buildIssueBody escapes a disjoint range so the table survives it", () => { + // `||` is legal in a semver range and is also the Markdown column separator. + const [group] = groupAlerts([ + alert({ ghsa: "GHSA-a", range: ">= 1.0.0, < 2.0.0 || >= 3.0.0, < 3.5.0" }), + ]); + const body = buildIssueBody(group, nested()); + const row = body + .split("\n") + .find((line) => line.includes("GHSA-a") && line.startsWith("|")); + // Count only the pipes Markdown will treat as separators: five columns means + // four inner separators plus the two outer ones. + const separators = row.replace(/\\\|/g, "").split("|").length - 1; + assert.equal( + separators, + 6, + `escaped row should keep its column count: ${row}`, + ); + assert.ok(row.includes(String.raw`\|\|`), "the range's own pipes survive"); +}); + test("buildIssueTitle names the bump and pluralizes the advisory count", () => { const [many] = groupAlerts([ alert({ ghsa: "GHSA-a" }), @@ -460,8 +531,9 @@ function fakeSpawn({ return ok(JSON.stringify(alertPages)); } if (joined.includes("milestones")) return ok(milestone); - if (args[0] === "issue" && args[1] === "list") - return ok(JSON.stringify(issues)); + // `--slurp`, so one array per page — `issues` may be a flat list or pages. + if (joined.includes("/issues?")) + return ok(JSON.stringify(Array.isArray(issues[0]) ? issues : [issues])); if (args[0] === "issue" && args[1] === "view") return ok(JSON.stringify({ comments })); if (args[0] === "issue" && args[1] === "create") @@ -823,6 +895,60 @@ test("main leaves an unmilestoned issue off the board for triage", () => { assert.ok(log.some((l) => l.includes("unmilestoned and unboarded"))); }); +test("main reads every page of open dependabot issues", () => { + // The second page holds the matching marker. Truncating the lookup would + // file a duplicate issue rather than recognising this one. + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" })]], + issues: [ + [{ number: 1, body: "an unrelated dependabot issue" }], + [{ number: 41, body: buildIssueBody(group, nested()) }], + ], + }); + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.5") }, + () => withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.equal(ghCall(spawn, "create"), undefined); + assert.ok(log.some((l) => l.includes("#41 already covers"))); +}); + +test("main drops a pull request returned by the issues endpoint", () => { + // `/issues` returns PRs too; one carrying no marker must not be mistaken for + // a match, nor crash the lookup. + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" })]], + issues: [[{ number: 9, body: "a PR body", pull_request: { url: "..." } }]], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.ok(ghCall(spawn, "create"), "the issue is still filed"); +}); + +test("main files an issue naming only the advisories that apply here", () => { + const spawn = fakeSpawn({ + alertPages: [ + [ + alert({ ghsa: "GHSA-narrow", range: ">= 3.1.3, < 3.1.6" }), + alert({ ghsa: "GHSA-wide", range: ">= 3.0.0, < 3.1.6" }), + ], + ], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.0") }, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + const create = ghCall(spawn, "create"); + const body = create.args[create.args.indexOf("--body") + 1]; + assert.deepEqual(parseMarker(body).ghsas, ["GHSA-wide"]); + assert.ok(!body.includes("GHSA-narrow"), "3.1.0 is out of that range"); + assert.match( + create.args[create.args.indexOf("--title") + 1], + /\(1 advisory\)$/, + ); +}); + test("main fails the run when a card is added but its fields are not set", () => { const spawn = fakeSpawn({ alertPages: [ From a17eace0bbc9b15ac6a180106a082c86b15800f5 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 01:15:28 -0400 Subject: [PATCH 06/14] chore(deps): address Copilot review round 5 on #2243 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- .github/workflows/dependabot-alerts.yml | 10 ++++++ scripts/dependabot-alerts.mjs | 30 +++++++++++++----- scripts/dependabot-alerts.test.mjs | 41 ++++++++++++++++++++++++- 3 files changed, 73 insertions(+), 8 deletions(-) diff --git a/.github/workflows/dependabot-alerts.yml b/.github/workflows/dependabot-alerts.yml index 4e1f3f388..7fe631f9a 100644 --- a/.github/workflows/dependabot-alerts.yml +++ b/.github/workflows/dependabot-alerts.yml @@ -55,6 +55,16 @@ on: - cron: "17 6 * * *" # 06:17 UTC daily; alerts are not minute-sensitive workflow_dispatch: +# The marker check is a read-before-write, not an atomic one, and nothing stops +# a `workflow_dispatch` from landing on top of the scheduled run. Two overlapping +# runs would both see no open issue and both file one, which is the exact +# duplicate this sweep's whole idempotency design exists to prevent (Copilot). +# `cancel-in-progress: false` because the queued run must WAIT and then re-read +# the state the first run wrote — cancelling it would drop a sweep instead. +concurrency: + group: dependabot-alert-sweep + cancel-in-progress: false + permissions: contents: read issues: write diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index 1cb674118..dc6b596a2 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -308,6 +308,27 @@ export function groupAlerts(alerts) { ); } +/** + * The milestone a new issue takes: the open one with the NEAREST due date. + * + * ⚠️ Selected here rather than in a `jq` expression because jq sorts `null` + * BEFORE every string, so a `sort_by(.due_on) | .[0]` over the raw list hands + * back an undated milestone in preference to every dated one (Copilot). An + * undated bucket has no due date and so cannot be the nearest; it is dropped + * rather than sorted last, and if nothing dated is open the issue is filed + * unmilestoned and triage places it. + * + * @param {Array<{title: string, state?: string, due_on?: string | null}>} milestones + * @returns {string | null} + */ +export function pickMilestone(milestones) { + const dated = (milestones ?? []).filter( + (m) => (m.state ?? "open") === "open" && m.due_on, + ); + if (dated.length === 0) return null; + return dated.sort((a, b) => a.due_on.localeCompare(b.due_on))[0].title; +} + /** * Narrow a group to the advisories that actually apply to what is installed. * @@ -683,16 +704,11 @@ function announcedAdvisories(repo, number, spawn) { } function currentMilestone(repo, spawn) { - const result = gh(spawn, [ - "api", - `repos/${repo}/milestones`, - "--jq", - 'map(select(.state=="open")) | sort_by(.due_on) | .[0].title // empty', - ]); + const result = gh(spawn, ["api", `repos/${repo}/milestones?state=open`]); if (result.status !== 0) { throw new Error(`milestone lookup failed: ${(result.stderr ?? "").trim()}`); } - return result.stdout.trim() || null; + return pickMilestone(JSON.parse(result.stdout || "[]")); } /** diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index 076d4a9d9..08d2edc78 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -16,6 +16,7 @@ import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; import { buildCommentMarker, + pickMilestone, narrowToApplicable, lockfileEntries, remediation, @@ -334,6 +335,31 @@ test("buildIssueBody escapes a disjoint range so the table survives it", () => { assert.ok(row.includes(String.raw`\|\|`), "the range's own pipes survive"); }); +test("pickMilestone takes the nearest due date, never an undated bucket", () => { + // jq sorts null before every string, so `sort_by(.due_on) | .[0]` over this + // list would return "Backlog" — an open bucket with no release date at all. + const milestones = [ + { title: "Backlog", state: "open", due_on: null }, + { title: "v2.7.0", state: "open", due_on: "2026-09-16T00:00:00Z" }, + { title: "v2.6.0", state: "open", due_on: "2026-09-09T00:00:00Z" }, + ]; + assert.equal(pickMilestone(milestones), "v2.6.0"); +}); + +test("pickMilestone ignores closed milestones and empty input", () => { + assert.equal( + pickMilestone([ + { title: "v2.5.0", state: "closed", due_on: "2026-01-01T00:00:00Z" }, + ]), + null, + ); + // Nothing dated and open means no bucket to take: filed unmilestoned, and + // the board write is skipped so triage places it. + assert.equal(pickMilestone([{ title: "Backlog", due_on: null }]), null); + assert.equal(pickMilestone([]), null); + assert.equal(pickMilestone(undefined), null); +}); + test("buildIssueTitle names the bump and pluralizes the advisory count", () => { const [many] = groupAlerts([ alert({ ghsa: "GHSA-a" }), @@ -530,7 +556,20 @@ function fakeSpawn({ // `--slurp` yields one array PER PAGE; main() must flatten them. return ok(JSON.stringify(alertPages)); } - if (joined.includes("milestones")) return ok(milestone); + if (joined.includes("milestones")) + return ok( + JSON.stringify( + milestone + ? [ + { + title: milestone, + state: "open", + due_on: "2026-09-09T00:00:00Z", + }, + ] + : [], + ), + ); // `--slurp`, so one array per page — `issues` may be a flat list or pages. if (joined.includes("/issues?")) return ok(JSON.stringify(Array.isArray(issues[0]) ? issues : [issues])); From ff339dc7c30d9f466cf19aa373668374f7106eb2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 01:31:32 -0400 Subject: [PATCH 07/14] chore(deps): address Copilot review round 6 on #2243 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- scripts/dependabot-alerts.mjs | 68 +++++++++++++----- scripts/dependabot-alerts.test.mjs | 108 +++++++++++++++++++++++++---- 2 files changed, 145 insertions(+), 31 deletions(-) diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index dc6b596a2..a8fce781d 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -375,13 +375,16 @@ export function narrowToApplicable(group, entries) { } /** - * @param {ReturnType[number]} group - * @param {number} [count] advisories the issue covers, when that is more than - * this run saw — an issue grown by a later advisory keeps one title. + * The title counts the advisories that APPLY, which is what the body shows — + * so it tracks an issue that grows a new advisory and one whose exposure + * shrinks alike. The marker's GHSA list is a different thing: it is monotonic, + * because its job is to remember what has already been announced. + * + * @param {ReturnType[number]} group narrowed to what applies * @returns {string} */ -export function buildIssueTitle(group, count = group.advisories.length) { - const n = count; +export function buildIssueTitle(group) { + const n = group.advisories.length; return `chore(deps): bump \`${group.package}\` to \`${group.fixedIn}\` in \`${group.manifestPath}\` (${n} ${n === 1 ? "advisory" : "advisories"})`; } @@ -560,19 +563,27 @@ function ghJson(spawn, args) { } /** - * Is this failed lookup the "the token may not read this" answer, rather than a - * real API failure? + * Is this failed lookup the "this token may not read that" answer, rather than + * a real API failure? * * The distinction is what keeps the security-PR guard honest: a bad token, a * rate limit or a transient 5xx must NOT be waved through as "unverified", or - * the sweep exits green having silently skipped its own precondition - * (Copilot). Only an authorization-shaped status is tolerated. + * the sweep exits green having silently skipped its own precondition. + * + * ⚠️ Status alone is not enough, which is what the first version got wrong + * (Copilot). GitHub answers BOTH "you lack `administration: read`" and "you + * have exhausted your quota" with **403**, and the second is a real failure — + * so the rate-limit wording is excluded explicitly. **401** is a bad or expired + * token, never a scope question, and is a real failure too. **404** stays + * tolerated because GitHub hides resources a token cannot see behind one rather + * than admitting they exist. * * @param {string} stderr stderr from a non-zero `gh api` call * @returns {boolean} */ export function isPermissionDenied(stderr) { - return /HTTP (401|403|404)\b/.test(stderr); + if (/rate limit/i.test(stderr)) return false; + return /HTTP (403|404)\b/.test(stderr); } /** @@ -672,7 +683,11 @@ function openDependabotIssues(repo, spawn) { return (pages ?? []) .flat() .filter((issue) => !issue.pull_request) - .map((issue) => ({ number: issue.number, body: issue.body })); + .map((issue) => ({ + number: issue.number, + title: issue.title, + body: issue.body, + })); } /** @@ -945,9 +960,26 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { } const { merged, added } = mergeGhsas(existing.marker.ghsas, group.ghsas); - if (added.length === 0) { + const title = buildIssueTitle(group); + const body = buildIssueBody(group, { + affected, + declared, + ghsas: merged, + }); + + // ⚠️ "Nothing NEW" is not the same as "nothing CHANGED" (Copilot). An issue + // filed for A+B whose branch has since moved so only B applies has no added + // GHSAs, yet its table, severity, affected copies and remediation are all + // stale. So the no-op is decided by comparing the rendered issue, not by + // counting additions — while a COMMENT stays reserved for advisories that + // are genuinely new. + if ( + added.length === 0 && + existing.title === title && + existing.body === body + ) { console.log( - `dependabot-alerts: #${existing.number} already covers ${group.package} — no-op`, + `dependabot-alerts: #${existing.number} is up to date for ${group.package} — no-op`, ); continue; } @@ -976,8 +1008,6 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { } } - // The title carries the advisory count, so it goes stale the moment the - // issue covers one more than it was filed with (Copilot). const edit = gh(spawn, [ "issue", "edit", @@ -985,15 +1015,17 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { "--repo", repo, "--title", - buildIssueTitle(group, merged.length), + title, "--body", - buildIssueBody(group, { affected, declared, ghsas: merged }), + body, ]); if (edit.status !== 0) { throw new Error(`gh issue edit failed: ${(edit.stderr ?? "").trim()}`); } console.log( - `dependabot-alerts: added ${added.join(", ")} to #${existing.number}`, + added.length > 0 + ? `dependabot-alerts: added ${added.join(", ")} to #${existing.number}` + : `dependabot-alerts: refreshed #${existing.number} for ${group.package}`, ); } diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index 08d2edc78..593179240 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -212,18 +212,35 @@ test("buildCommentMarker and parseCommentMarker round-trip", () => { assert.equal(parseCommentMarker("an ordinary comment"), null); }); -test("isPermissionDenied separates an authorization failure from a real one", () => { +test("isPermissionDenied tolerates a scope refusal", () => { + // The two ways GitHub says "this token may not read that": an explicit 403, + // and a 404 hiding a resource the token cannot see. assert.equal( - isPermissionDenied("gh: HTTP 403: Resource not accessible"), + isPermissionDenied("gh: HTTP 403: Resource not accessible by integration"), true, ); - assert.equal(isPermissionDenied("gh: HTTP 401: Bad credentials"), true); assert.equal(isPermissionDenied("gh: HTTP 404: Not Found"), true); +}); + +test("isPermissionDenied treats a bad token or a rate limit as a real failure", () => { + // ⚠️ A rate limit is also a 403, so status alone cannot decide this — waving + // it through would exit green having skipped the sweep's own precondition. + assert.equal( + isPermissionDenied("gh: API rate limit exceeded (HTTP 403)"), + false, + ); + assert.equal( + isPermissionDenied( + "gh: HTTP 403: You have exceeded a secondary rate limit", + ), + false, + ); + // 401 is a bad or expired token, never a scope question. + assert.equal(isPermissionDenied("gh: HTTP 401: Bad credentials"), false); assert.equal( isPermissionDenied("gh: HTTP 500: Internal Server Error"), false, ); - assert.equal(isPermissionDenied("gh: API rate limit exceeded"), false); }); test("parseMarker returns null for an unmarked or absent body", () => { @@ -637,6 +654,16 @@ function inTempRepo(files, body) { } } +/** + * The probe `main()` derives from `lockWith`: one hoisted, undeclared copy. + * A fixture issue built from this renders byte-identically to what `main()` + * would produce, which is what makes the no-op path assertable. + */ +const asInstalled = (version = "3.1.5") => ({ + affected: [{ path: "node_modules/fast-uri", version, hoisted: true }], + declared: false, +}); + /** A lockfile holding one transitive copy of `pkg` at `version`. */ const lockWith = (pkg, version) => ({ lockfileVersion: 3, @@ -739,7 +766,8 @@ test("main is a complete no-op on a second run", () => { issues: [ { number: 41, - body: buildIssueBody(group, nested()), + title: buildIssueTitle(group), + body: buildIssueBody(group, asInstalled()), }, ], }); @@ -750,7 +778,7 @@ test("main is a complete no-op on a second run", () => { assert.equal(ghCall(spawn, "create"), undefined); assert.equal(ghCall(spawn, "comment"), undefined); assert.equal(ghCall(spawn, "edit"), undefined); - assert.ok(log.some((l) => l.includes("#41 already covers"))); + assert.ok(log.some((l) => l.includes("#41 is up to date"))); }); test("main will not update an issue whose bump differs, even for the same package", () => { @@ -761,7 +789,8 @@ test("main will not update an issue whose bump differs, even for the same packag issues: [ { number: 41, - body: buildIssueBody(old, nested()), + title: buildIssueTitle(old), + body: buildIssueBody(old, asInstalled()), }, ], }); @@ -779,7 +808,8 @@ test("main comments a new advisory BEFORE rewriting the marker", () => { issues: [ { number: 41, - body: buildIssueBody(old, nested()), + title: buildIssueTitle(old), + body: buildIssueBody(old, asInstalled()), }, ], }); @@ -814,7 +844,8 @@ test("main does not repeat a comment it already posted", () => { issues: [ { number: 41, - body: buildIssueBody(old, nested()), + title: buildIssueTitle(old), + body: buildIssueBody(old, asInstalled()), }, ], comments: [{ body: `${buildCommentMarker(["GHSA-b"])}\nsaid already` }], @@ -843,7 +874,8 @@ test("main announces only the advisories no comment has claimed yet", () => { issues: [ { number: 41, - body: buildIssueBody(old, nested()), + title: buildIssueTitle(old), + body: buildIssueBody(old, asInstalled()), }, ], comments: [{ body: `${buildCommentMarker(["GHSA-b"])}\nannounced b` }], @@ -866,7 +898,8 @@ test("main refreshes the title when an issue grows another advisory", () => { issues: [ { number: 41, - body: buildIssueBody(old, nested()), + title: buildIssueTitle(old), + body: buildIssueBody(old, asInstalled()), }, ], }); @@ -934,6 +967,49 @@ test("main leaves an unmilestoned issue off the board for triage", () => { assert.ok(log.some((l) => l.includes("unmilestoned and unboarded"))); }); +test("main refreshes an issue whose exposure shrank, without commenting", () => { + // Filed when both advisories applied; `v2/main` has since moved to 3.1.0, + // which is out of range of the narrow one. Nothing is NEW, so `added` is + // empty — but the body still claims an advisory that no longer applies. + const [filed] = groupAlerts([ + alert({ ghsa: "GHSA-narrow", range: ">= 3.1.3, < 3.1.6" }), + alert({ ghsa: "GHSA-wide", range: ">= 3.0.0, < 3.1.6" }), + ]); + const spawn = fakeSpawn({ + alertPages: [ + [ + alert({ ghsa: "GHSA-narrow", range: ">= 3.1.3, < 3.1.6" }), + alert({ ghsa: "GHSA-wide", range: ">= 3.0.0, < 3.1.6" }), + ], + ], + issues: [ + { + number: 41, + title: buildIssueTitle(filed), + body: buildIssueBody(filed, asInstalled()), + }, + ], + }); + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.0") }, + () => withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + + // A comment is for genuinely new advisories, and there are none. + assert.equal(ghCall(spawn, "comment"), undefined); + + const edit = ghCall(spawn, "edit"); + assert.ok(edit, "the stale body is rewritten"); + const body = edit.args[edit.args.indexOf("--body") + 1]; + assert.ok(!body.includes("| [GHSA-narrow]"), "no longer in the table"); + assert.match(body, /`3\.1\.0`/, "the affected version is refreshed"); + assert.match(edit.args[edit.args.indexOf("--title") + 1], /\(1 advisory\)$/); + // The marker stays monotonic: it records what has been announced, so the + // dropped advisory cannot be re-announced later. + assert.deepEqual(parseMarker(body).ghsas, ["GHSA-narrow", "GHSA-wide"]); + assert.ok(log.some((l) => l.includes("refreshed #41"))); +}); + test("main reads every page of open dependabot issues", () => { // The second page holds the matching marker. Truncating the lookup would // file a duplicate issue rather than recognising this one. @@ -942,7 +1018,13 @@ test("main reads every page of open dependabot issues", () => { alertPages: [[alert({ ghsa: "GHSA-a" })]], issues: [ [{ number: 1, body: "an unrelated dependabot issue" }], - [{ number: 41, body: buildIssueBody(group, nested()) }], + [ + { + number: 41, + title: buildIssueTitle(group), + body: buildIssueBody(group, asInstalled()), + }, + ], ], }); const log = inTempRepo( @@ -950,7 +1032,7 @@ test("main reads every page of open dependabot issues", () => { () => withoutProjectToken(() => captureLog(() => main("o/r", spawn))), ); assert.equal(ghCall(spawn, "create"), undefined); - assert.ok(log.some((l) => l.includes("#41 already covers"))); + assert.ok(log.some((l) => l.includes("#41 is up to date"))); }); test("main drops a pull request returned by the issues endpoint", () => { From 1d6ea1a8f63b3690b62060938dd70d9a8ac82825 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 01:44:51 -0400 Subject: [PATCH 08/14] chore(deps): address Copilot review round 7 on #2243 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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- scripts/dependabot-alerts.mjs | 89 +++++++++++++++++++++--- scripts/dependabot-alerts.test.mjs | 106 +++++++++++++++++++++++++++++ 2 files changed, 185 insertions(+), 10 deletions(-) diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index a8fce781d..1dd384b96 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -435,6 +435,7 @@ export function remediation(affected, declared) { */ export function buildIssueBody(group, { affected, declared, ghsas }) { const covered = ghsas ?? group.ghsas; + const applying = group.advisories.length; // ⚠️ Every free-form cell is escaped, the RANGE included: a semver range is // allowed to contain `||`, so a disjoint advisory range like // `>= 1.0, < 2.0 || >= 3.0, < 3.5` would otherwise inject two extra column @@ -478,7 +479,10 @@ export function buildIssueBody(group, { affected, declared, ghsas }) { return [ buildMarker({ ...group, ghsas: covered }), - `Filed automatically from ${covered.length} open Dependabot ${covered.length === 1 ? "alert" : "alerts"} (#2233). Dependabot opens no security-update PRs on this repo; the fix is written by hand against \`v2/main\`.`, + // Counts what APPLIES, like the title and the table — `covered` is the + // marker's monotonic history and would keep counting an advisory that has + // since closed (Copilot). + `Filed automatically from ${applying} open Dependabot ${applying === 1 ? "alert" : "alerts"} (#2233). Dependabot opens no security-update PRs on this repo; the fix is written by hand against \`${TARGET_BRANCH}\`.`, "", "| | |", "| --- | --- |", @@ -504,6 +508,32 @@ export function buildIssueBody(group, { affected, declared, ghsas }) { ].join("\n"); } +/** + * The body an issue is rewritten to once its exposure is gone. + * + * The marker is retained, so the sweep still recognises this issue and will not + * file a fresh one if the same advisory comes back into range. The issue is NOT + * closed automatically: whether the exposure went away because a PR fixed it or + * because the dependency was dropped decides whether the board card is moved to + * Done or deleted, and that is a judgement the sweep cannot make. + * + * @param {ReturnType[number]} group + * @param {{ghsas: string[], reason: string, today: string}} context + * @returns {string} + */ +export function buildClearedBody(group, { ghsas, reason, today }) { + return [ + buildMarker({ ...group, ghsas }), + `**No longer applicable on \`${TARGET_BRANCH}\` as of ${today}** — ${reason}.`, + "", + `Nothing here needs bumping any more: \`${group.package}\` is no longer exposed to ${ghsas.length === 1 ? "the advisory" : "the advisories"} below on the branch we ship from. This body is rewritten in place rather than the issue being closed, because whether the card belongs in **Done** or should be **deleted** depends on why the exposure went away — a merged fix shipped something, a dropped dependency did not.`, + "", + `Previously covered: ${ghsas.map((g) => `\`${g}\``).join(", ")}.`, + "", + "If the same advisory comes back into range, this issue is reused rather than a new one filed.", + ].join("\n"); +} + /** * The marker that makes a "new advisories" comment idempotent on its own. * @@ -883,7 +913,11 @@ function createIssue(repo, group, body, spawn) { return { url, milestone }; } -export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { +export function main( + repo = process.env.GITHUB_REPOSITORY, + spawn = spawnSync, + today = new Date().toISOString().slice(0, 10), +) { if (!repo) throw new Error("repo not specified (GITHUB_REPOSITORY unset)"); checkSecurityPrsStillDisabled(repo, spawn); @@ -903,6 +937,43 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { const boardProblems = []; for (const rawGroup of groups) { + // ⚠️ Resolved BEFORE the skips below, not after. An issue filed yesterday + // is still open today, and if the manifest has since gone or every copy has + // moved out of range, skipping straight past it leaves its body asserting a + // vulnerability that no longer exists and its Todo/High card live forever + // (Copilot). Matched on the full grouping key, `fixedIn` included: a second + // bump of the same package is a different issue, not an update to this one. + const existing = existingIssues.find( + (i) => + i.marker?.package === rawGroup.package && + i.marker?.manifestPath === rawGroup.manifestPath && + i.marker?.fixedIn === rawGroup.fixedIn, + ); + + /** Rewrite an open issue to its cleared state, once. */ + const clear = (reason) => { + if (!existing) return; + const body = buildClearedBody(rawGroup, { + ghsas: existing.marker.ghsas, + reason, + today, + }); + if (existing.body === body) return; + const edit = gh(spawn, [ + "issue", + "edit", + String(existing.number), + "--repo", + repo, + "--body", + body, + ]); + if (edit.status !== 0) { + throw new Error(`gh issue edit failed: ${(edit.stderr ?? "").trim()}`); + } + console.log(`dependabot-alerts: cleared #${existing.number} — ${reason}`); + }; + if (!manifests.has(rawGroup.manifestPath)) { manifests.set(rawGroup.manifestPath, readManifest(rawGroup.manifestPath)); } @@ -911,6 +982,7 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { console.log( `dependabot-alerts: ${rawGroup.manifestPath} absent on ${TARGET_BRANCH} — skipping ${rawGroup.package}`, ); + clear(`\`${rawGroup.manifestPath}\` is no longer part of this repo`); continue; } @@ -921,6 +993,11 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { console.log( `dependabot-alerts: ${rawGroup.package}@${seen.join("/") || "(absent)"} is already out of range on ${TARGET_BRANCH} — skipping`, ); + clear( + seen.length > 0 + ? `every installed copy is out of range (${seen.map((v) => `\`${v}\``).join(", ")})` + : "the package is no longer installed at all", + ); continue; } // From here on `group` carries only the advisories that apply to this @@ -928,14 +1005,6 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) { const { group, affected } = applicable; const declared = isDirectDependency(lock, group.package); - // Matched on the full grouping key, `fixedIn` included: a second bump of - // the same package is a different issue, not an update to this one. - const existing = existingIssues.find( - (i) => - i.marker?.package === group.package && - i.marker?.manifestPath === group.manifestPath && - i.marker?.fixedIn === group.fixedIn, - ); if (!existing) { const { url, milestone } = createIssue( diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index 593179240..85c4bac04 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -15,6 +15,7 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; import { + buildClearedBody, buildCommentMarker, pickMilestone, narrowToApplicable, @@ -1010,6 +1011,111 @@ test("main refreshes an issue whose exposure shrank, without commenting", () => assert.ok(log.some((l) => l.includes("refreshed #41"))); }); +test("main clears an open issue when the manifest is gone", () => { + const [group] = groupAlerts([ + alert({ ghsa: "GHSA-a", manifest: "clients/gone/package-lock.json" }), + ]); + const spawn = fakeSpawn({ + alertPages: [ + [alert({ ghsa: "GHSA-a", manifest: "clients/gone/package-lock.json" })], + ], + issues: [ + { + number: 41, + title: buildIssueTitle(group), + body: buildIssueBody(group, asInstalled()), + }, + ], + }); + const log = inTempRepo({}, () => + withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ), + ); + + const edit = ghCall(spawn, "edit"); + assert.ok(edit, "the stale issue is rewritten, not silently skipped"); + const body = edit.args[edit.args.indexOf("--body") + 1]; + assert.match(body, /No longer applicable on `v2\/main` as of 2026-09-04/); + assert.match(body, /no longer part of this repo/); + // The marker survives, so the issue is reused if the advisory comes back. + assert.deepEqual(parseMarker(body).ghsas, ["GHSA-a"]); + assert.equal(ghCall(spawn, "create"), undefined); + assert.ok(log.some((l) => l.includes("cleared #41"))); +}); + +test("main clears an open issue when every copy moved out of range", () => { + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" })]], + issues: [ + { + number: 41, + title: buildIssueTitle(group), + body: buildIssueBody(group, asInstalled()), + }, + ], + }); + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.6") }, + () => + withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ), + ); + const edit = ghCall(spawn, "edit"); + assert.ok(edit); + const body = edit.args[edit.args.indexOf("--body") + 1]; + assert.match(body, /every installed copy is out of range \(`3\.1\.6`\)/); + assert.ok(log.some((l) => l.includes("cleared #41"))); +}); + +test("main does not re-clear an issue it already cleared", () => { + // The second run of a cleared sweep must touch nothing at all. + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" })]], + issues: [ + { + number: 41, + title: buildIssueTitle(group), + body: buildClearedBody(group, { + ghsas: ["GHSA-a"], + reason: "every installed copy is out of range (`3.1.6`)", + today: "2026-09-04", + }), + }, + ], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.6") }, () => + withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ), + ); + assert.equal(ghCall(spawn, "edit"), undefined); + assert.equal(ghCall(spawn, "create"), undefined); +}); + +test("main skips quietly when nothing applies and no issue is open", () => { + const spawn = fakeSpawn({ alertPages: [[alert({ ghsa: "GHSA-a" })]] }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.6") }, () => + withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.equal(ghCall(spawn, "edit"), undefined); + assert.equal(ghCall(spawn, "create"), undefined); +}); + +test("buildIssueBody counts the applicable alerts in its prose, not the marker", () => { + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + // The marker carries a second, since-closed advisory; the prose must not. + const body = buildIssueBody(group, { + ...nested(), + ghsas: ["GHSA-a", "GHSA-closed"], + }); + assert.match(body, /Filed automatically from 1 open Dependabot alert\b/); + assert.deepEqual(parseMarker(body).ghsas, ["GHSA-a", "GHSA-closed"]); +}); + test("main reads every page of open dependabot issues", () => { // The second page holds the matching marker. Truncating the lookup would // file a duplicate issue rather than recognising this one. From 9e516ae0a18773f976f9514bb5d25b34606a6491 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 07:52:16 -0400 Subject: [PATCH 09/14] chore(deps): address Copilot review rounds 8-10 on #2243 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- AGENTS.md | 9 +- scripts/dependabot-alerts.mjs | 137 +++++++++++++++++++-- scripts/dependabot-alerts.test.mjs | 187 ++++++++++++++++++++++++++++- 3 files changed, 316 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ba08444fe..71288c60b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -114,7 +114,14 @@ Four things about this that are not obvious from the code: - **Alerts are computed from the default branch (`main`), and we ship from `v2/main`.** So the sweep re-checks each alert's vulnerable range against `v2/main`'s own lockfile before filing, and skips one that is already fixed there. The converse is a real blind spot with no fix on this path: a vulnerable dependency introduced on `v2/main` and not yet merged to `main` produces **no alert at all**. The release-time `npm audit --audit-level=high` report (#2231) is the partial second signal — and only at release time. - **`automated-security-fixes` can be re-enabled from the UI without a commit**, so nothing in the repo would record it. The sweep reads it back and **fails loudly on an explicit `enabled: true`**. ⚠️ It is a *conditional* guard, not an invariant: the endpoint needs `administration: read`, which `GITHUB_TOKEN` cannot be granted (`permissions:` has no such key), so under the default token the sweep logs **UNVERIFIED** and carries on rather than going red every day for an unrelated reason. Only a token carrying that scope makes it a real assertion. -An issue filed by either sweep is an ordinary board item — `v2` + `chore` + `dependabot`, the current milestone, and a card on #28. A security issue lands at **Todo / High**: arriving through this pipeline *is* the approval, and `High` is a standing override of the [priority rubric](.claude/skills/issue-triage/SKILL.md), which would otherwise score a routine bump Medium. Board placement needs an org-project PAT that `GITHUB_TOKEN` cannot have, so it is **best-effort** — without the secret the issue is still created labeled and milestoned, and the next triage sweep boards it. +An issue filed by either sweep is an ordinary board item — `v2` + `chore` + `dependabot`, the current milestone, and a card on #28. **How it gets its card differs, and the two sweeps are not interchangeable here:** + +| | files the card itself? | +| --- | --- | +| Monthly version sweep | **No, never.** It does not attempt a board write at all and has no `PROJECT_TOKEN`; the issue arrives labeled and milestoned, and `/issue-triage` places it. | +| Daily security sweep | **Only when it can.** With an org-project PAT it places the card directly at **Todo / High**; without one it degrades to the same triage hand-off. | + +The board write needs `organization projects: write`, which `GITHUB_TOKEN` cannot have — hence "only when it can", and hence a filed-but-unboarded issue is a normal outcome rather than a failure. **Todo, not Incoming**, when the security sweep does place it: arriving through this pipeline *is* the approval. **`High` is a standing override** of the [priority rubric](.claude/skills/issue-triage/SKILL.md), which would otherwise score a routine bump Medium; the issue body records the override so it does not read as a mis-score. ⚠️ A milestone is a precondition for either — `Incoming` ⇔ no milestone — so an issue filed when no dated milestone is open is deliberately left unboarded rather than parked at Todo. ## Contributing diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index 1dd384b96..480d2174b 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -235,6 +235,22 @@ export function isDirectDependency(lock, pkg) { const SEVERITY_RANK = { critical: 4, high: 3, medium: 2, moderate: 2, low: 1 }; +/** + * The grouping key: one bump, i.e. one edit to one manifest. + * + * JSON rather than a delimited string, because the three fields are free-form + * and any separator would have to be argued for — the one that was here was a + * literal NUL, which classified the whole source file as binary and made + * repository searches skip it (Copilot). Shared with the end-of-run + * reconciliation, so a key built from a marker and a key built from an alert + * cannot drift apart. + * + * @returns {string} + */ +export function groupKey(pkg, manifestPath, fixedIn) { + return JSON.stringify([pkg, manifestPath, fixedIn]); +} + /** * Collapse per-advisory alerts into one entry per BUMP. * @@ -258,11 +274,7 @@ export function groupAlerts(alerts) { // for an unavailable upgrade is noise, so it waits for one to be published. if (!pkg || !manifestPath || !fixedIn) continue; - // JSON rather than a delimited string: the three fields are free-form, - // so any separator has to be argued for — and the one that was here was - // a literal NUL, which classified the whole source file as binary and - // made repository searches skip it (Copilot). - const key = JSON.stringify([pkg, manifestPath, fixedIn]); + const key = groupKey(pkg, manifestPath, fixedIn); const advisory = { ghsa: alert.security_advisory?.ghsa_id ?? "", cve: alert.security_advisory?.cve_id ?? null, @@ -394,6 +406,46 @@ const cell = (value) => String(value).replace(/\|/g, "\\|"); const PLACEMENT_DOC = "https://github.com/modelcontextprotocol/inspector/blob/v2/main/AGENTS.md#dependency-placement"; +/** + * The packages a nested copy sits under, outermost first. + * + * `node_modules/ajv/node_modules/fast-uri` -> `["ajv"]`. Scope-aware, since a + * scoped name contains a slash of its own. + * + * @param {string} path a lockfile `packages` key + * @returns {string[]} empty for the hoisted copy + */ +export function overrideAncestors(path) { + const segments = path.replace(/^node_modules\//, "").split("/node_modules/"); + return segments.slice(0, -1); +} + +/** + * A concrete parent-scoped `overrides` block for the nested vulnerable copies. + * + * npm rejects a package-wide override that contradicts a direct dependency of + * the same name (`EOVERRIDE`), so when the manifest declares the package the + * nested copies must be reached through their parents instead. + * + * @param {Array<{path: string, hoisted: boolean}>} affected + * @param {{package: string, fixedIn: string}} group + * @returns {string} pretty-printed JSON + */ +export function scopedOverrideExample(affected, group) { + const overrides = {}; + for (const entry of affected) { + const ancestors = overrideAncestors(entry.path); + if (ancestors.length === 0) continue; + let node = overrides; + for (const ancestor of ancestors) { + node[ancestor] = node[ancestor] ?? {}; + node = node[ancestor]; + } + node[group.package] = group.fixedIn; + } + return JSON.stringify({ overrides }, null, 2); +} + /** * What the maintainer actually has to change, derived from WHICH copies are * vulnerable rather than from whether the package is declared. @@ -460,7 +512,13 @@ export function buildIssueBody(group, { affected, declared, ghsas }) { } if (transitive) { steps.push( - `**Add an [\`overrides\`](${PLACEMENT_DOC}) entry** pinning \`${group.package}\` to \`${group.fixedIn}\`, for the ${direct ? "nested copies below, which the declared range does not reach" : "copies below, which no declared range reaches"}. **Not** \`npm audit fix\`, which "resolves" an advisory with no upward escape by silently downgrading.`, + direct + ? // ⚠️ A package-wide override cannot be used here: npm rejects an + // override whose spec differs from a direct dependency of the same + // name with EOVERRIDE, and this manifest declares one (Copilot). The + // nested copies have to be reached through their parents. + `**Add a parent-scoped [\`overrides\`](${PLACEMENT_DOC}) entry** for the nested copies below, which the declared range does not reach:\n\n\`\`\`json\n${scopedOverrideExample(affected, group)}\n\`\`\`\n\n A package-wide \`"${group.package}": "${group.fixedIn}"\` would be rejected with \`EOVERRIDE\`, because this manifest also declares \`${group.package}\` directly and npm refuses an override that contradicts a direct dependency. **Not** \`npm audit fix\` either, which "resolves" an advisory with no upward escape by silently downgrading.` + : `**Add an [\`overrides\`](${PLACEMENT_DOC}) entry** pinning \`${group.package}\` to \`${group.fixedIn}\`, for the copies below, which no declared range reaches. **Not** \`npm audit fix\`, which "resolves" an advisory with no upward escape by silently downgrading.`, ); } const fix = [ @@ -504,7 +562,9 @@ export function buildIssueBody(group, { affected, declared, ghsas }) { fix, "", "> [!NOTE]", - `> **Priority is a standing rubric override.** A routine bump scores Medium; a security bump is filed **${BOARD_PRIORITY}** so it does not sit. The version and severity above come from \`${TARGET_BRANCH}\`'s own lockfile, not from the alert — GitHub computes alerts from the default branch, so an alert is only filed here after its vulnerable range is re-checked against the branch we ship from.`, + `> **Priority is a standing rubric override.** A routine bump scores Medium; a security bump is filed **${BOARD_PRIORITY}** so it does not sit.`, + ">", + `> **Where each number comes from.** The GHSA, CVE, severity, vulnerable range and summary are the advisory's own, reported by Dependabot. What was verified independently against \`${TARGET_BRANCH}\` is the **installed versions, their paths, and whether each advisory's range still matches** — GitHub computes alerts from the default branch, so an alert is filed here only after that re-check.`, ].join("\n"); } @@ -562,7 +622,11 @@ export function buildNewAdvisoryComment(group, added) { .join("\n"); return [ buildCommentMarker(added), - `${added.length} new Dependabot ${added.length === 1 ? "advisory" : "advisories"} for \`${group.package}\`, cleared by the same bump to \`${group.fixedIn}\`. The issue body's marker now covers ${added.length === 1 ? "it" : "them"} too.`, + // ⚠️ Posted BEFORE the body edit, so it must not assert anything about the + // body's current state — the edit may not have happened yet, and may fail + // (Copilot). It speaks for the comment's own marker, which is true the + // moment this is posted. + `${added.length} new Dependabot ${added.length === 1 ? "advisory" : "advisories"} for \`${group.package}\`, cleared by the same bump to \`${group.fixedIn}\`. This comment's own marker records ${added.length === 1 ? "it" : "them"} as announced, so a later run will not repeat this even if the issue body has yet to catch up.`, "", "| GHSA | Severity | Summary |", "| --- | --- | --- |", @@ -923,11 +987,13 @@ export function main( checkSecurityPrsStillDisabled(repo, spawn); const groups = groupAlerts(openAlerts(repo, spawn)); - if (groups.length === 0) { - console.log("dependabot-alerts: no open alerts — no-op"); - return; - } + // ⚠️ Loaded BEFORE the zero-group early return, and reconciled after the loop. + // `openAlerts` asks for `state=open`, so an alert that is FIXED or DISMISSED + // simply vanishes from the feed — its group is never built, the loop never + // visits it, and the issue it produced would keep asserting a vulnerability + // with a live Todo/High card forever (Copilot). The disappearance is the + // signal, so it has to be read from the issues rather than from the alerts. const existingIssues = openDependabotIssues(repo, spawn).map((issue) => ({ ...issue, marker: parseMarker(issue.body), @@ -935,8 +1001,11 @@ export function main( const manifests = new Map(); const boardProblems = []; + /** Grouping keys this run actually saw in the open feed. */ + const seenKeys = new Set(); for (const rawGroup of groups) { + seenKeys.add(rawGroup.key); // ⚠️ Resolved BEFORE the skips below, not after. An issue filed yesterday // is still open today, and if the manifest has since gone or every copy has // moved out of range, skipping straight past it leaves its body asserting a @@ -1098,6 +1167,50 @@ export function main( ); } + // Any marked issue whose bump is no longer in the open feed at all: its last + // alert was fixed or dismissed, so there is nothing left to bump. + for (const issue of existingIssues) { + if (!issue.marker) continue; + const key = groupKey( + issue.marker.package, + issue.marker.manifestPath, + issue.marker.fixedIn, + ); + if (seenKeys.has(key)) continue; + const body = buildClearedBody( + { + package: issue.marker.package, + manifestPath: issue.marker.manifestPath, + fixedIn: issue.marker.fixedIn, + }, + { + ghsas: issue.marker.ghsas, + reason: "every alert it tracked has been fixed or dismissed", + today, + }, + ); + if (issue.body === body) continue; + const edit = gh(spawn, [ + "issue", + "edit", + String(issue.number), + "--repo", + repo, + "--body", + body, + ]); + if (edit.status !== 0) { + throw new Error(`gh issue edit failed: ${(edit.stderr ?? "").trim()}`); + } + console.log( + `dependabot-alerts: cleared #${issue.number} — no open alert remains for ${issue.marker.package}`, + ); + } + + if (groups.length === 0) { + console.log("dependabot-alerts: no open alerts"); + } + // Every group is processed before this throws: a half-placed card is worth // failing the run over, but not at the cost of the issues still unfiled. if (boardProblems.length > 0) { diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index 85c4bac04..7f3348799 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -15,7 +15,11 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; import { + PRIORITY_FIELD_ID, + STATUS_FIELD_ID, buildClearedBody, + overrideAncestors, + scopedOverrideExample, buildCommentMarker, pickMilestone, narrowToApplicable, @@ -467,7 +471,10 @@ test("buildIssueBody asks for BOTH edits when declared and nested copies are vul }); assert.match(body, /Both edits are needed/); assert.match(body, /1\. \*\*Raise the declared range/); - assert.match(body, /2\. \*\*Add an \[`overrides`\]/); + assert.match(body, /2\. \*\*Add a parent-scoped \[`overrides`\]/); + // A package-wide pin would be rejected: the manifest declares it directly. + assert.match(body, /EOVERRIDE/); + assert.match(body, /"ajv": \{\n\s+"fast-uri": "3\.1\.6"/); // The table names the copies, so the maintainer can see why. assert.match( body, @@ -795,11 +802,23 @@ test("main will not update an issue whose bump differs, even for the same packag }, ], }); - inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => - withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.5") }, + () => + withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ), ); assert.ok(ghCall(spawn, "create"), "a different bump gets its own issue"); - assert.equal(ghCall(spawn, "edit"), undefined); + // ...and the 3.1.6 issue, whose alert is no longer in the open feed, is + // cleared rather than left asserting a vulnerability nobody tracks. + const edit = ghCall(spawn, "edit"); + assert.ok(edit, "the superseded issue is reconciled"); + assert.match( + edit.args[edit.args.indexOf("--body") + 1], + /fixed or dismissed/, + ); + assert.ok(log.some((l) => l.includes("no open alert remains"))); }); test("main comments a new advisory BEFORE rewriting the marker", () => { @@ -1116,6 +1135,166 @@ test("buildIssueBody counts the applicable alerts in its prose, not the marker", assert.deepEqual(parseMarker(body).ghsas, ["GHSA-a", "GHSA-closed"]); }); +test("main clears an issue when its last alert is fixed or dismissed", () => { + // `openAlerts` asks for state=open, so a fixed alert simply vanishes and its + // group is never built. The issue has to be reconciled from the other side. + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [[]], + issues: [ + { + number: 41, + title: buildIssueTitle(group), + body: buildIssueBody(group, asInstalled()), + }, + ], + }); + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.5") }, + () => + withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ), + ); + const edit = ghCall(spawn, "edit"); + assert.ok(edit, "the zero-alert run still reconciles open issues"); + const body = edit.args[edit.args.indexOf("--body") + 1]; + assert.match(body, /every alert it tracked has been fixed or dismissed/); + assert.deepEqual(parseMarker(body).ghsas, ["GHSA-a"]); + assert.ok(log.some((l) => l.includes("no open alert remains"))); +}); + +test("main reconciles a vanished group even while other groups remain", () => { + // The early return is only half of it: a disappeared group is also never + // visited by the loop when the feed still has other bumps in it. + const [gone] = groupAlerts([ + alert({ ghsa: "GHSA-gone", pkg: "qs", fixed: "6.16.0" }), + ]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" })]], + issues: [ + { + number: 41, + title: buildIssueTitle(gone), + body: buildIssueBody(gone, { + affected: [ + { path: "node_modules/qs", version: "6.15.3", hoisted: true }, + ], + declared: false, + }), + }, + ], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ), + ); + assert.ok(ghCall(spawn, "create"), "the live bump is still filed"); + const edit = ghCall(spawn, "edit"); + assert.ok(edit, "the vanished bump's issue is still cleared"); + assert.equal(edit.args[2], "41"); +}); + +test("main does not re-clear a vanished group's issue on the next run", () => { + const spawn = fakeSpawn({ + alertPages: [[]], + issues: [ + { + number: 41, + title: + "chore(deps): bump `fast-uri` to `3.1.6` in `package-lock.json` (1 advisory)", + body: buildClearedBody( + { + package: "fast-uri", + manifestPath: "package-lock.json", + fixedIn: "3.1.6", + }, + { + ghsas: ["GHSA-a"], + reason: "every alert it tracked has been fixed or dismissed", + today: "2026-09-04", + }, + ), + }, + ], + }); + inTempRepo({}, () => + withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ), + ); + assert.equal(ghCall(spawn, "edit"), undefined); +}); + +test("main boards a filed issue at Todo/High using resolved option ids", () => { + // The acceptance-critical path: the two field edits that actually place the + // card. Previously only its failure modes were covered. + const spawn = fakeSpawn({ alertPages: [[alert({ ghsa: "GHSA-a" })]] }); + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.5") }, + () => withProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + + const add = spawn.calls.find( + (c) => c.args[0] === "project" && c.args[1] === "item-add", + ); + assert.ok(add, "the card is added"); + const edits = spawn.calls.filter( + (c) => c.args[0] === "project" && c.args[1] === "item-edit", + ); + assert.equal(edits.length, 2, "Status and Priority are separate calls"); + const optionOf = (call) => + call.args[call.args.indexOf("--single-select-option-id") + 1]; + const fieldOf = (call) => call.args[call.args.indexOf("--field-id") + 1]; + assert.deepEqual( + edits.map((e) => [fieldOf(e), optionOf(e)]), + [ + [STATUS_FIELD_ID, "todo-id"], + [PRIORITY_FIELD_ID, "high-id"], + ], + ); + // Resolved by NAME at run time, never hardcoded. + assert.ok( + spawn.calls.some( + (c) => c.args[0] === "project" && c.args[1] === "field-list", + ), + ); + assert.ok(log.some((l) => l.includes("boarded"))); +}); + +test("overrideAncestors reads the parent chain, scoped names included", () => { + assert.deepEqual( + overrideAncestors("node_modules/ajv/node_modules/fast-uri"), + ["ajv"], + ); + assert.deepEqual( + overrideAncestors( + "node_modules/@sc/a/node_modules/b/node_modules/fast-uri", + ), + ["@sc/a", "b"], + ); + assert.deepEqual(overrideAncestors("node_modules/fast-uri"), []); +}); + +test("scopedOverrideExample nests each vulnerable copy under its parents", () => { + const json = scopedOverrideExample( + [ + { path: "node_modules/fast-uri", hoisted: true }, + { path: "node_modules/ajv/node_modules/fast-uri", hoisted: false }, + { path: "node_modules/@sc/x/node_modules/fast-uri", hoisted: false }, + ], + { package: "fast-uri", fixedIn: "3.1.6" }, + ); + // The hoisted copy is the declared one and gets no override entry. + assert.deepEqual(JSON.parse(json), { + overrides: { + ajv: { "fast-uri": "3.1.6" }, + "@sc/x": { "fast-uri": "3.1.6" }, + }, + }); +}); + test("main reads every page of open dependabot issues", () => { // The second page holds the matching marker. Truncating the lookup would // file a duplicate issue rather than recognising this one. From 233647163ffa5790e74f13291e1828aef94c69f1 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 08:09:45 -0400 Subject: [PATCH 10/14] chore(deps): address Copilot review round 11 on #2243 - 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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- AGENTS.md | 3 +- README.md | 1 + scripts/dependabot-alerts.mjs | 73 ++++++++++++++--- scripts/dependabot-alerts.test.mjs | 122 ++++++++++++++++++++++++++++- 4 files changed, 187 insertions(+), 12 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 71288c60b..34487261e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -52,7 +52,8 @@ inspector/ │ ├── react/ React hooks over the state stores (read during render — see React instructions) │ └── storage/ File I/O helpers for the OAuth persist backends ├── test-servers/ Composable MCP test servers + JSON configs -├── scripts/ Root build/verify tooling: install cascade, smokes, verify:* guards +├── scripts/ Root build/verify tooling (install cascade, smokes, verify:* guards) +│ plus repo automation run from CI (the dependency + alert sweeps) ├── docs/ Task-oriented guides ├── specification/ Design/build specifications └── .claude/skills/ The procedures (see the index above) diff --git a/README.md b/README.md index 4593d217c..42ae110b0 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,7 @@ inspector/ ├── core/ Shared code consumed via the `@inspector/core` alias (no package.json) ├── test-servers/ Composable MCP test servers + fixtures used by integration and smoke tests ├── scripts/ Root build/verify tooling (install cascade, smokes, the verify:* guards) +│ and repo automation run from CI (the dependency and Dependabot-alert sweeps) ├── docs/ Task-oriented guides — see below ├── specification/ Design/build specifications ├── .claude/skills/ Agent skills: the repo's procedures, invokable by name diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index 480d2174b..8f3159f05 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -79,6 +79,19 @@ export const BOARD_PRIORITY = "High"; */ export const TARGET_BRANCH = "v2/main"; +/** + * The one ecosystem this sweep can act on. + * + * ⚠️ Dependabot alerts are NOT npm-only. This repo has a `Dockerfile` and + * GitHub Actions workflows, and an alert against either arrives in the same + * feed with a `manifest_path` that is not a lockfile — which the JSON parse + * would reject, aborting the whole daily sweep before any npm group was + * processed (Copilot). Everything downstream reads npm lockfiles, so a non-npm + * alert is reported and skipped rather than guessed at: filing it properly + * means knowing how to fix it, which is different work per ecosystem. + */ +export const SUPPORTED_ECOSYSTEM = "npm"; + const MARKER_RE = /^/; @@ -267,6 +280,7 @@ export function groupAlerts(alerts) { for (const alert of alerts) { if (alert.state !== "open") continue; const pkg = alert.dependency?.package?.name; + const ecosystem = alert.dependency?.package?.ecosystem ?? "unknown"; const manifestPath = alert.dependency?.manifest_path; const fixedIn = alert.security_vulnerability?.first_patched_version?.identifier; @@ -298,6 +312,7 @@ export function groupAlerts(alerts) { groups.set(key, { key, package: pkg, + ecosystem, manifestPath, fixedIn, scope: alert.dependency?.scope ?? "runtime", @@ -485,7 +500,10 @@ export function remediation(affected, declared) { * rewritten to cover advisories it did not originally name. * @returns {string} */ -export function buildIssueBody(group, { affected, declared, ghsas }) { +export function buildIssueBody( + group, + { affected, declared, ghsas, securityPrsOff = true }, +) { const covered = ghsas ?? group.ghsas; const applying = group.advisories.length; // ⚠️ Every free-form cell is escaped, the RANGE included: a semver range is @@ -540,7 +558,11 @@ export function buildIssueBody(group, { affected, declared, ghsas }) { // Counts what APPLIES, like the title and the table — `covered` is the // marker's monotonic history and would keep counting an advisory that has // since closed (Copilot). - `Filed automatically from ${applying} open Dependabot ${applying === 1 ? "alert" : "alerts"} (#2233). Dependabot opens no security-update PRs on this repo; the fix is written by hand against \`${TARGET_BRANCH}\`.`, + // ⚠️ The security-PR claim is only made when the run actually READ the + // setting. The guard degrades to UNVERIFIED when the token cannot see it, + // and an issue asserting what the run explicitly could not confirm is worse + // than one that says so (Copilot). + `Filed automatically from ${applying} open Dependabot ${applying === 1 ? "alert" : "alerts"} (#2233). ${securityPrsOff ? "Dependabot opens no security-update PRs on this repo; the" : "The"} fix is written by hand against \`${TARGET_BRANCH}\`.`, "", "| | |", "| --- | --- |", @@ -750,12 +772,23 @@ function openAlerts(repo, spawn) { * against a manifest this branch does not have is not actionable. */ function readManifest(manifestPath) { + let raw; try { - return JSON.parse(readFileSync(manifestPath, "utf8")); + raw = readFileSync(manifestPath, "utf8"); } catch (error) { if (error.code === "ENOENT") return null; throw error; } + try { + return JSON.parse(raw); + } catch { + // Belt and braces behind the ecosystem filter: whatever this is, it is not + // an npm lockfile, and one unparseable manifest must not abort the sweep. + console.log( + `dependabot-alerts: ${manifestPath} is not JSON — skipping (not an npm lockfile)`, + ); + return null; + } } /** @@ -984,7 +1017,7 @@ export function main( ) { if (!repo) throw new Error("repo not specified (GITHUB_REPOSITORY unset)"); - checkSecurityPrsStillDisabled(repo, spawn); + const securityPrsOff = checkSecurityPrsStillDisabled(repo, spawn); const groups = groupAlerts(openAlerts(repo, spawn)); @@ -1003,9 +1036,20 @@ export function main( const boardProblems = []; /** Grouping keys this run actually saw in the open feed. */ const seenKeys = new Set(); + /** Every GHSA still open, in ANY group — the check a vanished key needs. */ + const openGhsas = new Set(groups.flatMap((g) => g.ghsas)); for (const rawGroup of groups) { seenKeys.add(rawGroup.key); + + if (rawGroup.ecosystem !== SUPPORTED_ECOSYSTEM) { + // Loud, not silent: nothing else will file this, so a human has to. + console.log( + `dependabot-alerts: ${rawGroup.package} (${rawGroup.ecosystem}, ${rawGroup.manifestPath}) is not an npm dependency — this sweep cannot file it, raise it by hand: ${rawGroup.ghsas.join(", ")}`, + ); + continue; + } + // ⚠️ Resolved BEFORE the skips below, not after. An issue filed yesterday // is still open today, and if the manifest has since gone or every copy has // moved out of range, skipping straight past it leaves its body asserting a @@ -1079,7 +1123,7 @@ export function main( const { url, milestone } = createIssue( repo, group, - buildIssueBody(group, { affected, declared }), + buildIssueBody(group, { affected, declared, securityPrsOff }), spawn, ); // `Incoming` <=> no milestone, everything past it <=> milestoned. With no @@ -1103,6 +1147,7 @@ export function main( affected, declared, ghsas: merged, + securityPrsOff, }); // ⚠️ "Nothing NEW" is not the same as "nothing CHANGED" (Copilot). An issue @@ -1177,17 +1222,25 @@ export function main( issue.marker.fixedIn, ); if (seenKeys.has(key)) continue; + + // ⚠️ A vanished KEY is not the same as a closed ADVISORY. GitHub can revise + // an alert's `first_patched_version`, which moves it to a different key + // while the GHSA stays open — reporting that as "fixed or dismissed" would + // stand down a live exposure (Copilot). So the reason is decided by whether + // the GHSAs are still in the open feed, not by the key's absence. + 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"; + const body = buildClearedBody( { package: issue.marker.package, manifestPath: issue.marker.manifestPath, fixedIn: issue.marker.fixedIn, }, - { - ghsas: issue.marker.ghsas, - reason: "every alert it tracked has been fixed or dismissed", - today, - }, + { ghsas: issue.marker.ghsas, reason, today }, ); if (issue.body === body) continue; const edit = gh(spawn, [ diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index 7f3348799..f8325b22e 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -15,6 +15,7 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; import { + SUPPORTED_ECOSYSTEM, PRIORITY_FIELD_ID, STATUS_FIELD_ID, buildClearedBody, @@ -51,12 +52,13 @@ function alert({ scope = "runtime", cve = null, state = "open", + ecosystem = "npm", }) { return { state, html_url: `https://github.com/o/r/security/dependabot/${ghsa}`, dependency: { - package: { name: pkg }, + package: { name: pkg, ecosystem }, manifest_path: manifest, scope, }, @@ -1295,6 +1297,124 @@ test("scopedOverrideExample nests each vulnerable copy under its parents", () => }); }); +test("groupAlerts records the ecosystem so non-npm alerts are identifiable", () => { + const [docker] = groupAlerts([ + alert({ + ghsa: "GHSA-d", + pkg: "node", + manifest: "Dockerfile", + ecosystem: "docker", + }), + ]); + assert.equal(docker.ecosystem, "docker"); + assert.notEqual(docker.ecosystem, SUPPORTED_ECOSYSTEM); +}); + +test("main skips a non-npm alert loudly instead of crashing on its manifest", () => { + // ⚠️ This repo has a Dockerfile, so this is reachable. Parsing it as a + // lockfile threw and aborted the entire sweep before any npm group ran. + const spawn = fakeSpawn({ + alertPages: [ + [ + alert({ + ghsa: "GHSA-docker", + pkg: "node", + manifest: "Dockerfile", + ecosystem: "docker", + }), + alert({ ghsa: "GHSA-a" }), + ], + ], + }); + const log = inTempRepo( + { + "package-lock.json": lockWith("fast-uri", "3.1.5"), + }, + () => withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + + // The npm bump is still filed — the non-npm alert must not abort the run. + const created = ghCalls(spawn, "create"); + assert.equal(created.length, 1); + assert.match( + created[0].args[created[0].args.indexOf("--title") + 1], + /`fast-uri`/, + ); + // ...and the skipped one is named, with its GHSA, so a human can file it. + assert.ok( + log.some( + (l) => + l.includes("docker") && + l.includes("Dockerfile") && + l.includes("GHSA-docker") && + l.includes("raise it by hand"), + ), + `expected a loud skip line, got: ${log.join(" | ")}`, + ); +}); + +test("main says superseded, not fixed, when the GHSA is still open elsewhere", () => { + // GitHub revised `first_patched_version`, so the advisory moved to a new key + // while staying open. Calling that "fixed or dismissed" would stand down a + // live exposure. + const [filed] = groupAlerts([alert({ ghsa: "GHSA-a", fixed: "3.1.6" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a", fixed: "3.1.7", range: "< 3.1.7" })]], + issues: [ + { + number: 41, + title: buildIssueTitle(filed), + body: buildIssueBody(filed, asInstalled()), + }, + ], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ), + ); + const edit = ghCall(spawn, "edit"); + assert.ok(edit); + const body = edit.args[edit.args.indexOf("--body") + 1]; + assert.match(body, /superseded/); + assert.match(body, /`GHSA-a` is still open/); + assert.doesNotMatch(body, /fixed or dismissed/); + // ...and the new bump gets its own issue. + assert.ok(ghCall(spawn, "create")); +}); + +test("buildIssueBody does not claim security PRs are off when unverified", () => { + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const verified = buildIssueBody(group, nested()); + assert.match(verified, /Dependabot opens no security-update PRs/); + + const unverified = buildIssueBody(group, { + ...nested(), + securityPrsOff: false, + }); + assert.doesNotMatch(unverified, /opens no security-update PRs/); + assert.match(unverified, /The fix is written by hand/); +}); + +test("main omits the security-PR claim when the token could not read it", () => { + const spawn = fakeSpawn({ + securityFixesStatus: 1, + securityFixesStderr: "gh: HTTP 403: Resource not accessible by integration", + alertPages: [[alert({ ghsa: "GHSA-a" })]], + }); + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.5") }, + () => withoutProjectToken(() => captureLog(() => main("o/r", spawn))), + ); + assert.ok(log.some((l) => l.includes("UNVERIFIED"))); + const create = ghCall(spawn, "create"); + assert.ok(create); + assert.doesNotMatch( + create.args[create.args.indexOf("--body") + 1], + /opens no security-update PRs/, + ); +}); + test("main reads every page of open dependabot issues", () => { // The second page holds the matching marker. Truncating the lookup would // file a duplicate issue rather than recognising this one. From bc47fcbcf0485dcf600fdb399be8ae78ec3015b8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 08:22:18 -0400 Subject: [PATCH 11/14] chore(deps): address Copilot review round 12 on #2243 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- scripts/dependabot-alerts.mjs | 22 ++++++++++++++++--- scripts/dependabot-alerts.test.mjs | 35 ++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index 8f3159f05..4d388bc6d 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -1019,7 +1019,8 @@ export function main( const securityPrsOff = checkSecurityPrsStillDisabled(repo, spawn); - const groups = groupAlerts(openAlerts(repo, spawn)); + const alerts = openAlerts(repo, spawn); + const groups = groupAlerts(alerts); // ⚠️ Loaded BEFORE the zero-group early return, and reconciled after the loop. // `openAlerts` asks for `state=open`, so an alert that is FIXED or DISMISSED @@ -1036,8 +1037,23 @@ export function main( const boardProblems = []; /** Grouping keys this run actually saw in the open feed. */ const seenKeys = new Set(); - /** Every GHSA still open, in ANY group — the check a vanished key needs. */ - const openGhsas = new Set(groups.flatMap((g) => g.ghsas)); + /** + * Every GHSA still open, taken from the RAW feed rather than from `groups`. + * + * ⚠️ `groupAlerts` deliberately drops an alert with no `first_patched_version` + * — there is nothing to bump to, so nothing to file. Building this set from + * the groups would inherit that filter, so an advisory that stays open but + * LOSES its patched version would vanish from both the keys and this set, and + * reconciliation would call it "fixed or dismissed" (Copilot). Same wrong + * direction as the superseded case, reached a different way: what a still-open + * advisory must never do is stand itself down. + */ + const openGhsas = new Set( + alerts + .filter((a) => a.state === "open") + .map((a) => a.security_advisory?.ghsa_id) + .filter(Boolean), + ); for (const rawGroup of groups) { seenKeys.add(rawGroup.key); diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index f8325b22e..499ff580d 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -1415,6 +1415,41 @@ test("main omits the security-PR claim when the token could not read it", () => ); }); +test("main will not stand down an open alert that lost its patched version", () => { + // groupAlerts drops an alert with no first_patched_version, so the bump's key + // disappears — but the advisory is still OPEN. Reading the open-GHSA set from + // the groups instead of the raw feed would report it as fixed or dismissed. + const [filed] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const unpatched = alert({ ghsa: "GHSA-a" }); + unpatched.security_vulnerability.first_patched_version = null; + + const spawn = fakeSpawn({ + alertPages: [[unpatched]], + issues: [ + { + number: 41, + title: buildIssueTitle(filed), + body: buildIssueBody(filed, asInstalled()), + }, + ], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ), + ); + const edit = ghCall(spawn, "edit"); + assert.ok(edit, "the issue is still reconciled"); + const body = edit.args[edit.args.indexOf("--body") + 1]; + assert.doesNotMatch( + body, + /fixed or dismissed/, + "a still-open advisory must never stand itself down", + ); + assert.match(body, /superseded/); + assert.match(body, /`GHSA-a` is still open/); +}); + test("main reads every page of open dependabot issues", () => { // The second page holds the matching marker. Truncating the lookup would // file a duplicate issue rather than recognising this one. From c0d1d567824033ece086b5723dd9084352a2f69d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 08:45:14 -0400 Subject: [PATCH 12/14] chore(deps): address Copilot review round 13 on #2243 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- scripts/dependabot-alerts.mjs | 149 ++++++++++++++++++++--------- scripts/dependabot-alerts.test.mjs | 138 +++++++++++++++++++++++--- 2 files changed, 228 insertions(+), 59 deletions(-) diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index 4d388bc6d..f041708db 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -603,6 +603,27 @@ export function buildIssueBody( * @param {{ghsas: string[], reason: string, today: string}} context * @returns {string} */ +/** + * The date an already-cleared body records, or `null` if it is not one. + * + * ⚠️ A cleared issue is deliberately left OPEN, so the sweep sees it again + * tomorrow. With today's date baked into the rendered body, the regenerated + * body would differ by the date alone and every cleared issue would be edited + * once a day, forever (Copilot). Reusing the original date is what makes the + * comparison stable — and it is the more useful date to show anyway: when the + * exposure went away, not when the sweep last looked. + * + * @param {string | undefined} body + * @returns {string | null} + */ +export function parseClearedDate(body) { + const match = + /\*\*No longer applicable on `[^`]+` as of (\d{4}-\d{2}-\d{2})\*\*/.exec( + body ?? "", + ); + return match ? match[1] : null; +} + export function buildClearedBody(group, { ghsas, reason, today }) { return [ buildMarker({ ...group, ghsas }), @@ -771,23 +792,31 @@ function openAlerts(repo, spawn) { * A manifest's contents in the checkout, or `null` when it is absent — an alert * against a manifest this branch does not have is not actionable. */ +/** + * Read a manifest, distinguishing the two ways it can fail to produce a lock. + * + * ⚠️ These must NOT collapse into one `null` (Copilot). "Absent" means the + * manifest is genuinely gone from the branch, which is real evidence that the + * exposure went away and is grounds for clearing the issue. "Unparseable" is + * evidence of nothing at all — a malformed or truncated lockfile, or a + * non-npm manifest — and clearing on it would stand down a live alert on the + * strength of a read error. + * + * @returns {{lock: object} | {absent: true} | {unparseable: true}} + */ function readManifest(manifestPath) { let raw; try { raw = readFileSync(manifestPath, "utf8"); } catch (error) { - if (error.code === "ENOENT") return null; + if (error.code === "ENOENT") return { absent: true }; throw error; } try { - return JSON.parse(raw); + return { lock: JSON.parse(raw) }; } catch { - // Belt and braces behind the ecosystem filter: whatever this is, it is not - // an npm lockfile, and one unparseable manifest must not abort the sweep. - console.log( - `dependabot-alerts: ${manifestPath} is not JSON — skipping (not an npm lockfile)`, - ); - return null; + // One unreadable manifest must not abort the sweep either. + return { unparseable: true }; } } @@ -1035,6 +1064,39 @@ export function main( const manifests = new Map(); const boardProblems = []; + + /** + * Rewrite an issue to its cleared state, at most once. + * + * The date is taken from the body already there when there is one, so a + * cleared issue — which stays open, and so is seen again tomorrow — does not + * get re-edited every day for a date change alone. + */ + const writeCleared = (issue, group, reason) => { + const priorDate = parseClearedDate(issue.body); + const ghsas = issue.marker.ghsas; + if ( + priorDate && + issue.body === + buildClearedBody(group, { ghsas, reason, today: priorDate }) + ) { + return; + } + const body = buildClearedBody(group, { ghsas, reason, today }); + const edit = gh(spawn, [ + "issue", + "edit", + String(issue.number), + "--repo", + repo, + "--body", + body, + ]); + if (edit.status !== 0) { + throw new Error(`gh issue edit failed: ${(edit.stderr ?? "").trim()}`); + } + console.log(`dependabot-alerts: cleared #${issue.number} — ${reason}`); + }; /** Grouping keys this run actually saw in the open feed. */ const seenKeys = new Set(); /** @@ -1048,6 +1110,8 @@ export function main( * direction as the superseded case, reached a different way: what a still-open * advisory must never do is stand itself down. */ + /** GHSAs that made it into a group, i.e. ones this sweep can actually file. */ + const filableGhsas = new Set(groups.flatMap((g) => g.ghsas)); const openGhsas = new Set( alerts .filter((a) => a.state === "open") @@ -1082,38 +1146,29 @@ export function main( /** Rewrite an open issue to its cleared state, once. */ const clear = (reason) => { if (!existing) return; - const body = buildClearedBody(rawGroup, { - ghsas: existing.marker.ghsas, - reason, - today, - }); - if (existing.body === body) return; - const edit = gh(spawn, [ - "issue", - "edit", - String(existing.number), - "--repo", - repo, - "--body", - body, - ]); - if (edit.status !== 0) { - throw new Error(`gh issue edit failed: ${(edit.stderr ?? "").trim()}`); - } - console.log(`dependabot-alerts: cleared #${existing.number} — ${reason}`); + writeCleared(existing, rawGroup, reason); }; if (!manifests.has(rawGroup.manifestPath)) { manifests.set(rawGroup.manifestPath, readManifest(rawGroup.manifestPath)); } - const lock = manifests.get(rawGroup.manifestPath); - if (lock === null) { + const manifest = manifests.get(rawGroup.manifestPath); + if (manifest.unparseable) { + // Deliberately does NOT clear: a read error is not evidence that the + // exposure went away, and treating it as such stands down a live alert. + console.log( + `dependabot-alerts: ${rawGroup.manifestPath} could not be parsed as an npm lockfile — skipping ${rawGroup.package} WITHOUT clearing its issue`, + ); + continue; + } + if (manifest.absent) { console.log( `dependabot-alerts: ${rawGroup.manifestPath} absent on ${TARGET_BRANCH} — skipping ${rawGroup.package}`, ); clear(`\`${rawGroup.manifestPath}\` is no longer part of this repo`); continue; } + const { lock } = manifest; const entries = lockfileEntries(lock, rawGroup.package); const applicable = narrowToApplicable(rawGroup, entries); @@ -1245,34 +1300,34 @@ export function main( // stand down a live exposure (Copilot). So the reason is decided by whether // the GHSAs are still in the open feed, not by the key's absence. const stillOpen = issue.marker.ghsas.filter((g) => openGhsas.has(g)); + + // ⚠️ Three states, not two. An advisory can be open and yet absent from + // every group, because `groupAlerts` drops one with no + // `first_patched_version` — there is nothing to bump to. Such an advisory + // has NO replacement issue, so calling it "superseded" would be false and + // clearing it would stand down a live exposure with nothing tracking it + // (Copilot). Leave the issue exactly as it is and say so. + const unpatched = stillOpen.filter((g) => !filableGhsas.has(g)); + if (unpatched.length > 0) { + console.log( + `dependabot-alerts: #${issue.number} left as is — ${unpatched.join(", ")} ${unpatched.length === 1 ? "is" : "are"} still open with no patched version to bump to`, + ); + continue; + } + 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"; - const body = buildClearedBody( + writeCleared( + issue, { package: issue.marker.package, manifestPath: issue.marker.manifestPath, fixedIn: issue.marker.fixedIn, }, - { ghsas: issue.marker.ghsas, reason, today }, - ); - if (issue.body === body) continue; - const edit = gh(spawn, [ - "issue", - "edit", - String(issue.number), - "--repo", - repo, - "--body", - body, - ]); - if (edit.status !== 0) { - throw new Error(`gh issue edit failed: ${(edit.stderr ?? "").trim()}`); - } - console.log( - `dependabot-alerts: cleared #${issue.number} — no open alert remains for ${issue.marker.package}`, + reason, ); } diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index 499ff580d..18d66ad4f 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -15,6 +15,7 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; import { + parseClearedDate, SUPPORTED_ECOSYSTEM, PRIORITY_FIELD_ID, STATUS_FIELD_ID, @@ -820,7 +821,7 @@ test("main will not update an issue whose bump differs, even for the same packag edit.args[edit.args.indexOf("--body") + 1], /fixed or dismissed/, ); - assert.ok(log.some((l) => l.includes("no open alert remains"))); + assert.ok(log.some((l) => l.includes("fixed or dismissed"))); }); test("main comments a new advisory BEFORE rewriting the marker", () => { @@ -1163,7 +1164,7 @@ test("main clears an issue when its last alert is fixed or dismissed", () => { const body = edit.args[edit.args.indexOf("--body") + 1]; assert.match(body, /every alert it tracked has been fixed or dismissed/); assert.deepEqual(parseMarker(body).ghsas, ["GHSA-a"]); - assert.ok(log.some((l) => l.includes("no open alert remains"))); + assert.ok(log.some((l) => l.includes("fixed or dismissed"))); }); test("main reconciles a vanished group even while other groups remain", () => { @@ -1433,21 +1434,134 @@ test("main will not stand down an open alert that lost its patched version", () }, ], }); - inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + const log = inTempRepo( + { "package-lock.json": lockWith("fast-uri", "3.1.5") }, + () => + withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ), + ); + // Nothing to bump to means no replacement issue exists, so "superseded" would + // be false and clearing would stand down a live exposure nothing is tracking. + // The correct move is to leave the issue exactly as it is. + assert.equal(ghCall(spawn, "edit"), undefined); + assert.ok( + log.some( + (l) => + l.includes("left as is") && + l.includes("GHSA-a") && + l.includes("no patched version"), + ), + `expected a left-as-is line, got: ${log.join(" | ")}`, + ); +}); + +test("a cleared issue is not re-edited on a LATER day", () => { + // ⚠️ The bug the same-date no-reclear test could never catch: a cleared issue + // stays open, so the sweep sees it again tomorrow. With today's date rendered + // into the body, every cleared issue would be edited once a day forever. + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const cleared = buildClearedBody( + { + package: "fast-uri", + manifestPath: "package-lock.json", + fixedIn: "3.1.6", + }, + { + ghsas: ["GHSA-a"], + reason: "every alert it tracked has been fixed or dismissed", + today: "2026-09-04", + }, + ); + const spawn = fakeSpawn({ + alertPages: [[]], + issues: [{ number: 41, title: buildIssueTitle(group), body: cleared }], + }); + inTempRepo({}, () => withoutProjectToken(() => - captureLog(() => main("o/r", spawn, "2026-09-04")), + // A DIFFERENT day from the one the body records. + captureLog(() => main("o/r", spawn, "2026-09-11")), + ), + ); + assert.equal(ghCall(spawn, "edit"), undefined); +}); + +test("a cleared issue keeps its original date when its reason changes", () => { + // A real change still gets one edit — and takes the new date, since the state + // genuinely changed on that day. + const cleared = buildClearedBody( + { + package: "fast-uri", + manifestPath: "package-lock.json", + fixedIn: "3.1.6", + }, + { ghsas: ["GHSA-a"], reason: "an older reason", today: "2026-09-04" }, + ); + const spawn = fakeSpawn({ + alertPages: [[]], + issues: [{ number: 41, title: "t", body: cleared }], + }); + inTempRepo({}, () => + withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-11")), ), ); const edit = ghCall(spawn, "edit"); - assert.ok(edit, "the issue is still reconciled"); - const body = edit.args[edit.args.indexOf("--body") + 1]; - assert.doesNotMatch( - body, - /fixed or dismissed/, - "a still-open advisory must never stand itself down", + assert.ok(edit, "a changed reason is still written"); + assert.equal( + parseClearedDate(edit.args[edit.args.indexOf("--body") + 1]), + "2026-09-11", + ); +}); + +test("parseClearedDate reads the date back, and only from a cleared body", () => { + const body = buildClearedBody( + { package: "p", manifestPath: "package-lock.json", fixedIn: "1.0.0" }, + { ghsas: ["GHSA-a"], reason: "why", today: "2026-09-04" }, + ); + assert.equal(parseClearedDate(body), "2026-09-04"); + assert.equal(parseClearedDate("an ordinary issue body"), null); + assert.equal(parseClearedDate(undefined), null); +}); + +test("main does not clear an issue when the lockfile cannot be parsed", () => { + // A malformed lockfile is evidence of nothing. Treating it like an absent one + // would stand down a live alert on the strength of a read error. + const [group] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a" })]], + issues: [ + { + number: 41, + title: buildIssueTitle(group), + body: buildIssueBody(group, asInstalled()), + }, + ], + }); + + const dir = mkdtempSync(join(tmpdir(), "dependabot-alerts-")); + const cwd = process.cwd(); + let log; + try { + writeFileSync(join(dir, "package-lock.json"), "{ truncated…"); + process.chdir(dir); + log = withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ); + } finally { + process.chdir(cwd); + rmSync(dir, { recursive: true, force: true }); + } + + assert.equal(ghCall(spawn, "edit"), undefined, "the issue is left alone"); + assert.equal(ghCall(spawn, "create"), undefined); + assert.ok( + log.some( + (l) => + l.includes("could not be parsed") && l.includes("WITHOUT clearing"), + ), + `expected a parse-failure line, got: ${log.join(" | ")}`, ); - assert.match(body, /superseded/); - assert.match(body, /`GHSA-a` is still open/); }); test("main reads every page of open dependabot issues", () => { From e846dec491215fe8dfffc3ef613542ed827369c2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 09:00:13 -0400 Subject: [PATCH 13/14] chore(deps): address Copilot review round 14 on #2243 - 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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- AGENTS.md | 2 +- scripts/dependabot-alerts.mjs | 52 +++++++++++++++++++++++---- scripts/dependabot-alerts.test.mjs | 58 +++++++++++++++++++++++++++++- 3 files changed, 103 insertions(+), 9 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 34487261e..d5103dd71 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -122,7 +122,7 @@ An issue filed by either sweep is an ordinary board item — `v2` + `chore` + `d | Monthly version sweep | **No, never.** It does not attempt a board write at all and has no `PROJECT_TOKEN`; the issue arrives labeled and milestoned, and `/issue-triage` places it. | | Daily security sweep | **Only when it can.** With an org-project PAT it places the card directly at **Todo / High**; without one it degrades to the same triage hand-off. | -The board write needs `organization projects: write`, which `GITHUB_TOKEN` cannot have — hence "only when it can", and hence a filed-but-unboarded issue is a normal outcome rather than a failure. **Todo, not Incoming**, when the security sweep does place it: arriving through this pipeline *is* the approval. **`High` is a standing override** of the [priority rubric](.claude/skills/issue-triage/SKILL.md), which would otherwise score a routine bump Medium; the issue body records the override so it does not read as a mis-score. ⚠️ A milestone is a precondition for either — `Incoming` ⇔ no milestone — so an issue filed when no dated milestone is open is deliberately left unboarded rather than parked at Todo. +The board write needs `organization projects: write`, which `GITHUB_TOKEN` cannot have — hence "only when it can", and hence a filed-but-unboarded issue is a normal outcome rather than a failure. **Todo, not Incoming**, when the security sweep does place it: arriving through this pipeline *is* the approval. **`High` is a standing override** of the [priority rubric](.claude/skills/issue-triage/SKILL.md), which would otherwise score a routine bump Medium; the issue body records the override so it does not read as a mis-score. ⚠️ A milestone is a precondition for placing a card — `Incoming` ⇔ no milestone — so the security sweep leaves an issue **unboarded** rather than parked at Todo when no dated milestone is open. It picks the open milestone with the nearest **due date**, ignoring undated buckets; the monthly sweep's own selection does not yet filter those out (raised on #2239), so don't read this as a guarantee both scripts already implement. ## Contributing diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index f041708db..067b7ec7a 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -264,6 +264,22 @@ export function groupKey(pkg, manifestPath, fixedIn) { return JSON.stringify([pkg, manifestPath, fixedIn]); } +/** + * One advisory as it applies to one manifest. + * + * ⚠️ A GHSA alone is not enough. Dependabot alerts are per MANIFEST, and this + * repo has five lockfiles — so the same advisory legitimately appears for the + * root install and for a client. Keying reconciliation on the GHSA alone lets + * another manifest's still-filable alert vouch for this one, and an issue whose + * own alert lost its patched version would be cleared on the strength of a + * different lockfile's alert (Copilot). + * + * @returns {string} + */ +export function advisoryKey(pkg, manifestPath, ghsa) { + return JSON.stringify([pkg, manifestPath, ghsa]); +} + /** * Collapse per-advisory alerts into one entry per BUMP. * @@ -1100,7 +1116,8 @@ export function main( /** Grouping keys this run actually saw in the open feed. */ const seenKeys = new Set(); /** - * Every GHSA still open, taken from the RAW feed rather than from `groups`. + * Every open `(package, manifest, GHSA)`, taken from the RAW feed rather + * than from `groups`. * * ⚠️ `groupAlerts` deliberately drops an alert with no `first_patched_version` * — there is nothing to bump to, so nothing to file. Building this set from @@ -1110,12 +1127,29 @@ export function main( * direction as the superseded case, reached a different way: what a still-open * advisory must never do is stand itself down. */ - /** GHSAs that made it into a group, i.e. ones this sweep can actually file. */ - const filableGhsas = new Set(groups.flatMap((g) => g.ghsas)); - const openGhsas = new Set( + /** + * `(package, manifest, GHSA)` tuples that made it into a group — the ones + * this sweep can actually file a bump for. + */ + const filableAdvisories = new Set( + groups.flatMap((g) => + g.ghsas.map((ghsa) => advisoryKey(g.package, g.manifestPath, ghsa)), + ), + ); + const openAdvisories = new Set( alerts .filter((a) => a.state === "open") - .map((a) => a.security_advisory?.ghsa_id) + .map((a) => + a.dependency?.package?.name && + a.dependency?.manifest_path && + a.security_advisory?.ghsa_id + ? advisoryKey( + a.dependency.package.name, + a.dependency.manifest_path, + a.security_advisory.ghsa_id, + ) + : null, + ) .filter(Boolean), ); @@ -1299,7 +1333,11 @@ export function main( // while the GHSA stays open — reporting that as "fixed or dismissed" would // stand down a live exposure (Copilot). So the reason is decided by whether // the GHSAs are still in the open feed, not by the key's absence. - const stillOpen = issue.marker.ghsas.filter((g) => openGhsas.has(g)); + const key3 = (ghsa) => + advisoryKey(issue.marker.package, issue.marker.manifestPath, ghsa); + const stillOpen = issue.marker.ghsas.filter((g) => + openAdvisories.has(key3(g)), + ); // ⚠️ Three states, not two. An advisory can be open and yet absent from // every group, because `groupAlerts` drops one with no @@ -1307,7 +1345,7 @@ export function main( // has NO replacement issue, so calling it "superseded" would be false and // clearing it would stand down a live exposure with nothing tracking it // (Copilot). Leave the issue exactly as it is and say so. - const unpatched = stillOpen.filter((g) => !filableGhsas.has(g)); + const unpatched = stillOpen.filter((g) => !filableAdvisories.has(key3(g))); if (unpatched.length > 0) { console.log( `dependabot-alerts: #${issue.number} left as is — ${unpatched.join(", ")} ${unpatched.length === 1 ? "is" : "are"} still open with no patched version to bump to`, diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index 18d66ad4f..8441161b4 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -15,6 +15,7 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; import { + advisoryKey, parseClearedDate, SUPPORTED_ECOSYSTEM, PRIORITY_FIELD_ID, @@ -1486,7 +1487,7 @@ test("a cleared issue is not re-edited on a LATER day", () => { assert.equal(ghCall(spawn, "edit"), undefined); }); -test("a cleared issue keeps its original date when its reason changes", () => { +test("a cleared issue takes the new date when its reason changes", () => { // A real change still gets one edit — and takes the new date, since the state // genuinely changed on that day. const cleared = buildClearedBody( @@ -1564,6 +1565,61 @@ test("main does not clear an issue when the lockfile cannot be parsed", () => { ); }); +test("advisoryKey distinguishes the same GHSA in different manifests", () => { + assert.notEqual( + advisoryKey("fast-uri", "package-lock.json", "GHSA-a"), + advisoryKey("fast-uri", "clients/tui/package-lock.json", "GHSA-a"), + ); +}); + +test("another manifest's alert cannot vouch for this one when clearing", () => { + // The same GHSA legitimately covers the root install and a client's. Here the + // ROOT alert lost its patched version while the TUI one is still filable — + // keying on the GHSA alone would let the TUI group vouch for the root issue + // and clear it, even though the root alert is open with no bump available. + const [rootFiled] = groupAlerts([alert({ ghsa: "GHSA-a" })]); + const rootUnpatched = alert({ ghsa: "GHSA-a" }); + rootUnpatched.security_vulnerability.first_patched_version = null; + + const spawn = fakeSpawn({ + alertPages: [ + [ + rootUnpatched, + alert({ + ghsa: "GHSA-a", + manifest: "clients/tui/package-lock.json", + range: "< 3.1.6", + }), + ], + ], + issues: [ + { + number: 41, + title: buildIssueTitle(rootFiled), + body: buildIssueBody(rootFiled, asInstalled()), + }, + ], + }); + const log = inTempRepo( + { + "package-lock.json": lockWith("fast-uri", "3.1.5"), + "clients/tui/package-lock.json": lockWith("fast-uri", "3.1.5"), + }, + () => + withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ), + ); + + // The TUI bump is filed; the root issue is left alone, not cleared. + assert.ok(ghCall(spawn, "create")); + assert.equal(ghCall(spawn, "edit"), undefined); + assert.ok( + log.some((l) => l.includes("#41 left as is")), + `expected the root issue left as is, got: ${log.join(" | ")}`, + ); +}); + test("main reads every page of open dependabot issues", () => { // The second page holds the matching marker. Truncating the lookup would // file a duplicate issue rather than recognising this one. From 0bf8fbd36b2e0333e48018fef8f81c265909ae06 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 09:26:20 -0400 Subject: [PATCH 14/14] chore(deps): address Copilot review round 15 on #2243 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall --- scripts/dependabot-alerts.mjs | 75 ++++++++++++++++++++++-------- scripts/dependabot-alerts.test.mjs | 69 ++++++++++++++++++++++++++- 2 files changed, 124 insertions(+), 20 deletions(-) diff --git a/scripts/dependabot-alerts.mjs b/scripts/dependabot-alerts.mjs index 067b7ec7a..daca7b43c 100644 --- a/scripts/dependabot-alerts.mjs +++ b/scripts/dependabot-alerts.mjs @@ -1128,14 +1128,31 @@ export function main( * advisory must never do is stand itself down. */ /** - * `(package, manifest, GHSA)` tuples that made it into a group — the ones - * this sweep can actually file a bump for. + * What this run actually established about each `(package, manifest, GHSA)`. + * + * ⚠️ Recorded DURING the loop, not derived from `groups` beforehand. Being 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 + * (Copilot). Reading "is it in a group?" as "does it have an issue?" would + * clear an old issue as superseded while its replacement was skipped. + * + * - `tracked` — an issue for it exists after this run. + * - `not-exposed` — probed, and nothing installed is in range. + * - `indeterminate` — could not be probed at all this run. + * + * Absent means no group carried it, i.e. no bump is available for it. + * + * @type {Map} */ - const filableAdvisories = new Set( - groups.flatMap((g) => - g.ghsas.map((ghsa) => advisoryKey(g.package, g.manifestPath, ghsa)), - ), - ); + const disposition = new Map(); + const note = (group, ghsas, value) => { + for (const ghsa of ghsas) { + disposition.set( + advisoryKey(group.package, group.manifestPath, ghsa), + value, + ); + } + }; const openAdvisories = new Set( alerts .filter((a) => a.state === "open") @@ -1161,6 +1178,7 @@ export function main( console.log( `dependabot-alerts: ${rawGroup.package} (${rawGroup.ecosystem}, ${rawGroup.manifestPath}) is not an npm dependency — this sweep cannot file it, raise it by hand: ${rawGroup.ghsas.join(", ")}`, ); + note(rawGroup, rawGroup.ghsas, "indeterminate"); continue; } @@ -1193,12 +1211,14 @@ export function main( console.log( `dependabot-alerts: ${rawGroup.manifestPath} could not be parsed as an npm lockfile — skipping ${rawGroup.package} WITHOUT clearing its issue`, ); + note(rawGroup, rawGroup.ghsas, "indeterminate"); continue; } if (manifest.absent) { console.log( `dependabot-alerts: ${rawGroup.manifestPath} absent on ${TARGET_BRANCH} — skipping ${rawGroup.package}`, ); + note(rawGroup, rawGroup.ghsas, "not-exposed"); clear(`\`${rawGroup.manifestPath}\` is no longer part of this repo`); continue; } @@ -1211,6 +1231,7 @@ export function main( console.log( `dependabot-alerts: ${rawGroup.package}@${seen.join("/") || "(absent)"} is already out of range on ${TARGET_BRANCH} — skipping`, ); + note(rawGroup, rawGroup.ghsas, "not-exposed"); clear( seen.length > 0 ? `every installed copy is out of range (${seen.map((v) => `\`${v}\``).join(", ")})` @@ -1221,6 +1242,14 @@ export function main( // From here on `group` carries only the advisories that apply to this // branch, so the marker, title, severity and table cannot overstate it. const { group, affected } = applicable; + // The narrowing dropped advisories whose range no longer matches: those are + // probed-and-clear, the survivors get an issue. + note( + rawGroup, + rawGroup.ghsas.filter((g) => !group.ghsas.includes(g)), + "not-exposed", + ); + note(group, group.ghsas, "tracked"); const declared = isDirectDependency(lock, group.package); @@ -1339,24 +1368,32 @@ export function main( openAdvisories.has(key3(g)), ); - // ⚠️ Three states, not two. An advisory can be open and yet absent from - // every group, because `groupAlerts` drops one with no - // `first_patched_version` — there is nothing to bump to. Such an advisory - // has NO replacement issue, so calling it "superseded" would be false and - // clearing it would stand down a live exposure with nothing tracking it - // (Copilot). Leave the issue exactly as it is and say so. - const unpatched = stillOpen.filter((g) => !filableAdvisories.has(key3(g))); - if (unpatched.length > 0) { + // ⚠️ Clearing needs positive evidence about every advisory still open here. + // Two things deny it, and both mean "leave the issue alone" (Copilot): + // an advisory this run could not probe (`indeterminate` — a non-npm + // manifest, or one that would not parse), and one no group carried at all + // (absent — `groupAlerts` drops an alert with no `first_patched_version`, + // so there is nothing to bump to and no replacement issue). + const unresolved = stillOpen.filter((g) => { + const state = disposition.get(key3(g)); + return state === undefined || state === "indeterminate"; + }); + if (unresolved.length > 0) { console.log( - `dependabot-alerts: #${issue.number} left as is — ${unpatched.join(", ")} ${unpatched.length === 1 ? "is" : "are"} still open with no patched version to bump to`, + `dependabot-alerts: #${issue.number} left as is — ${unresolved.join(", ")} ${unresolved.length === 1 ? "is" : "are"} still open and this run could not establish a replacement`, ); continue; } + const tracked = stillOpen.filter( + (g) => disposition.get(key3(g)) === "tracked", + ); 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"; + stillOpen.length === 0 + ? "every alert it tracked has been fixed or dismissed" + : tracked.length > 0 + ? `this bump was superseded — ${tracked.map((g) => `\`${g}\``).join(", ")} ${tracked.length === 1 ? "is" : "are"} still open under a different patched version, and ${tracked.length === 1 ? "has" : "have"} their own issue` + : "no installed copy is in range of its advisories any more"; writeCleared( issue, diff --git a/scripts/dependabot-alerts.test.mjs b/scripts/dependabot-alerts.test.mjs index 8441161b4..c90e2fac1 100644 --- a/scripts/dependabot-alerts.test.mjs +++ b/scripts/dependabot-alerts.test.mjs @@ -1451,7 +1451,7 @@ test("main will not stand down an open alert that lost its patched version", () (l) => l.includes("left as is") && l.includes("GHSA-a") && - l.includes("no patched version"), + l.includes("could not establish a replacement"), ), `expected a left-as-is line, got: ${log.join(" | ")}`, ); @@ -1620,6 +1620,73 @@ test("another manifest's alert cannot vouch for this one when clearing", () => { ); }); +test("a revised bump does not clear the old issue when the probe is indeterminate", () => { + // GitHub revised first_patched_version, so the old key vanished — but the + // replacement group was skipped because the lockfile would not parse. Nothing + // was established, so "superseded, it has its own issue" would be a guess. + const [filed] = groupAlerts([alert({ ghsa: "GHSA-a", fixed: "3.1.6" })]); + const spawn = fakeSpawn({ + alertPages: [[alert({ ghsa: "GHSA-a", fixed: "3.1.7", range: "< 3.1.7" })]], + issues: [ + { + number: 41, + title: buildIssueTitle(filed), + body: buildIssueBody(filed, asInstalled()), + }, + ], + }); + + const dir = mkdtempSync(join(tmpdir(), "dependabot-alerts-")); + const cwd = process.cwd(); + let log; + try { + writeFileSync(join(dir, "package-lock.json"), "{ truncated…"); + process.chdir(dir); + log = withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ); + } finally { + process.chdir(cwd); + rmSync(dir, { recursive: true, force: true }); + } + + assert.equal(ghCall(spawn, "edit"), undefined, "the old issue is preserved"); + assert.equal(ghCall(spawn, "create"), undefined); + assert.ok( + log.some((l) => l.includes("could not establish a replacement")), + `expected an indeterminate line, got: ${log.join(" | ")}`, + ); +}); + +test("a revised bump that is no longer exposed clears without claiming an issue", () => { + // Same revision, but the probe DID run and found nothing in range. That is + // positive evidence, so the old issue clears — saying exposure is gone rather + // than claiming a replacement issue that was never filed. + const [filed] = groupAlerts([alert({ ghsa: "GHSA-a", fixed: "3.1.6" })]); + const spawn = fakeSpawn({ + alertPages: [ + [alert({ ghsa: "GHSA-a", fixed: "3.1.7", range: ">= 3.1.6, < 3.1.7" })], + ], + issues: [ + { + number: 41, + title: buildIssueTitle(filed), + body: buildIssueBody(filed, asInstalled()), + }, + ], + }); + inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () => + withoutProjectToken(() => + captureLog(() => main("o/r", spawn, "2026-09-04")), + ), + ); + const edit = ghCall(spawn, "edit"); + assert.ok(edit); + const body = edit.args[edit.args.indexOf("--body") + 1]; + assert.match(body, /no installed copy is in range/); + assert.doesNotMatch(body, /has their own issue|superseded/); +}); + test("main reads every page of open dependabot issues", () => { // The second page holds the matching marker. Truncating the lookup would // file a duplicate issue rather than recognising this one.