Commit Graph

15 Commits

Author SHA1 Message Date
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>
2026-08-24 12:16:20 +02:00
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 b9d17437
and 3eb59ef7. **This round supersedes several claims made above — they
are listed at the end.**

**The launch gate refused every `launch:` of an Apple system app.** A
coordinate-only flow gave
`["launch:error","tool:skip"]` with `resolveService` called 0 times —
the guard short-circuited
before any measurement — and the reason it emitted said *"Drive it by
coordinate instead"*, from
the gate that had just killed a purely coordinate-driven flow.
`argent-create-flow`'s worked
example (SKILL.md:171, :199-209) is exactly that shape.

The premise underneath it did not survive checking. #453, which
motivated `isInjectableBundleId`,
recorded `connected: false` for `com.apple.Preferences` on an iOS 26.5
sim; the review recorded
`connected: true`, with both dylibs mapped, on 18.5. So injection into a
simulator system app
appears to be runtime-dependent rather than categorically impossible,
and neither reading is
checkable from CI. The gate therefore no longer decides: it polls
exactly as before — where the app
does connect, nothing changes — and only on timeout does a
non-injectable id return "no failure"
instead of a reason. The launch step's job is to start the app; it
succeeded. **Whether
`isInjectableBundleId`'s blanket prefix rule is correct at all is now an
open question, and a
separate one from this PR.**

**The flow tree source could not report it either, because its
measurement was unreachable.**
`resolveNativeTargetApp(api, undefined)` draws candidates from
`listConnectedBundleIds()` — the same
map `appConnectionState` reads to answer `connected` — so a
non-connected app threw the stock
auto-target error first, whose "Launch or restart the app first" is the
loop this PR exists to
break. Driving the real factory: `appConnectionState === "unregistered"`
and
`listConnectedBundleIds() === []` at the same instant. The race between
the two is closed, not
narrow: no macrotask boundary separates them, while the socket `close`
handler is one. The branch's
own test passed only because its stub let the two accessors disagree.
The flow now threads the id
its `launch` step named — which does not come from that map — through
`ActionEnv` → `fetchFlowTree`
→ `queryFullHierarchyTree`; the unreachable branch is gone.

**The gate emitted `buildAppStateMessage` verbatim for every state but
`unregistered`.** Those are
written for the native-* tool surface, whose reader has not launched
anything, so a crash-on-launch
app was answered with "Call launch-app (or restart-app) then retry" —
the action the step had taken
9.5 s earlier. `flowLaunchGateReason` now rewrites each state for a
caller that has just launched,
exhaustively, so a state added later cannot inherit the tool-surface
remedy.

**`describe` without a `bundleId` — the default form — lost the whole
diagnosis.** Auto-targeting
throws before anything is measured and the outer catch returned the tree
bare, so
`await-ui-element`'s blind-read guard (which keys off exactly
`hint`/`should_restart`) let a
`hidden` wait release in 0 ms against an element still on screen, where
the same call *with* a
`bundleId` correctly refused. An empty read the hierarchy could not
corroborate now says so. A
device with **no** native-devtools service at all is deliberately left
unqualified — that is an
absence of a second opinion, not a failed attempt at one, and marking it
blind broke a real
`await-ui-element` case.

**Three of the six `native-*` descriptions named the terminal system-app
rejection and three did
not**, though all six throw the identical error (verified: one
byte-identical message across all
six). The clause is inlined verbatim in all six —
`scripts/extract-tools.mjs` accepts only a single
plain literal and silently drops anything else from the SpiderShield
scan.

**`not_running` cannot tell "stopped" from "not installed"** — both
simply lack a
`UIKitApplication:<id>` row — and for the second reading its only remedy
is refused outright by
`simctl launch`. It now names both readings instead of asserting the one
it cannot see. A real
probe was considered and not taken: `get_app_container` needs a booted
sim and ios-remote has no
such verb, so it would answer `undefined` on exactly the host where
`indeterminate` is already the
only reachable state.

### Claims above that this round supersedes

- *"Library validation means it can never connect, so the gate now says
that terminally."* — the
gate now lets system-app launches through; the premise is contested (see
above).
- *"The flow launch gate was the one caller that never asked whether the
app could be injected at
all"* and *"`flow-ios-tree` only ever auto-resolves a connected app"* —
both inverted here.
- *"Known and deliberately left"* — the `describe` no-`bundleId` path
was closed, with tests, so
  that paragraph now over-counts by one.
- The suite figures in **Verification** and **Review round 2** are
pre-round-3 counts.


---

## Review round 4 — reviewing round 3

Three lenses over 856efd8e's two parent commits (claims-vs-code,
non-happy
paths + reachability, absence + mutation). Round 3 did not survive
intact, and
what follows is what was wrong with it.

**Three statements were false in prose an agent acts on.** `connecting`
requires
a process inside the 3 s connect grace, while this gate takes its
verdict 9.5 s
after launching the app — so its remedy claimed the step had launched
the very
process it was measuring, next to a base message reading "it launched
moments
ago". The figure that arm and `not_running` quoted was the connect wait
alone,
omitting the 1.5 s post-launch settle that precedes it
(`LAUNCH_TO_VERDICT_MS`
now names the whole spend). And `stale_process` blamed the launchd
environment,
which is false for the producer reached only when
`processCarriesInjection` is
**true** — that process carries this endpoint and is merely older than
the
listener. The measured text it wraps names both producers, so the two
contradicted each other in one message.

**A `hidden` wait still released in 1 ms.** Round 3 carved out a failed
native-devtools resolution as "no service on this device at all". The
blueprint
is registered unconditionally, so on iOS that rejection only ever means
the
service failed to come up — a socket bind losing to a concurrent
same-udid
server, which this repo documents as the dominant crash source in 0.16.0
telemetry. Reproduced: `describe` → no hint, then `await-ui-element
hidden` →
`{"success":true,"elapsed":1}`. It is now as unexplained as its
siblings.

**The failure-code discrimination was defeatable.** The flow tree source
classified auto-targeting's throw by reading a code off a module-local
`Symbol`,
so a duplicate `@argent/registry` instance would read it as absent and
fall back
to the exact message the change replaces. It tests the empty connected
list
directly now — the same condition, structurally, with no symbol
involved. The
`connected` arm added in round 3 was unreachable for the same reason the
branch
it replaced was, and is gone.

**Coverage the mutation lens found missing.** Three of the four sites
threading
the launched bundle id were unpinned — including the `settleTree` read
every
`tap`/`type`/`scroll-to` uses, so an action step could have silently
lost the
terminal reason with the suite green. Also unpinned: `unexplainedHint`
dropping
a pre-existing degraded hint (where its exact twin fifty lines up **is**
pinned), the tree source's "Flows resolve selectors" half, and both of
the
gate's withholdings.

