ci(security): harden checkout credentials, pin actions, fail-closed tag check - #276
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Comment |
…ag check Three supply-chain items. The first two carry over from the sibling project's `v2.2.2` audit (logged in the lockstep checklist but never scoped to a rung); the third was raised by CodeRabbit reviewing #275 and declined there because pinning only the new job would have left the repo inconsistent while claiming to have addressed it. ## `persist-credentials: false` on 15 of 17 checkouts `actions/checkout` writes the workflow `GITHUB_TOKEN` into `.git/config`, where anything the job then executes from the checked-out tree -- build scripts, proc macros, test binaries, `scripts/*.sh`, the MkDocs build -- can read it. On a pull request that tree is by definition unreviewed code, and nearly every job here compiles or runs it. Highest exposure was not a `ci.yml` job but `web.yml`'s `build`, whose workflow-level `permissions:` grant `pages: write` + `id-token: write` while it runs `trunk`, `cargo doc`, `pip install`, and `mkdocs build`. Audited per site rather than applied blanket, and that distinction mattered: **`release-auto.yml`'s `prepare` job genuinely needs the credential.** It creates an annotated tag and `git push origin "$TAG"`, which authenticates through exactly that token. The sibling's audit concluded no job needed it -- that conclusion does NOT transfer here, and a blanket sweep would have silently broken every future release. It is now the only checkout in the repository that keeps the token, with the reason recorded inline so the exception reads as a decision. Its exposure is bounded by its trigger: `workflow_run` on a completed CI run of `main`, never a pull request, so the tree it executes is already-reviewed code. `antigravity-review.yml` already had the setting and is unchanged. ## The release-tag check now fails closed It was `git ls-remote --exit-code --tags origin "refs/tags/${tag}"`, which collapses THREE outcomes into two -- tag present (0), tag absent (2), and lookup failed (non-zero, various) -- reading any non-zero as "absent". A transient network blip, an auth hiccup, or a rate limit therefore sent an ALREADY-RELEASED version down the `should_release=true` path, re-tagging a release `to-dos/ROADMAP.md`'s ceremony treats as immutable. Now `gh api git/matching-refs/tags/<tag>`, chosen over `git/ref/tags/<tag>` because it answers "absent" with HTTP 200 and an empty array, so a genuine miss can never be confused with an error and no error-body parsing is needed. That endpoint matches by PREFIX, so the exact ref is compared in `jq`. Verified necessary rather than assumed: against this repository, `v1.2` prefix-matches SEVEN real tags while exact-matching none, so a naive `length > 0` would report an unreleased version as already released. Exercised all four cases against the live API -- existing tag, absent tag, prefix-only match, lookup failure. The step had no token of its own (it previously used only plain `git`), so `GH_TOKEN` is added to it; `gh` errors out without one rather than falling back to an ambient credential. ## All 34 action references pinned to commit SHAs Across all 8 workflows plus the composite `rust-setup` action. An upstream tag move could previously change what CI executes with no repository review. Each pin carries its original tag as a trailing comment so intent stays readable at a glance. Also aligned one `actions/upload-artifact@v4` -- introduced with the fuzz job in the previous commit -- to the `@v7` the rest of the repo already used, before pinning it. ## Verification `actionlint` reports the SAME three findings before and after (a self-hosted runner label, two pre-existing shellcheck notes), confirmed by linting the HEAD copies side by side -- no new lint surface. All nine files parse as YAML. The tag-check logic was exercised against the live GitHub API, including the prefix trap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
doublegate
force-pushed
the
feat/ci-supply-chain-hardening
branch
from
July 31, 2026 04:06
be158eb to
860552b
Compare
Antigravity review (Gemini via Ultra)This PR hardens GitHub Actions security by setting Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
Owner
Author
Review adjudicationTwo findings, both declined with measurements against the live API. No code change required. Blocking: "
|
| release | published |
|---|---|
v7.0.1 |
2026-04-10 |
v7.0.0 |
2026-02-26 |
and the v7 tag resolves to 043fb46d1a93c77aae656e7c1c64a875d1fc6a0a, which is exactly v7.0.1 — the SHA pinned here. The repo was already on @v7 in two places before this PR; the @v4 I aligned was the one I introduced with the fuzz job in #275, so this change removes an inconsistency rather than creating one.
Verification
All checks green including ci-success. actionlint reports the same three findings before and after (a self-hosted runner label and two pre-existing shellcheck notes) — confirmed by linting the HEAD copies side by side.
doublegate
added a commit
that referenced
this pull request
Jul 31, 2026
…nt (#277) * ci(test): run the goldens on PRs; measure run-ahead instead of assuming Third and final PR of the `v1.26.0` hardening rung. ## The golden vectors ran in no pull-request job Only `lint`, `test-light`, and `accuracysnes` run on a PR. `test-light` never passes `--features test-roms`, and the `accuracysnes` job scoped itself to `--test accuracysnes`. So the 53 rendered-scene goldens and every framebuffer golden were reached only by `full-test`, which is `mode == 'full'` -- `main`, tags, the weekly cron. A PR could shift PPU behaviour, move a golden, and hear nothing until merge-to-main. That is the same structure behind this project's own coprocessor-golden staleness (goldens left stale by post-bless PPU accuracy fixes): a golden vector nothing executes only accumulates drift. Here it was not that nothing ran them ever, but that nothing ran them while it was still cheap to fix. The job now also runs the scene, undisbeliever, rainwarrior, coprocessor, and save-state suites. Cost is bounded and known: ~5 minutes for the three with a committed corpus (scenes ~84s, undisbeliever ~158s, rainwarrior ~54s). The `*_oncart` suites need gitignored commercial dumps and firmware, so they self-skip for free; they are listed anyway so the intent is explicit rather than inferred from an omission, and so they start gating the moment a runner ever has that corpus. Found while looking for RustySNES's analogue of the sibling's `expansion_level_tripwire`. That mechanism does NOT port, and saying so matters more than inventing a stand-in: the only `commercial-roms`-gated suite here (`commercial_screenshots`) is a screenshot generator that asserts nothing, so it holds no golden that can go stale, and PPU accuracy is not parameterised by a small constant set the way expansion-audio levels are. Looking for it surfaced a larger real gap. ## Run-ahead stays opt-in -- measured, not assumed `docs/frontend.md` recorded the per-frame save-state allocation as THE blocker on run-ahead going default-on. `v1.25.0` removed that allocation, so the question was re-measured rather than assumed resolved: save_state ~119 us load_state ~285 us save/load round trip ~0.40 ms one emulated frame 6.39 ms (headless_frame_steady_state) NTSC frame budget 16.64 ms The round trip is 2.4% of the budget. It was never the dominant cost, and removing the allocation -- a real improvement -- did not move the decision. What run-ahead costs is the extra frame of emulation, inherent to the technique: `frames = 1` needs 79% of the budget, leaving ~3.5 ms for present/UI/audio on a fast development machine; `frames = 2` needs 118% and cannot hold 60 fps at all. So it stays opt-in. The plan for this rung said "default-on only if the measurement supports it, and report the number either way" -- it does not, and this is the number. One thing did change as a result. `RunAheadConfig`'s `Default` is now hand-written so `throttle_ms` defaults to 14 ms (ARMED) rather than the derived `0.0` (DISABLED). The throttle is what its own doc calls the thing that keeps run-ahead from turning a latency win into stutter, and the derived default left it off for precisely the user who had just enabled run-ahead from Settings and had no `throttle_ms` line in their config. 14 ms sits below the 16.64 ms NTSC deadline with headroom and is conservative against PAL's 20 ms. An existing config that spells the field out keeps its value -- `#[serde(default)]` fills only absent fields, pinned by a negative-control test alongside the positive one. ## Lockstep Log row for RustyNES `v2.2.1`-`v2.2.4`, and the two 2026-07-12 flags CORRECTED: the GIF/WAV capture subsystem and CRT shader-preset depth were both closed by `v1.25.0` (`av_record.rs`; the multi-pass stack + richer CRT + NTSC pass + `.slangp` import with the naga bridge), yet the roadmap still asked for a maintainer go/no-go on them two weeks later. Also carries the verification comment for #276's declined blocking finding (that `git/matching-refs` 404s on an absent tag -- it returns HTTP 200 with an empty array; `git/ref/tags` is the one that 404s, which is why it was rejected). Documentation of that adjudication, not a fix. Verified: `cargo fmt --check`, frontend clippy `-D warnings`, the new config tests, and the golden suites run under the exact multi-`--test` invocation the CI step uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(config): apply review — drop the epsilon dance, derive the NTSC budget Two review suggestions applied, both verified rather than taken on description -- and the first one corrected my own wrong assumption. `(x - 0.0).abs() < f32::EPSILON` -> `x == 0.0`. I expected this to trip `clippy::float_cmp` (the workspace runs `pedantic` at `-D warnings`) and was going to decline it on that basis; testing showed clippy exempts a constant comparison, so the suggestion is right and the epsilon form was just noise. The FORM matters though, and the comment now says so: the apparently-more-idiomatic `assert_eq!(x, 0.0)` DOES fire `float_cmp` and fails the gate, so `assert!(x == 0.0)` is the spelling that works. The magic `1000.0 / 60.0988` now derives from `Region::Ntsc.frame_rate()` so the two cannot drift apart, compared in `f64` (what `frame_rate()` returns) to avoid a lossy `as f32` the pedantic cast lints would object to. Declined: the PR title is 71 characters, not the 73 the review reported, so it is already inside the 72-character limit. Verified: `cargo fmt --check`, frontend clippy `-D warnings`, and `config::tests` 13/13. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci(test): --no-fail-fast so one broken golden doesn't hide the other nine Review round two. Two applied, one declined. **`--no-fail-fast` on the golden-vector step.** Real, and verified by injection rather than taken on description: cargo's default stops after the first failing test BINARY, across binaries and not merely within one. Injecting a failure into `save_state_backward_compat` and running it alongside `save_state_determinism` showed the second suite never ran at all. With ten suites in this step that means one broken golden hides the status of the other nine — an author fixes one, pushes, and only then discovers the next, serialising a debug loop over a ~6-minute job. Fixed with `--no-fail-fast` rather than the suggested split into ten separate steps: one flag against ten cargo startups and ten log sections, and re-verified that the step still FAILS the job while reporting every suite. **`DEFAULT_RUN_AHEAD_THROTTLE_MS`.** The `14.0` is now a named public constant so the value, the doc comment justifying it, and the test that bounds it cannot drift apart; the test asserts against the constant instead of restating the literal. **Declined: "the CHANGELOG entry is too verbose, keep it focused on user-facing behaviour."** This project's CHANGELOG is not a summary — per `docs/adr/0007` the annotated tag body IS the CHANGELOG section verbatim, and `release-auto.yml` reuses it as the GitHub release notes with no separate hand-authored text. Entries are therefore written at release-note quality by design, and the measurements here are the whole justification for a decision (run-ahead staying opt-in) that would otherwise read as an unexplained non-change. Trimming to "user-facing behaviour" would leave the release notes asserting a conclusion with its evidence deleted. Verified: `cargo fmt --check`, frontend clippy `-D warnings`, `cargo doc` with warnings-as-errors, `config::tests` 13/13, ci.yml parses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(changelog): mark the timings host-specific; record why PAL isn't asserted Review round three. One applied, two declined with measurements — and the PAL suggestion is worth recording because I applied it first and it was wrong. **Applied: the absolute timings are host-specific.** The CHANGELOG now says so, and says why they are there anyway: the ratios are what decide run-ahead's default, and a reader can only check a ratio by re-running `save_state_cost` + `headless_frame` and comparing. The percentages are the portable part; the microseconds are the working. **Declined: add a matching `Region::Pal` assertion.** I applied this, then checked the arithmetic before pushing: NTSC's frame budget is 16.64 ms and PAL's is 20.00 ms, so PAL is the LOOSER bound by 3.36 ms and `throttle_ms < NTSC` strictly implies `throttle_ms < PAL`. The added assertion could never fail while the existing one passed — a vacuous assertion sitting beside a real one and reading like extra coverage. That is the exact shape `CLAUDE.md` names under "a guard must not subsume the assertion it protects", and the project already has four recorded instances of the vacuous-test trap. Reverted, with the arithmetic left in the test so the next reader does not re-derive it. (My first pass also wrote a comment claiming NTSC was the bound that would "silently carry a PAL-violating value through", which is backwards — a PAL-violating value exceeds the NTSC bound too and is caught. Fixed.) **Declined: merge the two harness `cargo test` steps.** Measured: a second cargo invocation against an already-built target dir costs 0.58 s — 0.16% of the ~6-minute step. Against that, merging would apply the battery step's `-- --nocapture` to all ten golden suites (flooding the log) and collapse two separately-attributed CI steps into one, so a failure no longer says at a glance whether the battery or the goldens broke. Verified: `cargo fmt --check`, frontend clippy `-D warnings`, `config::tests` 13/13. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(config): note the float_cmp form at the second assertion too Review round four, nitpick applied: the `assert!(x == y)` at the default check lacked the note its sibling assertion carries about why `assert_eq!` fails clippy's `float_cmp` under `-D warnings`. Two identical-looking lines where only one explains itself is how the next person "simplifies" the unexplained one and breaks the gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <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.
Second PR of the
v1.26.0hardening rung. Stacked on #275 (feat/fuzzing-infrastructure) — it touches the same workflow files, so stacking avoids a conflict. Retarget tomainafter #275 merges.Three supply-chain items: two carried over from the sibling project's
v2.2.2audit (logged into-dos/LOCKSTEP-CHECKLIST.mdbut never scoped to a rung), and one raised by CodeRabbit on #275 and declined there because pinning only the new job would have left the repo inconsistent while claiming to have addressed it.1.
persist-credentials: falseon 15 of 17 checkoutsactions/checkoutwrites the workflowGITHUB_TOKENinto.git/config, where anything the job then executes from the checked-out tree — build scripts, proc macros, test binaries,scripts/*.sh, the MkDocs build — can read it. On a pull request that tree is by definition unreviewed code, and nearly every job here compiles or runs it.Highest exposure was not a
ci.ymljob butweb.yml'sbuild, whose workflow-levelpermissions:grantpages: write+id-token: writewhile it runstrunk,cargo doc,pip install, andmkdocs build.Audited per site rather than applied blanket, and that mattered.
release-auto.yml'spreparejob genuinely needs the credential — it creates an annotated tag andgit push origin "$TAG", which authenticates through exactly that token. The sibling's audit concluded no job needed it; that conclusion does not transfer here, and a blanket sweep would have silently broken every future release.It's now the only checkout in the repo that keeps the token, with the reason recorded inline so the exception reads as a decision rather than an oversight. Its exposure is bounded by its trigger:
workflow_runon a completed CI run ofmain, never a pull request, so the tree it executes is already-reviewed code.antigravity-review.ymlalready had the setting and is unchanged.2. The release-tag check now fails closed
It was:
which collapses three outcomes into two — tag present (0), tag absent (2), and lookup failed (non-zero, various) — reading any non-zero as "absent". A transient network blip, auth hiccup, or rate limit therefore sent an already-released version down the
should_release=truepath, re-tagging a release the ceremony treats as immutable.Now
gh api git/matching-refs/tags/<tag>, chosen overgit/ref/tags/<tag>because it answers "absent" with HTTP 200 and an empty array — so a genuine miss can never be confused with an error, and no error-body parsing is needed. That endpoint matches by prefix, so the exact ref is compared injq.Verified against the live API rather than assumed:
v1.25.0should_release=false(correct)v1.26.0should_release=true(correct)v1.2should_release=true— a naivelength > 0would have said "already released"The step had no token of its own (it previously used only plain
git), soGH_TOKENis added —gherrors out without one rather than falling back to an ambient credential. Caught while testing, not in review.3. All 34 action references pinned to commit SHAs
Across all 8 workflows plus the composite
rust-setupaction. An upstream tag move could previously change what CI executes with no repository review. Each pin carries its original tag as a trailing comment so intent stays readable.Also aligned one
actions/upload-artifact@v4— introduced with the fuzz job in #275 — to the@v7the rest of the repo already used, before pinning it.Verification
actionlintreports the same three findings before and after (a self-hosted runner label, two pre-existing shellcheck notes) — confirmed by linting the HEAD copies side by side, so no new lint surface.🤖 Generated with Claude Code