Skip to content

CI: hermetic spawns in redirect build tests (#823) - #1457

Open
Mikola Lysenko (mikolalysenko) wants to merge 2 commits into
mainfrom
agent/fix-ci-hermetic-redirect-build
Open

Mikola Lysenko (mikolalysenko) wants to merge 2 commits into
mainfrom
agent/fix-ci-hermetic-redirect-build

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

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.rs suites (cargo, composer, gem, maven, npm) started socket-patch with a bare Command::new and a hand-rolled SOCKET_* scrub. The copies disagreed: composer and maven removed every SOCKET_* var, the other three kept SOCKET_NO_CONFIG, and none of them forced SOCKET_NO_UPDATE_CHECK on. Each suite's run helper now goes through tests/common/hermetic.rs (hermetic::command + Extra::Venv), and the five files come off PENDING_RAW_SPAWNS (79 -> 74 on main) so the ratchet in spawn_env_hygiene.rs keeps them hermetic.

Suite-specific env stays: the maven suite still sets MAVEN_REPO_LOCAL and removes M2_HOME. The gem suite's hand-written BUNDLE_MIRROR__* sweep is replaced by hermetic::Extra::Bundle, which removes every ambient BUNDLE_* var. Before this, an ambient BUNDLE_GEMFILE or BUNDLE_PATH reached the crawler. Tests that need a BUNDLE_* value still pass it in through envs, which is applied after the scrub.

Scope and overlap

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).
  • gem again after the Extra::Bundle change, with BUNDLE_PATH=/nonexistent BUNDLE_MIRROR__ALL=http://127.0.0.1:1 also exported: 49/49 pass (--include-ignored).
  • Non-ignored tests in all five files plus spawn_env_hygiene (15/15) pass.
  • cargo fmt --all -- --check and cargo clippy --locked --workspace --all-features -- -D warnings (the CI clippy command) are clean.
  • cargo clippy --tests on 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:

  • Fixed: the gem runs scrubbed only BUNDLE_MIRROR__*. They now use Extra::Bundle (f3fa056).
  • vex_e2e_common::run_vex still uses a bare Command::new: CI: hermetic spawns in VEX tests (#823) #1454 moves run_vex onto the hermetic builder. Doing it here too would conflict with that PR.
  • The maven suite doesn't call jvm_env::isolate_cli; the composer suite doesn't pin COMPOSER_HOME for the socket-patch child: both gaps are already on main and aren't about SOCKET_*. They belong to a later harness slice (folding these runners into common::run_bin_with_env), not this spawn-hygiene ratchet.
  • The gem suite's bundle_env tool child has its own BUNDLE_* sweep: this is a package-manager child, not a socket-patch spawn, so it's out of scope for this slice.
  • Two duplication nits (run_socket copies; use binary_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

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
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 13, 2026 00:17
@mikolalysenko Mikola Lysenko (mikolalysenko) added the ci-perf CI / merge-queue performance finding (profiler routine) label Oct 13, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@mikolalysenko Mikola Lysenko (mikolalysenko) removed the ci-perf CI / merge-queue performance finding (profiler routine) label Oct 13, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 13, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Labeled Ready for review by the burn-down agent.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does: Moves the five e2e_redirect_{cargo,composer,gem,maven,npm}_build suites off hand-rolled SOCKET_* scrubs and onto the shared hermetic spawn builder (tests/common/hermetic.rs), and drops them from the PENDING_RAW_SPAWNS ratchet so they can't regress. The gem suite also swaps its BUNDLE_MIRROR__*-only sweep for Extra::Bundle, which removes every ambient BUNDLE_* var. Test-only; refs #823 (slice 8).

Risk: low. Only test harness code changes. The child env gets stricter (SOCKET_NO_CONFIG, SOCKET_NO_UPDATE_CHECK, telemetry-off forced on in all five), and every suite-specific var (CARGO_HOME, MAVEN_REPO_LOCAL, per-test BUNDLE_GEMFILE/BUNDLE_MIRROR__ALL) is still set after the scrub, so it survives.

Look here:

  • e2e_redirect_gem_build.rs:163:`` run_socket_env scrubs `Venv` + `Bundle` first, then applies the per-test `envs` (the dual-boot and mirror drivers depend on that order).
  • e2e_redirect_maven_build.rs:140:``` keeps MAVEN_REPO_LOCAL / `M2_HOME` removal on top of the builder.
  • e2e_redirect_composer_build.rs:311:```` standalone #[path] load of `hermetic.rs`.
  • spawn_env_hygiene.rs:48: five list entries removed (bun and cargo_shapes deliberately left; see PR body).

Verified:

  • Read the full diff and hermetic.rs (command, scrub_extra, Extra::Bundle). Checked that no test in these files reads telemetry, the update check or an ambient BUNDLE_* value that the stricter env would now hide.
  • Merged this PR and CI: hermetic spawns in VEX tests (#823) #1454 onto current main (15c2c83) locally. Both merge cleanly (spawn_env_hygiene.rs auto-merges), and spawn_env_hygiene plus the npm redirect build suite's non-ignored tests pass on the combined tree.
  • CI on f3fa056: 36 success / 50 skipped of 86, ci-ok and clippy green. Cursor Bugbot: success, no findings. No unresolved review threads. CHANGELOG.md untouched.

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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants