skills/software-mansion__argent
1
0
Fork 0
mirror of https://github.com/software-mansion/argent.git synced 2026-09-14 19:27:14 +08:00
Code Issues Packages Projects Releases Wiki Activity
708 Commits 225 Branches 38 Tags
feat/flow-script-drop-logs
Commit Graph

3 Commits

Author SHA1 Message Date
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
Hubert Gancarczyk 1a7e97fb14 feat(flows): run a local script in a bounded child process (#864) 
Adds the executor a flow `script:` step runs on: a fresh Node child per
step,
a protocol over its IPC channel, and the two watchdogs that make a hung
or
runaway script the runner's problem rather than the host's.

The step itself is not here. This branch stops at the executor and its
unit
tests; `feat/flow-script-step` stacks the YAML directive, the runner
integration, and the reference docs on top.

## What it does

- **One child per step.** Spawned with an old-space heap limit, an
explicit
working directory, and an environment built from an allowlist rather
than
  copied from the tool server.
- **A deadline and a lifeline.** A separate watchdog holds the deadline,
so a
script that wedges the event loop still dies on time; a second one reaps
the
whole process tree when the parent goes away, so a grandchild cannot
outlive
  the run.
- **A concurrency queue.** Slots are bounded per server. A step that
never gets
one is refused with a message that says so, and an aborted run frees its
slot
  immediately.
- **Log budgets.** 64 KiB per step and 256 KiB per run, counted on the
bytes
the report keeps: redaction and V8 frame collapsing both run before
anything
is counted, so what the limits bound is the size of the report rather
than
  the size of the script's writes.
- **Secret hold-back.** The scrub walks a chunk and stops where a value
could
still begin, so neither half of a value split across two chunks is
released
  on its own, and a shorter value is never taken where the longer one
  containing it has not arrived yet.
- **A failure taxonomy.** Twelve kinds, split into what the script did
(it
threw, it did not load, it exited non-zero, it wrote an unusable
`output`)
and what the host did to it (a limit, a signal, a spawn that failed, a
queue
  slot it never got).

## Docs

`packages/docs/docs/reference/configuration.mdx` lists the two
configuration
keys this branch adds, `scripts.maxTimeoutMs` and `scripts.heapLimitMb`.
The
`script:` step itself is documented with the step, on
`feat/flow-script-step`.

## Verification

`npm run build`, `npx eslint . --max-warnings 0`, `npx prettier --check
.`,
`npm run knip`, `npm run typecheck:scripts`, the test typecheck across
every
workspace, the tool-server suite (4719 passed, 1 skipped) and `npm run
test:scripts` (92 tests) are green, as is `npx docusaurus build`. The
executor's
own behaviour is covered by the eight `test/flows/script/` files added
here,
against real child processes, and the compiled `dist/` executor was
driven
against its copied runner assets over every path the review reached.

Each review fix carries the run that reproduced it before the change and
the
mutation that proves the new test fails without it: the prefix-secret
leak and
all three npm `node-options` routes were reproduced end to end through
the
executor against real child processes, and the five coverage findings
were
confirmed by re-applying the exact mutation each thread named.




<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Added reliable execution of trusted flow scripts with concurrency
control, cancellation, timeouts, memory limits, logging, and detailed
failure reporting.
* Added safeguards for process cleanup, watchdog termination, output
validation, and secret redaction.
* Added global configuration for maximum script runtime and memory
usage.
* **Bug Fixes**
* Improved handling of script failures, stalled processes, malformed
output, and child-process termination.
* **Documentation**
  * Documented script resource limits and global configuration behavior.
* **Chores**
* Updated builds and packaging to include required flow-script runtime
assets.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com>
 2026-09-07 11:34:30 +02:00
Ignacy Łątka 7400179b5b Windows support: Android + Chromium host control plane (#421) 
## What

Makes Argent run on Windows. iOS Simulator is macOS-only (Apple), so on
Windows that means the **Android + Chromium** host control plane. The
blocker was twofold: (1) no Windows `simulator-server` binary, and (2)
POSIX-only assumptions in the tool-server.

Companion radon PR builds the binary and runs the Android emulator E2E
on Windows: software-mansion/radon#134.

## Changes

**Windows-aware tool-server / resolver / bundling**
- New shared `commandOnPath()` helper — `where` on Windows, `command -v`
via `/bin/sh` on POSIX. `android-binary`, `check-deps`, and `vega-cli`
previously hardcoded `/bin/sh`, which never matches on Windows.
- `android-binary`: append `.exe` to `adb`/`emulator` in the
`$ANDROID_HOME` fallback; probe `%LOCALAPPDATA%\Android\Sdk` (Android
Studio's Windows default).
- simulator-server resolver + dispatcher + `bundle-tools` +
`download-simulator-server.sh`: ship and resolve `simulator-server.exe`
on `win32` (new `simulatorServerBinaryName()`), add `win32` to the
bundled host keys, add the `simulator-server-argent-windows.exe`
download target with PE arch validation.
- `stop-metro`: resolve listening PIDs via `netstat -ano` on Windows
(`lsof` is POSIX-only and threw ENOENT).
- `adb.ts`: AVD root resolution used `$HOME` + `startsWith("/")`
(rejects `C:\…`); now `os.homedir()` + `path.isAbsolute()`.
- **`boot-device`**: the emulator was spawned `detached` with
`stdio:"ignore"`; on Windows a detached emulator with NUL stdout/stderr
never reaches `sys.boot_completed` (verified on `windows-latest`: the
identical flag set boots fine with real handles, hangs with NUL).
Redirect the detached emulator's output to a temp log file on win32.
POSIX keeps `"ignore"`.

**Tests** — cross-platform unit coverage for `commandOnPath` (both
branches), win32 `.exe` Android resolution, win32 `.exe`
simulator-server resolution, and a win32 AVD drive-path case; dep-gate
tests now mock at the `commandOnPath` boundary so they're
platform-agnostic.

**CI** — `windows-e2e.yml` (`windows-chromium` on `windows-latest`):
runs the resolver/dep unit tests natively on Windows, confirms `adb.exe`
resolution via the SDK root, then drives a **real headless Chrome over
CDP** through the tool-server (discover → screenshot → describe → tap →
observe the DOM mutation). Flow in `scripts/ci/windows-chromium-e2e.mjs`
(also runnable locally / in Docker). The `simulator-server.exe` build +
the full **Android-emulator E2E on Windows** live in radon CI (the
binary's source is the private radon repo).

## Verification (all green)
- **Chromium control plane on real Windows** (`windows-latest`): `✓
discovered chromium-cdp-9222 → ✓ screenshot → ✓ describe → ✓ gesture-tap
→ ✓ DOM mutation observed`. Resolver/dep unit tests run natively;
`adb.exe` resolves via the SDK root (it's *not* on PATH on the runner —
the gap the resolver closes).
- **Android emulator on real Windows** (radon#134, `windows-latest` /
WHPX): the real Argent path — `boot-device` → `{ booted: true }` →
screenshot via `simulator-server.exe` → gesture-tap — all green. (The
boot-device fix above is what makes the detached emulator reach
`boot_completed`.)
- **Local clean-room E2E**: the Chromium flow runs green in a headless
Linux Docker container (fresh `npm ci`, no host pollution).
- **No regressions**: full tool-server suite (1624) +
native-devtools-ios (16); ESLint/Prettier/typecheck clean; **Wayland
E2E** (Linux AVD boot) and **Vega VVD E2E** still pass (they exercise
the changed `adb.ts` / `boot-device.ts` / `vega-cli.ts` paths).

## Notes
Android-emulator E2E needs WHPX nested virtualization — confirmed
available on the current hosted `windows-latest` image (Server 2025);
the emulator boots hardware-accelerated in ~3 min.

---------

Co-authored-by: Ignacy Łątka <claude-hubert.gancarczyk@swmansion.com>
Co-authored-by: Ignacy Łątka <latekvo@users.noreply.github.com>
 2026-07-22 13:53:32 +02:00