GeezerVison is showing github.com — Scrub color env in hermetic test children by mikolalysenko · Pull Request #1964 · SocketDev/socket-patch
Conversation
LLM Description written by Claude Code:claude-opus-5-5
Fixes #1958
Register row T59 (register); living document Part 5 (tests).
Why
hermetic::command removed the SOCKET_* env but none of the env the CLI's color and width policy reads (ui::color_enabled, ui::width_of). An ambient CLICOLOR_FORCE=1 put ANSI codes into captured stdout. Its HOSTILE_SEEDS were set and then removed by the same list, so a dropped scrub line could never show up in a test.
Changes (test-only)
tests/common/hermetic.rs
- Adds
COLOR_ENV (NO_COLOR, CLICOLOR, CLICOLOR_FORCE, COLUMNS), which command removes. pty_command inherits this. TERM is left alone because the PTY suites choose their own.
- Deletes
HOSTILE_SEEDS, its seed loop and the false "turns tests red" doc. Deletes the Extra::Yarn/Pnpm/Bundle seed-then-remove pairs and their docs.
- Keeps by-name removal of
SOCKET_UPDATE_BASE_URL/SOCKET_UPDATE_STATE_DIR. They aren't flag-bound, and the deleted seed loop was the only thing removing them by name.
- The module doc now says that only
run_bin_with_env (and suites that call jvm_env::isolate_cli) pin HOME. Moving the pin into command would change HOME for about 150 suites and their package-manager grandchildren. That is a separate, riskier change, so this PR only corrects the doc.
tests/spawn_env_hygiene.rs
- Replaces
command_never_leaks_its_hostile_seeds and the PTY variant with command_scrubs_hostile_ambient_env and pty_command_scrubs_hostile_ambient_env_and_keeps_caller_env. Both set every GLOBAL_ARG_ENV_VARS/LOCAL_ARG_ENV_VARS name (except the 3 forced opt-outs), the token, the update vars and the color env in the test process, then assert the child never sees any of them.
- Deletes
extra_seeds_are_scrubbed_whatever_the_ambient_value. It could not fail, because the ambient-seeded Extra tests above it already cover the same thing.
- Stale comments: fixed the hostile-seed comments in
e2e_golang_build.rs, e2e_vendored_production.rs and vendor/e2e_golang_redirect.rs.
Evidence
- On
main @ cbbbdb9, CLICOLOR_FORCE=1 cargo test -p socket-patch-cli --test cli_parse_list fails populated_manifest_plain_lists_full_record_via_binary (68/1). On this branch it passes 69/69, and also with CLICOLOR_FORCE=1 NO_COLOR= COLUMNS=20.
- Red→green: with
CLICOLOR_FORCE removed from COLOR_ENV, both canary tests fail with ambient CLICOLOR_FORCE must be scrubbed.
- With
CLICOLOR_FORCE=1 COLUMNS=20, the cli, scan, update, rollback and remove suites pass: 654 tests, including the PTY render and prompt tests.
spawn_env_hygiene passes 19/19. clippy -D warnings is clean on spawn_env_hygiene. e2e_vendored_production's clippy --tests errors are pre-existing on main, and this PR changes only comments there.
Size
+69 / −82 across 5 test files. No production lines change.
Behavior
None in the product. Test children no longer inherit NO_COLOR/CLICOLOR/CLICOLOR_FORCE/COLUMNS, and a test that wants them sets them explicitly, after command.
Risk
Low. The change is test-only and no cfg(windows) code changed.
/code-review high raised 6 findings and all are fixed: the by-name update-var removal, 2 stale comments, the doc reflow, the PTY test's Option round-trip, and the canary now seeding every flag-bound var.
🤖 Generated with Claude Code
https://claude.ai/code/session_011f9aePX7rzgjWUhucTZmxi
Note Low Risk
Test-only changes to hermetic spawn helpers and hygiene tests; no production code paths modified.
Overview
Hermetic test spawns no longer inherit terminal color/width env (NO_COLOR, CLICOLOR, CLICOLOR_FORCE, COLUMNS), which the CLI uses for output formatting—so ambient values like CLICOLOR_FORCE=1 stop polluting captured stdout assertions.
The old HOSTILE_SEEDS pattern (set hostile values then remove them in the same builder) is removed because it could never fail if a scrub line was dropped. spawn_env_hygiene now seeds the full hostile set in the test process (flag-bound SOCKET_*, token/update vars, color env) and asserts the child never sees them; the same approach replaces the PTY canary. Package-manager Extra scrubs (Yarn/Pnpm/Bundle) drop their seed-then-remove steps; command still removes SOCKET_UPDATE_BASE_URL / SOCKET_UPDATE_STATE_DIR by name. Docs/comments in a few e2e files are updated to match.
Product behavior is unchanged—test harness only (~+69/−82 lines).
Reviewed by Cursor Bugbot for commit df7d599. Configure here.
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
hermetic::command now removes NO_COLOR, CLICOLOR, CLICOLOR_FORCE and
COLUMNS, so an ambient CLICOLOR_FORCE=1 no longer puts ANSI codes into
captured stdout (it failed a cli_parse_list test). TERM is left to the
PTY suites.
The HOSTILE_SEEDS list and the Yarn/Pnpm/Bundle seeds are deleted:
each seed was removed right after it was set, so a dropped scrub could
never show. spawn_env_hygiene now sets the hostile values in the test
process and asserts the child (piped and PTY) never sees them. The
module doc now says which builder pins HOME.
Assisted-by: Claude Code:claude-opus-5-5
The canary now sets every GLOBAL/LOCAL_ARG_ENV_VARS name, so a new
flag-bound var is covered without a hand-kept list. command keeps the
by-name removal of SOCKET_UPDATE_BASE_URL and SOCKET_UPDATE_STATE_DIR
that the deleted seed loop used to give. Stale seed comments in the
golang and vendored-production suites now point at spawn_env_hygiene.
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 df7d599. Configure here.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
[burn-down agent] Labeled Ready for review at df7d599.
- CI: 82/82 check runs on df7d599 passed or were skipped (32 success, 50 skipped). The PR is mergeable and doesn't touch
CHANGELOG.md.
- Bugbot reviewed df7d599 and found nothing new. No review threads are open.
- For the reviewer: the change is test-only. Hermetic test children no longer inherit
NO_COLOR/CLICOLOR/CLICOLOR_FORCE/COLUMNS. spawn_env_hygiene now seeds hostile values in the parent instead of setting and removing them in the same builder.
- Slack: not announced. The Slack send tool wasn't available in this run, so the next run should retry.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Final review brief
What it does. Test-only. hermetic::command now also strips the CLI's color/width env (NO_COLOR, CLICOLOR, CLICOLOR_FORCE, COLUMNS) from spawned test children, so an ambient CLICOLOR_FORCE=1 can no longer put ANSI codes into captured stdout. It also deletes the HOSTILE_SEEDS set-then-remove pairs, which could never catch a dropped scrub. In their place, canary tests set the hostile values in the test process and assert the child never sees them.
Risk: low. No production code changes. The scrub only gets stricter. The one behavior change is that test children no longer inherit COLUMNS/NO_COLOR, and the PTY and render suites pass in CI.
Look here
- hermetic.rs:40: the new
COLOR_ENV list. TERM is deliberately not on it.
- hermetic.rs:63-82:
command. The seed loop is gone, and SOCKET_UPDATE_* are still removed by name.
- spawn_env_hygiene.rs:245:
hostile_ambient(), which seeds every flag-bound var except the forced opt-outs, plus the token, update and color vars.
- spawn_env_hygiene.rs:265 and :361: the command and PTY canaries. Both are
#[serial] and restore the env through with_ambient.
Verified
- I read the full diff (5 test files, +69/−82).
cargo test -p socket-patch-cli --test spawn_env_hygiene: 19/19 pass.CLICOLOR_FORCE=1 COLUMNS=20 cargo test --test cli_parse_list: 69/69 pass. The PR reports that main fails this test.- clippy
-D warnings is clean on spawn_env_hygiene.
- No
HOSTILE_SEEDS references remain, and CHANGELOG.md is untouched.
- CI:
ci-ok and clippy are green, with 32 checks passing and 50 skipped. Cursor Bugbot passed. There are no unresolved threads.
Changes I made: none.
Open questions: none. Note that command still leaves HOME alone, and the PR explicitly defers that riskier change.
Auto-merge is armed, so approving sends this straight to the merge queue.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Nov 2, 2026
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.
[agent] Reviewed df7d599bde11417b8194bd6f8534b5626db59b24. Checked color/environment scrubbing and hostile-ambient regression coverage for regular and PTY children. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.
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.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Labels
arch-refactor
PR opened by the scheduled architecture refactor routine
Ready for review
Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review
refactor
Structural change: duplicated code or logic, missing abstraction, layering, dead code
Milestone
No milestone
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.
|
Wenxin Jiang (Wenxin-Jiang)
left a comment