From e846dec491215fe8dfffc3ef613542ed827369c2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 09:00:13 -0400 Subject: [PATCH] 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 34487261..d5103dd7 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 f041708d..067b7ec7 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 18d66ad4..8441161b 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.