Fail a pull request for the advisories it introduces, not the world's - #402
Closed
lamemustafa wants to merge 2 commits into
Closed
lamemustafa wants to merge 2 commits into
lamemustafa wants to merge 2 commits into
Conversation
`dependency-security.yml` ran only the unconditional `cargo audit`, which answers "does this lockfile contain an accepted-severity advisory" and has no concept of before and after. On 2026-09-15 `RUSTSEC-2026-0285` was published against `rustls` (transitively via `reqwest`) and that step went red on an unchanged master, turning all nine then-open pull requests red at once -- including ones touching neither crate. `scripts/check-advisory-delta.mjs` was written for exactly this, with contract tests, and wired into nothing: its only reference anywhere under `.github/` or `scripts/` was its own test file. `docs/proposed-dependency-policy.md` proposed the wiring and deliberately stopped short of applying it. This applies section 1 of that proposal, and nothing else. The differential step is **additional, never a replacement**. The unconditional audit still runs and still fails on a real advisory whoever introduced it -- a pull request opened while master is genuinely broken must still be visibly blocked by something. What the new step adds is the question a pull request's author can act on: did *this* change introduce one. It runs on pull requests only, because a `push: master` run has no other side to diff against. `actions/checkout` gains `fetch-depth: 0`: the default shallow clone has no merge base to resolve, and fetching only the pull request ref would not bring the base branch with it. One thing the proposal predates. It was written while this workflow ran only for pull requests targeting master, which #401 changed. The script already handles it: `resolveBaseCommit` prefers `origin/$GITHUB_BASE_REF` before falling back to `origin/master`, so a stacked pull request diffs against its own base rather than against master. Verified rather than assumed -- `GITHUB_BASE_REF=claude/voucher-presence` against #308's head resolves `via origin/claude/voucher-presence` to 64b8666, that branch's tip. Driven both directions on the real commit pair for RUSTSEC-2026-0285 rather than asserted. Base 9558a31 to head 8b17f95 reports the advisory introduced and exits 1; reversed, it reports the advisory resolved and exits 0. On this branch the default invocation resolves via `origin/master` and reports the lockfile byte-identical. The failing path exits non-zero with a complete, actionable message carried on a thrown Error, which produces a stack trace. That is left alone deliberately: every sibling check script in `scripts/` signals failure the same way and none calls `process.exit`, so changing this one would make it the odd one out. Not applied: section 2's scheduled daily audit of master, which is a new workflow file and a separate decision. Without it, master's own drift is still only discovered when someone opens a pull request. `dependency-security.yml` is pinned, so surface and matrix are resealed: one hash changed, pin set set-compared against master and unchanged at 212. `check-advisory-delta.mjs` is not pinned and is unmodified. Verified with every exit status read directly: contract tests `check-advisory-delta.test.mjs` pass; `reseal.sh --verify` exit 0; `check-ci-workflow-consistency.mjs` exit 0. The workflow parses: seven steps, the new one last and gated on `github.event_name == 'pull_request'`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Master gained d09bf9b (#400, one ledger name making a whole book unreadable), which resealed master_binding.rs, bridge-tally-protocol's lib.rs and simulator_corpus.rs. This branch had resealed the workflow, so both sides moved the same two manifests and they conflicted. The pin lists were identical on both sides (212, no path only on either), so the conflict was hash content alone. Resolved by taking master's manifests and running scripts/reseal.sh, which recomputes every hash from the merged tree rather than hand-merging. Exactly one hash now differs from master -- dependency-security.yml, this branch's own change -- and no path is missing versus either side. reseal.sh --verify exit 0, read directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Closing without merging. Not because anything here is wrong — both reviews came back clean and the gate was driven both directions on the real RUSTSEC-2026-0285 pair — but because research into how other projects solve this changed the recommendation, and it was research the owner had asked for before adopting the proposal. This duplicates a first-party capability the repo can already use. The Rust ecosystem's own canonical action takes the opposite approach on pull requests. That second point matters most. The event that motivated this PR — an advisory published against unchanged code turning nine open PRs red on 2026-09-15 — is answered in that model by the scheduled job, which catches drift before anyone opens a PR. That is section 2 of So the differential half is being dropped in favour of the scheduled half, which is going up separately. What was verified here and is worth not losing:
The script stays in the tree, unwired and tested, exactly as it was before this PR. |
lamemustafa
added a commit
that referenced
this pull request
Sep 15, 2026
* 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 `<kind>:<name>@<version>` 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: t <dev@example.invalid> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applies section 1 only of
docs/proposed-dependency-policy.md, which proposed this wiring and deliberately stopped short of applying it. Follows #398 and #401, which widened these triggers to stacked pull requests.The problem
dependency-security.ymlran only the unconditionalcargo audit. That answers "does this lockfile contain an accepted-severity advisory" and has no concept of before and after — no flag turns it into one.On 2026-09-15
RUSTSEC-2026-0285was published againstrustls(transitively viareqwest), and that step went red on an unchanged master, turning all nine then-open PRs red at once — including ones touching neither crate.scripts/check-advisory-delta.mjswas written for precisely this, with contract tests, and was wired into nothing: its only reference anywhere under.github/orscripts/was its own test file.What this adds
A second step, additional and never a replacement. The unconditional audit still runs and still fails on a real advisory whoever introduced it — a PR opened while master is genuinely broken must remain visibly blocked by something. The new step adds the question a PR author can act on: did this change introduce one.
Pull requests only, because a
push: masterrun has no other side to diff against.actions/checkoutgainsfetch-depth: 0— the default shallow clone has no merge base to resolve, and fetching only the PR ref would not bring the base branch with it.One thing the proposal predates
It was written while this workflow ran only for PRs targeting master. #401 changed that. The script already handles it —
resolveBaseCommitprefersorigin/$GITHUB_BASE_REFbefore falling back toorigin/master— so a stacked PR diffs against its own base rather than against master.Verified rather than assumed:
GITHUB_BASE_REF=claude/voucher-presenceagainst #308's head resolvesvia origin/claude/voucher-presenceto64b8666b, that branch's tip.Driven, not asserted
Both directions on the real commit pair for RUSTSEC-2026-0285:
A papercut left alone deliberately
The failing path exits non-zero with a complete, actionable message carried on a thrown
Error, so CI shows a stack trace above it. Every sibling check script inscripts/signals failure the same way and none callsprocess.exit. Changing this one would make it the odd one out, so it keeps the house convention. Worth knowing the proposal's transcript shows the message without the trace.Verification
Every exit status read directly, not through a pipe.
check-advisory-delta.test.mjsreseal.sh --verifycheck-ci-workflow-consistency.mjsdependency-security.ymlis pinned, so surface and matrix are resealed: one hash changed, pin set set-compared against master and unchanged at 212.check-advisory-delta.mjsis not pinned and is unmodified. The workflow parses: seven steps, the new one last and gated ongithub.event_name == 'pull_request'.Not applied
Section 2's scheduled daily audit of master, which is a new workflow file and a separate decision. Without it, master's own drift is still only discovered when someone opens a PR — which is how RUSTSEC-2026-0285 went unnoticed until the pipeline froze. That gap is narrowed by this PR, not closed.
🤖 Generated with Claude Code