Audit master on a clock and track what it finds in one issue - #404
Merged
Merged
Conversation
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>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…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>
lamemustafa
added a commit
that referenced
this pull request
Sep 15, 2026
`dependency-security-scheduled.yml` landed in #404 unpinned, because the surface held exactly 216 entries against a cap of 216 and `reseal.sh --pins-changed` refuses a 217th with `surface_file_count_invalid`. Raising a cap the count sits exactly on is the deliberate decision that constant exists to force, so it was left for review rather than taken as a side effect of adding a workflow. This is that decision. The constant's own rule is one file for one named reason, not headroom, so the reason is recorded beside it. It differs in kind from the three raises above it. Those bound files that decide what Bridge admits -- `agent_ledgers.rs`, `master_binding.rs`, the voucher-presence engine. This one binds a file that decides what Bridge is allowed to do to its own repository while nobody is watching: it is the only workflow that runs unattended on a schedule holding `issues: write`, and it was the only one of the five whose sibling is pinned while it was not. Left unpinned, an edit that widened its permissions or pointed its audit at a different lockfile would leave the surface digest unchanged and let existing evidence attest a workflow it never covered. That is not asserted. Widening the job's `contents: read` to `contents: write` now fails `reseal.sh --verify` and fails the gate with `surface_file_changed`; before this change the same edit passed both. The file was restored byte-identically afterwards and verify returns to exit 0. The comment's closing sentence reserved the 216th slot for `agent_catalog.rs`. That slot has since been taken by it, as intended, so the sentence is replaced rather than left reading as a pending claim. `RESERVED_SURFACE_FILES` is untouched at 15 and the invariant it guards still holds: `MAX_SURFACE_FILES - files.len()` is 0, well inside it, because the cap moved by exactly one alongside exactly one pin. Resealed through the documented inverted order for a changed pin list -- `reseal.sh --pins-changed` seals the new file list before rehashing, since `rehash-surface` validates `manifest_sha256` before it does anything and the ordinary order fails with `surface_checksum_mismatch`. Verified with every exit status read directly: tools workspace 11 binaries, 53 passed, 0 failed, including `surface_file_cap_refuses_one_entry_above_the_cap` and `real_tree_has_complete_migration_and_report_surface_coverage`, the two tests that assert on this constant. Compatibility gate passes. `reseal.sh --verify` exit 0. 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 2 of
docs/proposed-dependency-policy.md. Section 1 — the differential pull-request gate — is deliberately not applied; see #402, closed after research showedactions/dependency-review-actionalready does that natively and this repository qualifies (public, dependency graph enabled, and that graph already carries 661 cargo packages parsed fromCargo.lock).Why a scheduled job is the piece that was missing
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-0285was published againstrustlsand went unnoticed until the next person to open a PR found the pipeline frozen and nine PRs red.This is also what the Rust ecosystem's own action recommends:
rustsec/audit-checksplits by trigger exactly this way — pull-request runs fail the check, scheduled runs against the default branch file issues.Reconciling, not reporting
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.
scripts/audit-tracking-issue.mjskeeps one open issue in step with what the audit finds: located by a hidden marker (so a retitled issue is still found), updated only when the finding set changes, 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 auditexits 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.noneandunchangedare distinct even though both do nothing: clean-and-untracked versus still-broken-and-already-tracked. Collapsing them would hide a standing advisory behind the same silence as a clean run.Choices that differ from the proposal, and why
area:securitylabel rather than inventingdependency-security. Creating a label is a repository configuration change this script should not make as a side effect of its first run.gh issue list, notgh api .../issues, whichcheck-gh-api-paginationwould flag as an unpaginated list call.idsAndDetailsfromcheck-advisory-delta.mjsrather than importing it: that module callsmain()at import time, so importing would run a full audit as a side effect. The part to keep in step is the synthetic<kind>:<name>@<version>key — a yanked crate carries no advisory id, and without it a yank is invisible to a set comparison. Not hypothetical: the real audit of this lockfile today reports exactly one finding,yanked:chacha20@0.10.1, which only the synthetic key makes visible.One decision left for review
This workflow is not pinned in the compatibility surface, though its sibling
dependency-security.ymlis. The surface sits at 216 entries andMAX_SURFACE_FILESis 216 — adding this pin needs that constant raised first.reseal.sh --pins-changedrefuses withsurface_file_count_invalidotherwise, which I confirmed by trying it and then reverting. Raising a cap the count sits exactly on is a maintainer decision, not a side effect of adding a workflow. Three of the five existing workflows are unpinned, so this is not unprecedented — it is still worth a decision.Verification
scripts/*.test.mjsglob (CI's "Frontend build" job)check:ci-workflow/check:gh-api-pagination/check:unbounded-readsreseal.sh --verifyThe new tests are pure — no
gh, no network, no Rust toolchain, since that CI job has none. The load-bearing one is the body round trip:recordedIds(issueBody(findings))must return what went in, because the moment it cannot, every scheduled run files a duplicate. The planner was also driven against a realcargo audit --jsonof this lockfile, not only fixtures.🤖 Generated with Claude Code