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
674 Commits 225 Branches 38 Tags
fix/docs-high-severity
Commit Graph

3 Commits

Author SHA1 Message Date
Ignacy Łątka da61d4f2eb test(tool-server): pin the ARGENT_* suite guard and correct what it documents (#859) 
Post-merge review of #707. Three findings, all from that change's own
surface; no behaviour outside the test suite moves.

## 1. Nothing pins the guard

`test/setup/clear-argent-env.ts` strips the developer's `ARGENT_*`
overrides so unit tests assert argent's shipped defaults. On a machine
with nothing exported its loop body never runs, and CI is exactly such a
machine, so no suite failure can report the guard being weakened.
Measured at `9dc7c7cfc`, each mutation applied alone to the shipped
file:

| mutation | full tool-server suite |
|---|---|
| body replaced with `export {}` | 350 files / 4300 passed, **exit 0** |
| `startsWith("ARGENT_")` -> `startsWith("ARGENT")` | 350 files / 4300
passed, **exit 0** |
| `delete process.env[name]` -> `process.env[name] = ""` |
`bind-failure-telemetry.test.ts` 3 passed, **exit 0** |
| entry removed from `setupFiles` | suite green |

All four survive every gate: `vitest`, `eslint`, `prettier`, `tsc`,
`typecheck:tests`, `knip`.

`test/clear-argent-env.test.ts` re-imports the module against sentinels
planted at call time, so the loop runs on demand rather than only when
the developer happens to have an override exported. Each mutation now
kills exactly one test, and the mapping is distinct:

```
body removed        -> removes an ARGENT_ key outright instead of blanking it
prefix underscore   -> keeps a name that shares the prefix without the underscore
delete -> blank     -> removes an ARGENT_ key outright instead of blanking it
setupFiles entry    -> is registered as a setup file, so it runs before any test module
```

`ARGENTINA_REGION` is the look-alike: it pins that the underscore is
load-bearing. The `in` check rather than a truthiness check is
deliberate -- `= ""` reads as cleared to `if (process.env.X)` while
still reaching a spawned child as a set key.

## 2. The header named the wrong file, and argued from the smallest
cases

> `ARGENT_PORT` / `ARGENT_HOST` retarget the bind that the
startup-telemetry tests inspect.

`startup-telemetry.test.ts` exists in that directory, mocks
`createHttpApp`, and asserts nothing about host or port.
`bind-failure-telemetry.test.ts` is the file that inspects the bind, and
it is the one #707's own table named. A reader following the comment
opens the sibling that literally bears that name and finds no bind
assertion.

The example list also covered only the four variables worth 9 failures,
while the widest one was absent. Guard removed, one variable, full
suite:

```
ARGENT_AUTH_TOKEN=devtoken -> Test Files 10 failed | 340 passed (350)
                              Tests 80 failed | 4220 passed | 1 skipped (4301)
```

It puts every HTTP route behind a bearer check (`src/http.ts:59`), so
ten files fail on status code alone. That is the strongest argument for
scrubbing the whole prefix instead of a hand-maintained list, and the
paragraph defending exactly that choice did not have it.

## 3. A comment #707 silently falsified

`malloc-stack-logging.test.ts:89-95` promised to "restore the ambient
value afterwards", capturing `process.env.ARGENT_IOS_CAPTURE` at module
scope. The guard clears that variable before the module is imported, so
the capture is always `undefined` and the `else` arm of the `afterEach`
restore is unreachable. Clearing in `afterEach` is what the file
actually needs and all it now claims.

`export {}` is appended to the setup file so `tsconfig.test.json` --
which includes `test/**/*` -- treats it as a module rather than a global
script; without it `typecheck:tests` fails with TS2306.

## Verification

Run at `d0cb84de5`, rebased on `9dc7c7cfc`.

- Full `packages/tool-server`: **351 files, 4306 passed, 1 skipped, 0
failed** on a clean environment, and identical with
`ARGENT_EMULATOR_GPU_MODE=host ARGENT_EMULATOR_NO_WINDOW=1
ARGENT_SIMULATOR_NO_WINDOW=1 ARGENT_PORT=9999 ARGENT_HOST=0.0.0.0
ARGENT_AUTH_TOKEN=devtoken ARGENT_IOS_CAPTURE=all-processes` exported.
- Mutation matrix above re-run against the new test: 4/4 killed,
intended test each time, control green.
- `prettier --check .` 0, `eslint . --max-warnings 0` 0, `tsc --build`
0, `typecheck:tests` 0, `knip` 0 (206 issues against the 215 ceiling,
unchanged -- the new file is not flagged).

## Checked and left alone

- The four `it.each(PLATFORMS)` retitles from #707 render correctly (`on
linux ... -gpu swiftshader`); all four sites were fixed and a sweep of
all 77 `.each(` sites in the repo found no other transposition.
- The guard is genuinely unnecessary in the other ten packages: the
union of all 53 `ARGENT_*` names read anywhere in `packages/*/src`,
exported in two opposing polarities, leaves every other suite green.
`argent-tools-client` already has its own copy from #714.
- No CI workflow exports an `ARGENT_*` variable into a vitest process,
so nothing in CI is destroyed by the scrub. `wayland-e2e.yml:46` sets
`ARGENT_EMULATOR_NO_WINDOW` at job level but runs no vitest.
- The other vestigial ambient save/restore pairs (`window-shake`,
`http-auth`, `http-upload`, `http-flow-path-boundary`,
`update-argent-tool`, `boot-device`, `boot-device-hotboot`) carry no
false prose and are harmless defence-in-depth, so they stay.
 2026-08-21 12:01:10 +02:00
Ignacy Łątka 513aca5e2e test(tool-server): stop unit tests inheriting the developer's ARGENT_* overrides (#707) 
## Problem

The unit suite inherits the developer's `ARGENT_*` shell configuration,
so tests that assert argent's shipped defaults instead assert a property
of the machine running them.

I hit this as six failures in `boot-device-hotboot.test.ts` on a Linux
box:

```
AssertionError: expected [ '-avd', 'Pixel_7_API_34', …(9) ] to deeply equal ArrayContaining ["-gpu", "swiftshader"]
-   "swiftshader"
+   "host"
```

`swiftshader` is the Linux default and `auto` the macOS one. `host` is
neither — it came from `ARGENT_EMULATOR_GPU_MODE=host` exported in my
shell, which is the documented escape hatch for hosts where swiftshader
is too slow.

`selectGpuMode` (`boot-device.ts:210`) reads the env override and
returns **before** the `process.platform === "linux" ? "swiftshader" :
"auto"` fallback. That precedence is correct and is not what this
changes. The `it.each(PLATFORMS)` parameterisation is also fine —
`setPlatform` really does control `process.platform`; the env override
simply shadows it.

Proof that the source is not implicated:

```
env -u ARGENT_EMULATOR_GPU_MODE npx vitest run test/boot-device-hotboot.test.ts
→ Tests  25 passed (25)      # unchanged source
```

## The failure reads as a source regression

This is the reason it is worth fixing rather than documenting. Someone
who has ever set `ARGENT_EMULATOR_GPU_MODE` — the variable we tell
people to set — gets six red tests naming a `-gpu` value they did not
choose, in a file they did not touch, and nothing in the output mentions
the environment. The obvious next move is to go read `selectGpuMode`,
which is correct.

## Not one file

`ARGENT_EMULATOR_GPU_MODE` is the one I tripped over; the class is every
`ARGENT_*` override, since each is by definition a user-facing switch
that changes asserted behaviour. Measured by exporting each and
re-running:

| variable | file | failures |
|---|---|---|
| `ARGENT_EMULATOR_GPU_MODE` | `boot-device-hotboot.test.ts` | 6 |
| `ARGENT_EMULATOR_NO_WINDOW` | `boot-device-hotboot.test.ts` | 4 |
| `ARGENT_SIMULATOR_NO_WINDOW` | `boot-device.test.ts` | 1 (suppresses
the `open -a Simulator.app` it asserts) |
| `ARGENT_PORT` / `ARGENT_HOST` | `bind-failure-telemetry.test.ts` | 2
each |

`bind-failure-telemetry.test.ts` is the pointed one: its header comment
already worries about colliding with the developer's real tool-server
and mocks the probe for exactly that reason — but it never considered
the env override that retargets the bind.

With all eleven `ARGENT_*` variables set at once, across the three
files:

```
before →  Test Files  3 failed (3)      Tests  9 failed | 42 passed (51)
after  →  Test Files  3 passed (3)      Tests  51 passed (51)
```

## Fix

A suite-wide setup file deletes every `ARGENT_*` key from `process.env`
before the test module graph is imported, so module-level env reads see
the cleared state too.

Clearing the prefix rather than a hand-maintained allowlist is
deliberate: an override added to `src` later is covered without a second
edit here, which is precisely the kind of follow-up edit that does not
happen. A test that exercises an override sets it itself, so nothing
depends on the ambient value — verified by the full suite staying green.

Also reorders the four `it.each(PLATFORMS)` titles. Their `%s`
placeholders ran against the `[platform, gpu]` tuple and rendered as
`"picks the hot-boot spawn args + -gpu linux on swiftshader"` — platform
and GPU mode transposed, in the first string a diagnoser reads.

## Deliberately not scrubbed

`ANDROID_HOME`, `ANDROID_SDK_ROOT`, `ANDROID_AVD_HOME`,
`ANDROID_USER_HOME`, `ANDROID_SDK_HOME`, `XDG_CONFIG_HOME`,
`LOCALAPPDATA`. I ran the full package suite with all seven pointed at
nonexistent paths: 297/297 green, so those tests already stub what they
need. Scrubbing them would be defensiveness nothing pins, and they are
shared SDK variables a test could legitimately want to read.

Tests that pin `process.platform` explicitly and restore it
(`android-binary-windows`, `command-on-path`,
`lens-tools-platform-gate`, `linux-preflight`) are correct as written
and untouched. `adb-resolve-avd-path.test.ts:54`'s
`it.skipIf(process.platform !== "win32")` is a deliberate gate on
Windows-only path parsing and stays.

## Verification

- The six `-gpu` failures reproduced at `origin/main` with the variable
exported, and pass with it cleared — same command, same shell.
- Hostile-env sweep above: 9 → 0.
- **Mutation:** changing the Linux arm of `selectGpuMode` from
`swiftshader` to `auto` is killed by four named `on linux …` tests,
while the `darwin` arm stays green. Without this change that mutation is
masked whenever the variable is set.
- Full `packages/tool-server`: **297 files, 3090 passed, 1 skipped, 0
failed** (was `6 failed | 3084 passed`).
- `eslint .` 0, `prettier --check .` 0, `tsc --build` 0 — run unpiped,
exit codes read directly.

## Unrelated, found while sweeping

`test/vega-cli-timeout.test.ts` fails roughly 1 run in 5 **at
`origin/main` with this change absent** — it touches only `PATH`, never
`ARGENT_*`. Two real-subprocess timing tests (`rejects on its own
deadline when the CLI never returns`, `reaps the ENTIRE worker tree on
timeout`) racing under parallel load. Not caused by this branch and not
fixed here.
 2026-08-17 21:50:21 +02:00
Jakub Piasecki 551c781fb0 feat(flow): support running end-to-end flows without agent (#429) 
# Flow E2E

This PR turns flows from replayable tool-call recordings into a
declarative, portable e2e test system that runs headlessly via `argent
flow run <name>` — no LLM in the loop, non-zero exit on failure,
suitable for CI.

Agents can still replay flows via `flow-execute` and get the full output
from each step after it finishes.

## Flow format

Flows are YAML files in `.argent/flows/`, now built from **directives**
interpreted by the runner rather than raw tool calls:

- `launch` — terminate + relaunch the app under test (per-platform app
id map supported); its presence marks a flow as **e2e** (controls its
own start state) vs. a **fragment** (runs against current state,
declares an `executionPrerequisite`, composable via `run:` with
cycle/depth guards).
- `tap`, `type` (focus handshake + optional submit), `scroll-to`
(momentum-free, increments by half the container, `within:` for nested
scrollers), `await`/`assert` (`visible`/`exists`/`hidden`/`text`
conditions), `wait`, `snapshot`, `echo`, and raw `tool:` as escape hatch
— still fully supported, so previously recorded flows keep working
unchanged.
- **Selectors** are `{ text, identifier, role }`; a bare string is a
loose selector resolving identifier-first, then visible text. Any
directive failure hard-stops the flow; later steps report `skip`.

## Execution engine

- Flows store no device id — the runner binds a device at replay
(explicit `--device`/`--platform` or the single booted one).
- Selector resolution runs against **platform-unified UI trees**: iOS
native view hierarchy (full testID coverage, gated on native-devtools
connecting), Android full accessibility hierarchy (full resource-id
coverage), Chromium CDP DOM, and the Vega automation toolkit. A shared
flatten/text-hoisting pass lets `text` assertions read what an
identified container visibly shows. Vega is remote-driven, so touch
directives fail with guidance to use `tv-remote`/`keyboard`.
- Steps wait for the tree to **settle** (two consecutive identical
reads) before acting; `launch` additionally gates on platform
tree-source readiness instead of silently degrading.

## Visual snapshots

`snapshot` diffs a full-resolution screenshot against baselines in
`.argent/flows/__baselines__/<flow>/`, keyed by platform + resolution,
with configurable `maxMismatch` and `--update-baselines`. The status bar
is pinned for the whole run (iOS `simctl status_bar`, Android demo mode)
so clocks/battery don't drive diffs. Baseline/current/diff images are
returned as artifacts on failure (and the written baseline on first
capture/update); a clean pass carries none, keeping reports and agent
context lean.

## Recording

The record-live workflow (`flow-start-recording` → `flow-add-step` →
`flow-finish-recording`) now emits directives automatically: coordinate
taps are rewritten to portable selector `tap:` steps (selector captured
from the pre-tap tree), a leading `restart-app` becomes `launch:`,
`flow-execute` of a sibling fragment becomes `run:`, and device ids are
stripped. The skill instructs the agent to polish the saved YAML
afterwards (fold tap+keyboard into `type:`, swipes-to-reach into
`scroll-to:`, awaits into sugar form).

## Also included

- `argent flow run` / `argent flow list` CLI with per-step report
rendering, artifact materialization, and `--json` output.
- New structured failure codes for flow validation, device resolution,
composition, and devtools readiness.
- Extensive unit test coverage (tree adapters, selector matching,
scrolling, composition, type focus, snapshots, status bar).
 2026-07-13 17:15:01 +02:00