Repository navigation
CI: hermetic spawns in redirect build tests (#823) - #1457
Mikola Lysenko (mikolalysenko) wants to merge 2 commits into
Conversation
Five e2e_redirect_*_build suites (cargo, composer, gem, maven, npm) started socket-patch with a bare Command::new and their own SOCKET_* scrub. The copies disagreed: composer and maven removed every SOCKET_* var, the others kept SOCKET_NO_CONFIG, and none forced the update notifier off. Their run helpers now go through tests/common/hermetic.rs, and the files leave PENDING_RAW_SPAWNS (79 -> 74) so the ratchet keeps them hermetic. The gem suite's BUNDLE_MIRROR__ scrub and the maven suite's M2_HOME removal stay. Refs #823. Assisted-by: Claude Code:claude-opus-5-5
The gem redirect suite's socket-patch runs removed only BUNDLE_MIRROR__* vars. An ambient BUNDLE_GEMFILE, BUNDLE_PATH or BUNDLE_APP_CONFIG still reached the crawler and could point it at a different Gemfile or gem store than the fixture install used. Use hermetic::Extra::Bundle, which removes every BUNDLE_* var; tests that need one still pass it in. Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f3fa056. Configure here.
|
Labeled Ready for review by the burn-down agent.
Generated by Claude Code |
Final review briefWhat it does: Moves the five Risk: low. Only test harness code changes. The child env gets stricter ( Look here:
Verified:
Changes I made: none. Open questions: none. Auto-merge (squash) is armed, so approving sends it straight to the merge queue. Generated by Claude Code |
LLM Description written by Claude Code:claude-opus-5-5
Refs #823 (slice 8; the issue stays open for the remaining files).
What
Five
e2e_redirect_*_build.rssuites (cargo, composer, gem, maven, npm) startedsocket-patchwith a bareCommand::newand a hand-rolledSOCKET_*scrub. The copies disagreed: composer and maven removed everySOCKET_*var, the other three keptSOCKET_NO_CONFIG, and none of them forcedSOCKET_NO_UPDATE_CHECKon. Each suite's run helper now goes throughtests/common/hermetic.rs(hermetic::command+Extra::Venv), and the five files come offPENDING_RAW_SPAWNS(79 -> 74 on main) so the ratchet inspawn_env_hygiene.rskeeps them hermetic.Suite-specific env stays: the maven suite still sets
MAVEN_REPO_LOCALand removesM2_HOME. The gem suite's hand-writtenBUNDLE_MIRROR__*sweep is replaced byhermetic::Extra::Bundle, which removes every ambientBUNDLE_*var. Before this, an ambientBUNDLE_GEMFILEorBUNDLE_PATHreached the crawler. Tests that need aBUNDLE_*value still pass it in throughenvs, which is applied after the scrub.Scope and overlap
e2e_redirect_bun_build.rsis left for a later slice. ItsSOCKET_*scrub is folded intocache_env::scrub_ambient_bun_env, and the non-SOCKET_*half of that scrub (scrub_ambient_bun_tool_env) is added by CI: hermetic spawns in in_process tests (#823) #1452. Moving it now would duplicate that helper.e2e_redirect_cargo_shapes.rsis left out because Fix hosted/vendored cargo floor reads (#653, #651) #1429 edits it.CI cost
Test-only change: no workflow edits. A lean PR run has the same jobs before and after: about 28 Linux jobs and roughly 8 min wall clock (at most ~3.7 job-hours).
Test evidence
Run locally, with a hostile ambient env (
SOCKET_DRY_RUN=true SOCKET_JSON=true SOCKET_CWD=/nonexistent SOCKET_API_URL=http://127.0.0.1:1):cargo test -p socket-patch-cli --all-features --test e2e_redirect_{npm,composer,gem,maven}_build -- --ignored: npm 6/6, composer 5/5, gem 29/29, maven 1/1 pass (real npm 10, composer, bundler, mvn).Extra::Bundlechange, withBUNDLE_PATH=/nonexistent BUNDLE_MIRROR__ALL=http://127.0.0.1:1also exported: 49/49 pass (--include-ignored).spawn_env_hygiene(15/15) pass.cargo fmt --all -- --checkandcargo clippy --locked --workspace --all-features -- -D warnings(the CI clippy command) are clean.cargo clippy --testson the gem and npm targets reports "file is loaded as a module multiple times". That error is already there on main (same count with this diff stashed), and CI's clippy doesn't lint tests./code-review high
7 findings. 1 fixed, 6 not fixed:
BUNDLE_MIRROR__*. They now useExtra::Bundle(f3fa056).vex_e2e_common::run_vexstill uses a bareCommand::new: CI: hermetic spawns in VEX tests (#823) #1454 movesrun_vexonto the hermetic builder. Doing it here too would conflict with that PR.jvm_env::isolate_cli; the composer suite doesn't pinCOMPOSER_HOMEfor the socket-patch child: both gaps are already on main and aren't aboutSOCKET_*. They belong to a later harness slice (folding these runners intocommon::run_bin_with_env), not this spawn-hygiene ratchet.bundle_envtool child has its ownBUNDLE_*sweep: this is a package-manager child, not asocket-patchspawn, so it's out of scope for this slice.run_socketcopies; usebinary_command()): this follows the per-file pattern that CI: hermetic spawns in scan/vendor tests (#823) #1448 already merged. Consolidating every copy is a separate refactor.🤖 Generated with Claude Code
https://claude.ai/code/session_01CGnZRHxsCpiHxNFw6BqLbL
Generated by Claude Code