4 Commits

Author	SHA1	Message	Date
filip131311	b232114422	docs: audit every comment in src against the code it describes (#931) ... Audits every comment in `src/` and `scripts/` against the code it describes. **462 files, 57 commits, net −8,194 lines.** Comment text only — the whole branch is code-identical to `main`. ## Method One subagent per file, strictly sequential. Scope was `src/` + `scripts/` (459 files); three more files were added at the end because they carried dead references of the same kind — two test headers citing design docs that do not exist, and `publish-npm.yml` citing a retired workflow. Each agent verified every comment — line, block, JSDoc, file header, trailing — against the surrounding code and the rest of the repo, following identifiers, paths, tool ids, config keys, env vars and issue links to see whether they still exist and still behave as described. Rules: - **A false or misleading comment is deleted, not reworded.** If a claim could not be confirmed by reading the source, it went. That is why the deletion count is so much larger than the rewrite count. - Survivors are cut to the shortest form carrying something the code does not already say. Restatement, preamble, hedging, changelog prose and ASCII banners are gone; the non-obvious *why* stays. - Preserved byte-identical: license headers, pragmas and directives (`@ts-*`, `eslint-disable`, shebangs, `/// <reference>`), JSDoc tag tokens, everything inside a string or template literal, and the sole comment inside an otherwise empty block (ESLint `no-empty` counts a comment-bearing block as non-empty). ## Verification Every file passed two independent gates before being recorded as done: 1. `comments-only` — the required check. 2. A second comment-stripping comparator with a proper mode stack, written for this pass because `comments-only`'s flat scanner desyncs on nested template literals and quote-bearing regex literals and then reports comment lines as code changes. Two files hit that false FAIL (`utils/android-profiler/pipeline/index.ts`, `scripts/extract-tools.mjs`); in both the "changed code" it printed was literally `//` lines, and the second checker confirmed the code was byte-identical. After the last file, all 462 changed files were re-checked against `main` with the same comparator, rather than trusting any agent's self-report. **459 code-identical; 2 are non-code (`.svg`, `.md`); 1 intentional.** The intentional one is `packages/argent/scripts/bundle-tools.cjs`: the changed template literal *is* the comment header of the file it generates, `packages/native-devtools-android/src/bundled-meta.ts`. Fixing only the generated file would have been reverted by the next build, so the generator changed too — and it has been verified to reproduce the committed generated file byte-for-byte. ## Representative false claims removed Not wording nits — statements a reader would have acted on: - **Reversed directions.** `proxyStart`'s JSDoc had the tunnel backwards (it is a reverse tunnel: the host binds first and the simulator dials in). A `paste()` doc had the pasteboard copy direction reversed. - **Contradicted by the code below it.** A timeout budget multiplied by three where the probes run concurrently — the same comment said so six lines later. A "warn once" that warns on every call. A "binary search" that is a linear scan. - **Named things that do not exist.** A `vega-fast-cli` binary, a `finish-recording.ts`, a `publish-next.yml` workflow, two `profiler-react19-*.md` design docs, a `DebuggerTarget.ts`, a commit hash git does not know, two tool ids, an `ensureEnv` cycle. - **Wrong by construction.** "Welford accumulators" across four files where the code keeps naive `n`/`sum`/`sumSq`; `sum`/`sumSq` documented over `actualDuration` when reduce sums `selfDuration`; a strict-mode halving written `n/2` where the code ceils; field docs listing enum values the producers never emit. - **Guarantees the code does not make.** A validation matrix claiming to cover "EVERY tool" that skips flagless ones; a Pareto cutoff that `slice(0, 20)` makes inert; an idempotence claim where the real rule is at-or-ahead; a capability note describing a clean 400 the shape-based device resolver can never produce. - **Unverifiable assertions** about prebuilt binaries, external CLIs and the cloud SDK — deleted rather than kept as folklore, since nothing in the repo can confirm them. - **Stale numbers**: invented Android tool versions, hard-coded tool counts and description lengths that had drifted. ## Review A Fable agent reviewed both halves adversarially for over-deletion, misread code, `no-empty` hazards and byte-identity violations. Second-half verdict: **SHIP**, with two one-line restores, both applied in the final commit — the `npm view ""` rationale behind a blank-token guard, and the note that `argent-mcp` keeps a copy of `SECRET_PLACEHOLDER_MARKER` it cannot import. ## Code issues surfaced but deliberately not fixed This pass changes comments only. Eight genuine findings are logged for a follow-up: 1. `telemetry/src/consent.ts` — a non-ENOENT read error returns null and falls through to the default-on path, so file errors *can* silently flip telemetry on. 2. `chromium-server/navigation.ts` — `navigate()` is reachable from `POST /api/navigate` with only a `typeof === "string"` check; open-url's schema is a bare `z.string()`, so the "already validated by zod" premise never held. 3. `http.ts` — `constantTimeEqual` returns early on a length mismatch, so the auth token's length is observable. 4. `describe/index.ts:~114` — the ios-remote branch passes `{ isTvOs: false }` unconditionally, so a remote tvOS simulator takes the iOS ax-service path, though `isRemoteTvOsSimulator` exists and shake/paste do use it. 5. `devices/boot-device.ts` — `-crash-report-mode never` is passed unconditionally *and* appended again by the feature-detecting path, so every emulator spawn passes it twice. 6. `react-profiler/pipeline/04-rank.ts` — `PARETO_THRESHOLD_PCT` is dead: `slice(0, 20)` always wins. 7. `reaped-sessions.ts` — a user-facing hint string tells the agent that `react-profiler-start { force: true }` disposes the debugger and profiler session; it does not. Left byte-identical because it is a string literal, not a comment. 8. `utils/simctl-backend.ts` — `localSimctl` is exported with no importers anywhere. ## Docs No documentation change is needed: this pass touches only source comments, and no user-facing capability, tool, CLI flag, config key or flow-file behaviour changed. --------- Co-authored-by: filip131311 <f.kaminski2000@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-24 12:16:20 +02:00
Ignacy Łątka	8c8cdb885e	fix(e2e): stop the harness reporting checks it never made (#595) ... Found while running the full E2E harness against the packed 0.17.0 bundle for the release check. All of these are harness bugs, not product bugs. ## The core problem The harness reported results it had not actually measured. `assert_reject` looked for zod issues on **stdout**, but the CLI prints them on stderr and leaves stdout empty on a rejection. Both arms of the check called `pass()`, so the miss was invisible: the zod-path argument every case passes in was decorative, and a CLI that simply exited 1 for every input would have scored the same 88/88. **All 88 validation cases asserted nothing beyond "exit code was non-zero."** Once that check could fail, it exposed the rest: - **awk has no `\b` word boundary.** `/\bnumber\b/` matches nothing, so every flag kind resolved to `unknown`, `model_number_flags` was always empty, and **the bad-type matrix never emitted a single case** - while `_build_args` handed the string `"x"` to every required numeric/boolean/object field. - The describe parser matched `--[a-zA-Z0-9-]+`, **truncating every snake_case flag at the underscore** (`--device_id` → `device`), and kept the `-json` display suffix (`--selector-json`) that is not part of the schema key. Both produced payloads carrying keys no schema knows. - `missing-required` asserted `invalid_type` for enum flags, which zod reports as `invalid_value`. Parsed kinds went from **270 unknown / 21 enum / 1 array** to 131 string, 68 number, 28 boolean, 25 integer, 21 enum, 16 array, 3 object, 2 unknown (the last two are `any`, which is correct). ## Teardown and the exit code `run-e2e.sh` had no `trap`. An unbound variable inside a phase is fatal to the whole shell - `run_phase || warn` does not catch it - so an abort skipped the cleanup phase, the report, and `rm -rf $E2E_WORK`, leaking a ~500MB sandbox plus a tool-server, Electron, Metro and a fixture http server. Report generation, teardown and the exit code now live in an EXIT trap, with a completion flag: without it an aborted run exits **0** on "0 failures", which is verified below by mutation. Teardown runs *before* the sandbox is removed - `$E2E_HOME` holds the detached tool-server's pid file and that server is started with no idle timeout, so deleting the sandbox first strands a process `argent server stop` can no longer find. `90-cleanup.sh` gated `stop-all-simulator-servers` and `stop-metro` on `ARGENT_TOOLS_URL`, which the harness never sets (`ensure_server` deliberately does not pin it), so **neither ever ran** and every run left a simulator-server and Metro alive - while `teardown-complete` passed regardless. ## Other findings fixed - **`--android-avd` could never work**: `boot-device` was asked for 840s inside `run_tool`'s 120s timeout, and the booted serial stayed in a `local`, so the RN tier blanket-skipped all 24 debugger/profiler tools for want of a device that was right there. Nothing shut the emulator down either - the tool-server only reaps devices Lens booted, not ones `boot-device` started. - **The validation tier was byte-identical green against a dead server.** `argent run` does no client-side validation, so every call failed with a transport error that no one distinguished from a rejection. It refuses to run instead, and no longer counts `timeout(1)`'s 124-127 as a rejection. - **The post-uninstall restore** hardcoded `--omit=optional` and the sandbox prefix - dropping electron on a full run (the chromium tier then skipped itself as "electron not installed"), and under `--system` restoring the machine's real global `argent` into the sandbox instead of putting it back. - **The Android teardown claimed to leave an injected device alone** and then issued the same `stop-simulator-server` call as the self-booted branch, discarded the result, and recorded a skip saying it had not - tearing down the allocator's device service immediately before the RN tier drove that same serial. - **The chromium port-owner fallback** matched `:$port` as a substring and took the first pid anywhere in `ss -ltnp` output, so cleaning up `:1234` would kill a process listening on `:12345`. - **The RN tier's `stop-metro`** sat after every early return (a failed `debugger-connect` left Metro running) and ran unconditionally, killing a developer's own Metro on :8081 that the harness never started. - **The chromium display gate** accepted an installed `xvfb-run` as a substitute for `DISPLAY`, but nothing wraps the Electron spawn in it - so a headless box with it installed cleared the gate and failed `boot-device`, turning the gate red for exactly the environment it exists to skip. - A phase file with a syntax error silently re-ran the **previous** phase under the new phase's name; an unknown `--phase` selected nothing and exited 0; `-h` printed 7 lines of source code; a value-taking flag with no value died on an unbound `$2`; two runs in the same second shared one results file. - `flow-add-step` declares its own `args` property, which makes `--args` that field's value rather than the whole-payload escape hatch, so `run_tool` cannot address its fields. Skipped with that reason rather than failing on a payload the tool never received. ## Verification Against `swmansion-argent-0.17.0.tgz`, `introspection,validation`: | | before | after | |---|---|---| | totals | 177 pass / 0 fail / 5 skip | 221 pass / 0 fail / 6 skip | | rejection cases naming the field they test | **0 of 88** | **130 of 131** | | bad-type cases emitted | **0** | **43** | - `assert_reject` checked against right-field, wrong-field, wrong-code, no-path and unreachable-binary inputs. Every one of those cases passes on the un-fixed code. - Abort path: injected an unbound variable into a phase - cleanup phase runs, report written, sandbox removed, exit 1, `PARTIAL` reported. With the completion flag mutated to always-set, the same abort exits **0**, which is what pins it. - awk `\b`, the `-json` suffix, the underscore truncation, the `ss` port collision and the `timeout` 124-127 contract were each reproduced standalone before being fixed. ## Found by running against a live Android emulator The android tier executed end-to-end for the first time: **23 pass / 1 fail / 10 skip**, including the `screenshot-diff` pair that the original unbound-variable bug aborted on. Three further defects only surfaced by running: - **Nine `pass`/`fail`/`skip` calls passed their message as the third argument**, which `_record()` takes as the *case name* — so the detail column was empty and the case string changed every run. The chromium boot failure was the visible cost: it recorded `case=" "` with **no diagnostic whatsoever**. - **A call killed by `timeout(1)` writes nothing**, so `RT_OUT` is empty exactly when the operator most needs to know what happened. A hung `boot-device` was recorded as a failure with a blank detail. All nine ad-hoc detail formatters now share `rt_detail()`, which names the timeout (124) and a command `timeout` could not run (125-127). - **The chromium display gate tested that `$DISPLAY` was non-empty, not that it was usable.** A display-manager session switch on this box left `$DISPLAY` pointing at a server the process holds no cookie for; Electron then bound its CDP port and never answered on it (verified: the TCP connection is accepted, the HTTP response never arrives), so the tier hard-failed instead of skipping. Moving `profiler-stack-query` and `profiler-combined-report` into the native block also closed a hole their removal had opened: both were listed in `RN_TOOLS` and in the failure-arm skips but had no case on the success path, so a successful `react-profiler-start` left them with no pass, fail or skip at all. ## Not covered here - The **postinstall assertion** is left alone - #556 already fixes it, and this branch does not touch it. Until that merges the install phase still records that one failure. - The **RN tier still has not executed.** The emulator itself is fine — it cold-boots in ~24-30s and ran the whole android tier — but it dies from CPU starvation (`detected a hanging thread 'QEMU2 main loop'`) whenever this machine's load average climbs, which it does to 90-120 on 16 cores from concurrent work. Four attempts: it survived the android tier at load ~10, and was starved each time by a competing `npm install` or by Metro's own bundler startup. The four RN profiler fixes are verified against the tools' source contracts (the throw sites and their required params), not against a live run. - The **chromium tier still has not executed** past `boot-device`, for the display reason above. - Known remaining, deliberately left: `assert_ok` cannot fail for calls whose result is a constant `true`; the cookie/storage set→get pair never verifies the round-trip value; a tier-level skip writes one row so `report.py`'s coverage matrix silently drops the tools that tier would have recorded; `report.py` renders an empty or partial log as a clean green report.	2026-08-20 16:32:09 +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	cd4190b80e	Full tgz-only E2E harness (scripts/e2e-full) (#495) ... Now verified end-to-end on the real Mac: install/introspection/validation, the android tier (against a booted emulator), and the chromium tier all pass from a locally packed tgz; the rn tier auto-skips without `~/dev/bluesky` as designed. ## What this is A release-gating end-to-end harness under `scripts/e2e-full/` that starts from **only a `swmansion-argent-*.tgz` bundle** and exercises the whole product: the install flow, every CLI command, all 70 tools' argument validation, and a happy-path run of every tool against real devices. Intended to be run on the real Linux box and the real Mac before a release. Everything runs under a throwaway `HOME` + npm prefix, so it never touches the machine's real `~/.argent`, editor MCP configs, or global packages (safe on a shared box). It emits a JSONL log + a markdown coverage report and exits non-zero if any hard assertion fails. ## Phases | phase | needs | covers | |---|---|---| | `install` | npm + network | `npm i -g <tgz>`, bundled binaries, `init` (global + `--local`), `update`, `uninstall`, telemetry, MCP-config generation | | `introspection` | — | `--version/--help`, `tools describe` for **all 70** tools, feature flags, `server start/status/logs/stop`, `link/unlink` | | `validation` | — | per tool: missing-required / bad-enum / bad-type rejection (schema-driven, no hardware) | | `android` | Android emulator | happy-path of every touch/gesture/screenshot/app-lifecycle tool | | `chromium` | Electron (bundled optional dep) + a display | boots a generated Electron app; drives CDP tools (scroll/drag/tabs/cookies/storage) | | `rn` | `~/dev/bluesky` + Android device | debugger + react/native profiler + network chain against the real Bluesky app | Tiers auto-skip with a recorded reason when prerequisites are missing. iOS / tvOS / Vega are intentionally out of scope for now. ## What I actually verified (real runs, Linux) - **install** 18✓/0✗, **introspection** 85✓/0✗ (all 70 `tools describe`), **validation** 81✓/0✗, **chromium** 24✓/0✗ (real Electron app + CDP). - Holistic run: **190✓ / 0✗ / 34∼, exit 0.** Failure-gate confirmed (non-zero on fail), `bash -n` clean on all files, all 72 android/rn JSON arg literals linted. ## What I did NOT verify live (please scrutinize) - **`android` and `rn` tiers were never run against a live device.** The Android emulator would not stay up on my box (memory-constrained shared machine; QEMU threads hung seconds after boot across ~7 attempts). The code is complete and uses the same machinery the `chromium` tier exercised live, but it has **not** been proven end-to-end. This is the main reason for the "not certain how well it works" caveat. - The RN tier's exact debugger/profiler arg shapes are derived from `tools describe`, not confirmed against a running Metro. ## Run it ```bash bash scripts/e2e-full/run-e2e.sh # everything applicable to this OS bash scripts/e2e-full/run-e2e.sh --skip-install --phase introspection,validation # fast offline core bash scripts/e2e-full/run-e2e.sh --phase android --android-serial emulator-5554 # inject a device ```	2026-07-20 16:43:34 +02:00