2 Commits

Author	SHA1	Message	Date
filip131311	b232114422	docs: audit every comment in src against the code it describes (#931) ... Audits every comment in `src/` and `scripts/` against the code it describes. **462 files, 57 commits, net −8,194 lines.** Comment text only — the whole branch is code-identical to `main`. ## Method One subagent per file, strictly sequential. Scope was `src/` + `scripts/` (459 files); three more files were added at the end because they carried dead references of the same kind — two test headers citing design docs that do not exist, and `publish-npm.yml` citing a retired workflow. Each agent verified every comment — line, block, JSDoc, file header, trailing — against the surrounding code and the rest of the repo, following identifiers, paths, tool ids, config keys, env vars and issue links to see whether they still exist and still behave as described. Rules: - **A false or misleading comment is deleted, not reworded.** If a claim could not be confirmed by reading the source, it went. That is why the deletion count is so much larger than the rewrite count. - Survivors are cut to the shortest form carrying something the code does not already say. Restatement, preamble, hedging, changelog prose and ASCII banners are gone; the non-obvious *why* stays. - Preserved byte-identical: license headers, pragmas and directives (`@ts-*`, `eslint-disable`, shebangs, `/// <reference>`), JSDoc tag tokens, everything inside a string or template literal, and the sole comment inside an otherwise empty block (ESLint `no-empty` counts a comment-bearing block as non-empty). ## Verification Every file passed two independent gates before being recorded as done: 1. `comments-only` — the required check. 2. A second comment-stripping comparator with a proper mode stack, written for this pass because `comments-only`'s flat scanner desyncs on nested template literals and quote-bearing regex literals and then reports comment lines as code changes. Two files hit that false FAIL (`utils/android-profiler/pipeline/index.ts`, `scripts/extract-tools.mjs`); in both the "changed code" it printed was literally `//` lines, and the second checker confirmed the code was byte-identical. After the last file, all 462 changed files were re-checked against `main` with the same comparator, rather than trusting any agent's self-report. **459 code-identical; 2 are non-code (`.svg`, `.md`); 1 intentional.** The intentional one is `packages/argent/scripts/bundle-tools.cjs`: the changed template literal *is* the comment header of the file it generates, `packages/native-devtools-android/src/bundled-meta.ts`. Fixing only the generated file would have been reverted by the next build, so the generator changed too — and it has been verified to reproduce the committed generated file byte-for-byte. ## Representative false claims removed Not wording nits — statements a reader would have acted on: - **Reversed directions.** `proxyStart`'s JSDoc had the tunnel backwards (it is a reverse tunnel: the host binds first and the simulator dials in). A `paste()` doc had the pasteboard copy direction reversed. - **Contradicted by the code below it.** A timeout budget multiplied by three where the probes run concurrently — the same comment said so six lines later. A "warn once" that warns on every call. A "binary search" that is a linear scan. - **Named things that do not exist.** A `vega-fast-cli` binary, a `finish-recording.ts`, a `publish-next.yml` workflow, two `profiler-react19-*.md` design docs, a `DebuggerTarget.ts`, a commit hash git does not know, two tool ids, an `ensureEnv` cycle. - **Wrong by construction.** "Welford accumulators" across four files where the code keeps naive `n`/`sum`/`sumSq`; `sum`/`sumSq` documented over `actualDuration` when reduce sums `selfDuration`; a strict-mode halving written `n/2` where the code ceils; field docs listing enum values the producers never emit. - **Guarantees the code does not make.** A validation matrix claiming to cover "EVERY tool" that skips flagless ones; a Pareto cutoff that `slice(0, 20)` makes inert; an idempotence claim where the real rule is at-or-ahead; a capability note describing a clean 400 the shape-based device resolver can never produce. - **Unverifiable assertions** about prebuilt binaries, external CLIs and the cloud SDK — deleted rather than kept as folklore, since nothing in the repo can confirm them. - **Stale numbers**: invented Android tool versions, hard-coded tool counts and description lengths that had drifted. ## Review A Fable agent reviewed both halves adversarially for over-deletion, misread code, `no-empty` hazards and byte-identity violations. Second-half verdict: **SHIP**, with two one-line restores, both applied in the final commit — the `npm view ""` rationale behind a blank-token guard, and the note that `argent-mcp` keeps a copy of `SECRET_PLACEHOLDER_MARKER` it cannot import. ## Code issues surfaced but deliberately not fixed This pass changes comments only. Eight genuine findings are logged for a follow-up: 1. `telemetry/src/consent.ts` — a non-ENOENT read error returns null and falls through to the default-on path, so file errors *can* silently flip telemetry on. 2. `chromium-server/navigation.ts` — `navigate()` is reachable from `POST /api/navigate` with only a `typeof === "string"` check; open-url's schema is a bare `z.string()`, so the "already validated by zod" premise never held. 3. `http.ts` — `constantTimeEqual` returns early on a length mismatch, so the auth token's length is observable. 4. `describe/index.ts:~114` — the ios-remote branch passes `{ isTvOs: false }` unconditionally, so a remote tvOS simulator takes the iOS ax-service path, though `isRemoteTvOsSimulator` exists and shake/paste do use it. 5. `devices/boot-device.ts` — `-crash-report-mode never` is passed unconditionally *and* appended again by the feature-detecting path, so every emulator spawn passes it twice. 6. `react-profiler/pipeline/04-rank.ts` — `PARETO_THRESHOLD_PCT` is dead: `slice(0, 20)` always wins. 7. `reaped-sessions.ts` — a user-facing hint string tells the agent that `react-profiler-start { force: true }` disposes the debugger and profiler session; it does not. Left byte-identical because it is a string literal, not a comment. 8. `utils/simctl-backend.ts` — `localSimctl` is exported with no importers anywhere. ## Docs No documentation change is needed: this pass touches only source comments, and no user-facing capability, tool, CLI flag, config key or flow-file behaviour changed. --------- Co-authored-by: filip131311 <f.kaminski2000@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-24 12:16:20 +02:00
Paweł Fornagiel	62d0fe0c73	refactor: overhaul React profiler - DevTools backend, session ownership, richer data (#156) ... ## What this PR does This refactor: - replaces the DFS fiber-walking hook inside the app's JS runtime with the **React DevTools backend** as the source of profiling data - adds a session-ownership system so two tool-server instances can't silently clobber each other - gives agent a new status tool for recovery after interruptions - improves what the profiling analysis report shows the agent I realise this a big pull request, but changes began to affect each other when it comes to registered scripts on the react-native side. This, all-in-all, should however cover our needs when it comes to the react profiler and if it proves sufficient - we will only have to iterate on small fixes afterwards. If I were you, I would skip over the implementation of scripts on the native-side and focus on what the tool-server does when it comes to profiling right now. --- ## Profiling workflow The overall flow has not changed from the agent's perspective - agent calls `react-profiler-start`, do something in the app, call `react-profiler-stop`. What changed is how the profiling data is collected. **Before:** `react-profiler-start` injected a custom `onCommitFiberRoot` hook that walked the live fiber tree at every React commit in DFS-like way and appended records to a global `__ARGENT_DEVTOOLS_COMMITS__` array in the app's JS heap. On stop, the array was fetched in chunks over CDP. **Now:** `react-profiler-start` calls `ri.startProfiling(true)` on the React DevTools backend's `rendererInterface`. React's own profiler accumulates commit data internally. On stop, a single `STOP_AND_READ_SCRIPT` call stops the backend and reads `ri.getProfilingData()` in one round-trip. The DevTools backend data is richer, more accurate, and doesn't require maintaining a bespoke fiber-walking implementation. --- ## Session ownership The profiler runs inside the app's JS heap. There's only one "slot": it used to be that when two tool-server instances (or the same instance after a process restart) both try to start profiling, the second call silently overwrites the first and all data from the first session is gone. Right now, there is an approach which blocks silent overtake of the profiling session. When `react-profiler-start` runs, it: 1. Reads the current state from the runtime using `READ_STATE_SCRIPT` (a pure, side-effect-free read). 2. If a session is already running, it reads the `__ARGENT_PROFILER_OWNER__` object written into `globalThis`, which contains `sessionId`, `startedAtEpochMs`, and `lastHeartbeatEpochMs`. 3. Classifies the existing session as **fresh** or **stale** using `classifyStaleness()` (stale = no heartbeat for > 5 minutes). If the session is fresh and you don't pass `{ force: true }`, start refuses and returns an `already_running` payload instead of overwriting. 4. If the session is stale or `force: true`, it calls `STOP_FOR_TAKEOVER_SCRIPT` to cleanly stop the prior session, then starts a new one. ### Heartbeats Each `sessionId` UUID is written to `__ARGENT_PROFILER_OWNER__` in the runtime at start. While profiling, any side query tool (`react-profiler-renders`, `react-profiler-fiber-tree`) that this same tool-server process calls will bump `lastHeartbeatEpochMs` via `HEARTBEAT_SCRIPT`. This keeps the session from being classified as stale as long as the owning process is still making calls. ### What the agent sees when something goes wrong Previously, if you called `react-profiler-start` into an already-running session you'd get a silent clobber or a confusing Hermes error. Now: - **Running session detected, still fresh:** `{ already_running: true, owner: { sessionId, startedAtEpochMs, lastHeartbeatEpochMs }, age_seconds, how_to_reclaim }` — the agent can read the owner timestamp and decide whether to tell the user to wait or force-reclaim. - **Running session detected, stale:** automatic takeover with a log note. - **No owner record (e.g. another DevTools client started the session):** also automatic takeover. ### New `react-profiler-status` tool A read-only tool that reports the current state without starting or stopping anything. Returns `session_status` as one of `"active"` / `"taken_over"` / `"stopped"` / `"no_react_runtime"`, plus the full owner record. Use this after a debugger disconnect, an unexpected error, or when the agent restarts mid-session to decide whether to continue, reclaim, or give up. The agent has instructions to use the tool whenever it is interrupted during the profiling by the user or otherwise. Will have to look into how much this is actually used. --- ## What changed in the data we collect The old hook read fiber fields directly (`fiber.type.displayName`, `fiber.memoizedProps`, `fiber.selfBaseDuration`, etc.) at commit time. This meant: - Names for **transient components** (modals, tooltips, navigation screens) that unmounted before stop were already lost - the fiber was gone. - Change-detection logic (which props changed, which hooks fired) was reimplemented manually against the fiber internals. The new approach reads `ri.getProfilingData()` which gives us the DevTools backend's own commit log. Each commit entry has `fiberActualDurations`, `fiberSelfDurations`, `changeDescriptions`, `timestamp`, and `duration` - the same data React DevTools Profiler shows in its browser panel. ### Fiber name cache for transient components To address the lost-name problem, `FIBER_ROOT_TRACKER_SCRIPT` now hooks `onCommitFiberRoot` and, immediately after the DevTools backend finishes processing each commit (synchronously, while the fiber is still in memory), populates `globalThis.__argent_fiberNames__` with `fiberID → displayName` entries. Because fiber IDs are monotonically increasing and never reused, this cache never goes stale. `STOP_AND_READ_SCRIPT` uses this cache as a fallback for any fiber whose name `getDisplayNameForElementID` can no longer resolve. ### Unattributed fiber tracking When a transient fiber renders during the session but still can't be named at stop time (not in the name cache, not in the DevTools instance map), its work is now **surfaced rather than silently dropped**. The stop tool computes `unattributedByCommit`: a per-commit tuple of `[commitIndex, droppedFiberCount, droppedSelfMs]`. This flows through to: - The session metadata file on disk (`unattributedByCommit` array). - The pipeline (`buildHotCommitSummaries` receives and indexes it). - The `HotCommitSummary` type gains `unattributedMs` and `unattributedFiberCount`. - The generated report shows a warning line per commit with unattributed work, e.g. `⚠️ 12ms unattributed — 3 fibers unmounted before stop`. - `react-profiler-stop` returns top-level `unattributed_ms`, `unattributed_fiber_count`, `unattributed_commit_count` in its response so the agent immediately knows if there's a gap. ### Actual vs. self duration now both surfaced The old report showed only `selfDuration` (exclusive render time). Both are now captured in `HotCommitComponentEntry` and rendered in the report as: ``` - `ComponentName` — 4.2ms self, 18.7ms w/children ``` A legend at the top of the report explains when to sum self and when not to sum inclusive time. The summary table also shows both columns. ### Component name unwrapping Display names from the DevTools backend can be wrapped: `Forget(Memo(MyButton))`, `ForwardRef(Icon)`, etc. The pipeline's render stage now strips these wrappers using `annotateComponentName()` and appends a human-readable tag, so the report shows `\`MyButton\` [React.memo + React Compiler]` instead of the raw internal string. The raw DevTools name is still used in all tool-call suggestions (since every pipeline stage keys on the original string). --- ## What scripts do what All injected JS scripts now live in a single file — `packages/tool-server/src/utils/react-profiler/scripts.ts` — grouped by responsibility: | Script | When injected | What it does | |---|---|---| | `REACT_NATIVE_PROFILER_SETUP_SCRIPT` | Every `react-profiler-start` (idempotent) | Wraps `ri.startProfiling` / `ri.stopProfiling` to track `isProfiling` state and `startedAtEpochMs` without clock skew; installs the `__argent_profilerHeartbeat` helper | | `FIBER_ROOT_TRACKER_SCRIPT` | On CDP connect (idempotent) | Intercepts `onCommitFiberRoot` to populate the `__argent_fiberNames__` commit-time name cache | | `READ_STATE_SCRIPT` | Every `react-profiler-start`, `react-profiler-status` | Pure read: returns `{ hookExists, rendererInterfaceFound, isRunning, owner, nowEpochMs }` | | `buildStartScript(ownerJson)` | `react-profiler-start` on the happy path | Calls `ri.startProfiling`, writes owner record, returns post-start verification flags | | `STOP_FOR_TAKEOVER_SCRIPT` | `react-profiler-start` on the takeover path | Stops the prior session cleanly before starting a new one | | `HEARTBEAT_SCRIPT` | `react-profiler-renders`, `react-profiler-fiber-tree` | Bumps `lastHeartbeatEpochMs` while the owning process is still active | | `STOP_AND_READ_SCRIPT` | `react-profiler-stop` | Stops the backend profiler and reads `getProfilingData()` + name cache in one round-trip | | `RESOLVE_FIBER_META_SCRIPT` | `react-profiler-stop` (if commits > 0) | Walks the live fiber tree to collect `hookTypes`, `parentName`, and `isCompilerOptimized` metadata per component |	2026-04-29 19:20:50 +02:00