chore(deps): address Copilot review round 13 on #2243
Three findings, all of them the same invariant reached by new routes: a still-open advisory must never stand itself down, and a cleared issue must be written once. - Never clear an advisory that is open but unpatched. groupAlerts drops an alert with no first_patched_version, so it has no replacement group and no replacement issue — calling it "superseded" was false and clearing it removed the only thing tracking a live exposure. Such an issue is now left exactly as it is, and said so in the log. - Stop re-editing cleared issues daily. A cleared issue stays open, so the next run regenerated its body with a new date and edited it again, forever. The date is now read back off the existing body, so only a real change writes — and a real change takes today's date. - Distinguish an absent manifest from an unparseable one. Both returned null, so a malformed lockfile was treated as "the manifest is gone" and cleared the issue. A read error is evidence of nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
GitHub
parent
bc47fcbcf0
commit
c0d1d56782
2 changed files with 228 additions and 59 deletions
|
|
@ -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,
|
||||
);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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", () => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue