[Mikola Lysenko (mikolalysenko)]

LLM Description written by Claude Code:claude-opus-5-5

Fixes #1956

Test-only. This implements register row T56 (living document Part 5, tests). Each cli_parse_* suite and cli_global_args kept the developer's ambient SOCKET_* env out of its parses in its own way:

• four byte-identical EnvScrub copies, each with a drifted hand list;
• scan's with_clean_env;
• cli_global_args' save_and_clear_global_env;
• nothing at all in list, remove and main. On main, SOCKET_JSON=true fails 2 remove tests and 1 list test.

Separately, four hand Snap mirrors of GlobalArgs each claimed to cover "every parsed field". get and repair missed 4 fields, vendor missed 9 and vex missed 1.

Changes

• One scrub. Every parse helper goes through common/hermetic.rs try_parse, which removes every SOCKET_* var once per process.
• New common/clap_parse.rs:
  • try_parse_with_env sets the given vars for one parse and removes them after. It replaces vendor's parse_vendor_with_env body, vex's parse_with_env, scan's parse_scan_with_env body, and the hand set_var/remove_var blocks in repair and cli_global_args.
  • try_parse_keeping_opt_outs is for list and remove. Those binaries also call list::run/remove::run in-process on parallel threads, so their scrub keeps hermetic::FORCED_OPT_OUTS (SOCKET_NO_CONFIG, SOCKET_NO_UPDATE_CHECK, SOCKET_TELEMETRY_DISABLED) and forces them on. Without that, those run() calls could read a real socket login and send telemetry; /code-review caught this. As a result their parses expect no_telemetry: true.
  • global_defaults() is a hand-written GlobalArgs struct literal, the independent default oracle. A new global field fails to compile there until its default is written.
  • Globals compares GlobalArgs through Debug.
• Deleted:
  • the 4 EnvScrub structs and their 4 hand SOCKET_ENV_VARS lists;
  • SCAN_ENV_VARS and with_clean_env;
  • save_and_clear_global_env/restore_global_env;
  • 6 scrub self-tests, which only tested the deleted lists, plus scan's snap, used only by its self-test.
• Snapshots.
  • The 4 Snap mirrors keep their command-local fields plus common: Globals, so every global field is compared.
  • vex's Snap had been used only by its scrub test. Its 5 vex-command tests now compare whole snapshots instead of one field each.
  • The no_vlt_install_cleanup tests in get, repair, vendor and vex are now whole-snapshot comparisons.
  • The defaults tests in list, remove, scan and cli_global_args compare the whole GlobalArgs against the oracle instead of a hand-picked subset.
• #[serial]: 150 → 105.
  • get loses all 34: no test there sets env anymore.
  • Tests that don't parse lose theirs, for example vendor_appears_in_subcommand_list.
  • repair, scan, vendor, vex and cli_global_args keep #[serial] on every parsing test, because some tests in each binary set a SOCKET_* var and clap reads env during every parse. The issue suggested dropping these too, but that would race.
• About 150 lines of comments are gone: the hermeticity essays, Snap rationale and history notes.

Evidence

• SOCKET_JSON=true SOCKET_YES=true SOCKET_STRICT=garbage cargo test -p socket-patch-cli --test 'cli_parse_*' --test cli_global_args: all 11 suites pass (321 tests). On main this fails remove and list.
• rg 'struct EnvScrub|SOCKET_ENV_VARS' crates/socket-patch-cli/tests finds nothing.
• Red check: I added --maven-config none to get's long_yes_sets_yes and left want unchanged. It fails now (cli_parse_get.rs:131), and it passes on main.
• cargo clippy --workspace --all-features -- -D warnings is clean. cargo clippy over the 11 test targets with -D warnings is clean too.

Size

+418 / −1,471, all test code; 0 production lines.

Behavior change

None. The diff touches only test code.

Risk

Low. Only test code changes, and no cfg(windows)/cfg(unix) code is touched. One thing to know: hermetic.rs is also changed by #1964. This PR doesn't edit it; it only calls try_parse, scrub_process_socket_env and FORCED_OPT_OUTS, which #1964 keeps.

/code-review high

9 findings. 6 are fixed:

• the list/remove opt-out loss (2 findings);
• the cli_global_args hand set/remove that leaked a var on a panic (2);
• scan's subset defaults test (1);
• a stray /// (1).

3 are not fixed:

• Globals compares via Debug instead of a derived PartialEq on GlobalArgs. src/args.rs is held by Sweep cli args.rs: inline, dedupe tests, trim docs #1842. A derive can replace Globals after Sweep cli args.rs: inline, dedupe tests, trim docs #1842 lands.
• scan's min_severity test sets SOCKET_MIN_SEVERITY by hand around overrides(), which reads env at run time. It leaks only if overrides() itself panics.
• No test checks that the Once scrub clears ambient values. The hostile-env run above is that check. A per-binary tripwire would just be the deleted self-tests again.