**Test cost.** The four per-state gate tests drove the whole runner in
pumped
fake time, ~9 s each, which is what pushed this file past vitest's
default under
parallel load. The mapping is a pure function and is tested as one; one
end-to-end test still proves the gate reaches it. 46 s → 16 s, with more
tests
than before. Widening the fake-timer step was tried and is worse (52 s)
— the
pumps are what let the run's disk I/O proceed.

**Also corrected:** comments the round-3 commits had falsified in the
very files
they edited; the `ActionEnv` claim that a fragment never carries a
launch step
(a flow is classified by its FIRST non-echo step, so a later launch is
legal and
does thread its id); `isBlindRead`'s enumeration of what sets its flags;
and
change narration that had leaked into permanent prose.
`argent-create-flow` now
states what a `com.apple.*` target costs a flow — its worked example is
selector-driven, and round 3's justification comment claimed the
opposite.

**Verification.** 17 mutations across both rounds, every one killed by a
specifically-named test. Two consecutive full runs of
`packages/tool-server`:
3181 passed, 1 skipped, and 6 failures in `boot-device-hotboot.test.ts`
that are
pre-existing on this host (it asserts `-gpu darwin`/`swiftshader` where
a Linux
box resolves `-gpu host`; the file is untouched by this PR). `tsc
--build`,
`tsc --noEmit -p tsconfig.test.json`, eslint and prettier clean on every
changed
file; `scripts/extract-tools.mjs` emits all 75 tools with zero warnings.


---

## Review round 5 — reviewing round 4

Round 4 left two of the defects it set out to fix and introduced one. It
also
broke CI.

