Repository navigation
ci(miri): gate PRs that touch the Miri-scoped crate - #712
Merged
Merged
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe Miri workflow now runs for changes under ChangesMiri workflow updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
Member
Author
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #712 +/- ##
==========================================
- Coverage 94.08% 94.08% -0.01%
==========================================
Files 180 180
Lines 109210 109210
==========================================
- Hits 102751 102750 -1
- Misses 6459 6460 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/report-cron-failure/action.yml:
- Around line 51-53: Update the gh issue list lookup that assigns existing to
request enough results for all relevant open issues by adding an explicit limit
of 1000, preserving the existing repository, label, state, JSON fields, and jq
title matching behavior.
In @.github/workflows/miri.yml:
- Around line 114-124: Move cron failure reporting out of the test-job steps
because timeouts prevent later steps from running. In .github/workflows/miri.yml
lines 114-124, add a dependent reporting job with needs: miri and the specified
schedule/failure condition; in .github/workflows/stress.yml lines 58-68, add the
equivalent job with needs: stress-tests. Have each job check out the repository
before invoking report-cron-failure, grant only these reporting jobs contents:
read and issues: write, and keep the test jobs limited to contents: read.
- Around line 39-44: Update the pull_request trigger in the Miri workflow to run
for every pull request instead of relying on the paths filter. If scoped
execution is still needed, move changed-path detection into a dedicated job and
use its result to control subsequent Miri jobs, ensuring the required gate is
not skipped for large pull requests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33ff2a31-335f-4b56-84dc-122e8de48fce
📒 Files selected for processing (3)
.github/actions/report-cron-failure/action.yml.github/workflows/miri.yml.github/workflows/stress.yml
Miri ran only on a daily cron, so the earliest a UB regression could surface was the next morning, attributed to a whole day of merges rather than to the change that introduced it. Bisecting UB is expensive; naming the commit is most of Miri's value. Add a `pull_request` trigger filtered to `crates/fgumi-raw-bam/**` and this workflow file. The filter is what preserves the reason the workflow was cron-only in the first place — a nightly-Miri regression should not block unrelated work — because a PR that does not touch the covered crate never starts the workflow and so cannot be blocked by it. What it can block is exactly the set of PRs able to introduce UB in the code Miri checks. Cost was the other objection and does not survive the current scope: the `sort` tests run in seconds under Miri, against a `test` job that takes minutes. The recompile dominates and is still not the long pole. No `push:` on `main`. A `pull_request` run tests the simulated merge commit, so a merged PR was already covered, and the cron re-checks `main` daily. Adding it would double every Miri run to shave hours off drift the cron already catches. The cron stays, with a different job: it covers a nightly/toolchain regression against unchanged code, and `main` between merges — neither of which a PR run can see. It is also where scope widens (the `fgumi-sort` FFI exclusion) without charging every PR for it. The failure-reporting step needs no change: it is already gated to `github.event_name == 'schedule'`, so a PR failure stays in that PR's checks instead of filing an issue. Verified with `actionlint`, and by checking the filter against nine paths — the covered crate and this workflow trigger it; `fgumi-sort`, `fgumi-umi`, `src/`, `Cargo.lock`, `README.md` and `check.yml` do not. Note for the runall stack: `feat-runall` adds `fgumi-pipeline-core` and a second Miri step for its `erased` module. That crate is not on `main`, so it is deliberately absent from the filter here; whichever branch lands that step must add `crates/fgumi-pipeline-core/**` alongside it or the new step will only ever run on the cron.
nh13
force-pushed
the
nh/miri-gate-unsafe-crates
branch
from
August 8, 2026 17:28
c55489f to
1b1305d
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was previously deployed
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.
Stacked on #702 — review that one first; this PR's diff against it is
.github/workflows/miri.ymlonly.Why
Miri ran only on a daily cron, so the earliest a UB regression could surface was the next morning, attributed to a whole day of merges rather than to the change that introduced it. Bisecting UB is expensive, and naming the commit is most of Miri's value.
There was a good reason it was cron-only, recorded in the workflow header: a nightly-Miri regression should not block unrelated work. Path filtering keeps that true rather than trading it away — a PR that does not touch the covered crate never starts the workflow, so it cannot be blocked by it. What it can block is exactly the set of PRs able to introduce UB in the code Miri checks.
The other objection was cost, and it does not survive the current scope: the
sorttests run in seconds under Miri, against atestjob that takes minutes.What
pull_requesttrigger filtered tocrates/fgumi-raw-bam/**and.github/workflows/miri.yml.push:onmain— apull_requestrun tests the simulated merge commit, so a merged PR is already covered, and the cron re-checksmaindaily. Adding it would double every Miri run to shave hours off drift the cron already catches.mainbetween merges. It is also where scope can widen (thefgumi-sortFFI exclusion) without charging every PR for it.github.event_name == 'schedule', so a PR failure stays in that PR's checks instead of filing an issue.Verification
actionlintclean, and the filter checked against nine paths:crates/fgumi-raw-bam/{src/sort.rs,Cargo.toml}and this workflow trigger it;fgumi-sort,fgumi-umi,src/main.rs,Cargo.lock,README.mdandcheck.ymldo not.This PR demonstrates itself. It touches
.github/workflows/miri.yml, so the new filter matched and amiricheck ran on it — the first per-PR Miri run in this repo. Measured on that run:miritestcoveragesort-correctnessSo the cost argument is settled with real numbers rather than an estimate: the whole Miri job, recompile included, is under half the
testjob and is nowhere near the critical path. Inside it, the scoped filter ran 30 tests passed, 694 filtered out — the intended narrow scope, and not a vacuous run.Two things reviewers should weigh
mainis not branch-protected, so nothing here makes the job a required check. If required checks are ever configured, note the standard GitHub trap: a required check skipped by a path filter leaves a PR waiting forever. That needs a "skipped is success" companion job, not a change here.Miri covers 2 of the 7 files carrying
#[allow(unsafe_code)]—fgumi-sorthas five, excluded becausememory_probe.rscalls mimalloc/mach2 FFI that Miri cannot execute. So a green Miri means less than it reads like. The higher-value follow-up is a cheap, stable-toolchain check incheck.ymlasserting the set ofallow(unsafe_code)-bearing files matches an expected allowlist, failing on any addition or move. That would cover all 7 and catch newunsafeappearing in an uncovered crate, which nothing does today.Follow-up for the runall stack
feat-runalladdsfgumi-pipeline-coreand a second Miri step for itserasedmodule. That crate is not onmain, so it is deliberately absent from the filter here. Whichever branch lands that step must addcrates/fgumi-pipeline-core/**alongside it, or the new step will only ever run on the cron.Risk: command output none;
unsafenone andCLAUDE.mdallowlist unchanged; memory, queue, and backpressure policy none.Fix: Run Miri on relevant pull requests while retaining daily regression coverage.
pull_requestcoverage forcrates/fgumi-raw-bam/**and.github/workflows/miri.yml.actionlintand path checks pass.