🤖 Generated with Claude Code

https://claude.ai/code/session_011f9aeSfv2chfPtLgeW8LYF

---

Note

Low Risk
Changes are confined to test code under socket-patch-cli/tests; no production CLI behavior is modified.

Overview
CLI parser contract tests no longer scrub SOCKET_* env vars per file. cli_global_args and all cli_parse_* suites now route parses through common/hermetic::try_parse (process-wide scrub) and the new common/clap_parse.rs helpers.

Added shared helpers: try_parse_with_env for env-binding tests (replaces hand set_var/remove_var and duplicate EnvScrub/with_clean_env/save_and_clear_global_env); try_parse_keeping_opt_outs for list and remove, which also call run() in-process and must keep FORCED_OPT_OUTS (so defaults assertions expect no_telemetry: true); global_defaults() plus a Globals wrapper for full GlobalArgs snapshot compares.

Removed: four copy-pasted EnvScrub lists, scan’s SCAN_ENV_VARS/with_clean_env, global save/restore helpers, and six “scrub list completeness” self-tests. Snapshot structs nest globals under common: Globals instead of flattening incomplete hand mirrors.

Test hygiene: many #[serial] attributes drop where parses no longer mutate env (e.g. cli_parse_get); suites that still set env during parse stay serial. cli_parse_main scrubs before parse_argv_with_shortcuts. Default/regression checks compare the whole global surface to the oracle rather than cherry-picked fields.

^{Reviewed by Cursor Bugbot for commit c211c60. Configure here.}

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

✅ 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 c211c60. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Ready for review at c211c60a.

• CI: 82/82 checks on the head passed or were skipped (32 success, 50 skipped). No main-wide failures.
• Bugbot: reviewed c211c60a, no findings. No unresolved review threads.
• Mergeable against main, and CHANGELOG.md is untouched.

Slack announcement didn't go out this run (the Slack send tool wasn't available), so the next run will retry it.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief

What it does: Test-only refactor. Replaces four drifted per-suite EnvScrub copies (and two other scrub helpers) in the CLI parse suites with the shared, process-wide hermetic::try_parse scrub, plus a small tests/common/clap_parse.rs helper (env-setting parse, list/remove opt-out parse, a hand-written global_defaults() oracle, and a whole-GlobalArgs compare). No production code changes.

Risk: low. It only touches crates/socket-patch-cli/tests/. Coverage goes up: per-field asserts are now whole-struct snapshot compares, so get, repair and vendor check all 27 global fields instead of a subset. The only deleted tests are 6 self-tests of the deleted scrub lists.

Look here

• common/clap_parse.rs:12 try_parse_with_env: the scrub must run before set_var, and it does.
• common/clap_parse.rs:31 try_parse_keeping_opt_outs: list and remove keep the forced opt-outs.
• common/clap_parse.rs:51 global_defaults(): the oracle. Its 27 fields match the tripwire in cli_global_args.
• cli_parse_remove.rs:34 / cli_parse_list.rs:49: the defaults now expect no_telemetry: true.
• cli_global_args.rs:551 and :758: the largest rewrites.

Verified

• I read the full diff and compared #[test] names and #[serial] coverage per binary, base vs head.
• The 9 touched binaries pass. Test count went from 252 to 246; the −6 are exactly the deleted scrub self-tests.
• With a hostile env (SOCKET_JSON/YES/STRICT/API_TOKEN/CWD set), head passes all 246. Base fails cli_parse_list with SOCKET_JSON=true, which confirms the bug this fixes.
• Clippy with -D warnings is clean on lib, bins and the touched test targets. A local --all-targets run fails on both base and head, only in files this PR doesn't touch (local toolchain issue; CI clippy is green).
• CI 82/82 passed or skipped on the head (ci-ok and clippy green). Bugbot reviewed with no findings. No unresolved threads. CHANGELOG.md is untouched.

Non-blocking notes

• The comment at clap_parse.rs:24-30 (and "run() reads no developer config" in the body) overclaims. Hand-built run() tests in list and remove never parse, so they can start before the Once scrub runs. This isn't a regression, since base had no scrub there.
• The scrub's starts_with("SOCKET_") is case-sensitive, so a mixed-case Socket_Json on Windows would get through. Same behavior as the existing hermetic.rs.

Auto-merge is armed, so approving sends it straight to the merge queue.

---

Generated by Claude Code

[Wenxin Jiang (Wenxin-Jiang)]

[agent] Reviewed c211c60a5a4f735d800865304fe05fb54eb59bdc. Checked hermetic parser initialization, serialized env tests, explicit defaults and whole-argument snapshots. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.