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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
GitHub
parent
ff339dc7c3
commit
1d6ea1a8f6
2 changed files with 185 additions and 10 deletions
|
|
@ -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<typeof groupAlerts>[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(
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Reference in a new issue