chore(deps): address Copilot review round 2 on #2243
- Remove the literal NUL bytes that were separating the grouping key's fields; they classified the whole source file as binary, so repo searches skipped it. The key is a JSON array now, with no separator left to justify. - De-duplicate advisory comments per GHSA rather than per whole set. A run that comments and then fails before the marker edit left the next run computing a different set, which matched nothing and announced the same advisory twice. - Refresh the issue title on edit; it carries the advisory count, so it went stale as soon as an issue grew past what it was filed with. - Correct the test file's header, which still claimed main() was left to workflow_dispatch. 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
b53266bc72
commit
761bdd128d
2 changed files with 97 additions and 15 deletions
|
|
@ -233,7 +233,11 @@ export function groupAlerts(alerts) {
|
|||
// for an unavailable upgrade is noise, so it waits for one to be published.
|
||||
if (!pkg || !manifestPath || !fixedIn) continue;
|
||||
|
||||
const key = `${pkg} ${manifestPath} ${fixedIn}`;
|
||||
// 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 advisory = {
|
||||
ghsa: alert.security_advisory?.ghsa_id ?? "",
|
||||
cve: alert.security_advisory?.cve_id ?? null,
|
||||
|
|
@ -281,10 +285,12 @@ export function groupAlerts(alerts) {
|
|||
|
||||
/**
|
||||
* @param {ReturnType<typeof groupAlerts>[number]} group
|
||||
* @param {number} [count] advisories the issue covers, when that is more than
|
||||
* this run saw — an issue grown by a later advisory keeps one title.
|
||||
* @returns {string}
|
||||
*/
|
||||
export function buildIssueTitle(group) {
|
||||
const n = group.advisories.length;
|
||||
export function buildIssueTitle(group, count = group.advisories.length) {
|
||||
const n = count;
|
||||
return `chore(deps): bump \`${group.package}\` to \`${group.fixedIn}\` in \`${group.manifestPath}\` (${n} ${n === 1 ? "advisory" : "advisories"})`;
|
||||
}
|
||||
|
||||
|
|
@ -510,7 +516,15 @@ function openDependabotIssues(repo, spawn) {
|
|||
);
|
||||
}
|
||||
|
||||
/** The GHSA sets already announced by comments on an issue. */
|
||||
/**
|
||||
* Every GHSA already announced by a comment on an issue, unioned.
|
||||
*
|
||||
* Unioned per ADVISORY, not compared per comment. Comparing whole sets looks
|
||||
* equivalent and is not: a run that posts `[B]` and then fails before rewriting
|
||||
* the marker leaves the next run computing `[B, C]`, which matches no existing
|
||||
* comment, and `B` is announced a second time (Copilot). Individual GHSAs are
|
||||
* what a comment actually claims to have announced.
|
||||
*/
|
||||
function announcedAdvisories(repo, number, spawn) {
|
||||
const issue = ghJson(spawn, [
|
||||
"issue",
|
||||
|
|
@ -521,9 +535,13 @@ function announcedAdvisories(repo, number, spawn) {
|
|||
"--json",
|
||||
"comments",
|
||||
]);
|
||||
return (issue?.comments ?? [])
|
||||
.map((comment) => parseCommentMarker(comment.body))
|
||||
.filter(Boolean);
|
||||
const announced = new Set();
|
||||
for (const comment of issue?.comments ?? []) {
|
||||
for (const ghsa of parseCommentMarker(comment.body) ?? []) {
|
||||
announced.add(ghsa);
|
||||
}
|
||||
}
|
||||
return announced;
|
||||
}
|
||||
|
||||
function currentMilestone(repo, spawn) {
|
||||
|
|
@ -785,9 +803,9 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) {
|
|||
// run take the no-op branch above and skip the comment permanently
|
||||
// (Copilot). In this order the worst case is a repeat, and the comment's
|
||||
// own marker rules that out too.
|
||||
const alreadyAnnounced = announcedAdvisories(repo, existing.number, spawn);
|
||||
const marker = buildCommentMarker(added);
|
||||
if (!alreadyAnnounced.some((set) => buildCommentMarker(set) === marker)) {
|
||||
const announced = announcedAdvisories(repo, existing.number, spawn);
|
||||
const unannounced = added.filter((ghsa) => !announced.has(ghsa));
|
||||
if (unannounced.length > 0) {
|
||||
const comment = gh(spawn, [
|
||||
"issue",
|
||||
"comment",
|
||||
|
|
@ -795,7 +813,7 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) {
|
|||
"--repo",
|
||||
repo,
|
||||
"--body",
|
||||
buildNewAdvisoryComment(group, added),
|
||||
buildNewAdvisoryComment(group, unannounced),
|
||||
]);
|
||||
if (comment.status !== 0) {
|
||||
throw new Error(
|
||||
|
|
@ -804,12 +822,16 @@ export function main(repo = process.env.GITHUB_REPOSITORY, spawn = spawnSync) {
|
|||
}
|
||||
}
|
||||
|
||||
// The title carries the advisory count, so it goes stale the moment the
|
||||
// issue covers one more than it was filed with (Copilot).
|
||||
const edit = gh(spawn, [
|
||||
"issue",
|
||||
"edit",
|
||||
String(existing.number),
|
||||
"--repo",
|
||||
repo,
|
||||
"--title",
|
||||
buildIssueTitle(group, merged.length),
|
||||
"--body",
|
||||
buildIssueBody(group, { installed: affected, direct, ghsas: merged }),
|
||||
]);
|
||||
|
|
|
|||
|
|
@ -1,7 +1,13 @@
|
|||
// Unit tests for the pure halves of dependabot-alerts.mjs (#2233). The impure
|
||||
// half (`main()`, which shells out to `gh`) is exercised only via
|
||||
// `workflow_dispatch` in CI, per the same split `dependency-refresh.mjs` and
|
||||
// `verify-skills.mjs` use. Run via `npm run test:scripts`.
|
||||
// Tests for dependabot-alerts.mjs (#2233) — both the pure grouping/formatting
|
||||
// helpers and `main()`'s orchestration, the latter driven through the injected
|
||||
// spawn function so no `gh` process is ever started.
|
||||
//
|
||||
// `main()` is covered rather than left to `workflow_dispatch` because a
|
||||
// production trigger is not a test (Copilot): everything that can go wrong in
|
||||
// the orchestration — a paginated alert feed, a manifest that has moved on, a
|
||||
// half-written board card, a comment posted twice — goes wrong only against the
|
||||
// real API, where nothing would be asserted.
|
||||
// Run via `npm run test:scripts`.
|
||||
|
||||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
|
|
@ -605,6 +611,60 @@ test("main does not repeat a comment it already posted", () => {
|
|||
assert.ok(ghCall(spawn, "edit"));
|
||||
});
|
||||
|
||||
test("main announces only the advisories no comment has claimed yet", () => {
|
||||
// The exact shape a failed marker edit leaves behind: the comment for `b`
|
||||
// went out, the body edit did not, and a third advisory has since arrived.
|
||||
// Comparing whole GHSA sets would find no match and announce `b` twice.
|
||||
const [old] = groupAlerts([alert({ ghsa: "GHSA-a" })]);
|
||||
const spawn = fakeSpawn({
|
||||
alertPages: [
|
||||
[
|
||||
alert({ ghsa: "GHSA-a" }),
|
||||
alert({ ghsa: "GHSA-b" }),
|
||||
alert({ ghsa: "GHSA-c" }),
|
||||
],
|
||||
],
|
||||
issues: [
|
||||
{
|
||||
number: 41,
|
||||
body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }),
|
||||
},
|
||||
],
|
||||
comments: [{ body: `${buildCommentMarker(["GHSA-b"])}\nannounced b` }],
|
||||
});
|
||||
inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () =>
|
||||
withoutProjectToken(() => captureLog(() => main("o/r", spawn))),
|
||||
);
|
||||
|
||||
const comment = ghCall(spawn, "comment");
|
||||
assert.ok(comment, "the unannounced advisory still gets a comment");
|
||||
const text = comment.args[comment.args.indexOf("--body") + 1];
|
||||
assert.deepEqual(parseCommentMarker(text), ["GHSA-c"]);
|
||||
assert.ok(!text.includes("GHSA-b"), "b was already announced");
|
||||
});
|
||||
|
||||
test("main refreshes the title when an issue grows another advisory", () => {
|
||||
const [old] = groupAlerts([alert({ ghsa: "GHSA-a" })]);
|
||||
const spawn = fakeSpawn({
|
||||
alertPages: [[alert({ ghsa: "GHSA-a" }), alert({ ghsa: "GHSA-b" })]],
|
||||
issues: [
|
||||
{
|
||||
number: 41,
|
||||
body: buildIssueBody(old, { installed: ["3.1.5"], direct: false }),
|
||||
},
|
||||
],
|
||||
});
|
||||
inTempRepo({ "package-lock.json": lockWith("fast-uri", "3.1.5") }, () =>
|
||||
withoutProjectToken(() => captureLog(() => main("o/r", spawn))),
|
||||
);
|
||||
const edit = ghCall(spawn, "edit");
|
||||
// Filed as "(1 advisory)"; editing only the body would leave it saying so.
|
||||
assert.match(
|
||||
edit.args[edit.args.indexOf("--title") + 1],
|
||||
/\(2 advisories\)$/,
|
||||
);
|
||||
});
|
||||
|
||||
test("main fails loudly when Dependabot security PRs are back on", () => {
|
||||
const spawn = fakeSpawn({ securityFixes: { enabled: true, paused: false } });
|
||||
assert.throws(
|
||||
|
|
|
|||
Loading…
Reference in a new issue