mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
fix/docs-high-severity
2 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Ignacy Łątka
|
5d8fc10799 |
test: stop tests binding machine-wide resources they do not own (#714)
## Why Three flaky or failing tests were diagnosed separately this week, and all three turned out to be one defect wearing different clothes: **a test binding a machine-wide resource it does not own**, so a second concurrent run — another checkout, a CI matrix cell, a developer's own app — perturbs it. Those three are fixed in #707, #709 and #711. Each of those was found by chasing a symptom, and each audit covered only its own resource type. This is the systematic sweep: all **448 test files across 13 packages**, plus the 4 `node --test` scripts. (The sweep itself ran over the 400 files that existed when it started; merging `main` brought the count to 448, and every category below was re-run over the full set — 25 files assign `env.HOME` and none omit `USERPROFILE`, one test file calls `pkill`/`pgrep` (`vega-cli-timeout.test.ts`, scoped out to #711 below), and no `listen()` names a fixed port.) ## Findings | Category | Files checked | Real findings | |---|---|---| | Network endpoints (bind / probe / unix socket) | 448 | 4 | | Filesystem — shared temp namespace (`os.tmpdir()` and literal `/tmp`) | 448 | 8 | | Filesystem — real `$HOME` / `~/.argent` | 448 | 4 (28 files) | | Ambient environment | 448 + all `src` readers | 12 | | Device / emulator / SDK state | 448 | 2 | | Process table (`pgrep` / `pkill` / fixed-pattern sweeps) | 448 | **0** | | In-process globals leaking across files | 13 packages × 13 modes | **0** | Every finding was reproduced with a concrete instrument before being touched, and the instrument re-run after. Representative: - **`stop-tools.test.ts`** ran the real `stop-metro` against port 59999, which `lsof`s and **SIGTERMs whatever it finds**. With a squatter there: `16 passed` → `1 failed`, and the squatter logged `received SIGTERM #1`. Now port 1. - **`bind-failure-telemetry` / `lens-relay-telemetry`** were gated on the real `~/.argent/flags.json`. Enabling the documented `tool-server-event-log` flag: `4 passed` → **`4 failed`**, and records were appended to the developer's real event log. - **LogFileWriter (12 files)** — its constructor `mkdir -p`s `os.homedir()/.argent/tmp`. Instrument: run each of the 16 test files that reference a writer, or a blueprint that builds one, under a scratch `HOME`/`USERPROFILE`, then test `$HOME/.argent/tmp` for existence. 4 created it; the 12 that actually construct a writer now all call `scopeTempHome`, and none creates it. - **`screen-recording.test.ts`** — the device ids are fixtures and `vi.useFakeTimers()` seeds its clock from the real time, so only the millisecond a run starts in separates two runs' `.mp4` paths, and each test deletes the path it derived. Self-concurrent: **2 of 3 rounds failed** → 0 of 4. Scoping `TMPDIR` removes the window rather than narrowing it. - **`DO_NOT_TRACK=1`** → telemetry `282 passed` → **`13 failed`**. **`npm_config_user_agent=pnpm|yarn|bun`** → installer `535 passed` → 4/5/3 failed. **19 tests required a real `adb`** — PATH shadow + empty HOME: `30 passed` → **`19 failed`**. - A fixed `/tmp/argent-nd-<udid8>.sock` **destroyed a concurrent holder's live socket** (proven by inode) in **3** files that run the native-devtools factory. Two of them also fail outright: `native-devtools-app-state` lost **3 of 3** rounds of two concurrent runs on `expected 'unregistered' to be 'connected'`. All three now derive the UDID from `process.pid`. - `flow-visual`, `react-profiler/dump`, `file-inputs`, `http-upload` — shared `/tmp` paths and prefix scans, each reproduced with a squatter. In `http-upload` the collision surfaced *as a timeout*, inside `vi.waitFor`. (`flow-chromium-boot` was fixed on this branch too, but main's #585 rewrote that file afterwards and the merge kept main's copy, so it is byte-identical to `origin/main` here and carries no fix.) ## Clean, with the evidence - **Process table** — an `LD_PRELOAD` exec interposer over a full run (507 execs) plus all 12 other suites. Excluding `vega-cli-timeout.test.ts`, which is #711's and does sweep the table (`pkill -f '^sleep 6910[0-9]$'` at line 134): no `pgrep`/`pkill`/`killall`/fixed-pattern sweep anywhere, and **no test signals a pid it did not spawn**. - **Real device state** — **zero** exec attempts of real `adb` / `simctl` / `emulator` / `avdmanager` / `vega`; no test asserts a device list. That list was too narrow: `workspace-reader-integration` spawned the real `yarn` / `pnpm` / `bun` / `pod` / `eas` / `expo`, which is the second finding in this row. - **Cross-file in-process leakage** — structurally impossible: vitest 4.1.9 resolves `pool: "forks"` / `isolate: true`, confirmed by a scratch experiment (distinct pids, no env leak, module `Map` = 1,1,1). 120 default-mode runs across 12 packages produced 1 failure. - **`scripts/`** — 4 files, 86 tests: `mkdtemp` throughout, no ports, no sweeps, writes nothing. ## Verification Each fix was mutated back and killed by a **named** test under its instrument — e.g. `× reads a plain absolute path`, `× returns null when only the extensionless binary exists`, `× pins both names os.homedir() consults, at the same directory`. Restores verified with `git diff` / `git status`, never a substring grep. **Pinned by an instrument only:** the socket fix for `ios-only-blueprint-gate`, the `~/.argent/tmp` and `~/.argent/flags.json` fixes, the `/tmp` fixture cleanup, the `stop-metro` port change (the test asserts `stopped: false` / `pids: []`, which hold for either port whenever nothing is listening) and the `workspace-reader` `PATH` pin (only `node` is asserted; the other seven probes are not) are not pinned by any suite assertion — reverting them still passes, and only the external squatter / scratch-`HOME` instruments catch them. They are pollution fixes. The `native-devtools-app-state` socket fix is the exception in the other direction: reverting it turns two concurrent runs red. Gates, unpiped and read by exit code: `eslint .` 0, `prettier --check .` 0, `tsc --build` 0, `typecheck:tests` 0. Suite, run as six shards: tool-server **4287 passed, 1 skipped, 0 failed**; argent-cli 462; telemetry 306; argent-tools-client 191; configuration-core 122; argent-mcp 80; argent-installer 555. All other packages and `test:scripts` green, identical to baseline. `lens-tools-platform-gate.test.ts` times out at the 5s default under full-suite load and passes at `--testTimeout=30000` (24.7s, almost all of it transform). It predates this branch and is untouched by it. ## Scope This branch is **test-only** — no `src/` file is touched. Both production defects the sweep turned up are separated out, so each can be reviewed as the user-facing fix it is: `ps` without `-ww` orphaning live tool-servers is #713, and #852 is `ensureToolsServer` respawning on every call when `ARGENT_HOST` is exported — `buildToolsServerEnv` passes the inherited value to the child while the state records the literal `127.0.0.1` the health check then probes. The second surfaced here as eight stranded `fake-tool-server` processes, and pinning the environment in this branch removes that signal, which is why it is filed rather than merely worked around. A third, outside this branch entirely, is #853: ten `tool-server` test servers omit the host on `listen(0)`, binding every interface where twenty-two sibling sites pass `127.0.0.1`. It is hardening, not a live exposure — `createHttpApp`'s Host allow-list answers a LAN-addressed request with 403, and only a raw client forging `Host: 127.0.0.1` reaches the stub registry behind it, inside the ~100 ms one `it()` holds the socket. It is also not a collision risk: ephemeral ports are kernel-assigned and never reissued while in use, which is why the fixed-port audit above is unaffected. All ten predate this branch. Left untouched on purpose: `boot-electron-spawn-error.test.ts`, `chromium-discovery.test.ts` and `vega-cli-timeout.test.ts` — owned by the still-open #709 and #711, and all three still carrying their defects on this base. (#707 has since merged into this base, so `boot-device-hotboot.test.ts` and `vitest.config.ts` carry its fix rather than a defect; `test/setup/clear-argent-env.ts` is its work, and this branch relies on it.) **Not fixed, deliberately** — an adjacent class (test-*order* dependence, not machine-global resources), reproducible if wanted: `link-config.test.ts` (deterministic: `-t "returns null when required field"` → 4 failed), `network/network-integration.test.ts` and `android-perfetto/dispatch.test.ts` (fail under `--sequence.shuffle` seeds). **Also observed, not addressed:** one green tool-server run leaves **56** entries behind in a scratch `TMPDIR` — **30** in the `argent-screenshot-diff*` family, 4 `argent-events-*`, 3 `argent-flow-crop-*`, 3 `argent-tar-upload-*`, 2 `argent-flow-diff-*`, 2 `argent-file-input-*`, and 12 assorted singletons (`ast-index-*`, `component-source-*`, `argent-profiler-cwd`, `argent-chromium-media`, …). Measured per run against a scoped temp dir, so it ranks the contributors rather than counting one machine's accumulated history — screenshot-diff, not flow-crop, is the dominant one. A separate leak. That instrument is blind to a *literal* path, which is how it missed the two `/tmp/native-profiler-*-report.md` writers the fourth pass found. Re-run across all 13 packages, `argent-mcp` is the only one outside `tool-server` that leaves anything — two entries per run, filed as #854. ## Second pass A review of this branch re-ran every category above with its own instrument rather than reading the diff, and found the sweep had pinned four resources in one file while leaving a sibling that does the same thing live. Fixed in `4289e3d8f` and `8164955b4`; the table and bullets above are the corrected totals. - **`tool-server-event-log`** was mocked off in 2 of the 4 files that call the real `start()`. With the flag on, `startup-telemetry` and `tool-fail-invalid-params-emission` failed 3 tests in `attachRegistryEventLogger` and truncated the real `~/.argent/tool-server-events.jsonl` to 0 bytes first. `ARGENT_EVENT_LOG` cannot redirect that: `test/setup/clear-argent-env.ts` strips every `ARGENT_*` var before the module graph loads. - **`scopeTempHome`** reached 8 files; 4 more built a real `LogFileWriter` through the JS-runtime-debugger blueprint. - **`os.homedir()` on Windows** reads `USERPROFILE`, not `HOME`. `android-binary-windows` and `adb-resolve-avd-path` pinned only `HOME`, and both run on a real windows-latest host. Proven by running the resolver under a `node:os` mock that reproduces the Windows lookup: with `HOME` alone the stock Studio SDK under the profile resolves; with both pinned it misses. `update-checker` had the same gap. - **`getConsentState`** honours a falsy `ARGENT_TELEMETRY` at the same precedence as `DO_NOT_TRACK`, ahead of the config override the suite writes. Only `DO_NOT_TRACK` was pinned: `ARGENT_TELEMETRY=0` gave `17 failed | 6 passed`. - **`workspace-reader-integration`** let the version probe spawn the real package managers off `PATH`; their shims self-install, so the file pulled **~44 MB** into `~/.cache/node/corepack` and `~/Library/Caches/eas-cli`. `PATH` is now a directory holding only `node`, the one version the test asserts — 44 MB → 4 KB. - **`argent-mcp`** snapshotted `ARGENT_AUTO_SCREENSHOT_DELAY_MS` without clearing it, and that package has no `ARGENT_*`-stripping setup file, so 3 tests asserted the developer's exported value. Smaller items in the same commits: the `/tmp` socket fixtures now remove the directories they create per run; `http-upload`'s pre-scan snapshot is gone, since scoping `TMPDIR` to a fresh directory made it permanently empty and its filter inert, as its two siblings already did; and two `detectProjectPackageManager` assertions that enumerated the entire return union are pinned to a single value. The confirmation pass also caught a defect the first round of fixes introduced: pinning `HOME`/`USERPROFILE` to the fixture root in `android-binary-windows` let `<home>/Android/Sdk` shadow the `%LOCALAPPDATA%` root the two positive tests are named after, so deleting `roots.push(join(localAppData, …))` from the resolver left all three green. Home has its own directory in `6aee3ee7a`, and that mutation now fails both tests. A third pass, running the nearest-twin move repo-wide, found the `os.homedir()`-on-Windows gap in one more package: eight `argent-tools-client` test files redirect only `HOME`, and four of them recursively delete `STATE_DIR` — two before each test, two after — which on Windows is the developer's real `%USERPROFILE%\.argent`. Fixed in `139447d4a`, together with the same gap in `avd-snapshot`. Nothing runs that package on Windows CI today, so no current run fires it. ## Fourth pass A fourth pass audited the three fix commits and re-ran the sweep with two instruments the earlier ones did not have. Both holes were in the *measurement*, not the diff — which is why three passes missed what they found. Fixed in `a67c43f8d`. - **`os.tmpdir()` does not read the same variable on every platform.** POSIX takes `TMPDIR`, `TMP`, `TEMP` in that order; Windows takes `TEMP`, then `TMP`, and never looks at `TMPDIR`. The five pins this branch added set `TMPDIR` alone, so on Windows production still materialized into the machine-wide `%TEMP%` while the test scanned a scratch directory nothing ever wrote to — and `file-inputs`, `http-upload` and `flow-visual` each end in a leak assertion over that listing, which then passes whatever the code does. Against `origin/main`, which diffed a before/after listing of the real tmpdir, that is a regression rather than a wash. `redirectTmpdir` (`test/helpers/tmpdir-env.ts`) sets all three and returns the restore; the **7** files that scope a tmpdir use it, including the two `android-perfetto` ones that predate this branch. `test/tmpdir-env.test.ts` pins both lookup chains — reverting the helper to `TMPDIR` alone fails two of its three tests. All three leak assertions were re-confirmed live on POSIX by mutating the production cleanup each one covers. - **A hardcoded absolute path is invisible to a `TMPDIR`-scoped instrument.** `session-correctness` and `ios-instruments/analyze-freshness` passed a literal `/tmp/native-profiler-{x,ios}.trace` as the session's trace file, and the renderer writes its report beside it (`deriveReportPath` = `dirname(traceFile)/<base>-report.md`). Every run left `native-profiler-x-report.md` and `native-profiler-ios-report.md` in the shared `/tmp`; nothing removed them, and two concurrent runs write the same names. Reproduced with `TMPDIR` pointed at a scratch dir — the files still landed in `/tmp`, which is exactly why the `os.tmpdir()` row and the leftover census below both read clean here. Both tests now own the directory. - **A prefix-scanned env var is invisible to a grep for variable names.** `secretSources` enumerates `process.env` by the `ARGENT_SECRET_` prefix and ranks it *above* every file the test writes, so `secrets-command.test.ts` — which already pins `HOME`, `USERPROFILE` and cwd — still read the developer's shell. `ARGENT_SECRET_FOO=bar`, argent's own documented way to supply a secret: `6 passed` → **`3 failed | 3 passed`**, deterministic. `argent-cli` has no `ARGENT_*`-stripping setup file, so the prefix is cleared per test. Its sibling `configuration-core/tests/secrets.test.ts` was already immune — it threads an explicit `env: NO_ENV` into every call. - **`android-binary` and `vega-cli-fallback`** were the last two files redirecting `HOME` without `USERPROFILE` — the former the direct twin of `android-binary-windows`, in the same directory, driving the same resolver. Under a `node:os` mock reproducing the Windows lookup with an SDK planted in the profile, it loses **5 of 11** tests. No test file in the repo redirects `HOME` alone any more. (`6fbb66ce1` later skipped both of this file's describes on win32 — its fixtures are extensionless and the win32 resolver only takes `.exe` — so its own `USERPROFILE` pin is now symmetry with the sibling files that do run there, not a live guard.) Two smaller items in the same commit. `native-devtools-socket-bind`'s ENOENT fixture still rooted at `os.tmpdir()` while the comment three screens above explains why the others do not: with a 142-character ambient `$TMPDIR` that path exceeds `sun_path` and `listen` rejects it for *that* reason, which the assertions cannot tell from the missing-directory one they were written for. And `<home>\AppData\Local\Android\Sdk` — the resolver's fallback for a GUI-spawned server that never inherited `%LOCALAPPDATA%` — could be deleted with the whole suite green; it now has a test, and deleting either win32 root fails exactly the tests named for it. ## Fifth pass A fifth pass took the platform-branch shape the fourth pass exposed and ran it as its own move: *for every function that branches on `process.platform`, does the test's guard cover every arm, or only the one the author's machine takes?* Fixed in `e08b466b6`. - **`init-stale-config.test.ts` deletes the developer's real VS Code MCP config on Windows.** The file mocks `os.homedir()` and says it does so "so hidden-scope probes (`~/.claude.json`, VS Code user profile) never touch the real home directory". `vscodeUserDirs()` branches `darwin → homedir()/Library/Application Support`, `win32 → %APPDATA%`, `else → homedir()/.config` — the win32 arm never calls `homedir()`, so the mock is inert there, and the test's own `userMcpJsonPath()` mirrors the same branch. Both user-profile tests then write a one-entry fixture over `%APPDATA%\Code\User\mcp.json` with a whole-file `writeFileSync`, and assert that `remove()` deleted it. Reproduced by forcing `process.platform` to `win32` with a two-server `mcp.json` planted at a probe `%APPDATA%`: **`3 passed`, and the file and its directory were gone.** With `APPDATA` pinned, the whole file passes under the same instrument — 31 tests — and the planted config is byte-identical. - **`argent-tools-client` hands its ambient `ARGENT_*` to a real spawned child.** The launcher spawns an actual tool-server, and `buildToolsServerEnv` overrides only the keys it is given — everything else rides in on `...baseEnv`. `ARGENT_HOST` is the one that bites: the fixture binds the host it names while the launcher records and health-checks `127.0.0.1`, so reuse misses and a second server is spawned. With six overrides exported: **`10 failed | 181 passed`, and eight stray `fake-tool-server` processes still holding TCP listen sockets after the run.** With the prefix stripped: `191 passed`, none surviving. The package now carries the same setup file `tool-server` has, for the same reason — and it is the second of the two packages whose missing `ARGENT_*` strip this review found. ## Later passes A sixth pass audited the fix commits themselves and bounded the SDK probe (`6fbb66ce1`); a seventh ran the moves against the diff one more time and turned up the two below, plus the `listen(0)` defect filed as #853. - **The `PATH` pin made `workspace-reader-integration` POSIX-only without the guard its siblings got.** The shim it puts on `PATH` is an extensionless `node`, and `symlink()` to a file needs an elevated process or Developer Mode on Windows, so `beforeAll` throws `EPERM` there before any assertion runs; a copy in place of the symlink would not rescue it either, since a bare command name resolves through `PATHEXT`. This sweep already skips `android-binary` and `native-devtools-socket-bind` on win32 for exactly this class, and the Windows job runs a curated list of seven `tool-server` files that excludes this one, so nothing was catching it. Fixed in `095d75bd8` — verified POSIX `2 passed`, and under a forced win32 platform `2 skipped` with the top-level `beforeAll` never entered. - **A rationale that rested on a case the file excludes.** `android-binary`'s header justified pinning `USERPROFILE` by what `os.homedir()` reads on Windows, two sentences before noting that both describes skip on win32. Fixed in `f37b3605e`; the variable stays in the snapshot list, the justification no longer claims a platform this file never reaches. A seventh pass also re-ran the sweep against a hostile machine: every feature flag on in a scratch `~/.argent/flags.json`, no `adb` on `PATH`, and a `TMPDIR` long enough to overrun the 104-byte `sun_path` limit. It found one resource this branch had not closed. - **A red `launcher-version-gate` run strands a real tool-server.** `ensureToolsServer` spawns a fixture server, but its pid only reached the reaper after three or four assertions on the state it returned; any of those throwing left the pid unrecorded, and the same `afterEach` then deleted `STATE_DIR`, the only other record of what to kill. **Eight such processes were alive on the development machine**, spread from 14:33 to 18:23, every one from an `argent-version-gate-test-*` fixture and none from any other family — each still holding a TCP listen socket with its fixture directory long gone. Forcing one assertion to fail took the `tool-server.cjs start` count from 10 to 11. The nearest twin, `launcher-duplicate-spawn`, guards this twice: it records the pid straight after the null check and sets `FAKE_TTL_MS` so an escaped server self-exits. Fixed in `26c2c60ec` with both, the TTL also covering the window between the spawn and the state read where no pid exists yet — the same forced failure now leaves **zero** strays. An eighth pass audited the seventh's own fixes, and found the version-gate fix had been applied to one file when the defect was a property of four. - **Three more files strand a real tool-server on a red run.** `launcher-spawn` and `launcher-exit` set `FAKE_TTL_MS` nowhere; `launcher-duplicate-spawn` — the file `26c2c60ec` cites as the model — armed it inside one test's `try/finally`, so eight of its nine spawns ran without it and the `finally` disarmed it for the remainder of that test. Reproduced per file by forcing a failure before the pid is recorded: a detached node process survives, reparented to init, holding a listening socket, with its `TEST_HOME` removed and its state record already cleared. Fixed in `3bb69ded6` by moving the net to a file-wide `beforeEach` in all three. Verified end to end — the child carries `FAKE_TTL_MS=60000`, and a stranded server spawned at 21:28:36 exited by itself at 21:29:36 with nothing signalling it; three consecutive green runs stay at 191 passed. - **A 29th file writes the real `~/.argent`.** `boot-electron-spawn-error`'s success-path test completes a boot, and a completed boot calls `trackChromiumPort`, persisting to `os.homedir()/.argent/chromium-cdp-ports.json`. The real file on the development machine had accumulated **106 dead ephemeral ports**. Its two siblings in the same directory already mock `chromium-discovery` for exactly this reason; this was the third, with no mock. Fixed in `3306b6f40`. - **`scopeTempHome` was itself unpinned.** Deleting its `USERPROFILE` line — the Windows half, the same property the sibling helper's test pins — left all twelve consumers green at 48 passed. `3306b6f40` adds the missing test; it kills that mutation plus dropping the `HOME` pin and replacing the per-test `mkdtemp` with one shared directory. Two findings from that pass were refuted on verification and are recorded rather than acted on: the tool-server tmpdir leftovers (mechanism real, but they are the separate leak this body already scopes out) and a claim that `26c2c60ec`'s message overstated its twin. Four comments were also corrected against their code: `deriveReportPath` joins `<base>-report.md` *inside* the trace's directory rather than appending to the directory's name, which is what makes the `rm(traceDir)` added in `a67c43f8d` cover it; two `android-perfetto` comments still named `TMPDIR` as the mechanism after the switch to `redirectTmpdir`; and `android-binary`'s header claimed the home pin lets its two positive tests resolve "on either platform", though its fixtures are extensionless and the win32 resolver only accepts `.exe`. |
||
|
Ignacy Łątka
|
5ac6805040 |
fix(native-devtools): derive restart_required from the running process instead of asserting it (#560)
this is urgent
---
## Problem
Every `native-*` tool can get stuck telling the agent to do something
that will never work.
I hit this during an end-to-end pass over 0.17.0. After a mid-session
simulator reboot, `native-devtools-status` reported:
```json
{ "envSetup": true, "appRunning": true, "connected": false,
"requiresRestart": true, "nextLaunchWillBeInjected": true, "injectable": true }
```
and every `native-*` tool returned `restart_required` with "Call
restart-app then retry." Following that advice never helped, and the
message offered nothing else. `flow-execute` was blocked too, since its
`launch` step needs the connection, and flow recording silently degraded
a portable `tap: { selector }` to raw coordinates when it couldn't reach
native devtools.
The app was injected the whole time. All three dylibs were mapped into
the process and the simulator log showed a successful dial on every
launch, while the tool-server held the listening end of that exact
socket and still reported `connected: false`. `argent server stop &&
argent server start` cleared it instantly.
## The real defect is the state, not the wording
`requiresAppRestart` returned `true` for every app without a live
connection:
```ts
async requiresAppRestart(bundleId) {
if (connections.has(bundleId)) return false;
await reverifyEnv();
return true; // <- asserted, never measured
}
```
That is not a measurement. `restart_required` was a constant wearing a
state's clothes, and the advice built on it stays literally true
forever, so an agent obeying it restarts the app indefinitely.
Softening the message would have left that intact. This PR measures the
state instead.
## Fix — derive it from the running process
`restart-app` relaunches into the simulator's **current** launchd
environment, so it can only ever help a process launched under different
terms than a fresh launch would get. That is answerable: read the
process. `launchctl list` already gives its pid; `ps eww -p <pid> -o
etime=,command=` gives the environment it was exec'd with and its age.
Simulator apps are ordinary host processes owned by the same user, so
both are readable.
`appConnectionState` (replacing `requiresAppRestart`) returns:
| state | meaning | remedy |
| --- | --- | --- |
| `connected` | devtools are live | — |
| `not_running` | no process to read | `launch-app` |
| `stale_process` | no bootstrap dylib, a previous run's endpoint, or
older than this service's listener | `restart-app` |
| `unregistered` | injected against **this** endpoint, started after the
listener came up, still silent | restart the **tool-server** |
| `indeterminate` | process could not be read (ios-remote, unreadable
`ps`) | `restart-app`, with the loop warning |
Only `not_running` / `stale_process` / `indeterminate` report
`restart_required`. `unregistered` gets a new `service_stale` precheck
status that names the tool-server restart and explicitly says not to
relaunch the app — the launch a restart would perform has demonstrably
already happened.
The listener-age comparison is load-bearing: after a tool-server restart
the unix path is identical (it is derived from the udid) but the inode
is new, so an injected process older than the listener still needs
relaunching. Without that check this would send people off to restart a
tool-server they had just restarted — a new loop in place of the old
one. Both grace comparisons lean away from `unregistered` - the first
towards `stale_process`, the second towards `indeterminate`, and both of
those ask for a relaunch - so an uncertain read costs at most one wasted
`restart-app`.
Call sites follow the same rule rather than each inventing guidance:
- `describe` withholds `should_restart` for `unregistered` (setting it
would rebuild the restart-app → describe loop through a live service
instead of a system app) and carries the diagnosis as a `hint`, which
still marks the empty read untrustworthy for `await-ui-element`.
- `flow-run` raises the measured diagnosis instead of a blanket
"relaunch it" — both when a flow
resolves a selector against the full hierarchy, and at the `launch`
step's readiness gate, which
previously polled for the connection and then guessed at "a stale or
duplicate argent server".
- `native-devtools-status` reports `state`, and `requiresRestart` is now
false for `unregistered`.
- The six `native-*` tools type their block as the shared
`NativeDevtoolsPrecheckBlock` instead of restating it, so a new precheck
outcome cannot reach an agent through a tool that never mentions it.
## Scope
Why the service can stop registering a connection it is listening for is
still #561 and is not fixed here. What changes is that the state is now
measured, so that defect reports itself accurately instead of
masquerading as an app that needs restarting.
## Verification
**E2E, real iOS 18.6 simulator, Bluesky (RN 0.81.5).** Two tool-servers
were raced onto the same per-udid socket — the #561 shape, where the app
is genuinely injected and connected, just to the newer instance. Both
were then queried on the same app at the same instant:
| | `connected` | `requiresRestart` | `state` | `native-describe-screen`
|
| --- | --- | --- | --- | --- |
| main, stale instance | false | **true** | — | `restart_required` —
"Call restart-app then retry." |
| this branch, stale instance | false | **false** | `unregistered` |
`service_stale` — "Restarting the app cannot change that … restart the
tool-server" |
| healthy instance | true | false | `connected` | `ok`, 44 elements |
Restarting the app in main's row lands on the newer instance again,
leaving the queried one stale forever. That is the loop, reproduced on
demand.
The other states were walked on the same simulator: a pre-existing
process against a fresh listener → `stale_process` (`requiresRestart:
true`); `restart-app` → `connected`; terminate → `not_running` ("Call
launch-app (or restart-app)").
**Unit.** `native-devtools-app-state.test.ts` drives the real blueprint
factory with a stubbed process table and covers all five states plus the
`etime` and env-token parsers. Mutation-checked — dropping the
listener-age comparison, the endpoint match, or the connect grace each
fails exactly one test and nothing else.
**Suite.** `vitest run` in `packages/tool-server`: 296 files, 3113
passed, 1 skipped. `tsc --build` across the workspace, `tsc --noEmit -p
tsconfig.test.json`, `eslint` and `prettier --check` on every changed
file: clean. `origin/main` merged in first.
## Review round
A review pass over the derivation itself, with each finding reproduced
before it was acted on.
**`describe` computed the diagnosis and threw it away.**
`buildAppStateMessage` ran for every
unconnected state, but only `unregistered` carried it out; the rest
returned a bare
`should_restart: true`. That boolean reaches an agent as an undocumented
JSON field — `describe`'s
own description never mentions it — and the single place it is rendered
as prose is
`await-ui-element`'s timeout note, which spells it *"call restart-app
and retry"*. So the two states
whose message carries the escape hatch lost it exactly where it was
needed. `indeterminate` is the
sharp end: on `ios-remote`, whose app processes are out of reach of the
local process table, it is
the **only** reachable unconnected state, so that path had no exit at
all. The diagnosis now rides
out as a `hint` for every state; `should_restart` still marks only the
states a relaunch fixes.
**`native-devtools-status` measured the same device twice.**
`isAppRunning` ran its own
`launchctl list`, and `appConnectionState` ran another one after
re-verifying the launchd env — so
the two snapshots sat seconds apart and could contradict each other
(`appRunning: true` beside
`state: "not_running"`), with `requiresRestart` gated on the staler of
the pair. Both are derived
from one measurement now; only `indeterminate`, which genuinely leaves
running-ness unanswered,
pays for a second probe.
**Coverage.** Mutation testing found the 3 s grace term entirely
unpinned — the fixtures sat 200x
away from the boundary on either side, so both dropping the term and
weakening `>=` to `>` kept the
suite green while flipping a relaunchable process to `unregistered`, the
one verdict the file exists
to withhold. Also unpinned: the `reverifyEnv` call (swapping it for the
latching `ensureEnvReady`
the comment explicitly forbids passed the whole suite) and an
unparseable `ps` age staying "no
evidence" rather than becoming a measurement. All three are now covered,
and each new test was
confirmed to fail on the un-fixed code.
**Verified against the real thing.** The `launchctl` and `ps` contracts
were re-checked on an
iOS 18.6 simulator rather than reasoned about: row layout and pid
column, one-line `ps eww` output
with the argent tokens present in a live app's environment, `etime`
matched against `lstart` to the
second, and the four `etime` format strings read out of the `ps` binary.
Two corrections came out of
it — the environment can reach `kern.argmax`, which is exactly Node's
default `maxBuffer`, so the
probe now sets the same 16 MiB its siblings use; and `ps` suppresses the
environment of a
SIP-protected platform binary, which the docstring's "never as 'not
injected'" promise did not
account for (unreachable here, since `com.apple.*` is rejected as
non-injectable upstream, but the
promise now says so).
**Also corrected:** the `argent-device-interact` skill offered
`restart-app` as the only remedy for
a missing injection; the dylib-platform check in
`download-native-binaries.sh` and a
`requiresAppRestart` reference in `boot-device.ts` described states that
no longer exist; and three
tool descriptions still said *"Fails if native devtools are not
connected"* directly beside the new
sentence explaining that this case returns a status rather than failing.
**Known and deliberately left** (both pre-existing, neither introduced
here):
`describe` with no `bundleId` auto-targets from the connected list, so
an unregistered app throws
`NATIVE_TARGET_NO_CONNECTED_APPS` before any state is measured and the
message is swallowed by the
fallback's catch — surfacing it would newly mark those empty reads blind
for `await-ui-element`'s
`hidden` condition, which wants its own verification pass. And a dylib
that is inserted but silently
skipped by dyld (a platform mismatch) reads as injected-but-silent, so
`service_stale` → tool-server
restart → `stale_process` → `restart-app` is a two-step cycle;
distinguishing it needs a load
confirmation the process table cannot give.
## Review round 2
A second pass over the derivation, run as four independent sweeps
(claims-vs-code, nearest-twin
and reachability, non-happy paths and inputs, and absence). Three of the
four independently landed
on the same defect.
**The flow launch gate was the one caller that never asked whether the
app could be injected at
all.** Every other consumer of `appConnectionState` gates `com.apple.*`
first - the precheck
throws, `describe` returns its terminal hint, `native-devtools-status`
short-circuits,
`flow-ios-tree` only ever auto-resolves a connected app.
`waitForNativeDevtools` measured the raw
launch id. That matters because the launchd environment carrying the
bootstrap dylib is
simulator-wide, so a system app's process inherits the very tokens the
measurement reads.
Confirmed on an iOS 18.6 simulator: `com.apple.Preferences`, launched
under argent, reports
```
71940 0 UIKitApplication:com.apple.Preferences[9a63][rb-legacy]
etime=00:03
DYLD_INSERT_LIBRARIES=.../libArgentInjectionBootstrap.dylib
NATIVE_DEVTOOLS_IOS_CDP_SOCKET=/tmp/argent-nd-39B9C547.sock
```
Run through the shipped algorithm that is `processCarriesInjection:
true` against an older
listener, i.e. **`unregistered`** - so a flow with an `ios:` launch
entry of `com.apple.Preferences`
failed its launch step with *"restart the tool-server"*, for an app no
tool-server restart can ever
help. Library validation means it can never connect, so the gate now
says that terminally.
**`describe` dropped the entire diagnosis when the probe rejected.**
`appConnectionState` re-applies
the launchd env before it can answer anything, so it throws outright on
a sim that goes away
mid-call; that landed in the native fallback's outer `catch` and
returned an empty tree with no
hint. An agent saw "nothing on screen" rather than "could not be read",
and `await-ui-element` had
nothing to append to its timeout note. The two other consumers of the
same call already degraded to
`indeterminate` instead - this one now matches them.
**`native-devtools-status` re-ran the probe that had just failed.** One
route into `indeterminate`
is `inspectRunningApp` rejecting, i.e. `simctl spawn <udid> launchctl
list` failing. The
running-ness fallback then issues that same command unguarded, so a
shut-down simulator surfaced a
raw subprocess error rather than the structured `init_failed` guidance -
which the branch 40 lines
above it handles correctly, for exactly this reason.
**`native-devtools-status` was the only surface withholding the loop
escape.** Every other consumer
carries `buildAppStateMessage`, whose `indeterminate` text ends *"do not
keep restarting the app;
restart the tool-server"*. This tool returned booleans only, and
`requiresRestart: true` is the
whole of what an `indeterminate` app tells you. On ios-remote, where no
app process can ever be
inspected, that is the only state a running app reaches - so the tool
whose stated job is verifying
readiness answered with the restart loop and nothing else. It now
returns `message`.
**Also fixed:** both probes log their own failure (a broken probe
degrades every app to
`indeterminate`, indistinguishable at the tool surface from a genuinely
uninspectable one - the
sibling `ps` probe in `vega-process.ts` logs for the same reason); the
gate skips its
several-second measurement once the run is cancelled; and the gate no
longer repeats the bundle id
its caller's prefix already names.
**Prose that had drifted from the code:** `not_running` is reachable on
ios-remote too, so
`indeterminate` is only the sole unconnected state a *running* app
reaches there; the second grace
comparison leans to `indeterminate`, not `stale_process`; four of the
five states settle
running-ness, not four besides `not_running`; a null pid means a row
that did not parse, not a job
launchd reported as having no process (measured: a `UIKitApplication`
row is removed outright when
the app exits, and `-` appears only on daemon labels);
`waitForNativeDevtools`'s documented return
contract described the branch the change had rewritten; and a rationale
comment was attached to the
test above the one it described.
**Coverage.** The gate's measured reason had none at all - replacing the
whole diagnosis with a
literal left all 542 flow tests green. Eleven mutations were run against
the fixes and every one is
now killed: the injectability guard, the measured reason, the doubled
bundle id, `describe`'s reject
guard, `describe`'s `unregistered` hint merge, the `message` field, the
precheck fallback, the
`appRunning` guard on `requiresRestart`, a state renamed in the
description only, the connect grace
widened from above (the old fixtures all sat 10x clear of it, so it
could have grown to 25 s
unnoticed), and the `indeterminate` guidance line.
---
## Review round 3 — external review findings
Six findings from @hubgan's E2E review, each reproduced before it was
acted on. Fixed in
|