GeezerVison is showing github.com — CI: hermetic spawns in cli/covgap tests (#823) by mikolalysenko · Pull Request #1441 · SocketDev/socket-patch
Conversation
LLM Description written by Claude Code:claude-opus-5-5
Refs #823 (slice 4 of the hermetic-spawn migration; the issue stays open for the remaining files).
What was wrong
20 CLI test files (cli/, cli_*, coverage_fix_*, covgap_*) spawned socket-patch with a bare Command::new and their own hand-rolled SOCKET_* scrub. The copies disagreed. Several removed only the global flag vars, so SOCKET_API_TOKEN, SOCKET_API_URL or a per-command var like SOCKET_SKIP_ROLLBACK from a developer shell still reached the child. Two plain list runs in cli_parse_list scrubbed nothing, and none forced the update notifier off.
There was also a race in the shared builder. hermetic::command removed only the SOCKET_* names present when it swept the parent env. Test binaries that set flag-bound vars in-process (apply_env_toggles, save/restore helpers) could leak one set between the sweep and the spawn, and each suite patched that with its own removal list. Those lists had already drifted: 4 vars in one place, 2 in another, the full list in a third.
Change (test-only)
- All 20 files spawn through
tests/common/hermetic.rs and leave PENDING_RAW_SPAWNS (126 → 106), so the ratchet keeps them hermetic. Deliberate opt-ins stay as caller env: the config-layer suites still set SOCKET_NO_CONFIG=0, and the telemetry suite still re-enables telemetry.
hermetic::command now removes every flag-bound var (GLOBAL_ARG_ENV_VARS ∪ LOCAL_ARG_ENV_VARS) by name, which closes the race once and deletes the per-file lists. SOCKET_CWD joins the hostile seeds (the telemetry suite used to seed it locally). New contract test: command_drops_flag_bound_vars_set_after_it_is_built.
- No workflow change. A lean PR run keeps the same job count (~28 Linux jobs) and job-minutes.
Evidence
- Hostile shell (
SOCKET_DRY_RUN=true SOCKET_JSON=true SOCKET_API_TOKEN=… SOCKET_API_URL=http://127.0.0.1:1 SOCKET_PROXY_URL=… SOCKET_SKIP_ROLLBACK=true SOCKET_ALL_RELEASES=true SOCKET_ORG_SLUG=… SOCKET_NO_CONFIG=0) over the 19 affected targets:
- main: 11 failures. 6 spawn tests in
cli_parse_list (*_via_binary), covgap_commands_vendor::npm_only_human_vendor_does_not_read_composer_lock, plus 4 noted below.
- this branch: the 7 spawn tests pass. What remains is
cli_parse_list::defaults_match_contract (an in-process clap parse, not a spawn; out of scope here) and the 3 root-only failures below.
command_drops_flag_bound_vars_set_after_it_is_built fails against main's hermetic.rs (SOCKET_OFFLINE set after build leaked) and passes with this change. spawn_env_hygiene: 15/15.
cargo test --workspace --all-features --no-fail-fast, 249 targets: everything passes except 13 tests that need a non-root user to make a file unwritable (*_write_failure_*, *unremovable*, relax_loop_must_not_traverse_symlinked_root) and e2e_bun_lockb::binary_shared_bundled_record_hosted_pin_is_managed. My container runs as root. Each of these fails the same way with main's hermetic.rs and test files, so none are from this PR. CI runs as non-root.
cargo fmt --all -- --check and cargo clippy --locked --workspace --all-features -- -D warnings: clean.
/code-review high: findings not fixed here
Follow-ups for #823: the 106 remaining PENDING_RAW_SPAWNS files (mostly e2e_*, in_process_*, vex_*), the PTY spawns, and the in-process parse in cli_parse_list.
🤖 Generated with Claude Code
https://claude.ai/code/session_01CQp9sCPnUPTSFrEGsvTQmQ
Generated by Claude Code
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Assisted-by: Claude Code:claude-opus-5-5
Twenty CLI test files spawned socket-patch with a bare Command::new and
a hand-rolled SOCKET_* scrub. The copies disagreed: several only removed
the global flag vars, so SOCKET_API_TOKEN, SOCKET_API_URL or a
per-command var like SOCKET_SKIP_ROLLBACK from a developer shell still
reached the child, cli_parse_list's two plain list runs scrubbed
nothing, and none forced the update notifier off.
All 20 now spawn through the shared hermetic builder in
tests/common/hermetic.rs and leave PENDING_RAW_SPAWNS, so the ratchet
keeps them hermetic. Deliberate opt-ins stay as caller env: the config
layer suites still set SOCKET_NO_CONFIG=0 and the telemetry suite still
re-enables telemetry. Suites whose in-process tests mutate SOCKET_* keep
their unconditional removals for the spawn race.
Refs #823
Assisted-by: Claude Code:claude-opus-5-5
Some test binaries set flag-bound SOCKET_* vars in their own process
(apply_env_toggles, env save/restore helpers). The hermetic builder only
removed names present when it swept the parent environment, so a var
set by a parallel test before the spawn still reached the child, and
suites patched that with their own, already diverging, removal lists.
hermetic::command now removes every flag-bound var by name, so the
per-file lists go away, and SOCKET_CWD joins the hostile seeds. A new
contract test sets vars after the builder runs and checks none leak.
Refs #823
Assisted-by: Claude Code:claude-opus-5-5
cursor[bot] reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 6d44a29. Configure here.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Ready for review (burn-down agent)
- Head:
6d44a29a
- CI: 86/86 settled on the current head (36 success, 50 path-skipped); no main-wide failures.
- Mergeable against
main, no CHANGELOG.md change.
- Bugbot: reviewed
6d44a29, no findings; no open review threads.
- Reviewer focus: the shared
tests/common/hermetic.rs helper that the CLI/covgap tests now use to spawn the binary with a scrubbed environment.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Oct 12, 2026
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Labels
Ready for review
Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|