diff --git a/AGENTS.md b/AGENTS.md index ba08444f..71288c60 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 1dd384b9..480d2174 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 85c4bac0..7f334879 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.