mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
fix/flow-bad-entry-undefined-render
583 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Ignacy Łątka
|
e8c9de68ac |
fix(flows): classify a flow step that omits a selector key
`badEntry` measured `JSON.stringify(raw).length`, and `JSON.stringify` returns the value `undefined` for an absent entry, so a step written without its `in:`/`into:` key crashed with a raw TypeError instead of the classified FailureError. The unclassified throw also carried no `error_kind`, so `argent flow run <dir>` stopped the whole batch on one invalid file. Fixes #962 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
filip131311
|
b835de2326 |
fix(packaging): ship one shared copy of the Android screen-sharing agent (#983)
## Summary Fixes the biggest chunk of #980: the published package shipped **four byte-identical copies** of the Android screen-sharing agent (`resources/android/`: jar + per-ABI `.so`, ~12.7 MiB each) — one next to each platform's simulator-server binary, because the binary resolves the agent relative to its working directory and the spawn set cwd to the per-platform dir. This ships **one shared copy** at `bin/resources/android/` and points the spawn cwd at the bin root instead. Package size (built from this branch, same artifact set as 0.22.1): | | before | after | |---|---|---| | unpacked | 115.5 MB | **76.4 MB** | | packed | 37.6 MB | 25.4 MB | 76.4 MB is under npmmirror's 83886080-byte unpacked-size cap (the limit #980 trips — note it applies to `dist.unpackedSize`, not the tarball). Already-published oversized versions are immutable, so the mirror also needs the `cnpm/unpkg-white-list` entry to sync the backlog; this PR keeps future versions under the cap. ## Changes - `scripts/download-simulator-server.sh` extracts the agent tarball once, to `bin/resources/android/`, instead of next to each binary. - `bundle-tools.cjs` copies that single shared directory into the package. - `@argent/native-devtools-ios` replaces `simulatorServerBinaryDir()` with `simulatorServerRunDir()`: the bin root when the shared `resources/android/` exists there, falling back to the per-platform dir so an `ARGENT_SIMULATOR_SERVER_DIR` override pointing at a pre-dedup layout keeps working. - The blueprint spawns simulator-server with `cwd: RUN_DIR` (binary path stays absolute; the agent lookup is the only cwd-relative path in the simulator-server crate — audited: every other fs path there is a tempdir, `dirs::cache_dir()`, or caller-supplied). No symlinks, no postinstall scripts — plain files plus a different cwd, so nothing changes for npm/pnpm/yarn/bun or Windows. ## Testing - Unit: resolver tests updated + new shared-layout case; blueprint test asserts the spawn cwd. - Packaging: ran the modified download script + `npm run pack` end-to-end; tarball contains exactly one `bin/resources/android/` (94 files vs 112). - Live: branch tool-server with `ARGENT_SIMULATOR_SERVER_DIR` pointed at the new layout — iOS simulator (screenshot) and Android emulator (boot-device, screenshot, gesture-tap) green; `lsof` confirms both spawned simulator-servers run with cwd = bin root, where `resources/android/<jar>` and each per-ABI `.so` resolve (verified present at those exact relative paths). - Physical Android phone (Galaxy S24, the one path that actually pushes the agent): live-tested — the `android_device` controller resolved the agent from the shared layout, pushed it, and connected ("All agent connections established successfully"); screenshot and tap through the on-phone agent both green, spawn cwd confirmed as the shared bin root. No docs update needed: internal packaging/layout only — no MCP tool, CLI, config, or flow-file surface changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01UGAsAvYBJ3Jy9DKLaX9aoZ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved simulator-server resource handling by using one shared screen-sharing resource location across host platforms. * Added a warning when required shared resources are unavailable. * Ensured simulator-server processes start from the correct runtime directory. * **Documentation** * Clarified simulator-server runtime and shared resource behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Ignacy Łątka
|
2cc1f5281c |
test(android-profiler): drop an assertion that cannot fail (#933)
Audit of every assertion in the repo that pins shipped agent-facing text, looking for more of the `/restart/i` shape that reddened main in #871 — an assertion satisfiable only by making the product string restate itself. **The class is essentially absent. One instance found.** ## The one `tool-server/test/android-perfetto/hang-stacks-offcpu.test.ts` ```ts expect(out).toMatch(/No usable on-CPU stack samples/); // L156 // The thread was on-CPU/executing — this is genuine CPU work, not a wait. expect(out).toMatch(/on-CPU/); // L158 ← removed expect(out).toMatch(/executing/); ``` `/on-CPU/` is a substring of the literal asserted two lines above, so `L156 ∧ L158 ≡ L156`. It cannot fail while L156 passes. Mutation-checked rather than argued: changing the pipeline's sentence from `_No usable on-CPU stack samples were captured…` to `_No usable stack samples were captured…` still fails the test — ``` × explains an empty sample set as un-unwound CPU work when the thread was executing AssertionError: expected ... to match /No usable on-CPU stack samples/ ``` — so nothing this line was catching goes uncaught. `/executing/` is **not** implied and stays; the comment above it still reads correctly. ## What the audit covered, and what it found instead | stage | count | |---|---| | assertions pinning shipped agent-facing text | 707 | | …in the `/restart/i` shape (short generic literal beside stronger siblings on the same value) | 44 | | …surviving hand-verification | **1** | The 43 rejected are doing real work. A sample, so the reasoning is auditable: - `"844x390"` / `"390x844"` — dimension-mismatch test; the point is argument order. - `"(2×)"` / `"(1×)"` — different dedup counts. - `/wait/` beside `/No on-CPU stack samples/` — the sibling literal contains no "wait"; it pins the wait-vs-CPU explanation. - `"localhost:8081"` / `"127.0.0.1:8081"` — two hosts the interceptor must filter. - metro `discovery.test.ts` `"Do not retry in a loop"` — a *different* test from the one asserting the full sentence. More importantly, the 707 is not a pile of redundancy. It is mostly the only way to test a function whose output is a string — `formatTraceFreshness(NOW - 5*60*MIN, NOW)` can only be checked by asserting `/5 hours ago/` — plus assertions that are load-bearing in their own right, e.g. `expect(description).not.toContain("hunter2")` in `configuration-core/tests/secrets.test.ts`, the sole guarantee a secret value never reaches a source description. Stripping the category wholesale would empty 74 tests into unconditional passes. ## Two adjacent findings, not changed here - `tool-server/test/native-profiler-missing-trace.test.ts:90` — `expect(result.report).toMatch(/missing|not found|unreadable/i)` is meant (per its comment) to check the wording tells the user the file is missing rather than the export empty. It passes vacuously: the fixture is named `missing_cpu.xml`, so `/missing/i` matches the filename the assertion two lines up already pins. It enforces nothing. The fix is a discriminating regex or a renamed fixture, not deletion. - `tool-server/test/capability.test.ts:58-59` — `toContain("demo-tool")` and `toContain("android")` are both substrings of `"tools/demo-tool/platforms/android.ts"` asserted at L60, so they are provably implied. Left in place deliberately: the test is named *"composes a uniform message with toolId, platform, and the file"*, and deleting them would make the name over-claim relative to what the body checks. ## Verification Branched off `366f17c61`. | gate | result | |---|---| | `vitest run` (tool-server) | **358 files, 4401 passed, 1 skipped**, exit 0 | | `tsc --build` | clean | | `typecheck:tests --workspaces` | clean | | `eslint . --max-warnings 0` | exit 0 (with `npm ci` in `packages/docs`, as CI does) | | `prettier --check` | clean | |
||
|
Juliusz Wajgelt
|
bf38e9069b |
fix(sim-remote): pass the udid before -b in simctl bootstatus (#982)
## What `simctlBootstatus` in `packages/tool-server/src/utils/sim-remote.ts` built the command as `simctl bootstatus -b <udid>`, placing the flag before the device. simctl's grammar puts the device first — `bootstatus <udid> -b`. ## Why Newer Argent Cloud deployments are more faithful to how the local `simctl` handles its arguments, so they now reject `-b` in that position as an unknown flag. The result is that **every remote boot fails**, on every device: ``` [tool: mcp__argent__boot-device] -> invalid option -- b ``` The local `xcrun` path already used the correct order (`boot-device.ts:461`), so this only affected sim-remote / Argent Cloud. ## Notes for the reviewer - I checked the other `sim-remote` wrappers for the same flag-before-udid shape. `injectDylib` is the only other one with an optional flag and it already appends after the udid, so `bootstatus` was the sole offender. - `packages/tool-server/test/boot-device.test.ts` passes (23/23), but it only exercises the local `xcrun` path — there is no existing test for the remote wrapper, so **this fix is not covered by tests**. It's worth a manual check against a Cloud device before merging. - `tsc --noEmit` reports pre-existing errors in this package from a stale `@argent/registry` build; `sim-remote.ts` itself is clean and none of them are related to this change. - No docs update needed: this is an internal argument-construction bug with no user-facing surface change. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed simulator boot status checks by correcting the order of command-line arguments. * Ensured the simulator identifier is processed correctly when the optional boot flag is used. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
filip131311
|
23b9a07760 |
feat(mcp): auto-describe — element tree after interaction tools + relaxed tapping rule (#958)
## Why
The rule's `<tapping_rule>` ("Before **every** tap, you MUST call a
discovery tool") makes agents loop `tap → describe → tap → describe`:
over 35 baseline runs, `describe` was called 90 times against 86 taps —
one round-trip per tap, each a full model turn. The screen after an
action is already captured by the auto-screenshot; the element tree is
the only thing the agent still had to fetch.
## What
- **MCP** (`packages/argent-mcp`): after the action settles, call
`describe` and append `--- Elements after action (describe) ---` + the
tree to the tool result. Follows the same tool allow-list as the
auto-screenshot, skips `describe` itself, and sits inside the non-secret
branch so a `{{secret:…}}` value never re-enters context as text.
Spy-logged as `auto_describe` (duration, chars). MCP `instructions`
reworded.
- **Flag** `disable-auto-describe` (opt-out, mirrors
`disable-auto-screenshot`). Auto-describe has its own tool list
(`AUTO_DESCRIBE_TOOLS`) and its own branch, so the two features are
independent; the secret-placeholder skip and the screen-idle wait cover
both.
- **Rule + skill**: `tapping_rule` now says take coordinates from the
returned tree, call a discovery tool only when there is no fresh tree
for the screen (first action on a screen, after a wait, target not
listed); the two-failed-taps rule and the "never from pixels"
prohibition stay. `argent-device-interact` best-practice #6 and the
general-rules line updated to match.
- **Docs**: `reference/configuration.mdx` flag row,
`features/interacting-with-apps.mdx` discovery section.
- **Tests**: `autoDescribeEnabled` (global / project scope) and
`shouldAutoDescribe`.
## Evidence
A/B with `claude -p --model sonnet`, one build (variant env-gated during
the experiment), two sandbox projects differing only in rule/skill text
and the toggle; 7 tasks × 2 arms × 5 reps = 70 runs on the iPhone 17 Pro
simulator (iOS Settings ×3, Expo `my-app` ×2, Contacts create verified
in sqlite) and the Electron testbed.
| | main | this PR | Δ |
|---|---|---|---|
| Task success | 35/35 | 35/35 | = |
| Turns / run | 10.4 | 8.2 | −21% |
| Wall / run | 36.5 s | 28.0 s | −23% (new won 28/34 pairs) |
| Cost / run | $0.28 | $0.23 | −17% (27/34) |
| `describe` calls / run | 2.2 | 0.24 | −89% |
| Cache-read tokens / run | 622k | 421k | −32% |
(34 clean pairs; one pair excluded for external interference on the
simulator — with it included: wall −5%, cost −10%, turns −16%.) Every
iOS task moved 15–30% in favour of the new arm; Electron was neutral
(both arms describe once after `boot-device`, then a single
`run-sequence`). The appended tree costs ≈2.4k chars (~700 tokens) and a
median 73 ms per action; per-turn latency was unchanged, the saving is
the removed model round-trip.
Full report:
https://claude.ai/code/artifact/279528cf-ea92-4f52-9276-4f0475ba3e3c
## Caveats
- Sonnet only, short tasks, small screens. On feed-heavy apps with
10–20k-char trees the per-action overhead grows; a follow-up could cap
or filter the appended tree.
- Flags are read once at MCP start (same as `disable-auto-screenshot`),
so toggling needs an MCP restart.
## Checks
`vitest` argent-mcp (85) + configuration-core (122) green; `eslint
--max-warnings 0` on changed packages; `prettier --check .`; `npx
docusaurus build` green.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **New Features**
- Interaction tools now return an accessibility element tree alongside
the latest screenshot.
- Agents can use fresh element data for follow-up actions without an
additional discovery step.
- **Configuration**
- Added the `disable-auto-describe` feature flag to turn off automatic
element trees.
- **Documentation**
- Updated interaction guidance and configuration references to explain
the new behavior and flag.
- **Bug Fixes**
- Secret-placeholder handling now correctly accounts for both
screenshots and element trees.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
filip131311
|
e7c6f3324c |
fix(tool-server): frame device sockets on \n only — U+2028 in a label stalled describe for 15 s (#975)
## Problem
`describe` returned an empty tree after ~15 s whenever the screen had an
accessibility label containing U+2028 (LINE SEPARATOR) — e.g. any
Element/Matrix reply preview, or rich text pasted from the web. Nothing
was wrong on the device: ax-service answered in ~170 ms with a full,
valid tree.
The tool-server framed the ax-service socket with
`readline.createInterface`, which splits on the four ECMAScript line
terminators (`\n`, `\r`, **U+2028, U+2029**). JSON permits U+2028
unescaped inside a string and `NSJSONSerialization` emits it raw, so one
valid frame became three fragments, none parsed, each was dropped by
`catch { return }`, and the pending RPC waited out its 10 s timeout —
then the native-devtools fallback waited out its 5 s. On top of that,
the timeout was reported as *"this simulator was not booted through
argent … call boot-device with force=true"*, sending the agent to reboot
a healthy simulator.
Minimal repro (no device): `JSON.stringify({label:"a
b"})+"\n"` through
`readline` → 2 lines, 0 parsed.
## Fix
- **`utils/ndjson-socket.ts`** (new): `attachNdjsonReader` frames on the
`\n` byte only (`0x0A` can never occur inside a multi-byte UTF-8
sequence, so this is exact) and reports every unparseable frame to
stderr instead of swallowing it.
- Wired into the three device-facing JSON sockets: `ax-service.ts`,
`native-devtools.ts`, `android-devtools-client.ts` (the Android helper's
`org.json` and the iOS dylib's `NSJSONSerialization` both emit U+2028
raw, so all three were exposed). The two remaining `readline` uses read
process **stdout** for readiness markers — no JSON, no user text — and
are left alone.
- **`describe/platforms/ios/index.ts`**: a throw from `axApi.describe()`
(timeout, RPC error) now carries the error message in the hint and uses
the resolved api's real `degraded` flag, instead of being reported as
the boot caveat. Separate commit so it can be reverted independently.
## Verification
- 6 new unit tests for the reader (U+2028/U+2029 in a string, chunk
split mid-multibyte char, several frames per chunk, CRLF/empty lines,
dropped-frame reporting, trailing frame on `end`) and 3 for the hint
(`describe-tool.test.ts`). Full tool-server suite: 4816 passed.
- Live A/B on iPhone 16 / iOS 18.5, Safari page with `<meta
charset=utf-8>` and a U+2028 inside a `<p>`:
- argent 0.22.1 (`main`): `describe` **12.6 s**, `source:
native-devtools`, web text missing, false "not booted through argent"
hint.
- This branch (`dist` against installed binaries): **50 ms**, `source:
ax-service`, label `"In reply to Dave
Loving it so far"` present, no
hint, 0 dropped frames logged.
## Docs
No docs update needed — no tool, CLI, config or flow surface changes.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01VwqqHRhfuHXbKr6LYskxhJ
|
||
|
Ignacy Łątka
|
43b7d79a2a |
fix(boot-device): stop passing -crash-report-mode to emulators that reject it (#557)
## Problem
`boot-device` cannot launch an Android emulator on builds that do not
accept
`-crash-report-mode`. The emulator rejects the unknown option and exits
before
the device ever comes up:
```
$ argent run boot-device --args '{"avdName":"Pixel_6_API_34"}'
emulator binary exited with code 1 before the device booted
```
## Cause
`-crash-report-mode` is undocumented and missing from many emulator
builds, so
#306 added a feature-detect gate:
```ts
const crashReportArgs = (await emulatorSupportsFlag("-crash-report-mode"))
? ["-crash-report-mode", "never"]
: [];
```
That commit was a pure addition. It never removed the flag from the
shared
`LAUNCH_HARDENING_ARGS` constant, which still reads:
```ts
const LAUNCH_HARDENING_ARGS = [
"-no-boot-anim", "-netfast",
"-crash-report-mode", "never",
"-no-metrics",
] as const;
```
So the gate is dead code. Every launch gets the flag unconditionally
through
the constant, and builds that do support it are handed it twice.
## Proof
On Android emulator 36.1.9.0, whose `-help` does not list the flag:
```
$ emulator -version
Android emulator version 36.1.9.0 (build_id 13823996)
$ emulator -help | grep -c -- -crash-report-mode
0
$ emulator -help | grep -c -- -no-metrics
1
```
`-list-avds` and an unknown `-avd` both short-circuit before option
validation, so the rejection only surfaces once a real AVD resolves.
Against
one, with everything else held equal:
```
$ emulator -avd Pixel_6_API_34 -check-snapshot-loadable default_boot \
-crash-report-mode never
INFO | Found systemPath .../android-34/google_apis/arm64-v8a/
unknown option: -crash-report-mode
please use -help for a list of valid options # exits, never boots
$ emulator -avd Pixel_6_API_34 -check-snapshot-loadable default_boot
INFO | Ok: System requirements to run avd: `Pixel_6_API_34` are met
```
The launch dies at option parsing, after the AVD resolves and before any
device work. `hardeningArgs` reaches the snapshot probe, the hot boot
and the
cold boot alike, so on such a build all three spawns abort.
## Fix
- Drop `-crash-report-mode never` from `LAUNCH_HARDENING_ARGS`, leaving
the
`crashReportArgs` gate as the only source of the flag.
- Spread `crashReportArgs` into the `-check-snapshot-loadable` probe's
`extraArgs`. The probe is itself an emulator spawn, so it gets the same
crash-dialog suppression as the two boots; once the flag left the
constant
it would otherwise have been the only site missing it. This one is not a
snapshot-parity requirement - unlike `-noaudio` and `-netfast`,
`-crash-report-mode` selects crash reporting rather than qemu devices,
so a
mismatch would not invalidate the saved snapshot.
- Leave the gate where it is, after the already-running reuse fast path,
so
the `emulator -help` probe is still skipped when nothing will be
spawned.
- Fix the prose that had drifted: the comment above
`LAUNCH_HARDENING_ARGS`
still listed `-crash-report-mode` among its flags and claimed they all
need
no detection, and both it and the `emulatorSupportsFlag` docstring said
the
flag is "only present in newer emulator releases (~36.x and late 35.x)"
-
which 36.1.9.0 not having it contradicts.
`bootAndroidImpl` is the only emulator launch path in the repo, and all
three
of its spread sites are covered.
Making the gate load-bearing put weight on a probe that was only ever
advisory, so `emulator -help` detection is hardened alongside it:
- `emulatorSupportsFlag` memoized its verdict for the process lifetime
even
when the probe learned nothing - a transient spawn failure, a timeout,
or a
silent zero-exit run pinned "unsupported" until restart. An empty
capture is
now reported to stderr and left uncached, so the next boot retries.
- A killed probe is no verdict at all: the flag may sit in the part of
the
listing the child never transmitted, so partial output from a timeout is
discarded rather than read as absence.
- The three emulator probe spawns (`-help`, `-list-avds`,
`-check-snapshot-loadable`) pass `killSignal: "SIGKILL"`, matching
`runAdb`.
`execFile`'s default SIGTERM can be ignored by an emulator wedged in
Qt/graphics init, which leaves the promise unsettled past every caller's
deadline.
## Tests
A new `-crash-report-mode feature gate` block in
`packages/tool-server/test/boot-device-hotboot.test.ts` asserts the real
spawned argv and the probe's `extraArgs`:
- gate false: zero occurrences at the probe, hot-boot and cold-boot
sites;
- gate true: exactly one occurrence at each, always followed by `never`.
The
count matters - asserting mere presence passes on the double-pass code
and
would guard nothing;
- plus a test that `-help` detection is skipped entirely on the
already-running
reuse fast path, pinning why the computation sits where it does.
`emulatorSupportsFlag` is stubbed in that file's existing
`vi.mock("../src/utils/adb")` factory, because the real one memoizes its
`-help` result per binary for the process lifetime and both branches
could not
otherwise be exercised in one file. The two pre-existing
launch-hardening
assertions were updated to the four remaining flags.
`packages/tool-server/test/emulator-supports-flag.test.ts` is new and
covers
the detection hardening: which outcomes are memoized (a verdict read
from a
non-zero exit's captured listing, and a normal zero-exit run) and which
are
not (spawn failure, timeout with empty streams, zero exit with no
listing,
timeout with partial output), the stderr warning, and `SIGKILL` on all
three
spawns.
Discrimination check - both source changes reverted (flag back in
`LAUNCH_HARDENING_ARGS`, `crashReportArgs` dropped from the probe's
`extraArgs`), tests kept:
```
Tests 5 failed | 26 passed (31)
x omits the flag from the snapshot probe and the hot-boot spawn when the emulator rejects it
x omits the flag from the cold-boot spawn when the emulator rejects it
x passes the flag exactly once to the snapshot probe and the hot-boot spawn when supported
x passes the flag exactly once to the cold-boot spawn when supported
x keeps exactly one flag occurrence on the probe-reject cold fallback when supported
AssertionError: expected 1 to be +0 (x2 - the flag reaches a site that must not see it)
AssertionError: expected 2 to be 1 (x3 - the flag arrives twice)
```
Fix restored: `Tests 31 passed (31)`.
## Verification
- `npm run build` (`tsc --build`): clean.
- `npx vitest run` in `packages/tool-server`: clean.
- `npm run typecheck:tests --workspaces --if-present`: clean. This is
the CI
gate that plain `tsc` + vitest does not cover.
- `npx prettier --check .` and `npm run lint` (`eslint . --max-warnings
0`):
clean.
- End to end on a real emulator (36.1.9.0, the build that rejects the
flag).
The gate reads the binary correctly:
```
emulatorSupportsFlag(-crash-report-mode) = false
emulatorSupportsFlag(-no-metrics) = true
emulatorSupportsFlag(-no-boot-anim) = true
```
and `boot-device` against `Pixel_6_API_34` returns
`{"platform":"android","serial":"emulator-5554","booted":true}` in 4.8
s.
The live process argv shows the flag dropped and the four remaining
hardening flags intact:
```
qemu-system-aarch64 -avd Pixel_6_API_34 -force-snapshot-load
-no-snapshot-save \
-gpu auto -noaudio -no-boot-anim -netfast -no-metrics
```
`-force-snapshot-load` means the hot-boot path ran, so the snapshot
probe
accepted its flag-free argv too.
## Docs
No docs update needed: `-crash-report-mode` is an internal launch
argument
that no reference page, config key or tool parameter exposes.
|
||
|
Jakub Piasecki
|
d4a712c401 |
refactor(gesture): rename the momentum-free flag to momentum (#732)
`settle: true` collided with the runner's other `settle` — waiting for the tree to quiesce — which is the very thing a swipe step does after every dispatch, whichever way the flag is set. One sentence of the flow skill used both senses at once. This names the physics instead: `momentum: false` is the momentum-free gesture, and the natural fling is the default the prose already described. ### Renamed | surface | before | after | | --- | --- | --- | | `swipe` flow directive | `settle: true` | `momentum: false` | | `gesture-swipe` param | `settle` | `momentum` | | `gesture-drag` param | `settle` | `momentum` | The directive passes the flag straight through to the tools, so all three move together. `momentum: true` is the default and normalizes to absent, mirroring what `settle: false` did — parse/serialize stay exact inverses. ### Also - Recording summary label `(settle)` → `(momentum-free)` - Internal constants → `MOMENTUM_FREE_EASE_EXPONENT`, `MOMENTUM_FREE_MIN_STEPS` - `flow-run` tool description and `argent-create-flow` skill updated ### Testing `tsc -b`, eslint (`--max-warnings 0`) and prettier clean; `test/flows` + `test/tools` pass 671/671, including a new case covering the legacy-spelling rejection. |
||
|
Jakub Piasecki
|
e314b3ef4a |
feat(flow): swipe directive (#520)
Add one semantic finger flick for dismissing cards, paging carousels, opening drawers, and other gesture-driven actions distinct from goal-seeking scroll-to. Support bare directions plus from with exactly one of direction, to, or by, optional settle, and positive finite duration. Resolve selector or normalized-point anchors through the shared target path. Direction uses the Maestro geometry presets and now rejects anchored gestures that would collapse or travel opposite to the requested direction. Relative by deltas saturate supplied axes to [0, 1] and reject any effective zero or sign reversal. Resolved starts must remain on-screen, while explicit endpoints stay author-controlled. Keep parse and serialization invariants aligned for programmatic steps, improve unknown-key hints and selector diagnostics, surface swipe targets in run reports and recording summaries, share positive-millisecond validation, and reuse the common flow test harness. Dispatch gesture-swipe on touch platforms and gesture-drag on Chromium, reject touch directives on Vega, and document when to prefer scroll-to or a raw system-edge gesture. |
||
|
Hubert Gancarczyk
|
2740a3ec38 |
chore(knip): clear the parked dead-code backlog and drop the ceiling to 0 (#770)
> Built on top of #769, so this PR carries that commit too — the workflows only run for PRs targeting `main`, which is why it is based there rather than on the branch. #769 on its own is the minimal, low-risk fix for the currently-red job; if it merges first this shrinks to its own commit, and if this one merges first it closes #769 as well. ## What this does #572 landed the gate with 215 findings parked under `--max-issues`. That bounded the backlog but left it in place, and the first thing to touch it (#663) pushed the total to 218 and turned the job red. This clears the backlog and sets the ceiling to **0**, so the next unused export fails the job instead of consuming headroom nobody knew was there. | | before | after | | --- | ---: | ---: | | Unused exports | 47 | 0 | | Unused exported types | 156 | 0 | | Unused exported class members | 12 | 0 | | `--max-issues` | 215 | **0** | ## Scope check first Every workspace holding a finding is `private: true`. `@swmansion/argent` — the one published package — had **zero**, so nothing here moves a public API. ## Two shapes of fix **Still referenced inside its own file → drop `export`** (146 types, 36 exports). The declaration stays put; it just leaves the module's public surface. `tsc` keeps emitting these into the `.d.ts` as local declarations that the exported signatures reference. For the 146 **types** that is the whole story: they are erased, so the emitted `.js` is byte-identical. For the 36 **values** it is not. Dropping `export` from a `const`/`function`/`class` removes its `exports.X` binding from the emitted `dist/*.js` — 36 declarations across 26 files here. `packages/tool-server/dist/utils/device-info.js` no longer carries `exports.REMOTE_PREFIX` or `exports.VEGA_SERIAL_PREFIX`; `telemetry/dist/posthog.js` loses `exports.POSTHOG_PROJECT_TOKEN`; in the ESM-emitting `argent-installer`, `isTempRunnerPath` / `resolveLocalArgentDir` / `PACKAGE_ROOT` leave the module namespace object entirely. Nothing in the repo or in the `packages/argent-private` submodule at the pinned revision reads any of them by path, so nothing breaks. Flagging it because a `require()` of a built `dist/*.js` loses a dropped `export` exactly the way it lost the deleted `__resetAndroidDevtoolsInstallCache` this branch had to restore — so this half of the diff does deserve runtime scrutiny, not a skim. **Referenced nowhere → delete.** Orphaned test seams (`__test`, `_testValidators`), stale compatibility re-exports, and a few real orphans. The ones worth a reviewer's eye: - **`localSimctl`** — the `SimctlBackend` strategy is only ever constructed with `remoteSimctl`. What keeps the local iOS impl off it is the handler signature: `buildIosLaunchHandler` / `buildIosRestartHandler` read `services.nativeDevtools`, and `ios-remote` is the only platform that declares that service, so the local impl — which resolves native-devtools per device inside its own handler — has nothing to hand them and shells out to `xcrun` itself. - **`sendCharInsert`** — removing the `__sendCharInsert` test alias exposed the underlying helper as unreferenced. It is superseded, not unwired: the Chromium keyboard impl dispatches its own `type: "char"` event inline (`keyboard/platforms/chromium.ts:58`), which is exactly what this helper did. Chromium typing is unaffected. - **Error metadata fields** — `ServiceNotFoundError.serviceId`, `ToolNotFoundError.toolId` and their two siblings were never read. The `e.serviceId` in `registry-error-events.test.ts` is a field on the event object the handler pushes, not on the error class; the `err.toolId` in `http.ts` is `NotImplementedOnPlatformError`, a different class that keeps its fields. Flagging these in case they are wanted as deliberate diagnostics. Deleting `SourceMapsRegistry.toGeneratedPosition` / `findMatchingSource` cascaded into their only helper (`buildSourceCandidates`) and then into `projectRoot`, which had no remaining reader — so the constructor parameter goes too. `StubSourceMapsRegistry` existed only to pass `super("")`, so it goes with it. ## The three cross-workspace members `ArtifactStore.register`, `ArtifactStore.list` and `TypedEventEmitter.off` are all called from other workspaces — the artifact route and screenshot tools in `tool-server`, and `registry-listener` in `telemetry`. They are reported unused anyway, and the reason is the **unbuilt tree** the gate runs against: every workspace resolves `main`/`types` to a `dist/` that does not exist, so `@argent/registry` resolves to nothing and the cross-workspace edge never forms. Build the tree and all three findings disappear on their own; a control member that really is dead is still reported in the built run, so the pass is live either way. Nothing about this is specific to `classMembers` — every cross-workspace reference is invisible the same way. Each is exempted at the declaration with a `@public` JSDoc tag rather than by name in `knip.jsonc`, for one reason: **scope**. `ignoreMembers` matches a name across the whole workspace, so it would also hide a future dead `register`/`list`/`off` on any other class in `packages/registry`; `@public` binds to the member it is written on. What `@public` does **not** buy is a staleness check, and neither does `ignoreMembers`. Measured on a clean unbuilt worktree: a bogus `ignoreMembers` name and a redundant `@public` each leave the run at exit 0 with no hint, while a bogus `ignoreDependencies` name exits 1 with `Remove from ignoreDependencies`. `treatConfigHintsAsErrors` is live and simply has no producer for either exemption style — so when one of the three stops being earned, nothing says so, and the JSDoc at each member is the only record a re-audit has. Each one names its callers, and `off` records how to re-derive them (rename the member and read the `Property 'off' does not exist` errors; grepping `.off(` over-reports, since 14 of the 38 call-site hits are Node `EventEmitter`s). One correction to the commit message on that change: it says 35 hits where the exact count is 38 — 14 Node `EventEmitter`, 24 `TypedEventEmitter` (21 production, 3 in a test). The 14 and the 21 are right. `knip.jsonc`, `CONTRIBUTING.md` step 5 and the workflow's failure-explanation step all described a parked backlog and a ceiling to stay under. All three are rewritten for a gate that must simply come back empty, and all three now name the `@public` escape hatch — the remedy for a symbol whose only caller is out of knip's reach. ## Verification Run against a clean worktree with **no build output**, the way CI counts: - `npm run knip` — **exit 0, prints nothing**. Both passes empty, no config hints. - `npx tsc --build` — clean. - `npm run lint` (`eslint . --max-warnings 0`) — clean. - `npx prettier --check .` — clean. - `npm test --workspaces` — tool-server 329 files / 3857 passed; registry, telemetry, update-core, tools-client, mcp, cli green. - `npm run typecheck:tests --workspaces` — 13 workspaces define the script and all 13 are clean, but **the command itself exits 1**: npm errors `Missing script: "typecheck:tests"` for the three that do not define it rather than skipping them. Worth knowing before re-running this bullet and reading the exit code as a failure. End-to-end on a tool-server built from this branch, driven over its HTTP API: - **Chromium** (Electron smoke app) — `describe`, `screenshot`, `gesture-tap` (counter advanced 0 → 1), `POST /api/clipboard/text` read back out of the renderer, the WS `clipboardSync` command, and `debugger-connect` / `debugger-status` / `debugger-evaluate` (`sourceMapReady: true` with `StubSourceMapsRegistry` gone). - **iOS** — `boot-device`, `launch-app`, `describe` via ax-service, `screenshot`. - **tvOS** — `boot-device`, `launch-app` on an Apple TV udid, focus-driven `describe`. The registry snapshot then carries `NativeDevtools:<tvOS udid>`, confirming injection **is** resolved on tvOS through the local impl. - **Android** — `boot-device` on a dedicated AVD, `describe` via android-devtools, `gesture-tap`. - **Metro source maps** — the registry driven against a real 12.5 MB Metro `.map` over loopback, the way `Debugger.scriptParsed` drives it: allowlist passes and `waitForPending()` resolves on both, while retained heap after gc drops from 17.3 MB to 3.6 MB. ## Review follow-ups Five commits on top, one per finding. The base is back on `main`, so the workflows run again. - **`docs(launch-app)`** — the services comment explained the empty shape through `LaunchAppAndroidServices`, deleted here, and skipped `LaunchAppVegaServices`, which is still there. Same edit in `restart-app/types.ts`. - **`docs(registry)`** — the `@public` tag on `ArtifactStore.register` named 2 of 7 callers. Derived the full set the way the sibling tag on `off` prescribes: renamed the member, read the 16 `TS2339` errors `tsc --build` reports across flow-visual (5), screenshot (4), native-profiler-stop (2), screenshot-diff (2), native-profiler-analyze, react-profiler-analyze and screen-recording-stop. The re-derivation recipe is recorded too — `.register(` greps as badly as `.off(`. - **`docs(knip)`** — "the build, the tests and this gate all stay green on a wrong delete" held for argent-private and not for the cross-workspace case that justifies three of the four `@public` tags. Deleting `ArtifactStore.register` gives 16 `TS2339` errors and `9 failed | 2 passed` in `test/artifacts.test.ts`; only the gate stays green. Split in the workflow comment, the failure annotation and `CONTRIBUTING.md` step 5. - **`perf(debugger)`** — `doRegister`'s `data:` branch still base64-decoded and `JSON.parse`d a payload it dropped, inside a `catch` that swallows. Both arms returned the same `void`, so the branch collapses to an early return; `scriptUrl` / `scriptId` go with it, since their last reader was the deleted `this.maps.push(...)` and `doRegister` is private. - **`test(debugger)`** — the allowlist check moved to a top-level early return with nothing pinning it: delete the line and both SSRF files still pass 17/17. Two tests now assert `doRegister` consults it — four rejected URLs reach no `fetch`, and the loopback `*.map` Metro emits still does. Re-verified after: `npm run knip` exit 0 and empty on a `tsc --build --clean` tree, `tsc --build`, `eslint . --max-warnings 0`, `prettier --check .` all clean, and `npm test --workspaces` with tool-server at 329 files / 3854 passed. The one red test, `argent-installer`'s `globalPath returns ~/.config/opencode/opencode.json`, fails identically with these commits stashed — it reads the real `~/.config/opencode`, which holds a `.jsonc` on this machine. End-to-end on a tool-server run from this branch over its HTTP API, on every platform: - **Chromium** — `launch-app`, `describe`, `screenshot` (returned an artifact handle, `GET /artifacts/<id>` served the 137542-byte PNG back), `gesture-tap` (counter 0 → 1), `debugger-connect` / `debugger-status` (`sourceMapReady: true`) / `debugger-evaluate` (read `taps: 1` out of the page). - **iOS** — `launch-app`, `describe` via native-devtools, `restart-app`, `screenshot`. - **tvOS** — `launch-app`, focus-driven `describe` (12 focusables in TVSettings), `tv-remote`, `restart-app`. - **Android** — `launch-app`, `describe` via android-devtools, `restart-app`, `screenshot`. - **Vega** — `boot-device` on the `tv` VVD, `describe` via the automation toolkit. - **Metro** — the real `debugger-connect` / `debugger-status` / `debugger-evaluate` tools against a stand-in Metro (HTTP `/json/list` + CDP WebSocket) emitting three `Debugger.scriptParsed` events: a loopback `*.map` on a counting server, a 1.4 MB inline `data:` map, and `http://169.254.169.254/latest.map`. `sourceMapReady: true`, `loadedScripts: 3`, evaluate 42, the loopback map fetched exactly once and the metadata URL never. A malformed inline payload gives byte-identical output, which is the finding. ### Merge with `main` Putting the base back on `main` made CI resolve the merge, and the Unit Tests job went red on a conflict no side could see on its own: `#771` fixed the same red gate this branch's first commit did, the other way round — it made `component-names.test.ts` import `StrippedName`, `ComponentAnnotation` and `ComponentNameResolution` instead of dropping their `export`. Merged `main` in, reproduced it (`tsc --noEmit -p tsconfig.test.json` → three `TS2459`s) and took main's resolution: the three exports are back. The gate stays green because knip treats test files as entry points, so a type a test imports is not an unused export. All nine workflows are green on the head commit. |
||
|
Ignacy Łątka
|
2e19c97de0 |
fix(cli): accept --help after a telemetry subcommand (#959)
Found while differentially regression-testing the 0.22.1 release against a bundle built from `v0.22.0`. ## The problem `argent telemetry status --help` exits 2 with `Error: Unknown flag: --help`. So do `-h`, and the same after `enable` / `disable`. On 0.22.0 all of them exited 0. ``` $ argent telemetry status --help 0.22.0 telemetry: / state: enabled / device id: not created (rc=0) 0.22.1 Error: Unknown flag: --help (rc=2) ``` `telemetry` is the only subcommand that behaves this way. Checked against every sibling on both versions — `server status|logs|start`, `config list|get|set`, `flow list|run`, `secrets`, `flags`, `enable`, `link`, `unlink` all return 0 on both. And the top-level help tells the user to run exactly this: "Run `argent <command> --help` for command-specific help." ## Cause `TELEMETRY_OPTIONS` declares only `scope`, and `parseCommandArgs(args.slice(1), …)` reads everything after the subcommand — so `--help` arrives as an unknown flag. The `case "--help"` arm in the switch only fires when help is `args[0]`, which is why bare `argent telemetry --help` still works. Introduced when `telemetry` was converted to the declarative parser (#952, extended by #953). ## The fix Hoist the help check above the parser, matching what `config.ts:220` and `flags.ts:92` already do; `server`, `link` and `lens` instead declare `help` in their option spec. Telemetry did neither. Ordering of `telemetryInit` / `telemetryShutdown` around the help path is unchanged, so a help invocation behaves exactly as `argent telemetry --help` did before. ## Tests Six cases added to `telemetry-command.test.ts` covering `--help` / `-h`, bare and after each subcommand, asserting usage is printed and no config file is written. Mutation-checked: with the source change reverted, 4 of the 6 fail (the 2 bare-help cases pass either way, since those already worked) — so the test pins the actual defect rather than the behaviour around it. `@argent/cli`: 498 passed, 3 skipped. `tsc --build` clean, prettier clean. ## Docs No docs update needed — this restores previously documented behaviour rather than changing a documented surface. Co-authored-by: Ignacy Łątka <f.kaminski2000@gmail.com> |
||
|
filip131311
|
efb87f1917 |
chore: bump version to 0.22.1 (#956)
Bumps every workspace package, `package-lock.json`, and both version fields in `server.json` from 0.22.0 to 0.22.1. - `npm version 0.22.1 --workspaces --no-git-tag-version --workspaces-update=false` - lockfile regenerated on Node 24 / npm 11 (version-only diff, 16 lines each way) - `npm run check:versions` passes No docs update needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01EjrDqoeJa5X8Ry2ND1P714 |
||
|
filip131311
|
26405e68c7 |
refactor(cli): one declarative option parser for every hand-written subcommand (#953)
Follows #952 (merged); rebased onto main. ## Why `argent-cli` had **seven** copies of the same argv loop — `server start`, `link`/`unlink`, `flow run`, `lens`, `config`, `enable`/`disable`, and (from #952) `telemetry` — each hand-rolling `--x value` / `--x=value` / `-x` / unknown-flag / missing-value handling with slightly different messages and gaps (e.g. `lens` silently ignored unknown flags). Adding an option meant another `if (tok === "--x") … else if (tok.startsWith("--x="))` pair. ## What `packages/argent-cli/src/command-args.ts` (introduced in #952) now covers the whole subcommand surface, and every command declares its options as a spec: ```ts const START_OPTIONS = { help: { kind: "boolean", alias: "h" }, port: { kind: "value", alias: "p" }, … } as const satisfies OptionSpecs; ``` Parser features: `--name value`, `--name=value`, single-letter aliases, boolean switches, `--` end-of-options, `choices` validation, "don't swallow a following flag as a value", and the guard against a stray `true`/`false` word after a switch (previously duplicated in `flow` and `lens`). Value *semantics* (port ranges, host wildcards, flag-name charset, link targets) stay in each command — the parser only shapes argv. `flag-parser.ts` is untouched: it is the schema-driven `argent run <tool>` payload builder (JSON Schema → tool args, `--args`/stdin hatches), a different job. ## Behavior changes (bad-input paths only) | Command | Before | After | |---|---|---| | `lens --bogus` / `lens extra` | silently ignored | `lens: Unknown flag: --bogus`, exit 2 | | `lens --terminal x` | `--terminal expects "iterm" or "terminal", got "x"` | `--terminal must be "iterm" or "terminal", got "x"` | | `flow run --platfrom=ios` | `unknown flag --platfrom=ios` | `Unknown flag: --platfrom=ios` | | `unlink foo` | `Unknown flag: foo` | `Unexpected argument "foo"` | | `config … --scope x` | `--scope must be "global" or "project", got "x"` | unchanged wording | | `enable/disable`, `server start`, `link` | — | unchanged wording (tests pinned) | Happy paths are byte-identical; `server`/`link` still throw `StartFlagError`, `flow` still throws `FlagParseException` (wrapped from `UsageError`), so callers and tests keep their contracts. ## Tests `argent-cli` 489/489 (26 files): rewritten `command-args.test.ts` (aliases, `-` positional, empty-token value, true/false guard, choices message), pinned-string updates only where the table above says the wording changed (`flow.test.ts` ×4, `parse-link-flags.test.ts` ×1). `tsc --build`, `typecheck:tests`, eslint, prettier, knip green. Smoke-tested every command's error path through the built `packages/argent/dist/cli.js` (see the "Behavior changes" table — all exit 2 with usage where the command prints one). ## Docs No docs update needed: the pages document flags and their meaning, not error strings; no flag was added, removed or renamed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01BkkAihA8Kp26hHS4uhkNGu |
||
|
Jakub Piasecki
|
2e182f7971 |
fix(flows): pin iOS tree reads to the launched app instead of auto-resolving every connection (#772)
## The problem Flow runs on CI iOS fail with `ViewInspector RPC timed out: Application.getState` at seemingly random steps - mid-flow after typing, on the first read after a fresh launch, in a probe flow 25 s after the original failure - while the app under test is provably healthy: spindumps taken at failure time show its main thread idle in the runloop, display links firing, and the injected devtools queue empty. The same flows pass locally. The chain: argent's iOS injection is simulator-wide (`launchctl setenv DYLD_INSERT_LIBRARIES` inside the simulator's launchd), so every process the simulator spawns afterwards loads the devtools dylib, not just the app under test. The dylib's constructor connects unconditionally (no UIKit or `com.apple.*` guard) and the tool-server registers whatever bundle id shakes hands, so Spotlight workers (`mdworker_shared`), background-launched system apps doing first-boot work (`MobileCal`), and bare daemons (nil bundle id, registered as `"unknown"`) all become connections indistinguishable from the app under test. Flow tree reads auto-resolved the target on every read: `queryFullHierarchyTree` passed `undefined` to `resolveNativeTargetApp`, which `Promise.all`s `Application.getState` over every connection. The dylib serves `getState` with a `dispatch_sync` onto the main thread, and a background-launched or suspended process never services its main queue, so its `getState` parks until the 5 s RPC timeout and the `Promise.all` rejects. One poisoned sibling connection fails every tree read in the run; the app under test is never even asked. That one mechanism explains everything the failures did. CI spindumps caught the poisoner red-handed (`MobileCal` parked in `HandleViewInspectorRequest -> CurrentApplicationState`) next to a healthy app. "Permanent wedge" vs "recovery" was just poisoner lifetime: a Spotlight worker exits and its connection drops with it, so reads recover; Calendar lingers and the whole job stays poisoned, fresh app launches and probe flows included. And it never reproduced locally because a warm simulator did its first-boot indexing and migrations long ago, while CI boots a pristine device and does exactly that work during the test window - which also made the failures correlate with typing and tapping purely by timing. ## The fix Pin the target instead of resolving it per read. `runLaunch` records the launched app on the run state once the tree-source gate passes, as `ActionEnv.treeTarget` - a `FlowTreeTarget` carrying the `bundleId`, whether the runner still vouches for it (`pinned`), and whether its probe has ever answered in this run (`probeAnswered`). `fetchFlowTree` threads that target down to `queryFullHierarchyTree`, the only platform source that consumes it. A pinned read talks to the pinned app and nobody else: it never calls `resolveNativeTargetApp` at all, and runs four gates of its own instead (`flow-ios-tree.ts`). A `com.apple.*` id is refused first, terminally and before anything is spent - a system app is never the app under test. Then `isConnected`, a map lookup, so a dead pin reports the dropped connection instead of driving env repair from 300 ms read polls, which could otherwise latch the device's process-wide give-up. Then a probe of the pinned app's own `getAppState`, and only that app's: a timeout before this pin has ever answered is ridden out as the cold start it usually is, one after an earlier answer is refused as an app that stopped servicing its main queue. Finally a foreground check on the answered state, as lenient over one app as auto-resolve is, so a flow that left the app fails with the crisp diagnosis instead of describing a screen that is not on screen. A poisoned sibling connection is never touched. The pin is only trusted while the runner can vouch for what is on screen, and it comes off in two tiers. The five `FOREGROUND_CHANGING_TOOLS` (`launch-app`, `restart-app`, `reinstall-app`, `open-url`, `button`) drop the target outright, because the launched app may no longer be on screen at all; `launch-app` and `restart-app` then re-set it from their own `bundleId` arg, unpinned, since they name the app they switched to - which is how a recorded flow switches apps, the recorder keeping them as raw `tool:` steps. Every other raw `tool:` step demotes the pin to an unpinned hint, its effect on the foreground being opaque to the runner; a launch attempt drops the target outright until it succeeds. Either tier also retires a proven tree-outage memo: a verdict proven against the pinned branch's four gates says nothing about the path the next read runs. An unpinned read is the pre-pin behavior with one arbiter added. `resolveNativeTargetApp` fans out over every connection, and only when that fan-out times out - the CI condition this change exists for - does the hint take the read, and only after passing the same terminal `com.apple.*` policy gate, the connections check, and a foreground-checked probe of its own. A resolution that answers always wins, including its deliberate "single app but backgrounded" refusal, so the arbiter never overrides a guard that fired: it only rides out a probe a stalled sibling made unanswerable. Nested `run:` fragments share the run state, so they inherit the pin, its demotion and its clearing. E2e flows begin with a `launch` by definition, so every selector read in a CI run happens pinned. Only iOS consumes the target; the other platforms' tree sources are per-device and never auto-resolve. The review rounds also surfaced that the `isInjectableBundleId` "library validation, the dylib can never load" rationale was disproven by this PR's own spindump evidence, so that text - and every other home of the same claim, `unreadableHierarchyReason` and `treeSourceGate` included - is rewritten as the policy it actually is (system apps are never the app under test; their observed connections are background processes whose reads hang or lie) with the gates' behavior unchanged. ## What this doesn't cover Auto-resolve's own fan-out is still all-or-nothing, so every read that reaches it without a hint the gates accept stays poisonable: top-level fragment runs, `describe` without an explicit bundle id, and the stretch after a raw `tool:` step whose hint is refused or cannot answer. Recording (`captureTapSelector`) now passes the session's last `launch` id as an unpinned hint, so it arbitrates the same timed-out fan-out, but it never pins - the recorder cannot vouch for the foreground app the way a `launch:` step can. Closing the rest means hardening auto-resolve itself (exclude connections that don't answer instead of failing the fan-out) and refusing the connections at the source (dylib constructor bail for non-UIKit and `com.apple.*` processes, plus a server-side handshake filter to cover already-shipped dylibs). Both are follow-ups. ## Tests Three files, and every behavior below is mutation-proven: disabling the seam, dropping the target from any single call site, removing an invalidation, or removing any one of the four gates each fails at least one dedicated test. The reviewers' repros were turned into tests rather than argued away. `flow-ios-tree-pinned-target.test.ts` works at the tree source. It reproduces the failure shape - a healthy app plus a poisoned `com.apple.mobilecal` sibling whose `getState` only times out - and proves a pinned read probes only the pinned app while an unpinned read still fails. It then covers the states auto-resolve could never reach: the terminal `com.apple.*` refusal, including its position ahead of the connections gate so a disconnected system app is not sent into a restart loop; a dead pin, where the dropped connection is reported and no env repair runs; both sides of the `probeAnswered` split; and the foreground guard over the states a real device produces (`inactive` under a permission dialog, `inactive` with no scenes, `background` still holding a foreground-inactive scene, and `background` with none). A second block covers the unpinned arbiter: the hint read on a timeout, refused when its own probe answers from the background, and rethrowing the fan-out timeout when the hint is disconnected, cannot answer, or when the resolution failed for any other reason. `flow-launch-pins-tree-target.test.ts` proves the runner wiring through the real `fetchFlowTree` seam: every read after a `launch` carries the pin across all five read paths (assert, tap, type, rotate, idle), retry polls included; fragments inherit it; a foreground-neutral `tool:` step demotes it and a foreground-changing one that names no app drops it outright; a later launch re-pins on a fresh target with the ride-out re-armed; and a launch-free run keeps the fallback. `flow-tool-step-tree-target-e2e.test.ts` drives `createRunFlowTool` with only `resolveService` stubbed, so the real `fetchFlowTree` and `queryFullHierarchyTree` run. It pins what the two tiers cost against a wedged sibling: the launched app still arbitrating across a foreground-neutral `tool:` step, the outage memo being retired when that step demotes the pin, the `com.apple.*` and foreground refusals staying in force through the demote, and a `hidden:` assert failing after the demote instead of going green against a backgrounded app. |
||
|
filip131311
|
c2324f052d |
feat(telemetry): project-scope opt-out (restrictive merge) + init --local --no-telemetry (#952)
## Why
Teams that must not send usage data to a third party (a
strict-compliance customer asked) had no way to turn telemetry off for a
repository. `telemetry.enabled` was global-only
(`~/.argent/config.json`), so a committed local install (`init --local`)
still defaulted every teammate to **on** — `--no-telemetry` only opted
out the machine that ran `init`.
## What
- **`telemetry.enabled` accepts the project scope.** `consent.ts` now
reads both `~/.argent/config.json` and
`<project-root>/.argent/config.json` (root resolved from `cwd`, per-file
cache keyed by path) and merges **restrictively**: `false` in either
document wins. A committed project opt-out holds on every clone, and a
project file can never re-enable a global opt-out. Env overrides
(`DO_NOT_TRACK`, `ARGENT_TELEMETRY`) still outrank both.
- **`argent telemetry enable|disable --scope global|project`** (default
`global`). `status` names the document that decided (`config.json
(project)`, `(global)`, `(project and global)`). `enable` at one scope
while the other still opts out reports that telemetry stays disabled
instead of claiming success. Unknown scope / argument → exit 2, nothing
written.
- **`argent init --local --no-telemetry`** also writes
`telemetry.enabled: false` to `.argent/config.json`, and the "commit
these files" checklist lists it. Global mode is unchanged.
- `argent config set/unset telemetry.enabled` stays delegated to `argent
telemetry` (`manageCommand`), so the lifecycle work (draining the live
client) is still done by the dedicated command.
## Tests
Unit: `telemetry` (+8 project-scope cases: single-scope opt-out, both
restrictive directions, both-off, env precedence, live re-read of the
project file, malformed project file), `configuration-core` (schema
scopes), new `argent-cli/test/telemetry-command.test.ts` (real-fs, 6
cases), `argent-installer` — all green; eslint, prettier, knip,
`docusaurus build` green.
Live, from a real `npm pack` tarball of this branch (sandboxed `HOME`):
1. Fresh project → `init --local --yes --no-telemetry --from <tgz>`:
writes `.argent/config.json` `{telemetry:{enabled:false}}` + global
opt-out; summary lists `.argent/config.json` to commit.
2. `telemetry status` → `disabled`, source `config.json (project and
global)`. `telemetry enable` (global) → "stays disabled: config.json
(project) still opts out"; from `~` (no project) → `enabled`. `enable
--scope project` → enabled; `disable --scope project` → disabled.
`--scope team` → exit 2.
3. **Teammate simulation**: copied only the committed files to a fresh
clone with a fresh `HOME` (no global opt-out), bundle built with a dummy
OTLP token, `ARGENT_TELEMETRY_DEBUG=1`, drove `argent mcp` (initialize →
tools/list → `list-devices` call). Control clone without
`.argent/config.json`: tool-server emitted `toolserver:start`,
`tool:invoke`, `tool:complete`, `toolserver:stop`. Clone with the
committed opt-out: **0 events**, and the first-run telemetry notice is
not printed.
## Caveat (documented)
The project file is resolved from the working directory of the process.
Editors start the MCP server in the workspace root, so it and the
tool-server it launches read it. The tool-server is shared by every
editor on the machine and reads the project file of the workspace that
started it; the global opt-out or `DO_NOT_TRACK` still covers the
machine-wide case.
## Docs
Updated in this PR: `reference/configuration.mdx` (scopes + merge
wording), `reference/telemetry.mdx` (new "Opt out for a whole
repository" section), `reference/cli.mdx` (`init`, `telemetry` rows),
`fundamentals/installation.mdx` (`--no-telemetry` in local mode).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01BkkAihA8Kp26hHS4uhkNGu
|
||
|
Hubert Gancarczyk
|
50c9580e30 |
fix(tools): make a schema failure name the parameter that was sent (#718)
> Stacked on #717. ## The problem An invalid tool call returned Zod's raw issue array. Someone who wrote `flow_name` instead of `name` got back, depending on the tool: ``` Invalid params for tool "flow-start-recording": [{"expected":"string","code":"invalid_type","path":["name"]}] Invalid params for tool "flow-execute": [{"code":"custom","path":["flow_path"],"message":"Pass exactly one flow source: name or flow_path."}] ``` The first names the parameter the tool **wanted** and never the one the caller **sent**. The second — flow-execute's, whose `name` is optional so `flow_path` can stand in — names neither: it is anchored on the one source field the caller had no reason to send. Either way the mistake is invisible, and working it out costs a whole turn. ## What changes **1. `describeParamIssues` — one sentence per bad parameter.** Missing fields read as `` `name` is required (string) and was not provided`` rather than as a type error about `undefined`. Unknown keys are named and path-qualified, so a key nested in `selector` is not reported as a bare name that contradicts the top-level list printed beside it. The message closes with the caller's own keys — `You sent: \`flow_name\`, \`project_root\`.` — which is what makes the mistake self-evident. Key names only, never values: this string reaches logs, telemetry and the agent transcript, and a params object can carry a secret. Wired into both entry points: the registry's `invokeTool` and the HTTP boundary. **2. `flow-execute` accepts `flow_name` as an alias for `name`.** The tool is called `flow-execute`, so "the flow's name" spells itself that way; this is a mistake worth absorbing rather than rejecting. The name is resolved in code rather than by the schema — a Zod `required` failure would once again name only the field it wanted — and the error text names the spellings that are silently discarded (`flowName`, `flow`, `flow_file`), because by then Zod has already stripped them and the caller has no other way to learn their key was dropped. **3. A flow DIRECTIVE passed as `command` points at the tool that records it.** `command: "echo"` used to come back as a bare "Tool not found". It now names `flow-add-echo` — and says to call it **directly**, since routing it through the recorder would write the echo *and* a `tool: flow-add-echo` step that fails on every replay. The hint distinguishes directives the recorder **rewrites** from those it stores **raw** for the polish pass, because promising a rewrite that does not happen sends the author looking for a directive that is not in the file. `wait` and `long-press` get their own answers: neither has a recording tool at all. Only a genuine not-found is rewritten — a tool that ran and failed still reports its own error. **4. A recorder tool as `command` is refused.** `flow-add-echo`, `flow-add-step`, `flow-start-recording`, `flow-finish-recording` each mutate the recording itself, and nesting one reported success either way. Each is refused for its own reason: `flow-add-echo` would write the echo **and** a replay-breaking `tool:` step, `flow-start-recording` would **erase** this flow at replay, `flow-finish-recording` would end the take it is a step in, and `flow-add-step` cannot record itself. Now nothing is executed and nothing is written. --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com> |
||
|
filip131311
|
efae6a8fb6 |
feat(tools): opt-in emulator sound on boot-device + boot-sound default flag (#813)
Field report ([Slack thread](https://swmansion.slack.com)): when a session needs to test audio, it is very non-obvious that argent boots the Android emulator muted — the agent (and the user) burned significant time discovering it. The mute default is intentional (several agent-driven emulators all playing sound on the host would be worse), so this PR keeps it and instead makes it **discoverable and overridable**: ## 1. `sound` argument on `boot-device` (default `false`) - `sound: true` boots the emulator with audio output (drops the mute launch flag). - Applied consistently to the snapshot-loadability probe, hot boot, and cold boot — the qemu-device-topology parity invariant is preserved. A `default_boot` snapshot saved in the other audio mode fails the probe and falls through to a cold boot that re-saves it, so toggling costs one slower boot and nothing else. - iOS/Vega/Electron are unaffected (argent never mutes them); the argument documents itself as Android-only. ## 2. Agent-facing information: argument description only Per the thread, the info lives in the tool argument's description (no skill changes): it states the muted default, *why* (multiple agent-driven devices sharing the host's speakers), the `force: true` hint for an already-running muted emulator, the cold-boot cost of toggling, and the `boot-sound` flag. The underlying emulator flag is described behaviorally ("muted"), not by name, per our agent-docs convention. **Validated that the description alone is sufficient** by driving three models through the Cursor `agent` CLI with the schema excerpt and the exact scenario from the field report ("testing notification sound, hear nothing, emulator already booted with defaults"). All three — composer-2.5, gemini-3.7-flash, gpt-5.2 — correctly diagnosed the muted default and produced the exact fix `boot-device({avdName, sound: true, force: true})`, including the weakest model. Transcript excerpts in the PR discussion below if wanted. ## 3. `boot-sound` feature flag `argent enable boot-sound` defaults the `sound` **argument** to `true`. It only moves the argument's default, read live per call: an explicit `sound: false` on a call still boots muted, and without the flag the default remains muted. Registered in `FLAG_REGISTRY` so `argent enable`/`argent flags` pick it up with no further wiring. ## Tests - New `boot-device-sound.test.ts`: mute-by-default, `sound: true` dropping the mute flag from **both** probe and hot-boot spawn (parity), cold-boot spawn, flag-driven default, and explicit-`sound:false`-beats-flag. - The three existing Android boot test files now pin `isFlagEnabled` to `false` so a developer's real `flags.json` can't flip the args they assert on. - `tool-server` boot suites 62/62 green, `argent-cli` flags 33/33, `configuration-core` 122/122; lint, `typecheck:tests`, and knip all pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
filip131311
|
8dda6aa1f9 |
docs: cover the paste tool on the interacting features page (#941)
The `paste` tool was the only capability in the **Interacting** tool group with no mention on the `features/interacting-with-apps` page — typing, gestures, shake, buttons and permissions are all covered there. It already has its row in the tools reference. This adds the minimal parity: - one sentence in **Gestures and text**: for long text, the agent puts the text on the clipboard and pastes it in one step; - the **Limits** bullet for the sim/emu-only restriction, merged into the existing shake bullet, which has the same restriction. No reference changes needed — `reference/tools.mdx` already lists `paste`. Checks: `npx docusaurus build` in `packages/docs` passes; `npm run format` from the repo root leaves the file unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01YPSUSz8gDxGm7tb9tA3b4D |
||
|
filip131311
|
b232114422 |
docs: audit every comment in src against the code it describes (#931)
Audits every comment in `src/` and `scripts/` against the code it
describes.
**462 files, 57 commits, net −8,194 lines.** Comment text only — the
whole branch is code-identical to `main`.
## Method
One subagent per file, strictly sequential. Scope was `src/` +
`scripts/` (459 files); three more files were added at the end because
they carried dead references of the same kind — two test headers citing
design docs that do not exist, and `publish-npm.yml` citing a retired
workflow. Each agent verified every comment — line, block, JSDoc, file
header, trailing — against the surrounding code and the rest of the
repo, following identifiers, paths, tool ids, config keys, env vars and
issue links to see whether they still exist and still behave as
described. Rules:
- **A false or misleading comment is deleted, not reworded.** If a claim
could not be confirmed by reading the source, it went. That is why the
deletion count is so much larger than the rewrite count.
- Survivors are cut to the shortest form carrying something the code
does not already say. Restatement, preamble, hedging, changelog prose
and ASCII banners are gone; the non-obvious *why* stays.
- Preserved byte-identical: license headers, pragmas and directives
(`@ts-*`, `eslint-disable`, shebangs, `/// <reference>`), JSDoc tag
tokens, everything inside a string or template literal, and the sole
comment inside an otherwise empty block (ESLint `no-empty` counts a
comment-bearing block as non-empty).
## Verification
Every file passed two independent gates before being recorded as done:
1. `comments-only` — the required check.
2. A second comment-stripping comparator with a proper mode stack,
written for this pass because `comments-only`'s flat scanner desyncs on
nested template literals and quote-bearing regex literals and then
reports comment lines as code changes. Two files hit that false FAIL
(`utils/android-profiler/pipeline/index.ts`,
`scripts/extract-tools.mjs`); in both the "changed code" it printed was
literally `//` lines, and the second checker confirmed the code was
byte-identical.
After the last file, all 462 changed files were re-checked against
`main` with the same comparator, rather than trusting any agent's
self-report. **459 code-identical; 2 are non-code (`.svg`, `.md`); 1
intentional.**
The intentional one is `packages/argent/scripts/bundle-tools.cjs`: the
changed template literal *is* the comment header of the file it
generates, `packages/native-devtools-android/src/bundled-meta.ts`.
Fixing only the generated file would have been reverted by the next
build, so the generator changed too — and it has been verified to
reproduce the committed generated file byte-for-byte.
## Representative false claims removed
Not wording nits — statements a reader would have acted on:
- **Reversed directions.** `proxyStart`'s JSDoc had the tunnel backwards
(it is a reverse tunnel: the host binds first and the simulator dials
in). A `paste()` doc had the pasteboard copy direction reversed.
- **Contradicted by the code below it.** A timeout budget multiplied by
three where the probes run concurrently — the same comment said so six
lines later. A "warn once" that warns on every call. A "binary search"
that is a linear scan.
- **Named things that do not exist.** A `vega-fast-cli` binary, a
`finish-recording.ts`, a `publish-next.yml` workflow, two
`profiler-react19-*.md` design docs, a `DebuggerTarget.ts`, a commit
hash git does not know, two tool ids, an `ensureEnv` cycle.
- **Wrong by construction.** "Welford accumulators" across four files
where the code keeps naive `n`/`sum`/`sumSq`; `sum`/`sumSq` documented
over `actualDuration` when reduce sums `selfDuration`; a strict-mode
halving written `n/2` where the code ceils; field docs listing enum
values the producers never emit.
- **Guarantees the code does not make.** A validation matrix claiming to
cover "EVERY tool" that skips flagless ones; a Pareto cutoff that
`slice(0, 20)` makes inert; an idempotence claim where the real rule is
at-or-ahead; a capability note describing a clean 400 the shape-based
device resolver can never produce.
- **Unverifiable assertions** about prebuilt binaries, external CLIs and
the cloud SDK — deleted rather than kept as folklore, since nothing in
the repo can confirm them.
- **Stale numbers**: invented Android tool versions, hard-coded tool
counts and description lengths that had drifted.
## Review
A Fable agent reviewed both halves adversarially for over-deletion,
misread code, `no-empty` hazards and byte-identity violations.
Second-half verdict: **SHIP**, with two one-line restores, both applied
in the final commit — the `npm view ""` rationale behind a blank-token
guard, and the note that `argent-mcp` keeps a copy of
`SECRET_PLACEHOLDER_MARKER` it cannot import.
## Code issues surfaced but deliberately not fixed
This pass changes comments only. Eight genuine findings are logged for a
follow-up:
1. `telemetry/src/consent.ts` — a non-ENOENT read error returns null and
falls through to the default-on path, so file errors *can* silently flip
telemetry on.
2. `chromium-server/navigation.ts` — `navigate()` is reachable from
`POST /api/navigate` with only a `typeof === "string"` check; open-url's
schema is a bare `z.string()`, so the "already validated by zod" premise
never held.
3. `http.ts` — `constantTimeEqual` returns early on a length mismatch,
so the auth token's length is observable.
4. `describe/index.ts:~114` — the ios-remote branch passes `{ isTvOs:
false }` unconditionally, so a remote tvOS simulator takes the iOS
ax-service path, though `isRemoteTvOsSimulator` exists and shake/paste
do use it.
5. `devices/boot-device.ts` — `-crash-report-mode never` is passed
unconditionally *and* appended again by the feature-detecting path, so
every emulator spawn passes it twice.
6. `react-profiler/pipeline/04-rank.ts` — `PARETO_THRESHOLD_PCT` is
dead: `slice(0, 20)` always wins.
7. `reaped-sessions.ts` — a user-facing hint string tells the agent that
`react-profiler-start { force: true }` disposes the debugger and
profiler session; it does not. Left byte-identical because it is a
string literal, not a comment.
8. `utils/simctl-backend.ts` — `localSimctl` is exported with no
importers anywhere.
## Docs
No documentation change is needed: this pass touches only source
comments, and no user-facing capability, tool, CLI flag, config key or
flow-file behaviour changed.
---------
Co-authored-by: filip131311 <f.kaminski2000@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
filip131311
|
5d76ed4d66 |
feat: tag artifacts with a semantic kind at registration (#564)
Fresh, from-current-main take on the artifact-kind idea from #398 / #466. Knowing what an artifact *represents* (not just its MIME type) is what a UI needs to group and name artifacts (e.g. in EAS); this PR delivers exactly that — the tagged handle — without the declared-output-map machinery. ## Summary - Adds a closed `ArtifactKind` union to the artifact wire contract; `kind` is carried on `ArtifactHandle`, `ArtifactEntry`, and the `/artifacts` inventory items. - `ArtifactStore.register` now takes a single `{ hostPath, kind, ... }` options object, so the compiler forces every producer — current and future — to declare what it is emitting. - Tags **all** producers on main today, including the ones #398/#466 predate: screenshot (all four capture paths), screenshot-diff, **flow snapshot baselines/diffs** (`screenshot` / `screenshot-diff-context`), **screen recordings** (new `screen-recording` kind), and the native/React profiler exports and reports. - Types the native-profiler export keys at their source (`IosExportKey` from the `EXPORTS` table, `AndroidExportKey`) and maps them through a kind table that is **total over both unions** — a newly added export key fails to compile until classified, replacing the previous stringly-keyed loop (and the `?? "native-profile-trace"` silent-default this design would otherwise need). - Client-side (`@argent/tools-client`), the mirrored handle type keeps `kind` **optional and widened** (`ArtifactKind | (string & {})`): an older, already-running tool-server sends no kind, and a newer server may send kinds this client build predates. Consumers treat it as advisory metadata; unknown kinds are opaque, never an error. ## Differences from #398 / #466 (deliberate) - No `@argent/artifacts` package, no third `ToolDefinition` generic, no per-tool output-name registrar, no runtime throw for “undeclared output” — the kind is plain data at the registration call site, checked at compile time where it can be, and a wrong label can never fail a tool at runtime. If a pre-invocation `artifacts` declaration in tool-listing metadata is ever needed (an actual EAS consumer), it can be layered on later without changing this wire contract. - The two `ArtifactKind` definitions (registry / tools-client) stay deliberately separate — same as the existing `ARTIFACT_MARKER` must-match convention — because the client-side type is intentionally *looser* (optional + open) than the server-side one for version skew. - Extending the union stays backward-compatible on the wire. ## 2026-08-24 update Merged current main (0.22.0) into the branch. The flow-visual rewrite that landed since (cropOn crops, per-app snapshot-key dedupe, diff scratch-dir hygiene) is preserved verbatim; its five registration sites now carry kinds (`screenshot` for baseline/current, `screenshot-diff-context` for the annotated diff — the cropped `current` included). No producer added since the PR was opened needed a new kind: `paste`, `shake`, and the other new tools emit no file artifacts, and every register call site in the tree now goes through the typed options object. No docs update is needed: the docs site (added since this PR) does not document the artifact handle wire contract; `kind` is advisory wire metadata with no user-facing surface. ## Validation At the merge with main (0.22.0), 2026-08-24: - `npm run build` (full `tsc --build`) — clean - `npm run typecheck:tests` in registry, tools-client, tool-server, mcp, cli — clean - Full suites green: registry 90, tools-client 191, tool-server 4495 (+1 skipped), mcp 80, cli 472 - `prettier --check .`, `eslint . --max-warnings 0`, and the knip gate — all pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
Kacper Kapuściak
|
e0fcf6c995 |
chore(docs): add Google Search Console verification meta tag (#939)
Adds the `google-site-verification` meta tag to the docs site via Docusaurus `headTags`, so docs.swmansion.com/argent can be verified as a property in Google Search Console. ## Test plan - `npx docusaurus build` in `packages/docs/`, confirm the meta tag renders in the built HTML `<head>`. - After deploy, hit Verify in Google Search Console. No further docs update needed: this is a site config change with no user facing capability change. |
||
|
Hubert Gancarczyk
|
f21811d069 |
fix(flow): re-probe a recorded wait against the tree the runner reads (#717)
> Stacked on #715. ## The problem `await-ui-element` evaluates against the tree it reads — the **accessibility hierarchy** on iOS and Android, the **CDP DOM** on Chromium, the **toolkit page source** on Vega. The `await:` / `assert:` directive that the polish pass converts a recorded wait into is evaluated against the **runner's** tree instead. They overlap, but outside Vega neither contains the other — an id present in one can be absent from the other, and on iOS even the role vocabularies are disjoint. Both directions have been hit in practice: an id visible to the runner but not the recorder, and text-field values visible to the recorder but not the runner. The consequence is that a check can pass live and fail once converted. That makes the core promise — *each step is executed live so you verify it works before it's recorded* — untrue at exactly the point it is relied on. ## What changes After a recorded `await-ui-element` succeeds, the recorder re-runs the same condition against the tree the runner resolves directives against, and reports the result on the step it just wrote. **It warns; it does not refuse.** What gets written is a raw `tool: await-ui-element` step, and at replay that tool reads the same tree it just passed against — so "this would fail every run" was never true of the form actually recorded. What the probe genuinely reports is whether the **polish-time conversion** is safe, which is why the warning says to act on it *then* rather than re-record now (re-recording would duplicate the step). Because polish begins after `flow-finish-recording`, each verdict is also carried there: it is repeated on its own step's `summary` line, and `message` counts how many steps carry one. A warning raised at step 7 of a 40-step recording is otherwise long out of reach by the time it is actionable. ## Three outcomes, kept distinct | Runner tree says | Reported as | | --- | --- | | condition holds | nothing — step recorded clean | | condition does not hold | recorded + warning naming the causes to rule out before converting | | tree unreadable, or too slow to answer | recorded + warning that the conversion is **UNKNOWN, not known-bad** | The middle row is deliberately not "the conversion will fail". The identical verdict comes back when the screen simply moved on between the live wait and the re-probe, and on `text` checks when the selector matches several elements and the two sides elect different ones — so the consequence stays conditional on the cause, and the message names the causes in the order worth ruling out. The third row matters: nothing was compared, so calling it a divergence would send the author off to rewrite a selector that may be perfectly good. It is also the injection-free case the flow skills explicitly sanction, so refusing there would block a supported way of working. On iOS the quoted reason ends "provide bundleId explicitly" — advice aimed at the native tools. The warning corrects it, because no directive carries a bundleId and forwarding one would make the probe answer a question the runner never asks. ## Platform-specific explanation The *why* differs per platform, and stating the iOS story everywhere makes the message wrong where someone is trying to act on it. iOS gets AX-vs-native-hierarchy; Chromium gets "the flow tree keeps only addressable nodes"; Android gets the importance trim; Vega gets "the trees agree, so the screen changed". The named tool for reading the runner's side follows the same rule — pointing an Android author at `native-find-views` points at a tool they cannot call. The remedy is per **condition** as well as per platform: on `hidden` a determinate verdict means the runner's tree still *has* the element, so "retarget at an id the full hierarchy carries" is the opposite of what helps. ## A failed live wait is reported by cause `await-ui-element` returns `{ success: false }` for three different things: a condition read and found false, a tree source that never answered, and a cancelled wait. Only the first judges the condition, so only the first is narrated as "the wait never held" and prescribes re-recording or deleting the step. The other two observed nothing and are reported as UNKNOWN — the same unknown-vs-known-bad distinction the probe draws for the runner's tree. A wait nested inside a recorded `run-sequence` gets neither warning: that tool reports its own shape, which carries no `success` key. The skill says so, and says to read `toolResult` there. --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com> |
||
|
filip131311
|
366f17c619 |
chore: bump version to 0.22.0 (#891)
Bumps every workspace package, `package-lock.json`, and both `server.json` version fields from 0.21.0 to 0.22.0 (18 files). Lockfile regenerated on Node 24 / npm 11 so the diff is version-only; `npm run check:versions` passes. Covers everything on main since `v0.21.0`: `paste` tool (#879), rotation-aware Android capture (#693), 0.25 default screenshot scale (#878), no-reboot describe on externally booted sims (#583/#877), NDJSON flow progress (#855), OTLP telemetry (#570), native-devtools restart_required derivation (#560), Android profiler empty-CPU explanation (#669), plus test/CI fixes. Full-surface live QA of this main (tgz vs npm 0.21.0) on iOS, Android, tvOS, Electron, CLI and Linux/remote found zero regressions. Two release notes: the published artifact must be built with `ARGENT_OTEL_INGEST_TOKEN` set (a local tgz without it compiles `readIngestToken()` to an empty string, making telemetry a silent no-op), and remember to push the `v0.22.0` git tag so online `init` skills installs resolve. No docs update needed: version-only change. |
||
|
filip131311
|
172e4d3a64 |
fix(android): capture the screen the way it is actually oriented (#693)
Part of #609. ## What was wrong On a rotated Android device `screenshot` returned a portrait-framed image with the content lying sideways. `describe` frames and gesture coordinates were **already upright** — uiautomator measures against the rotated display — so the capture was the one surface out of step. An agent reading that image saw geometry that disagreed with every coordinate it was given. Measured on a landscape Pixel_9 (API 36, `mCurrentOrientation=1`): | | space | |---|---| | `describe` | upright — `"Search Settings"` centre `(0.530, 0.234)` | | taps | upright — tapping describe's `"Display & touch"` centre opens it | | `screenshot` (default) | **portrait-native, sideways** | ## The fix simulator-server already applies `rotation` correctly; nothing was telling it which one. The rotation is now queried from the device per capture, so `screenshot`, `screenshot-diff`'s live captures and the auto-attached "Screen after action" screenshot all land in `describe`/tap space. **Nothing is tracked.** A cached orientation could only be staler than the ~40 ms probe, and serving a stale one is exactly the failure being fixed. When the rotation cannot be read the request is left byte-identical to before rather than guessing — a wrong orientation is a confidently wrong image, which is worse than a sideways one. **iOS is deliberately untouched.** There the whole surface is consistently portrait-native, so rotating only the capture would *break* an agreement rather than restore one. The iOS half of #609 needs a different change and is not in this PR. ### Also fixed: the legacy uiautomator describe path Same root cause, worse symptom. `wm size` reports the **unrotated** size — on the landscape Pixel_9 it still answered `Physical size: 1080x2424`. Dividing rotated bounds by that does not merely squash frames: `isVisibleRect` prunes nodes whose left edge is past the screen width, so **the right-hand half of a landscape screen vanished from the tree**. The dump carries `<hierarchy rotation="N">`, so the divisor is oriented from bytes already in hand — no extra round-trip on the path that runs precisely when the helper could not be reached. ## On the rotation-name table Pinned by measurement, never by what the names mean — the reporter of #609 found the same names are 180° inverted on iOS. `adb exec-out screencap` is rotation-aware, so it is ground truth; each candidate was scored as mean absolute difference against it both as-is and rotated 180°, so an inverted mapping could not pass: ``` rotation 1 LandscapeLeft 2.19 (vs 6.54 for its 180° twin) rotation 3 LandscapeRight 2.19 (vs 6.34) rotation 2 PortraitUpsideDown 2.67 (vs 13.13) rotation 0 Portrait identity ``` This table is the **inverse** of simulator-server's own `90° → LandscapeRight` convention, because it compensates for a rotation that layer applies while decoding the scrcpy stream. That makes it a property of a particular simulator-server build rather than of Android — which is why a table test alone is not enough, and the delivered PNG's aspect is checked against the rotation requested, falling back to an unrotated capture if they disagree. ## Verified live Branch tool-server against a real emulator: - landscape, default capture → **2424×1080 upright**, MAD 2.19 vs ground truth (6.54 vs its 180° twin) - cropping `describe`'s frame for the toolbar out of that default capture shows exactly the "Display & touch" toolbar — the two spaces agree - rotation 0 → 1080×2424, MAD 2.82, unchanged - rotated **iPad** → still 2064×2752 sideways, i.e. iOS provably untouched Plus 27 new unit tests, including one that asserts the legacy path *drops* the right-hand node before the fix and keeps it after. Full suite 3125 passing; lint, prettier, `typecheck:tests` and the tool-description gate all clean. ## Not in scope - The iOS half of #609. - `rotate` on Android silently no-ops when auto-rotate is enabled (it returned `{orientation:"LandscapeLeft"}` while `dumpsys` still read `cur=1080x2424`). Filing separately. - `screen-recording` still records sideways — same root cause, different pipeline. Worth noting a session's screenshots are now upright while its video is not. - Existing visual baselines captured on a rotated Android device will stop matching, since the baseline key embeds capture dimensions and those now swap. This fails loudly as a missing baseline rather than silently comparing the wrong thing — and those baselines were sideways captures of an upright UI. --- > **#673 and #658 are both stacked on this branch** — each conflicts with this one (`screenshot/index.ts` and `screenshot-diff/index.ts` respectively) but not with each other, so both were rebased onto this branch as siblings rather than chained. Merge this first; GitHub then retargets both to `main`. --------- Co-authored-by: Filip131311 <f.kaminski2000@gmail.com> |
||
|
filip131311
|
d0b3356ebf |
feat(tool-server): paste tool (device clipboard + paste shortcut) (#879)
## Summary Registers a real `paste` tool. The `tools/paste/index.ts` from #506 was never registered and sent a `{cmd:"paste"}` WebSocket command simulator-server doesn't implement — dead on every platform. - **iOS simulator**: simulator-server `POST /api/clipboard/text` fills a private pasteboard and pushes it into the sim synchronously (host clipboard untouched); the tool then sends the ⌘V chord over the existing `key` command (the same `simulatorSendKeyEvent` path the keyboard tool uses). - **Android emulator**: same route over the emulator gRPC bridge, then `KEYCODE_PASTE` via `adb shell input`. - **ios-remote**: the MoQ transport's existing paste primitive, awaited. - Clean 400 for tvOS / Android TV (runtime probe), physical devices, Chromium (a renderer paste would clobber the *host* clipboard), Vega. - `{{secret:<NAME>}}` placeholders as in `keyboard`; allowed inside `run-sequence`; device-interact SKILL updated. - Description/skill tell the agent `paste` is **not** a `keyboard` substitute — only where a real user would paste (OTP/2FA code, long link/token, testing paste handling). ## Blocked on a simulator-server built with the `clipboard` feature: software-mansion/radon#185 adds it to the `argent` alias (no sim-server API change). Until a release + bin bump, the tool reports `PASTE_CLIPBOARD_UNSUPPORTED` (with an update hint) against the shipped binary. ## Test plan - 13 new unit tests (`test/paste.test.ts`) + secret-placeholder tests; full tool-server suite, lint, knip green (the 4 telemetry/bind test files that fail locally fail identically on `main`). - Live against a locally built simulator-server: iOS 26.5 Settings search (12 back-to-back pastes in order), Messages composer (multi-line/tab/emoji/diacritics preserved), `{{secret:EMAIL}}` never echoed or logged; Pixel_9 Settings search (ASCII + `Zażółć🔑`), Expensify RN login field; `run-sequence` paste steps; live 400s for tvOS sim / phone serial / chromium id. |
||
|
Kacper Kapuściak
|
5e81136096 |
docs: generate an Open Graph card for every page at build time (#889)
## What Every docs page now gets its own Open Graph card, rendered during the build. The card is `static/img/og-background.png` with a frosted glass panel in the middle: a 920x400 rounded rectangle with a 32px radius, the background blurred behind it (`backdrop-filter: blur(60px)`, an SVG `feGaussianBlur` of stdDeviation 30) and tinted with `#C3D3E033`. The white Argent logo sits centred on the panel, with the page title centred underneath in DM Sans Medium at `#FFFFFF`. Before, every page shared one static `img/logo-icon.png`. ## How `plugins/og-image/index.js` is a Docusaurus plugin with a `postBuild` hook. It: 1. Renders the background and the frosted panel once. That part is the same on every card, so it is reused as a bitmap and only the logo and the title are rendered per page. 2. Reads each built page's `<title>`, strips the ` | Argent` suffix, and lays out the logo and the title with satori. Long titles wrap and stay centred. 3. Rasterises the result with resvg into `build/img/og/<route>.png`. 4. Rewrites the `og:image` and `twitter:image` tags of that page to the new absolute URL, and adds `og:image:width` / `og:image:height`. Satori embeds every glyph as a path, so resvg needs no font lookup. DM Sans Medium is vendored in `scripts/og-assets/` (with its OFL license), so CI renders the cards exactly as a local build does. Layout, colours and blur are constants at the top of the plugin. Pages with no title of their own get a card with the logo alone. That covers the home page, whose title is the site title the logo already shows, and the search page, whose title the Algolia search theme renders as `[object Object]`. The shared `@swmansion/t-rex-ui` theme points doc pages at `img/og/<title>.png` and hardcodes a "React Native Reanimated" fallback. The plugin overwrites those tags after the build, so the naming is route based and consistent across doc pages, the home page, search and 404. ## Verification Against a clean `npx docusaurus build`, checked for all 20 built pages: the tag is an absolute site URL, the target exists in `build/`, it is a valid PNG at 1200x630, `twitter:image` matches `og:image`, the width/height tags are 1200/630, and `twitter:card` is `summary_large_image`. No unreferenced cards. Serving the production build over HTTP, every card URL returns `HTTP 200` with `content-type: image/png`. `npm run typecheck`, `npm run build`, and the root `npm run format` and `npm run lint` all pass. ## Notes - The cards add ~16MB to a 65MB build, about 845KB each. The background is a photograph and resvg only emits PNG. Re-encoding as JPEG would bring that down to roughly 2MB at the cost of a `sharp` dependency, if the deploy size becomes a concern. - New `packages/docs` dependencies: `satori` and `@resvg/resvg-js`. Both record all platform binaries in the lockfile, including `linux-x64-gnu` for the CI runner. - The branch also carries a `docs: update favicon` commit, which replaces `static/img/favicon.png`. ## Docs No user facing docs change: this is build internals. The build step is documented for contributors in `packages/docs/CLAUDE.md`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01JovgRQBFngBsHnUYSoRF56 |
||
|
Kacper Kapuściak
|
36aa14a5fc |
docs: add Argent documentation (#814)
Adds the Argent documentation site, published to [docs.swmansion.com/argent](https://docs.swmansion.com/argent): a branded Docusaurus project under `packages/docs/`, the full first set of pages, and the CI that checks and deploys it. <img width="1512" height="945" alt="Screenshot 2026-08-21 at 13 05 04" src="https://github.com/user-attachments/assets/4607e143-4359-49a8-93a2-aaa3af4c9534" /> ## Site A standalone Docusaurus 3.9 project, built on the shared Software Mansion docs theme ([`@swmansion/t-rex-ui`](https://www.npmjs.com/package/@swmansion/t-rex-ui)) and restyled for Argent: - **Palette** from [argent.swmansion.com](https://argent.swmansion.com): dark `#0D0F26`, blue `#99DAFF`, lavender `#E4E1FF`, light `#FEFEFE`, mist `#F1F1F1`. The `--swm-*` token names stay, since the shared theme resolves against them. - **Typography**: DM Sans and DM Mono, matching the landing page. - **Sidebar** with lucide icons on every page entry (`sidebar_custom_props.icon` in the front matter, registered in `src/theme/SidebarIcon`), the logo kept on the page background and the panel inset from the edge. - **Video component** for embedded screen recordings, with `scripts/encode-video.sh` producing a web-sized MP4 and a poster frame for each clip in `static/video/`. - **Copy page button** and a few theme fixes: inline code badges in tables, paginator hover. - **No landing page.** That stays at argent.swmansion.com. The root route redirects to Getting started. ## Content Three sections in `docs/`: - **Fundamentals**: getting started, installation, supported platforms. - **Features**: interacting with apps, flows, network, screen recording, lens, visual regression, profiling, debugging. Conceptual overviews with recordings, each linking to the tools reference. - **Reference**: tools, CLI, configuration, flow YAML, editors, telemetry. Prose follows Simplified Technical English. The conventions (style, front matter, icons, checks) are written down in `packages/docs/CLAUDE.md`, and a root `CLAUDE.md` adds a checklist so that code changes to tools, CLI, configuration or flows update the matching docs page in the same pull request. ## CI and deploy - `Docs build` runs `format:check`, `lint`, `typecheck` and `build` on every pull request that touches `packages/docs/`. The build has `onBrokenLinks: "throw"`, so it catches links left dangling by a moved page. - `Docs publish` deploys `packages/docs/build` through the GitHub Actions Pages source on every push to `main` that touches the docs. The repository Pages source needs to be set to **GitHub Actions** for the first run. ## Notes for review - The site is excluded from the root `packages/*` workspaces (`!packages/docs`) and keeps its own `package.json` and `package-lock.json`, so its dependency tree stays out of the toolkit's lockfile. Root Prettier, ESLint and knip ignore it; it formats and lints itself with the repo `.prettierrc` and its own `eslint.config.mjs`. `check-workspace-versions.mjs` skips it so its `0.0.0` version is not read as drift. - `webpack` is pinned to `5.105.4` and `@docusaurus/plugin-content-docs` / `theme-common` to `3.9.2` via `overrides`. Newer webpack fails Docusaurus 3.9's ProgressPlugin option validation, and a hoisted `plugin-content-docs@3.10.2` produced a duplicate React context that crashed SSR. - **Search is not wired up yet.** The shared theme always mounts a DocSearch bar, so an `algolia` block has to be present. It reads `ALGOLIA_APP_ID` / `ALGOLIA_API_KEY` / `ALGOLIA_INDEX_NAME` from the environment and the bar stays hidden until Argent has its own DocSearch application. Follow-up PR. ## Testing - `npm run format:check`, `npm run lint`, `npm run typecheck` and `npm run build` pass in `packages/docs/`. - Root `prettier --check`, `eslint` and `npm run knip` pass. - Walked the served production build in the browser in light and dark themes while iterating: root redirect, docs pages, sidebar icons, videos, TOC, footer. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Ignacy Łątka
|
08f91fffd1 |
perf(screenshot): lower the default capture scale to 0.25 (#878)
Screenshots are billed to an agent's context by **pixel area**, not by file size — so PNG level, JPEG quality and codec choice buy nothing here, and the default scale is the only real lever. `0.3` had never been measured against what a model can actually read, so I measured it. ## What was run Bluesky (`bsky.app`) built from source and driven on an iOS 18.6 simulator, signed in against the repo's own seeded dev PDS (`yarn e2e:mock-server` + `e2eSignInAlice`) — a live feed, profile, notifications, Explore and Settings, not marketing screens. Every image came out of argent's own `screenshot` tool over the tool-server HTTP API, so the downscaler under test is the shipped one. **160 API calls**: 5 screens × 5 scales × 2 models × 3 reps, plus a blind control per screen per model. Questions were the things an agent actually needs off a screen — an `@handle` two rows down, a relative timestamp, a follower count, a badge number, which tab carries the underline, the label of the row above another row. Two controls decide whether the numbers mean anything: - **Blind pass.** Each model answered every question with *no image at all*. The 6 of 30 questions it still got right from priors alone (the screen title, the tab selected by default) were struck from scoring. 24 survived. - **Counting split out.** "How many rows between X and Y" was unreliable at *every* scale including full resolution — it measures arithmetic, not legibility — so it is excluded from the headline and reported separately. ## Result Text-reading accuracy, 66 graded answers per cell. Token counts are the API's own reported `input_tokens`, image column net of the ~345-token prompt. | scale | image tokens | Haiku 4.5 | Opus 5 | |---|---|---|---| | 1.0 | 2692 | 100.0% (66/66) | 100.0% (66/66) | | 0.3 *(old default)* | 451 | 98.5% (65/66) | 100.0% (66/66) | | **0.25** *(new default)* | **298** | **100.0% (66/66)** | **100.0% (66/66)** | | 0.2 | 196 | 98.5% (65/66) | 100.0% (66/66) | | 0.15 | 114 | 81.8% (54/66) | 97.0% (64/66) | 0.25 is the only scale that scored perfectly for both models on every repetition, and it costs **34% less context** than 0.3. ## The cliff, and why the margin Every wrong answer at 0.15: | question | truth | answered | model | | |---|---|---|---|---| | 2nd author's handle | `@bob.test` | `@carla` | Haiku ×3 | read the row above | | selected feed tab | `Following` | `Feeds` | Haiku ×3 | couldn't resolve the underline | | 4th profile tab | `Videos` | `Likes` | Haiku ×3 | drifted two positions | | 3rd settings row | `Moderation and content filters` | `Privacy and security` | Haiku ×3 | off by one row | | bell badge number | `1` | `UNREADABLE` | Opus ×2 | declined rather than guess | **Opus fails safe; Haiku fails silent.** Opus refused the unreadable badge. Haiku returned a confidently wrong *neighbouring* row — the same wrong answer three times out of three. For an agent that turns a read into a tap, a confident off-by-one row is worse than a refusal, because nothing downstream catches it. ## Why 0.25 and not 0.2 0.2 measured no worse than the old default and would have saved 57% instead of 34%. I didn't take it: the evidence is five screens of one app at one device size, and Bluesky sets fairly generous type. A denser interface — a table, a settings screen with subtitles, a smaller phone — reaches the floor sooner. 0.25 keeps two steps of margin above the cliff instead of one. `ARGENT_SCREENSHOT_SCALE=0.2` stays available for anyone who wants to spend that margin deliberately. ## Scope - **Chromium is untouched.** Its default is 1.0 and its `scale` is silently dropped without the optional `sharp` dep — that's #673's territory and I'm not duplicating it. Worth noting the two interact: once Chromium can downscale (via the in-repo `resizeDecodedPng`, as #673 suggests as a follow-up), it should pick up this default too, and it is currently by far the most expensive platform per capture. - **`ARGENT_SCREENSHOT_SCALE` is documented as `0.3` in `argent-private/docs/environment-variables.md`** — a submodule, so it needs its own PR to stay in sync. - Stale prose fixed in passing: `getScreenshotScale` carried a `// default: halve the resolution` comment next to a `0.3`, and `vega-screen.ts` documented the fallback as `0.3`. ## Checks `npm run build` clean, `tsc --noEmit -p packages/tool-server/tsconfig.test.json` clean, eslint `--max-warnings 0` clean, prettier clean. Shard 1/4 of the tool-server suite: 88 files / 1261 tests pass; the four files touching screenshot scale pass individually (22 tests). The default had **no test coverage at all** — the constant could be edited to any value and the suite stayed green. This adds one. Mutation-checked: reverting the constant to `0.3` fails 6 of the 8 new cases. ### On the red Unit tests check Inherited, not introduced. This branch's Unit tests job fails on exactly one test — `describe-tool.test.ts > states the limitation without ordering a reboot when the degraded read returned a tree` — which is the same single test failing on `main` itself (main has been red since #583, this branch's base). Already tracked as #871 and fixed by #877. `flow-composition.test.ts` passes in CI on both. Everything else is green: ESLint, Prettier, Knip, Static checks, SpiderShield, lockfile sync, Linux + Chromium E2E, macOS + Chromium E2E, macOS + iOS E2E. |
||
|
Ignacy Łątka
|
d43938e4fe |
fix(describe): drop the redundant restart assertion that reddened main (#877)
Fixes #871. Supersedes #872, which fixed this the wrong way round. ## The assertion was demanding a restatement `describe-tool.test.ts:793` asserted: ```ts // It still names the remedy and what the remedy costs. expect(result.hint).toMatch(/boot-device/); expect(result.hint).toMatch(/restart/i); ``` against `DEGRADED_STANDING_HINT`, which reads: > …If something you expect is missing, **boot-device with force=true reboots the simulator** with the full accessibility settings "reboots the simulator" is already there. So `/restart/i` could only ever be satisfied by a *second* clause restating the reboot — and that is precisely what #583 carried: ``` … with the full accessibility settings — at the cost of restarting it and everything running on it. ``` A review suggestion applied through the GitHub web editor trimmed that clause (`dd99a2625`, `07f39cea8`) at 07:41:49Z and 07:41:57Z. The squash-merge fired at **07:42:23Z** — 34 seconds later, so no CI ran on that head. The trim was right: it removed eight words that said the same thing twice. The test was what was wrong. #872 restored the redundant clause to turn CI green. This deletes the assertion instead, and leaves the shipped hint byte-for-byte as it is on main today. ## No coverage leaves with it The four remaining assertions are the ones that carry the test's purpose — #583 existed to stop the caveat *ordering* a reboot that kills the developer's Metro session: ```ts expect(result.hint).not.toMatch(/MUST/); expect(result.hint).not.toMatch(/\bnow\b/i); expect(result.hint).not.toMatch(/before continuing/i); expect(result.hint).toMatch(/boot-device/); ``` Mutation-checked. Restoring the pre-#583 wording — `"You MUST call boot-device with force=true now to restart the simulator … before continuing."` — fails the test on the first assertion: ``` × states the limitation without ordering a reboot when the degraded read returned a tree AssertionError: expected 'This simulator was not booted through…' not to match /MUST/ Test Files 1 failed (1) | Tests 1 failed | 36 passed (37) ``` So the regression the test guards is still guarded. The deleted line was pinning a redundancy, not a behaviour. ## Verification Branched off `dd5d82383` (current main). Reproduced the failure there first — `1 failed | 36 passed` on the untouched tree. | gate | result | |---|---| | `vitest run` (tool-server) | **351 files, 4313 passed, 1 skipped**, exit 0 | | `tsc --build` | clean | | `typecheck:tests --workspaces` | clean | | `eslint . --max-warnings 0` | clean | | `prettier --check` | clean | Diff is one test file, one assertion and half a comment: ```diff - // It still names the remedy and what the remedy costs. + // It still names the remedy. expect(result.hint).toMatch(/boot-device/); - expect(result.hint).toMatch(/restart/i); ``` The comment loses "and what the remedy costs" because that half described the assertion being removed. |
||
|
schicaelmott
|
dd5d823833 |
feat(flow): stream run progress as NDJSON (#855)
## Summary - add `argent flow run --json-stream` - stream flow progress as JSON lines - finish with a JSON result or error ## Why `--json` prints only after a flow finishes. `--json-stream` lets scripts and CI see live progress. ## Why this is a separate PR This is useful without the custom-tools feature in #856. ## Validation - `npm test -w @argent/cli -- flow.test.ts` — 106 tests passed - `npm run typecheck:tests -w @argent/cli` - `npm run build -w @argent/cli` --------- Co-authored-by: Filip131311 <contact@filipkaminski.com> |
||
|
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. |
||
|
Ignacy Łątka
|
13f43bd0cd |
test(chromium): pin the ports-file guard and the readiness failure code (#862)
Follow-up to #709, which merged while this was still under review. #709 landed the port fix; these are the two gaps a review pass over it turned up, both in the same two files. ## The ports-file guard was pinned by nothing #714 added `vi.mock("../src/utils/chromium-discovery", …)` to `boot-electron-spawn-error.test.ts` so a completed boot stops persisting its port into the developer's real `~/.argent/chromium-cdp-ports.json`. Nothing asserted it still worked. Measured, on the pre-change file: deleting the mock leaves the suite **green at 9/9** while `$HOME/.argent/chromium-cdp-ports.json` is created containing a live ephemeral port. Replacing it with a call-through variant — the shape `list-devices.test.ts:76-81` already uses on this same module — does the same: ```ts vi.mock("../src/utils/chromium-discovery", async () => { const actual = await vi.importActual<...>("../src/utils/chromium-discovery"); return { ...actual, trackChromiumPort: vi.fn(actual.trackChromiumPort), untrackChromiumPort: vi.fn(actual.untrackChromiumPort) }; }); ``` → `Tests 9 passed (9)`, and the ports file contains `[55770]`. That is how the file accumulated entries before #714, so the guard rotting back is not hypothetical. Each file now scopes `HOME` with #714's own `scopeTempHome()` helper and asserts, after a completed boot, that no ports file exists. Both mutations now fail. **All three files that carry this mock are fixed, not just the one.** `boot-electron-kill.test.ts` and `boot-electron-ready-race.test.ts` hold the byte-identical mock — under a comment naming each other — and each completes a real boot. Measured before this change: deleting the mock left `kill` green at **15/15** while eleven live ports, one per boot, were written; the call-through variant did the same to `ready-race` **while still satisfying its own `expect(trackChromiumPort).toHaveBeenCalledWith(port)` at line 175**, which is precisely why the absence assertion is the right shape. Asserting the absence rather than `expect(trackChromiumPort).toHaveBeenCalledWith(port)` is deliberate: the call is already pinned by `boot-electron-ready-race.test.ts:175`, and a spy assertion only proves the export *is* a spy — the call-through mutation satisfies it while still writing. ## The readiness timeout was pinned by its wording, not its code The catch-path test matched `/CDP never became reachable/`. `FAILURE_CODES.CHROMIUM_ELECTRON_CDP_TIMEOUT` had **no assertion anywhere in the repo** — `grep -rn CHROMIUM_ELECTRON_CDP_TIMEOUT packages/ --include=*.ts` outside `dist/` hits only the throw site and the enum. Swapping the throw site to `CHROMIUM_ELECTRON_SPAWN_FAILED` left `boot-electron-spawn-error` + `-kill` + `-ready-race` + `failure-classification` + `chromium-discovery` at **60/60 passed**. So the half agents and telemetry bucket on was free to change, while a harmless reword would have broken the test. Now pinned by code via `getFailureSignal`, matching how `boot-electron-ready-race.test.ts:137` pins its own failure path. It still discriminates the vacuity the message match was added for: a nonexistent `appPath` yields `CHROMIUM_ELECTRON_APP_PATH_INVALID` and fails. `failure_stage` is pinned alongside it, as `ready-race` already does for its own path — `electron_cdp_ready` likewise had no assertion anywhere. ## Two rationales corrected - The catch-path comment blamed "no HTTP server on the port" for the probe failing. `fetch` declines port 1 as a WHATWG bad port *before it dials* — verified with `net.connect` hooked: **0 sockets opened** for port 1, 2 sockets for ports 2, 1023 and 19222. No listener is involved either way, and the file's own constant JSDoc says so. - The `chromium-discovery.test.ts` constant pointed at "a `listen(0)` fake CDP server **in a sibling test file**", when the server that makes the point is `startFakeCdpServer` in this same file, above the constant. - It also credited "anything in the unprivileged range" with being bindable by `listen(0)`. `listen(0)` draws from the kernel's *ephemeral* range — `net.inet.ip.portrange` 49152-65535 on macOS, `ip_local_port_range` 32768-60999 on the Ubuntu runner. That narrower range is what put the old `43211` at risk; calling it "unprivileged" also revives the privilege reasoning the sibling constant explicitly disowns. ## `chromium-discovery.test.ts` gets a second layer too It is the one file here that drives the **real** `persistPorts`, and its only guard was a single `beforeAll` line setting `ARGENT_CHROMIUM_PORTS_FILE`. That line is pinned — deleting it fails 3 tests — but the write lands before the detection, and `persistPorts` merges rather than replaces, so on a real machine the test ports would be appended for `list-devices` to probe afterwards. It now scopes `HOME` as well, so a lapsed redirect falls back somewhere disposable. Measured with a pre-seeded `$HOME/.argent/chromium-cdp-ports.json` = `[9222]`: deleting the redirect still fails 3 tests, and the seeded file comes back **unchanged**. ## Verification | mutation | before | after | |---|---|---| | delete the module mock | 9/9 pass, ports file written | fails | | call-through `vi.fn(actual.trackChromiumPort)` | 9/9 pass, ports file written | fails | | swap `CHROMIUM_ELECTRON_CDP_TIMEOUT` at the throw site | 60/60 pass | fails | | nonexistent `appPath` in the catch-path test | — | fails (`APP_PATH_INVALID`) | | swap `failure_stage` at the throw site | — | fails | | delete / call-through mock in `boot-electron-kill` | 15/15 pass, 11 ports written | 11 failed \| 4 passed | | call-through mock in `boot-electron-ready-race` | 2/2 pass | 1 failed \| 1 passed | Full `packages/tool-server` in 6 shards: 4303 passed, 1 skipped, every shard exit 0, and no `.argent` directory created under a redirected `HOME`. `eslint` 0, `prettier --check` 0, `tsc --build` 0, `tsc -p tsconfig.test.json` 0, `knip` 0 — exit codes read directly. |
||
|
Ignacy Łątka
|
6d31aed752 |
fix(describe): stop ordering a simulator reboot on every externally-booted read (#583)
Fixes #521. ## The report holds, and is worse than described Reproduced against an unmodified `origin/main` tool-server driving a simulator booted the way a developer boots one (plain `xcrun simctl boot`, no argent involvement), iOS 18.6: ``` ===== describe call #1 ===== ===== #2 ===== ===== #3 ===== hint: "This simulator was not booted through argent — system dialogs and native modals may not appear. You MUST call boot-device with force=true now to restart the simulator and apply full accessibility settings before continuing." ``` Identical on every call, as reported. The part that makes it more than a wording nit is what came back *alongside* it — a complete, working tree: ``` ROOT AXGroup (0.000, 0.000, 1.000, 1.000) AXStaticText "19:51" (0.139, 0.023, 0.102, 0.023) AXGroup "Maps" value="Widget" (0.082, 0.096, 0.395, 0.206) ... ``` And the caveat's own claim does not hold either. Triggering a real system permission alert on that same externally-booted sim (`simctl privacy reset location com.apple.Maps` + launch): ``` AXGroup "Allow “Maps” to use your location?" (0.193, 0.367, 0.614, 0.296) AXButton "Allow Once" (0.193, 0.524, 0.614, 0.046) AXButton "Allow While Using App" (0.193, 0.570, 0.614, 0.046) AXButton "Don’t Allow" (0.193, 0.617, 0.614, 0.046) ``` The system dialog reads fine, buttons and all. So an agent obeying "You MUST ... now" restarts the simulator — killing the Metro session, dev client and staged app state — to fix nothing. ## Root cause `AXServiceApi.degraded` is `!entitlementBypassActive`, read from `IgnoreAXServerEntitlements`. That pref is only writable pre-boot, so `boot-device` is the only thing that ever sets it and *any* externally-booted sim reads as degraded for its whole life. The pref bypasses the AX-server entitlement check introduced in iOS 26.5 — on earlier runtimes it is irrelevant to whether the read works. The flag describes how the sim was booted, never whether describe succeeded, and the hint conflated the two. ## Change Resolve the caveat against what the accessibility read produced: - **Blind read** — the reboot has something to gain, so the actionable guidance stays, on every call, and now names what the reboot costs. - **Populated read** — the read is its own proof no reboot is needed. States the limitation instead of ordering anything, once per device per server lifetime. The per-device gate is applied at the `describe` tool boundary only. `await-ui-element`, the flow runner and `await-screen-idle` poll `describeIos` directly, treat a present `hint` on an empty tree as "this emptiness is untrustworthy" (`isBlindRead`), and fold it into one terminal timeout note — they keep every copy, so a `hidden` wait still cannot resolve off a blind read. The native-devtools fallback can fill a tree the accessibility read left empty, and that split has to key on the accessibility read rather than on the tree that comes back. Its elements are the injected app's own view hierarchy: no SpringBoard chrome, no system dialog, and no evidence the accessibility subsystem works. Found doing exactly that on a fresh iOS 18.6 sim. Describing within seconds of `simctl boot` leaves the accessibility read blind for the rest of that boot (3/3; it survives a tool-server restart, and only `boot-device force=true` recovers it). With the Maps location prompt on screen in that state, the fallback returned the app's own 20 elements with the dialog absent — and resolving against *that* tree picked the standing note, which the per-device gate then deleted, so from the second call the agent had a tree silently missing a modal blocking the screen and no caveat at all. The note it briefly carried says "everything else reads normally", which nothing there did. Keying on the accessibility read makes both wordings true of the state that produces them. I skipped the suppression env var / flag the issue floated as a "maybe" — once the caveat is a single non-directive line per device, there is little left to suppress, and it seemed better not to add config surface for it. Happy to add it if you'd rather have the switch. ## Verification E2E against the branch build, same externally-booted sim: | | elements | hint | |---|---|---| | describe #1 | 14 | standing caveat, no MUST/now | | describe #2-4 | 14 | *(none)* | | after `simctl shutdown` (blind) ×2 | 0 | blind caveat, both times | | after `boot-device force=true` ×2 | 28 | *(none)* — `IgnoreAXServerEntitlements` = 1 | Unit: 6 new tests in `describe-tool.test.ts`. Each was mutation-checked — restoring the MUST wording, removing the dedup, collapsing the blind/standing split, keying the gate globally instead of per device, and re-resolving the caveat against the native-devtools tree each fail exactly one test and nothing else. Full gate: `vitest run` 4715 passed / 1 skipped, `tsc --build`, `typecheck:tests`, `eslint --max-warnings 0`, `prettier --check`, `test:scripts` 59 passed. |
||
|
Ignacy Łątka
|
9dc7c7cfcb |
test(chromium): stop electron boot tests depending on host CDP ports (#709)
## Problem
`boot-electron-spawn-error.test.ts` drove `bootElectronApp` against
hardcoded CDP ports 19222-19226 and relied on nothing answering there.
## It is not load, and it is not the timeout
I chased the obvious hypothesis first — vitest's 5000 ms default
colliding with a ~3.5 s built-in wait — and it is wrong. Measured at
`origin/main` before any edit:
| condition | result |
|---|---|
| 15 isolated runs | 15/15 pass |
| 10 concurrent copies | 10/10 pass |
| 3 full-suite runs (load avg 16) | 3/3 pass |
| 10 runs under sustained full-suite load (load avg 19-20) | 10/10 pass
|
**38/38, a 0% spontaneous failure rate.** And the one test whose
`readyTimeoutMs` is 5000 against vitest's 5000 ms default completes in
15-54 ms at load ~20 — nowhere near the boundary.
## The real trigger, and it is deterministic
Anything on the host answering the hardcoded CDP ports the test used.
Squatting one with an HTTP server that serves `/json/version` fails the
file 100% of the time. Reproduced against `origin/main`:
```
× rejects with a clear, actionable message when spawn emits ENOENT
AssertionError: promise resolved "{ platform: 'chromium', …(5) }" instead of rejecting
+ "id": "chromium-cdp-19223",
Tests 1 failed | 8 passed (9)
```
**Mechanism.** When a real endpoint answers, `waitForCdpReady` wins
`bootElectronApp`'s `Promise.race` in ~1 ms, long before the test's 10
ms `setTimeout` fires its synthetic error. The boot then completes and
*resolves*, so the rejection the test asserts never arrives.
The source is correct throughout. The test bound a global shared
resource it did not own. `chromium-cdp-19222` is argent's canonical
device id (`src/utils/chromium-discovery.ts:9`) and
`e2e-device-smoke.yml:139,173` boots a real Electron on exactly that
port — a developer with an Electron app open, or a second agent on the
same machine, is enough.
## Fix
One `UNREACHABLE_CDP_PORT` constant pinned to port 1, which
`ensureCdpReachable` can never reach: it dials through Node's `fetch`,
which refuses port 1 as a WHATWG **"bad port"** before it opens a
socket. Measured:
| port | result |
|---|---|
| **1** | `fetch failed` → cause `bad port`, no socket opened |
| 2 | `connect ECONNREFUSED 127.0.0.1:2` |
| 22 | `bad port` |
| 1023 | `connect ECONNREFUSED 127.0.0.1:1023` |
Privilege is *not* the mechanism, and would not be sufficient: 2 and
1023 are equally privileged yet dial normally, and Windows has no
privileged range at all. The JSDoc says so, so the constant cannot be
"tidied" below 1024 and quietly reinstate the flake. Not a timeout
widening.
## Second defect, same file
This file was the only one of the three Electron boot tests that did not
isolate port persistence, so its success-path boot wrote a real
ephemeral port into the developer's actual
`~/.argent/chromium-cdp-ports.json` on **every run**. That file
currently holds 107 entries, and I watched a concurrent session on this
machine append one mid-review.
Both siblings — `boot-electron-kill.test.ts:27` and
`boot-electron-ready-race.test.ts:30` — already guard this with
`vi.mock("../src/utils/chromium-discovery", …)`. This file now uses the
same mock, and asserts `trackChromiumPort` was called with the booted
port, so deleting the guard fails the suite instead of silently resuming
the writes.
## Sibling fixed
`chromium-discovery.test.ts` asserted port `43211` was unreachable, but
that sits inside Linux's ephemeral range (`ip_local_port_range` → `32768
60999`), so a `listen(0)` fake CDP server in a parallel test file can
legitimately be handed it. It now shares one named `UNREACHABLE_PORT`,
replacing that literal and a second bare `1` that was already there.
**Left alone, verified safe:** `boot-electron-kill.test.ts`,
`flows/flow-chromium-boot.test.ts`, `list-devices.test.ts` — each owns
its port via `listen(0)` or fully mocks the launcher. The fixed ports
`43210`, `54321`, `54322` (all in `chromium-discovery.test.ts`) are only
ever tracked or untracked, never network-probed.
## Verification
- Deterministic repro above, reproduced at `origin/main` and re-run
after the change with all five of 19222-19226 squatted: **20/20 pass**.
- **Mutations, each of which passed 9/9 before the guard was added:**
deleting the module mock → `TypeError: [Function trackChromiumPort] is
not a spy`; pointing the catch-path test at a nonexistent `appPath` →
`expected … /CDP never became reachable/ but got 'path does not exist'`.
- Ports file: full suite under a redirected `HOME` writes no
`chromium-cdp-ports.json` at all; the real file's md5 is unchanged
across runs.
- `eslint` 0, `prettier --check` 0, `tsc --build` 0, `tsc -p
tsconfig.test.json` 0 — exit codes read directly.
- Full `packages/tool-server` in 6 shards: 4065 passed, 1 skipped, every
shard exit 0.
|
||
|
Ignacy Łątka
|
f79e53a6e3 |
test(tool-server): scope vega-cli-timeout sentinels to the running process (#711)
## Problem
`vega-cli-timeout.test.ts` fails intermittently, always the same two
tests, always the same way:
```
rejects on its own deadline when the CLI never returns
→ expected { stdout: '', stderr: '' } to be an instance of Error
reaps the ENTIRE worker tree on timeout
→ promise resolved "{ stdout: '', stderr: '' }" instead of rejecting
```
Both at **~60-130 ms against a 400 ms deadline** — `runVega` *resolved*
rather than timing out. It never fails in isolation.
## Not a tight budget, and not a reaping race
| condition | failures |
|---|---|
| isolated, 20 runs | 0/20 |
| under a parallel full-suite run, 15 runs | **2/15** |
| pinned to one CPU (`taskset -c 0`), 8 runs | 0/8 |
The single-CPU pin is the discriminator: if the 400 ms budget were too
tight, or the reap raced under contention, starving it to one core would
make it worse. It passes 8/8.
`packages/tool-server/src/utils/vega-cli.ts` is **not** implicated and
is unchanged here.
## Cause: the sentinel namespace is machine-global
The test's worker sentinels were fixed constants (`sleep 69101` … `sleep
69107`), and cleanup matches them with `pkill -f` / `pgrep -f` — which
are **global to the machine, not scoped to the process**.
Two concurrent runs of this file therefore share one namespace, and each
run's `afterEach` `pkill`s the *other* run's live workers. The fake
`vega` launcher's blocking `spawnSync("sleep", …)` returns early, the
launcher exits 0, and `runVega` resolves cleanly well inside its
deadline — exactly the observed signature.
The existing comment shows the pattern was already narrowed once
(*"Match only this suite's sentinels (6910[0-9]) rather than a loose
`sleep 691`"*). That tightened it against other suites but not against a
second copy of this suite, which is the case that actually occurs: CI
matrices, `vitest --no-file-parallelism` reruns, and any machine running
more than one checkout.
## Deterministic reproduction
A neighbour issuing this file's own sweep (`pkill -f '^sleep
6910[0-9]$'`) on a 20 Hz loop reproduces it **every time**:
```
before → Tests 2 failed | 7 passed (9) (3/3 rounds, both named tests)
after → Tests 12 passed (12) (3/3 rounds, same neighbour live)
```
## Fix
Derive the sentinels from a per-process tag (pid plus three random
digits) and scope every `pgrep`/`pkill` to it. Two live processes cannot
share a pid, so two concurrent runs never collide; the random digits cut
the leftover case — a worker leaked by an earlier run whose pid has
since been recycled — to 1/1000. A concurrent run computes a different
tag and its sweeps no longer reach these workers.
The tag rides in the *fractional* part of the sleep duration, which also
shortens a leaked worker's life from ~19 h to 10 min. That matters more
than it looks: previously a later run of this file would clean up an
abandoned worker as a side effect of the shared namespace, and scoping
the namespace deliberately removes that accident.
The same investigation turned up three more problems in the file, and
the rest of the diff is those:
- **Regressions reported a bare timeout.** Several tests here outrun
vitest's 5s default - the
delayed-close test's own deadline is 10s - so a real failure printed
`Test timed out in 5000ms` and
named nothing. The file now sets its own 20s budget; `beforeAll`
carries an explicit one because a hook resolves its budget when it is
registered.
- **Four assertions could pass with nothing to assert against.**
`waitForClear` reads 0 just as readily
for a worker that never spawned, so both reap tests observe their
workers alive first, and `strayCount`
returns -1 rather than 0 when it cannot look at all. The two output-cap
reaps have no such window (#841).
- **The reap test's premise was unasserted.** It is named for a worker
that escaped the launcher's process
group, but nothing checked that it had: with the observation below
removed, dropping `detached: true`
from the `hang` fake leaves every test green, and with that fixture the
descendant sweep could be
deleted from `reapVegaGroup` with everything still green. One polled
sample - a single `ps`, so the count and the groups cannot disagree
about the
instant they describe - now returns both, so dropping the flag fails
with `{ workers: 2, groups: 1 }` — naming which half broke — while
deleting the sweep still fails with `expected 1 to be +0`.
Smaller, each covered below: the near-deadline `timeoutMs` 600 -> 900ms,
now sharing one const with the
bound that pins it; the reap deadline 400ms -> 4s, to give the launcher
room to spawn both workers
before the reap ends the observation window; a `beforeAll` warm-up that
absorbs macOS's first-exec penalty; and three added tests (sweep
positive control, random-block width,
longer-pid decoy). Three comments about production behaviour were
corrected too - the header's account of
which mechanism reaps which worker, the reap test's on what the
descendant sweep covers by itself, and
the fake's on what reaps the launcher's own sleep.
## Verification
- Deterministic repro above, before and after, same neighbour process.
- **Teeth check** — a sweeper matching the post-fix *shape* but ignoring
the tag (`^sleep 600\.[0-9]+$`) still kills the fixed code 4/4. The
workers remain real and killable; only the tag protects them, so the fix
is not passing by making them unmatchable.
- **Vacuity check** — mutating the source (`collectDescendantPids` →
`[]`) makes `strayCount` report a live stray (`expected 1 to be +0`),
proving the new escaped `pgrep` pattern genuinely matches.
- **Mutation** — pinning `RUN_TAG` to a constant, with a neighbour using
the pattern a concurrent run of the *mutated* file computes: 4/4 fail.
It now takes down four tests, not the two originally named — the two
observations this PR adds, the near-deadline elapsed bound and the drain
count, are killed by the same neighbour.
- **Positive control** — every reap assertion here reads
`waitForClear(...) === 0`, which is also what
`strayCount` returns when the pattern matches nothing, so the suite
passed with a pattern that matched
nothing. Two of the three `sentinel bookkeeping` tests require a count
of 1 first, and the drain reap counts its
worker before the reap; the timeout reap counts through `ps` instead, so
it is the bookkeeping tests
that stand behind it here. Neutering `cmdlinePattern` fails those three
(`expected +0 to be 1`),
neutering `sweep()` fails one (`expected 1 to be +0`); the rest stay
green in both cases.
- **The workers under test are the ones observed** — giving only the
fake launcher's `hang` workers a
sentinel `strayCount` cannot see leaves the whole suite green without
the counts, and fails
`{ workers: +0, groups: +0 }` against `{ workers: 2, groups: 2 }` with
them. Same for the drain path. The two overflow tests keep no count, and
are
therefore **not** pinned: breaking only the `flood` branch's sentinel
leaves all 12 green. Their reap
fires within ~20-100ms of the spawn, so an observation taken after the
call never catches it (0/8), and
one taken concurrently loses the race about a third of the time under
load. Filed as #841, which
records a non-racy alternative.
- **Each observation is budgeted off the call it watches** — a poll
shorter than that call turns a slow
launcher start into a failed observation (`expected +0 to be 1` on the
drain path, `{ workers: +0,
groups: +0 }` on the reap) rather than into the reap it guards. The
drain
observation gets its call's whole 10s; the timeout reap's window is
ended by the reap itself, so there
the deadline *is* the budget, and it and the poll are both 4s. The reap
observation's worst was 1485ms at
50-way concurrency against that 4s (2.7x); the drain observation's own
worst was 1836ms against its 10s
(5.5x). Stalling the `linger-grouped` branch 3.5s fails the drain
observation on the 3s default poll and
passes on the call's 10s; stalling `hang` 2.5s fails the reap
observation on a 2s deadline, which kills
the launcher mid-stall, and passes on 4s.
The reap and drain observations are both new here, so the reap deadline
is what the added runtime buys: that test goes from
~450ms on main to 4.13s, about 3.7s of the file's 4.4s increase.
Measured against the branch's own
intermediate 2s deadline the step is +1.95s idle (7.18 -> 9.13s) and
+1.99s under load (10.52 -> 12.51s), and it takes
that observation from 1.35x to 2.7x. The file's thinnest margin is the
near-deadline row above, which no
deadline change can widen. The drain-side change is free, since the poll
returns as soon as the count
matches. An interleaved A/B soak, 40 runs per arm at load 90-240,
produced **no** failure of either observation in **either** arm, so it
is neutral rather than
confirming: the case rests on the injected stall and on the measured
margins, not on a reproduced
natural rate.
- **Slot width is pinned** — `SentinelSlot` makes a 2-digit slot a
compile error
(`TS2345: Argument of type '10' is not assignable to parameter of type
'SentinelSlot'`). A 2-digit slot
would both escape the sweep's `[0-9]` and break the equal-width argument
the isolation rests on.
- **The launcher is warmed in `beforeAll`** — the drain-grace test gives
the launcher 900ms to start,
and no larger value is available: it also has to stay under
`VEGA_EXIT_DRAIN_GRACE_MS` (1000) to force
the race at all. macOS charges the FIRST exec of a newly written file
and nothing
after it, and that cold exec does not fit — measured p90 1099ms / max
1684ms at 20-way concurrency
under 92 hog processes, which made that test fail **38 of 60 (63%)**
when run on its own. It is not
`node` starting: a fresh `#!/bin/sh` script shows the same jump, and
warming `node` elsewhere leaves the
solo rate at **55/60** - worse than not warming at all. Without the
warm-up a whole-file run never saw
it, because whichever tests run first pay the exec incidentally. Paying
that cost once in `beforeAll` makes it invocation-independent: startup
at that call site measured
136-148ms without the warm-up and 32-37ms with it.
- **Margins, worst observed against budget** — across ~950 runs at up to
80-way concurrency under 92-132 hog
processes (load avg 218-275):
| wait | budget | worst observed | margin |
|---|---|---|---|
| near-deadline launcher start | 900ms | 700ms (80-way) | **1.29x** |
| `elapsed < 5_000` (delayed close) | 5000ms | 3017ms | 1.66x |
| worst `waitForClear` | 3000ms | 1648ms | 1.82x |
| reap observation | 4000ms | 1485ms | 2.69x |
| `elapsed < 2_500` (deadline) | 2500ms | 742ms | 3.37x |
| slowest test vs `testTimeout` | 20_000ms | 4130ms (idle) | 4.84x |
| drain observation | 10_000ms | 1836ms | 5.45x |
The slowest-test row is an idle measurement, and a floor rather than a
worst: the reap test's launcher
never exits, so that test cannot settle before its own 4s deadline. The
thinnest margin is structural,
not tunable: that test's `timeoutMs` must clear the launcher's startup
*and*
stay under `VEGA_EXIT_DRAIN_GRACE_MS` (1000) for the race it forces to
exist at all. Both bounds sit in
that one sub-second band, so buying margin here spends it on the
`toBeGreaterThan` bound below; the
`beforeAll` warm-up is the only lever that does not. It still
discriminates —
dropping the exit path's `clearTimeout` fails it with `promise rejected
"FailureError: vega linger ..."
instead of resolving`. The `toBeGreaterThan(900)` bound on that same
test sits at 1.11x but is safe by
construction: the drain cannot fire before `grace` ms and scheduler
delay only pushes it later, so load
can never move it toward the bound.
- **Considered and not taken: sweeping the eight exact sentinels** —
replacing the `[0-9]` slot glob with an
ERE alternation of the literal sentinels looks like it would drop the
equal-width invariant and the two
tests that pin it. It would not. `pkill -f` still matches every process
on the machine, so isolation still
rests on equal width: pid `1234` / rand `567` / a two-digit slot `11`
spells `600.123456711`, byte-identical
to pid `12345` / rand `671` / slot `1`, so the second run's alternation
lists the first run's worker
verbatim and kills it. The unpadded random block collides the same way
(`1234`+`56`+`7` = `123`+`456`+`7`).
`SentinelSlot`, the padding test and the `$` anchor are load-bearing
under either form; only part of
`cmdlinePattern`'s docblock would go.
- **Deliberate idiom deviation, flag if you disagree** — `vi.setConfig`
and `testTimeout` appear nowhere
else in the repo; the convention here is a positional `it(..., 10_000)`
(`boot-device-hardening.test.ts:214`)
or `{ timeout: N }` (14 uses). Several tests in this file can exceed the
5s default, and two rounds of
per-test budgeting each missed some, so the budget is set once for the
file instead of enumerated per
test. The wall-clock assertions (`elapsed < 2_500`, `< 5_000`) are
unaffected — they assert speed
directly rather than relying on the test budget.
- **The wrapper self-match is real, but GNU-only** — `pgrep`'s own `sh
-c "pgrep -f '<pattern>' || true"`
wrapper carries the pattern in its argv. Verified in debian against
procps-ng 4.0.2: a bare sentinel with
no anchors returns **2** matches (the second being the wrapper), and
`^`, `\.` or `$` alone each cut it
to 1. On macOS it cannot be reproduced at all — BSD `pgrep` excludes its
own ancestors — so the docblock
says which platform the hazard belongs to.
- **The `$` anchor is pinned** — a decoy shaped like a longer-pid run's
sentinel must survive `sweep()`.
Dropping `$` kills it (`expected +0 to be 1`). `^` is deliberately left
unpinned: without it the pattern
reaches only command lines ending in this run's unique tag, which
nothing can produce.
- **The slow-failing regressions stay legible** — tests here outrun
vitest's 5s default on a runVega
deadline of up to 10s, a 3s clearance poll, or both. Past 5s vitest
reports `Test timed out in 5000ms` and
names nothing, so the file raises its own budget with `vi.setConfig({
testTimeout: 20_000 })`. Measured,
each with the matching production path disabled: the delayed-close test
`Test timed out in 5000ms` @5029ms
→ `promise rejected "FailureError: vega linger …" instead of resolving`
@10031ms; the flood test
`Test timed out in 5000ms` @5059ms → `expected 'timeout' to be
'subprocess'` @5030ms; the reap test
→ `expected 1 to be +0` @7254ms. No regression in this file reports a
bare vitest timeout any more.
- Two simultaneous runs, 4 rounds: 8/8 green; 15 sequential and 15
concurrent-triple runs green, no strays.
- `eslint` 0, `prettier --check` 0, `tsc -p tsconfig.test.json --noEmit`
0 — exit codes read directly.
## Sibling audit
No other test identifies its workers by matching a pattern against the
machine-wide process table — the only other `pkill`/`pgrep` invocation
under `packages/` or `scripts/` is a research CLI
(`argent-private/research/physical-ios/cli/argent-ios.ts:441`), which is
not a test and owns no sentinel workers. Every candidate checked:
`adb-terminal-error-format` (mocks `child_process`), `await-ui-element`
/ `await-screen-idle` / `mjpeg-stream` (in-process timers, no
subprocess), `artifacts` (real `tar`, no deadline assertion),
`list-devices` (`ps` mocked), `vega-process` (real `ps`, read-only, owns
no workers), `launcher-helpers` (TCP, no subprocess).
That is the narrow claim. The broader one — "the only test that spawns
real workers into a global namespace under a deadline" — does **not**
hold: `launcher-spawn`, `launcher-duplicate-spawn`,
`launcher-version-gate` and `launcher-sweep` all spawn real long-lived
servers (the first three through `spawnToolsServer`, so detached;
`launcher-sweep` a plain `spawn`), and `launcher-spawn` asserts on
elapsed time. They are safe for a reason worth stating, because it is
the same principle as this fix: each redirects `HOME` to a `mkdtemp`
dir, so their state directory is per-run.
Several unrelated tests do share the defect and are filed separately,
since none is in this diff: #835, #836, #837.
The mutation sweep run over this suite also surfaced `runVega`
behaviours its assertions do not pin: the signal-death `subprocess`
classification (#845), `settle()` clearing the exit timer (#847), the
output cap's UTF-8-byte semantics (#849), and both `destroy()` pairs
(#850) each survive as a mutant through a fully green run, as does
`VEGA_KILL_SIGNAL` softened to `SIGTERM` (#848). The process-group
SIGKILL (#846) is caught, but incidentally and flakily - by the
stderr-cap test, in 1 of 3 consecutive runs. All of these predate this
PR: the nine tests that exercise `runVega` are the same nine on `main`,
the three added here covering the sentinel helpers themselves.
|
||
|
Ignacy Łątka
|
c0a63875df |
test(debugger): pin the source-map wait debugger-status promises (#706)
Found while reviewing #610 and set aside as out of scope: the code and the prose it comes from are byte-identical at that PR's merge base, and the same mutant survives there too. ## The gap `debugger-status` returns `sourceMapReady: true` as a **hardcoded literal** (`debugger-status.ts:57`). The only thing that makes the literal true is the line above it: ```ts await api.sourceMaps.waitForPending(); ``` and the description sells exactly that: *"sourceMapReady (always true — **waits for pending source maps before returning**; no-op on Chromium)"*. Nothing pins it. Deleting the await: | | existing suite (`test/debugger test/metro test/flows test/utils`) | |---|---| | unmutated | 856 passed | | await deleted | **856 passed** | So a status that reports readiness it never established would ship green. ## Scope, stated honestly This is a test-only change; the shipped behaviour is correct. The impact of a regression is narrow but real: `js-runtime-debugger.ts:214` already drains the registry once while the service is being created, so the await in the tool is what covers maps registered **after** that point - a Fast Refresh, or a lazily loaded bundle chunk. In that window a non-awaiting status would return `sourceMapReady: true` while `SourceMapsRegistry.pendingRegistrations` is non-empty, and a `debugger-inspect-element` issued straight after can miss the file:line it would otherwise resolve. I did not manufacture a user-visible failure end to end, and I am not claiming one - the finding is that the single promised behaviour of this field has no test. ## The tests Both assert the **ordering**, not the literal - a test that only checked `sourceMapReady === true` would pass on the mutant, since the literal is unconditional. 1. `does not resolve until the pending source-map registration settles` - hands the tool a `waitForPending` that returns a deferred promise, drains several macrotask turns, asserts the tool has not returned, then releases it. 2. `reports sourceMapReady only after the wait` - records `["maps-settled", "status-returned"]` and asserts that exact order. | | new tests | |---|---| | on `main` as shipped | 2 passed | | with the await deleted | **2 failed** | eslint `--max-warnings 0`, `tsc --noEmit -p tsconfig.test.json` and prettier all clean. |
||
|
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`. |
||
|
filip131311
|
82d0209b9e |
fix(android-profiler): explain an empty CPU section instead of rendering it as an idle app (#669)
Fixes #629. ## Reproduced, then diagnosed harder than the report Pixel_9 / API 36, `com.android.settings`, real interaction — all three symptoms reproduced: ``` native-profiler-analyze → 70 hangs + RSS populated, NO CPU section, "showing top 0 CPU hotspots", exportErrors: {} thread_breakdown → 296 samples across 9 threads function_callers epoll_wait → not found ``` The report infers "unsymbolized frames". It's stronger than that — I queried the `.pftrace` directly: | | value | |---|---| | samples for the process | 296 | | samples with a `callsite_id` | **0** | | `stack_profile_frame` rows (trace-wide) | **0** | | `stats.perf_samples_skipped` | **296** | | `unwind_error` | NULL on every row | **The unwinder never ran.** There are no frames to symbolize. That's why `thread_breakdown` — which counts `perf_sample` rows and never joins the symbol tables — reports 296 while every stack-dependent view is empty. ## Positive control: same emulator, debuggable app ``` adb shell getprop ro.build.type → user dumpsys package com.android.settings | grep pkgFlags → [ SYSTEM HAS_CODE … ] ← no DEBUGGABLE dumpsys package com.anonymous.myapp | grep pkgFlags → [ DEBUGGABLE HAS_CODE … ] ``` | | com.android.settings | com.anonymous.myapp (debuggable) | |---|---|---| | samples / with callsite / named leaf | 296 / **0** / **0** | 86 / **86** / **78** | | frames in trace | **0** | **723** | | `perf_samples_skipped` | **296** | **0** | | analyze | no CPU section | `writel` 48.72%, `internal_get_user_pages_fast` 6.41% | So this is **not** an emulator limitation and **not** symbolization — it's target profileability. The remedy the message gives is verified to work on the very device it describes. ## The fix The pipeline detects "samples exist, none carry a usable stack" and says so. It names the count and the package, and distinguishes two causes using **`leaf_mapping`, already on the row** — no new query needed. It comes from the same join chain as `leaf_function`, so a mapping with no name means the stack *was* unwound and only the symbol is missing (stripped `.so`s), while neither means nothing was unwound (not profileable). Different problems, different fixes. Live output on the bad trace: > ⚠️ **CPU sampling:** 296 CPU samples were captured for `com.android.settings`, but none carry a usable call stack, so no CPU hotspots could be computed and `profiler-stack-query` mode=`function_callers` / mode=`hang_stacks` will find nothing. The app was running — this is a capture limitation, not an idle app. mode=`thread_breakdown` still works, since it counts samples and needs no stacks. No stack was unwound at all: Perfetto only unwinds a process it can read `/proc/<pid>/mem` for. Profile a debug build, or add `<profileable android:shell="true"/>` to `<application>` and rebuild — verify with `adb shell dumpsys package com.android.settings`, which shows `DEBUGGABLE` for a profileable target. **Not routed through `exportErrors`.** That looked free — it renders above the Summary with no plumbing — but `renderAllClear` counts its entries as failed queries and would print *"1 of 3 queries errored"* about three queries that all succeeded. It goes through a `cpuDiagnostic` note instead, alongside the existing `freshnessNote`, and `status` stays `ok`. **Also fixed:** the footer no longer says "showing top 0 CPU hotspots" — the literal artifact in the report title. ## What I deliberately did NOT touch The existing no-samples-at-all manifest hint stays gated on `hangRows.length === 0`. That gate is intentional and pinned by `manifest-hint.test.ts:52` ("does NOT emit a manifest hint when hangs were found but CPU was empty") — once hangs come back the app clearly *was* traced, so blaming the manifest is wrong advice. It also wouldn't have helped: in the reported case `cpuRows.length` is **9, not 0**, so state A is never reached. Ungating it would have fired the hint on ~6 more existing fixtures and flipped their `status`, for no benefit. I only corrected its text, which claimed "Perfetto silently drops the linux.perf data source" — this issue proves a non-profileable app still yields samples. ## Verified live, both directions | trace | CPU warning | CPU section | footer | status | |---|---|---|---|---| | stackless (`com.android.settings`) | ✅ shown | absent | `showing top 3 hangs` | `ok`, `exportErrors: {}` | | healthy (debuggable app) | ✅ absent | present | `showing top 2 CPU hotspots and top 3 hangs` | `ok` | ## Deliberately deferred (filed separately) - `thread_breakdown` / `function_callers` notes — the issue's "and ideally" half. Needs a new `thread-breakdown.sql` aggregate plus fixture updates in two test files. - A partially-stackless trace renders percentages that **sum to 100%** while describing only the surviving fraction — arguably more misleading than the empty case here, and a distinct defect. - Repo docs assert the behaviour this PR only now implements. ## Checks - 3093 tests pass; 7 new. **All 200 existing android-perfetto tests pass unmodified**, including the manifest-hint gate. - `tool-description-quality.yml` doesn't run (path-filtered to `src/tools/**`); prettier, eslint, both typechecks clean; lock untouched. |
||
|
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. |
||
|
Ignacy Łątka
|
0a6dbec87c |
feat(telemetry): export events via OpenTelemetry OTLP logs instead of PostHog (#570)
## What
Swaps the `posthog-node` transport for an OpenTelemetry Logs exporter.
Everything above the transport stays as it is - opt-out consent,
per-machine identity and fingerprint, the sanitizer allowlist, the typed
event surface, crash diagnostics. Only the ~3-method client boundary
changes.
One analytics event becomes one OTLP/HTTP log record:
| PostHog | OpenTelemetry |
| --- | --- |
| `capture({ event, distinctId, properties })` | `logger.emit({ body:
event, attributes: { distinct_id, "event.name", ...props } })` |
| event name | log-record `body` |
| `distinctId` | `distinct_id` attribute |
| event/base properties | log-record attributes |
| `client.shutdown()` | `LoggerProvider.shutdown()` (force-flush +
teardown) |
Records go to a hard-coded Software Mansion collector. The endpoint and
the authorization header are passed to the exporter explicitly in code,
and an explicit value beats the corresponding `OTEL_EXPORTER_OTLP_*`
variable - the same anti-exfiltration property the fixed PostHog host
had.
## Changes
- `packages/telemetry/src/posthog.ts` becomes `otel.ts`:
`LoggerProvider` + `BatchLogRecordProcessor` + `OTLPLogExporter`
(HTTP/JSON). Queue size 20 and a 10 s flush interval mirror the old
client; the per-export deadline is 1.5 s, kept at or below `index.ts`'s
drain budget so a stalled export can't hold a short-lived command open.
- Dependencies: drop `posthog-node`, add
`@opentelemetry/{api,api-logs,sdk-logs,exporter-logs-otlp-http,resources}`.
They inline cleanly into the esbuild bundles.
- Build-time ingest token via esbuild `define`
(`ARGENT_OTEL_INGEST_TOKEN`), same mechanism as `ARGENT_CLI_VERSION`.
Unset in a dev build leaves the client unconstructed and telemetry
inert.
- Base props: drop `$process_person_profile` (a PostHog directive,
meaningless to a collector) and rename `$session_id` to `session_id`.
Null-valued attributes are dropped, since OTel rejects them -
`cloud_agent` is null on the common path, and absence means the same
thing.
- `Telemetry.md`: the "who we disclose to" and "international transfers"
sections describe a Software Mansion-operated OTLP collector instead of
PostHog as a third-party processor.
## A hang this uncovered
`timeoutMillis` only bounds a request once its socket is connected,
because the exporter applies it with `req.setTimeout()`. Against a
collector address that drops packets rather than refusing them - a
corporate egress filter, a dead host behind a firewall - the socket sits
in the connecting state where no export deadline reaches it, and the
process stays alive for the OS connect timeout: 75 s past a `shutdown()`
that had already resolved at 1.75 s.
Pairing the export deadline with an agent-level socket timeout, armed at
socket creation so it covers connection establishment too, fixes it:
| collector state | before | after |
| --- | --- | --- |
| healthy | 0.18 s, delivered | 0.18 s, delivered |
| connection refused | 0.94 s | 1.03 s |
| blackholed (http and https) | 75.0 s | 1.6 s |
| connected but never responds | - | 1.6 s |
`posthog-node` exited in 10.6 s under the same blackhole (undici aborts
a connecting socket), so this was a regression as well as a hang. A unit
test that fails without the fix guards it.
## Found in review
Each of these was reproduced before it was changed, and each is pinned
by a test that fails without the fix.
- **The environment's OTLP headers were forwarded to the collector.**
The SDK merges `OTEL_EXPORTER_OTLP_HEADERS` / `_LOGS_HEADERS` into every
request, keeping any key the code does not set itself. Out in the world
that variable holds the developer's *own* observability credential - a
Honeycomb team key, a Dynatrace `Api-Token`, Grafana Cloud basic auth -
so any machine already running OpenTelemetry shipped that third-party
secret here on every batch, without it passing the sanitizer or
appearing in `Telemetry.md`. Reproduced by watching `x-honeycomb-team:
hcaik_…` arrive at a capture server. The variables are now cleared
across the exporter constructor (where the SDK resolves its header set)
and restored after.
- **`httpAgentOptions` silently disabled connection reuse.** Supplying
it replaces the agent the SDK would build, whose default is `keepAlive:
true`. Measured on loopback: three batches shared one socket by default,
and opened three sockets with `httpAgentOptions` - a fresh TCP+TLS
handshake per 10 s batch in the long-lived tool-server. `keepAlive` is
restated alongside the timeout.
- **`markDisabled()`'s drain had no grace period.** It raced flat
against `SHORT_FLUSH_TIMEOUT_MS` while `shutdown()` allowed `timeoutMs +
250`. Since the exporter's own deadline equals that budget, the flat
race fired at the same instant and always abandoned the batch instead of
letting it finish. Both call sites share one `raceDrain()` now.
- **Export failures reached nobody.** The batch processor hands errors
to OpenTelemetry's global handler, so `LoggerProvider.shutdown()`
resolves identically for a delivered batch and a rejected token - making
`shutdown()`'s `catch` unreachable. The SDK's diagnostics are now routed
into the existing debug channel under `ARGENT_TELEMETRY_DEBUG=1`; a run
without the flag stays silent so no host application's diag logger is
disturbed.
- **The build-time token now beats the `__ARGENT_OTEL_TOKEN_TEST` seam**
rather than losing to it, so the seam reaches unbundled source only.
Verified with esbuild across all three build modes.
- **The endpoint guard test mocked the exporter it guards.** It could
only show that the code passes a `url`; an SDK upgrade that changed the
precedence would have kept it green. `otel-endpoint-live.test.ts` drives
the real exporter against loopback servers - dropping the explicit
`url`, the explicit `authorization`, or the header clearing each fails
it.
- **The wire shape had no pins at all.** Bypassing the sanitizer
entirely, sending an explicit `null`, renaming `service.name` and
dropping `event.name` all left the suite green. Four tests now fail on
those mutations. The sanitizer bypass was unpinned on `main` too.
## Before this ships
1. ~~**Collector URL**~~ - done in
|
||
|
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
|
||
|
filip131311
|
aa9ee0a5d6 |
chore: bump version to 0.21.0 (#810)
Bumps the monorepo from 0.20.0 to 0.21.0 across the full 18-file set: - all 16 `packages/*/package.json` (via `npm version --workspaces`) - `package-lock.json` regenerated with `npm install --package-lock-only --ignore-scripts` on Node 24 / npm 11 (version-only diff, matching the lockfile CI job) - `server.json` — both the top-level `version` and `packages[0].version` `npm run check:versions` passes: all workspace packages and server.json are at 0.21.0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
Kacper Kapuściak
|
f8d58489d0 |
feat(shake): wobble the host window behind a microinteractions flag (#803)
`shake` only touches the guest, so nothing on screen explains why an app popped its dev menu. This wobbles the host window carrying that device while the gesture runs. - Opt-in `microinteractions` flag, off by default (`argent enable microinteractions`). Read inside the handlers, not as `ToolDefinition.featureFlag`, which would hide the whole tool. - Can never fail a shake: every path warns once to stderr and resolves. Skipped off macOS, on headless boots, and for `sim-remote`. - Windows are title-matched on the iOS display name, so a host with several booted simulators wobbles the right one. - `prepareHostWindowShake` resolves flag/platform/device-set/script once per call; `begin()` per gesture, `settle()` once after the loop. Not in the `finally`, so a real failure throws immediately rather than waiting on cosmetics. - `ARGENT_SIMULATOR_NO_WINDOW` / `ARGENT_EMULATOR_NO_WINDOW` parsing extracted to `utils/no-window-env`, shared with `boot-device`. ### iOS simulators only The wobble originally covered Android emulators too and has been removed there. Picking the emulator's window meant matching a title among several `qemu-system-*` host processes, using needles built from the console port plus an extra `adb emu avd name` round trip per shake. That lookup was the fragile half of the feature: a build that drops the port from the title, or a serial with no console port, falls back to either the wrong window or no animation at all. An Android shake also already produces visible in-guest feedback, so the amplification bought less there. `shake` itself is unchanged on Android: the same accelerometer burst, the same resting-vector capture and restore, the same per-device lock. Only the cosmetic host-window animation is gone. `ARGENT_EMULATOR_NO_WINDOW` handling stays in `utils/no-window-env`, where `boot-device` still uses it. Verified live on an iPhone 17 Pro Max: window returns to its exact origin and the wobble fits inside one gesture. The Android sensor path was verified live on a Pixel_9 before the removal and is unchanged by it; it has not been re-run since. Full tool-server suite green. The AppleScript motion itself is only verified by eye; unit tests stub `osascript`. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Hubert Gancarczyk
|
20b5e8b4f0 |
fix(tool-server): reject a keyboard call carrying both text and key (#579)
## Problem
`keyboard` accepted `text` and `key` in one call, and that shape has no
meaning a caller can rely on. The same request reads two ways:
- `{ text: "hello", key: "enter" }` — "type, then submit"
- `{ text: "abc", key: "backspace" }` — "delete, then type" reads just
as naturally
Whichever order a backend picks, the other reading silently corrupts the
field. The backends did not even agree: `typeAndroidPhone` pressed
`params.key` **before** `params.text`, while simulator-server, chromium
and vega all did the reverse and the tool's own description promised
"the text is typed first and the key is pressed after it".
## Change
**`keyboard` takes `text` or `key`, never both.** A combined call is
rejected in `execute`, above the platform dispatch, so it resolves no
`ARGENT_SECRET_*` value and reaches no device — no backend has to defend
against the shape.
**To mix text and a key, use `run-sequence`.** That is what the error
message, both parameter descriptions, the tool description and the skill
docs now say:
```json
{ "udid": "<UDID>", "steps": [
{ "tool": "keyboard", "args": { "text": "hello world" } },
{ "tool": "keyboard", "args": { "key": "enter" } }
]}
```
Two separate calls work too, but cost an extra round-trip — and are
**not** equivalent when the text carries a secret (see below).
### Why the rule is prose, not JSON Schema
An earlier revision hand-wrote `inputSchema` with `not: { required:
["text", "key"] }`. That is no longer legal: #782 banned every top-level
combinator repo-wide, `not` included, because the Messages API rejects a
request whose tool schemas declare one — and that 400 fails **every tool
in the request**, not just this one.
`tool-input-schema-contract.test.ts` enforces it across the catalog.
So the constraint is restated in **both** fields' `.describe()` (the
`boot-device` house rule) and in the tool description.
`keyboard-text-key-exclusive.test.ts` pins both texts, and pins that the
advertised schema stays combinator-free.
### What the error message carries
Three things a caller needs and cannot get elsewhere:
1. **The literal retry**, not an ellipsis the Android backend cannot
type.
2. **The TV caveat.** A TV target rejects `key` outright
(`platforms/tv.ts`), so the prescribed `{ key: "enter" }` step cannot
succeed there. The guard runs above the dispatch on purpose, so it would
otherwise swallow that diagnosis; the caveat is carried statically
rather than by an async TV probe.
3. **The secret steer**, only when `text` holds a `{{secret:…}}`
placeholder. The auto-screenshot skip is decided per tool call from the
whole request, so:
| call shape | auto-screenshot |
| --- | --- |
| one `run-sequence` carrying both steps | skipped |
| `{ text: "{{secret:…}}" }` alone | skipped |
| a separate `{ key: "enter" }` | **not** skipped — captured after the
key lands, and can hand the still-visible secret back as pixels |
The check is the syntactic `.includes` `flow-utils.ts` already uses, so
the guard still resolves nothing. A plain combined call does not carry
the sentence, with a positive control pinning that.
### Simplifications the rule makes possible
With at most one of the two ever reaching a backend, the
resolve-then-inject splits that existed only to order a combined call
fold back into their injectors (`android-input.ts`, `vega-input.ts`),
and each backend collapses to two exclusive branches.
### Fixed alongside
`run-sequence`'s description claimed "No screenshot is captured
automatically". `shouldAutoScreenshot("run-sequence")` is `true` with a
15000 ms settle cap, so that line was already false — and this change
makes it load-bearing, since the secret guidance rests on it.
## Compatibility
A stored flow or scripted `run-sequence` whose step is `{ tool:
keyboard, args: { text: "…", key: "enter" } }` now errors on replay, and
the steps after it are skipped. The recorder never emitted the combined
shape (`flow-add-step` executes the call before it appends the step), so
the blast radius is hand-authored flows and scripted `run-sequence`
calls — which is exactly what the old worked example taught. The fix is
to split the step in two.
## Test coverage
- **`keyboard-text-key-exclusive.test.ts`** (new, replaces
`keyboard-key-order.test.ts`) — the rejection on all four backends with
nothing injected and no service resolved, plus a positive control per
backend that text alone and a key alone still work; shape-not-truthiness
(`{ text: "", key: "enter" }` rejects); at-most-one rather than
exactly-one (`{}` stays a no-op); rejection before secret resolution;
the run-sequence retry, TV and secret sentences, with a positive control
against an unconditional secret steer; and the two channels the
constraint actually reaches a client through.
- **`keyboard-backend-fidelity.test.ts`** — absorbs what key-order
uniquely pinned. The unknown-key 400 now names the offending key on iOS
and chromium here, on android in `keyboard-android.test.ts`, and on vega
in `vega-injection.test.ts` (tightened from the bare `/Unknown Vega
key/` prefix), so stripping the name is red on every backend.
- **`keyboard-android.test.ts`** — the combined-call tests are gone.
What they uniquely covered survives as single-parameter tests: the
multi-segment `%` codepoint count, the pressed key's identity against a
second literal keycode, and un-typeable text rejected with no on-device
side effect. The exhaustive 24-keycode loop now runs through
`injectAndroidNamedKey` and asserts the emitted `input keyevent <code>`,
so it pins the wire and not just the lookup.
- **`interaction-messages.test.ts`** — the two formatters see different
shapes now. `startedMsg` renders before the rejection, so it still words
four (including the combined one); `completedMsg` runs only after
success, so it words three. Both empty-request branches stay pinned.
- **`keyboard-tv.test.ts`** — the TV `key` rejection is independent of
this guard, which only ever sees requests carrying both. Its header says
so, since the exclusivity message's TV clause depends on it.
- **`scripts/e2e-full/phases/30-android.sh`** — the combined call
becomes an `assert_reject`, and the prescribed `run-sequence` remedy is
driven end to end.
## Live verification
Isolated tool-server built from this branch (own port, shared MCP server
untouched), against the Android settings search field on an API 36 AVD,
read back out of the view hierarchy rather than off a screenshot:
| Call | HTTP | Field / result |
| --- | --- | --- |
| seed `{ text: "XYZ" }` | 200 | `XYZ` |
| `{ text: "abc", key: "backspace" }` | **400** | field still `XYZ` —
byte-identical, nothing typed |
| `run-sequence` [`{text:"abc"}`, `{key:"backspace"}`] | 200,
`completed: 2` | `XYZab` — exactly what the combined call used to
produce |
| `{ text: "hello" }` | 200 | `{ typed: "hello", keys: 5 }` |
| `{ key: "enter" }` | 200 | `{ typed: "enter", keys: 1 }` |
| `{}` | 200 | `{ typed: "", keys: 0 }` — still a no-op |
| `{ text: "", key: "enter" }` | **400** | shape, not truthiness |
| `{ text: "{{secret:APP_PASSWORD}}", key: "enter" }` | **400** |
message carries the ONE-`run-sequence` steer and the
still-visible-secret reason |
| `{ text: "hi", key: "enter" }` | **400** | message does **not** carry
the secret sentence |
`GET /tools` reports `["properties", "required", "type"]` for `keyboard`
— no top-level combinator.
Suite: `packages/tool-server` 336 files / 4041 passed,
`packages/registry` 90 passed, `packages/argent-mcp` 79 passed. `tsc
--build`, `tsc --noEmit -p tsconfig.test.json`, eslint (0 warnings),
prettier and knip all clean.
---------
Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com>
|
||
|
Ignacy Łątka
|
123cc6abe1 |
fix(screenshot-diff): honor an outputDir that does not exist yet on the host (#558)
## Problem
Passing `outputDir` to `screenshot-diff` silently did nothing when the
directory didn't exist yet. It was never created, the diff images went
to a temp dir instead, and nothing in the result said the requested
location had been discarded.
Found during an end-to-end pass over the 0.17.0 release: the diff itself
was correct, but the requested `outputDir` was never created and no diff
PNG existed anywhere under it.
## Cause
`outputDir` is declared as a `kind: "probe"` file input, and
`probeHostPath` for that kind is just `stat(path)`. It answers "does
this path already exist on the tool-server host", nothing more.
`resolveOutputDir` read `presentOnHost: false` as "this is a remote
client's directory":
```ts
if (params.outputDir && (probe === undefined || probe.presentOnHost)) {
return params.outputDir;
}
// ...otherwise a temp dir
```
But a local agent naming a fresh output directory produces exactly the
same `presentOnHost: false`, so it took the remote fallback too.
## Fix
When the probe says the path is absent, try to create it
non-recursively. That succeeds only when the parent already exists on
this host - true for a local agent picking a new subfolder, false for a
remote client's unrelated directory tree - so the remote fallback is
preserved. `EEXIST` (raced into existence between probe and create)
counts as ours.
It also makes the tool self-consistent: `captureLiveInput` already did
`fs.mkdir(params.outputDir, { recursive: true })` when writing
live-capture intermediates into that same directory.
## Tests
- `creates and honors an outputDir that does not exist yet on this host`
- asserts the diff artifact's `hostPath` is inside the requested
directory and that the file is really there. Fails on main, where the
artifact lands in a temp dir.
- `falls back to a temp dir when outputDir is not creatable on this
host` - parent missing, i.e. the remote case. Passes before and after,
pinning the behavior that has to survive.
## Verification
- `vitest run` in `packages/tool-server`: 288 files, 2916 passed, 1
skipped.
- `tsc --noEmit -p tsconfig.test.json`, `tsc --build` across the
workspace, and `prettier --check` on both changed files: clean.
The comment on the `fileInputs` block still described the old fallback,
so it's updated in the same commit.
|
||
|
filip131311
|
9e45b98135 |
fix(profiler): measure CPU windows from the samples, not from the question (#685)
Fixes #619. ## Reproduced, then verified, on the reporter's own session Their artifacts were still on disk (`react-profiler-20260731-150333`: 1873 samples, 25114.0 ms, one hot commit at t=12307.5 ms), so every number below is from the session in the issue rather than a fixture. ## Two defects **Self-time was the question, not the answer.** `avgIntervalMs = (endMs - startMs) / totalSamples`, so self-time = *requested window width* × share of hits. Same samples, three windows, three answers: ``` [0, 25114] → createAnimatedNode 29.12 [0, 50000] → 2× [0, 100000] → 4× ``` **Samples were displaced by the first hot commit.** `clockOffsetMs = firstCommitTimestampMs - cpuStartMs`, applied when `|diff| > 1000`. `startTime` is a since-boot monotonic value (1_276_275_277_894 µs ≈ 14.8 days), so that guard is *always* satisfied and the offset was *always* the first hot commit — 12307.5 ms here, which is exactly how far every sample moved. **Together they predict the reported 2×**: displaced samples covered `25114.0 − 12307.5 = 12806.5` ms of a full-range query, so `25114.0 / 12806.5 = 1.961`. The reporter measured `completeRoot` 52.87 vs 26.82 — a ratio of 1.971. ## The fix **Clock: subtract, don't infer.** React DevTools emits every commit as `performance.now() - profilingStartTime`, so commit timestamps are already ms-since-profiling-start. Sample times are now the same thing — `(accumulatedUs - startTime) / 1000` — and `buildCpuSampleIndex` no longer accepts a commit timestamp at all, so the inference cannot come back. **Weights: each sample's own interval, clipped to the window.** A sample stands for the interval that *ended* at it, so a window cutting through one counts only the part inside. That gives four properties the old formula had none of: independent of the requested bounds, additive across adjoining windows, bounded by the window (a 45 ms commit can no longer be credited with more than 45 ms), and summing to the profile's own duration. Per-sample rather than averaged because the sampler is not isochronous — measured on the real profile, deltas run 0–36.6 ms around a 13.1 ms median. An average smears a stall evenly across every function in the window. *(A note on my own reasoning: I initially justified per-sample weighting with a synthetic fixture showing a 20× cost difference reported as a tie. That demonstrates the mechanism but overstates it — real Hermes sampling is roughly periodic, so the effect is a redistribution, not a rank inversion. The durable arguments are window-invariance and additivity.)* **Finding nothing now says which kind of nothing.** Outside the recorded range; inside it but unreached by any sample; a profile with no samples at all; and — the case that actually dominates — a window **fully covered by samples that were all idle**. 99% of samples in this session were idle, so a window can be completely covered and still rank nothing. Reporting that as "no CPU hotspots found" is what made a miss read as "this commit was cheap". Every table now states what it measured: sample count, covered ms, idle ms, and the sampling interval — plus a warning when the window is narrower than ~3 intervals, or when a sampler stall lands wholly on one function. **The on-disk index is versioned.** A file written before this change holds displaced timestamps and no interval starts; reusing one would answer from cache with exactly the numbers being removed. Readers reject it and rebuild from the raw profile, which is always retained. The same validation stops a truncated index deserializing into an empty profile that would answer "no hotspots" forever (`new Float64Array(undefined)` is silently empty). ## The reporter's five queries, after ``` 12300–12345 → recordProfilingDurations 15.05ms, completeRoot 4.86ms (was: nothing) 10000–15000 → 371 samples, real hotspots (was: nothing) 0–1000 → 71 samples, 1000.0ms covered, ALL IDLE — stated (was: nothing) 24000–25114 → 85 samples, 1113.1ms covered, ALL IDLE — stated (was: touch events from t≈12s) 0–25114 → 1872 samples, 25113.1ms covered, self-times correct (was: 2×) ``` Two remain empty — correctly, and they now say why. ## Tests 15 new cases in `test/react-profiler/cpu-correlate.test.ts`, all pure. There was **no test at all** for either file before, which is why this survived. Covered: window invariance across four widths, sub-window partitioning, boundedness by window width, additivity, per-interval weighting, rebasing a since-boot start time, coverage within one interval of the profile's reported duration, defensive deltas (missing/negative/NaN), the four empty-window shapes, and index round-trip plus v1/truncated rejection. Mutation-verified: restoring the window-derived average fails 8 of 15. Full tool-server suite green — 3101 passed / 298 files. ## Behaviour change — worth calling out in the release notes Numbers move in both directions. Windowed self-times fall by the ratio of the window to its sampled coverage; previously-empty windows now return data; and per-commit CPU blocks in **every** analyze report change wholesale, because they were reading a region of the profile the commit never occupied. **Any before/after optimisation baseline recorded with the current build must be re-measured.** ## Not in scope - **`time-align.ts:50-58` has the same family of bug** — `buildReactAnchor` sets `monotonicStartMs` from the since-boot `cpuProfileStartTimeUs` while commit timestamps are profile-relative, so commits are projected ~device-uptime into the past and **no hang can ever correlate with a commit**. It is invisible to the existing test because that test passes `cpuProfilePath: null`. Filing separately: fixing it makes `buildReactAnchor`'s second parameter and a `readCpuProfile` call dead, so it is not the one-liner it looks like. - `queryCpuWindow` emits one row per *node id*, so a function reached via several call paths is listed repeatedly with its cost split. Real, and moves numbers again — separate issue. - `component_cpu` applies its top-50 cut per window *before* aggregating, truncating the long tail. - `renderCallTree` and `react-profiler-cpu-summary` still derive self-time their own ways; unifying them onto these weights is a follow-up. - Hermes ignores the requested `sample_interval_us` (100 µs requested, ~13.4 ms observed). Not argent's bug, but it is why no fix may derive an interval from the requested config. |
||
|
Jakub Piasecki
|
568660509c |
fix(flow): floor every settle at two read attempts (#780)
`settleTree` bounded its polling by a 3s window but never bounded the number of reads it took, and a tree read can fail by TIMING OUT. Every tree source's RPC timeout outlasts that window, the shortest being iOS's 5s, so the window fit exactly one such read: "every read in the window failed", the condition the throw reports as a tree-source outage, collapsed into "the first read was slow", erroring a tap/type/scroll step on one transient blip with no retry at all. The floor makes the deadline stop deciding how much evidence an outage verdict is drawn from. `await`/`assert` have never had this flaw, since `waitForCondition`'s final poll already guarantees a second attempt, so this is the same guarantee given to the directives that resolve through a settle. A run whose reads answer inside the window pays nothing: two reads are what a settle already costs there. A run whose reads outlast it pays a second read, and gets a settle it never actually got before, since past the deadline the old code returned its single tree having compared it against nothing. A wedged source costs two full reads rather than one, and a read is several RPCs deep with each hop on its own timeout, so that is tens of seconds on a step that used to fail in one read. The alternative is the step failing on evidence the window was too short to gather. |
||
|
Jakub Piasecki
|
8062da77a5 |
fix(flow): settle before gestures that resolve no selector (#779)
A selector target settles inside `waitForFrame`, but a target that resolves nothing skipped the settle altogether: a coordinate `tap`/`long-press` and a centre-anchored `pinch`/`rotate` dispatched into whatever motion was in flight, so they could land mid-fling, which is the exact race the settle exists to close. Every input directive now settles first, however it is addressed. The settle is best-effort here, unlike on the selector path: these gestures read no frame out of the tree, so an outage must not fail them, which would break the escape hatch coordinates exist to be (an element the tree cannot see). This is the allowance `snapshot` already makes before a capture that reads pixels rather than nodes. Best-effort is not free, though, and the run pays it per gesture. A tree the run can never read is not only the disconnect mid-run: `fetchFlowTree` serves no tree at all on `ios-remote`, and an app that cannot load the instrumentation fails every read off an in-memory list, an Apple system app being the case in point, which flows drive by coordinates for exactly that reason, so every step of such a flow is one of these gestures. Measured against the real fetch on an `ios-remote` device, a coordinate tap went from 0ms to 3022ms, once per gesture, for a settle that was never going to arrive. So a settle that spends its whole window without one successful read records the outage on the run, against the device that proved it, and a later gesture rethrows that instead of buying the same verdict again. Not a shorter budget: any threshold low enough to spare a source that serves no tree would also abandon the mid-navigation blip the settle's retry exists for. But the record is a prediction, not a proof of repetition. One window mints it and nothing re-tests it, and on iOS that window can be a single stalled read, since the deadline is only tested after a read returns and `ViewHierarchy.getFullHierarchy` sits on a 15s RPC tier behind a serial 5s `Application.getState` probe. Being wrong therefore costs a settle rather than a step, and it is not silent: a gesture that spends the record warns its step report that it dispatched unsettled, quoting the source's own reason, so a run whose prediction was wrong says which of its greens mean the gesture was sent rather than that it landed. Every tree read a directive makes now goes through one helper so that a read which comes back clears the record whichever directive asked for it. `await`/`assert` and `idle` never settle, and a run that reads the tree successfully must not keep skipping. A relaunch clears it too, whether written as a `launch:` directive or as one of the raw `tool:` steps that perform the same operation, since a relaunch is the repair the commonest of these errors asks for by name. Two more reads route through the same helper for uniformity but can never be the clearing one: `rotate`'s aspect read skips itself while a record stands, and the `type` focus wait runs only behind a `waitForFrame` whose own settle already cleared it. What it costs, in two directions. An outage that lasted a full window, in a run that then never reads the tree again and never relaunches, leaves later gestures unsettled even once it clears: a flow of nothing but coordinate gestures giving up a best-effort settle it never had before this change, against every one of those gestures otherwise paying the window for a settle that is not coming. And on a screen that never holds still, a spinner, a video, a ticking clock, nothing converges, so every one of these gestures pays the whole window, measured at about 3.05s each. The record buys no relief there by design, because a window that read the tree proves no outage, and the fingerprint cannot be narrowed the way `scrollToVisible` narrows its own: a scroll knows the motion it waits on is its own, inside the containers under its anchor, while a gesture waits on motion of unknown origin that can move what sits under the point without touching it. It is the same window every selector directive already paid on such a screen; what changed is that coordinate targets pay it too. Two smaller pieces ride along. A centre-anchored `rotate` skips its screen-aspect read on a recorded outage as well, degrading to the same legacy orbit a failed read gives it, so the record actually takes the window off that directive instead of handing back a 15s-tier RPC. And the settle's tree read now takes the run's abort signal, so a cancelled run reports its skip immediately rather than once the source finally answers. It deliberately takes no time budget: one cut to what is left of the window would fail the cold start #778 raised that tier to ride out. |
||
|
filip131311
|
b1abdec13a |
feat(tools): add a shake tool for iOS simulators and Android emulators (#786)
Shaking a device was the one hardware gesture argent couldn't perform, putting iOS's "Undo Typing" prompt, React Native's dev menu, and app-side shake-to-report handlers out of reach. iOS needs no simulator-server work: Simulator.app drives Device > Shake by posting a plain Darwin notification, `com.apple.UIKit.SimulatorShake`, which UIKit turns into the same motion event a physical device raises. So the whole path is one `simctl spawn <udid> notifyutil -p <name>` — and the same argv runs on a remote simulator through `sim-remote spawn`, whose exit code must be read explicitly because it exits 0 even when the child fails. Android has no OS-level shake event at all; every shake feature is an app-side accelerometer detector, so the sensor is driven over the emulator console with a burst of hard direction changes. That override outlives the call, so the resting vector is captured up front and always restored. Simulators and emulators only. Physical phones have real accelerometers with no host-side hook, and TV targets have no shake gesture — but Apple TV and Android TV are not separate platforms, so the capability matrix cannot exclude them and each handler probes the runtime and rejects, pointing at `tv-remote`. Live testing on a packaged build found three Android defects, each now fixed and covered by a test that fails without the fix: - The resting-vector regex could not read scientific notation, which is exactly how a rotated emulator reports its near-zero axes. The read failed, a portrait fallback was adopted, and the restore then rotated a landscape device back to portrait while still reporting success. - Overlapping shakes raced: the second captured a mid-burst sample as its resting vector and, finishing last, left the accelerometer pinned at ±30 m/s² — a self-perpetuating state every later shake re-restored. Capture, burst and restore are now one critical section per device, and an implausible resting magnitude is rejected so a foreign tilt heals instead of being adopted. - `adb emu` exits 0 whether or not the console accepted the command, reporting a refusal only as a `KO:` line, so a rejected sensor write read as a successful shake. The reply is now read, taking the last non-empty line as the verdict. Verified live: iOS raises the undo alert (local and remote), the Android burst reaches the framework per `dumpsys sensorservice` with the resting pose restored, landscape survives a shake, four concurrent shakes serialize cleanly, and a refusing console fails loudly. |
||
|
Jakub Piasecki
|
abe5ff3c56 |
refactor(flow): generalize when's block machinery so a second block directive can't miss a site (#748)
`when` is the only directive with a sibling `steps:` list, and four runner sites asked `kind === "when"` directly to decide whether a step has authored children to expand as skip lines. A second block directive would have had to remember all four, and a forgotten one silently drops a whole block from the report — exactly the invariant `reportBlockSkipped` exists to hold. This introduces `blockSteps(step)` as the single runtime predicate and `BLOCK_DIRECTIVE_KEYS` as its parse-time counterpart, and extracts the parse-side block plumbing (`assertBlockDepth`, `parseBlockSteps`) that a second directive must share — notably the depth cap, renamed `MAX_BLOCK_DEPTH`, which has to be ONE counter across directives or an alternating nesting chain evades it and reopens the cyclic-YAML-alias hole it was added to close. No behavior change, with one deliberate exception: the depth-cap message now reads `block directives nest deeper than 20 levels` where it said `when blocks nest deeper than 20 levels` — the cap stopped being `when`'s the moment it became shared, and a message naming only `when` would misdirect an author who hit it through a different directive. Every other error message, their ordering, and the report shapes are untouched. 559 flow tests pass unchanged. --- **Stack** 1. **#748 — you are here** · `refactor(flow)`: generalize when's block machinery → `main` 2. #749 · `feat(flow)`: add `repeat:` blocks → #748 |