mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
stack/721-base-legacy
18 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Ignacy Łątka
|
c0a63875df |
test(debugger): pin the source-map wait debugger-status promises (#706)
Found while reviewing #610 and set aside as out of scope: the code and the prose it comes from are byte-identical at that PR's merge base, and the same mutant survives there too. ## The gap `debugger-status` returns `sourceMapReady: true` as a **hardcoded literal** (`debugger-status.ts:57`). The only thing that makes the literal true is the line above it: ```ts await api.sourceMaps.waitForPending(); ``` and the description sells exactly that: *"sourceMapReady (always true — **waits for pending source maps before returning**; no-op on Chromium)"*. Nothing pins it. Deleting the await: | | existing suite (`test/debugger test/metro test/flows test/utils`) | |---|---| | unmutated | 856 passed | | await deleted | **856 passed** | So a status that reports readiness it never established would ship green. ## Scope, stated honestly This is a test-only change; the shipped behaviour is correct. The impact of a regression is narrow but real: `js-runtime-debugger.ts:214` already drains the registry once while the service is being created, so the await in the tool is what covers maps registered **after** that point - a Fast Refresh, or a lazily loaded bundle chunk. In that window a non-awaiting status would return `sourceMapReady: true` while `SourceMapsRegistry.pendingRegistrations` is non-empty, and a `debugger-inspect-element` issued straight after can miss the file:line it would otherwise resolve. I did not manufacture a user-visible failure end to end, and I am not claiming one - the finding is that the single promised behaviour of this field has no test. ## The tests Both assert the **ordering**, not the literal - a test that only checked `sourceMapReady === true` would pass on the mutant, since the literal is unconditional. 1. `does not resolve until the pending source-map registration settles` - hands the tool a `waitForPending` that returns a deferred promise, drains several macrotask turns, asserts the tool has not returned, then releases it. 2. `reports sourceMapReady only after the wait` - records `["maps-settled", "status-returned"]` and asserts that exact order. | | new tests | |---|---| | on `main` as shipped | 2 passed | | with the await deleted | **2 failed** | eslint `--max-warnings 0`, `tsc --noEmit -p tsconfig.test.json` and prettier all clean. |
||
|
Ignacy Łątka
|
5d8fc10799 |
test: stop tests binding machine-wide resources they do not own (#714)
## Why Three flaky or failing tests were diagnosed separately this week, and all three turned out to be one defect wearing different clothes: **a test binding a machine-wide resource it does not own**, so a second concurrent run — another checkout, a CI matrix cell, a developer's own app — perturbs it. Those three are fixed in #707, #709 and #711. Each of those was found by chasing a symptom, and each audit covered only its own resource type. This is the systematic sweep: all **448 test files across 13 packages**, plus the 4 `node --test` scripts. (The sweep itself ran over the 400 files that existed when it started; merging `main` brought the count to 448, and every category below was re-run over the full set — 25 files assign `env.HOME` and none omit `USERPROFILE`, one test file calls `pkill`/`pgrep` (`vega-cli-timeout.test.ts`, scoped out to #711 below), and no `listen()` names a fixed port.) ## Findings | Category | Files checked | Real findings | |---|---|---| | Network endpoints (bind / probe / unix socket) | 448 | 4 | | Filesystem — shared temp namespace (`os.tmpdir()` and literal `/tmp`) | 448 | 8 | | Filesystem — real `$HOME` / `~/.argent` | 448 | 4 (28 files) | | Ambient environment | 448 + all `src` readers | 12 | | Device / emulator / SDK state | 448 | 2 | | Process table (`pgrep` / `pkill` / fixed-pattern sweeps) | 448 | **0** | | In-process globals leaking across files | 13 packages × 13 modes | **0** | Every finding was reproduced with a concrete instrument before being touched, and the instrument re-run after. Representative: - **`stop-tools.test.ts`** ran the real `stop-metro` against port 59999, which `lsof`s and **SIGTERMs whatever it finds**. With a squatter there: `16 passed` → `1 failed`, and the squatter logged `received SIGTERM #1`. Now port 1. - **`bind-failure-telemetry` / `lens-relay-telemetry`** were gated on the real `~/.argent/flags.json`. Enabling the documented `tool-server-event-log` flag: `4 passed` → **`4 failed`**, and records were appended to the developer's real event log. - **LogFileWriter (12 files)** — its constructor `mkdir -p`s `os.homedir()/.argent/tmp`. Instrument: run each of the 16 test files that reference a writer, or a blueprint that builds one, under a scratch `HOME`/`USERPROFILE`, then test `$HOME/.argent/tmp` for existence. 4 created it; the 12 that actually construct a writer now all call `scopeTempHome`, and none creates it. - **`screen-recording.test.ts`** — the device ids are fixtures and `vi.useFakeTimers()` seeds its clock from the real time, so only the millisecond a run starts in separates two runs' `.mp4` paths, and each test deletes the path it derived. Self-concurrent: **2 of 3 rounds failed** → 0 of 4. Scoping `TMPDIR` removes the window rather than narrowing it. - **`DO_NOT_TRACK=1`** → telemetry `282 passed` → **`13 failed`**. **`npm_config_user_agent=pnpm|yarn|bun`** → installer `535 passed` → 4/5/3 failed. **19 tests required a real `adb`** — PATH shadow + empty HOME: `30 passed` → **`19 failed`**. - A fixed `/tmp/argent-nd-<udid8>.sock` **destroyed a concurrent holder's live socket** (proven by inode) in **3** files that run the native-devtools factory. Two of them also fail outright: `native-devtools-app-state` lost **3 of 3** rounds of two concurrent runs on `expected 'unregistered' to be 'connected'`. All three now derive the UDID from `process.pid`. - `flow-visual`, `react-profiler/dump`, `file-inputs`, `http-upload` — shared `/tmp` paths and prefix scans, each reproduced with a squatter. In `http-upload` the collision surfaced *as a timeout*, inside `vi.waitFor`. (`flow-chromium-boot` was fixed on this branch too, but main's #585 rewrote that file afterwards and the merge kept main's copy, so it is byte-identical to `origin/main` here and carries no fix.) ## Clean, with the evidence - **Process table** — an `LD_PRELOAD` exec interposer over a full run (507 execs) plus all 12 other suites. Excluding `vega-cli-timeout.test.ts`, which is #711's and does sweep the table (`pkill -f '^sleep 6910[0-9]$'` at line 134): no `pgrep`/`pkill`/`killall`/fixed-pattern sweep anywhere, and **no test signals a pid it did not spawn**. - **Real device state** — **zero** exec attempts of real `adb` / `simctl` / `emulator` / `avdmanager` / `vega`; no test asserts a device list. That list was too narrow: `workspace-reader-integration` spawned the real `yarn` / `pnpm` / `bun` / `pod` / `eas` / `expo`, which is the second finding in this row. - **Cross-file in-process leakage** — structurally impossible: vitest 4.1.9 resolves `pool: "forks"` / `isolate: true`, confirmed by a scratch experiment (distinct pids, no env leak, module `Map` = 1,1,1). 120 default-mode runs across 12 packages produced 1 failure. - **`scripts/`** — 4 files, 86 tests: `mkdtemp` throughout, no ports, no sweeps, writes nothing. ## Verification Each fix was mutated back and killed by a **named** test under its instrument — e.g. `× reads a plain absolute path`, `× returns null when only the extensionless binary exists`, `× pins both names os.homedir() consults, at the same directory`. Restores verified with `git diff` / `git status`, never a substring grep. **Pinned by an instrument only:** the socket fix for `ios-only-blueprint-gate`, the `~/.argent/tmp` and `~/.argent/flags.json` fixes, the `/tmp` fixture cleanup, the `stop-metro` port change (the test asserts `stopped: false` / `pids: []`, which hold for either port whenever nothing is listening) and the `workspace-reader` `PATH` pin (only `node` is asserted; the other seven probes are not) are not pinned by any suite assertion — reverting them still passes, and only the external squatter / scratch-`HOME` instruments catch them. They are pollution fixes. The `native-devtools-app-state` socket fix is the exception in the other direction: reverting it turns two concurrent runs red. Gates, unpiped and read by exit code: `eslint .` 0, `prettier --check .` 0, `tsc --build` 0, `typecheck:tests` 0. Suite, run as six shards: tool-server **4287 passed, 1 skipped, 0 failed**; argent-cli 462; telemetry 306; argent-tools-client 191; configuration-core 122; argent-mcp 80; argent-installer 555. All other packages and `test:scripts` green, identical to baseline. `lens-tools-platform-gate.test.ts` times out at the 5s default under full-suite load and passes at `--testTimeout=30000` (24.7s, almost all of it transform). It predates this branch and is untouched by it. ## Scope This branch is **test-only** — no `src/` file is touched. Both production defects the sweep turned up are separated out, so each can be reviewed as the user-facing fix it is: `ps` without `-ww` orphaning live tool-servers is #713, and #852 is `ensureToolsServer` respawning on every call when `ARGENT_HOST` is exported — `buildToolsServerEnv` passes the inherited value to the child while the state records the literal `127.0.0.1` the health check then probes. The second surfaced here as eight stranded `fake-tool-server` processes, and pinning the environment in this branch removes that signal, which is why it is filed rather than merely worked around. A third, outside this branch entirely, is #853: ten `tool-server` test servers omit the host on `listen(0)`, binding every interface where twenty-two sibling sites pass `127.0.0.1`. It is hardening, not a live exposure — `createHttpApp`'s Host allow-list answers a LAN-addressed request with 403, and only a raw client forging `Host: 127.0.0.1` reaches the stub registry behind it, inside the ~100 ms one `it()` holds the socket. It is also not a collision risk: ephemeral ports are kernel-assigned and never reissued while in use, which is why the fixed-port audit above is unaffected. All ten predate this branch. Left untouched on purpose: `boot-electron-spawn-error.test.ts`, `chromium-discovery.test.ts` and `vega-cli-timeout.test.ts` — owned by the still-open #709 and #711, and all three still carrying their defects on this base. (#707 has since merged into this base, so `boot-device-hotboot.test.ts` and `vitest.config.ts` carry its fix rather than a defect; `test/setup/clear-argent-env.ts` is its work, and this branch relies on it.) **Not fixed, deliberately** — an adjacent class (test-*order* dependence, not machine-global resources), reproducible if wanted: `link-config.test.ts` (deterministic: `-t "returns null when required field"` → 4 failed), `network/network-integration.test.ts` and `android-perfetto/dispatch.test.ts` (fail under `--sequence.shuffle` seeds). **Also observed, not addressed:** one green tool-server run leaves **56** entries behind in a scratch `TMPDIR` — **30** in the `argent-screenshot-diff*` family, 4 `argent-events-*`, 3 `argent-flow-crop-*`, 3 `argent-tar-upload-*`, 2 `argent-flow-diff-*`, 2 `argent-file-input-*`, and 12 assorted singletons (`ast-index-*`, `component-source-*`, `argent-profiler-cwd`, `argent-chromium-media`, …). Measured per run against a scoped temp dir, so it ranks the contributors rather than counting one machine's accumulated history — screenshot-diff, not flow-crop, is the dominant one. A separate leak. That instrument is blind to a *literal* path, which is how it missed the two `/tmp/native-profiler-*-report.md` writers the fourth pass found. Re-run across all 13 packages, `argent-mcp` is the only one outside `tool-server` that leaves anything — two entries per run, filed as #854. ## Second pass A review of this branch re-ran every category above with its own instrument rather than reading the diff, and found the sweep had pinned four resources in one file while leaving a sibling that does the same thing live. Fixed in `4289e3d8f` and `8164955b4`; the table and bullets above are the corrected totals. - **`tool-server-event-log`** was mocked off in 2 of the 4 files that call the real `start()`. With the flag on, `startup-telemetry` and `tool-fail-invalid-params-emission` failed 3 tests in `attachRegistryEventLogger` and truncated the real `~/.argent/tool-server-events.jsonl` to 0 bytes first. `ARGENT_EVENT_LOG` cannot redirect that: `test/setup/clear-argent-env.ts` strips every `ARGENT_*` var before the module graph loads. - **`scopeTempHome`** reached 8 files; 4 more built a real `LogFileWriter` through the JS-runtime-debugger blueprint. - **`os.homedir()` on Windows** reads `USERPROFILE`, not `HOME`. `android-binary-windows` and `adb-resolve-avd-path` pinned only `HOME`, and both run on a real windows-latest host. Proven by running the resolver under a `node:os` mock that reproduces the Windows lookup: with `HOME` alone the stock Studio SDK under the profile resolves; with both pinned it misses. `update-checker` had the same gap. - **`getConsentState`** honours a falsy `ARGENT_TELEMETRY` at the same precedence as `DO_NOT_TRACK`, ahead of the config override the suite writes. Only `DO_NOT_TRACK` was pinned: `ARGENT_TELEMETRY=0` gave `17 failed | 6 passed`. - **`workspace-reader-integration`** let the version probe spawn the real package managers off `PATH`; their shims self-install, so the file pulled **~44 MB** into `~/.cache/node/corepack` and `~/Library/Caches/eas-cli`. `PATH` is now a directory holding only `node`, the one version the test asserts — 44 MB → 4 KB. - **`argent-mcp`** snapshotted `ARGENT_AUTO_SCREENSHOT_DELAY_MS` without clearing it, and that package has no `ARGENT_*`-stripping setup file, so 3 tests asserted the developer's exported value. Smaller items in the same commits: the `/tmp` socket fixtures now remove the directories they create per run; `http-upload`'s pre-scan snapshot is gone, since scoping `TMPDIR` to a fresh directory made it permanently empty and its filter inert, as its two siblings already did; and two `detectProjectPackageManager` assertions that enumerated the entire return union are pinned to a single value. The confirmation pass also caught a defect the first round of fixes introduced: pinning `HOME`/`USERPROFILE` to the fixture root in `android-binary-windows` let `<home>/Android/Sdk` shadow the `%LOCALAPPDATA%` root the two positive tests are named after, so deleting `roots.push(join(localAppData, …))` from the resolver left all three green. Home has its own directory in `6aee3ee7a`, and that mutation now fails both tests. A third pass, running the nearest-twin move repo-wide, found the `os.homedir()`-on-Windows gap in one more package: eight `argent-tools-client` test files redirect only `HOME`, and four of them recursively delete `STATE_DIR` — two before each test, two after — which on Windows is the developer's real `%USERPROFILE%\.argent`. Fixed in `139447d4a`, together with the same gap in `avd-snapshot`. Nothing runs that package on Windows CI today, so no current run fires it. ## Fourth pass A fourth pass audited the three fix commits and re-ran the sweep with two instruments the earlier ones did not have. Both holes were in the *measurement*, not the diff — which is why three passes missed what they found. Fixed in `a67c43f8d`. - **`os.tmpdir()` does not read the same variable on every platform.** POSIX takes `TMPDIR`, `TMP`, `TEMP` in that order; Windows takes `TEMP`, then `TMP`, and never looks at `TMPDIR`. The five pins this branch added set `TMPDIR` alone, so on Windows production still materialized into the machine-wide `%TEMP%` while the test scanned a scratch directory nothing ever wrote to — and `file-inputs`, `http-upload` and `flow-visual` each end in a leak assertion over that listing, which then passes whatever the code does. Against `origin/main`, which diffed a before/after listing of the real tmpdir, that is a regression rather than a wash. `redirectTmpdir` (`test/helpers/tmpdir-env.ts`) sets all three and returns the restore; the **7** files that scope a tmpdir use it, including the two `android-perfetto` ones that predate this branch. `test/tmpdir-env.test.ts` pins both lookup chains — reverting the helper to `TMPDIR` alone fails two of its three tests. All three leak assertions were re-confirmed live on POSIX by mutating the production cleanup each one covers. - **A hardcoded absolute path is invisible to a `TMPDIR`-scoped instrument.** `session-correctness` and `ios-instruments/analyze-freshness` passed a literal `/tmp/native-profiler-{x,ios}.trace` as the session's trace file, and the renderer writes its report beside it (`deriveReportPath` = `dirname(traceFile)/<base>-report.md`). Every run left `native-profiler-x-report.md` and `native-profiler-ios-report.md` in the shared `/tmp`; nothing removed them, and two concurrent runs write the same names. Reproduced with `TMPDIR` pointed at a scratch dir — the files still landed in `/tmp`, which is exactly why the `os.tmpdir()` row and the leftover census below both read clean here. Both tests now own the directory. - **A prefix-scanned env var is invisible to a grep for variable names.** `secretSources` enumerates `process.env` by the `ARGENT_SECRET_` prefix and ranks it *above* every file the test writes, so `secrets-command.test.ts` — which already pins `HOME`, `USERPROFILE` and cwd — still read the developer's shell. `ARGENT_SECRET_FOO=bar`, argent's own documented way to supply a secret: `6 passed` → **`3 failed | 3 passed`**, deterministic. `argent-cli` has no `ARGENT_*`-stripping setup file, so the prefix is cleared per test. Its sibling `configuration-core/tests/secrets.test.ts` was already immune — it threads an explicit `env: NO_ENV` into every call. - **`android-binary` and `vega-cli-fallback`** were the last two files redirecting `HOME` without `USERPROFILE` — the former the direct twin of `android-binary-windows`, in the same directory, driving the same resolver. Under a `node:os` mock reproducing the Windows lookup with an SDK planted in the profile, it loses **5 of 11** tests. No test file in the repo redirects `HOME` alone any more. (`6fbb66ce1` later skipped both of this file's describes on win32 — its fixtures are extensionless and the win32 resolver only takes `.exe` — so its own `USERPROFILE` pin is now symmetry with the sibling files that do run there, not a live guard.) Two smaller items in the same commit. `native-devtools-socket-bind`'s ENOENT fixture still rooted at `os.tmpdir()` while the comment three screens above explains why the others do not: with a 142-character ambient `$TMPDIR` that path exceeds `sun_path` and `listen` rejects it for *that* reason, which the assertions cannot tell from the missing-directory one they were written for. And `<home>\AppData\Local\Android\Sdk` — the resolver's fallback for a GUI-spawned server that never inherited `%LOCALAPPDATA%` — could be deleted with the whole suite green; it now has a test, and deleting either win32 root fails exactly the tests named for it. ## Fifth pass A fifth pass took the platform-branch shape the fourth pass exposed and ran it as its own move: *for every function that branches on `process.platform`, does the test's guard cover every arm, or only the one the author's machine takes?* Fixed in `e08b466b6`. - **`init-stale-config.test.ts` deletes the developer's real VS Code MCP config on Windows.** The file mocks `os.homedir()` and says it does so "so hidden-scope probes (`~/.claude.json`, VS Code user profile) never touch the real home directory". `vscodeUserDirs()` branches `darwin → homedir()/Library/Application Support`, `win32 → %APPDATA%`, `else → homedir()/.config` — the win32 arm never calls `homedir()`, so the mock is inert there, and the test's own `userMcpJsonPath()` mirrors the same branch. Both user-profile tests then write a one-entry fixture over `%APPDATA%\Code\User\mcp.json` with a whole-file `writeFileSync`, and assert that `remove()` deleted it. Reproduced by forcing `process.platform` to `win32` with a two-server `mcp.json` planted at a probe `%APPDATA%`: **`3 passed`, and the file and its directory were gone.** With `APPDATA` pinned, the whole file passes under the same instrument — 31 tests — and the planted config is byte-identical. - **`argent-tools-client` hands its ambient `ARGENT_*` to a real spawned child.** The launcher spawns an actual tool-server, and `buildToolsServerEnv` overrides only the keys it is given — everything else rides in on `...baseEnv`. `ARGENT_HOST` is the one that bites: the fixture binds the host it names while the launcher records and health-checks `127.0.0.1`, so reuse misses and a second server is spawned. With six overrides exported: **`10 failed | 181 passed`, and eight stray `fake-tool-server` processes still holding TCP listen sockets after the run.** With the prefix stripped: `191 passed`, none surviving. The package now carries the same setup file `tool-server` has, for the same reason — and it is the second of the two packages whose missing `ARGENT_*` strip this review found. ## Later passes A sixth pass audited the fix commits themselves and bounded the SDK probe (`6fbb66ce1`); a seventh ran the moves against the diff one more time and turned up the two below, plus the `listen(0)` defect filed as #853. - **The `PATH` pin made `workspace-reader-integration` POSIX-only without the guard its siblings got.** The shim it puts on `PATH` is an extensionless `node`, and `symlink()` to a file needs an elevated process or Developer Mode on Windows, so `beforeAll` throws `EPERM` there before any assertion runs; a copy in place of the symlink would not rescue it either, since a bare command name resolves through `PATHEXT`. This sweep already skips `android-binary` and `native-devtools-socket-bind` on win32 for exactly this class, and the Windows job runs a curated list of seven `tool-server` files that excludes this one, so nothing was catching it. Fixed in `095d75bd8` — verified POSIX `2 passed`, and under a forced win32 platform `2 skipped` with the top-level `beforeAll` never entered. - **A rationale that rested on a case the file excludes.** `android-binary`'s header justified pinning `USERPROFILE` by what `os.homedir()` reads on Windows, two sentences before noting that both describes skip on win32. Fixed in `f37b3605e`; the variable stays in the snapshot list, the justification no longer claims a platform this file never reaches. A seventh pass also re-ran the sweep against a hostile machine: every feature flag on in a scratch `~/.argent/flags.json`, no `adb` on `PATH`, and a `TMPDIR` long enough to overrun the 104-byte `sun_path` limit. It found one resource this branch had not closed. - **A red `launcher-version-gate` run strands a real tool-server.** `ensureToolsServer` spawns a fixture server, but its pid only reached the reaper after three or four assertions on the state it returned; any of those throwing left the pid unrecorded, and the same `afterEach` then deleted `STATE_DIR`, the only other record of what to kill. **Eight such processes were alive on the development machine**, spread from 14:33 to 18:23, every one from an `argent-version-gate-test-*` fixture and none from any other family — each still holding a TCP listen socket with its fixture directory long gone. Forcing one assertion to fail took the `tool-server.cjs start` count from 10 to 11. The nearest twin, `launcher-duplicate-spawn`, guards this twice: it records the pid straight after the null check and sets `FAKE_TTL_MS` so an escaped server self-exits. Fixed in `26c2c60ec` with both, the TTL also covering the window between the spawn and the state read where no pid exists yet — the same forced failure now leaves **zero** strays. An eighth pass audited the seventh's own fixes, and found the version-gate fix had been applied to one file when the defect was a property of four. - **Three more files strand a real tool-server on a red run.** `launcher-spawn` and `launcher-exit` set `FAKE_TTL_MS` nowhere; `launcher-duplicate-spawn` — the file `26c2c60ec` cites as the model — armed it inside one test's `try/finally`, so eight of its nine spawns ran without it and the `finally` disarmed it for the remainder of that test. Reproduced per file by forcing a failure before the pid is recorded: a detached node process survives, reparented to init, holding a listening socket, with its `TEST_HOME` removed and its state record already cleared. Fixed in `3bb69ded6` by moving the net to a file-wide `beforeEach` in all three. Verified end to end — the child carries `FAKE_TTL_MS=60000`, and a stranded server spawned at 21:28:36 exited by itself at 21:29:36 with nothing signalling it; three consecutive green runs stay at 191 passed. - **A 29th file writes the real `~/.argent`.** `boot-electron-spawn-error`'s success-path test completes a boot, and a completed boot calls `trackChromiumPort`, persisting to `os.homedir()/.argent/chromium-cdp-ports.json`. The real file on the development machine had accumulated **106 dead ephemeral ports**. Its two siblings in the same directory already mock `chromium-discovery` for exactly this reason; this was the third, with no mock. Fixed in `3306b6f40`. - **`scopeTempHome` was itself unpinned.** Deleting its `USERPROFILE` line — the Windows half, the same property the sibling helper's test pins — left all twelve consumers green at 48 passed. `3306b6f40` adds the missing test; it kills that mutation plus dropping the `HOME` pin and replacing the per-test `mkdtemp` with one shared directory. Two findings from that pass were refuted on verification and are recorded rather than acted on: the tool-server tmpdir leftovers (mechanism real, but they are the separate leak this body already scopes out) and a claim that `26c2c60ec`'s message overstated its twin. Four comments were also corrected against their code: `deriveReportPath` joins `<base>-report.md` *inside* the trace's directory rather than appending to the directory's name, which is what makes the `rm(traceDir)` added in `a67c43f8d` cover it; two `android-perfetto` comments still named `TMPDIR` as the mechanism after the switch to `redirectTmpdir`; and `android-binary`'s header claimed the home pin lets its two positive tests resolve "on either platform", though its fixtures are extensionless and the win32 resolver only accepts `.exe`. |
||
|
Ignacy Łątka
|
0a6dbec87c |
feat(telemetry): export events via OpenTelemetry OTLP logs instead of PostHog (#570)
## What
Swaps the `posthog-node` transport for an OpenTelemetry Logs exporter.
Everything above the transport stays as it is - opt-out consent,
per-machine identity and fingerprint, the sanitizer allowlist, the typed
event surface, crash diagnostics. Only the ~3-method client boundary
changes.
One analytics event becomes one OTLP/HTTP log record:
| PostHog | OpenTelemetry |
| --- | --- |
| `capture({ event, distinctId, properties })` | `logger.emit({ body:
event, attributes: { distinct_id, "event.name", ...props } })` |
| event name | log-record `body` |
| `distinctId` | `distinct_id` attribute |
| event/base properties | log-record attributes |
| `client.shutdown()` | `LoggerProvider.shutdown()` (force-flush +
teardown) |
Records go to a hard-coded Software Mansion collector. The endpoint and
the authorization header are passed to the exporter explicitly in code,
and an explicit value beats the corresponding `OTEL_EXPORTER_OTLP_*`
variable - the same anti-exfiltration property the fixed PostHog host
had.
## Changes
- `packages/telemetry/src/posthog.ts` becomes `otel.ts`:
`LoggerProvider` + `BatchLogRecordProcessor` + `OTLPLogExporter`
(HTTP/JSON). Queue size 20 and a 10 s flush interval mirror the old
client; the per-export deadline is 1.5 s, kept at or below `index.ts`'s
drain budget so a stalled export can't hold a short-lived command open.
- Dependencies: drop `posthog-node`, add
`@opentelemetry/{api,api-logs,sdk-logs,exporter-logs-otlp-http,resources}`.
They inline cleanly into the esbuild bundles.
- Build-time ingest token via esbuild `define`
(`ARGENT_OTEL_INGEST_TOKEN`), same mechanism as `ARGENT_CLI_VERSION`.
Unset in a dev build leaves the client unconstructed and telemetry
inert.
- Base props: drop `$process_person_profile` (a PostHog directive,
meaningless to a collector) and rename `$session_id` to `session_id`.
Null-valued attributes are dropped, since OTel rejects them -
`cloud_agent` is null on the common path, and absence means the same
thing.
- `Telemetry.md`: the "who we disclose to" and "international transfers"
sections describe a Software Mansion-operated OTLP collector instead of
PostHog as a third-party processor.
## A hang this uncovered
`timeoutMillis` only bounds a request once its socket is connected,
because the exporter applies it with `req.setTimeout()`. Against a
collector address that drops packets rather than refusing them - a
corporate egress filter, a dead host behind a firewall - the socket sits
in the connecting state where no export deadline reaches it, and the
process stays alive for the OS connect timeout: 75 s past a `shutdown()`
that had already resolved at 1.75 s.
Pairing the export deadline with an agent-level socket timeout, armed at
socket creation so it covers connection establishment too, fixes it:
| collector state | before | after |
| --- | --- | --- |
| healthy | 0.18 s, delivered | 0.18 s, delivered |
| connection refused | 0.94 s | 1.03 s |
| blackholed (http and https) | 75.0 s | 1.6 s |
| connected but never responds | - | 1.6 s |
`posthog-node` exited in 10.6 s under the same blackhole (undici aborts
a connecting socket), so this was a regression as well as a hang. A unit
test that fails without the fix guards it.
## Found in review
Each of these was reproduced before it was changed, and each is pinned
by a test that fails without the fix.
- **The environment's OTLP headers were forwarded to the collector.**
The SDK merges `OTEL_EXPORTER_OTLP_HEADERS` / `_LOGS_HEADERS` into every
request, keeping any key the code does not set itself. Out in the world
that variable holds the developer's *own* observability credential - a
Honeycomb team key, a Dynatrace `Api-Token`, Grafana Cloud basic auth -
so any machine already running OpenTelemetry shipped that third-party
secret here on every batch, without it passing the sanitizer or
appearing in `Telemetry.md`. Reproduced by watching `x-honeycomb-team:
hcaik_…` arrive at a capture server. The variables are now cleared
across the exporter constructor (where the SDK resolves its header set)
and restored after.
- **`httpAgentOptions` silently disabled connection reuse.** Supplying
it replaces the agent the SDK would build, whose default is `keepAlive:
true`. Measured on loopback: three batches shared one socket by default,
and opened three sockets with `httpAgentOptions` - a fresh TCP+TLS
handshake per 10 s batch in the long-lived tool-server. `keepAlive` is
restated alongside the timeout.
- **`markDisabled()`'s drain had no grace period.** It raced flat
against `SHORT_FLUSH_TIMEOUT_MS` while `shutdown()` allowed `timeoutMs +
250`. Since the exporter's own deadline equals that budget, the flat
race fired at the same instant and always abandoned the batch instead of
letting it finish. Both call sites share one `raceDrain()` now.
- **Export failures reached nobody.** The batch processor hands errors
to OpenTelemetry's global handler, so `LoggerProvider.shutdown()`
resolves identically for a delivered batch and a rejected token - making
`shutdown()`'s `catch` unreachable. The SDK's diagnostics are now routed
into the existing debug channel under `ARGENT_TELEMETRY_DEBUG=1`; a run
without the flag stays silent so no host application's diag logger is
disturbed.
- **The build-time token now beats the `__ARGENT_OTEL_TOKEN_TEST` seam**
rather than losing to it, so the seam reaches unbundled source only.
Verified with esbuild across all three build modes.
- **The endpoint guard test mocked the exporter it guards.** It could
only show that the code passes a `url`; an SDK upgrade that changed the
precedence would have kept it green. `otel-endpoint-live.test.ts` drives
the real exporter against loopback servers - dropping the explicit
`url`, the explicit `authorization`, or the header clearing each fails
it.
- **The wire shape had no pins at all.** Bypassing the sanitizer
entirely, sending an explicit `null`, renaming `service.name` and
dropping `event.name` all left the suite green. Four tests now fail on
those mutations. The sanitizer bypass was unpinned on `main` too.
## Before this ships
1. ~~**Collector URL**~~ - done in
|
||
|
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)
|
||
|
Ignacy Łątka
|
d4b9818d64 |
fix(debugger): deliver component-tree errors via binding + stop truncating log levels (#438)
Fixes two confirmed defects in the JS-runtime debugger subsystem.
## Defect A — `debugger-component-tree` hangs ~15s and throws on every
error condition
**File:**
`packages/tool-server/src/utils/debugger/scripts/component-tree.ts`
The injected fiber-walk script delivers its success result through the
`__argent_callback` CDP binding, but every error path instead did
`return JSON.stringify({ error: ... })` from the async IIFE — `'No
DevTools hook'`, `'No fiber roots'`, `'Could not find UIManager'`.
The consumer (`evaluateWithBinding`, `cdp-client.ts`) runs
`Runtime.evaluate` with `returnByValue:false`/`awaitPromise:false`, so
the IIFE's return value is discarded. The promise only settles when the
binding fires or the 15s timeout elapses. So on a release/production
build, before the first render, or on a non-RN runtime,
`debugger-component-tree` blocked for ~15s and then threw a generic
`DEBUGGER_CDP_BINDING_TIMEOUT` instead of returning the intended
diagnostic immediately.
**Observed:** callback never invoked (0 calls) on the error paths; the
tool hangs to the binding timeout.
**Expected:** the error is delivered through the binding right away and
surfaces as `Error: <message>` via the consumer's
`JSON.parse(response.result).error` path.
**Fix:** mirror the sibling `inspect-at-point.ts`. Added an
`__argent_fail(msg)` helper that routes the error through
`__argent_callback` in the exact same wire shape as the success path (`{
requestId, result: JSON.stringify({ error: msg }) }`), and converted all
three error paths to call it and `return`.
## Defect B — `LogFileWriter` truncates console levels longer than 5
chars
**File:** `packages/tool-server/src/utils/debugger/log-file-writer.ts`
The flat-line level was rendered as `LEVEL_DISPLAY[entry.level] ??
entry.level.toUpperCase().padEnd(5).slice(0, 5)`. The `.slice(0,5)`
truncated any level not in the 5-entry `LEVEL_DISPLAY` map. CDP's
`Runtime.consoleAPICalled.type` includes `"warning"` (`console.warn`)
and `"assert"` (`console.assert`) — neither is in the map — so
`readAll()`/`readFiltered()` returned `"warni"`/`"asser"`, and
`readFiltered({ level: "warning" })` matched nothing.
**Observed:** writing level `"warning"` → `readAll()[0].level ===
"warni"`; `readFiltered({ level: "assert" }).total === 0`.
**Expected:** the canonical level round-trips exactly for levels of any
length.
**Fix:** drop `.slice(0,5)` — keep `padEnd(5)` for column alignment but
never cut. `LINE_RE` captures the level as `\S+`, so a non-truncated,
whitespace-free level round-trips through `parseFlatLine` unchanged.
Short levels (log/info/warn/error/debug) are unaffected.
## Tests
- `packages/tool-server/test/debugger/comptree-error-channel.test.ts` —
asserts `__argent_callback` is invoked on the error paths and the error
is encoded as `parsed.error`.
- `packages/tool-server/test/debugger/log-file-writer-level.test.ts` —
asserts `"warning"`/`"assert"` round-trip and filter correctly, and
short levels still work.
Both new files fail before the fix and pass after. Full `tool-server`
suite green (1627 tests); repo-root `tsc --build` clean; eslint clean on
changed files.
|
||
|
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.
|
||
|
Ignacy Łątka
|
b5fc1f2a50 |
fix(tool-server): key Fabric component-tree measurement by nativeTag so tap coords don't collapse to 0.5,0.5 (#286)
## Problem
`debugger-component-tree` returned **every** element with the same tap
coordinate — `(tap: 0.50,0.50)` — on the New Architecture
(Fabric/bridgeless), making the tool's primary output (per-element tap
coordinates) unusable. Reproduced live on an RN 0.81.5 Fabric app; the
tree walk, skip stats, and screen size were all correct, only the
coordinates collapsed.
## Root cause
The injected script (`utils/debugger/scripts/component-tree.ts`)
batch-measures host views and caches each rect under a per-host key
built as `(hi.f ? 'f' : 'p') + hi.n`. On **Fabric**, `getHostInfo`
returns the shadow **node object** as `hi.n` (`{ f: true, n:
fiber.stateNode.node }`), so the key stringifies to `"f[object Object]"`
for *every* host. All hosts collapsed to a single cache entry → only the
first (root) view was measured → every component inherited the root's
full-screen rect → `(x + w/2)/screenW, (y + h/2)/screenH = 0.5, 0.5` for
all. Paper/old-arch was unaffected because there `hi.n` is already a
numeric `nativeTag`.
## Fix
Key the cache by a primitive `nativeTag` on Fabric
(`fiber.stateNode.canonical.nativeTag`, the same field the Paper branch
already uses), with a `WeakMap`-by-identity fallback so distinct shadow
nodes can never share a key even if `nativeTag` were ever absent.
`getHostInfo` now carries an explicit `key`, and the three cache-key
derivations use it. Paper/old-arch behavior is unchanged.
## Testing
**Live (first-hand), on a running RN 0.81 Fabric app** — walking the
live fiber tree:
| | Fabric host nodes | distinct measure keys |
|---|---|---|
| old key (`'f' + node`) | 2140 | **1** (`"f[object Object]"`) |
| new key (`'f' + nativeTag`) | 2140 | **2140** |
**Unit** — adds `test/debugger/component-tree-script.test.ts`, which
evaluates the injected script against a mocked Fabric tree (the existing
`component-tree.test.ts` only covered the pure post-processor with
pre-filled rects, which is why this shipped). The collapse is exercised
on both Fabric measure paths with **tagless** hosts (no numeric
`nativeTag`), so the cache key is forced onto the new
`WeakMap`-by-identity fallback — with a numeric tag the buggy `'f' +
node` key was already distinct and never collapsed. `vitest run` → 6/6
pass with the fix; reverting `component-tree.ts` to `main` fails both
collapse tests with `{ y: 100 }` where `{ y: 300 }` is expected (the
second component inheriting the first's rect — exactly the collapse).
|
||
|
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. |
||
|
Ignacy Łątka
|
ff49143d03 |
fix(debugger): make inspect-element work on old-arch RN (RN 0.81 bridge) (#344)
## Problem
`debugger-inspect-element` returned `{"error":"no host fiber"}` at every
coordinate on a React Native 0.81.5 app (Hermes, old architecture /
legacy bridge, React Compiler). Verified live on a real device.
## Root causes (three layers, each verified live)
1. **Host-fiber resolution** - `findHostFiber` only recognised old-arch
host fibers via `stateNode.canonical`, but RN 0.81 bridge host fibers
expose `stateNode._nativeTag` with no `canonical`, so it found nothing
and threw "no host fiber". Fixed by also matching `_nativeTag` (mirrors
the working `component-tree.ts` getHostInfo). On the real tree: 87/87
host fibers had `_nativeTag` and 0 had `canonical`.
2. **Wrong anchor** - even with the host fiber resolved, the first host
fiber is a sibling subtree that does not contain the touch point, so the
renderer's native `findSubviewIn` resolved nothing for *every*
coordinate (touchedViewTag undefined). Fixed by anchoring at the
**FiberRoot container** (its `containerTag`), the universal ancestor.
Fabric and older Paper (no containerTag) keep the host-fiber anchor.
3. **Changed data shape** - RN 0.81 no longer returns
`data.closestInstance`/`hierarchy`; it returns `data.componentStack` (a
React stack string). The old extraction produced empty results. Now
parsed into items; bundle source locations are symbolicated by the tool
layer exactly like `_debugStack` frames.
## Result
Live on a real RN 0.81 app, inspecting a point now returns the real
component hierarchy (170 frames: `SplashScreen`, `ErrorBoundary`,
`LoggedOut`, `NativeStackNavigator`, `HomeTabNavigator`, `SceneView`,
`NavigationProvider`, ...) with source locations.
## Safety / scope
- Fabric (new-arch) and iOS paths unchanged; canonical-publicInstance
handling is byte-for-byte preserved (incl. the lazily-null Fabric case).
- Distinct from #284 (symbolication fallback - a different, later
failure mode).
- Unit tests added for: `_nativeTag` host-fiber resolution, container
anchoring, componentStack parsing, and the Fabric null-publicInstance
no-regression case. The test harness evaluates the generated script, so
it exercises the real emitted code.
- Validated end-to-end against a live emulator, not only unit tests.
|
||
|
Ignacy Łątka
|
4999102238 |
fix(profiler): verbose errors when target app cannot be profiled (#201)
If you point the profiler or inspect-element tool at an app that can't actually be profiled (usually a release build with React DevTools stripped out), the tools currently blow up with a cryptic `Cannot read property 'X' of undefined` and leave the session in a weird half-started state. This PR makes them say what went wrong and what to do about it instead. Profiler and inspector tools used to crash with generic "Cannot read property 'X' of undefined" TypeErrors whenever they hit a runtime without `__REACT_DEVTOOLS_GLOBAL_HOOK__` (release/production builds where DevTools is stripped) or against a partial/malformed Hermes CPU profile. Surface the diagnosis up front instead, and stop leaving a half-initialised session for downstream tools to trip over. # Technical details - `inspect-at-point.ts`: guard the injected script for missing hook / renderer / fiber root and report through the existing `__argent_callback` error channel; export `INSPECT_NO_DEVTOOLS_HOOK_ERROR`, `INSPECT_NO_RENDERER_ERROR`, `INSPECT_NO_FIBER_ROOT_ERROR` so callers can recognise the same diagnosis without string-matching bespoke phrasings. - `react-profiler-start.ts`: replace the terse "production build" one-liner with the verbose `NO_DEVTOOLS_HOOK_ERROR` (rebuild in dev mode, three likely causes, concrete `run-ios` / Expo dev client hints). Export `NO_RENDERERS_ATTACHED_ERROR` for the distinct "hook present but no renderer registered" state. - `react-profiler-stop.ts`: refuse to stop when `api.profilingActive` is false (start failed before reaching the sampler), and validate the Hermes profile shape (`samples` / `nodes` / `timeDeltas` arrays) before handing it to the analyse pipeline. - `dump.ts` (`readCpuProfile`): validate shape inline at load so `profiler-load` of an older / partial dump fails fast with an actionable message instead of crashing `buildCpuSampleIndex` later. - `react-profiler-renders.ts` / `react-profiler-fiber-tree.ts`: branch on the in-runtime error code so "no `__REACT_DEVTOOLS_GLOBAL_HOOK__`" gets the rebuild-in-dev advice while "no renderers attached to hook" gets the wait-for-render / let-`react-profiler-start`-bootstrap advice — separating two failure modes that the old `HOOK_MISSING_MESSAGE` collapsed into one misleading message. ### Tests - New `inspect-at-point-guards.test.ts` — eval the injected IIFE against a controlled `globalThis` to verify each guard fires the verbose diagnostic and routes through `__argent_callback`. --------- Co-authored-by: Filip131311 <contact@filipkaminski.com> |
||
|
Alexander Brokking
|
232762ebd0 |
fix(tool-server): make debugger-component-tree work on New Arch / multi-renderer apps (#316)
## Summary
`debugger-component-tree` returns **"No visible components found on
screen"** on React Native **New Architecture (Fabric / bridgeless)**
apps that also use a secondary React reconciler such as
`react-native-skia` — even when the app is fully rendered and CDP is
connected. This PR fixes the two root causes so the tool returns the
real component tree with names, testIDs, and accurate tap coordinates.
## Root cause
The injected fiber-walk script
(`packages/tool-server/src/utils/debugger/scripts/component-tree.ts`)
made two assumptions that don't hold on modern RN:
1. **Hardcoded renderer id.** `hook.getFiberRoots(1)` assumes the React
Native renderer is DevTools renderer id `1`. Libraries that ship their
own React reconciler (`react-native-skia`, `react-native-svg`,
`react-three-fiber`, …) register a renderer too and frequently take id
`1`. On such apps `getFiberRoots(1)` returns only that library's roots
(e.g. Skia shapes), so the walk never reaches the app UI → `No visible
components`.
2. **`nativeFabricUIManager.measure` under bridgeless.** Layout was read
via `nativeFabricUIManager.measure(stateNode.node, …)`. Under
bridgeless, `globalThis.nativeFabricUIManager` is an **empty object**
(no `.measure`), so every measure threw and all rects became `null` —
components were then filtered out (and the few kept by testID reported
no usable rect).
## Fix
- **Renderer-agnostic root selection:** enumerate `hook.renderers`,
collect roots from every renderer, and walk the mounted root with the
largest fiber subtree (the real app UI) instead of hardcoding id 1.
- **Fabric layout via the public host instance:** measure through
`stateNode.canonical.publicInstance.measureInWindow()` (a
`ReactNativeElement`) — the **same approach this repo's
`inspect-at-point.ts` already uses** — falling back to
`nativeFabricUIManager.measure(shadowNode, …)` when that global is
functional (non-bridgeless).
- **Hang guard:** a per-measure 1200 ms timeout so `Promise.all` can't
stall on a measure callback that never fires (detached / off-screen
nodes).
No change to the output schema or the Paper (old-architecture) path.
## How to reproduce
On a Fabric + bridgeless RN app that also renders `react-native-skia`:
```
argent run debugger-connect --device_id <udid> --port <metro>
argent run debugger-component-tree --device_id <udid> --port <metro>
# before: "No visible components found on screen."
```
The DevTools hook reports multiple renderers — e.g. `getFiberRoots(1)`
is the Skia reconciler (only canvas nodes), while the real app UI lives
in a different react-native renderer that the hardcoded id never
reaches.
## Testing
- Full `npm run build` (`tsc --build`, project references) passes; the
modified file typechecks under `strict`.
- Verified live against a **React 19.2.0 / Fabric (bridgeless)** app
whose DevTools hook exposes 3 renderers (id 1 = `react-native-skia`
`0.0.1`; id 3 = `react-native` `19.2.0` holding the real ~2,900-fiber UI
tree).
Before (unpatched): `No visible components found on screen.` — even with
`--includeSkipped` and after forcing a fresh commit.
After (this change) — names + testIDs + accurate coords (example
genericized):
```
Screen: 440x956
OnboardingQuestion "<question text>" (tap: 0.50,0.17)
SelectableCard [testID=onboarding.option.yes] (tap: 0.50,0.84)
Text "Yes" (tap: 0.50,0.84)
SelectableCard [testID=onboarding.option.no] (tap: 0.50,0.93)
Pressable [testID=screen.back-button] (tap: 0.09,0.09) ← top-left ✓
TouchableOpacity [testID=dev-settings.trigger] (tap: 0.06,0.97) ← bottom-left ✓
```
Coordinates were spot-checked against the visible layout (back button
top-left, an off-screen/hidden trigger bottom-left).
## Notes
`inspect-at-point.ts` has the same hardcoded `getFiberRoots(1)` /
`renderers[0]` renderer assumption (its Fabric layout path already uses
`publicInstance`, which is what motivated this fix). I scoped this PR to
`component-tree`; happy to follow up on the inspect path separately.
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Filip131311 <contact@filipkaminski.com>
Co-authored-by: filip131311 <159789821+filip131311@users.noreply.github.com>
|
||
|
Filip131311
|
9267940391 | style: prettier formatting in inspect-at-point-script test | ||
|
Alexander Brokking
|
d2071fafc5 |
fix(tool-server): select the correct renderer in debugger-inspect-element (#317)
## Summary Companion to #316. `debugger-inspect-element` has the same hardcoded-renderer assumption as the component-tree script, so it inspects the wrong React renderer on multi-renderer apps. ## Root cause `makeInspectScript` (`packages/tool-server/src/utils/debugger/scripts/inspect-at-point.ts`) picks: ```js var renderer = Array.from(hook.renderers.values())[0]; var roots = hook.getFiberRoots(1); var root = Array.from(roots)[0]; ``` On React Native **New Architecture** apps that also use a secondary React reconciler (`react-native-skia`, `react-native-svg`, `react-three-fiber`, …), that library registers its own renderer and frequently takes id `1`. `renderer` then points at the Skia/SVG reconciler and `getFiberRoots(1)` returns only its roots, so `renderer.rendererConfig.getInspectorDataForViewAtPoint(...)` runs against the wrong tree and returns no app component at the touch point. ## Fix Enumerate `hook.renderers`, walk each renderer's roots, and select the **(renderer, root)** pair with the largest fiber subtree (the real app UI) before inspecting — then call `getInspectorDataForViewAtPoint` on that renderer. Falls back to the previous first-renderer behavior if enumeration yields nothing. Unlike the component-tree script, the Fabric layout path here was already correct (it uses `hostFiber.stateNode.canonical.publicInstance`), so no measurement change is needed — this PR is purely the renderer/root selection. ## Testing - Full `npm run build` (`tsc --build`) passes; the modified file typechecks under `strict`. - Same environment as #316: a React 19.2 / Fabric (bridgeless) app whose DevTools hook exposes a `react-native-skia` renderer at id 1 alongside the real `react-native` UI renderer. With the fix, inspection resolves against the app renderer instead of the Skia one. No change to the output schema or the Paper path. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Paweł Fornagiel
|
42cc5e3c09 |
Enhance error handling in registry and tool server blueprints (#115)
* feat: enhance error handling in registry and tool server blueprints * test: add tests for error handling in registry and tool server blueprints * refactor: streamline exception detail formatting in CDP client * format: run format |
||
|
Filip131311
|
8c2a313c76 |
chore: format codebase with Prettier
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Paweł Fornagiel
|
3d1a5dbe08 | feat: Enhance component tree filtering in debugger (#35) | ||
|
Paweł Fornagiel
|
0071a09d99 |
feat: add log registry for debugger console logs (#30)
* feat: first log registry implementation * feat: refactor skills to mention how to use new log registry * docs: update SKILL.md to include guidance for handling large log files * refactor: update log file handling, change to .log instead of JSONL |
||
|
Paweł Fornagiel
|
2fa515d628 |
feat: component tree & element inspector improvements (#26)
* feat: include skiplists * feat: allow for verbose logging in component tree and element-inspect * feat: trim the skills related to PR from unneeded information |