577 Commits

Author	SHA1	Message	Date
Ignacy Łątka	00e3e07cf9	Merge origin/main into fix/docs-high-severity	2026-08-25 14:05:24 +02:00
Ignacy Łątka	6e1e1c2770	docs(tool-server): correct why auth can be disabled ... The empty-string token is no longer just an npm-run-dev artifact: the launcher ships it on purpose for `argent server start --no-auth`. Say what the warning actually guards (non-loopback reachability) instead of implying the flag does not exist.	2026-08-25 12:07:03 +02:00
Ignacy Łątka	5d491a92f1	chore(docs): warn when only one DocSearch credential is set ... The algolia block is built only when ALGOLIA_APP_ID and ALGOLIA_API_KEY are both set. Setting just one silently produced a search-less deploy that looked successful; the build now says why DocSearch stayed off.	2026-08-25 11:46:33 +02:00
Ignacy Łątka	12cee83e75	fix(tools-client): stop an exported ARGENT_AUTH_TOKEN authenticating a --no-auth server ... buildToolsServerEnv set ARGENT_AUTH_TOKEN only when a token was given, so the child inherited the caller's export instead. `argent server start --no-auth` then spawned a server that enforced bearer auth while the state file and connection info promised it was open, and every client call got 401. The key is now always written: the empty string is the tool-server's auth disabled value. Reproduced against the real server (401 before, 200 after).	2026-08-25 11:45:54 +02:00
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	2026-08-25 11:24:02 +02:00
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>	2026-08-25 10:48:49 +02:00
Ignacy Łątka	43a0acabdb	Merge origin/main into fix/docs-high-severity	2026-08-24 15:03:27 +02:00
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)	2026-08-24 14:51:16 +02:00
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	2026-08-24 12:47:44 +02:00
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
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)	2026-08-24 12:09:51 +02:00
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.	2026-08-24 10:36:46 +02:00
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>	2026-08-23 22:18:26 +02:00
Ignacy Łątka	af4cc74eca	fix(tools-client): pin the auto-spawned tool-server to 127.0.0.1 ... buildToolsServerEnv always overwrites ARGENT_PORT but only sets ARGENT_HOST when the caller asks for one, and the spawned child inherits the caller's environment. The auto-spawn path (MCP, `argent run`) never asked, so a developer with ARGENT_HOST exported got a server bound to that address while the handle it returned, the health check and the state file all named 127.0.0.1: every tool call then failed with ECONNREFUSED, and each run left the unreachable server behind. `argent server start` was unaffected because it always passes an explicit --host. Pass the host the auto-spawn path already records, and stop the env table promising that exporting ARGENT_PORT, ARGENT_HOST or ARGENT_IDLE_TIMEOUT_MINUTES reaches the tool-server: both launchers set all three, so the flags on `argent server start` are what actually move them.	2026-08-21 21:19:17 +02:00
Ignacy Łątka	65dfbe8e2a	fix(docs): ship the search bar only when DocSearch is configured ... themeConfig.algolia fell back to the literal strings ARGENT_DOCSEARCH_APP_ID and ARGENT_DOCSEARCH_API_KEY, and docs-publish.yml sets no ALGOLIA_* variables, so those placeholders were baked into every published page. The CSS rule that was meant to hide the bar only hid the button: DocSearch still bound Cmd+K, the modal still opened, and its queries went to a host that does not resolve, so every search answered "No results found". The build also emitted a /search page listed in sitemap.xml and a preconnect to the same dead host. Build the algolia block only when ALGOLIA_APP_ID and ALGOLIA_API_KEY are both set. Without them the classic preset never loads the search theme, so there is no /search route and no preconnect, and the navbar is told not to mount the bar at all. The shared theme still pulls @docusaurus/theme-search-algolia/client into the bundle either way, so a small plugin supplies the one theme alias that module needs while the search theme is absent.	2026-08-21 21:19:17 +02:00
Ignacy Łątka	5a470217d2	fix(docs): make the CLI command table copy-pasteable again ... Every `argent <subcommand>` cell in docs/reference/cli.mdx separated the two words with U+00A0 rather than a space, and the character survives into the built HTML and llms-full.txt. Copying `argent init` out of the page and running it gives "argent init: command not found". Replace the 19 occurrences with ordinary spaces, and add a Repo Hygiene step so the next one fails CI instead of shipping: the table grew three more of them between the page landing and today.	2026-08-21 21:19:17 +02:00
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.	2026-08-21 19:33:57 +02:00
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>	2026-08-21 17:34:59 +02:00
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.	2026-08-21 16:51:51 +02:00
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	2026-08-21 16:31:57 +02:00
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>	2026-08-21 16:03:35 +02:00
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.	2026-08-21 14:47:00 +02:00
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.	2026-08-21 14:46:49 +02:00
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>	2026-08-21 12:14:34 +02:00
Ignacy Łątka	da61d4f2eb	test(tool-server): pin the ARGENT_* suite guard and correct what it documents (#859) ... Post-merge review of #707. Three findings, all from that change's own surface; no behaviour outside the test suite moves. ## 1. Nothing pins the guard `test/setup/clear-argent-env.ts` strips the developer's `ARGENT_*` overrides so unit tests assert argent's shipped defaults. On a machine with nothing exported its loop body never runs, and CI is exactly such a machine, so no suite failure can report the guard being weakened. Measured at `9dc7c7cfc`, each mutation applied alone to the shipped file: | mutation | full tool-server suite | |---|---| | body replaced with `export {}` | 350 files / 4300 passed, **exit 0** | | `startsWith("ARGENT_")` -> `startsWith("ARGENT")` | 350 files / 4300 passed, **exit 0** | | `delete process.env[name]` -> `process.env[name] = ""` | `bind-failure-telemetry.test.ts` 3 passed, **exit 0** | | entry removed from `setupFiles` | suite green | All four survive every gate: `vitest`, `eslint`, `prettier`, `tsc`, `typecheck:tests`, `knip`. `test/clear-argent-env.test.ts` re-imports the module against sentinels planted at call time, so the loop runs on demand rather than only when the developer happens to have an override exported. Each mutation now kills exactly one test, and the mapping is distinct: ``` body removed -> removes an ARGENT_ key outright instead of blanking it prefix underscore -> keeps a name that shares the prefix without the underscore delete -> blank -> removes an ARGENT_ key outright instead of blanking it setupFiles entry -> is registered as a setup file, so it runs before any test module ``` `ARGENTINA_REGION` is the look-alike: it pins that the underscore is load-bearing. The `in` check rather than a truthiness check is deliberate -- `= ""` reads as cleared to `if (process.env.X)` while still reaching a spawned child as a set key. ## 2. The header named the wrong file, and argued from the smallest cases > `ARGENT_PORT` / `ARGENT_HOST` retarget the bind that the startup-telemetry tests inspect. `startup-telemetry.test.ts` exists in that directory, mocks `createHttpApp`, and asserts nothing about host or port. `bind-failure-telemetry.test.ts` is the file that inspects the bind, and it is the one #707's own table named. A reader following the comment opens the sibling that literally bears that name and finds no bind assertion. The example list also covered only the four variables worth 9 failures, while the widest one was absent. Guard removed, one variable, full suite: ``` ARGENT_AUTH_TOKEN=devtoken -> Test Files 10 failed | 340 passed (350) Tests 80 failed | 4220 passed | 1 skipped (4301) ``` It puts every HTTP route behind a bearer check (`src/http.ts:59`), so ten files fail on status code alone. That is the strongest argument for scrubbing the whole prefix instead of a hand-maintained list, and the paragraph defending exactly that choice did not have it. ## 3. A comment #707 silently falsified `malloc-stack-logging.test.ts:89-95` promised to "restore the ambient value afterwards", capturing `process.env.ARGENT_IOS_CAPTURE` at module scope. The guard clears that variable before the module is imported, so the capture is always `undefined` and the `else` arm of the `afterEach` restore is unreachable. Clearing in `afterEach` is what the file actually needs and all it now claims. `export {}` is appended to the setup file so `tsconfig.test.json` -- which includes `test/**/*` -- treats it as a module rather than a global script; without it `typecheck:tests` fails with TS2306. ## Verification Run at `d0cb84de5`, rebased on `9dc7c7cfc`. - Full `packages/tool-server`: **351 files, 4306 passed, 1 skipped, 0 failed** on a clean environment, and identical with `ARGENT_EMULATOR_GPU_MODE=host ARGENT_EMULATOR_NO_WINDOW=1 ARGENT_SIMULATOR_NO_WINDOW=1 ARGENT_PORT=9999 ARGENT_HOST=0.0.0.0 ARGENT_AUTH_TOKEN=devtoken ARGENT_IOS_CAPTURE=all-processes` exported. - Mutation matrix above re-run against the new test: 4/4 killed, intended test each time, control green. - `prettier --check .` 0, `eslint . --max-warnings 0` 0, `tsc --build` 0, `typecheck:tests` 0, `knip` 0 (206 issues against the 215 ceiling, unchanged -- the new file is not flagged). ## Checked and left alone - The four `it.each(PLATFORMS)` retitles from #707 render correctly (`on linux ... -gpu swiftshader`); all four sites were fixed and a sweep of all 77 `.each(` sites in the repo found no other transposition. - The guard is genuinely unnecessary in the other ten packages: the union of all 53 `ARGENT_*` names read anywhere in `packages/*/src`, exported in two opposing polarities, leaves every other suite green. `argent-tools-client` already has its own copy from #714. - No CI workflow exports an `ARGENT_*` variable into a vitest process, so nothing in CI is destroyed by the scrub. `wayland-e2e.yml:46` sets `ARGENT_EMULATOR_NO_WINDOW` at job level but runs no vitest. - The other vestigial ambient save/restore pairs (`window-shake`, `http-auth`, `http-upload`, `http-flow-path-boundary`, `update-argent-tool`, `boot-device`, `boot-device-hotboot`) carry no false prose and are harmless defence-in-depth, so they stay.	2026-08-21 12:01:10 +02:00
Ignacy Łątka	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.	2026-08-21 12:00:50 +02:00
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.	2026-08-21 09:42:22 +02:00
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.	2026-08-20 16:32:31 +02:00
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.	2026-08-20 16:32:22 +02:00
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.	2026-08-20 15:24:28 +02:00
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`.	2026-08-19 23:06:48 +02:00
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.	2026-08-18 16:58:35 +02:00
Ignacy Łątka	513aca5e2e	test(tool-server): stop unit tests inheriting the developer's ARGENT_* overrides (#707) ... ## Problem The unit suite inherits the developer's `ARGENT_*` shell configuration, so tests that assert argent's shipped defaults instead assert a property of the machine running them. I hit this as six failures in `boot-device-hotboot.test.ts` on a Linux box: ``` AssertionError: expected [ '-avd', 'Pixel_7_API_34', …(9) ] to deeply equal ArrayContaining ["-gpu", "swiftshader"] - "swiftshader" + "host" ``` `swiftshader` is the Linux default and `auto` the macOS one. `host` is neither — it came from `ARGENT_EMULATOR_GPU_MODE=host` exported in my shell, which is the documented escape hatch for hosts where swiftshader is too slow. `selectGpuMode` (`boot-device.ts:210`) reads the env override and returns **before** the `process.platform === "linux" ? "swiftshader" : "auto"` fallback. That precedence is correct and is not what this changes. The `it.each(PLATFORMS)` parameterisation is also fine — `setPlatform` really does control `process.platform`; the env override simply shadows it. Proof that the source is not implicated: ``` env -u ARGENT_EMULATOR_GPU_MODE npx vitest run test/boot-device-hotboot.test.ts → Tests 25 passed (25) # unchanged source ``` ## The failure reads as a source regression This is the reason it is worth fixing rather than documenting. Someone who has ever set `ARGENT_EMULATOR_GPU_MODE` — the variable we tell people to set — gets six red tests naming a `-gpu` value they did not choose, in a file they did not touch, and nothing in the output mentions the environment. The obvious next move is to go read `selectGpuMode`, which is correct. ## Not one file `ARGENT_EMULATOR_GPU_MODE` is the one I tripped over; the class is every `ARGENT_*` override, since each is by definition a user-facing switch that changes asserted behaviour. Measured by exporting each and re-running: | variable | file | failures | |---|---|---| | `ARGENT_EMULATOR_GPU_MODE` | `boot-device-hotboot.test.ts` | 6 | | `ARGENT_EMULATOR_NO_WINDOW` | `boot-device-hotboot.test.ts` | 4 | | `ARGENT_SIMULATOR_NO_WINDOW` | `boot-device.test.ts` | 1 (suppresses the `open -a Simulator.app` it asserts) | | `ARGENT_PORT` / `ARGENT_HOST` | `bind-failure-telemetry.test.ts` | 2 each | `bind-failure-telemetry.test.ts` is the pointed one: its header comment already worries about colliding with the developer's real tool-server and mocks the probe for exactly that reason — but it never considered the env override that retargets the bind. With all eleven `ARGENT_*` variables set at once, across the three files: ``` before → Test Files 3 failed (3) Tests 9 failed | 42 passed (51) after → Test Files 3 passed (3) Tests 51 passed (51) ``` ## Fix A suite-wide setup file deletes every `ARGENT_*` key from `process.env` before the test module graph is imported, so module-level env reads see the cleared state too. Clearing the prefix rather than a hand-maintained allowlist is deliberate: an override added to `src` later is covered without a second edit here, which is precisely the kind of follow-up edit that does not happen. A test that exercises an override sets it itself, so nothing depends on the ambient value — verified by the full suite staying green. Also reorders the four `it.each(PLATFORMS)` titles. Their `%s` placeholders ran against the `[platform, gpu]` tuple and rendered as `"picks the hot-boot spawn args + -gpu linux on swiftshader"` — platform and GPU mode transposed, in the first string a diagnoser reads. ## Deliberately not scrubbed `ANDROID_HOME`, `ANDROID_SDK_ROOT`, `ANDROID_AVD_HOME`, `ANDROID_USER_HOME`, `ANDROID_SDK_HOME`, `XDG_CONFIG_HOME`, `LOCALAPPDATA`. I ran the full package suite with all seven pointed at nonexistent paths: 297/297 green, so those tests already stub what they need. Scrubbing them would be defensiveness nothing pins, and they are shared SDK variables a test could legitimately want to read. Tests that pin `process.platform` explicitly and restore it (`android-binary-windows`, `command-on-path`, `lens-tools-platform-gate`, `linux-preflight`) are correct as written and untouched. `adb-resolve-avd-path.test.ts:54`'s `it.skipIf(process.platform !== "win32")` is a deliberate gate on Windows-only path parsing and stays. ## Verification - The six `-gpu` failures reproduced at `origin/main` with the variable exported, and pass with it cleared — same command, same shell. - Hostile-env sweep above: 9 → 0. - **Mutation:** changing the Linux arm of `selectGpuMode` from `swiftshader` to `auto` is killed by four named `on linux …` tests, while the `darwin` arm stays green. Without this change that mutation is masked whenever the variable is set. - Full `packages/tool-server`: **297 files, 3090 passed, 1 skipped, 0 failed** (was `6 failed | 3084 passed`). - `eslint .` 0, `prettier --check .` 0, `tsc --build` 0 — run unpiped, exit codes read directly. ## Unrelated, found while sweeping `test/vega-cli-timeout.test.ts` fails roughly 1 run in 5 **at `origin/main` with this change absent** — it touches only `PATH`, never `ARGENT_*`. Two real-subprocess timing tests (`rejects on its own deadline when the CLI never returns`, `reaps the ENTIRE worker tree on timeout`) racing under parallel load. Not caused by this branch and not fixed here.	2026-08-17 21:50:21 +02:00
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 8bca962f. `OTLP_LOGS_ENDPOINT` is `https://argent-otel.swmansion.com/v1/logs`. That host serves OTLP as of 2026-08-12: a `POST` answers the OTLP-shaped `401` (`{"code":16,"message":"provided authorization does not match expected scheme or token"}`), where it previously `301`d to `/v1/logs/` and `404`d there. It is a distinct host from the test collector (`167.235.245.1` vs `46.224.172.171`, its own certificate). 2. **Ingest token** - `ARGENT_OTEL_INGEST_TOKEN` has to be set in the release build environment. Nothing under `.github/` sets it today, so every published artifact resolves an empty token and stays inert - silently: with no token the client is never constructed, so a run emits nothing and writes no `telemetry-debug.log` even under `ARGENT_TELEMETRY_DEBUG=1`. It must be the **production** host's token: `argent-otel.swmansion.com` runs its own token list and rejects the test collector's. 3. **IP geolocation** - PostHog's `disableGeoip: true` set `$geoip_disable` on every event, which is what stopped location being derived from the source IP. There is no in-repo equivalent for a collector, so that suppression has to be configured collector-side to keep `Telemetry.md`'s "what we collect" list accurate. 4. **Legal** - the privacy-notice edits and the version/date bump to 1.03 are drafted to match the new architecture, but Legal should review and finalize them. Note the notice actually served at `TELEMETRY_DETAILS_URL` is still Version 1.01 and names PostHog as the processor. ## Verification Unit and integration: - `packages/telemetry`: 293 tests pass, including the rewritten `index.test.ts`, `otel-endpoint.test.ts` and the new `otel-endpoint-live.test.ts`. - Every workspace suite green on the merged branch: argent-cli 276, argent-installer 535, argent-mcp 78, argent 60, tools-client 178, configuration-core 83, registry 87, tool-server 3070, update-core 31, archive 11. - `bundle-tools.cjs` leaves no `posthog` in any of the four bundles. OTel is inlined into the three that emit - `cli-cmds.mjs`, `installer.mjs`, `tool-server.cjs` - each carrying the endpoint and the token define as string literals. `mcp-server.mjs` carries none: it is a stdio shim over the tool-server, which is the process that reports. - `lint`, `prettier --check`, `tsc --build`, the `tsconfig.test.json` gate and `npm install --package-lock-only` (idempotent on Node 24) all clean.	2026-08-17 21:25:51 +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
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)	2026-08-17 18:19:54 +02:00
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>	2026-08-17 18:19:39 +02:00
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>	2026-08-17 15:01:23 +02:00
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.	2026-08-17 13:37:49 +02:00
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.	2026-08-17 11:52:14 +02:00
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.	2026-08-17 11:15:47 +02:00
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.	2026-08-17 11:15:47 +02:00
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.	2026-08-13 23:20:24 +02:00
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	2026-08-13 21:51:23 +02:00
filip131311	6177409c7c	fix(tools): drop top-level JSON Schema combinators from tool input schemas (#782) ... Fixes #773. ## The bug `gesture-rotate` (`anyOf`), `flow-execute` and `flow-read-prerequisite` (`oneOf`) hand-write an `inputSchema` that spreads a top-level combinator onto the derived schema. The Anthropic Messages API rejects that outright: ``` tools.N.custom.input_schema: input_schema does not support oneOf, allOf, or anyOf at the top level ``` The 400 fails the **whole request — every tool in it** — so any client forwarding our schemas verbatim as `custom` tools is bricked entirely, not just for these three tools. Reported from opencode. ## Why we never hit it **Claude Code silently repairs it before sending.** Verified live: it strips the combinator and re-expresses the rule as a prose line prepended to the description. All of our E2E/QA runs through Claude Code, so the defect was invisible from inside our own test loop. Secondary reasons: the code is recent (#546, 2026-07-24 for `gesture-rotate`; #568, 2026-08-05 for the two flow tools — before late July no argent tool had a top-level combinator), and nothing in CI asserted the shape of any tool's `inputSchema`. ## Why removing it is safe The combinators never enforced anything. Nothing in argent validates against the JSON Schema — the zod `.refine()`/`.superRefine()` does all the rejecting, on every dispatch path (HTTP 400, MCP `isError`, CLI flag error, internal registry dispatch), and always did. #546's stated rationale was "to keep invalid calls rejected at the schema layer", but that layer does not exist; the only JSON Schema evaluator in the repo was a 5-line helper inside that PR's own test. Verified equivalence before deleting: the removed `anyOf` accepts exactly the same 8 radius shapes as the two refinements, and `oneOf` the same 4 source combinations as the superRefine. ## What changed - The three hand-written `inputSchema` consts are gone. No tool hand-writes one now, so `Registry.registerTool` is the single emission point for all 76 schemas. - Each rule moves into the tool description, **worded differently per tool** — `tool-description-quality.yml` triggers on these paths and scores `disambiguation_score` (W=1.5), so identical prose on two tools would work against the gate. - `ToolDefinition.inputSchema` documents why it should stay underived. - New per-tool contract test over the whole catalog: no top-level `oneOf`/`allOf`/`anyOf` (API-rejected) and no top-level `not`/`if`/`then`/`else`/`$ref`/`$schema` (blocked pre-emptively — the natural way to re-encode a rejected combinator). Nested combinators stay legal and in use: `tv-remote`'s `button` is a `z.union`. - Catalog walk + expected count extracted to `test/helpers/catalog.ts`, shared with `interaction-messages` so a new tool needs the count bumped once, not twice. - Six existing tests read `.inputSchema` off unregistered definitions (populated only at registration) and now derive it. ## Not done by design **No throw in `registerTool`.** `createRegistry()` runs at tool-server bootstrap (`index.ts:182`), where a throw becomes an uncaught exception → `crashShutdown` → `exit 1`: a total outage, and `update-argent` is itself a tool so it could not run to repair it. The catalog is static and compiled in, so the contract test catches the same regressions before merge — and it also covers `paste` and the Lens tools, which never pass through `registerTool` at all. **Deliberately a denylist, not an allowlist** of permitted top-level keys: `additionalProperties` legitimately appears on schemas derived from `.strict()` objects (`screenshot-diff`, `stop-all-simulator-servers`). ## Follow-up left out `gesture-rotate`'s refinements carry `path: []`, and `deriveInvalidParams` drops empty-path issues — so every cross-field rejection is telemetered with an empty `invalid_params`. Fixing it changes CLI wording and starts populating that field, so it is not riding in this PR. ## Verification - 75 registered tools probed live: **zero** top-level combinators; the three now advertise `type, properties, required` only. - Negative test: re-introducing an `anyOf` fails the gate with `gesture-rotate declares top-level anyOf`. - `packages/registry` 90/90 pass; `packages/tool-server` 3952 pass with **7 pre-existing failures** in `bind-failure-telemetry`, `lens-relay-telemetry`, `startup-telemetry` and `tool-fail-invalid-params-emission` — all failing identically on pristine `fe5976857` (they die in `attachRegistryEventLogger`, unrelated to schemas). - Build, eslint, prettier clean; knip identical to baseline (60 unused exports / 187 unused exported types both before and after). Reaches users on the next npm publish; current version is 0.20.0.	2026-08-13 19:40:32 +02:00
filip131311	fe59768575	fix(flows): ride out iOS main-thread stalls instead of failing the step (#778) ... ## Problem iOS flows against a heavy app (Expensify NewDot, release sim build) intermittently error with ``` ✗ 4 tap "Continue" — ViewInspector RPC timed out: Application.getState ``` The only known mitigation was enabling **Reduce Motion** on the simulator — which shouldn't be a functional requirement, and (as the investigation showed) wasn't actually the mechanism. ## Root cause The injected ViewInspector handlers hop onto the app's **main thread** (`dispatch_sync` in `DevtoolsConnection.m`). During a heavy first-launch cold start (first Hermes parse, asset decode, on a loaded host) the main thread can be pinned past the flat **5s** RPC timeout, and the flow layer turns one timed-out read into a step error: - every iOS tree read first runs target auto-resolution → an `Application.getState` probe per connected app, per read — the cheap probe is the first to hit the stall; - `settleTree`'s 3s window < the 5s RPC timeout, so an action step's **first** stalled read exhausts the window and throws; - a default `await` (7.5s) gets ~1 stalled attempt; - the flow launch readiness gate (8s) sits below the same stall ceiling, because the dylib's connect is also main-thread-gated. Steady-state RPCs are fast (instrumented: `getState` p50 0ms / max 402ms, `getFullHierarchy` p50 9–12ms — even with animations on and a 14-core CPU load). Only the stall window is the problem; Reduce Motion merely shortened it below the cliff (and correlated with cache warming from repeated relaunches). Error-code note: these failures are `NATIVE_DEVTOOLS_RPC_TIMEOUT` on an **established** connection — distinct from the multi-server socket-contention family (#775), which surfaces as `could not connect to native devtools` at launch. ## Fix (three coordinated changes, tool-server only) 1. **`getFullHierarchy` RPC timeout 5s → 15s** — mirrors the Android devtools client's 5s default / 15s `getHierarchy` tiers. Every other method (incl. `getState` and the interactive point queries) keeps 5s, so one-shot agent tools stay snappy on a wedged app. 2. **Flows remember the launched app** (`launchedNativeApp` on the run state, threaded to iOS tree reads; invalidated by `launch-app`/`restart-app`/`reinstall-app`/`open-url`/`button` `tool:` steps; shared with nested `run:` flows). It is a **timeout arbiter only**: auto-resolution still runs on every read, and any *answered* resolution — including the deliberate "single app but backgrounded" guard — wins. The hint decides the target only when the resolution probe itself dies of an RPC timeout and the hinted app's devtools connection is still up. 3. **Launch readiness gate 8s → 15s** — same stall class, same budget. Accepted trade-offs (from adversarial design review): a persistent outage now takes longer to report (a dead-but-connected app fails an `await` after up to ~2 slow reads instead of three 5s ones), and a stall that clears right at a settle deadline can act on a single post-stall read (previously a hard error). Both were judged worth the removal of the false-failure class; the review also killed an earlier variant of this fix that would have bypassed the backgrounded-app guard. ## Verification - **Fault injection**: a DYLD-inserted dylib pins the app's main thread on demand (12s). Pre-fix, deterministically reproduces both historical signatures — mid-flow stall → `tap error: ViewInspector RPC timed out: Application.getState`; launch-window stall → `launch error: could not connect`. Post-fix: **5/5 green** across both stall placements ×2 + no-stall control. - **Android regression**: the same sign-in flow on the Android emulator through the patched server — green (the hint is iOS-only by construction; Android/Chromium tree paths unchanged). - **Unit tests**: new `flow-ios-tree-launch-hint.test.ts` (6 cases: timeout fallback, no-hint rethrow, disconnected-hint rethrow, backgrounded-guard preserved, non-timeout rethrow, answered-resolution-beats-hint); existing `flow-ios-tree-no-windows` and `native-target-app` suites green. Full tool-server suite: 3875 passed; the 7 failures in `bind-failure/startup/lens-telemetry` tests fail identically on pristine `origin/main` sources in this environment (pre-existing, unrelated). Related: #775 (multi-server devtools socket contention, filed from the same investigation), #774 (sim keyboard shift wedge, unrelated mechanism ruled out during triage). 🤖 Generated with [Claude Code](https://claude.com/claude-code)	2026-08-12 11:16:24 +02:00
filip131311	8b3a8851a2	fix(profiler): honour top_n in profiler-commit-query by_index (#667) ... Fixes #630. ## The bug `by_index` never received `top_n`: `renderByIndex` had no such parameter and the dispatch passed only `(commits, commit_index)`. Reproduced against the released 0.18.1 CLI on a real session — `top_n: 8` returned **306 rows / 20,245 characters**. It matters because `react-profiler-analyze` points at this exact mode for drilling into a slow commit, so it is both the most likely call and the most likely to blow an agent's context. ## Correction to the issue (and to my own first pass) The issue says "the other three modes honour `top_n` correctly". That's not right — **`cascade_tree` ignores it too** (`renderCascadeTree` takes no `topN`; its dispatch also passes two arguments). Only `by_component` and `by_time_range` slice. I've deliberately **not** fixed `cascade_tree` here, and not for the reason that first looked right. A DFS-prefix budget would in fact be structurally safe there. The real problems are that its roots come from a `Set` in insertion order with no subtree-cost aggregation, so any cut keeps an *arbitrary* subtree rather than the expensive one — making it useful needs a cost-ranking pass, which is design work, not a slice. And it dedups by `name:depth`, so it never had the 306-row blow-up that motivated this issue. Filed separately with the other findings. ## Why `top_n` is honoured only when explicitly passed Applying the schema's existing default of 20 looks obviously right and is a trap. These rows are **fibers, not components**, and they collapse hard. Measured on the real commit: | top_n | distinct components covered | |---|---| | 8 | 3 | | 20 | **10** | | 50 | 18 | | (all 306 fibers) | 70 | The analyze report that recommends this mode already shows up to **15** distinct components (`MAX_COMPONENT_ENTRIES`). So a default of 20 would make the escape hatch return *strictly less than the report it escapes from* — silently, since the footer would only say "298 hidden", not "you learned nothing new". `top_n` is therefore `.optional()`, and the two modes that genuinely defaulted to 20 keep it explicitly at their call sites (`params.top_n ?? 20`), so their behaviour is byte-identical. Since that collapse is invisible from a row count, the footer states it outright: ``` _Showing the 8 costliest of 306 fibers — 298 cheaper fibers hidden. Note these are fibers, not components: the full set covers 70 distinct components and this view covers 3. Omit top_n for everything._ ``` ## The trap this had to avoid The root-cause chain block scans the **full** commit, not the truncated table. The fiber carrying the root cause is typically *cheap* — it triggers the cascade rather than doing the work — so it falls outside any `top_n`. Slicing before that lookup would silently drop the single most useful line in the output. There's a regression test that pins exactly this: the only fiber with a `rootCauseParent` is the cheapest, `top_n=5`, and the chain must still be printed. ## Also fixed The analyze report suggested `` `profiler-commit-query mode=by_index` to see all `` **without passing `commit_index`** — an agent following it verbatim queries a different commit than the one being described. Now names the commit. Note I did *not* change it to name a `top_n` value: `totalComponentCount` counts distinct component names, while `by_index` emits one row per fiber, so passing it would still truncate — a subtler falsehood inside the line being corrected. With no default cap, "to see all" is simply true again. ## Verified live | call | before | after | |---|---|---| | `by_index top_n=8` | 306 rows, 20,245 chars | 8 rows, 898 chars, heading `Top Fibers`, `**Fibers:** 306 (showing 8)` | | `by_index` (no top_n) | 306 rows | 306 rows, heading `Full Detail`, no notice | | `by_component` | — | unchanged | ## Checks - 3093 tests pass; 7 new. Note the A/B against the pre-fix source is not meaningful for this file — `renderByIndex` wasn't exported, so those failures are import errors, not behavioural. The real before/after is the live table above, captured against the released 0.18.1 build. - extract-tools 46/46; SpiderShield average 9.10 (tool `description:` untouched); prettier, eslint, both typechecks clean; lock untouched. - Touches `renderByIndex` only — no overlap with PR #663, which owns `renderByComponent` in the same file.	2026-08-11 17:18:11 +02:00
filip131311	7e7f7f73b6	fix(profiler): resolve a Metro logicalDeviceId instead of calling it an Android device (#687) ... Fixes #618. ## The reported symptom is the mildest part `debugger-connect` returns a Metro `logicalDeviceId`, and when several devices share one Metro the debugger tools *instruct* the user to pass it (#522). But profiler sessions are keyed by the list-devices id, and `classifyDevice` decides platform purely from an id's shape — an opaque logical id matches nothing, so it falls through to `"android"` (`utils/device-info.ts:52`). Reproduced against the built code: ``` ios ← 1E273101-2926-4A76-88D0-544C7EA5C2FD (real UDID) android ← bb2e9dd6a3f14c8e9d0f1a2b3c4d5e6f (Metro logicalDeviceId) android ← "" (empty string) android ← "totally-bogus" ``` Because that id names **no existing session**, `services()` builds `NativeProfilerSession:<logicalId>`, and the registry mints services on demand — so it resolves to a **brand-new session, frozen to android, holding no capture**. The user isn't told the wrong word; the tool is reporting on a session that never recorded anything. Fixing only the platform string would still be wrong, since the data sits under the UDID. It reaches past this one message: - `native-profiler-start` dispatches the same misclassified id down the Android path, where `platforms/android.ts` hands it to **adb as a serial**. - `react-profiler-status`, `-fiber-tree` and `-renders` build session URNs with **no canonicalization**, so following the debugger tools' advice opens a **second CDP connection** to a device that already has one — precisely what the alias map in #528 exists to prevent. ## The fix `device_id` resolves through the alias map **in the schema itself**. That placement is the whole point: the registry parses params exactly once and hands the same object to `services()` and `execute()` (`registry.ts:125-153`), so one transform covers URN construction, the platform branch, and every id the tool forwards to a platform impl — including the ones passed to `adb` and `simctl`. Canonicalizing inside `services()` alone would leave those disagreeing with each other, which is how the bug got past the places that *did* canonicalize. Verified on the repo's zod 4.4.3 that this is invisible to callers: ``` input JSON schema → {"type":"string","minLength":1,"description":"…"} parse(LOGICAL) → REAL-UDID double-parse → REAL-UDID (http.ts parses, then invokeTool parses again) parse("") → rejected ``` **`classifyDevice` is deliberately untouched.** Its android fallback is load-bearing — a physical Android serial is an arbitrary manufacturer string with no matchable shape (the fixtures in-tree are `8a44101d` for a logical id and `HT82A0203045` for a real serial; both are opaque alphanumerics, so any rule strict enough to reject one rejects the other). There is a test pinning that. ## Canonicalization alone would not have been enough Aliases are learned on debugger connect (`js-runtime-debugger.ts:267`) and **forgotten on dispose** (`:287`). A user who profiles, lets the app reload, and then asks for a report has nothing left to resolve through — so "alias unknown" is the *ordinary* state, not an edge case. So a session with **no capture state at all** now reports that without naming a platform, and points at `list-devices` as the id to use. The platform-specific messages are kept for the states that actually prove a platform — an Android user who started but never stopped still gets *"No Android trace loaded"*, and that existing test fixture required no change. ## Tests 12 new cases: alias resolution, the URN/platform actually produced by `services()`, pass-through once the alias is forgotten, the published schema keeping `string`/`minLength`/description while hiding the transform, empty-id rejection, and the neutral gate firing for a never-captured session **regardless of which platform the shape guessed** — while the Android-specific message survives when a trace proves the platform. Three of them exist to stop a future "tidy-up" reintroducing this: a physical Android serial, a wireless-debugging address, and an emulator serial must all still classify as they do today. Mutation-verified: removing the canonicalization fails 2; removing the neutral gate fails 2. Full tool-server suite green — 3098 passed / 297 files. ## Scope Deliberately limited to the eight profiler tools that route to a `NativeProfilerSession`, which is what closes this bug. **Not in this PR**, because it rewrites agent-facing semantics rather than fixing a defect, and a revert of the doc churn shouldn't revert the fix: - The remaining 16 Metro-family tools carrying `device_id` (24 in total across the registry). - Seven tool descriptions that say *"Device logicalDeviceId from debugger-connect"* — stale, since `react-profiler-start`/`-stop` both canonicalize before building their URN and cache under the canonical id, so the session is keyed by the list-devices id either way. - **The skill docs, which drive the agent harder than any zod description**: `argent-metro-debugger/SKILL.md:26` calls `device_id` *"a.k.a. logicalDeviceId"*, `:34` says to *"use it as the device_id for every subsequent debugger call"*, and `references/failure-scenarios.md:10` repeats that after a crash — which is exactly the alias-forgotten path. Also noted for a follow-up: `platforms/android.ts` uses `params.device_id` as the adb serial in some places and `api.deviceId` in others; that divergence closes by construction once the family change lands. And a bogus android session minted *before* this fix can linger in a long-running server — inert, and new calls no longer reach it. --- > **Stacked on #663** (`filip/profiler-component-name-resolution`). Both add an import after `readCommitTree` in `profiler/query/profiler-commit-query.ts`. Rebased on #663, so review only the top commit. GitHub retargets it to `main` when #663 merges.	2026-08-11 17:10:28 +02:00
filip131311	98fe7a86fc	test(profiler): type component-names tests with the module's exported types (#771) ... Unbreaks the **Dead Code** gate on main, red since #663 merged. ## What happened #663 branched before the knip workflow existed (#572), so its own PR checks never ran the gate. Its `component-names.ts` exports three types with no importers — `StrippedName`, `ComponentAnnotation`, `ComponentNameResolution` — which pushed the knip second pass to **218** parked issues against the **215** ceiling. Every open PR now inherits the red check (e.g. #687, whose own commit adds no parked items). Local runs hid this: counted the way `knip.jsonc` prescribes — against a tree with **no build output** — the failure reproduces exactly. ## The fix Import the types where the tests naturally need them, which also removes a wart: the two `renderComponentNameMiss(r as never)` casts existed precisely because the narrowing `ComponentNameResolution` enables was never written. The new `expectMiss` helper narrows properly and fails loudly if the resolver ever stops returning a miss. No ceiling change: 47 + 156 + 12 = **215**, exactly at the limit. Note the budget now has zero headroom — the next PR that parks an export will trip it, which is the gate working as designed. ## Checks - clean-tree `npm run knip` exit 0 (both passes) - component-names tests 18/18, tool-server test typecheck clean, prettier clean 🤖 Generated with [Claude Code](https://claude.com/claude-code)	2026-08-11 15:37:28 +02:00
filip131311	f136c024cc	fix(profiler): accept the component names the analyze report actually prints (#663) ... Fixes #632. ## The problem `react-profiler-analyze` strips `Forget()` / `Memo()` / `ForwardRef()` wrappers for readability, then the drill-down tools matched the raw DevTools name with `===`. The report names a component and the very next call refuses it. React Compiler is on by default in Expo SDK 54, so this hits essentially every new Expo app. ## Reproduced live Real session on an Expo SDK 54 app (`experiments.reactCompiler: true`, RN 0.81.5) — 821 fiber renders, 16 commits. The report printed `` `ParallaxScrollView` [React Compiler] ``: ``` profiler-cpu-query component_cpu ParallaxScrollView → "_Component `ParallaxScrollView` not found in commit data._" profiler-cpu-query component_cpu Forget(ParallaxScrollView) → "_No CPU samples found during `Forget(...)` commits._" ``` The two messages differ, and that is the proof: "not found in commit data" means the name never matched; the other is emitted only *after* a match. **Two things beyond the report:** 1. **`profiler-commit-query mode=by_component` has the same bug** — same refusal, full table under the raw name. 2. **It is not React-Compiler-specific.** `Memo()` and `ForwardRef()` are stripped too; the same report printed `` `InnerScreen` [forwardRef] ``. ## The workflow was a deadlock The AST index is keyed on bare source identifiers, so `react-profiler-component-source` accepts only the name the query tools reject: | name | query tools | component-source | |---|---|---| | `IconSymbol` | ✗ not found | ✓ file + line + source | | `Forget(IconSymbol)` | ✓ resolves | ✗ not found | There was **no single name an agent could carry** from analyze → drill-down → read source. Fixing only the query half would have left the issue's stated workflow broken. ## Approach Match a query against each recorded name's **display form**, rather than stripping both sides. Stripping both sides looks equivalent and is not: - it would quietly answer a query for `Forget(Foo)` with `Memo(Foo)` when only the latter exists — the agent asked about the compiler-wrapped fiber and silently got the memo one; - it isn't even reliable, because the display stripper stops after 4 wrappers, so a deeply wrapped name still *displays* with a wrapper and would never match. Comparing against the display form compares against exactly the string that was printed. **Exact matches always win and are never widened.** `StaticContainer` and `Memo(StaticContainer)` both appear in one ordinary 18-second session — I checked all 70 distinct names in the dump, and that is the one real collision. Asking for the bare name returns the bare fiber, and the output now names the sibling instead of silently showing one of two. **Ambiguity is refused, not merged.** Where a display name maps to several distinct fibers the tools list the exact strings to retry with. `Memo(X)` and `X` are often one component's two fibers whose durations nest, so a merged total describes no real component; and picking the largest silently picks the memo wrapper's inclusive time, which is the wrong fiber for self-cost analysis. The AST lookups get the reverse fallback: raw → stripped → expo-router file suffix removed (the report shows `TabTwoScreen(./(tabs)/explore.tsx)`; the source declares `TabTwoScreen`). Loosening is safe there in a way it is not for commit data — a wrong guess against the index simply misses and returns `found: false`. ## Verified against the live session | Case | Result | |---|---| | `component_cpu ParallaxScrollView` | resolves to `Forget(ParallaxScrollView)` — was refused | | `by_component ParallaxScrollView` | full table + `> Resolved … (React Compiler). Either name works here.` | | `by_component Forget(ParallaxScrollView)` | **byte-identical to before**, no note | | `by_component ParallaxScroll` (typo) | miss + `Forget(ParallaxScrollView)` as a re-queryable suggestion | | `component-source Forget(IconSymbol)` | `found: true`, `icon-symbol.tsx:28` — was `found: false` | | `component-source Forget(TabTwoScreen(./(tabs)/explore.tsx))` | `found: true`, `app/(tabs)/explore.tsx:12` | ## Visible report change (intentional) Fixing the analyze-side lookup means wrapped components stop losing their source location. In the live session exactly one row changes — every other row is a node_modules component with no user source: ``` before: | `IconSymbol` [React Compiler] | 6 | 0.1ms | … | — | after: | `IconSymbol` [React Compiler] | 6 | 0.1ms | … | `ui/icon-symbol.tsx:28` | ``` ## Deliberately out of scope - **`profiler-combined-report.ts:448`** correlates leaks to components by substring-matching raw names into native symbols, so wrapped components are structurally excluded. Same bug class — *affected, deferred*, not unaffected. - **A deeper namespace bug, filed separately:** `react-profiler-stop` joins fiber metadata by name across two different namespaces, so wrapped fibers lose `parentName`, `hookTypes` and `isCompilerOptimized` entirely. Measured on this session: **41 wrapped fibers, 0 with any of the three**, versus 181/272 bare fibers carrying `parentName`. That is why the report's header says `React Compiler: ✗` while the same document tags six components `[React Compiler]`. ## Checks - 3104 tests pass (298 files); 18 new, built on names taken verbatim from the real session - the `annotateComponentName` move is a pure refactor — verified output-identical before anything else changed - `03-tag.ts`'s `ANIMATED_PATTERN` deliberately left alone (it matches unanchored and covers `Motion`/`Transition`, which the stripper must not) - tool `description:` fields unchanged, so the SpiderShield average is untouched; skills gate 10.0; prettier, eslint, both typechecks clean --- > **#687 is stacked on this branch** — it conflicts with this one in `profiler-commit-query.ts`'s imports, so it was rebased on top. Merge this first.	2026-08-11 12:42:11 +02:00