chore(deps): address Copilot review rounds 8-10 on #2243
Three reviews I missed: my poll called the reviews endpoint without --paginate, so it only ever read page one — the same pagination bug this PR was reviewed for. - Reconcile marked issues whose bump has left the open alert feed entirely. openAlerts asks for state=open, so a fixed or dismissed alert simply vanishes: its group is never built, the loop never visits it, and its issue kept asserting a vulnerability with a live Todo/High card. Both cases are covered — the zero-alert run, which used to return before loading issues at all, and a vanished group while other groups remain. - Ask for a PARENT-SCOPED overrides entry when the package is also declared directly. npm rejects an override contradicting a direct dependency with EOVERRIDE, so the guidance for the mixed case would not have applied. The issue now prints the exact nested JSON. - Correct the body's NOTE: severity, range, GHSA and CVE are the advisory's own. Only the installed versions, their paths and range applicability are verified against v2/main. - Stop the new-advisory comment claiming the body marker "now covers" the advisory; it is posted before the edit, so that is false at posting time. It speaks for its own marker instead. - Cover the successful two-field board placement, not only its failure modes. - AGENTS.md: the monthly sweep NEVER boards; only the security sweep does, and only with a PAT. The old wording promised a card the monthly job cannot create. 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
1d6ea1a8f6
commit
9e516ae0a1
2 changed files with 316 additions and 17 deletions
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Reference in a new issue