Repository navigation
ci(miri): disable Miri isolation so the raw-bam proptest can run - #701
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMiri isolation prevents proptest environment-variable and regression-file lookup. The workflow documents the case limit and disables isolation for raw-pointer comparator tests. ChangesMiri workflow updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai pause |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/workflows/miri.yml:
- Around line 73-75: Update the explanatory comment near the comparator tests to
document both host accesses: proptest invokes std::env::current_dir() for
failure-persistence lookup and reads PROPTEST_* environment variables, including
PROPTEST_CASES.
🪄 Autofix (Beta)
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: c9f172e5-be32-43ff-96a1-b672a34bd912
📒 Files selected for processing (1)
.github/workflows/miri.yml
✅ Action performedReviews paused. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #701 +/- ##
==========================================
+ Coverage 93.92% 93.95% +0.03%
==========================================
Files 178 178
Lines 108062 108080 +18
==========================================
+ Hits 101494 101544 +50
+ Misses 6568 6536 -32 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The Miri workflow has failed on every run since it was added: all 13 scheduled runs aborted on `test_natural_compare_agrees_with_nul` with `getcwd not available when isolation is enabled`. The test itself is fine. Before running any case, proptest resolves the path to its `.proptest-regressions` file through `std::env::current_dir()`, and `getcwd` is unsupported under Miri's default isolation. The other 23 tests in the step pass; this one aborts the step, so the workflow has never reported a real result. That test is the one worth keeping: it is the agreement check between `natural_compare` and `natural_compare_nul`, the raw-pointer comparator this step exists to interpret. Skipping it would hollow out the gate, so disable isolation instead. These tests are pure comparator calls over in-memory byte buffers and reach for no host state of their own. Isolation also blocked the environment: an isolated `env::vars_os()` returns none of the host variables, so the `PROPTEST_CASES: "32"` cap has been silently ignored as well. It takes effect now. Verified locally: 30 passed, 0 failed, exit 0, ~16s.
7765daf to
2ebd016
Compare
Summary
The Miri workflow has never passed. Every one of its 13 runs since it was added on 2026-07-21 has failed, always in the same place:
It fails on a daily cron, so the red has gone unnoticed.
Cause
test_natural_compare_agrees_with_nulis aproptest. Before running a single case, proptest resolves the path to its.proptest-regressionsfile throughstd::env::current_dir()— andgetcwdis unsupported under Miri's default isolation. The failure is in proptest's setup, not in the code under test or in the property itself.The other 23 tests in the step pass. This one aborts the step, so the workflow has never reported a real result on the
unsafeit exists to check.Why disable isolation rather than skip the test
That test is the one most worth keeping: it is the agreement check between
natural_compareandnatural_compare_nul, the raw-pointer comparator this whole step exists to interpret under Miri. Skipping it, or filtering it out of the step, would leave the gate looking green while no longer covering the*const u8walk.Disabling isolation for this step is safe. These tests are pure comparator calls over in-memory byte buffers — they open no files, read no clock, and reach for no host state of their own. The only isolated operation in play is the cwd lookup proptest performs on their behalf. Stacked Borrows and the rest of Miri's UB checking are unaffected;
-Zmiri-disable-isolationrelaxes only host-interaction shims.Second effect, also fixed
Miri's isolation blocks environment access too: an isolated
env::vars_os()returns none of the host variables. So the workflow-levelPROPTEST_CASES: "32"— added to keep this very test tractable under the interpreter — has been silently ignored on every run, and the test has been running proptest's default case count whenever it got that far. Disabling isolation makes that cap effective for the first time. The comment above it now records the dependency so it does not get "cleaned up" later.This is also why the narrower fix does not work:
PROPTEST_DISABLE_FAILURE_PERSISTENCE=1would avoid thegetcwdcall, but proptest never sees the variable under isolation. I tried it first and it changed nothing.Verification
Run locally on this branch, with the exact command and environment the workflow uses:
Before the change, the same command without
MIRIFLAGSexits 1 on the proptest. Both directions were confirmed, as was the environment behaviour: a probe variable produces proptest's "Ignoring unknown env-var" warning with isolation off and no warning with it on.Note for #697
main-runalladds a second Miri step forfgumi-pipeline-core. Because both steps share one job underbash -e, this failure aborts the job before that step runs — so the new step has never executed in CI either. It is verified locally only. Once this lands andmain-runallpicks it up, that step will run for real.Summary by CodeRabbit