Conversation
`run-hermetic-test-process.sh` scrubs the environment to an explicit
allowlist. It passes every other `REBORN_COV_LANE_*` variable but not
`REBORN_COV_COLLECT`, so the wrapper strips it — and
`reborn-coverage-lane-run.sh` then reads `${REBORN_COV_COLLECT:-true}`,
defaulting the stripped value back to coverage mode and invoking
`cargo llvm-cov`, whose install step was correctly skipped because the
plan resolved `coverage_mode: "none"`.
The lane fails with `error: no such command: llvm-cov`. It only fires
when coverage is off *and* the lane selects a non-empty suite list, so
it lands on PRs touching `tests/integration/*` and looks like a test
failure rather than an environment defect.
The default is the deeper problem — an unset value means "collect", so
losing the variable fails toward the more expensive branch rather than
the safer one. This change fixes the allowlist only; hardening the
default belongs with whoever owns that script.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
🚅 Deployed to the ironclaw-pr-7066 environment in ironclaw-ci-preview
|
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe hermetic test process now preserves ChangesHermetic coverage environment
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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.
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 `@scripts/ci/run-hermetic-test-process.sh`:
- Line 61: The allowlist change in run-hermetic-test-process.sh lacks a
regression test for environment propagation. Add a CI harness test that verifies
REBORN_COV_COLLECT=false reaches the child command while an unrelated variable
remains unset, and ensure the test runs whenever run-hermetic-test-process.sh
changes by updating the relevant workflow configuration.
🪄 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 Plus
Run ID: 77c72dc5-0711-4719-b1c8-656d46153828
📒 Files selected for processing (1)
scripts/ci/run-hermetic-test-process.sh
| CARGO_INCREMENTAL|CARGO_PROFILE_DEV_DEBUG|CARGO_PROFILE_TEST_DEBUG|CARGO_TEST_ARGS|\ | ||
| RUSTFLAGS|RUST_MIN_STACK|COREPACK_HOME|PLAYWRIGHT_BROWSERS_PATH|\ | ||
| PROPTEST_CASES|REBORN_COV_LANE_INDEX|REBORN_COV_LANE_MODE|\ | ||
| PROPTEST_CASES|REBORN_COV_COLLECT|REBORN_COV_LANE_INDEX|REBORN_COV_LANE_MODE|\ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add a regression test for the allowlist contract.
Test that REBORN_COV_COLLECT=false reaches the child command and that an unrelated variable remains unset. bash -n checks syntax only. It does not verify this behavior.
.github/workflows/reborn-tests.yml:695-708 supplies this variable, while scripts/ci/reborn-coverage-lane-run.sh:51-55 defaults a missing value to true. A future regression could enable coverage and invoke unavailable cargo llvm-cov.
Add the test to the CI harness and ensure it runs when scripts/ci/run-hermetic-test-process.sh changes.
As per coding guidelines, “Guardrails, checks, and hooks require regression tests ... and must run when their own files change.” As per path instructions, “behavior changes need matching workflow updates.”
🤖 Prompt for 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.
In `@scripts/ci/run-hermetic-test-process.sh` at line 61, The allowlist change in
run-hermetic-test-process.sh lacks a regression test for environment
propagation. Add a CI harness test that verifies REBORN_COV_COLLECT=false
reaches the child command while an unrelated variable remains unset, and ensure
the test runs whenever run-hermetic-test-process.sh changes by updating the
relevant workflow configuration.
Sources: Coding guidelines, Path instructions
🔎 Review · PR #7066
Submitted review →The complete trusted base-to-head comparison is a correct, narrowly scoped CI fix. Adding REBORN_COV_COLLECT to the hermetic wrapper’s explicit environment allowlist preserves the workflow-provided true/false value for the coverage lane without weakening the wrapper’s handling of unrelated variables. No actionable findings. Automatic · PR opened · attempt 1 of 3 · completed in 1m 8s Run details
|
There was a problem hiding this comment.
🔍 Review complete · PR #7066
✅ No actionable findings
The complete trusted base-to-head comparison is a correct, narrowly scoped CI fix. Adding REBORN_COV_COLLECT to the hermetic wrapper’s explicit environment allowlist preserves the workflow-provided true/false value for the coverage lane without weakening the wrapper’s handling of unrelated variables. No actionable findings.
Validation and technical details
- Verified refs/ironloop/base resolves to e9e738c and refs/ironloop/head resolves to fc944d2.
- Inspected the complete base-to-head diff: one changed allowlist entry in scripts/ci/run-hermetic-test-process.sh.
- Traced REBORN_COV_COLLECT from .github/workflows/reborn-tests.yml through the hermetic deterministic-suite wrapper to scripts/ci/reborn-coverage-lane-run.sh, which validates true/false and selects cargo llvm-cov versus ordinary cargo test.
- bash -n scripts/ci/run-hermetic-test-process.sh passed.
- A direct runtime preservation probe could not execute in this review sandbox because process substitution failed at /dev/fd/63; static inspection confirms the new case-pattern entry follows the existing allowlist mechanism exactly.
- Base:
main - Head:
ci/allowlist-reborn-cov-collectatfc944d2 - Run:
ebb36198-2761-4eee-8c7e-b435721029b6
|
This PR's CI is not evidence — the Reborn suite was skipped entirely, so here is a local proof instead. Every Reborn lane on this PR reports Since CI cannot verify this change, I ran the wrapper itself. Same script, same environment, the single allowlist token as the only variable:
Chain, end to end: CI sets Full disclosure on my earlier attempt at this proof: my first A/B compared the patched script against a copy of main's version extracted to |
|
Superseded — closing as a no-op. #6780 merged and carried this same allowlist line, so Worth keeping from the investigation, since #6780's body doesn't cover it:
|
One token. Unblocks any PR touching
tests/integration/*.The defect
scripts/ci/run-hermetic-test-process.shscrubs the environment down to an explicit allowlist. It passesREBORN_COV_LANE_INDEX,REBORN_COV_LANE_MODE,REBORN_COV_LANE_PARTITIONSandREBORN_COV_LANE_TEST_TIMEOUT— but notREBORN_COV_COLLECT, so the wrapper strips it.scripts/ci/reborn-coverage-lane-run.sh:54then reads:collect_coverage="${REBORN_COV_COLLECT:-true}"which defaults the stripped value back to coverage mode and calls
cargo llvm-cov— whose install step was correctly skipped, because the test plan resolvedcoverage_mode: "none". The lane dies witherror: no such command: llvm-cov.Why it looks like a test failure
It only fires when coverage is off and the lane selects a non-empty suite list. A PR touching
tests/integration/*gets suites on some lanes and empty lists on others, so the same run shows lanes 0/2 failing and 1/3 passing — which reads like a flaky integration test rather than an environment defect. Observed on #7040 (Reborn integration tests (0)and(2)), where every affected suite passes locally.The fix, and what it deliberately leaves alone
Adds
REBORN_COV_COLLECTto the allowlist. Nothing else.Worth flagging separately: the default is the deeper problem.
${REBORN_COV_COLLECT:-true}means an unset value is read as "collect", so losing the variable fails toward the more expensive branch instead of the safer one — a fail-open default in a script whose whole job is deterministic isolation. Tightening that belongs with whoever owns the lane script, so it is not bundled here.Provenance
Open PR #6780 already carries this same allowlist line as part of a much larger feature ("deep-link register/install gateway + private manifest source"). This PR is the fix on its own so the queue is not gated on that feature's review; #6780 will see a trivial same-content conflict on the line.
Verified:
bash -nclean; diff is one line.