mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
Ignacy Łątka
86a2668fb0
Test env leakage, both directions. **Out** — six `argent-tools-client` suites pointed `HOME`/`USERPROFILE` at a temp dir, deleted the dir, and never restored, so a `--no-isolate` run handed every later file a home that no longer exists. Lifted the redirect into a shared `test/helpers/home-redirect.ts`; the three suites that already hand-rolled the save/restore moved onto it too. **In** — `packages/telemetry` had no `setupFiles` at all, so its unit tests read the developer's shell. Added `test/setup/clear-telemetry-env.ts` (ARGENT_ prefix sweep + `DO_NOT_TRACK` + the cloud-agent detector's markers), wired via `setupFiles`, following the `@argent/tool-server` precedent — and dropped the per-file guards it replaces. **Pinned, not just fixed** — a `test/setup/assert-env-restored.ts` in each of the three packages fails any suite that leaves a watched variable changed, so dropping a restore can no longer pass. Each watches `HOME` and `USERPROFILE`, plus `PATH` (`argent-tools-client`, `tool-server`) or `CI` and the five `OTEL_EXPORTER_OTLP_*` (`telemetry`) — the ones the sweep deliberately leaves ambient and individual tests then set by hand. Each of the three `clear-*-env` setup files has a pin test that re-imports it against planted sentinels; `packages/tool-server`'s existing copy gains the mixed-case one it was missing. `packages/tool-server` is in the diff because it has the same two gaps: it originated the prefix sweep (both pre-existing copies were case-sensitive, so they are fixed together) and it has the largest redirect surface of the three — 12 files hand-rolling the redirect plus 17 going through `scopeTempHome` — with no guard. It is already green under one; deleting `scopeTempHome`'s restore turns 17 files red. Two of its suites restored `HOME` by assigning back a saved value that can be `undefined`, writing the string `"undefined"`; the guard caught both on a host with no ambient `HOME`, and they are fixed here. `windows-e2e.yml` runs seven `tool-server` files through the config this branch edits, but neither that config nor `test/setup/**` was in its `paths:` filter — so the new setup file would have reached windows-latest unverified. Added both. No docs change: test infrastructure only, no user-facing behaviour. Fixes #1028 Fixes #839 Same-class problems found while reviewing this and filed rather than folded in: #1068 (`argent-cli` fails under `DO_NOT_TRACK=1`), #1069 (the `ios-device-runner` suites pin only `HOME`, so the redirect is inert on Windows — its second half, the restore that wrote the string `"undefined"`, is fixed here because the new guard caught it), #1070 (`useTempHome` shares one saved-`HOME` slot, so a nested `scopeHome()` restores a deleted dir), #1071 (`tool-server`'s sweep is prefix-only, so an exported `ADB_SERVER_SOCKET` reddens its suite), #1072 (`external-devices.test.ts` reads the developer's real `~/.argent/providers`) and #1073 (`flow-script-environment.test.ts` fails with no ambient `HOME`). <details> <summary>#1028 — probe results, six files</summary> `test/zz-home-probe.test.ts` from the issue body, run per suite. Vitest's sequencer orders by cached duration and falls back to file size, so the cache is cleared before each run to keep the tiny probe last: ``` rm -rf node_modules/.vite/vitest ../../node_modules/.vite/vitest npx vitest run test/<suite>.test.ts test/zz-home-probe.test.ts --no-isolate --no-file-parallelism ``` | suite | before | after | | --- | --- | --- | | `launcher-state` | 1 failed \| 23 passed — `argent-state-test-… exists=false` | 24 passed | | `launcher-spawn` | 1 failed \| 8 passed — `argent-spawn-test-… exists=false` | 9 passed | | `launcher-version-gate` | 1 failed \| 7 passed — `argent-version-gate-test-… exists=false` | 8 passed | | `launcher-duplicate-spawn` | 1 failed \| 10 passed — `argent-dup-test-… exists=false` | 11 passed | | `launcher-exit` | 1 failed \| 1 passed — `argent-exit-test-… exists=false` | 2 passed | | `link-config` | 1 failed \| 43 passed — `argent-link-config-test-… exists=false` | 44 passed | | `launcher-sweep` (control, fixed in #713) | passed | passed | | `launcher-ps-guard` (control, fixed in #713) | passed | passed | The probe itself is a diagnostic and is not part of the diff — `test/setup/assert-env-restored.ts` is the durable guard, and it needs no `--no-isolate` to fire. </details> <details> <summary>#839 — the stated repro is stale; here is what is actually left</summary> The issue was filed 2026-08-18. #714 landed 2026-08-19 and hand-enumerated the variables inside `index.test.ts` (a `CONSENT_ENV_KEYS` `beforeEach`) and `base-props.test.ts` (eight `delete`s). On `aa90873b5` all three named repros are green: ``` $ DO_NOT_TRACK=1 npx vitest run --root packages/telemetry → 319 passed $ ARGENT_TELEMETRY=0 npx vitest run --root packages/telemetry → 319 passed $ CURSOR_AGENT_WORKER_ID=x npx vitest run --root packages/telemetry → 319 passed ``` What remains is the structural gap the issue names: the protection is per-file and per-variable, nothing pins it, and `vitest.config.ts` still has no `setupFiles`. This PR moves the property to the setup file and deletes the two hand-rolled guards, so the setup file is what holds it — see the mutation section, where removing the `setupFiles` line reproduces the issue's 17 and 1 exactly. Post-change sweep, one hostile value per run, 329 tests each: `DO_NOT_TRACK=1`, `ARGENT_TELEMETRY=0`, `ARGENT_TELEMETRY_DEBUG=1`, `CLAUDE_CODE_ENVIRONMENT_KIND=byoc`, `CLAUDE_CODE_ENTRYPOINT=remote`, `CLAUDE_CODE_REMOTE_SESSION_ID=x`, `CURSOR_AGENT_WORKER_ID=x`, `CURSOR_WORKER_POOL_NAME=x`, `GITHUB_ACTIONS=true`, `GITHUB_ACTOR=copilot`, `GITHUB_WORKFLOW_REF=…copilot-swe-agent.yml@refs/heads/main`, `REPLIT_AGENT=1`, `ARGENT_HOST`/`ARGENT_PORT`/`ARGENT_AUTH_TOKEN`, `CI=1`, `CI=false`, `OTEL_EXPORTER_OTLP_ENDPOINT`, `OTEL_EXPORTER_OTLP_HEADERS`, the real CI combination, and all eleven at once — 329 passed every time. `GITHUB_ACTIONS` is a ci-info vendor input as well as a copilot-agent marker, so clearing it does move `isCi()` — but only its GitHub Actions branch, and an Actions run also sets `CI`, which `isCi()` answers ahead of any vendor check. `GITHUB_ACTOR` and `GITHUB_WORKFLOW_REF` appear in neither `ci-info/vendors.json` nor `ci-detect.ts`'s generic list, so they cost nothing. The rest of the ci-info surface and `OTEL_*` are deliberately left alone; the setup file says so. </details> <details> <summary>Mutation testing — every guard, applied by hand, verified to fail, reverted</summary> | mutation | result | | --- | --- | | drop `setupFiles` from `packages/telemetry/vitest.config.ts` | `DO_NOT_TRACK=1` → 19 failed (**17 in `index.test.ts`**, matching the issue, + the two registration pins) | | drop `CURSOR_WORKER_POOL_NAME` from `CLEARED_ENV_VARS` | drift test red | | move an `env.` read into `src/detect/vendor.ts` | drift test red, naming `detect/vendor.ts` | | rewrite a read as `env["X"]`, `const { X } = env`, `const { X }: NodeJS.ProcessEnv = env` or `const { X = "d" } = env` | drift test red | | add `cloud-agent-detect.ts` to `UNSWEPT_SRC_FILES` | drift test red | | revert `toUpperCase()` in any of the three sweeps | that package's mixed-case case red | | drop `restoreHome()` from one suite / from all nine | 1 file / 9 files red, each naming its deleted dir | | drop `scopeTempHome()`'s restore in `tool-server` | 17 files red | | drop the hand-rolled `CI` restore in `base-props.test.ts` | red, naming that file (green before; under `--no-isolate`, 7 unrelated failures in `otel-endpoint.test.ts`) | | narrow either guard's filter to variables that were already set | `assert-env-restored.test.ts` red | | replace the leaked half of the guard's message with a constant | `assert-env-restored.test.ts` red | | rename one function's `env` parameter in `cloud-agent-detect.ts` | drift test red — the read is found under the new binding | | narrow any guard's filter to a subset of `SCOPED_ENV_VARS` | `assert-env-restored.test.ts` red — the pin plants every entry | | drop the hand-rolled OTLP restore in `otel-endpoint-live.test.ts` | red, naming all four leaked variables | | move the guard's `AMBIENT` snapshot inside `assertEnvRestored` | `assert-env-restored.test.ts` red in all three packages | | delete the guard's `afterAll(...)` registration | `assert-env-restored.test.ts` red in all three packages | | drop `restoreHome()` from `artifacts.test.ts`'s two `afterEach` | red — the file-level guard catches a per-test omission | | drop ps-guard's `PATH` restore | red, naming the file | | empty `leakedEnvVars`, or drop a name from `SCOPED_ENV_VARS` | `assert-env-restored.test.ts` red | | replace the helper's delete branch with a plain assignment | `home-redirect.test.ts` red | | drop `USERPROFILE` from the helper | `home-redirect.test.ts` red | </details> <details> <summary>Local CI parity</summary> ``` npx vitest run --root packages/argent-tools-client 213 passed | 1 skipped npx vitest run --root packages/argent-tools-client --no-isolate 213 passed | 1 skipped npx vitest run --root packages/telemetry 329 passed npx vitest run --root packages/telemetry --no-isolate 329 passed npx vitest run --root packages/tool-server 6166 passed | 1 skipped npx tsc --noEmit -p packages/{argent-tools-client,telemetry,tool-server}/tsconfig.test.json clean npm run format / npm run lint / npm run knip clean ``` </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Improved test isolation by consistently restoring temporary environment settings after test runs. - Added safeguards that detect leaked environment settings and flag incomplete cleanup. - Added coverage to ensure telemetry-related environment variables are cleared before tests and do not affect results. - Added cross-platform checks for environment variable handling, including mixed-case names. - Expanded validation for home-directory redirection and restoration behavior. - Added checks confirming environment cleanup runs in the correct order across test suites. <!-- end of auto-generated comment: release notes by coderabbit.ai -->