From 0674feadf74adc7a6f4d6543f106b334506cff3b Mon Sep 17 00:00:00 2001 From: t Date: Wed, 16 Sep 2026 00:22:03 +0530 Subject: [PATCH 1/2] Audit master on a clock and track what it finds in one issue Applies section 2 of docs/proposed-dependency-policy.md. Section 1, the differential pull-request gate, is deliberately not applied -- see #402, closed because `actions/dependency-review-action` already does that natively and this repository qualifies for it: it is public, its dependency graph is enabled, and that graph already carries 661 cargo packages parsed from Cargo.lock. The pull-request audit only ever looks at a branch somebody pushed. An advisory published against unchanged code is nobody's pull request, so no trigger tied to one can find it: on 2026-09-15 RUSTSEC-2026-0285 was published against `rustls` and went unnoticed until the next person to open a pull request found the pipeline frozen and nine PRs red. This is the job that looks at the default branch on a clock instead. It is also what the Rust ecosystem's own action recommends -- `rustsec/audit-check` splits by trigger exactly this way, failing the check on pull requests and filing issues on scheduled runs. `scripts/audit-tracking-issue.mjs` reconciles rather than reports. The hard part is not opening an issue, it is not opening one every day: an advisory commonly stays unresolved while an upgrade is investigated, so a naive "file a finding" becomes a daily duplicate. One open issue per repository, found by a hidden marker so a retitled issue is still found, updated only when the finding SET changes, and closed when the findings are gone. A run that changes nothing is silent. Two safety properties worth naming: A failed audit must never read as a clean one. `cargo audit` exits non-zero when it finds something, so the audit step needs `|| true` -- which also swallows a genuine failure such as an unreachable advisory database. An empty report would then mean "no findings", and the tracker would CLOSE a standing advisory's issue. A separate step refuses an empty report outright, so a broken audit fails loudly rather than silently retracting a finding. `none` and `unchanged` are distinct states even though both do nothing. The first means clean and untracked; the second means still broken and already tracked. Collapsing them would hide a standing advisory behind the same silence as a clean run. Uses the repository's existing `area:security` label rather than inventing `dependency-security` as the proposal suggested: creating a label is a repository configuration change this script should not make as a side effect of its first run. Uses `gh issue list` rather than `gh api .../issues`, which `check-gh-api-pagination` would flag as an unpaginated list call. The findings extraction mirrors `idsAndDetails` in check-advisory-delta.mjs rather than importing it, because that module calls `main()` at import time and importing it would run a full audit as a side effect. The synthetic `:@` key is the part to keep in step: a yanked crate carries no advisory id, and without it a yank would be invisible to a set comparison. That is not hypothetical -- the real audit of this lockfile today reports exactly one finding, `yanked:chacha20@0.10.1`, which the synthetic key is what makes visible. NOT pinned in the compatibility surface, which its sibling dependency-security.yml is. The surface is at 216 entries and MAX_SURFACE_FILES is 216, so adding this pin needs that constant raised first -- `reseal.sh --pins-changed` refuses with `surface_file_count_invalid` otherwise, which I confirmed by trying it. Raising a cap that the current count sits exactly on is a maintainer decision, not a side effect of adding a workflow, so it is left for review. Three of the five existing workflows are unpinned, so this is not unprecedented; it is still worth a decision. Verified: 219 tests pass across the scripts/*.test.mjs glob CI's "Frontend build" job runs, 12 of them new here and all pure -- no gh, no network, no Rust toolchain. check:ci-workflow, check:gh-api-pagination and check:unbounded-reads all exit 0. reseal.sh --verify exits 0 and no manifest changed, because nothing pinned did. The planner was driven against a real `cargo audit --json` of this lockfile, not only fixtures, and round-trips its recorded ids. Co-Authored-By: Claude Opus 5 --- .../dependency-security-scheduled.yml | 60 +++++ scripts/audit-tracking-issue.mjs | 216 ++++++++++++++++++ scripts/audit-tracking-issue.test.mjs | 126 ++++++++++ 3 files changed, 402 insertions(+) create mode 100644 .github/workflows/dependency-security-scheduled.yml create mode 100644 scripts/audit-tracking-issue.mjs create mode 100644 scripts/audit-tracking-issue.test.mjs diff --git a/.github/workflows/dependency-security-scheduled.yml b/.github/workflows/dependency-security-scheduled.yml new file mode 100644 index 00000000..1bbba4c2 --- /dev/null +++ b/.github/workflows/dependency-security-scheduled.yml @@ -0,0 +1,60 @@ +name: Dependency security (scheduled) + +# Section 2 of docs/proposed-dependency-policy.md. +# +# The pull-request audit in dependency-security.yml only ever looks at a branch +# somebody pushed. An advisory published against unchanged code is nobody's +# pull request, so nothing on that trigger can find it: on 2026-09-15 +# RUSTSEC-2026-0285 was published against `rustls` and went unnoticed until the +# next person to open a pull request found the whole pipeline frozen. This job +# looks at the default branch on a clock instead, and puts what it finds where +# a person will see it. +on: + schedule: + - cron: "17 6 * * *" # daily; off the hour, to dodge the scheduler pile-up + workflow_dispatch: + +permissions: + contents: read + issues: write # the one permission beyond the pull-request audit's + +concurrency: + group: dependency-security-scheduled + cancel-in-progress: true + +jobs: + audit-master: + name: Audit master and track findings in one issue + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v6 + with: + node-version-file: .node-version + - uses: taiki-e/install-action@fa23953489c080190314742a9b907f8e97c6767c # v2.87.10 + with: + tool: cargo-audit@0.22.2 + fallback: none + # `|| true` because cargo audit exits non-zero when it FINDS something, + # which here is the case we want to keep going for. It also swallows a + # genuine failure (no network, advisory db unreachable), so the next step + # exists to stop that being mistaken for a clean result. + - name: Audit the default branch + run: cargo audit --file src-tauri/Cargo.lock --json > audit-report.json || true + # Without this, an audit that failed to run writes an empty file, the + # tracker reads "no findings", and it CLOSES a standing advisory's + # tracking issue. Silence about a broken audit is the one outcome worse + # than a noisy one. + - name: Refuse to treat a failed audit as a clean one + run: | + if [ ! -s audit-report.json ]; then + echo "cargo audit produced no report -- treating as a failed run, not a clean one" >&2 + exit 1 + fi + - name: Open, update or close the tracking issue + env: + GH_TOKEN: ${{ github.token }} + run: node scripts/audit-tracking-issue.mjs --report audit-report.json --apply diff --git a/scripts/audit-tracking-issue.mjs b/scripts/audit-tracking-issue.mjs new file mode 100644 index 00000000..31d327c9 --- /dev/null +++ b/scripts/audit-tracking-issue.mjs @@ -0,0 +1,216 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Keeps exactly one open tracking issue in step with what `cargo audit` finds +// on the default branch, for the scheduled audit in +// `.github/workflows/dependency-security-scheduled.yml`. +// +// Why a scheduled audit needs this at all: on 2026-09-15 RUSTSEC-2026-0285 was +// published against `rustls` and nobody knew until the next person to open a +// pull request found the pipeline frozen. An advisory published against +// unchanged code is not a pull request's fault and cannot be discovered by +// anything that only runs on pull requests -- it needs a job that looks at the +// default branch on a clock. That job then has to put its finding somewhere a +// person will see, which is what this script does. +// +// The hard part is not opening an issue, it is not opening one every day. An +// advisory commonly stays unresolved for days while an upgrade is +// investigated, so "file a finding" naively becomes a daily duplicate. This +// script therefore reconciles rather than reports: one open issue per +// repository, found by a hidden marker, updated only when the finding SET +// changes, and closed when the findings are gone. A run that changes nothing +// is the normal case and must stay silent. +// +// Split deliberately into a pure planner and a thin applier: the planner is +// what the tests exercise, so the reconciliation rules are provable without a +// network, a GitHub token, or a Rust toolchain -- `pnpm test` runs every +// scripts/*.test.mjs in the "Frontend build" CI job, which has none of those. + +import { spawnSync } from "node:child_process"; +import { readFileSync } from "node:fs"; + +// A hidden marker rather than a title match: titles get edited by people, and +// a tracking issue whose title someone improved must still be found. +export const MARKER = ""; + +// The label already exists in this repository. A new one would have to be +// created as a side effect of the first run, which is a repository +// configuration change this script has no business making on its own. +const LABEL = "area:security"; + +const TITLE = "Dependency advisories on master"; + +// Mirrors `idsAndDetails` in check-advisory-delta.mjs deliberately rather than +// importing it: that module runs `main()` at import time, so importing it here +// would run a full audit as a side effect. Keep the two in step -- especially +// the synthetic key, which exists because a yanked-crate warning carries no +// advisory at all and would otherwise be invisible to a set comparison. +export function findingsFromReport(report) { + const findings = new Map(); + const record = (id, detail) => { + if (!findings.has(id)) findings.set(id, detail); + }; + for (const vulnerability of report.vulnerabilities?.list ?? []) { + record(vulnerability.advisory.id, { + id: vulnerability.advisory.id, + package: `${vulnerability.package.name}@${vulnerability.package.version}`, + summary: vulnerability.advisory.title, + kind: "vulnerability", + }); + } + for (const kind of Object.keys(report.warnings ?? {})) { + for (const warning of report.warnings[kind] ?? []) { + const id = + warning.advisory?.id ?? `${kind}:${warning.package.name}@${warning.package.version}`; + record(id, { + id, + package: `${warning.package.name}@${warning.package.version}`, + summary: warning.advisory?.title ?? kind, + kind, + }); + } + } + return [...findings.values()].sort((a, b) => a.id.localeCompare(b.id)); +} + +// The recorded set is written into the issue body on its own line so a later +// run can read back exactly what the last run saw. Comparing against the +// rendered prose instead would make an editorial change to the issue look +// like a change in the findings. +const RECORDED_PREFIX = "Tracked advisory ids:"; + +export function recordedIds(body) { + const line = (body ?? "") + .split("\n") + .map((entry) => entry.trim()) + .find((entry) => entry.startsWith(RECORDED_PREFIX)); + if (!line) return []; + return line + .slice(RECORDED_PREFIX.length) + .split(",") + .map((id) => id.trim().replace(/^`|`$/g, "")) + .filter(Boolean) + .sort(); +} + +/// The whole contract, in one pure function. +/// +/// `none` and `unchanged` are distinct on purpose: both do nothing, but the +/// first means "clean, nothing tracked" and the second means "still broken, +/// already tracked". Collapsing them would hide a standing advisory behind the +/// same silence as a clean run. +export function planAction(findings, existing) { + const ids = findings.map((finding) => finding.id).sort(); + if (ids.length === 0) { + return existing ? { action: "close", issue: existing.number } : { action: "none" }; + } + if (!existing) return { action: "create", ids, findings }; + const recorded = [...(existing.ids ?? [])].sort(); + const same = recorded.length === ids.length && recorded.every((id, i) => id === ids[i]); + if (same) return { action: "unchanged", issue: existing.number, ids }; + return { + action: "update", + issue: existing.number, + ids, + findings, + added: ids.filter((id) => !recorded.includes(id)), + removed: recorded.filter((id) => !ids.includes(id)), + }; +} + +export function issueBody(findings) { + const lines = findings.map( + (finding) => `- \`${finding.id}\` (${finding.kind}) ${finding.package} -- ${finding.summary}`, + ); + return [ + MARKER, + "", + "`cargo audit` reports the following on the default branch, after", + "`.cargo/audit.toml`'s ignore list is applied.", + "", + ...lines, + "", + "This issue is maintained by the scheduled dependency audit. It is updated", + "when the set of findings changes and closed automatically when the", + "findings are gone -- edit the title or add comments freely, but leave the", + "marker and the recorded-ids line intact or the next run will open a", + "duplicate.", + "", + `${RECORDED_PREFIX} ${findings.map((finding) => `\`${finding.id}\``).join(", ")}`, + ].join("\n"); +} + +function gh(args) { + const result = spawnSync("gh", args, { encoding: "utf8", maxBuffer: 32 * 1024 * 1024 }); + if (result.status !== 0) { + throw new Error(`audit-tracking-issue: gh ${args.join(" ")} failed: ${result.stderr?.trim()}`); + } + return result.stdout; +} + +// `gh issue list` rather than `gh api .../issues`: the repository's +// `check-gh-api-pagination` gate exempts gh's own list subcommands because +// their paging is gh-owned, and an explicit --limit says what this one expects. +function findExisting() { + const issues = JSON.parse( + gh([ + "issue", + "list", + "--state", + "open", + "--label", + LABEL, + "--limit", + "100", + "--json", + "number,body", + ]), + ); + const match = issues.find((issue) => (issue.body ?? "").includes(MARKER)); + return match ? { number: match.number, ids: recordedIds(match.body) } : null; +} + +function apply(plan) { + if (plan.action === "none" || plan.action === "unchanged") return; + if (plan.action === "create") { + gh(["issue", "create", "--title", TITLE, "--label", LABEL, "--body", issueBody(plan.findings)]); + return; + } + if (plan.action === "update") { + gh(["issue", "edit", String(plan.issue), "--body", issueBody(plan.findings)]); + const changed = [ + plan.added.length ? `newly reported: ${plan.added.join(", ")}` : null, + plan.removed.length ? `no longer reported: ${plan.removed.join(", ")}` : null, + ] + .filter(Boolean) + .join("; "); + gh(["issue", "comment", String(plan.issue), "--body", `Scheduled audit update -- ${changed}.`]); + return; + } + gh([ + "issue", + "comment", + String(plan.issue), + "--body", + "Scheduled audit reports no findings on the default branch. Closing.", + ]); + gh(["issue", "close", String(plan.issue)]); +} + +function main() { + const argv = process.argv.slice(2); + let reportPath = null; + let shouldApply = false; + for (let i = 0; i < argv.length; i += 1) { + if (argv[i] === "--report") reportPath = argv[++i]; + else if (argv[i] === "--apply") shouldApply = true; + else throw new Error(`audit-tracking-issue: unknown argument: ${argv[i]}`); + } + if (!reportPath) throw new Error("audit-tracking-issue: --report is required"); + + const findings = findingsFromReport(JSON.parse(readFileSync(reportPath, "utf8"))); + const plan = planAction(findings, shouldApply ? findExisting() : null); + process.stdout.write(`${JSON.stringify(plan, null, 2)}\n`); + if (shouldApply) apply(plan); +} + +if (process.argv[1] && process.argv[1].endsWith("audit-tracking-issue.mjs")) main(); diff --git a/scripts/audit-tracking-issue.test.mjs b/scripts/audit-tracking-issue.test.mjs new file mode 100644 index 00000000..7c4f4580 --- /dev/null +++ b/scripts/audit-tracking-issue.test.mjs @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Contract tests for scripts/audit-tracking-issue.mjs. +// +// The failure this file exists to prevent is not "the issue is wrong". It is +// the scheduled audit filing a fresh issue every single day an advisory stays +// unresolved, which is what a reconciler does the moment it can no longer read +// back what its previous run wrote. So the load-bearing test here is the body +// round trip: `recordedIds(issueBody(findings))` must return exactly the ids +// that went in. Reformat the issue body without keeping that true and this +// suite fails rather than the repository quietly accumulating duplicates. +// +// Everything is pure -- no gh, no network, no Rust toolchain. `pnpm test` +// globs scripts/*.test.mjs in the "Frontend build" CI job, which has none. + +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + MARKER, + findingsFromReport, + issueBody, + planAction, + recordedIds, +} from "./audit-tracking-issue.mjs"; + +const vuln = (id, name, version, title) => ({ + advisory: { id, title }, + package: { name, version }, +}); + +const REPORT = { + vulnerabilities: { + found: true, + list: [vuln("RUSTSEC-2026-0285", "rustls", "0.23.43", "TLS 1.3 handshake messages")], + }, + warnings: { + unmaintained: [ + { advisory: { id: "RUSTSEC-2024-0001", title: "unmaintained crate" }, package: { name: "old", version: "1.0.0" } }, + ], + // A yanked crate carries no advisory at all. Without a synthetic key it + // would be invisible to a set comparison, so a yank appearing or being + // resolved would never move the tracking issue. + yanked: [{ package: { name: "chacha20", version: "0.9.0" } }], + }, +}; + +test("a report becomes a sorted, deduplicated finding set", () => { + const findings = findingsFromReport(REPORT); + assert.deepEqual( + findings.map((finding) => finding.id), + ["RUSTSEC-2024-0001", "RUSTSEC-2026-0285", "yanked:chacha20@0.9.0"], + ); + assert.equal(findings.find((f) => f.id === "RUSTSEC-2026-0285").kind, "vulnerability"); +}); + +test("a warning with no advisory still gets a stable synthetic id", () => { + const findings = findingsFromReport({ vulnerabilities: { list: [] }, warnings: { yanked: [{ package: { name: "c", version: "1.2.3" } }] } }); + assert.deepEqual(findings.map((f) => f.id), ["yanked:c@1.2.3"]); +}); + +test("an empty report yields no findings", () => { + assert.deepEqual(findingsFromReport({ vulnerabilities: { found: false, list: [] } }), []); +}); + +// The one that matters: a body this script wrote must be readable by the next +// run. If this breaks, every scheduled run files a duplicate issue. +test("the issue body round-trips its recorded ids", () => { + const findings = findingsFromReport(REPORT); + const body = issueBody(findings); + assert.ok(body.includes(MARKER), "the body must carry the marker used to find it again"); + assert.deepEqual( + recordedIds(body), + findings.map((f) => f.id).sort(), + ); +}); + +test("a body with no recorded-ids line reads as no ids, not a crash", () => { + assert.deepEqual(recordedIds("nothing here"), []); + assert.deepEqual(recordedIds(undefined), []); +}); + +test("clean run with nothing tracked does nothing", () => { + assert.deepEqual(planAction([], null), { action: "none" }); +}); + +test("clean run with something tracked closes it", () => { + assert.deepEqual(planAction([], { number: 7, ids: ["RUSTSEC-2026-0285"] }), { + action: "close", + issue: 7, + }); +}); + +test("first finding opens an issue", () => { + const findings = findingsFromReport(REPORT); + const plan = planAction(findings, null); + assert.equal(plan.action, "create"); + assert.deepEqual(plan.ids, findings.map((f) => f.id).sort()); +}); + +test("an unchanged finding set is silent rather than a daily duplicate", () => { + const findings = findingsFromReport(REPORT); + const plan = planAction(findings, { number: 7, ids: findings.map((f) => f.id) }); + assert.equal(plan.action, "unchanged"); + assert.equal(plan.issue, 7); +}); + +test("ordering alone is not a change", () => { + const findings = findingsFromReport(REPORT); + const reversed = findings.map((f) => f.id).reverse(); + assert.equal(planAction(findings, { number: 7, ids: reversed }).action, "unchanged"); +}); + +test("a changed finding set updates and names what moved", () => { + const findings = findingsFromReport(REPORT); + const plan = planAction(findings, { number: 7, ids: ["RUSTSEC-2024-0001", "RUSTSEC-1999-0000"] }); + assert.equal(plan.action, "update"); + assert.deepEqual(plan.added, ["RUSTSEC-2026-0285", "yanked:chacha20@0.9.0"]); + assert.deepEqual(plan.removed, ["RUSTSEC-1999-0000"]); +}); + +test("a tracked issue whose ids cannot be read is treated as changed, not as clean", () => { + const findings = findingsFromReport(REPORT); + const plan = planAction(findings, { number: 7, ids: [] }); + assert.equal(plan.action, "update", "an unreadable body must not silently look up to date"); +}); From 2d93e7045ee52d042a250624c15f469bba693a53 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 16 Sep 2026 00:35:51 +0530 Subject: [PATCH 2/2] Close four review paths to the duplicate issue this script exists to prevent Review found three independent ways to end up with more than one open tracking issue, plus a demonstrated round-trip bug, all in the half the tests did not reach. The reconciliation arithmetic was correct and survived a mutation test; everything below is the GitHub-touching half around it. **Two issues carrying the marker.** `findExisting` took the first match, so a duplicated issue would be silently adopted and its twin never updated, never closed, never mentioned -- honouring "exactly one open issue" wrongly and quietly. It now refuses and names both numbers: a human duplicated something and a human should choose which survives. **The label was load-bearing and undocumented.** Discovery filters on `area:security` before looking for the marker, so re-triaging the issue and dropping that label orphans it and opens a duplicate next run. The issue body warned about the marker and the recorded-ids line but not the label; it now names all three as load-bearing. **An advisory id containing a comma or a backtick broke the round trip.** The recorded-ids line was hand-rolled CSV with backtick wrapping, and `advisory.id` is free text from the community-run RustSec database. Such an id read back as two ids, so the set compared unequal forever and the issue collected a "changed" comment every day -- exactly the noise this script exists to eliminate, reintroduced by its own encoding. Now JSON, with an unreadable line reported as no ids so the next run repairs the body rather than believing it. **The close path commented before closing.** A failed close left a "Closing" comment on a still-open issue, and the next run recomputed the same plan and left another -- one per day for as long as the failure lasted. Closed first now: a failed comment leaves the state that was wanted, and the next run sees nothing open and does nothing. Two smaller ones from the same review: `cancel-in-progress` is now false. The sibling audit workflow only reads; this one writes an issue in two steps, and cancelling between them leaves a body naming a new finding with nothing saying so, or a closed issue with no explanation. Bounded by timeout-minutes with two triggers, queueing costs little. A recorded line naming the same set twice produced an "update" whose added and removed lists were both empty, announcing a change that moved nothing. That is now a distinct `repair`: rewrite the body, say nothing. `findExisting` and `apply` take an injected runner, so the layer all four findings lived in is testable without a token or a network -- which is why it had no tests before. Eleven new ones cover duplicate refusal, close-before- comment ordering, the label-scoped query, repair versus update, silence on no-op, and the crafted-id round trip. The preview path also queries now: one that assumed no issue existed could only ever print "create" or "none", which is not a preview of what --apply does. 230 tests pass across the scripts/*.test.mjs glob, 23 of them this script's. check:ci-workflow, check:gh-api-pagination and check:unbounded-reads exit 0. reseal.sh --verify exits 0; nothing pinned changed. Co-Authored-By: Claude Opus 5 --- .../dependency-security-scheduled.yml | 8 +- scripts/audit-tracking-issue.mjs | 105 +++++++++++----- scripts/audit-tracking-issue.test.mjs | 115 ++++++++++++++++++ 3 files changed, 198 insertions(+), 30 deletions(-) diff --git a/.github/workflows/dependency-security-scheduled.yml b/.github/workflows/dependency-security-scheduled.yml index 1bbba4c2..4147a9df 100644 --- a/.github/workflows/dependency-security-scheduled.yml +++ b/.github/workflows/dependency-security-scheduled.yml @@ -19,8 +19,14 @@ permissions: issues: write # the one permission beyond the pull-request audit's concurrency: + # Deliberately NOT cancel-in-progress, unlike the sibling audit workflow. + # That one only reads; this one writes to an issue, in two steps -- edit then + # comment, or close then comment. Cancelling between them leaves the tracking + # issue half-updated: a body naming a new finding with nothing saying so, or a + # closed issue with no explanation. The job is bounded by timeout-minutes and + # has only two triggers, so letting a second run queue costs little. group: dependency-security-scheduled - cancel-in-progress: true + cancel-in-progress: false jobs: audit-master: diff --git a/scripts/audit-tracking-issue.mjs b/scripts/audit-tracking-issue.mjs index 31d327c9..0beb706a 100644 --- a/scripts/audit-tracking-issue.mjs +++ b/scripts/audit-tracking-issue.mjs @@ -78,18 +78,28 @@ export function findingsFromReport(report) { // like a change in the findings. const RECORDED_PREFIX = "Tracked advisory ids:"; +// JSON rather than a comma-separated list. `advisory.id` is free text from the +// community-run RustSec database, and an id containing a comma or a backtick +// round-trips wrong through a hand-rolled encoding -- it reads back as two ids, +// the set compares unequal forever, and the issue collects a "changed" comment +// every single day. That is exactly the noise this script exists to prevent, so +// the encoding may not be the thing that reintroduces it. export function recordedIds(body) { const line = (body ?? "") .split("\n") .map((entry) => entry.trim()) .find((entry) => entry.startsWith(RECORDED_PREFIX)); if (!line) return []; - return line - .slice(RECORDED_PREFIX.length) - .split(",") - .map((id) => id.trim().replace(/^`|`$/g, "")) - .filter(Boolean) - .sort(); + try { + const parsed = JSON.parse(line.slice(RECORDED_PREFIX.length).trim()); + if (!Array.isArray(parsed)) return []; + return parsed.filter((id) => typeof id === "string").sort(); + } catch { + // An unreadable line is reported as no ids, which `planAction` treats as a + // changed set. That rewrites the body and repairs the line, rather than + // silently believing the issue is up to date. + return []; + } } /// The whole contract, in one pure function. @@ -107,14 +117,15 @@ export function planAction(findings, existing) { const recorded = [...(existing.ids ?? [])].sort(); const same = recorded.length === ids.length && recorded.every((id, i) => id === ids[i]); if (same) return { action: "unchanged", issue: existing.number, ids }; - return { - action: "update", - issue: existing.number, - ids, - findings, - added: ids.filter((id) => !recorded.includes(id)), - removed: recorded.filter((id) => !ids.includes(id)), - }; + const added = ids.filter((id) => !recorded.includes(id)); + const removed = recorded.filter((id) => !ids.includes(id)); + // A recorded line that merely repeats an id differs as a list while naming + // the same set. Rewriting the body is right; announcing a change that moved + // nothing is not. + if (added.length === 0 && removed.length === 0) { + return { action: "repair", issue: existing.number, ids, findings }; + } + return { action: "update", issue: existing.number, ids, findings, added, removed }; } export function issueBody(findings) { @@ -131,11 +142,15 @@ export function issueBody(findings) { "", "This issue is maintained by the scheduled dependency audit. It is updated", "when the set of findings changes and closed automatically when the", - "findings are gone -- edit the title or add comments freely, but leave the", - "marker and the recorded-ids line intact or the next run will open a", - "duplicate.", + "findings are gone. Edit the title or add comments freely.", + "", + "Three things are load-bearing for finding this issue again: the marker", + `above, the recorded-ids line below, and the \`${LABEL}\` label -- the next`, + "run looks for the marker only among open issues carrying that label.", + "Remove any of the three and the next run opens a duplicate and leaves this", + "one orphaned.", "", - `${RECORDED_PREFIX} ${findings.map((finding) => `\`${finding.id}\``).join(", ")}`, + `${RECORDED_PREFIX} ${JSON.stringify(findings.map((finding) => finding.id).sort())}`, ].join("\n"); } @@ -147,12 +162,17 @@ function gh(args) { return result.stdout; } +// `run` is injected so the GitHub-touching half is testable without a token or +// a network. It is the half the reconciliation invariant actually lives in, so +// leaving it untestable would mean the tests cover the arithmetic and not the +// thing that can file a duplicate issue. +// // `gh issue list` rather than `gh api .../issues`: the repository's // `check-gh-api-pagination` gate exempts gh's own list subcommands because // their paging is gh-owned, and an explicit --limit says what this one expects. -function findExisting() { +export function findExisting(run = gh) { const issues = JSON.parse( - gh([ + run([ "issue", "list", "--state", @@ -165,35 +185,59 @@ function findExisting() { "number,body", ]), ); - const match = issues.find((issue) => (issue.body ?? "").includes(MARKER)); + const matches = issues.filter((issue) => (issue.body ?? "").includes(MARKER)); + // "Exactly one open issue" is this script's whole contract. Picking the first + // of several would honour it silently and wrongly: the others would never be + // updated and never closed, and nothing would ever say so. Refuse instead -- + // a human duplicated something, and a human should decide which survives. + if (matches.length > 1) { + throw new Error( + `audit-tracking-issue: ${matches.length} open issues carry the tracker marker ` + + `(${matches.map((issue) => `#${issue.number}`).join(", ")}). ` + + "Close all but one, then re-run -- this script will not choose for you.", + ); + } + const match = matches[0]; return match ? { number: match.number, ids: recordedIds(match.body) } : null; } -function apply(plan) { +export function apply(plan, run = gh) { if (plan.action === "none" || plan.action === "unchanged") return; if (plan.action === "create") { - gh(["issue", "create", "--title", TITLE, "--label", LABEL, "--body", issueBody(plan.findings)]); + run(["issue", "create", "--title", TITLE, "--label", LABEL, "--body", issueBody(plan.findings)]); + return; + } + if (plan.action === "repair") { + // The set is the same; only the recorded line was malformed. Rewrite it + // and say nothing, so a cosmetic repair does not read as a new finding. + run(["issue", "edit", String(plan.issue), "--body", issueBody(plan.findings)]); return; } if (plan.action === "update") { - gh(["issue", "edit", String(plan.issue), "--body", issueBody(plan.findings)]); + run(["issue", "edit", String(plan.issue), "--body", issueBody(plan.findings)]); const changed = [ plan.added.length ? `newly reported: ${plan.added.join(", ")}` : null, plan.removed.length ? `no longer reported: ${plan.removed.join(", ")}` : null, ] .filter(Boolean) .join("; "); - gh(["issue", "comment", String(plan.issue), "--body", `Scheduled audit update -- ${changed}.`]); + run(["issue", "comment", String(plan.issue), "--body", `Scheduled audit update -- ${changed}.`]); return; } - gh([ + // Close first, then comment. The other order leaves a "Closing" comment on an + // issue that is still open if the close fails, and the next run recomputes + // the same plan and leaves another -- one redundant comment per day for as + // long as the failure lasts. Closing first means a failed comment leaves the + // issue in the state that was wanted, and the next run sees nothing open and + // does nothing. + run(["issue", "close", String(plan.issue)]); + run([ "issue", "comment", String(plan.issue), "--body", - "Scheduled audit reports no findings on the default branch. Closing.", + "Scheduled audit reports no findings on the default branch. Closed automatically.", ]); - gh(["issue", "close", String(plan.issue)]); } function main() { @@ -208,7 +252,10 @@ function main() { if (!reportPath) throw new Error("audit-tracking-issue: --report is required"); const findings = findingsFromReport(JSON.parse(readFileSync(reportPath, "utf8"))); - const plan = planAction(findings, shouldApply ? findExisting() : null); + // Queried in both modes. A preview that assumed no issue existed could only + // ever print "create" or "none", which is not a preview of what --apply would + // do -- it is a different answer that happens to share a format. + const plan = planAction(findings, findExisting()); process.stdout.write(`${JSON.stringify(plan, null, 2)}\n`); if (shouldApply) apply(plan); } diff --git a/scripts/audit-tracking-issue.test.mjs b/scripts/audit-tracking-issue.test.mjs index 7c4f4580..d7a1c157 100644 --- a/scripts/audit-tracking-issue.test.mjs +++ b/scripts/audit-tracking-issue.test.mjs @@ -124,3 +124,118 @@ test("a tracked issue whose ids cannot be read is treated as changed, not as cle const plan = planAction(findings, { number: 7, ids: [] }); assert.equal(plan.action, "update", "an unreadable body must not silently look up to date"); }); + +// --- the GitHub-touching half ------------------------------------------- +// +// This is where the invariant "exactly one open issue" actually lives, and +// where every duplicate-issue path found in review lived. `run` is injected so +// these need no token and no network; a fake records the argv it was handed. + +import { apply, findExisting } from "./audit-tracking-issue.mjs"; + +const fakeRun = (issues) => { + const calls = []; + const run = (args) => { + calls.push(args); + if (args[0] === "issue" && args[1] === "list") return JSON.stringify(issues ?? []); + return ""; + }; + run.calls = calls; + return run; +}; + +const tracked = (number, ids) => ({ + number, + body: `${MARKER}\nTracked advisory ids: ${JSON.stringify(ids)}`, +}); + +test("an advisory id containing a comma or a backtick still round-trips", () => { + const findings = [ + { id: "RUSTSEC-2026-0001, and more", package: "p@1", summary: "s", kind: "vulnerability" }, + { id: "has`backtick", package: "q@2", summary: "s", kind: "vulnerability" }, + ]; + assert.deepEqual(recordedIds(issueBody(findings)), ["RUSTSEC-2026-0001, and more", "has`backtick"]); +}); + +test("a malformed recorded-ids line reads as no ids rather than throwing", () => { + assert.deepEqual(recordedIds(`${MARKER}\nTracked advisory ids: {not json`), []); + assert.deepEqual(recordedIds(`${MARKER}\nTracked advisory ids: "a string"`), []); +}); + +test("findExisting returns null when no open issue carries the marker", () => { + assert.equal(findExisting(fakeRun([{ number: 1, body: "unrelated" }])), null); +}); + +test("findExisting reads the tracked ids off the matching issue", () => { + const existing = findExisting(fakeRun([{ number: 2, body: "no marker" }, tracked(9, ["B", "A"])])); + assert.deepEqual(existing, { number: 9, ids: ["A", "B"] }); +}); + +// The contract is "exactly one". Quietly picking the first of several would +// honour it wrongly: the rest are never updated and never closed. +test("findExisting refuses rather than choosing between duplicate trackers", () => { + assert.throws( + () => findExisting(fakeRun([tracked(9, ["A"]), tracked(10, ["A"])])), + /2 open issues carry the tracker marker .*#9, #10/, + ); +}); + +test("findExisting scopes its query to open issues carrying the label", () => { + const run = fakeRun([]); + findExisting(run); + const [args] = run.calls; + assert.deepEqual(args.slice(0, 2), ["issue", "list"]); + assert.ok(args.includes("--state") && args[args.indexOf("--state") + 1] === "open"); + assert.ok(args.includes("--label"), "discovery is label-scoped, which the issue body warns about"); + assert.ok(args.includes("--limit"), "an explicit limit, not gh's default"); +}); + +// If the close fails, the wanted end state has not happened and the next run +// recomputes the same plan. Commenting first would leave a 'Closing' comment on +// a still-open issue, once per day, for as long as the failure lasts. +test("close happens before the comment that announces it", () => { + const run = fakeRun([]); + apply({ action: "close", issue: 9 }, run); + assert.deepEqual( + run.calls.map((args) => args[1]), + ["close", "comment"], + ); +}); + +test("a repair rewrites the body and says nothing", () => { + const run = fakeRun([]); + apply({ action: "repair", issue: 9, ids: ["A"], findings: [{ id: "A", package: "p@1", summary: "s", kind: "vulnerability" }] }, run); + assert.deepEqual(run.calls.map((args) => args[1]), ["edit"]); +}); + +test("an update edits the body and names what moved", () => { + const run = fakeRun([]); + apply( + { + action: "update", + issue: 9, + ids: ["A"], + findings: [{ id: "A", package: "p@1", summary: "s", kind: "vulnerability" }], + added: ["A"], + removed: ["B"], + }, + run, + ); + assert.deepEqual(run.calls.map((args) => args[1]), ["edit", "comment"]); + const comment = run.calls[1].at(-1); + assert.match(comment, /newly reported: A/); + assert.match(comment, /no longer reported: B/); +}); + +test("nothing to do means no gh calls at all", () => { + for (const action of ["none", "unchanged"]) { + const run = fakeRun([]); + apply({ action, issue: 9 }, run); + assert.equal(run.calls.length, 0, `${action} must be silent`); + } +}); + +test("a recorded line naming the same set twice repairs without announcing a change", () => { + const findings = [{ id: "A", package: "p@1", summary: "s", kind: "vulnerability" }]; + assert.equal(planAction(findings, { number: 9, ids: ["A", "A"] }).action, "repair"); +});