chore(deps): address Copilot review round 15 on #2243
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013H6Cew3KB3jVmQ5x4Dq1sq Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
e846dec491
commit
0bf8fbd36b
2 changed files with 124 additions and 20 deletions
|
|
@ -1128,14 +1128,31 @@ export function main(
|
||||||
* advisory must never do is stand itself down.
|
* advisory must never do is stand itself down.
|
||||||
*/
|
*/
|
||||||
/**
|
/**
|
||||||
* `(package, manifest, GHSA)` tuples that made it into a group — the ones
|
* What this run actually established about each `(package, manifest, GHSA)`.
|
||||||
* this sweep can actually file a bump for.
|
*
|
||||||
|
* ⚠️ 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<string, "tracked" | "not-exposed" | "indeterminate">}
|
||||||
*/
|
*/
|
||||||
const filableAdvisories = new Set(
|
const disposition = new Map();
|
||||||
groups.flatMap((g) =>
|
const note = (group, ghsas, value) => {
|
||||||
g.ghsas.map((ghsa) => advisoryKey(g.package, g.manifestPath, ghsa)),
|
for (const ghsa of ghsas) {
|
||||||
),
|
disposition.set(
|
||||||
);
|
advisoryKey(group.package, group.manifestPath, ghsa),
|
||||||
|
value,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
};
|
||||||
const openAdvisories = new Set(
|
const openAdvisories = new Set(
|
||||||
alerts
|
alerts
|
||||||
.filter((a) => a.state === "open")
|
.filter((a) => a.state === "open")
|
||||||
|
|
@ -1161,6 +1178,7 @@ export function main(
|
||||||
console.log(
|
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(", ")}`,
|
`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;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -1193,12 +1211,14 @@ export function main(
|
||||||
console.log(
|
console.log(
|
||||||
`dependabot-alerts: ${rawGroup.manifestPath} could not be parsed as an npm lockfile — skipping ${rawGroup.package} WITHOUT clearing its issue`,
|
`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;
|
continue;
|
||||||
}
|
}
|
||||||
if (manifest.absent) {
|
if (manifest.absent) {
|
||||||
console.log(
|
console.log(
|
||||||
`dependabot-alerts: ${rawGroup.manifestPath} absent on ${TARGET_BRANCH} — skipping ${rawGroup.package}`,
|
`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`);
|
clear(`\`${rawGroup.manifestPath}\` is no longer part of this repo`);
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
@ -1211,6 +1231,7 @@ export function main(
|
||||||
console.log(
|
console.log(
|
||||||
`dependabot-alerts: ${rawGroup.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`,
|
||||||
);
|
);
|
||||||
|
note(rawGroup, rawGroup.ghsas, "not-exposed");
|
||||||
clear(
|
clear(
|
||||||
seen.length > 0
|
seen.length > 0
|
||||||
? `every installed copy is out of range (${seen.map((v) => `\`${v}\``).join(", ")})`
|
? `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
|
// From here on `group` carries only the advisories that apply to this
|
||||||
// branch, so the marker, title, severity and table cannot overstate it.
|
// branch, so the marker, title, severity and table cannot overstate it.
|
||||||
const { group, affected } = applicable;
|
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);
|
const declared = isDirectDependency(lock, group.package);
|
||||||
|
|
||||||
|
|
@ -1339,24 +1368,32 @@ export function main(
|
||||||
openAdvisories.has(key3(g)),
|
openAdvisories.has(key3(g)),
|
||||||
);
|
);
|
||||||
|
|
||||||
// ⚠️ Three states, not two. An advisory can be open and yet absent from
|
// ⚠️ Clearing needs positive evidence about every advisory still open here.
|
||||||
// every group, because `groupAlerts` drops one with no
|
// Two things deny it, and both mean "leave the issue alone" (Copilot):
|
||||||
// `first_patched_version` — there is nothing to bump to. Such an advisory
|
// an advisory this run could not probe (`indeterminate` — a non-npm
|
||||||
// has NO replacement issue, so calling it "superseded" would be false and
|
// manifest, or one that would not parse), and one no group carried at all
|
||||||
// clearing it would stand down a live exposure with nothing tracking it
|
// (absent — `groupAlerts` drops an alert with no `first_patched_version`,
|
||||||
// (Copilot). Leave the issue exactly as it is and say so.
|
// so there is nothing to bump to and no replacement issue).
|
||||||
const unpatched = stillOpen.filter((g) => !filableAdvisories.has(key3(g)));
|
const unresolved = stillOpen.filter((g) => {
|
||||||
if (unpatched.length > 0) {
|
const state = disposition.get(key3(g));
|
||||||
|
return state === undefined || state === "indeterminate";
|
||||||
|
});
|
||||||
|
if (unresolved.length > 0) {
|
||||||
console.log(
|
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;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const tracked = stillOpen.filter(
|
||||||
|
(g) => disposition.get(key3(g)) === "tracked",
|
||||||
|
);
|
||||||
const reason =
|
const reason =
|
||||||
stillOpen.length > 0
|
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"
|
||||||
: "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(
|
writeCleared(
|
||||||
issue,
|
issue,
|
||||||
|
|
|
||||||
|
|
@ -1451,7 +1451,7 @@ test("main will not stand down an open alert that lost its patched version", ()
|
||||||
(l) =>
|
(l) =>
|
||||||
l.includes("left as is") &&
|
l.includes("left as is") &&
|
||||||
l.includes("GHSA-a") &&
|
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(" | ")}`,
|
`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", () => {
|
test("main reads every page of open dependabot issues", () => {
|
||||||
// The second page holds the matching marker. Truncating the lookup would
|
// The second page holds the matching marker. Truncating the lookup would
|
||||||
// file a duplicate issue rather than recognising this one.
|
// file a duplicate issue rather than recognising this one.
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue