Files
software-mansion__argent/packages/argent-tools-client/test/setup/assert-env-restored.ts
Ignacy Łątka 86a2668fb0 test: stop the suite and the process environment leaking into each other (#1058)
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 -->
2026-09-09 12:38:51 +02:00

45 lines
2.3 KiB
TypeScript

import { afterAll } from "vitest";
// Suites in this package point HOME (and USERPROFILE, which os.homedir() reads
// on Windows) at a temp dir and then delete that dir; some do the same to PATH
// so a stub is found instead of the real binary. Restoring is the half that is
// easy to omit and impossible to notice: under the shipped `isolate: true` each
// file gets its own fork, so a suite that leaves a variable naming a deleted
// directory takes the evidence with it when the fork exits, and `--no-isolate`
// hands that dead value to every later file. That is #1028.
//
// test/home-redirect.test.ts pins the helper's contract; this pins the call
// sites. Registered as a setup file, its afterAll is the last one to run for
// every test file — vitest's default `sequence.hooks: "stack"` unwinds in
// reverse registration order, and a setup file registers before the test module
// is imported — so it sees whatever the file's own afterAll left behind.
/** The variables a suite here changes and must put back. */
export const SCOPED_ENV_VARS = ["HOME", "USERPROFILE", "PATH"];
// A PATH, or an OTLP header, runs to well over a kilobyte, and the tail of it is
// never what changed. Unset is spelled out rather than stringified, so a restore
// that assigned an undefined saved value back — writing the string "undefined" —
// does not report itself as `undefined -> undefined`.
const abbreviate = (value: string | undefined): string =>
value === undefined ? "(unset)" : value.length <= 60 ? value : `${value.slice(0, 60)}…`;
// Read at module load, which for a setup file is before the test module is
// imported. Snapshotting inside the hook instead would compare the environment
// with itself and pass whatever the file left behind.
const AMBIENT = Object.fromEntries(SCOPED_ENV_VARS.map((name) => [name, process.env[name]]));
/** Throws naming every variable the file failed to put back, or returns. */
function assertEnvRestored(): void {
const leaked = SCOPED_ENV_VARS.filter((name) => process.env[name] !== AMBIENT[name]).map(
(name) => `${name}: ${abbreviate(AMBIENT[name])} -> ${abbreviate(process.env[name])}`
);
if (leaked.length === 0) return;
throw new Error(
`this file left the process environment modified; put it back in the same hook that ` +
`changed it. ${leaked.join("; ")}`
);
}
afterAll(() => assertEnvRestored());