docs(ci): give security.yml the real reason it uses prebuilt tooling - #402
Conversation
Both jobs justified installing `cargo-audit` / `cargo-deny` as prebuilt binaries
with a sentence that argues against itself:
the repo pins rustc 1.96 (rust-toolchain.toml), but the current
cargo-audit release needs >= 1.88 to COMPILE
1.96 satisfies `>= 1.88`. The "but" has no force, and the constraint it names
does not exist. It was true when written -- the pin was 1.86, genuinely below
cargo-audit's requirement -- and it survived the v1.3.0 bump to 1.96 unchanged.
The DECISION was always right; only the reason rotted. That is the dangerous
shape: a reader who notices the arithmetic concludes the constraint has lifted,
switches to `cargo install`, and pays a multi-minute compile of a large
dependency tree on every security run -- having "fixed" a comment that was
protecting something real.
So the comment now gives both actual reasons, in order, because only the first
is unconditional:
1. SPEED. Compiling the tool on every run dwarfs downloading it.
2. TOOLCHAIN INDEPENDENCE. `rust-toolchain.toml` is a directory override, so a
build-from-source install is subject to whatever rustc this repo pins. If a
tool's own MSRV ever rises above that pin, the SECURITY gate is what
breaks. A prebuilt binary is immune -- it only parses `Cargo.lock`, never
compiles the project, and runs under any toolchain.
Reason 2 is recorded as a standing property rather than deleted as history,
because it is slack today and can bind again after an MSRV change in either
direction -- including a deliberate MSRV *reduction*, which is the case nobody
would think to check.
Found while reviewing a Dependabot bump of the very action these comments sit
above. Fourth instance this release train of prose asserting something its own
facts contradict, after the pixel-provenance clear, the VRC7 section-version
doc, and this month's `4-8x` CI timeout claim.
Comments only -- no workflow behaviour changes.
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Updates documentation/comments around the CI security workflow to accurately explain why cargo-audit / cargo-deny are installed via prebuilt binaries, and records that correction in the changelog.
Changes:
- Rewrites the
security.ymlrationale comment to remove the outdated/contradictory MSRV argument and replace it with two accurate reasons (speed, and decoupling from the repo’s rustc pin). - Adds a corresponding “Fixed” entry to
CHANGELOG.mddocumenting the correction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| CHANGELOG.md | Adds an Unreleased “Fixed” note describing the corrected rationale for prebuilt security tooling. |
| .github/workflows/security.yml | Updates comments explaining why cargo-audit / cargo-deny are installed as prebuilt binaries (no behavior changes). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two suggestions, both taken, and both were right about the same failure: I argued the case at the length of a review reply and then left it in the file. THE WORKFLOW COMMENT went from 18 lines to 9. It keeps what a reader needs -- both reasons, and the fact that reason 2 was an active constraint before the v1.3.0 pin bump and can bind again -- and drops the retelling of what the old comment said and how a reader might be misled by it. That belongs in this commit message, which is where it now lives. THE CHANGELOG ENTRY went from three paragraphs to four lines. The sharper form of the objection is the one worth recording: this project's rule is to change the CHANGELOG alongside USER-VISIBLE behaviour, and a comment carries none. A one-line note that a stale rationale was corrected is the most that belongs there; the reasoning is a commit-message concern. "The added prose is disproportionately long and reads a bit like a rant" is fair comment on a change whose entire content is a comment.
|
Antigravity review addressed — both suggestions taken, and both were right about The workflow comment: 18 lines → 9. It keeps what a reader needs (both The CHANGELOG entry: three paragraphs → four lines. The sharper form of your "The added prose is disproportionately long and reads a bit like a rant" is fair |
…built-rationale # Conflicts: # .github/workflows/security.yml # CHANGELOG.md
…built-rationale # Conflicts: # CHANGELOG.md
Antigravity review (Gemini via Ultra)This trivial PR updates comments in the CI workflow to correct an outdated historical explanation for using prebuilt binaries, and documents the correction in the changelog. Blocking issuesNone found. SuggestionsNone. NitpicksNone. Automated first-pass review by |
Summary
Both jobs in
security.ymljustified installingcargo-audit/cargo-denyasprebuilt binaries with a sentence that argues against itself:
1.96 satisfies
>= 1.88. The "but" has no force and the constraint it namesdoes not exist. It was true when written — the pin was 1.86, genuinely below
cargo-audit's requirement — and it survived the v1.3.0 bump to 1.96 unchanged.
Why this is worth a PR rather than a shrug
The decision was always right; only the reason rotted. That is the dangerous
shape. A reader who notices the arithmetic concludes the constraint has lifted,
switches to
cargo install, and pays a multi-minute compile of a largedependency tree on every security run — having "fixed" a comment that was
protecting something real.
What it says now
Both actual reasons, in order, because only the first is unconditional:
rust-toolchain.tomlis a directory override,so a build-from-source install is subject to whatever rustc this repo pins. If
a tool's own MSRV ever rises above that pin, the security gate is what
breaks. A prebuilt binary is immune — it only parses
Cargo.lock, nevercompiles the project, and runs under any toolchain.
Reason 2 is recorded as a standing property rather than deleted as history,
because it is slack today and can bind again after an MSRV change in either
direction — including a deliberate MSRV reduction, which is the case nobody
would think to check.
Provenance
Found while reviewing #401, a Dependabot bump of the very action these comments
sit above. Fourth instance this release train of prose asserting something its
own facts contradict, after the pixel-provenance clear, the VRC7
section-version doc, and the
4-8xCI timeout claim in #400.Comments only — no workflow behaviour changes. Verified it merges cleanly
with #401 and that the combined result keeps both
install-action@v2.86.1bumps.