**ESLint failed on the push.** Two `` \` `` escapes inside a
double-quoted string
(`no-useless-escape`). The pre-push check missed it because it was piped
through
`tail`, so the shell read the pipe's exit status rather than eslint's
and
reported success over a failure. Gates are run unpiped now, and
repo-wide —
`eslint .` is what CI runs, and a changed-files invocation covers a
subset of it.

**`unregistered` still quoted the connect wait alone** — the exact
understatement `LAUNCH_TO_VERDICT_MS` was introduced for, applied in
round 4 to
`not_running` only. And the end-to-end test asserted `8000 ms` while its
sibling
fifty lines down asserted the whole spend, so the file enforced two
contradictory rules and the *correct* figure would have failed CI. The
post-launch settle counts: the connection is read off the live map and
the poll
checks it once before its first sleep, so a dial landing inside those
1.5 s is
caught. All three arms that quote a figure now quote the same one,
pinned
together.

**`stale_process` named the launchd environment as fact** four lines
below a new
comment saying that is false for one of the state's two producers. It is
in fact
sound — but only where the message already scopes it, to a *second*
landing,
whose relaunch makes the process younger than any established listener
and so
rules that producer out (it needs `processAge + grace >= listenerAge`).
The
comment now says which producer and why. The test named
"does not blame the launchd environment" asserted only the phrase it
kept, so it
was green with the blame present; it now pins the ordering it is named
for.

**The `connected` arm is reachable — round 4's "unreachable" claim was
wrong.**
`appConnectionState` re-reads the live connections map *after* its env
re-apply
and process probe (`native-devtools.ts`), several simctl round-trips
past the
empty list that routed the call there, precisely so a dial landing in
that window
is not reported as unregistered. Folding the arm away left that case
throwing a
bare "Flows resolve selectors…" with no app named and no remedy. It is a
branch
again, and says the connection arrived mid-read.

**`unexplainedHint` dropped `screenshot` along with the `bundleId`
suggestion**,
so a caller that passed a bundle id received a hint with no next action
at all —
while `argent-device-interact` tells the agent to follow that hint.
`screenshot`
is unconditional now. Its JSDoc also claimed two of three call sites are
reached
only after an explicit id resolved and measured connected; all three are
reachable with an explicit id, as the file's own comment twenty lines
above says.

**Also:** `argent-create-flow` recommended `tool: gesture-tap` where the
same
file documents the first-class, equally tree-free `tap: { x, y }`; and
change
narration removed from four more comments.

### Standing caveats

- **The `com.apple.*` premise is still unsettled and is not settled by
this PR.**
  #453 recorded `connected: false` on iOS 26.5; the review recorded
`connected: true` with both dylibs mapped on 18.5. Every change here is
correct
under either reading, but `isInjectableBundleId` remains a blanket
prefix
  predicate and may be wrong on some runtimes. Worth its own issue.
- `boot-electron-spawn-error.test.ts` and `boot-device-hotboot.test.ts`
fail on
the Linux host used for this work; both were confirmed failing at the PR
base
  `b2e5c54d` and neither file is touched here.


---

## Review round 6 — reviewing round 5

Round 5's own headline fix was both broken and unpinned.

**The hint stopped parsing.** Moving `screenshot` out of the conditional
clause
without rejoining the sentence produced, on the default no-`bundleId`
form:

> … not evidence that nothing is on screen. Pass `bundleId` to have the
connection state measured, or Take a `screenshot` to see what is there.

That is the exact string `argent-device-interact` instructs an agent to
act on.
Both forms are one sentence again, and both are now pinned by a test
anchored to
the end of the string rather than matching a substring.

**And deleting the action entirely left the suite green.** The defect
round 5
describes — "a caller that passed a bundle id got a hint with no next
action at
all" — could be reintroduced with 635 tests passing. Nothing asserted
the hint
named a next step. It does now.

**Two claims round 5 corrected in the source survived verbatim in
tests.** The
"two of the three call sites are reached only after an explicit id
resolved and
measured connected" partition (all three are reachable with an explicit
id), and
"waited only NATIVE_READY_TIMEOUT_MS" — sitting ten lines above the
assertion
that now forbids that figure.

**A guard test did not test its own name.** `does not blame the launchd
environment for a stale process` pinned only *where* the blame sits.
Restating
it as fact on a first landing — `It lands here twice, so the …` —
passed. It
asserts the conditional now, which is the whole distinction: only a
repeat rules
out the producer that carries this endpoint.

**The `connected` branch still had no coverage** — which is how round 4
came to
delete it as dead in the first place. It is exercised now, including
that its
message still names the app and an action.

**Also:** the terminal system-app reason recommended `tool: gesture-tap`
where
the same round-5 fix in `argent-create-flow` had moved to the
first-class,
equally tree-free `tap: { x, y }`. The twin was missed.

**Verification.** Four mutations against these fixes, all killed:
dropping the
`screenshot` action; reintroducing the broken join; stating the launchd
blame as
fact; folding the `connected` branch away. Gates run unpiped and read by
exit
code — `eslint .` 0, `tsc --build` 0, `tsc --noEmit -p
tsconfig.test.json` 0,
`prettier --check .` 0.


---

## Review round 7 — reviewing round 6

**The gate asserted the contested fact as settled.** Round 3's
justification for
keeping the 8 s wait read "a system app's process inherits the injection
tokens
**and does connect on some runtimes**". Eight other places in this repo
say the
opposite — `blueprints/native-devtools.ts` ("our dylib can never load",
and the
agent-facing precheck throw), `native-devtools-status`, `describe`'s iOS
hint,
and `test/describe-tool.test.ts` ("a com.apple.* app never connects"), a
file
these commits edit. `flow-ios-tree.ts` contradicted *itself*, its
docblock
saying "can never load" above a message saying "cannot be relied on to
inject".

The fact is not settleable here, so nothing asserts it now. The comment
cites
both observations — #453's `connected: false` on iOS 26.5, the review's
`connected: true` with both dylibs mapped on 18.5 — and justifies the
wait
without picking: polling costs this step its timeout under the
pessimistic
reading and is the only thing that makes selectors work under the
optimistic
one, so waiting spends time on an open question where skipping would
bake an
answer into behaviour. The tree source's docblock is hedged to match the
message
it already had.

**This PR does not resolve that inconsistency**, and
`isInjectableBundleId`
remains a blanket `com.apple.` prefix predicate. It deserves its own
issue.

**A guard let the thing it guards be deleted.** Round 6 made the
repeat-landing
assertion conditional (`if (blame !== -1)`), so removing the whole
clause —
leaving a reader who lands there twice with "re-run the flow" and no
escape —
passed 36/36. Unconditional again, wording pinned.

**A rationale that was false in both directions.** "Keeps the
`not.toContain`
guards non-degenerate: they would pass vacuously if the constants were
made
equal" — equal constants make them *fail*, and 500 vs 1500 makes them
fail for
the wrong reason (`"1500 ms".includes("500 ms")`). It pins the substring
property those guards actually need.

**Two more presence-only guards.** "keeps the ax-degraded boot guidance
**ahead
of** the unreadable-hierarchy note" asserted only that both strings
appear, and
passed with them swapped. And round 6's own `tap: { x, y }` twin fix had
no test
at all — the same absence that got the `connected` branch deleted as
dead two
rounds earlier. Both pinned, and the `tap: { x, y }` claim was verified
end-to-end: that step form parses, resolves through `resolveTargetPoint`
with no
tree read, and passes in a flow whose selector step fails with this very
message.

**Two test names promised more than they checked** — a remedy "that is
not the
action the step just took", asserted for states whose remedy is another
relaunch; and a crash-on-launch guard forbidding only the literal `Call
launch-app` while the arm legitimately suggests launch-app for watching
the exit.

**Verification.** Three mutations, all killed: deleting the
repeat-landing
clause, reverting the coordinate form to `tool: gesture-tap`, and
putting the
read-note ahead of the boot guidance. Gates read by exit code, unpiped:
`eslint .` 0, `tsc --build` 0, `tsc --noEmit -p tsconfig.test.json` 0,
`prettier --check .` 0. Suite: 3187 passed, 1 skipped, and the 6
pre-existing
`boot-device-hotboot` failures on this Linux host.


---

## Review round 8 — reviewing round 7

**The gate's own justification was false, and the disproof was already
in this
PR's review.** Round 7's comment claimed polling "is the only thing that
makes
selectors work at all" under the optimistic reading. It is not:
`settleTree`
retries a failed tree read for `SETTLE_TIMEOUT_MS` on top of
`POST_LAUNCH_SETTLE_MS`, so roughly the first 4.5 s of connect latency
is
absorbed with no gate at all — which is exactly why the review's
fragment (no
`launch` step, therefore no gate) resolved selectors 4/4 against
`com.apple.Preferences` on 18.5. Confirmed with an in-repo
counterfactual:
at 2000 ms connect latency the flow passes with or without the gate; at
6000 ms
only with it. What the poll adds is the band from ~4.5 s out to
`LAUNCH_TO_VERDICT_MS`, and the comment now says that.

**Hedging one surface left its twins absolute — the same failure mode, a
fourth
time.** The precheck throw, 230 lines from the message this PR hedged
and in the
same file, still told agents Argent "can never be injected into it"; so
did the
predicate's docblock, `native-devtools-status`'s description ("the dylib
can
never load into them", "TERMINAL state — the app can never be
injected"), and
`describe`'s terminal hint. An agent reading `argent-create-flow` was
told
"depends on the simulator runtime" and the status tool told it "can
never".

Every agent-facing surface now says injection cannot be RELIED on, and a
test
pins that none of them claims impossibility — the six native-*
descriptions, the
status description, the shared recovery text, the device-interact skill,
and
(separately, since it is module-private and reachable only through the
tool's
output) `describe`'s terminal hint. Each surface is asserted non-empty
first, so
a missing one cannot satisfy the negative match vacuously.

**The decision is untouched.** `isInjectableBundleId` still answers "not
injectable", the precheck still throws, and the description still says
`Do NOT restart/retry` — pinned by the same test, because softening the
claim
must not soften the behaviour or #453's restart loop returns. An app
that MIGHT
connect cannot be the basis of a retry loop either, which is now the
predicate's
recorded reason.

**Also:** the round-7 reword left "The flow / flow path" duplicated;
withholding
the verdict before the measurement (promoted to its own paragraph last
round)
was asserted nowhere, so moving it after `appConnectionState` left the
suite
green — a test now proves the measurement is not taken for an app whose
verdict
is withheld either way; and two more test names were narrowed to what
they
actually check.

**Verification.** Mutations run against every decision these commits
added.
Killed: re-asserting "can never load" in the status description;
dropping its
terminal instruction; re-asserting impossibility in `describe`'s hint;
moving
the withholding after the measurement; and the earlier rounds' decisions
re-checked and still pinned. One survivor is unpinnable by construction
— a
claim inside a code comment, which no test can assert. Gates read by
exit code,
unpiped: `eslint .` 0, `tsc --build` 0, `tsc --noEmit -p
tsconfig.test.json` 0
(this one rejected a tuple vitest had accepted), `prettier --check .` 0.



---

## Review round 9 — scope: the skill docs

This is a state-derivation fix. It should not be rewriting skill docs,
and the
earlier rounds had it doing exactly that.

**`argent-create-flow` is back to its base.** Nothing in it was
falsified by this
change. The `com.apple.*` caveat rounds 4-6 grew there documents the
question
this PR explicitly declines to settle — whether `isInjectableBundleId`'s
blanket
prefix rule is right at all — and that belongs to its own issue, not to
a
paragraph in a skill an agent reads before recording a flow.

**`argent-device-interact` only got smaller.** It said *"call
`restart-app` first
if needed"* — the unconditional-restart advice the measured states exist
to
replace — and that clause is deleted. The four-status routing table the
first
round put in its place is gone with it: the remedy already travels on
the tool's
own output and on the six `native-*` descriptions, both pinned by tests,
and a
second copy in a doc no code reads is one more surface to drift.

Tests follow the prose. The two `SKILL.md` reads in
`native-devtools-status.test.ts`
go with the text they pinned, and two comments that cited the reverted
skill text
are corrected.

### Claims above that this round supersedes

- The first round's *"the `argent-device-interact` skill offered
`restart-app` as
the only remedy for a missing injection"* — the clause is deleted, not
replaced.
- Round 4's *"`argent-create-flow` now states what a `com.apple.*`
target costs a
flow"*, round 5's *"`argent-create-flow` recommended `tool:
gesture-tap`"*, and
round 6's twin fix in the skill — all in the reverted paragraph. The
matching
source-side fix in `flow-ios-tree.ts` stands; only its stated twin is
gone.
- Round 8's cross-surface certainty test no longer reads the skill. It
covers the
status description, the six `native-*` descriptions and the shared
recovery text
  — the surfaces that make the claim.

**`origin/main` merged in, and #519 needed one test to follow.** It
landed after
this branch last merged main and makes a `getFullHierarchy` reply with
no windows
an error — an untrustworthy read, not a blank screen. `does not measure
when
auto-targeting resolved an app` had been using an empty `windows` as a
don't-care
return, so the call now ended before it could observe anything. The stub
serves a
real window, which is the state the test names. Mutation-checked:
dropping the
empty-connected-list term from the guard makes the measurement fire and
fails
that test alone.

**Verification.** `packages/tool-server`: 304 files, 3242 passed, 1
skipped, on a
macOS host (the `boot-device-hotboot` failures noted in the standing
caveats are
Linux-only and do not reproduce here). Gates read by exit code, unpiped:
`eslint .` 0, `tsc --build` 0, `tsc --noEmit -p tsconfig.test.json` 0,
`prettier --check .` 0.
2026-08-17 21:06:26 +02:00
Juliusz Wajgelt 013d5b60c5 fix: get ax-service working on ios-remote (sim-remote --json spawn + TCP artifact diagnostics) (#536)
## Summary

Three fixes that together get `describe` (and the other ax-service /
native-devtools tools) working against **ios-remote** (`sim-remote`)
devices — including from a **Linux** host. Reproduced end-to-end against
a live remote iPhone 17 simulator over the QUIC relay.

Before: `argent run describe --udid remote:<UDID>` returned an empty
`ROOT AXGroup` with a misleading "reboot the simulator" hint.

### 1. `sim-remote spawn` output was never valid JSON
(`fix(tool-server)`)

`simctlSpawn` parses the CLI's stdout as one `{exit_code,stdout,stderr}`
(or `{pid}`) JSON object, but `sim-remote spawn` only emits that when
passed `--json`. Without it a **non-detached** spawn streams the child's
raw output live, so `JSON.parse` throws `Unexpected end of JSON input`.

This broke ios-remote describe at the first step:
`remoteIosHost.bootstrapAx` runs `defaults write com.apple.Accessibility
…` through a non-detached `simctlSpawn`, which failed before the
ax-service daemon ever started — surfacing only as an empty AX tree.
Fix: pass `--json` unconditionally (no-op for the already-JSON
`--detach` path).

### 2. The tcp ax-service was resolved/packaged under a host-platform
dir (`fix(native-devtools-ios)`)

The tcp `ax-service` is a **darwin/iOS-simulator** binary that is
uploaded to and `simctl spawn`d on the **remote** macOS orchestrator, so
it's needed on any *host* platform. But it was resolved (and packaged)
under `bin/<hostPlatform>/tcp` via `platformBinDir()`, so a **Linux host
looked at `bin/linux/tcp` and never found the darwin-built binary** —
the ios-remote path was effectively macOS-host-only. (The tcp dylibs
were already neutral at `dylibs/tcp`.)

Moved it to a platform-neutral `bin/tcp`, mirroring `dylibs/tcp`, and
rewired every producer/consumer:
- resolver: `platformTcpBinDir()` → `tcpBinDir()` = `BIN_DIR/tcp`
- `native-devtools-ios/scripts/build.sh`: `bin/darwin/tcp` → `bin/tcp`
- `scripts/download-native-binaries.sh`: `TCP_BIN_DIR` → `bin/tcp`
- `packages/argent/scripts/bundle-tools.cjs`: copy from/to `bin/tcp`
- tests assert the neutral location on both linux and darwin

### 3. Missing tcp artifacts were undiagnosable
(`fix(native-devtools-ios)`)

When a tcp artifact is absent (local/dev build, older package), the bare
`... not found` error was thrown deep in the ax-service factory, where
`describeIos` swallowed it and returned an empty tree plus
`DEGRADED_HINT` — telling the agent to `boot-device --force`, a dead
end. The only escape was discovering `ARGENT_SIMULATOR_SERVER_TCP_DIR`
by reading source. Fix: `requireTcpArtifact` throws an actionable error
naming the exact path, the override env var, and how to build it;
`describeIos` surfaces that instead of the reboot hint.

## Testing

- New `sim-remote-spawn-json.test.ts` — asserts `--json` on both plain
and `--bin/--detach` argv, before the `--` separator.
- `native-devtools-ios/test/index.test.ts` — the tcp binary resolves
from the neutral `bin/tcp` on **both** linux and darwin; the not-found
errors name their override env var.
- `native-devtools-ios` (16) + ios/describe tool-server suites pass;
typecheck, eslint, prettier clean.
- Manual end-to-end against `remote:<UDID>` (darwin host): binary at
`bin/tcp` → full tree from `source: "ax-service"`, no hint; binary
removed → actionable `ARGENT_SIMULATOR_SERVER_TCP_DIR` hint.
2026-07-22 08:47:52 +02:00
Juliusz Wajgelt b90970ffa8 ci: ship TCP-transport iOS binaries in the published package (#487)
## Why

argent's `ios-remote` (sim-remote) support drives **ax-service** and
**native-devtools** over a TCP socket, resolving them from
`bin/darwin/tcp/ax-service` and `dylibs/tcp/*.dylib`. `bundle-tools.cjs`
already stages those into the published package **when present**, but
`download-native-binaries.sh` never fetched them — so
`@swmansion/argent` 0.15.0 shipped with no TCP variant at all, and every
`ios-remote` `describe` / `native-*` call failed with "not found".

## What

- **`download-native-binaries.sh`** — fetch the TCP dylibs tarball into
`dylibs/tcp/` and `tcp-ax-service` into `bin/darwin/tcp/ax-service`
(best-effort: skipped with a warning on pre-sim-remote release tags),
with the same Mach-O platform + codesign verification applied to the
iOS/tvOS assets.
- **`publish.yml` / `publish-next.yml`** — hard `test -x`/`test -f`
gates for the four TCP files so a missing build fails the publish loudly
instead of silently regressing (mirrors the existing tvOS gates).
- **`package.json`** — add `build:ios-binaries:tcp` and wire it into
`pack:mcp:local` so local packs match CI.

`bundle-tools.cjs` needs no change — its `ax-service (tcp)` asset entry
and recursive `dylibs/` copy already handle the `tcp/` slot.

## Verified locally

- `npm pack --dry-run -w @swmansion/argent` now lists
`bin/darwin/tcp/ax-service` +
`dylibs/tcp/{libArgentInjectionBootstrap,libKeyboardPatch,libNativeDevtoolsIos}.dylib`.
- `package-manifest.test.ts` (download↔bundle-tools path agreement) and
`native-devtools-ios/index.test.ts` (TCP path resolution) pass.
- End-to-end against a real sim-remote device: with the TCP binaries in
place, `describe` returns the full ax tree and `native-describe-screen`
/ `native-find-views` / `native-full-hierarchy` all return `status: ok`.

## Paired PR

The producer side — the workflow that builds, signs, and uploads these
TCP release assets — is **software-mansion/argent-private#26**. Both
must land for a release to ship the TCP binaries.

## Known follow-up (out of scope here)

`axServiceBinaryPathTcp()` resolves
`bin/<hostPlatformKey()>/tcp/ax-service`, and `bundle-tools.cjs` stages
the TCP ax-service **darwin-only**. So `describe` via ios-remote works
from a macOS argent host (verified), but a **Linux** host would hit "not
found" at `bin/linux/tcp/`. The TCP dylibs are fine cross-host (flat
`dylibs/tcp/`, un-gated). Full Linux-host support means staging the
darwin TCP ax-service into each host's `tcp/` dir, modeled on the
per-platform `simulator-server` loop — left as a follow-up.
2026-07-10 12:42:47 +02:00
filip131311 d010ead67e fix(scripts): verify downloaded dylib Mach-O platforms (#450)
## What

Adds a platform-verification gate to
`scripts/download-native-binaries.sh`: after downloading the native
binaries, assert every arch slice of each injection dylib has the
expected Mach-O platform (iOS = `IOSSIMULATOR`, tvOS = `TVOSSIMULATOR`),
and fail loudly otherwise.

## Why

The script pulled the flat iOS `libArgentInjectionBootstrap.dylib`
release asset on trust. When a mis-built release ships a
**tvOS-platform** binary in that slot, `dyld` silently refuses the
platform-mismatched `DYLD_INSERT_LIBRARIES` insert, so native-devtools
never connects — every `native-*` tool returns `restart_required`, with
**no error** at download, sign, or pack time. That is exactly what
happened (the `argent-main` / `argent-v0.14.0` flat bootstrap asset was
`TVOSSIMULATOR`).

This guard runs on both paths that ingest the binaries — local
`pack:mcp` (`pack-mcp.cjs`) and the publish CI (`publish.yml` /
`publish-next.yml`) — so a bad upstream release can no longer be bundled
into an npm publish silently. `vtool` is macOS-only, so the check is
skipped on hosts without it (matching the existing `codesign` guard).

Complements the producer-side fix in `argent-private`
(`build-native-binaries`: build the tvOS dylibs outside `build/dylibs/`
so the flatten can't collide, plus a platform gate).

Verified: flags a `TVOSSIMULATOR` bootstrap, passes an `IOSSIMULATOR`
one.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
2026-07-01 19:31:15 +02:00
Mateusz Ścianek b8afbed183 feat(tv): Apple TV (tvOS) and Android TV support (#374)
Adds end-to-end support for driving **Apple TV (tvOS)** and **Android TV
(leanback)** targets through argent, using a focus-driven interaction
model (TVs have no touchscreen — interaction is moving a focus highlight
with the remote).

This PR was reworked after review to **reuse the existing cross-platform
tool surface and the `tv-remote` tool from the Vega/Fire TV work
(#366)** rather than ship a separate family of `tv-*` tools.

## What this delivers

TV targets are driven by the **same cross-platform tools as every other
platform**, routed through the existing `dispatchByPlatform` fork
mechanism — there are no dedicated
`tv-describe`/`tv-navigate`/`tv-type`/`tv-set-focus` tools:

- **`describe`** — on a `runtimeKind: "tv"` target, returns the
focus-driven view (focused + focusable elements) instead of a tap tree.
On Android TV it auto-falls-back to the full `uiautomator` tree when the
RN focus engine exposes nothing.
- **`tv-remote`** — the cross-platform remote/D-pad tool from #366, now
extended with `ios` (Apple TV HID daemon) and `android` (Android TV adb
keyevents) branches alongside the existing `vega` branch. Single button,
a `repeat`, or a whole path (`["up","right","select"]`) in one call.
- **`keyboard`** — types into the focused field on a TV target (named
keys are rejected there — they're navigation, which belongs to
`tv-remote`).
- **`button`** stays hardware-only (phones/tablets); it is not a TV
tool.

**Apple TV** runs two native daemons (in-sim AX service + host-side HID
daemon), shipped via the `argent-private` submodule. **Android TV**
reuses the same tool surface, adb-backed (`input keyevent`, `uiautomator
dump`, `input text`).

## Remote vocabulary parity

`tv-remote` exposes the full 16-button vocabulary
(`up`/`down`/`left`/`right`/`select`/`back`/`home`/`menu`/`playPause`/`rewind`/`fastForward`/`next`/`previous`/`volumeUp`/`volumeDown`/`mute`):

| | Apple TV | Android TV | Vega |
|---|---|---|---|
| D-pad / select / back / menu / home / playPause | ✅ | ✅ | ✅ |
| media-transport + volume/mute | ❌ rejected | ✅ | ✅ |

Media-transport/volume keys **genuinely work on Android TV** (real
keycodes — verified live: `volumeUp` moved `STREAM_MUSIC`, `mute`
toggled per `dumpsys audio`). On the **Apple TV simulator** they are
**rejected with a clear error**: on-device testing confirmed the tvOS
sim's HID stack silently drops Consumer-Control events, so returning
success would be a lie.

## Key design points
- **tvOS UDIDs are UUID-shaped and indistinguishable from iOS by
shape.** Handled with `runtimeKind` detection (tvOS runtime string for
Apple TV; `pm list features` leanback/television for Android TV — *not*
`ro.build.characteristics`, which lies on TV emulators).
- Converted launch-app / restart-app / screenshot / screenshot-diff /
run-sequence from **eager** to **lazy** service resolution, so a tvOS
target never spins up (and hangs on) the iOS-only simulator-server /
native-devtools blueprints.
- Native injection selects the platform-matched (TVOSSIMULATOR) dylib
slice; daemons recycle across sim reboots and app relaunches so
injection and focus survive.
- SKILL.md footprint minimized per review: the two TV skills were
collapsed into a single lean **`argent-tv-interact`** (~40 lines).

## Verification
- `npm run build`, ESLint, Prettier, and the full tool-server suite
(**1565 tests**) all pass; CI green (incl. the Apple TV / Android TV /
Vega e2e jobs).
- Core flows verified live against real apps on both an Apple TV 4K
simulator and a Google ATV emulator.

## Dependency / merge order
Pins the `argent-private` submodule to the tip of its `feat/tv-support`
branch (software-mansion/argent-private#20). **Merge that PR to `main`
first, then bump the submodule pointer here to the merged SHA before
merging this PR.**

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-01 14:59:37 +02:00
Paweł Fornagiel 3ccd89f333 feat: add native android profiling (#275)
# Android native profiling via in-process Perfetto WASM

Connected with:
https://github.com/software-mansion/argent-private/pull/16

## TL;DR

Adds **Android** to the native profiler (previously iOS-only) and does
it with a
brand-new engine: instead of shipping Perfetto's `trace_processor_shell`
native
binary and forking it per query, we now run **Perfetto's trace-processor
compiled
to WebAssembly, in-process** inside the tool-server.

- **One ~13 MB `trace_processor.wasm`** runs on every OS/arch — no
per-platform
  binary, no subprocess, fully offline.
- The native profiler is refactored from an iOS-only implementation into
a
**platform-dispatch facade** (`platforms/ios.ts` +
`platforms/android.ts`) over
a new **`profiler-shared/`** layer, so iOS and Android share the
aggregator,
  types, lifecycle, and time-alignment code.
- New **release/pack pipeline** fetches + sha256-verifies the WASM
bundle from
`argent-private-releases`, and bundles it (plus SQL queries and the
Perfetto
  TraceConfig) into the published `@swmansion/argent` package.

End-to-end on a 76 MB trace with 1013 jank rows: **6.3 s** total analyze
(the older per-hang-subprocess approach was projected at ~47 minutes and
never
finished within the tool-call deadline).

---

## TESTING MANUALLY

To test manually run the following packing command checked-out on this
branch:
```
npm run pack:mcp -- argent-@pFornagiel-native-android-devtools-perfetto
```

This will bundle the package with the new binaries. After this PR is
fixed and the binaries are resolved from the main release, the
`argent-@pFornagiel-native-android-devtools-perfetto` release can be
deleted.

## Why

Android needed native CPU/hang/memory profiling to reach parity with
iOS. The
natural engine is Perfetto's trace-processor, but the obvious ways to
ship it are
both bad:

- **`trace_processor_shell` binary** — per-platform (a mac-arm64 pack
shipped a
mac-arm64-only tarball), pinned an OS/arch matrix, and cost a fresh ~1.3
s
subprocess fork **per query**. With one subprocess forked per detected
hang, a
  large trace projected to ~47 minutes.
- **`get.perfetto.dev/trace_processor` Python launcher** — adds a
runtime Python 3
dependency, resolves "latest" at first run (defeats version pinning),
and makes
  the first analyze slow via a network fetch.

The WASM engine is one cross-platform artifact, in-process and
warm-cached: the
trace is parsed **once** and reused across the whole analyze run plus
every
drill-down query.

---

## The three big pieces

### 1. In-process Perfetto WASM engine

New package surface in `@argent/native-devtools-android`:

| File | Role |
| --- | --- |
| `src/wasm-trace-processor.ts` (+476) | The whole engine: boot, glue
patching, warm-cache, fault containment, chunked parse, cell decoding |
| `src/perfetto-engine.d.ts` (+49) | Hand-written types for the
version-agnostic RPC decoder (imported by path, so tsc can't see its own
`.d.ts`) |
| `src/errors.ts` (+68) | `TraceProcessorUnavailableError`
(`wasm_load_failed` / `wasm_path_invalid`) |
| `src/bundled-meta.ts` (+12) | `PERFETTO_VERSION` stamp, regenerated at
pack time |
| `src/index.ts` | Re-exports + package-root accessors
(`traceProcessorQueriesDir`, `traceConfigPath`) |

Public surface: `resolveTraceProcessorAssets()`,
`queryWarm<Row>(tracePath, sql)`,
`ensureTraceProcessorReady(tracePath)`, `disposeWarmEngine(tracePath)`.

Highlights of how it works:

- **Boots Google's prebuilt web/worker wasm under Node.** Installs a
minimal
worker scope (`self`, `WorkerGlobalScope`, `location`), reads the wasm
bytes,
patches the Emscripten glue (`useMemory64=false`, hand the bytes in
directly,
expose the bridge), and drives it over a `MessageChannel` + `EngineBase`
subclass. Glue patches are applied **by unique string anchors that throw
if they
  don't match** — the canary for glue-format drift on a Perfetto bump.
- **Warm-engine cache, one per trace path.** LRU with `MAX_WARM_ENGINES
= 3`
(bounds resident memory at ~26–76 MB trace + wasm heap each), 5-min idle
dispose
  (timer `unref()`'d), no failed-promise caching.
- **Fault containment (`fatal` promise).** `EngineBase.fail()` throws
synchronously
inside the `MessagePort.onmessage` handler (e.g. an RPC framing error);
left
unhandled it becomes a Node `uncaughtException` and would **kill the
entire
tool-server**, wiping every device + profiler session. Each engine
exposes a
`fatal: Promise<never>`; the handler try/catches, rejects `fatal`,
evicts the
engine, and every load/query is `Promise.race`'d against it — so a
contained
  fault is a clean rejection and the server stays up.
- **Chunked trace parse.** Perfetto's RPC ring buffer rejects any frame
> 64 MiB,
  so the trace is fed in 32 MiB chunks before `notifyEof()`.

### 2. Android native profiler + the shared/dispatch refactor

**Capture** (`utils/android-profiler/capture.ts`, `detect-app.ts`):
spawns
`adb shell perfetto --txt -c - --background-wait -o
/data/.../argent-<ts>.pftrace`,
piping the TraceConfig on **stdin** (SELinux denies `shell:s0` writes
under
`/data/misc/perfetto-traces/`, so the config can't be staged as a file),
waits for
the PID on stdout, then on stop sends `kill -TERM`, polls `/proc/<pid>`,
and
`adb pull`s the trace.

**Pipeline** (`utils/android-profiler/pipeline/`): PerfettoSQL does the
parsing and
aggregation, so the Android pipeline is intentionally **two files, not
iOS's four**.
The per-hang annotation pass is **batched** — `runBatchedHangFolds`
inlines all
hang windows as a `VALUES` table into one SQL script (two derived views
unioned in
a single terminal SELECT, because the engine returns only the final
statement's
rows) and runs it once against the warm engine.

**The platform-dispatch refactor** —
`native-profiler-start/stop/analyze` shrank
from monolithic iOS files (~160–330 lines each) into **thin routers**
(~60–110
lines) that branch on `api.platform`:

```
native-profiler-start ─┬─ ios?    → platforms/ios.ts      (xctrace)
                        └─ android → platforms/android.ts  (adb perfetto)
```

A new **`utils/profiler-shared/`** layer holds everything both backends
use:

| File | What was hoisted |
| --- | --- |
| `types.ts` | Unified `Bottleneck = CpuHotspot \| UiHang \| MemoryLeak
\| MemoryRssGrowth`, each now carrying a `platform` field; shared
`NativeProfilerAnalyzeResult`; `RECORDING_CAP_MS` |
| `aggregate.ts` | Generic `aggregateCpuHotspots(AggregatorInputRow[])`
— burst windowing (`BURST_GAP_MS=500`), severity banding (RED >15%,
YELLOW 3–15%, <3% dropped). iOS feeds it via a pre-pass; Android feeds
it precomputed SQL rows |
| `lifecycle.ts` | `shutdownChild` SIGINT→SIGTERM→SIGKILL ladder +
`waitForChildExit` (no PID-reuse hazard) |
| `thread.ts` | `normalizeThreadName` for both xctrace and Perfetto
thread names |
| `time-align.ts` | New `buildPerfettoAnchor()` + `windowsOverlap()`
alongside the existing iOS/React anchors |
| `format.ts` | `formatBytes` (moved out of the combined report) |

iOS was refactored to fit this shape: `ios-profiler/lifecycle.ts` and
`types.ts`
become thin re-exports; `pipeline/02-aggregate.ts` shrank from ~200 to
~60 lines
(keeps its iOS-only dominant-function pre-pass, delegates
grouping/bursting/
severity to the shared aggregator). **No iOS behavior change** — same
outputs,
shared internals.

**Session blueprint** (`native-profiler-session.ts`): generic
`capturePid` /
`captureProcess` (was `xctracePid` / `xctraceProcess`), new `platform`
and
`androidOnDeviceTracePath` fields, and **platform-branched dispose**
(iOS SIGKILLs
xctrace; Android `kill -KILL`s perfetto, `rm`s the device trace, and
disposes the
warm WASM engine).

**Drill-down tools**: `profiler-load.ts` learns to restore Android
`.pftrace`
sessions (with an optional metadata sidecar / `app_process` override);
`profiler-stack-query.ts` splits into iOS and Android branches. Unlike
iOS (which
caches `parsedData` in memory), Android **re-queries the `.pftrace`**
per
drill-down — the warm engine is the cache, so each query costs only SQL
execution
time.

### 3. Packaging & release pipeline

The WASM bundle's three third-party artifacts (`trace_processor.wasm`,
`engine_bundle.node.js`, `engine.mjs`) + `LICENSE` are **never
committed**
(`.gitignore` lists the generated files but not the dir). They're built
and
sha256-checksummed in `argent-private` CI, published to
`argent-private-releases`, and on our side:

- **`scripts/download-trace-processor.sh`** (+88, new) — `gh release
download` →
verify tarball sha256 (**FATAL on mismatch** — the blobs are unsigned) →
extract
  → re-verify via `SHA256SUMS` → assert all four present.
- **`scripts/pack-mcp.cjs`** (+60, new) — single pack entry point:
downloads
simulator-server, native binaries, **and** the trace-processor bundle,
then
builds and `npm pack`s `@swmansion/argent`. `package.json`'s `pack` /
`pack:mcp`
  now call this script.
- **`packages/argent/scripts/bundle-tools.cjs`** (heavily reworked,
+574/-...) —
copies `assets/trace-processor/`, `assets/queries/`, and
`argent.tracecfg.pbtxt`
into `packages/argent/assets/`, and regenerates `bundled-meta.ts` to
stamp the
  Perfetto version.
- **`publish.yml` / `publish-next.yml`** — run the download step and
then **verify**:
  all four WASM files present, the wasm magic-byte check
(`head -c4 … | grep $'\x00asm'`), plus the queries dir, TraceConfig,
manifest,
  and Android helper APK.

Asset reorg: `native-devtools-android` now ships `dist/`, `bin/`, and
`assets/`
(SQL queries, TraceConfig, manifest moved under `assets/`); `.gitignore`
swaps the
old per-file `packages/argent/manifest.json` entry for the whole
`packages/argent/assets/` dir.

---

## Testing

15 new test files under `packages/tool-server/test/android-perfetto/`
cover the
engine and pipeline, including:

- `dispatch.test.ts` — start/stop/analyze route to the right platform
backend.
- `aggregate-shared.test.ts` — the shared aggregator on both iOS and
Android-shaped input.
- `hang-folds-batched.test.ts` / `hang-fold.test.ts` — the batched
per-hang SQL fold.
- `hang-severity.test.ts` — jank-reason → RED/YELLOW classification.
- `dispose.test.ts` / `dispose-warm-engine.test.ts` — session dispose
kills the daemon, rms the device trace, and tears down the warm engine.
- `stop-recovery.test.ts` — `profilingActive` is forced false even if
`adb pull` fails (so a retry-start doesn't wedge).
- `run-tp.test.ts` — `renderSqlTemplate` token rendering + the "unused
substitution" guard.
- `manifest-hint.test.ts` — zero-callstack detection emits the manifest
hint.
- `profiler-load.test.ts` — `.pftrace` session restore with/without the
metadata sidecar.

Plus `native-profiler-analyze-failure.test.ts` for the
engine-unavailable banner,
and updated iOS tests for the shared-lifecycle refactor.
`@argent/native-devtools-android` gains its own vitest setup
(`vitest.config.ts`, `tsconfig.test.json`, `test/errors.test.ts`).

---

## Notes & follow-ups

- **Manifest requirement.** Perfetto's `linux.perf` data source needs
`/proc/<pid>/mem` access, granted only to a **debuggable** or
`<profileable
android:shell="true"/>` app. A release build without either silently
produces
zero callstacks; the pipeline detects this and emits a **manifest hint**
instead
  of an "All clear" report.
- **Version pinning lives in the `argent-private` submodule**
(`PERFETTO_VERSION`
/ `PERFETTO_WASM_TAG` / `LYNX_TRACE_PROCESSOR_VERSION`). Bumping is
documented in
`argent-private/docs/ANDROID_PERFETTO.md`. JS↔engine version skew is
structurally
  impossible (version-agnostic decoder + vendored wasm).
- **`disposeWarmEngine` is exported but not wired into session-stop** —
warm
engines are reclaimed only by the idle timer + LRU cap. Acceptable today
(bounded
memory); wire it into session dispose if explicit teardown is ever
needed.
- **Submodule bump.** This PR moves `packages/argent-private` to
`22e55f90` — see the companion `argent-private` PR description,
including its
merge-hygiene note (that branch should pick up latest `argent-private`
main
  before the pointer is finalized).

### Companion docs added in this branch
- `utils/android-profiler/ANDROID_PROFILER_REFERENCE.md` — stack,
capture, queries, manifest requirement.
- `utils/android-profiler/PIPELINE_DESIGN.md` — the "why" (two-file
pipeline, shared-aggregator hoist, re-query-don't-cache, batched fold).

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-09 16:06:24 +02:00
filip131311 18974cab93 fix(release): ship ax-service in bin/darwin/ so describe works in published packages (#303)
## Summary

`describe` is broken in every published package since **0.9.0**: on a
normally-booted simulator it returns only a bare `ROOT AXGroup` (empty
tree) or silently falls back to native-devtools, always accompanied by
the misleading hint *"this simulator was not booted through argent …
call boot-device with force=true"* — even right after a forced argent
boot, so there is no recovery.

**Root cause:** the published npm package is **missing the `ax-service`
binary** entirely.

## Bisect

| Release | `ax-service` in tarball | `describe` |
|---|---|---|
| 0.7.1 | `bin/ax-service` (flat) ✅ | works |
| 0.8.1 | `bin/ax-service` (flat) ✅ | works |
| **0.9.0** | **absent** ❌ | **broken** |
| 0.10.0 | **absent** ❌ | **broken** |

- **Last good release:** 0.8.1
- **First broken release:** 0.9.0
- **Breaking commit:** `80661fad` — *feat: Linux support (#249)*

## Why it broke

#249 migrated native binaries to a per-platform `bin/<platform>/` layout
and updated **two of the three** places that touch the ax-service path:

- `packages/native-devtools-ios/src/index.ts` — `axServiceBinaryPath()`
now resolves `bin/darwin/ax-service` (was flat `bin/ax-service`).
- `packages/argent/scripts/bundle-tools.cjs` — copies the published
binary **from** `bin/darwin/ax-service`.

…but it **did not** update `scripts/download-native-binaries.sh` — the
script the standard `pack` / CI release path runs — which kept
downloading ax-service to the **flat `bin/` root**.

Producer (download script) and consumer (bundler/resolver) disagreed.
During `pack`, the bundler looked under `bin/darwin/`, found nothing,
**warned and skipped the copy**, and the tarball shipped with no
ax-service. At runtime the ax-service factory throws *(binary not
found)* → `describe` catches it → empty tree + degraded hint →
native-devtools fallback (which itself fails on plain screens like
SpringBoard).

`pack:mcp:local` (which runs `build:ios-binaries` → `build.sh`, already
writing to `bin/darwin/`) was unaffected — only released/downloaded
builds were broken, which is why it wasn't caught locally.

## Fix

`download-native-binaries.sh` now downloads ax-service into
`bin/darwin/`, matching the resolver and the bundler. Plus a regression
test (`package-manifest.test.ts`) that pins the producer→consumer path
agreement so the two scripts can't silently drift again (verified it
fails on the pre-fix path and passes after).

## Verification

Rebuilt via **`npm run pack:mcp`** → tarball now contains
`package/bin/darwin/ax-service` (byte-identical, signed Mach-O). Drove
the **packaged** binary's `describe` RPC directly and via the full MCP
`describe` tool:

- iPhone 17 Pro — iOS 26.4 → `source: ax-service`, full tree (32
elements), no degraded hint ✅
- iPhone Air — iOS 26.4 → `source: ax-service`, full tree ✅
- iPhone 16 Pro — iOS 18.5 → `source: ax-service`, full tree (33
elements) ✅

Before the fix all three returned a bare `ROOT AXGroup` (or
native-devtools fallback) + the degraded hint.

Tests: `packages/tool-server` ax-service/describe suites (40) green;
`packages/argent` suite (12, incl. new guard) green.

## Notes
- Script-only change to release tooling + one test; no runtime TS
changes. The bundled `bin/` artifacts are gitignored (produced by
`pack`), so the diff is just the script and the test.
- Independent of `fix/boot-device-refresh-stale-services` (stale-service
eviction after reboot) — that fix can't help while the binary is absent
from the package; both are needed for fully healthy `describe`.
2026-06-03 22:28:22 +02:00
Paweł Fornagiel a96f1f1578 feat: refactor android describe to not rely on uiautomator (#237)
## Summary

- `formatDescribeTree` was branching on `source === "uiautomator"` to
pick nested rendering; the newer `android-devtools` source fell through
to flat mode and lost every node below depth 1. Extends the predicate to
cover both Android sources so the helper-service path renders with the
same hierarchy as the legacy `uiautomator dump` path.
- Bumps `packages/argent-private` to pick up the persistent Android
describe service that emits `source: "android-devtools"`, plus its
build-and-sign CI workflow.
- Wires the Android helper APK + `manifest.json` into the published
`@swmansion/argent` package via `bundle-tools.cjs`, and adds
`pack:mcp:local` / `build:android-binaries` so a local tarball can be
produced without going through `download:native-binaries`.
- Minor: tightens a couple of comments in `android-helper-install.ts`
and `format-tree.ts`.

This is connected to argent-private PR:
https://github.com/software-mansion/argent-private/pull/8
2026-05-27 11:46:12 +02:00
filip131311 9fee899e5f chore: bump package versions to 0.5.3 for mcp, registry, skills, and tool-server (#149) 2026-04-20 17:51:14 +02:00
pFornagiel 066aad2452 fix: fix problematic CLI install, bump package versions to 0.5.2 for mcp, registry, skills, and tool-server 2026-04-16 17:47:25 +02:00
filip131311 3979e2780d bump version (#136)
* init

* chore: update package versions to 0.4.5 for registry, skills, and tool-server

---------

Co-authored-by: pFornagiel <pawel.fornagiel2@gmail.com>
2026-04-16 14:15:44 +02:00
Filip131311 ea99e945f8 fix: default download:native-binaries tag to argent-main 2026-04-16 00:17:59 +02:00
filip131311 7db8305fff feat: pin native binaries to versioned release tags and publish to npm (#125)
- download-native-binaries.sh now requires an explicit release tag (e.g.
  argent-v0.4.3) and fails fast with a clear error if the release doesn't
  exist in argent-private-releases
- publish.yml is now manual-only (workflow_dispatch) with a ref input
  (branch/tag/SHA); derives the binaries tag as argent-{ref} and checks
  out the repo at that ref
- publish.yml now targets npm (registry.npmjs.org + NPM_TOKEN) instead
  of GitHub Packages
- download:native-binaries npm script auto-derives the tag from
  packages/mcp/package.json version for local pack:mcp usage
2026-04-15 16:26:55 +02:00
Filip131311 f30948fe46 feat: download native binaries from argent-private-releases instead of building locally 2026-04-15 11:41:10 +02:00