Skip to content

Commit 152b376

Browse files
committed
refactor: T59 in PR #1964
Assisted-by: Claude Code:claude-opus-5-5
1 parent 6514a28 commit 152b376

3 files changed

Lines changed: 29 additions & 6 deletions

File tree

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
[agent] 2026-11-02: architecture refactor (structural)
2+
3+
`main` @ `cbbbdb9`, unchanged since the last run. The previous entry was a sweep (#1961), so this run did structural work. None of my 21 open PRs has merged or closed, and the discussion has no maintainer steering. The top free candidate was **T59 [#1958](https://github.com/SocketDev/socket-patch/issues/1958)**, scoring about 9 (B1, D1, C2, risk L). No open PR touches `tests/common/hermetic.rs` or `spawn_env_hygiene.rs`, so I claimed it.
4+
5+
**PR:** [#1964](https://github.com/SocketDev/socket-patch/pull/1964), ready for review. CI and Bugbot were pending when this entry was written.
6+
7+
### What changed (test-only, +69 / −82)
8+
- `hermetic::command` removes `NO_COLOR`, `CLICOLOR`, `CLICOLOR_FORCE` and `COLUMNS` (`COLOR_ENV`). It leaves `TERM` to the PTY suites.
9+
- Deleted `HOSTILE_SEEDS`, its seed loop, and the `Extra::Yarn`/`Pnpm`/`Bundle` seed-then-remove pairs. `SOCKET_UPDATE_BASE_URL`/`SOCKET_UPDATE_STATE_DIR` keep their by-name removal.
10+
- `spawn_env_hygiene`: the 2 seed tests became canaries. Each sets every flag-bound var, the token, the update vars and the color env in the test process, then asserts the piped and PTY children never see them. The can't-fail `extra_seeds_are_scrubbed_whatever_the_ambient_value` test is deleted.
11+
- Corrected the module doc on which builder pins HOME. Fixed 3 stale seed comments in e2e suites.
12+
13+
### Evidence
14+
- On `main`, `CLICOLOR_FORCE=1` fails `cli_parse_list` 68/1. On the branch it passes 69/69.
15+
- Taking `CLICOLOR_FORCE` out of `COLOR_ENV` turns both canary tests red.
16+
- With hostile color env set, the `cli`, `scan`, `update`, `rollback` and `remove` suites pass (654 tests).
17+
- `/code-review high` raised 6 findings, and all are fixed.
18+
19+
### Merges recorded
20+
None.
21+
22+
### Lessons
23+
- When a scrub is defended twice (by name and by sweep), a single mutation stays green. The canary still catches a var that only one layer removes, such as the color env.
24+
- The forced opt-outs are flag-bound, so a canary built from `GLOBAL_ARG_ENV_VARS` has to exclude them.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ _Last updated 2026-11-01T22:02Z · main @ cbbbdb9 · owner: audit-cli-tests_
6161
| T56 | 3 | Parse-suite env scrub 4 ways: `EnvScrub` byte-identical ×4 + hand lists + 91 `#[serial]`, scan `with_clean_env`, global_args own guard, none in list/remove/main (`SOCKET_JSON=true` fails 3, executed); `hermetic::try_parse` used by 2. Hand `Snap` mirrors ×4 miss 1-9 fields (executed). | #1956 | filed #1956 |
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 |
64-
| 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 | filed #1958 |
64+
| 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 |
6565
#### Handed off
6666
_None._
6767

‎register/90-refactor.md‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
### Refactor routine
2-
_Last updated 2026-11-01T23:15Z · main @ cbbbdb9_
2+
_Last updated 2026-11-02T01:20Z · main @ cbbbdb9_
33

44
#### In flight
55
| PR | Kind | Scope | State |
@@ -25,20 +25,18 @@ _Last updated 2026-11-01T23:15Z · main @ cbbbdb9_
2525
| #1955 | structural | T52 #1942: 7 PTY runners → `common/pty_io::{run,run_command}` (test-only, +172/−567) | ready; CI (82 checks) and Bugbot green |
2626
| #1948 | sweep | `cli/src/commands/{vex_consumed,vex_sources}.rs` (+ 2 tests moved to `ecosystem_dispatch.rs`) | ready; CI (ci-ok) and Bugbot green |
2727
| #1961 | sweep | `cli/src/ui/{mod,prompt,status}.rs` (`test_support.rs` clean) | ready; CI (82 checks) and Bugbot green |
28+
| #1964 | structural | T59 #1958: `tests/common/hermetic.rs` color scrub; seeds → parent-env canary (test-only, +69/−82) | ready; CI/Bugbot pending |
2829

2930
#### Merged
3031
_None._
3132

3233
#### Queue
33-
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 candidates from the audit's T50-T55:
34-
1. T50 [#1940](https://github.com/SocketDev/socket-patch/issues/1940), toolchain e2e gate ×10 drifted: **~7**, but e2e_golang* are held by #1698/#1843/#1893/#1894: land the free suites' slice.
35-
2. T55 (no issue yet), test helpers across binaries (`stderr_chatter` 5×, dir snapshot ~8×): ~6 once filed; includes a `run_bin_with_env` `extras` param so `apply_fixture::run_gem` stops copying it.
34+
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: 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).
3635
A26 #1867, A46 #1917, A44 #1918 score ~4.
3736

3837
Next sweep files, in order: `update_notifier.rs`, `path_scope.rs`, `main.rs`+`interrupt.rs`, `json_envelope.rs`; `scan/policy.rs`, `scan/render.rs` after #1856/#1884; `python_lock.rs` (2,283), `python_script.rs` (1,768), `api/{blob_fetcher,date,retry,types}.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).
3938

4039
#### Notes
41-
- First-thread PRs: `review/2026-10-v1/register/90-refactor.md`.
4240
- Check every "redundant filter" note against all consumers and derived `GlobalArgs` (the `args.rs` proxy filter, `select_one`'s `quiet()`).
4341
- Clap field doc comments are `--help` output: sweeps leave them alone.
4442
- `--cwd` defaults to `.`: absolutize before walking `ancestors()`; test via chdir under `#[serial]`.
@@ -65,3 +63,4 @@ Next sweep files, in order: `update_notifier.rs`, `path_scope.rs`, `main.rs`+`in
6563
- Left from #1939: `remove.rs` L1327 doc still names the deleted `revert_vendor_entry` (#1875); `commands/repair.rs` ~L429 copies the build-once client (use `vendored_backend::repair::ensure_client`, #1875); `ApplyRequest.manifest` → `&HashMap` (drop `records_manifest`) and `stop_on_failure` wait on #1698/#1864/#1875; split `repair()`'s phases.
6664
- /code-review pushes back on inlining a single-caller fn that is the only unit-test seam for a constant (`fetch_concurrency`'s ceiling) or that names a phase of a 350-line fn (`build_candidates`): keep those.
6765
- Left from #1955 (pre-existing in every old PTY copy): the watchdog kills with SIGHUP (`ChildKiller::kill`), not SIGKILL; prompt waits are a fixed 10 s regardless of the run's timeout; dropping the writer sends `\n`+EOF, so an empty-input run still presses Enter.
66+
- Left from #1964: `CI`/`GITHUB_ACTIONS` still reach test children (PM grandchildren may need them); a HOME pin in `hermetic::command` hits ~150 suites. Only a parent-env canary tests a scrub.

0 commit comments

Comments
 (0)