19 Commits

Author	SHA1	Message	Date
Hubert Gancarczyk	2740a3ec38	chore(knip): clear the parked dead-code backlog and drop the ceiling to 0 (#770) ... > Built on top of #769, so this PR carries that commit too — the workflows only run for PRs targeting `main`, which is why it is based there rather than on the branch. #769 on its own is the minimal, low-risk fix for the currently-red job; if it merges first this shrinks to its own commit, and if this one merges first it closes #769 as well. ## What this does #572 landed the gate with 215 findings parked under `--max-issues`. That bounded the backlog but left it in place, and the first thing to touch it (#663) pushed the total to 218 and turned the job red. This clears the backlog and sets the ceiling to **0**, so the next unused export fails the job instead of consuming headroom nobody knew was there. | | before | after | | --- | ---: | ---: | | Unused exports | 47 | 0 | | Unused exported types | 156 | 0 | | Unused exported class members | 12 | 0 | | `--max-issues` | 215 | **0** | ## Scope check first Every workspace holding a finding is `private: true`. `@swmansion/argent` — the one published package — had **zero**, so nothing here moves a public API. ## Two shapes of fix **Still referenced inside its own file → drop `export`** (146 types, 36 exports). The declaration stays put; it just leaves the module's public surface. `tsc` keeps emitting these into the `.d.ts` as local declarations that the exported signatures reference. For the 146 **types** that is the whole story: they are erased, so the emitted `.js` is byte-identical. For the 36 **values** it is not. Dropping `export` from a `const`/`function`/`class` removes its `exports.X` binding from the emitted `dist/*.js` — 36 declarations across 26 files here. `packages/tool-server/dist/utils/device-info.js` no longer carries `exports.REMOTE_PREFIX` or `exports.VEGA_SERIAL_PREFIX`; `telemetry/dist/posthog.js` loses `exports.POSTHOG_PROJECT_TOKEN`; in the ESM-emitting `argent-installer`, `isTempRunnerPath` / `resolveLocalArgentDir` / `PACKAGE_ROOT` leave the module namespace object entirely. Nothing in the repo or in the `packages/argent-private` submodule at the pinned revision reads any of them by path, so nothing breaks. Flagging it because a `require()` of a built `dist/*.js` loses a dropped `export` exactly the way it lost the deleted `__resetAndroidDevtoolsInstallCache` this branch had to restore — so this half of the diff does deserve runtime scrutiny, not a skim. **Referenced nowhere → delete.** Orphaned test seams (`__test`, `_testValidators`), stale compatibility re-exports, and a few real orphans. The ones worth a reviewer's eye: - **`localSimctl`** — the `SimctlBackend` strategy is only ever constructed with `remoteSimctl`. What keeps the local iOS impl off it is the handler signature: `buildIosLaunchHandler` / `buildIosRestartHandler` read `services.nativeDevtools`, and `ios-remote` is the only platform that declares that service, so the local impl — which resolves native-devtools per device inside its own handler — has nothing to hand them and shells out to `xcrun` itself. - **`sendCharInsert`** — removing the `__sendCharInsert` test alias exposed the underlying helper as unreferenced. It is superseded, not unwired: the Chromium keyboard impl dispatches its own `type: "char"` event inline (`keyboard/platforms/chromium.ts:58`), which is exactly what this helper did. Chromium typing is unaffected. - **Error metadata fields** — `ServiceNotFoundError.serviceId`, `ToolNotFoundError.toolId` and their two siblings were never read. The `e.serviceId` in `registry-error-events.test.ts` is a field on the event object the handler pushes, not on the error class; the `err.toolId` in `http.ts` is `NotImplementedOnPlatformError`, a different class that keeps its fields. Flagging these in case they are wanted as deliberate diagnostics. Deleting `SourceMapsRegistry.toGeneratedPosition` / `findMatchingSource` cascaded into their only helper (`buildSourceCandidates`) and then into `projectRoot`, which had no remaining reader — so the constructor parameter goes too. `StubSourceMapsRegistry` existed only to pass `super("")`, so it goes with it. ## The three cross-workspace members `ArtifactStore.register`, `ArtifactStore.list` and `TypedEventEmitter.off` are all called from other workspaces — the artifact route and screenshot tools in `tool-server`, and `registry-listener` in `telemetry`. They are reported unused anyway, and the reason is the **unbuilt tree** the gate runs against: every workspace resolves `main`/`types` to a `dist/` that does not exist, so `@argent/registry` resolves to nothing and the cross-workspace edge never forms. Build the tree and all three findings disappear on their own; a control member that really is dead is still reported in the built run, so the pass is live either way. Nothing about this is specific to `classMembers` — every cross-workspace reference is invisible the same way. Each is exempted at the declaration with a `@public` JSDoc tag rather than by name in `knip.jsonc`, for one reason: **scope**. `ignoreMembers` matches a name across the whole workspace, so it would also hide a future dead `register`/`list`/`off` on any other class in `packages/registry`; `@public` binds to the member it is written on. What `@public` does **not** buy is a staleness check, and neither does `ignoreMembers`. Measured on a clean unbuilt worktree: a bogus `ignoreMembers` name and a redundant `@public` each leave the run at exit 0 with no hint, while a bogus `ignoreDependencies` name exits 1 with `Remove from ignoreDependencies`. `treatConfigHintsAsErrors` is live and simply has no producer for either exemption style — so when one of the three stops being earned, nothing says so, and the JSDoc at each member is the only record a re-audit has. Each one names its callers, and `off` records how to re-derive them (rename the member and read the `Property 'off' does not exist` errors; grepping `.off(` over-reports, since 14 of the 38 call-site hits are Node `EventEmitter`s). One correction to the commit message on that change: it says 35 hits where the exact count is 38 — 14 Node `EventEmitter`, 24 `TypedEventEmitter` (21 production, 3 in a test). The 14 and the 21 are right. `knip.jsonc`, `CONTRIBUTING.md` step 5 and the workflow's failure-explanation step all described a parked backlog and a ceiling to stay under. All three are rewritten for a gate that must simply come back empty, and all three now name the `@public` escape hatch — the remedy for a symbol whose only caller is out of knip's reach. ## Verification Run against a clean worktree with **no build output**, the way CI counts: - `npm run knip` — **exit 0, prints nothing**. Both passes empty, no config hints. - `npx tsc --build` — clean. - `npm run lint` (`eslint . --max-warnings 0`) — clean. - `npx prettier --check .` — clean. - `npm test --workspaces` — tool-server 329 files / 3857 passed; registry, telemetry, update-core, tools-client, mcp, cli green. - `npm run typecheck:tests --workspaces` — 13 workspaces define the script and all 13 are clean, but **the command itself exits 1**: npm errors `Missing script: "typecheck:tests"` for the three that do not define it rather than skipping them. Worth knowing before re-running this bullet and reading the exit code as a failure. End-to-end on a tool-server built from this branch, driven over its HTTP API: - **Chromium** (Electron smoke app) — `describe`, `screenshot`, `gesture-tap` (counter advanced 0 → 1), `POST /api/clipboard/text` read back out of the renderer, the WS `clipboardSync` command, and `debugger-connect` / `debugger-status` / `debugger-evaluate` (`sourceMapReady: true` with `StubSourceMapsRegistry` gone). - **iOS** — `boot-device`, `launch-app`, `describe` via ax-service, `screenshot`. - **tvOS** — `boot-device`, `launch-app` on an Apple TV udid, focus-driven `describe`. The registry snapshot then carries `NativeDevtools:<tvOS udid>`, confirming injection **is** resolved on tvOS through the local impl. - **Android** — `boot-device` on a dedicated AVD, `describe` via android-devtools, `gesture-tap`. - **Metro source maps** — the registry driven against a real 12.5 MB Metro `.map` over loopback, the way `Debugger.scriptParsed` drives it: allowlist passes and `waitForPending()` resolves on both, while retained heap after gc drops from 17.3 MB to 3.6 MB. ## Review follow-ups Five commits on top, one per finding. The base is back on `main`, so the workflows run again. - **`docs(launch-app)`** — the services comment explained the empty shape through `LaunchAppAndroidServices`, deleted here, and skipped `LaunchAppVegaServices`, which is still there. Same edit in `restart-app/types.ts`. - **`docs(registry)`** — the `@public` tag on `ArtifactStore.register` named 2 of 7 callers. Derived the full set the way the sibling tag on `off` prescribes: renamed the member, read the 16 `TS2339` errors `tsc --build` reports across flow-visual (5), screenshot (4), native-profiler-stop (2), screenshot-diff (2), native-profiler-analyze, react-profiler-analyze and screen-recording-stop. The re-derivation recipe is recorded too — `.register(` greps as badly as `.off(`. - **`docs(knip)`** — "the build, the tests and this gate all stay green on a wrong delete" held for argent-private and not for the cross-workspace case that justifies three of the four `@public` tags. Deleting `ArtifactStore.register` gives 16 `TS2339` errors and `9 failed | 2 passed` in `test/artifacts.test.ts`; only the gate stays green. Split in the workflow comment, the failure annotation and `CONTRIBUTING.md` step 5. - **`perf(debugger)`** — `doRegister`'s `data:` branch still base64-decoded and `JSON.parse`d a payload it dropped, inside a `catch` that swallows. Both arms returned the same `void`, so the branch collapses to an early return; `scriptUrl` / `scriptId` go with it, since their last reader was the deleted `this.maps.push(...)` and `doRegister` is private. - **`test(debugger)`** — the allowlist check moved to a top-level early return with nothing pinning it: delete the line and both SSRF files still pass 17/17. Two tests now assert `doRegister` consults it — four rejected URLs reach no `fetch`, and the loopback `*.map` Metro emits still does. Re-verified after: `npm run knip` exit 0 and empty on a `tsc --build --clean` tree, `tsc --build`, `eslint . --max-warnings 0`, `prettier --check .` all clean, and `npm test --workspaces` with tool-server at 329 files / 3854 passed. The one red test, `argent-installer`'s `globalPath returns ~/.config/opencode/opencode.json`, fails identically with these commits stashed — it reads the real `~/.config/opencode`, which holds a `.jsonc` on this machine. End-to-end on a tool-server run from this branch over its HTTP API, on every platform: - **Chromium** — `launch-app`, `describe`, `screenshot` (returned an artifact handle, `GET /artifacts/<id>` served the 137542-byte PNG back), `gesture-tap` (counter 0 → 1), `debugger-connect` / `debugger-status` (`sourceMapReady: true`) / `debugger-evaluate` (read `taps: 1` out of the page). - **iOS** — `launch-app`, `describe` via native-devtools, `restart-app`, `screenshot`. - **tvOS** — `launch-app`, focus-driven `describe` (12 focusables in TVSettings), `tv-remote`, `restart-app`. - **Android** — `launch-app`, `describe` via android-devtools, `restart-app`, `screenshot`. - **Vega** — `boot-device` on the `tv` VVD, `describe` via the automation toolkit. - **Metro** — the real `debugger-connect` / `debugger-status` / `debugger-evaluate` tools against a stand-in Metro (HTTP `/json/list` + CDP WebSocket) emitting three `Debugger.scriptParsed` events: a loopback `*.map` on a counting server, a 1.4 MB inline `data:` map, and `http://169.254.169.254/latest.map`. `sourceMapReady: true`, `loadedScripts: 3`, evaluate 42, the loopback map fetched exactly once and the metadata URL never. A malformed inline payload gives byte-identical output, which is the finding. ### Merge with `main` Putting the base back on `main` made CI resolve the merge, and the Unit Tests job went red on a conflict no side could see on its own: `#771` fixed the same red gate this branch's first commit did, the other way round — it made `component-names.test.ts` import `StrippedName`, `ComponentAnnotation` and `ComponentNameResolution` instead of dropping their `export`. Merged `main` in, reproduced it (`tsc --noEmit -p tsconfig.test.json` → three `TS2459`s) and took main's resolution: the three exports are back. The gate stays green because knip treats test files as entry points, so a type a test imports is not an unused export. All nine workflows are green on the head commit.	2026-08-25 17:27:12 +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	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	4975ff490c	fix(debugger): cut debugger tool fail rate — structured not-connected results, classified CDP failures, narrow recovery (#610) ... ## Why PostHog (30d) shows the debugger tool family is argent's least reliable surface: | tool | fail rate | dominant causes | |---|---|---| | `debugger-status` | **37%** | METRO_NO_TARGETS / SERVICE_INITIALIZATION_FAILED / zod | | `debugger-connect` | **24%** | SERVICE_INITIALIZATION_FAILED (~7k, >1k users), METRO_NO_TARGETS | | `debugger-log-registry` | 41.7% raw / 4.2% excl. storms | one user retry-looped it **12,742×** on NO_TARGETS | Three design issues drive this: (1) "app/Metro not ready yet" — an expected precondition — is reported as a tool *error*, which both inflates fail rates and invites agent retry storms; (2) every CDP init/lifecycle fault lands in telemetry as the catch-all `REGISTRY_SERVICE_INITIALIZATION_FAILED` with `error_kind=unknown`; (3) ~600 users/30d hit zod validation failures with zero visibility into which parameter was wrong. ## What **1. Classified failures** (`d71efa43`) — six former plain-`Error` sites in `cdp-client.ts` / `target-selection.ts` now carry dedicated `DEBUGGER_CDP_*` / `DEBUGGER_TARGET_DEVICE_MISMATCH` signals (messages byte-identical). The registry's two causeless "terminating window" `ServiceInitializationError`s get their own `REGISTRY_SERVICE_TERMINATING` code. **2. Narrow recovery** (`ff9c1955`) — `recoverable()` on both debugger blueprints, scoped to windows where retry is provably safe: Metro recovers only `DEBUGGER_CDP_NOT_CONNECTED` (request never left the host; a detected socket death already self-heals for the *next* call via the terminated cascade — recovering `CONNECTION_CLOSED` there would be dead code, see the blueprint comment). Chromium also recovers `CONNECTION_CLOSED` for the tab-switch reconnect window. `REQUEST_TIMEOUT` stays unrecoverable (double-execution risk). A reachability test proves the retry actually fires for a RUNNING node. **3. Structured not-connected results** (`48b91ffd`) — `debugger-status` and `debugger-log-registry` become factories that return `{ status: "not_connected", reason, detail, guidance }` for classified preconditions (`metro_not_running | no_app_connected | device_mismatch | cdp_unreachable | stale_connection | reconnecting`) and still throw on unexpected faults. `detail` preserves the original error text for string-matching agents. `debugger-status` gains a socket-state gate (Metro: dispose stale node; Chromium: report transient reconnect, no dispose — the client belongs to the ChromiumCdp dependency). log-registry keeps **no** gate on purpose: captured logs are readable over a dead socket, and disposal would destroy the post-crash log file. Flows/run-sequence map a not_connected result to a **failed step** (mirrors `isUnmetUiWaitResult`) so recorded connectivity gates don't silently pass. `debugger-evaluate` re-codes agent-expression throws as `DEBUGGER_EVALUATE_EXPRESSION_THREW`. **4. Telemetry + guidance + docs** (`7602a70d`) — `tool:fail.invalid_params` (schema-declared names only; unknown keys → literal `"unrecognized_keys"`; capped emit-side at 16), new `debugger:tool_outcome` event (coded enum; joins `tool:invoke`/`complete` via `tool_invocation_id` — `ai_client`/`duration` live on the joined row), anti-retry-loop guidance appended to the Metro discovery errors (first sentences verbatim — skills match on them), six skills updated, Telemetry.md bumped to v1.03 with the new coded diagnostics disclosed. ## Dashboard migration note `debugger-status` / `debugger-log-registry` precondition failures stop emitting `tool:fail` — track them via `debugger:tool_outcome` (`outcome != 'connected'`). Per-AI-client breakdowns of those outcomes need a join on `tool_invocation_id` against `tool:complete`. ## Testing - Full suites green: tool-server 3,142, telemetry 290, registry 87. New coverage: recoverable truth tables (incl. non-Error inputs), status not-connected matrix (Metro/Chromium/terminating-window/stale/dispose-side-effect/fall-through-rethrow), log-registry rows, zod `invalid_params` derivation + emission-level guard, flow gate, evaluate rewrap, sanitize-level enum checks. - Live-tested against a booted iOS simulator + RN app and an Electron target (details in PR comments). Plan hardened through 3 adversarial review rounds (46 confirmed findings addressed) before implementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code)	2026-08-10 11:51:45 +02:00
Hubert Gancarczyk	afd722924f	feat(flow): support concurrent flow recordings (#574) ... ## Problem Only one flow could be recorded at a time, anywhere on the machine. Recording state was three module globals in `flow-utils.ts` — harmless in a per-client process, but the tool-server is a **host-wide singleton per install bundle**. Every MCP client, every subagent sharing a parent's connection, every `argent` CLI call and every project on the machine attaches to the same process, the same `Registry`, and those same three globals. HTTP requests are served concurrently with no serialization. Three failures followed, all reproduced against the pre-change code: 1. **A second recording clobbered the first.** `startRecordingSession` just overwrote, so the first agent's next `flow-add-step` silently appended into the *other* agent's flow file — in a different project. 2. **Any agent could finish any agent's recording.** `flow-finish-recording`'s schema was literally `z.object({})`. 3. **Replay stomped recording.** `resolveFlowFilePath` called `setActiveProjectRoot`, so a `flow-execute` in project B rebound the global root mid-recording of project A. Already worked around, locally only, in `flow-add-step`. ## Approach Recording state is now a `Map` keyed by the resolved flow file path, `<project_root>/.argent/flows/<name>.yaml` — the identity of the artifact being built, so "same key" means "same output file" and two sessions on one key are a genuine collision rather than an accident of scoping. `flow-add-step`, `flow-add-echo` and `flow-finish-recording` each take `name` + `project_root`, so every call is self-contained; required rather than optional-with-fallback, since a fallback would succeed while an agent is alone and fail only once a second recording exists. `resolveFlowFilePath` is pure, which fixes defect 3 and retires `FLOW_PROJECT_ROOT_REQUIRED` (the state it guarded can no longer exist). Rejected alternatives — keying by device udid (flows are deliberately device-portable, and `flow-add-echo`/`flow-finish-recording` have no device at all), an opaque `recordingId` (an agent that loses the token cannot recover), and per-caller identity from the transport (`ToolContext` carries no caller identity, and subagents share their parent's MCP connection, so it would fail the exact scenario being fixed). ### Locking, and why readers needed more than a lock Every mutation of one flow file runs under a lock keyed by that path — appends, `flow-start-recording`'s truncate-and-register, and `flow-finish-recording`'s read-summarize-clear. Keyed by file rather than owned by the session, because a restart *replaces* the session: a session-owned lock cannot exclude the operation that supersedes it. Per file, not global, so two recordings never queue behind each other. An append whose session was finished, restarted or evicted meanwhile fails with `FLOW_NO_ACTIVE_RECORDING` rather than writing into a different take. The lock serializes writers only, and no reader of a flow YAML can join it — `flow-execute`'s own load, its `run:` fragment load, `flow-read-prerequisite`, `flow-add-step`'s sibling-fragment check, and the `argent` CLI reading from another process entirely, where an in-process lock cannot reach. Both writers used a plain `fs.writeFile`, which opens `O_TRUNC`, so a reader could land between the truncate and the write. That case is silent: `parseFlow("")` returns `{ steps: [] }` with no error and `summarize` derives `ok` from "no failures", so a `flow-execute` racing an append reports a **top-level PASS having replayed zero steps**. Both writes now go through a sibling temp file and a rename, so a reader sees either the whole old file or the whole new one — and unlike the lock, that also holds cross-process. ### `stop-all-simulator-servers` gets a `devices` scope Every agent is told to call this at session end, and unscoped it walked the whole registry — so agent A wrapping up tore down agent B's devtools mid-recording, degrading B's flow to brittle coordinate taps. It now takes an optional `devices` scope and reports `unmatched` ids, so a mistyped id doesn't read as a clean machine. Which namespaces count as "owned by a device" turned out to be the load-bearing part, because nothing cascades to most of them and `unmatched` turns any omission into an actively wrong diagnosis — a correct device id reported as a typo. The set now covers `AXService` (the in-sim ax daemon, spawned `--timeout 3600`; an iOS session that only ran boot/launch/describe owns this and nothing else), `ScreenRecordingSession` (an ffmpeg child plus the touch-visualizer overlay it enabled on the device), `NativeProfilerSession` (an xctrace child, or an on-device perfetto process and its trace file) and `JsRuntimeDebugger` (a bound loopback server, the CDP socket to Metro, a log handle). The debugger URNs interpose the Metro port — `<ns>:<port>:<deviceId>` — so the matcher understands both shapes, consuming only the first colon so a wireless adb serial after the port still compares whole. `stop-simulator-server` shared none of this and had drifted: it looked URNs up with an exact, case-sensitive `services.get()`, so a lower-cased UDID silently no-op'd there while the scoped stop-all reaped it. Both tools now share one matcher. Their namespace *sets* stay deliberately different, documented where they are defined — `stop-simulator-server` is also the documented recovery for a wedged transport, and widening it to devtools/AX would make a routine retry drop the native-devtools connection another agent's recording depends on, which is the hazard the `devices` scope exists to prevent. ## Verification **End-to-end, three devices, one shared tool-server** (iPhone 16 Pro, iPhone 17 Pro, Pixel 3a): three interleaved recordings, plus an unrelated `flow-execute` in a fourth project while all three were live. Both iOS devices held their own live native-devtools concurrently (two per-UDID sockets, `connected: true`). Each YAML contained exactly its own steps in order, taps captured as selectors rather than coordinates, and neither picked up another's or the third root's path. All three replayed green — also through the **released 0.17.0 CLI** against the modified server, which checks wire compatibility. A device-scoped teardown of sim 1 left sim 2 and the emulator running. Re-run against the final build: - Two interleaved recordings across two project roots on one device, with a **separate process** polling one of the flow files as fast as it could for the duration. Zero empty or truncated observations; each YAML held exactly its own steps; both replayed green (6/6 and 5/5). Against the pre-change in-place write the same reader observes zero-step reads interleaved among the real ones — the silent-PASS input. - A scoped teardown of the iPhone reaped `AXService` and `NativeDevtools` for that UDID only, reported no `unmatched`, and left the second sim and the emulator running. Lower-casing the UDID matched; a genuinely bogus id was still reported. - With a screen recording live on the Pixel 3a, a scoped teardown reaped `ScreenRecordingSession` and the **ffmpeg child actually exited** (1 → 0). Before the fix it survived to the 180 s cap while the tool called the correct serial a typo. The pre-change repros were re-run afterwards to confirm observed behavior changed, not just the tests. `npx vitest run` in `packages/tool-server` (295 files, 3154 tests) and `packages/argent-cli` (280), `tsc --build`, `tsc --noEmit` on both test projects, eslint and prettier — all green. ## Review notes Several rounds of review found real defects in this change; each was reproduced before fixing and re-verified after. Worth knowing when reading the diff, since the tests exist to pin them: - `flow-start-recording` truncated the `.yaml` outside any lock, so a step from the take being discarded landed in the freshly reset file and was reported as success (26-29 of 200 runs). - `flow-finish-recording` never took the lock; its `await fs.readFile` was a yield during which a concurrent append committed, leaving the reported summary disagreeing with disk (11-16 of 200). - A failed finish destroyed the recording. Hand-editing the `.yaml` mid-recording is a documented workflow, so `parseFlow` can legitimately throw — and the error told the agent to call `flow-start-recording`, which truncates the very take it would recover. - `unmatched` reported a device this session had just stopped, because `disposeService` returns a node to IDLE and keeps it. - A bare IP claimed every wireless-adb device at that address, since an adb serial is itself `ip:port`. - The eviction backstop stamped with `Date.now()`; ties at millisecond resolution let it drop the session that was just used. - `requireRecordingSession` ended with "Call flow-start-recording first" — reached for a key that was finished, superseded or evicted as well as one never started, and on those branches the file on disk is fully populated while that call truncates it and reports no `restarted`. - The atomic write regressed twice on its own terms: its scratch name was derived from the flow file's basename, which has no length cap, so a long flow name that appended fine before failed `ENAMETOOLONG`; and the cleanup guard began after the temp file was created, leaking a scratch file into a committed directory on any write that failed after opening. - Two tests passed against implementations they claimed to reject: the `stop-simulator-server` narrowness guard used a udid that classifies as *android*, where the iOS-only services it guards can never appear, so widening the iOS branch kept it green; and the client-mode finish asserted only the *shape* of `savedTo`, never its content, though that directive is the only thing that lands the file in client mode. ## Notes for reviewers - Client/remote persist mode is the least-exercised path, and now has its own concurrency tests (overlapping appends, and a restart superseding an in-flight one) — it previously had none. The `project_root` key is byte-stable across the file boundary (`kind: "probe"` passes it through unchanged), which is what keeps a remote recording addressable from `flow-add-step`, which declares no file input. - Two spellings of one flow file still mint two sessions — and two independent locks — over one file, which bypasses every guarantee here. Two ways in: the root spelled two ways (a symlink, or a case variant on APFS), and the likelier one, the *name* cased two ways on a case-insensitive volume. Neither is normalized away because the correct normalization is the filesystem's: case-folding the key would wrongly merge two distinct flows on ext4, and resolving symlinks is impossible in client mode, where the root does not exist on this host. Documented at `getFlowPath`. - No recording tool can detect that another agent took its key. `flow-add-step`, `flow-add-echo` and `flow-finish-recording` all re-resolve the key on every call, so after a takeover they act on whichever take is now live and report success: an append lands in the other agent's file, and a finish finishes and clears it. The only case that fails loudly is the narrow one the liveness check covers - an append already IN FLIGHT when the restart lands. There is no caller identity to key ownership on (see the rejected alternatives above), so this is spelled out for the agent under "Pick a name unique to your task" in `argent-create-flow` rather than mechanized. - Skew hazard in one direction only: a *new* client against an *older* tool-server. Zod strips the undeclared `name`/`project_root`, and the old server falls back to its module globals — so concurrent recordings silently collapse onto one session. The other direction (released 0.17.0 CLI against this server) was verified end-to-end. - `packages/argent-private/docs/reference.md` still describes `flow-add-step` as appending to "the current recording", and names two tool ids that do not exist (`flow-insert-echo`/`flow-run` are file names; the ids are `flow-add-echo`/`flow-execute`). It is a submodule, so the fix cannot ride on this branch. - A restart reported `discardedSteps` from the superseded session's in-memory flow while it truncates the *file*. Hand-editing the `.yaml` mid-recording is documented, so the two diverge: four steps wiped, one reported. It now counts the file, and reports no number at all when that file cannot be read or parsed - 0 is the answer a genuinely empty take gives. Client mode still counts from memory (this host cannot see the client's file), and the description now says which mode it is promising. - `ChromiumJsRuntimeDebugger` cascades from `ChromiumCdp` and was therefore already being torn down - silently, while `NetworkInspector` and `ReactProfilerSession` cascade the same way and were named. `stopped` is documented as the services that were live and got shut down, so it is listed now too. - The client-mode case named for a superseded in-flight append restarted *between* calls, so the next append re-resolved the key and succeeded - the guard was never reached, and neutering it left the whole file green. It now parks an append in its live sub-tool call across the restart. - `stop-tools.test.ts`' mock handed the tool the live service map where the real `getSnapshot` copies, so a cascade rewrote the state the sweep was still reading and the answer depended on map insertion order. The mock copies and recurses like `_teardown` now, and the cascade case runs under both orders. - Prose the code contradicted, each verified against source before rewriting: Vega owns `JsRuntimeDebugger`/`NetworkInspector` once the debugger has run (and can match anything at all through an ERROR node); "nothing here cascades" was denied twice in its own file; an iOS boot/launch/describe session owns `NativeDevtools` too; `:tcp` is mintable but unreached in production; the tool-server is a singleton *per install bundle*, not per machine; `argent flow list` does enumerate `.argent/flows`; the stop-tools comments narrated a "before" that only existed on this branch. Found while verifying, out of scope, worth their own issues: a `tool: flow-execute` self-reference recurses past both cycle guards (`run:` is guarded, a nested `flow-execute` reseeds the stack); react-profiler artifacts collide host-wide on a second-resolution filename; `stop-metro` is port-scoped and kills another agent's bundler; `debugger-reload-metro`'s HTTP fallback broadcasts to every attached client. A tool's `capability` is enforced only by the HTTP layer, not by `registry.invokeTool`, so a call that reaches the registry another way - `flow-add-step` takes `command` as a bare string - can resolve a service the tool declares it does not support, leaving an ERROR node behind (e.g. `SimulatorServer:<vega serial>`). --------- Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com>	2026-08-10 10:54:58 +02:00
filip131311	66e051fe8b	fix(debugger): collapse a device's two ids onto one debugger instance (#528) ... ## The problem A device is reached through **two id namespaces** that name the same thing but are different strings: - the **stable id** the caller connects with — an iOS UDID / Android serial / Vega serial from `list-devices`; - the **`logicalDeviceId`** Metro's inspector-proxy echoes back for that connection (an opaque per-connection handle). `logicalDeviceId` is not derived by Argent — it is echoed verbatim from Metro (`js-runtime-debugger.ts`), and Metro never sees the UDID, so there is **nothing to join on synchronously**. The debugger service is cached by its URN, which embeds `device_id` verbatim: ``` JsRuntimeDebugger:<port>:<device_id> ``` `debugger-connect` took the UDID, returned the logicalDeviceId, and its description told callers to *"forward it as device_id to all subsequent debugger-\* calls."* Following that instruction produced a **different URN** → a **second `JsRuntimeDebugger` instance** → a second `CDPClient`, a second console-log server, and a split log file — **all for one device**. The second attach also kicks the first off Metro's single-debugger-per-target proxy. Nothing deduplicated them. It only *looked* fine because with a single device on a Metro port, `selectTarget`'s single-device fallback quietly connects anyway; the split surfaces as "the debugger randomly drops" and logs landing in the wrong bucket. Secondary damage the same split caused: - `classifyDevice()` is shape-based and was fed the logicalDeviceId → an iOS device's forwarded id classified as **Android** in the capability gate + telemetry. - The docs asserted *"iOS simulator UDID, Android logicalDeviceId"* as if a UDID were the iOS logicalDeviceId — the repo's own test says the opposite. - `debugger-component-tree` printed the logicalDeviceId under a `udid:` label — teaching callers the wrong name. ## The fix — synchronous, within the existing `services()` design - **Learn an alias** `logicalDeviceId → connect-id` as a side effect of a successful connect (the one place both ids are known at once; see `utils/debugger/device-alias.ts`), and **canonicalize `device_id` through it at every URN-building site**. A forwarded logicalDeviceId now resolves to the already-open instance; unknown ids (the connect id itself, Chromium ids) pass through unchanged, so it is safe to call everywhere. **No Metro round-trip on the hot path — `services()` stays synchronous.** The alias is 1:1 (a logicalDeviceId is unique per connection) and is cleared on dispose. - **Rewrite the `device_id` docs** across `debugger-*`, `network-*`, and `react-profiler-*`: pass the **same list-devices id you connected with**; the returned logicalDeviceId is informational (forwarding it still resolves, via the alias). Removes the instruction that caused the double instance and the false "UDID = logicalDeviceId" wording. - **Fix the `debugger-component-tree` header** (`udid:` → `logicalDeviceId:`). Covered call sites (all that embed `device_id` into a Metro URN): `debuggerServiceRef` (connect/evaluate/status/log-registry), `debugger-reload-metro`, `debugger-component-tree`, `debugger-inspect-element`, `react-profiler-start` (+ its session URN), `react-profiler-stop`, `network-logs`, `network-request`. ## Verification **Live**, against a real iOS sim + Metro (my-app, RN 0.81.5), driving the branch tool-server directly: | step | result | URNs instantiated | |---|---|---| | `debugger-connect { device_id: <UDID> }` | returns `logicalDeviceId: d52ad36b…` | `JsRuntimeDebugger:8081:<UDID>` | | `debugger-evaluate { device_id: d52ad36b… }` (forward the logicalDeviceId) | `42` ✅ | **no new URN — reused** | | counterfactual: `debugger-evaluate { device_id: some-unaliased-id }` | `42` ✅ | **`JsRuntimeDebugger:8081:some-unaliased-id` — a distinct 2nd instance** (the pre-fix behavior, reproduced) | | `debugger-component-tree { device_id: <UDID> }` | header reads `logicalDeviceId: d52ad36b…` ✅ | reused | **Unit** — new `test/metro/device-alias.test.ts` covers the alias (learn / canonicalize / self-alias no-op / missing-id / forget) and that `debuggerServiceRef` yields the same URN whether called with the UDID or the forwarded logicalDeviceId. Full tool-server suite green (2726 tests). ## Not in scope Renaming the `device_id` param to `udid` (or vice-versa) — that is a breaking MCP wire change. This PR is additive: a forwarded logicalDeviceId keeps working, it just no longer opens a second connection. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-20 16:21:56 +02:00
Ignacy Łątka	af8476c0a9	feat(vega): support the JS-runtime debugger and network inspector on Fire TV (#502) ... Vega's React Native is a fork of **RN 0.72**, so it serves the *legacy* Hermes inspector rather than the post-0.73 (Fusebox) debugger. The `debugger-*` / `view-network-*` / `react-profiler-*` tools were never gated for it, so every call against a VVD was rejected before any RN code ran: ``` Tool 'view-network-logs' is not supported on vega vvd (no vega support declared). ``` That gate turned out to be the only thing in the way for most of them. The concern that prompted this was that the network inspector needs the new debugger. It does not: argent's network inspector never touches the CDP `Network` domain, it monkey-patches `fetch` over `Runtime.evaluate` and reads back a global array. `Runtime.evaluate` has worked in Hermes since long before 0.73, so the network inspector is precisely the tool that is *unaffected* by the missing new-debugger stack. ## What now works on Vega Gated in via `DEBUGGER_TOOL_CAPABILITY` (these need only `Runtime.evaluate`): `debugger-connect` · `debugger-status` · `debugger-evaluate` · `debugger-log-registry` · `view-network-logs` · `view-network-request-details` ## What stays gated off, and why `debugger-component-tree` · `debugger-inspect-element` · `debugger-reload-metro` · `react-profiler-*` · `profiler-*` Two different reasons, and it is worth keeping them apart: - **`debugger-component-tree` and `debugger-inspect-element` are hard-blocked.** They are the only two callers of `cdp.evaluateWithBinding`, i.e. they deliver their payload over `Runtime.addBinding` / `Runtime.bindingCalled`. RN 0.72's Hermes **ACKs `Runtime.addBinding` and never installs the binding** — confirmed on the VVD: after a successful `debugger-connect`, `typeof __argent_callback` is still `"undefined"`, and no `bindingCalled` ever fires. They would hang until timeout. - **`debugger-reload-metro` and the profiler tools are simply unverified here.** They do *not* use the binding (`reload-metro` is `Page.reload` plus an HTTP `/reload` fallback; the profilers ride `Runtime.evaluate` and the CDP `Profiler` domain). They share `RN_ONLY_TOOL_CAPABILITY` and stay out of scope rather than ship untested. Either way the capability gate turns a hang (or an unknown) into an immediate, explicit "not supported". `describe` remains the way to read on-screen structure on Vega. Worth noting for a follow-up: the component-tree data itself *is* reachable on RN 0.72 — walking the DevTools hook via a plain `Runtime.evaluate` returns the real fiber tree (640 fibers on the sample app). Component-tree is blocked by argent's transport choice, not by a missing runtime capability, so a non-binding fallback would unlock it. ## Two bugs this surfaced **A Vega `device_id` could bind to another device's runtime.** Vega's legacy inspector-proxy emits no `reactNative` block at all, so its targets carry no `logicalDeviceId`. `selectTarget` both matched `device_id` on `logicalDeviceId` *and* counted connected devices the same way — so a Vega device was invisible to the multi-device guard from #397: its id matched no target, the guard counted zero devices, concluded there was nothing to disambiguate, and fell through to the priority target. With a Fusebox iOS/Android app on the same Metro, that target wins the priority list, so `debugger-evaluate` would have run JS **in the wrong app** while `debugger-status` reported the other device's name and `isNewDebugger: true`. Devices are now keyed by `logicalDeviceId || deviceName` (the legacy proxy always reports a name), so the guard sees them and refuses to guess. A lone VVD still falls back to its own targets. This is not Vega-specific: any RN ≤ 0.72 device was invisible the same way. **Metro's missing project-root header.** RN 0.72's Metro never sends `X-React-Native-Project-Root`, and `discoverMetro` treated a missing header as fatal — which took down evaluate, console logs and the network inspector too. The header only feeds source-map / `file:line` resolution, which degrades cleanly to no-location, so it is now optional. Platform-agnostic, not a Vega special-case: a missing source-map hint should never kill an otherwise working debugger session. That made `projectRoot: ""` a reachable state, which two source-resolution paths mishandled. `symbolicate` stripped the root with `file.replace(projectRoot + "/", "")`, which on an empty root degrades to `.replace("/", "")` and deletes the first slash *anywhere* in the path (`src/screens/Home.tsx` → `srcscreens/Home.tsx`). And `readSourceFragment` did fail closed, but only because `fs/promises.realpath("")` happens to reject where the sync API returns the tool-server's cwd — it now bails explicitly instead of leaning on that quirk. Both are reachable from `debugger-inspect-element` on any legacy Metro (not on Vega, where it is gated off). ## Verification Driven end-to-end against a real VVD (`amazon-6b8a…`) running the `react-native-multi-tv-app-sample` Vega app (Debug `.vpkg`, Metro reachable via reverse port-forward), through a tool-server built from this branch: ``` debugger-status -> connected: true, deviceName: kepler-device, isNewDebugger: false, projectRoot: "" debugger-evaluate -> "hermes=object fetch=function global=object" debugger-log-registry -> captured console.log "PR502_E2E_MARKER" === NETWORK LOGS (page 1/1, 2 total) === {id: rn-net-1} "GET /todos/7" 200 Fetch 98 B 55 ms {id: rn-net-2} "GET /users/3" 200 Fetch 520 B 17 ms view-network-request-details -> full request/response headers, status, timing ``` And the gated tools fail fast (0.02s) rather than hanging: ``` debugger-component-tree -> not supported on vega vvd debugger-inspect-element -> not supported on vega vvd debugger-reload-metro -> not supported on vega vvd react-profiler-start -> not supported on vega vvd ``` Re-ran the whole sequence against the fixed target-selection to confirm the single-VVD path still connects. Also run locally: `tsc --build`, `tsc --noEmit -p tsconfig.test.json`, `eslint . --max-warnings 0`, `prettier --check`, `grade-skills`, and the tool-server unit tests (199 files / 1740 tests). Tests added: `vega-target-routing` (the mis-route, using a target payload captured verbatim from the live VVD), `source-resolver-empty-root` (both empty-root paths), and `vega-debugger-capability` — which now drives the real `ToolDefinition.capability` fields and `resolveDevice` instead of a hand-written name↔constant pairing, pinning all 6 enabled and all 14 gated tools. The old version passed even with `debugger-status` wired to the wrong capability constant. Scope note: VVD only — no tool declares `vega: { device: true }`, so physical Fire TV hardware is untouched.	2026-07-15 15:36:12 +02:00
Ignacy Łątka	71344c40b9	fix(debugger): stop unmatched device_id silently collapsing onto one device (#397) ... ## Problem Opening debugger sessions with **different** `device_id`s could all resolve to the **same** physical device. `selectTarget` filters Metro's `/json/list` CDP targets by `reactNative.logicalDeviceId === device_id`, but when the provided `device_id` matched **no** target it kept *all* candidates and fell through to the first/priority target: ```ts const filtered = candidates.filter((t) => t.reactNative?.logicalDeviceId === options.deviceId); if (filtered.length) candidates = filtered; // unmatched id → silently ignored ``` This silently no-ops surprisingly often: Metro's `logicalDeviceId` is an internal hash, **not** the iOS simulator UDID. Verified live against a real device: - simulator UDID: `6DBF83B4-F341-4F8D-B48D-CD8FF312CCFB` - Metro `logicalDeviceId`: `a1956f6bd1e3a3ac3dbb505e24d79687a5ea60b1` With one device the fallback is harmless (it picks the only device). With **several** devices on one Metro, every unmatched `device_id` collapses onto whichever device Metro lists first — so two debugger sessions opened with different `device_id`s drive the same device. ## Fix In `selectTarget`, only fall back to the priority target when there is **at most one distinct device** to fall back to. When multiple distinct devices are connected and the `device_id` matches none, throw an error that lists the valid `logicalDeviceId`s so the caller can re-target instead of being silently misrouted. The single-device convenience path (pass any id — including a UDID — and get the one connected device) is preserved. ## Verification - `selectTarget` unit tests: distinct ids → distinct devices; ambiguous unmatched id throws and surfaces the valid ids; single-device and no-`logicalDeviceId` fallbacks still work. - Full-stack multi-device test driving `Registry → JsRuntimeDebugger → debugger-connect` against a two-device mock Metro: distinct `device_id`s route to distinct devices; unmatched id is rejected. - Full `tool-server` suite: **1203 passed**. - Confirmed the real contract on a live iOS sim: `debugger-connect` with the UDID still connects (single-device fallback) and returns `logicalDeviceId a1956f…`. ## Notes The three RN-only debugger tools (`debugger-component-tree`, `debugger-inspect-element`, `debugger-reload-metro`) build their Metro service ref via a raw template literal instead of `debuggerServiceRef()`. For the Metro path the resulting URN is byte-identical, so it's not part of this bug; left untouched to keep the fix scoped.	2026-06-26 16:42:18 +02:00
Ignacy Łątka	e2e21c9ce4	fix(debugger): return object/array results from debugger-evaluate (#365) ... ## Problem `debugger-evaluate` silently returns nothing for any expression that evaluates to an object or array. `CDPClient.evaluate` calls `Runtime.evaluate` without `returnByValue`, so a non-primitive result comes back as a RemoteObject reference with no `value` field, and the tool drops it (only primitives/strings came through). ## Fix Pass `returnByValue` + `awaitPromise` (default on) so objects/arrays serialize by value and promise-returning expressions resolve to their value. `evaluateWithBinding` opts both off to keep its fire-and-forget binding path unchanged (it delivers its payload via a side binding, not the evaluate return). ## Verification - New cdp-client unit tests assert `evaluate` sends `returnByValue:true` + `awaitPromise:true` and returns an object result, and that `evaluateWithBinding` sends both `false`. - Full cdp-client + reconnect suites pass; 1254 tool-server tests pass; eslint + prettier clean. - The real-Hermes `returnByValue` path is already exercised in production by `react-profiler-fiber-tree` / `react-profiler-renders`.	2026-06-25 16:25:24 +02:00
Ignacy Łątka	fa5af59931	refactor: replace hand-rolled parsers with standard packages (#373) ... ## What & why Swept the repo for hand-rolled regex/string parsers that reimplement a solved problem, and replaced the genuinely fragile ones with battle-tested packages (or deps already in the tree). Each swap is paired with a regression test for an input the old code got wrong. | Site | Was | Now | |------|-----|-----| | `adb.ts` `resolveAvdPath`, `linux-preflight.ts` `readMb` | per-key anchored regex over AVD `.ini` / `config.ini` | **`ini`** | | `workspace-reader.ts` `extractEnvKeys` | split-on-first-`=` line loop | **`dotenv`** (`parse`) | | `uninstall.ts` `readBundledSkillName` | `^name:(.+)$` capture + outer-quote strip | **`yaml`** (already a dep) | | `mcp-configs.ts` `escapeRegExp` | hand-rolled char-class escape | **`escape-string-regexp`** | | `export.ts` xctrace version, `session-context.ts` RN minor | `match(/(\d+)\./)` / `split(".")[1]` | **`semver`** `coerce` (already a dep) | | `source-resolver.ts` `parseDebugStack` | `at fn (file:l:c)` regex (required both line+col) | **`stacktrace-parser`** | ### Bugs these fix (covered by new tests) - **ini**: a quoted `path = "/Users/My Name/.../X.avd"` kept its quotes, failed the `startsWith("/")` guard → AVD unresolved. Inline comments / trailing junk after a sizing value made the anchored regex return `null` → sizing warning silently lost. - **dotenv**: `export FOO=bar` produced the key `"export FOO"` (dropped by the identifier filter); a multi-line quoted value leaked its continuation lines (`FAKE=...`) as phantom keys. - **yaml**: a `name:` value with a trailing `# comment` was taken verbatim (comment included); odd quoting only had one outer pair stripped. - **stacktrace-parser**: a `:line`-only frame (no column, as Hermes/JSC can emit) became an unusable `unknown` frame. ## Deliberately left alone (not "flaky", or no safe swap) To avoid churn and regressions, I did **not** touch: - **`uiautomator-parser.ts`** hand-rolled XML parser — a deliberate, documented choice with a dedicated hardening suite, including a *leniency* test for malformed dumps (stray `</node>`). A spec XML parser would reject inputs this is built to recover from. - **`ios-profiler/pipeline/xml-parser.ts`** (xctrace XML) — the most tangled parser, but it has no real `.trace` fixtures and only thin entity-decode tests. A rewrite can't be proven equivalent without device-generated traces, so swapping it blind risks silently breaking iOS CPU/hang/leak reports. Best done separately with a captured fixture. - **`http.ts` `extractHostname`** — DNS-rebinding guard; `URL.hostname` keeps the IPv6 brackets the loopback check compares without, so the "fix" isn't simpler and the path is security-sensitive. - **`adb.ts` `shellQuote`** — the POSIX single-quote escape is the standard correct form. - **`parseAdbDevices`, `android-screen.ts` `wm size`, `detect-app.ts` dumpsys** — tool-specific CLI-output scraping with no clean package equivalent (and documented edge-case handling). - **`workspace-reader.ts` `extractMetroPort`** — a best-effort heuristic over JS config; a correct replacement needs an AST parse, out of scope here. - **`config-parse.ts` Yarn/npm age-gate parsing** — a precise, unit-tested domain parser; `ms` would introduce a bare-number ambiguity. ## Verification - `tsc --build` clean across the workspace graph. - Tests: tool-server **1205**, installer **264**, update-core **31** — all pass (incl. the new regression tests). - `prettier --check` clean on all changed files. - Production esbuild bundle (`bundle-tools.cjs`) emits a valid `tool-server.cjs`/`mcp-server.mjs` with `ini`/`dotenv`/`stacktrace-parser` inlined (`node --check` OK). *(The script's final step errored only on the auth-gated `trace-processor.wasm` download, unrelated to bundling.)* New runtime deps: `ini`, `dotenv`, `stacktrace-parser` (tool-server), `escape-string-regexp` (installer) — all small, pure-JS. Lockfile change is purely additive.	2026-06-25 13:29:10 +02:00
Ignacy Łątka	9c43da3b56	fix(tool-server): fall back to raw locations when inspect-element symbolication fails (#284) ... ## Problem `debugger-inspect-element`, called with the default `resolveSourceMaps: true`, returned `source: null` **and** `code: null` for **every** item whenever Metro symbolication did not yield a usable original location. This made the tool effectively useless in its default mode — the caller got a list of component names with no file:line and no code fragment to edit. ## Root cause Two independent issues compounded: 1. **No raw fallback in the tool.** In `debugger-inspect-element.ts`, the `resolveSourceMaps` branch only assigned `source`/`code` when `symbolicate()` returned a result. When it returned `null` (symbolication failed, or was filtered out — see below), the item was left with `source: null` / `code: null`, **discarding the raw bundled frame the tool already had** in `item.frame`. 2. **Over-broad `node_modules` rejection in the resolver.** In `source-resolver.ts`, `symbolicateFrame` did `if (frame.file.includes("node_modules")) return null;`. That was meant to drop frames that symbolication couldn't map (where Metro echoes the bundle URL back). But it also threw away **genuinely-mapped** sources whose real path legitimately lives in `node_modules` — e.g. `expo-router` / `react-navigation` route components — turning a perfectly good resolved location into `null`, which then hit issue (1) and nulled the whole item. ## Fix Two minimal, localized changes: - **`debugger-inspect-element.ts`** — add an `else` to the `resolveSourceMaps` branch that falls back to the raw bundled frame `{ file: item.frame.file, line: item.frame.line, column: item.frame.col }`, mirroring the exact shape produced by the `resolveSourceMaps: false` branch. When symbolication can't map a frame, the caller now still gets the bundled location instead of `null`. - **`source-resolver.ts`** (line ~93) — replace the `includes("node_modules")` guard with `if (/^https?:\/\//.test(frame.file)) return null;`. A successful symbolication yields a real file path (kept, including legitimate `node_modules` route components); a failed one yields the original `http(s)` bundle URL echoed back (rejected). This keeps the diff to a single line + comment at line ~93 to stay easy to reconcile. ## Testing `cd packages/tool-server && vitest run test/metro/source-resolver.test.ts test/debugger/inspect-element-filter.test.ts` Added fail-before / pass-after coverage at two layers: - **Resolver level** (`test/metro/source-resolver.test.ts`): mocks `fetch` for `/symbolicate` and asserts that - a frame mapping to a `node_modules/.../*.js` path is **kept** (returned that mapped, project-relative location), - an echoed-back `http(s)` bundle URL is **rejected** (`null`), - a real app-source path is mapped relative to the project root, - a thrown request returns `null`. - **Tool level** (`test/debugger/inspect-element-filter.test.ts`): drives `debuggerInspectElementTool.execute()` with a fake `JsRuntimeDebuggerApi` (no live CDP) and asserts that when `symbolicate()` returns `null`, the item retains the raw `{ file, line, column }` (and reads no source fragment); when it returns a mapped location, that location and its code fragment are used. Verified the fail-before / pass-after cycle by checking out `origin/main` for the two source files only (keeping the new tests): the 3 new assertions fail on `origin/main` (`source` is `null` for the raw-fallback case; the node_modules-mapped source is `null`; the echoed bundle URL is wrongly accepted), then pass once the fixes are restored. All 34 tests pass with the fix; the 31 pre-existing tests are unaffected. `tsc --noEmit -p tsconfig.test.json` reports no errors in any changed file (the only errors are pre-existing module-resolution artifacts from the symlinked `node_modules`: `pngjs`, `@argent/native-devtools-android`, `@argent/update-core`). ## Known limitation / follow-up User-defined and route component frames that lack `_debugStack` under **React 19** (where `_debugSource` was removed) are still `null` even in raw mode, because there is no bundled frame to fall back to in the first place. That is a separate, riskier change (it requires a different source for the frame entirely) and is intentionally **out of scope** here. ## Note for reconcilers There are other open branches touching `source-resolver.ts` (`fix/source-resolver-traversal`, `fix/source-map-ssrf`). This change is deliberately confined to the single guard at line ~93 (plus an explanatory comment) to keep it trivially mergeable alongside them.	2026-06-18 12:53:21 +02:00
Ignacy Łątka	acfe80ab6b	chore: add ESLint with typescript-eslint (#349) ... ## What Adds type-aware linting to the workspace. Until now the only static gate on JS/TS was Prettier (formatting) and `tsc` — there was no linter, so the whole class of bugs `tsc` doesn't catch (floating promises, misused promises, throwing non-Errors, unsafe stringification, dead code) went unguarded across ~450 TS files. - **`eslint.config.mjs`** — flat config. `recommendedTypeChecked` runs on the TS sources via an explicit `project` list (each package keeps tests in a separate `tsconfig.test.json` that `projectService` can't auto-discover, so it's listed explicitly). Type-checked rules are disabled for the loosely-typed plain-JS build/dev scripts. The `argent-private` submodule and build artifacts are ignored (mirrors `.prettierignore`). - **`lint` / `lint:fix`** npm scripts. - **`.github/workflows/lint.yml`** — builds the workspace (type-aware rules need the referenced projects' declarations) then runs `eslint .`. Node 24 (ESLint 10 requires `^20.19 || ^22.13 || >=24`). ## Rule calibration This is an adoption PR, so it's deliberately calibrated rather than maximal: - **Kept on (bug-catchers):** `no-floating-promises`, `no-misused-promises` (with `checksVoidReturn: false` to drop the noisy callback variant), `only-throw-error`, `no-base-to-string`, `prefer-promise-reject-errors`, `no-unused-vars` (`_`-prefix escape hatch), plus the ESLint recommended set. - **Parked off as documented debt:** the high-volume style/`any`-family rules — `no-unsafe-*`, `require-await`, `no-unnecessary-type-assertion`, `restrict-template-expressions`, `unbound-method`, `no-explicit-any`, etc. These flag broad classes of pre-existing code and are out of scope here; each is commented as a ratchet target for follow-up passes. All findings from the kept-on rules are fixed in this PR (dead code/imports, error-cause chaining, **3 genuine floating promises**, non-Error throws, unsafe stringification). Stale unused `eslint-disable` directives left over in the tree were removed by `--fix`. ## Verification Locally, all green: `eslint .` (0 errors), `tsc --build`, `typecheck:tests`/`typecheck:scripts`, the full `vitest` suite (no behavior change from the fixes), and `prettier --check`. ## Not included (intentionally) Other static-analysis gates discussed (CodeQL, dependency-audit, ShellCheck, actionlint) are out of scope for this PR.	2026-06-17 12:20:55 +02:00
Ignacy Łątka	74c2065ca0	fix: multiple minor bugs (#194) ... ## Human Summary This PR aims to fix bugs that are benign for now, could become serious in the future: - multiple CSRF via CORS misconfiguration on `tool-server` - multiple SSRF that were possible due to the CSRF - path traversal via flow creator - multiple shell injections - lack of auth on `MCP/CLI <-> tool-server` path (bug details discussed in person). After the other fixes this is somewhat redundant, but doesn't hurt and protects against theoretical access attempts from within other processes running on the user's device. ## AI Summary <details> Bundles six independent fixes that together close a CSRF cluster on the local tool-server. The vulnerabilities chain because the HTTP API has no auth and serves wildcard CORS, so any web origin (Firefox unconditionally; Chrome via PNA-disabled or loopback-served) can drive the tool-server from a cross-origin tab. Each individual fix lives on its own branch (linked below) and stands alone — this PR exists so the cluster can be reviewed and shipped as one piece. ### Fixes 1. **`fix(http): require auth token + drop wildcard CORS`** — per-process 32-byte hex Bearer token, generated by the launcher, persisted in `~/.argent/tool-server.json` with mode `0600`, required on every route except `/preview`. Constant-time compare. Wildcard CORS dropped. Threaded through MCP server + CLI + tools-client. Backwards-compat: when `ARGENT_AUTH_TOKEN` is unset (e.g. `npm run dev`), the server logs a one-shot warning and skips the gate. 2. **`fix(http): reject non-loopback Host headers`** — middleware (before the auth gate) returns 403 for any request whose Host is not `127.0.0.1`, `localhost`, or `::1`. Closes DNS-rebinding bypass. 3. **`fix(ios-profiler-start): prevent shell injection via device_id`** — `detectRunningApp(udid)` no longer interpolates `params.device_id` into `execSync` template strings. Both `xcrun simctl spawn ${udid} …` and the piped `xcrun simctl listapps ${udid} | plutil …` are rewritten to `execFileSync` array form, with the pipe to `plutil` happening in JS (input option) rather than the shell. 4. **`fix(flows): validate name to close path-traversal in getFlowPath`** — `getFlowPath()` requires `^[A-Za-z0-9_\\-]+$` and verifies `path.relative(getFlowsDir(), filePath)` does not start with `..`. Centralizing the check in `getFlowPath` covers every caller (start-recording, add-step, insert-echo, finish-recording, run, read-prerequisite) without per-tool churn. 5. **`fix(source-resolver): reject paths outside projectRoot + non-source extensions`** — `readSourceFragment()` previously accepted any `file` field on the SourceLocation it was given (the field traces back to a React fiber's `_debugSource.fileName`, which is attacker-controllable). Now: containment check via `path.relative(projectRoot, abs)` plus an extension allowlist (`.js/.jsx/.ts/.tsx/.mjs/.cjs/.json`). 6. **`fix(source-maps): reject non-loopback URLs (close SSRF)`** — `doRegister()` validates `sourceMapURL` before fetching: only `http://`/`https://` URLs whose hostname is loopback. Closes a blind-SSRF probe of the host network (cloud metadata at `169.254.169.254`, intranet pages, etc.). ### Individual branches - `fix/auth-token-drop-cors` - `fix/host-header-validation` - `fix/profiler-command-injection` - `fix/flow-name-validation` - `fix/source-resolver-traversal` - `fix/source-map-ssrf` If you want to land them as separate PRs, the integration branch can be dropped after each lands. ## Test plan Unit tests (already in CI): - [x] `npm run build` passes (workspace tsc clean) - [x] `npm run test -w @argent/tool-server` — **510 passed (was 473 on `main`)**, +37 new tests across: - `test/http-auth.test.ts` — 10 tests (auth, CORS removal, dev-mode escape hatch) - `test/http-host-validation.test.ts` — 7 tests (loopback allow, DNS-rebinding reject) - `test/flows/flow-utils.test.ts` — +5 tests (path-traversal name rejection) - `test/metro/source-resolver.test.ts` — +5 tests (containment + extension allowlist) - `test/metro/source-maps-ssrf.test.ts` — 10 tests (loopback-only allow) Live end-to-end (run locally against this branch's bundled artifacts): - [x] Launcher spawns tool-server; `~/.argent/tool-server.json` contains a 64-char hex token and is mode `0600` - [x] `GET /tools` without `Authorization` → **401** - [x] `GET /tools` with wrong token → **401** - [x] `GET /tools` with correct token → **200** (61 tools) - [x] Request with `Host: attacker.example` → **403** - [x] No `Access-Control-Allow-Origin` header on responses - [x] Cross-origin command-injection probe via `device_id=\"x;touch /tmp/marker;#\"` does NOT create the marker file (Vuln 4 closed) - [x] `flow-start-recording` with `name=\"../../../tmp/escape\"` rejected with `Invalid flow name` - [x] `flow-start-recording` with a valid name still works - [x] `tools-client.fetchTools()` and `tools-client.callTool('list-simulators')` succeed via the auth-token threading (the path the MCP server uses internally) - [x] MCP-stdio entry point (`startMcpServer`) end-to-end: `initialize` → `tools/list` (61 tools) → `tools/call('list-simulators')` returns content. Confirms the launcher → token → fetch → tool-server flow is intact for the production `argent mcp` path. ## Notes for reviewers - `/preview` is intentionally exempt from the auth gate (it was not gated before; tightening it deserves a separate PR). - The `ARGENT_AUTH_TOKEN`-unset escape hatch keeps `npm run dev` workable; production `argent mcp` always passes a token via env, so the warning never fires there. - The auth bundle changes the public return shape of `ensureToolsServer` from `Promise<string>` to `Promise<{ url, token }>`. The two in-tree callers (`argent-mcp/mcp-server.ts`, `argent-tools-client/tools-client.ts`) were updated; external consumers using `ARGENT_TOOLS_URL` should also set `ARGENT_AUTH_TOKEN` to match the server. </details>	2026-06-03 15:57:42 +02:00
Filip131311	8c2a313c76	chore: format codebase with Prettier ... Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-04-03 14:53:58 +02:00
Paweł Fornagiel	330a13a2f2	chore: remove deprecated debugger tools and update documentation (#66)	2026-04-01 18:50:34 +02:00
Paweł Fornagiel	85ef733835	refactor: rename RADON references to ARGENT across the codebase (#56) ... * refactor: rename RADON references to ARGENT across the codebase * Merge main into branch and complete ARGENT rename in new files Merges main's changes: modular CLI architecture, UI package deprecation, db tool removal, skills/agents prefix rename, and MCP server consolidation. Extends the RADON→ARGENT rename to cover new files introduced in main (ARGENT_MCP_LOG, ARGENT_TOOLS_URL env vars, argent-skills bin). Made-with: Cursor * fix: rename RADON_MCP_LOG in observe.cjs, regenerate lockfile for argent-skills bin Made-with: Cursor * chore: remove accidental PUBLIC_RELEASE_AUDIT.md	2026-03-31 17:24:51 +02:00
Ignacy Łątka	c4658ac8fa	fix: invalid imports in test files	2026-03-13 18:28:18 +01:00
pFornagiel	c86378e6c4	feat: change radon-lite references to argent	2026-03-10 17:25:57 +01:00
pFornagiel	dc3d4ec000	feat: rename tools package to tool-server, minor functionalities renamings	2026-03-06 16:13:54 +01:00