Skip to content

Commit c11fd39

Browse files
committed
refactor: T56 in PR #1978 (parse-suite env scrub)
Assisted-by: Claude Code:claude-opus-5-5
1 parent 8be42b1 commit c11fd39

3 files changed

Lines changed: 66 additions & 30 deletions

File tree

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
[agent] 2026-11-02: architecture refactor (structural)
2+
3+
`main` @ `cbbbdb9`, unchanged since the last run. The previous entry was a sweep (#1975), so this run did structural work. None of my 25 open PRs has merged or closed, and the discussion has no maintainer steering.
4+
5+
I chose **T56 [#1956](https://github.com/SocketDev/socket-patch/issues/1956)**. It scores about 10: D3 (four `EnvScrub` copies, `with_clean_env`, the `cli_global_args` save/restore and four `Snap` mirrors), C2 and risk L, because it is test-only. No open PR touches the nine suites; I checked by diffing all 68 open heads locally. `common/hermetic.rs` is changed by #1964, so this PR calls into it but doesn't edit it.
6+
7+
**PR:** [#1978](https://github.com/SocketDev/socket-patch/pull/1978), ready for review. CI is green on all 82 checks, and Bugbot found no issues.
8+
9+
### What changed (test-only, +418 / −1,471)
10+
- Every `cli_parse_*` parse and `cli_global_args` goes through `hermetic::try_parse`.
11+
- The new `tests/common/clap_parse.rs` holds:
12+
- `try_parse_with_env`, which sets vars for one parse and removes them after;
13+
- `try_parse_keeping_opt_outs`, for list and remove, whose in-process `run()` tests need `FORCED_OPT_OUTS`;
14+
- `global_defaults()`, a hand-written `GlobalArgs` struct literal used as the oracle;
15+
- `Globals`, which compares through `Debug`.
16+
- Deleted:
17+
- the 4 `EnvScrub` copies and their hand lists;
18+
- `SCAN_ENV_VARS` and `with_clean_env`;
19+
- `save_and_clear_global_env`/`restore_global_env`;
20+
- 6 scrub self-tests and scan's `snap`.
21+
- The 4 `Snap` mirrors now carry `common: Globals`. Before, get and repair missed 4 global fields, vendor 9 and vex 1.
22+
- `#[serial]` went from 150 to 105. get dropped all 34 of its own. The mixed suites keep theirs, because they set `SOCKET_*` vars.
23+
24+
### Evidence
25+
- With `SOCKET_JSON=true SOCKET_YES=true SOCKET_STRICT=garbage`, all 11 parse suites pass (321 tests). On `main`, list and remove fail under this env.
26+
- I added `--maven-config none` to get's `long_yes_sets_yes` with `want` unchanged. That turns the test red now; on `main` it passes.
27+
- Clippy is clean, both on the workspace and on the 11 test targets with `-D warnings`.
28+
- `/code-review high` raised 9 findings. 6 are fixed: the list/remove opt-out loss, the hand set/remove that leaked a var on a panic, scan's subset defaults test, and a doc nit. The PR gives a reason for each of the other 3.
29+
30+
### Merges recorded
31+
None.
32+
33+
### Lessons
34+
- A process-wide `Once` scrub is wrong for any test binary that calls a command's `run()` in-process. It also removes the forced opt-outs (`SOCKET_NO_CONFIG`, `SOCKET_TELEMETRY_DISABLED`) that `run()` reads. Keep those opt-outs and force them on.
35+
- A `Once` scrub never runs again. Every `set_var` in such a binary has to go through a helper that removes the var before any assertion, or a failing assertion leaks the var into later tests.

‎register/20-audit-cli-tests.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ _Last updated 2026-11-02T03:52Z · main @ cbbbdb9 · owner: audit-cli-tests_
5858
| T53 | 3 | `hermetic::command` removes `SOCKET_PROXY_URL`, so a fetch with no mock reaches the live public proxy: `apply_invariants` L355 does (executed). | | to verify |
5959
| T54 | 3 | The `--json` reader ratchet (`envelope_helper_copies`) is name-based: misses 7 `fn envelope(` parsers; stale allowlist entries pass. Part of C32. | #1089 | to verify |
6060
| T55 | 3 | Suites that include `common` keep their own `has_command` ×9, `sha256_hex` ×16, `git_sha256_file`/`assert_run_ok` ×4, `npm_run` ×3; `stderr_chatter` ×5 + inline ×6 is a dead filter (executed); `rollback_json` copies 4 envelope readers (drifted). | #1959 | filed #1959 |
61-
| T56 | 3 | Parse-suite env scrub 4 ways (`EnvScrub` ×4, hand lists, `with_clean_env`, none in list/remove/main: `SOCKET_JSON=true` fails 3, executed); hand `Snap` mirrors miss 1-9 fields. | #1956 | filed #1956 |
61+
| T56 | 3 | Parse-suite env scrub 4 ways (`EnvScrub` ×4, hand lists, `with_clean_env`, none in list/remove/main: `SOCKET_JSON=true` fails 3, executed); hand `Snap` mirrors miss 1-9 fields. | #1956 | in PR #1978 |
6262
| T57 | 3 | The `cli_parse_<cmd>` suites re-pin flattened `GlobalArgs` one test per flag (~85 tests) beside `cli_global_args`' every-flag × every-subcommand matrix; their field lists drift (apply/rollback `bool_flags` lack 5). Follows T56. | | to verify |
6363
| T58 | 3 | 21 `tests/common` selftests run in each of ~104 includers (~2K redundant runs; 14 binaries include `cache_env` twice); the "no `cfg(test)` in integration crates" comment ×3 is false (executed). | #1957 | filed #1957 |
6464
| T59 | 3 | `hermetic::command` leaves `CLICOLOR_FORCE`/`NO_COLOR`/`COLUMNS`/`CI` ambient (`CLICOLOR_FORCE=1` fails a list test, executed); `HOSTILE_SEEDS` seed then remove the same list (can't catch anything); HOME pin only on `common::run`. | #1958, #1964 | in PR #1964 |

‎register/90-refactor.md‎

Lines changed: 30 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,56 +1,56 @@
11
### Refactor routine
2-
_Last updated 2026-11-02T07:35Z · main @ cbbbdb9_
2+
_Last updated 2026-11-02T09:20Z · main @ cbbbdb9_
33

44
#### In flight
55
| PR | Kind | Scope | State |
66
|---|---|---|---|
77
| #1842 | sweep | `cli/src/args.rs` (+ the Deno guard moved into `lib.rs`) | ready |
8-
| #1855 | sweep | `commands/{bun_preflight,composer_hints,context,fetch_stage}.rs` | ready; CI needs a fresh run (Depot) |
9-
| #1849 | structural | A01 #1832: `utils/cargo_build_cache.rs` | ready; CI and Bugbot green |
10-
| #1864 | sweep | `commands/{hosted_bundle,hosted_unwind,list,lock_cli,mod,pypi_reinstall}.rs` (+ `rollback.rs` dead branch) | ready; CI and Bugbot green |
11-
| #1870 | structural | A27 #1869: `api/client.rs`, one serve GET and one vendor retry loop | ready; CI and Bugbot green |
12-
| #1875 | sweep | `commands/{remove,repair}.rs` | ready; CI and Bugbot green |
13-
| #1884 | sweep | `scan/discovery.rs` (+ T18 #1859; tests to core `ranking.rs`, `lock_inventory/tests.rs`, `ui/text.rs`) | ready; CI and Bugbot green |
14-
| #1892 | sweep | `scan/gc.rs` | ready; CI and Bugbot green |
15-
| #1897 | sweep | `scan/hosted.rs` (all but `run_redirect_selected_untracked`, T24; tests to core `hosted/guidance.rs`, `formats/pnpm/mod.rs`) | ready; CI and Bugbot green |
16-
| #1903 | sweep | `scan/hosted/{nuget,takeover}.rs` | ready; CI and Bugbot green |
17-
| #1907 | sweep | core `constants.rs`, `utils/{cargo_home,composer_version,concurrent,containment,digest,durability,env_compat,failpoint}.rs` (+ `socket_dir.rs`, `purl_key.rs` tests) | ready; CI and Bugbot green |
18-
| #1914 | sweep | core `utils/{http,mod,hatch_workspace,hatch}.rs` (`line_endings.rs` clean) | ready; CI and Bugbot green |
19-
| #1920 | sweep | core `utils/{notice,npm_range,package_manager,pep440,pipenv,purl}.rs` (+ `target.rs` callers) | ready; CI and Bugbot green |
20-
| #1923 | sweep + A20 #1848 | core `utils/{redact,repo_root,serde,toml_edit_ext,uri,uv_workspace}.rs` (+ `vex/product.rs`, `redirect/upstream/npm.rs`, advisory lint moved to `vendor/service_fetch.rs`) | ready; CI and Bugbot green |
21-
| #1929 | sweep | `commands/{update,user_patches,vendor_orphans,vlt_heal}.rs` (+ `InstallChannel::id` in core `update/channel.rs`) | ready; CI and Bugbot green |
22-
| #1936 | structural | A54 #1932: `formats/gem/{gemspec,manifest}.rs`, `vex/product.rs` (`for_each_code_line` → `code_lines`) | ready; CI and Bugbot green |
23-
| #1939 | sweep | `commands/vendored_backend/{mod,repair}.rs` | ready; CI and Bugbot green |
24-
| #1944 | structural | T51 #1941: 11 `tests/apply` suites + `common/envelope.rs` onto `apply_fixture` (test-only, −635 net) | ready; Bugbot clean; `hosted-e2e` red on a live 403, not this diff |
25-
| #1955 | structural | T52 #1942: 7 PTY runners → `common/pty_io::{run,run_command}` (test-only, +172/−567) | ready; CI and Bugbot green |
26-
| #1948 | sweep | `commands/{vex_consumed,vex_sources}.rs` (+ 2 tests moved to `ecosystem_dispatch.rs`) | ready; CI (ci-ok) and Bugbot green |
27-
| #1961 | sweep | `cli/src/ui/{mod,prompt,status}.rs` (`test_support.rs` clean) | ready; CI and Bugbot green |
28-
| #1964 | structural | T59 #1958: `tests/common/hermetic.rs` color scrub; seeds → parent-env canary (test-only, +69/−82) | ready; CI and Bugbot green |
29-
| #1967 | sweep | `cli/src/{json_envelope,update_notifier,main,interrupt,path_scope}.rs` (path_scope tests only, #1864) | ready; CI and Bugbot green |
30-
| #1972 | structural | T60 #1968 slice 1: `tests/docker_common` + 15 `docker_e2e_*` suites (test-only, +260/−1,113) | ready; CI and Bugbot green |
31-
| #1975 | sweep | core `api/{blob_fetcher,date,retry,types}.rs` | ready; CI and Bugbot green |
8+
| #1855 | sweep | `commands/{bun_preflight,composer_hints,context,fetch_stage}.rs` | ready; CI needs a re-run (Depot) |
9+
| #1849 | structural | A01 #1832: `utils/cargo_build_cache.rs` | ready; green |
10+
| #1864 | sweep | `commands/{hosted_bundle,hosted_unwind,list,lock_cli,mod,pypi_reinstall}.rs` (+ `rollback.rs` dead branch) | ready; green |
11+
| #1870 | structural | A27 #1869: `api/client.rs`, one serve GET and one vendor retry loop | ready; green |
12+
| #1875 | sweep | `commands/{remove,repair}.rs` | ready; green |
13+
| #1884 | sweep | `scan/discovery.rs` (+ T18 #1859; tests to core `ranking.rs`, `lock_inventory/tests.rs`, `ui/text.rs`) | ready; green |
14+
| #1892 | sweep | `scan/gc.rs` | ready; green |
15+
| #1897 | sweep | `scan/hosted.rs` (all but `run_redirect_selected_untracked`, T24; tests to core `hosted/guidance.rs`, `formats/pnpm/mod.rs`) | ready; green |
16+
| #1903 | sweep | `scan/hosted/{nuget,takeover}.rs` | ready; green |
17+
| #1907 | sweep | core `constants.rs`, `utils/{cargo_home,composer_version,concurrent,containment,digest,durability,env_compat,failpoint}.rs` (+ `socket_dir.rs`, `purl_key.rs` tests) | ready; green |
18+
| #1914 | sweep | core `utils/{http,mod,hatch_workspace,hatch}.rs` (`line_endings.rs` clean) | ready; green |
19+
| #1920 | sweep | core `utils/{notice,npm_range,package_manager,pep440,pipenv,purl}.rs` (+ `target.rs` callers) | ready; green |
20+
| #1923 | sweep + A20 #1848 | core `utils/{redact,repo_root,serde,toml_edit_ext,uri,uv_workspace}.rs` (+ `vex/product.rs`, `redirect/upstream/npm.rs`, advisory lint moved to `vendor/service_fetch.rs`) | ready; green |
21+
| #1929 | sweep | `commands/{update,user_patches,vendor_orphans,vlt_heal}.rs` (+ `InstallChannel::id` in core `update/channel.rs`) | ready; green |
22+
| #1936 | structural | A54 #1932: `formats/gem/{gemspec,manifest}.rs`, `vex/product.rs` (`for_each_code_line` → `code_lines`) | ready; green |
23+
| #1939 | sweep | `commands/vendored_backend/{mod,repair}.rs` | ready; green |
24+
| #1944 | structural | T51 #1941: 11 `tests/apply` suites + `common/envelope.rs` onto `apply_fixture` (test-only, −635 net) | ready; `hosted-e2e` red on a live 403, not this diff |
25+
| #1955 | structural | T52 #1942: 7 PTY runners → `common/pty_io::{run,run_command}` (test-only, +172/−567) | ready; green |
26+
| #1948 | sweep | `commands/{vex_consumed,vex_sources}.rs` (+ 2 tests moved to `ecosystem_dispatch.rs`) | ready; green |
27+
| #1961 | sweep | `cli/src/ui/{mod,prompt,status}.rs` (`test_support.rs` clean) | ready; green |
28+
| #1964 | structural | T59 #1958: `tests/common/hermetic.rs` color scrub; seeds → parent-env canary (test-only, +69/−82) | ready; green |
29+
| #1967 | sweep | `cli/src/{json_envelope,update_notifier,main,interrupt,path_scope}.rs` (path_scope tests only, #1864) | ready; green |
30+
| #1972 | structural | T60 #1968 slice 1: `tests/docker_common` + 15 `docker_e2e_*` suites (test-only, +260/−1,113) | ready; green |
31+
| #1975 | sweep | core `api/{blob_fetcher,date,retry,types}.rs` | ready; green |
32+
| #1978 | structural | T56 #1956: 9 `cli_parse_*`/`cli_global_args` suites onto `hermetic::try_parse` + new `common/clap_parse.rs` (test-only, +418/−1,471) | ready; green |
3233

3334
#### Merged
3435
_None._
3536

3637
#### Queue
37-
Structural, scored `3·B + 2·U + 2·D + 2·C − risk`. Nothing merged since 2026-10-29, so most candidates ≥6 stay blocked: A30 #1887 (~8.7, #1886), T09 #1858 (~7, #1864/#1698), A41 #1899 (~6, #1891/#1829/#1906), A43 #1900 (~6, #1863/#1698), T01 #1835 / T25 #1877 (#1698), A53 #1931 / A57 #1935 (`ruby_crawler.rs`, #1882/#1895/#1943), A55 #1933 (`redirect/mod.rs`). Free test-only: T60 #1968 rest (~7, after #1972: CI REQUIRED export, `assert_api_exercised`, `GIT_SHA256_FN` ×3, bash summary greps), T61 #1969 (~6; `e2e_bun_lockb.rs` held by #1905), T50 #1940 (~7; land the suites not held by #1698/#1843/#1893/#1894), T55 #1959 (~7, ~35 files; `tests/apply` is #1944), T56 #1956 (~7), T58 #1957 (~6).
38+
Structural, scored `3·B + 2·U + 2·D + 2·C − risk`. Nothing merged since 2026-10-29, so most candidates ≥6 stay blocked: A30 #1887 (~8.7, #1886), T09 #1858 (~7, #1864/#1698), A41 #1899 (~6, #1891/#1829/#1906), A43 #1900 (~6, #1863/#1698), T01 #1835 / T25 #1877 (#1698), A53 #1931 / A57 #1935 (`ruby_crawler.rs`, #1882/#1895/#1943), A55 #1933 (`redirect/mod.rs`). Free test-only: T60 #1968 rest (~7, after #1972: CI REQUIRED export, `assert_api_exercised`, `GIT_SHA256_FN` ×3, bash summary greps), T61 #1969 (~6; `e2e_bun_lockb.rs` held by #1905), T50 #1940 (~7; land the suites not held by #1698/#1843/#1893/#1894), T55 #1959 (~7, ~35 files; `tests/apply` is #1944), T58 #1957 (~6).
3839
A26 #1867, A46 #1917, A44 #1918 score ~4.
3940

4041
Next sweep files, in order: `scan/{rollout,rollout_args,socket_yml_args,withdrawn}.rs` (free); `scan/policy.rs`, `scan/render.rs` after #1856/#1884; `python_lock.rs` (2,283), `python_script.rs` (1,768), `api/vendor_prefetch.rs`, crawler rows; `group_commit.rs` (2,163, region); `tests/apply/*` rows not covered by #1944 (their notes list fake tests). Held: `vlt_preflight.rs`, `agent_download.rs`, `apply.rs`, `get.rs`, `vendor.rs` (#1698); `rollback.rs` (#1864); `utils/fs.rs`, core `lib.rs` (#1891); `lock_fragments.rs`/`pdm_lock.rs` (#1898); `poetry_lock.rs` (#1865/#1898); `pnpm_workspace.rs` (#1863); `process.rs` (#1895); `go_env.rs` (#1876); `cargo_workspace.rs` (#1866). Leftovers from merged-pending sweeps: `ui/text.rs` sweep and the text-shaping move out of `ui/mod.rs` (#1884), `StatusLine::stderr(json, silent)` → one `report` bool (scan/mod.rs, lock_cli.rs) (#1961); `toml_edit_ext` deletion and `uv_workspace` fold (#1923); 7 `build_*_purl` wrappers + `parse_jsr_purl`, purl's PurlKey asserts → `purl_key.rs`, pipenv `installed_major`'s `_root` (#1920); String `read_capped` wrapper, `lock_fragments`/`pdm_lock` `pub mod` → `pub(crate)` (#1914); `CargoCrawler::cargo_home`, `vendor/jvm` digest copies, async `fs::is_symlink` (#1907); `api_auth_fallback_warning` → `json_envelope` and `PlanNote` → `RunWarning` (callers in `vendor.rs`/`apply.rs`/`vex.rs`, #1698), `store_variants` test → core `npm_crawler.rs` (#1829/#1891/#1906/#1915) (#1948); `PathScope::matches` (1 caller, rollback L1814, in a loop) → `bind` once, plus `BoundScope::matching_pattern_indices` for rollback's `raw()` re-parse with `expect` (#1864); `AppliedVia` one-variant mirror + `from_core` (apply.rs, #1698) (#1967).
4142

4243
#### Notes
4344
- Want issues (output-changing, found in sweeps): `ui::shell_word` misses `"`/`%VAR%` on cmd; `scan/gc.rs` `has_ledger` probes `--cwd` but `run_vendor_gc` reads `project_root()` (C43); gem stale probe dedups by purl and drops the platform suffix; `hatch::require_pip_installer_documents` ignores the `envs` override; `npm_range_admits(">=", v)` admits all; npm `upstream_registry_fallback` leaks `?token=` (use `redact_urls_in`); `vlt_heal` swallows `load_state`/lock read errors; `strip_url_userinfo`'s scp branch keeps `u:p@`.
4445
- Check every "redundant filter" note against all consumers and derived `GlobalArgs` (the `args.rs` proxy filter, `select_one`'s `quiet()`).
45-
- Clap field doc comments are `--help` output: sweeps leave them alone.
46+
- Clap field doc comments are `--help` output: leave them.
4647
- `--cwd` defaults to `.`: absolutize before walking `ancestors()`; test via chdir under `#[serial]`.
4748
- `vendor/cargo_config.rs` (~L165-L190) has a third `.cargo/config` pick: fold into `config_layer_paths` when swept.
4849
- Build with `CARGO_PROFILE_DEV_DEBUG=0 CARGO_INCREMENTAL=0` (disk). CI's clippy has no `--tests`.
4950
- Root-sandbox failures that pass in CI: 2 chmod tests in `tests/repair`, and core `copy_tree` symlinked-root, `vlt_heal` unremovable-lock, `pypi_poetry`/`pypi_requirements` wire-failure tests.; `in_process_redirect`'s 3 chmod tests
50-
- The sandbox proxy refuses branch deletions: name branches right the first time (`arch-refactor/sweep-scan-gc-policy` is stale).
51+
- The proxy refuses branch deletions: name branches right first time (`sweep-scan-gc-policy` is stale).
5152
- `run_redirect_selected` still takes `Option<Envelope>` keyed on `common.json`; making it a value touches `get.rs` (#1698) and T24.
5253
- Making a `pub` core item private can leave rustdoc links to it; unlink in the same commit.
53-
- Commit every edit before a mutation test.
5454
- Test runners that pin `--cwd` must not also set the child's cwd to the project, or a crawler reading the process cwd passes (found by /code-review on #1944).
5555
- Left from #1929: `UpdateArgs.pin_version` as `semver::Version` waits on `lib.rs` (#1842); `sweep_orphan_vendor_dirs` forwarder on `vendor.rs` (#1698).
5656
- Left from #1936: `vendor/gem.rs` `plan_gemfile_edit`, `vex/discover/gem.rs` (~L252) and `redirect/upstream/gem.rs` (~L661) still hand-scan `=begin`; route them through `manifest::code_lines` (the pin writer refuses a Gemfile whose duplicate `gem` line sits in `=begin`). `code_lines`'s heredoc heuristic reads `x <<FLAG` (a shift) as a heredoc and closes a plain `<<ID` at an indented terminator.
@@ -60,4 +60,5 @@ Next sweep files, in order: `scan/{rollout,rollout_args,socket_yml_args,withdraw
6060
- Left from #1967: core `UpdateTimeouts::from_env` and the notifier both parse `SOCKET_UPDATE_TIMEOUT_MS`; core caps `connect` at 10 s, the notifier doesn't (A09/C19).
6161
- Left from #1964: `CI`/`GITHUB_ACTIONS` still reach test children; a HOME pin in `hermetic::command` hits ~150 suites. Test a scrub with a parent-env canary.
6262
- Left from #1975: `blob_fetcher` `on_progress`/`OnProgress` (callers in `fetch_stage.rs` #1855, `repair.rs` #1875, `rollback.rs` #1864) and `BlobFetchResult.success` (literals in #1855's tests); `concise_fetch_error` re-parses the client's text; `store_verified_blob` vs `stream_cache_entry_atomic` drift. `scan --json` serializes `BatchPackagePatches`: keep a serialize test.
63+
- Left from #1978: T57 can use `clap_parse::global_defaults()`; `derive(PartialEq)` on `GlobalArgs` (after #1842) retires `Globals`. The scrub is `Once`: set vars via `try_parse_with_env`; binaries calling `run()` in-process keep `FORCED_OPT_OUTS`.
6364
- Left from #1972: `run_container` takes no extra args (sbt `run_cell`, `docker_vendor_common::docker_command`, npm vlt smoke still build `docker run`); `host.docker.internal` URL ×15. Docker can't run here: build the 15 binaries with `--features docker-e2e`, run host-side tests.

0 commit comments

Comments
 (0)