chore(deps): address Copilot review round 14 on #2243
- Reconcile on (package, manifest, GHSA) tuples, not GHSAs alone. Dependabot alerts are per manifest and this repo has five lockfiles, so the same advisory legitimately covers the root install and a client. Keyed on the GHSA alone, another manifest's still-filable alert could vouch for this one and clear an issue whose own alert is open with no bump available. - Rename the cleared-date test: it asserts the NEW date is taken when the reason changes, which is the opposite of what its name said. - AGENTS.md no longer implies both sweeps select a dated milestone. Only the security sweep filters undated buckets; the monthly one's selection is raised on #2239. 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
c0d1d56782
commit
e846dec491
3 changed files with 103 additions and 9 deletions
|
|
@ -122,7 +122,7 @@ An issue filed by either sweep is an ordinary board item — `v2` + `chore` + `d
|
||||||
| Monthly version sweep | **No, never.** It does not attempt a board write at all and has no `PROJECT_TOKEN`; the issue arrives labeled and milestoned, and `/issue-triage` places it. |
|
| Monthly version sweep | **No, never.** It does not attempt a board write at all and has no `PROJECT_TOKEN`; the issue arrives labeled and milestoned, and `/issue-triage` places it. |
|
||||||
| Daily security sweep | **Only when it can.** With an org-project PAT it places the card directly at **Todo / High**; without one it degrades to the same triage hand-off. |
|
| Daily security sweep | **Only when it can.** With an org-project PAT it places the card directly at **Todo / High**; without one it degrades to the same triage hand-off. |
|
||||||
|
|
||||||
The board write needs `organization projects: write`, which `GITHUB_TOKEN` cannot have — hence "only when it can", and hence a filed-but-unboarded issue is a normal outcome rather than a failure. **Todo, not Incoming**, when the security sweep does place it: arriving through this pipeline *is* the approval. **`High` is a standing override** of the [priority rubric](.claude/skills/issue-triage/SKILL.md), which would otherwise score a routine bump Medium; the issue body records the override so it does not read as a mis-score. ⚠️ A milestone is a precondition for either — `Incoming` ⇔ no milestone — so an issue filed when no dated milestone is open is deliberately left unboarded rather than parked at Todo.
|
The board write needs `organization projects: write`, which `GITHUB_TOKEN` cannot have — hence "only when it can", and hence a filed-but-unboarded issue is a normal outcome rather than a failure. **Todo, not Incoming**, when the security sweep does place it: arriving through this pipeline *is* the approval. **`High` is a standing override** of the [priority rubric](.claude/skills/issue-triage/SKILL.md), which would otherwise score a routine bump Medium; the issue body records the override so it does not read as a mis-score. ⚠️ A milestone is a precondition for placing a card — `Incoming` ⇔ no milestone — so the security sweep leaves an issue **unboarded** rather than parked at Todo when no dated milestone is open. It picks the open milestone with the nearest **due date**, ignoring undated buckets; the monthly sweep's own selection does not yet filter those out (raised on #2239), so don't read this as a guarantee both scripts already implement.
|
||||||
|
|
||||||
## Contributing
|
## Contributing
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -264,6 +264,22 @@ export function groupKey(pkg, manifestPath, fixedIn) {
|
||||||
return JSON.stringify([pkg, manifestPath, fixedIn]);
|
return JSON.stringify([pkg, manifestPath, fixedIn]);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* One advisory as it applies to one manifest.
|
||||||
|
*
|
||||||
|
* ⚠️ A GHSA alone is not enough. Dependabot alerts are per MANIFEST, and this
|
||||||
|
* repo has five lockfiles — so the same advisory legitimately appears for the
|
||||||
|
* root install and for a client. Keying reconciliation on the GHSA alone lets
|
||||||
|
* another manifest's still-filable alert vouch for this one, and an issue whose
|
||||||
|
* own alert lost its patched version would be cleared on the strength of a
|
||||||
|
* different lockfile's alert (Copilot).
|
||||||
|
*
|
||||||
|
* @returns {string}
|
||||||
|
*/
|
||||||
|
export function advisoryKey(pkg, manifestPath, ghsa) {
|
||||||
|
return JSON.stringify([pkg, manifestPath, ghsa]);
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Collapse per-advisory alerts into one entry per BUMP.
|
* Collapse per-advisory alerts into one entry per BUMP.
|
||||||
*
|
*
|
||||||
|
|
@ -1100,7 +1116,8 @@ export function main(
|
||||||
/** Grouping keys this run actually saw in the open feed. */
|
/** Grouping keys this run actually saw in the open feed. */
|
||||||
const seenKeys = new Set();
|
const seenKeys = new Set();
|
||||||
/**
|
/**
|
||||||
* Every GHSA still open, taken from the RAW feed rather than from `groups`.
|
* Every open `(package, manifest, GHSA)`, taken from the RAW feed rather
|
||||||
|
* than from `groups`.
|
||||||
*
|
*
|
||||||
* ⚠️ `groupAlerts` deliberately drops an alert with no `first_patched_version`
|
* ⚠️ `groupAlerts` deliberately drops an alert with no `first_patched_version`
|
||||||
* — there is nothing to bump to, so nothing to file. Building this set from
|
* — there is nothing to bump to, so nothing to file. Building this set from
|
||||||
|
|
@ -1110,12 +1127,29 @@ export function main(
|
||||||
* direction as the superseded case, reached a different way: what a still-open
|
* direction as the superseded case, reached a different way: what a still-open
|
||||||
* advisory must never do is stand itself down.
|
* 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));
|
* `(package, manifest, GHSA)` tuples that made it into a group — the ones
|
||||||
const openGhsas = new Set(
|
* this sweep can actually file a bump for.
|
||||||
|
*/
|
||||||
|
const filableAdvisories = new Set(
|
||||||
|
groups.flatMap((g) =>
|
||||||
|
g.ghsas.map((ghsa) => advisoryKey(g.package, g.manifestPath, ghsa)),
|
||||||
|
),
|
||||||
|
);
|
||||||
|
const openAdvisories = new Set(
|
||||||
alerts
|
alerts
|
||||||
.filter((a) => a.state === "open")
|
.filter((a) => a.state === "open")
|
||||||
.map((a) => a.security_advisory?.ghsa_id)
|
.map((a) =>
|
||||||
|
a.dependency?.package?.name &&
|
||||||
|
a.dependency?.manifest_path &&
|
||||||
|
a.security_advisory?.ghsa_id
|
||||||
|
? advisoryKey(
|
||||||
|
a.dependency.package.name,
|
||||||
|
a.dependency.manifest_path,
|
||||||
|
a.security_advisory.ghsa_id,
|
||||||
|
)
|
||||||
|
: null,
|
||||||
|
)
|
||||||
.filter(Boolean),
|
.filter(Boolean),
|
||||||
);
|
);
|
||||||
|
|
||||||
|
|
@ -1299,7 +1333,11 @@ export function main(
|
||||||
// while the GHSA stays open — reporting that as "fixed or dismissed" would
|
// while the GHSA stays open — reporting that as "fixed or dismissed" would
|
||||||
// stand down a live exposure (Copilot). So the reason is decided by whether
|
// 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.
|
// the GHSAs are still in the open feed, not by the key's absence.
|
||||||
const stillOpen = issue.marker.ghsas.filter((g) => openGhsas.has(g));
|
const key3 = (ghsa) =>
|
||||||
|
advisoryKey(issue.marker.package, issue.marker.manifestPath, ghsa);
|
||||||
|
const stillOpen = issue.marker.ghsas.filter((g) =>
|
||||||
|
openAdvisories.has(key3(g)),
|
||||||
|
);
|
||||||
|
|
||||||
// ⚠️ Three states, not two. An advisory can be open and yet absent from
|
// ⚠️ Three states, not two. An advisory can be open and yet absent from
|
||||||
// every group, because `groupAlerts` drops one with no
|
// every group, because `groupAlerts` drops one with no
|
||||||
|
|
@ -1307,7 +1345,7 @@ export function main(
|
||||||
// has NO replacement issue, so calling it "superseded" would be false and
|
// has NO replacement issue, so calling it "superseded" would be false and
|
||||||
// clearing it would stand down a live exposure with nothing tracking it
|
// clearing it would stand down a live exposure with nothing tracking it
|
||||||
// (Copilot). Leave the issue exactly as it is and say so.
|
// (Copilot). Leave the issue exactly as it is and say so.
|
||||||
const unpatched = stillOpen.filter((g) => !filableGhsas.has(g));
|
const unpatched = stillOpen.filter((g) => !filableAdvisories.has(key3(g)));
|
||||||
if (unpatched.length > 0) {
|
if (unpatched.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 — ${unpatched.join(", ")} ${unpatched.length === 1 ? "is" : "are"} still open with no patched version to bump to`,
|
||||||
|
|
|
||||||
|
|
@ -15,6 +15,7 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
|
||||||
import { dirname, join } from "node:path";
|
import { dirname, join } from "node:path";
|
||||||
import { tmpdir } from "node:os";
|
import { tmpdir } from "node:os";
|
||||||
import {
|
import {
|
||||||
|
advisoryKey,
|
||||||
parseClearedDate,
|
parseClearedDate,
|
||||||
SUPPORTED_ECOSYSTEM,
|
SUPPORTED_ECOSYSTEM,
|
||||||
PRIORITY_FIELD_ID,
|
PRIORITY_FIELD_ID,
|
||||||
|
|
@ -1486,7 +1487,7 @@ test("a cleared issue is not re-edited on a LATER day", () => {
|
||||||
assert.equal(ghCall(spawn, "edit"), undefined);
|
assert.equal(ghCall(spawn, "edit"), undefined);
|
||||||
});
|
});
|
||||||
|
|
||||||
test("a cleared issue keeps its original date when its reason changes", () => {
|
test("a cleared issue takes the new date when its reason changes", () => {
|
||||||
// A real change still gets one edit — and takes the new date, since the state
|
// A real change still gets one edit — and takes the new date, since the state
|
||||||
// genuinely changed on that day.
|
// genuinely changed on that day.
|
||||||
const cleared = buildClearedBody(
|
const cleared = buildClearedBody(
|
||||||
|
|
@ -1564,6 +1565,61 @@ test("main does not clear an issue when the lockfile cannot be parsed", () => {
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("advisoryKey distinguishes the same GHSA in different manifests", () => {
|
||||||
|
assert.notEqual(
|
||||||
|
advisoryKey("fast-uri", "package-lock.json", "GHSA-a"),
|
||||||
|
advisoryKey("fast-uri", "clients/tui/package-lock.json", "GHSA-a"),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test("another manifest's alert cannot vouch for this one when clearing", () => {
|
||||||
|
// The same GHSA legitimately covers the root install and a client's. Here the
|
||||||
|
// ROOT alert lost its patched version while the TUI one is still filable —
|
||||||
|
// keying on the GHSA alone would let the TUI group vouch for the root issue
|
||||||
|
// and clear it, even though the root alert is open with no bump available.
|
||||||
|
const [rootFiled] = groupAlerts([alert({ ghsa: "GHSA-a" })]);
|
||||||
|
const rootUnpatched = alert({ ghsa: "GHSA-a" });
|
||||||
|
rootUnpatched.security_vulnerability.first_patched_version = null;
|
||||||
|
|
||||||
|
const spawn = fakeSpawn({
|
||||||
|
alertPages: [
|
||||||
|
[
|
||||||
|
rootUnpatched,
|
||||||
|
alert({
|
||||||
|
ghsa: "GHSA-a",
|
||||||
|
manifest: "clients/tui/package-lock.json",
|
||||||
|
range: "< 3.1.6",
|
||||||
|
}),
|
||||||
|
],
|
||||||
|
],
|
||||||
|
issues: [
|
||||||
|
{
|
||||||
|
number: 41,
|
||||||
|
title: buildIssueTitle(rootFiled),
|
||||||
|
body: buildIssueBody(rootFiled, asInstalled()),
|
||||||
|
},
|
||||||
|
],
|
||||||
|
});
|
||||||
|
const log = inTempRepo(
|
||||||
|
{
|
||||||
|
"package-lock.json": lockWith("fast-uri", "3.1.5"),
|
||||||
|
"clients/tui/package-lock.json": lockWith("fast-uri", "3.1.5"),
|
||||||
|
},
|
||||||
|
() =>
|
||||||
|
withoutProjectToken(() =>
|
||||||
|
captureLog(() => main("o/r", spawn, "2026-09-04")),
|
||||||
|
),
|
||||||
|
);
|
||||||
|
|
||||||
|
// The TUI bump is filed; the root issue is left alone, not cleared.
|
||||||
|
assert.ok(ghCall(spawn, "create"));
|
||||||
|
assert.equal(ghCall(spawn, "edit"), undefined);
|
||||||
|
assert.ok(
|
||||||
|
log.some((l) => l.includes("#41 left as is")),
|
||||||
|
`expected the root issue left as is, got: ${log.join(" | ")}`,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
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