diff options
| author | amankumarsingh77 <amankumarsingh7702@gmail.com> | 2026-08-02 19:17:20 +0530 |
|---|---|---|
| committer | amankumarsingh77 <amankumarsingh7702@gmail.com> | 2026-08-02 19:17:20 +0530 |
| commit | da64520016e1631d21a5e393cec6b1a51e72a2a3 (patch) | |
| tree | 538a18b2391517113a9984b8074ded13bf9dfec0 /docs/superpowers/reports | |
| parent | f20fdf7fc4039ea71f3a8b56c7c508e46f1bbc51 (diff) | |
docs: remove the docs/ folder
The plans, specs, and audit reports under docs/superpowers/ described work
that has since landed, and ARCHITECTURE.md duplicated what the boundary
scanner and the tool contract already enforce in code. CONTRIBUTING.md now
points at the source that enforces each rule instead of at prose about it.
.gitignore keeps docs/ ignored so local scratch docs stay out of the repo.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Diffstat (limited to 'docs/superpowers/reports')
| -rw-r--r-- | docs/superpowers/reports/2026-07-26-bug-audit.md | 1488 |
1 files changed, 0 insertions, 1488 deletions
diff --git a/docs/superpowers/reports/2026-07-26-bug-audit.md b/docs/superpowers/reports/2026-07-26-bug-audit.md deleted file mode 100644 index 4055adb..0000000 --- a/docs/superpowers/reports/2026-07-26-bug-audit.md +++ /dev/null @@ -1,1488 +0,0 @@ -# Bug audit sweep — `src/` read-every-file pass - -Task 15, Steps 1–2. Branch `refactor/codebase-typesafety`, HEAD `4909192`. -No `src/` or `test/` file was modified by this pass. This document is the deliverable. - -**Coverage.** Every `.ts` file under `src/` was read in full (80 files, ~10.8 kLOC): -`cdp/` (17), `daemon/` (5), `domains/` (30 incl. `search/`, `readpage/`), `profile/` (7), -`schemas/` (2), `util/` (10), and the 7 root files. Also read: `scripts/check-boundaries.ts`, -`package.json`, and the test tree layout. Depth notes are in *Coverage and confidence* at the end. - -**Numbering.** F1…F36, ordered by severity. The summary table is at the bottom. - -> **Line numbers in this report are stale.** They were recorded against `4909192`. The comment -> strip (`c34bdc4`…`630e056`) removed roughly 1,400 comment lines from `src/`, shifting almost -> every line in the files below, and the Task 15 fixes changed several of them further. Locate a -> finding by the **symbol** named in it, not by the line number. Findings marked fixed carry -> their commit; the rest are open. - ---- - -## Resolution status - -The user ruled on 2026-07-28: fix six findings, leave the rest documented. - -| Finding | Status | Commit | -|---|---|---| -| — | test infrastructure (`makeFakeClient`) | `1d4ae38` test: fake browser client over a stub transport | -| F3 | **fixed** | `d10e729` fix(cdp): re-enable domains when re-attaching to a known tab | -| F4 | **fixed** | `7c6b3fd` fix(cdp): route background-tab events to the tab they belong to | -| F1 | **fixed** | `2ff5459` fix(tool): stop the socket gate rejecting every tool call on Windows | -| F6 | **fixed** | `fe90a9c` fix(domains): keep element refs stable across a mutation | -| F5 + F10 | **fixed** (one engine) | `16e7a30` fix(domains): one fill engine, so browser_fill stops lying about checkboxes | -| F2 | **fixed** | `77086bb` fix(domains): report a page-side throw instead of blaming the ref | -| F12 | **left unfixed by ruling** — needs one live-Chrome experiment | — | -| all others | documented only, unchanged | — | - -Every fix was driven by a test that first failed for the reason recorded here. -Test count went 206 → 251 passing (8 skipped, 0 failing); `npm run check` is green. - -### Observable behaviour changes shipped - -Three, all in the fill engine commit, and all deliberate: - -1. **`browser_fill` on a disabled input now errors instead of returning success.** - User-approved. The old success was the bug, and `browser_fill_form` already - refused it. -2. **`browser_fill` on a checkbox/radio now toggles `checked`** instead of writing - the submit value and reporting a no-op as success (F5). -3. **`browser_fill_form` on a contenteditable now uses `execCommand("insertText")`** - rather than assigning `textContent`, so it drives ProseMirror/Slack-style - editors the way `browser_fill` already did. This was not in the ruling's - enumerated list — it is the unavoidable consequence of the two tools sharing - one engine, and it is the better of the two behaviours, so it stayed. Flagging - it because it changes what `browser_fill_form` does to a rich-text editor. - -Minor, non-behavioural: both fill tools now return `tag` in `details`; the six -domain-enable round trips now also run on a re-attach (F3), and tab registration -moved ahead of them (F4). - -Deliberately *not* changed while fixing these: the object-handle leak (F29) still -exists on the fill/focus/select ref paths — routing them through `resolveAndCall` -would have fixed it for free, and I kept them on `runOnElement` precisely so this -pass did not quietly fix an out-of-scope finding. - -**Reproducibility.** `test/cdp/session-call-test.ts` already contains a stub `CdpTransport` -with a controllable `events()` iterable and a per-method canned-response map. That harness is -enough to reach most of the `cdp/` and `domains/` findings below without a live browser, which is -why so many are marked reproducible. There is no equivalent fake for `BrowserClient`; findings -that need one say so. - ---- - -## F1 — Every browser tool fails on Windows: `access()` on a named pipe - -> **FIXED** in `2ff5459`. Both call sites now share `daemonSocketMayExist(platform, socketPath)` -> in `src/daemon/spawn.ts`, following the precedent the same file already set. -> Tests: `test/util/daemon-socket-test.ts` pins the platform matrix — a `\\.\pipe\…` -> path on `win32` is never judged absent, while POSIX still consults the filesystem. -> **Still unverified on Windows hardware**: the tests pin the platform *branch*, not -> Chrome's behaviour on Windows. One run of any `browser_*` tool on a real Windows -> box is still the only thing that closes this out. -> The gate's own branch could not be tested deterministically — it calls -> `daemonSocketMayExist()` with no arguments, and this machine has a live -> `/tmp/pi-browser-daemon.sock`, so a gate test asserting "handler skipped" passes or -> fails depending on the developer's machine. I dropped that assertion rather than -> ship an environment-dependent test; the gate is covered only through the helper. - -**Location** `src/util/tool.ts:143-157` (guard), `src/daemon/protocol.ts:143-146` (the path). - -**What breaks.** `registerBrowserTool`'s `execute` gates every tool on -`await access(DAEMON_SOCKET_PATH)`. On Windows `DAEMON_SOCKET_PATH` is -`\\.\pipe\pi-browser-daemon` — a named pipe, which is not a filesystem entry, so -`GetFileAttributesW` (what libuv's `access` resolves to) cannot see it and `access` rejects. -The guard therefore fails for a perfectly healthy daemon and every `browser_*` call returns -`not_connected: "Browser harness not initialized. Run /browser-setup first"`. - -The codebase already knows this. `src/daemon/spawn.ts:44-51` skips the identical pre-check -for exactly this reason, with the comment *"Windows named pipes are not filesystem entries — -`access()` always fails there — so skip the pre-check and probe directly."* `tool.ts` is the -same mistake in the one place it is fatal. - -**Trigger.** Any `browser_*` tool call on `process.platform === "win32"`, with or without a -running daemon. `browser_setup` and `browser_http_get` still work (`ensureAlive: false`). - -**Impact** The harness is non-functional on Windows, and the error message misdirects the user -to a wizard that cannot fix it. Windows is clearly meant to be supported — the code carries -`.cmd` shim handling, `windowsVerbatimArguments`, PowerShell CIM process detection, and -`LOCALAPPDATA` profile paths. - -**Severity** crash (total loss of function on one platform). - -**Proposed fix.** Replace the `access` pre-check with the platform-correct liveness probe that -already exists: on POSIX keep `access`, on Windows skip straight to `client.ensureAlive()` -(whose transport `connect` already reports `ENOENT` as "Daemon not running"). Factoring the -POSIX-only branch out of `spawn.ts:45-51` into one shared helper would prevent a third copy. - -**Observable behaviour change.** Yes, on Windows only: tools start working. On POSIX, none. - -**Reproducible in a test?** Yes, but only after extraction. As written the check is inline in -`execute` and reads `process.platform` transitively through a module constant. Extract -`daemonEndpointReachable(): Promise<boolean>` (or pass the path in) and a test can assert that a -`\\.\pipe\…`-shaped path is not rejected on the strength of `access` alone. Asserting the real -Windows behaviour needs Windows. - -**Confidence.** High on the mechanism, **unverified on Windows hardware** — I could not run it -here. Corroborated by `spawn.ts`'s own comment, which the earlier work treated as a verified fact. - ---- - -## F2 — `runOnElement` ignores `exceptionDetails`: a page-side throw is reported as "No element matched" - -**Location** `src/domains/form.ts:30-62`; live at `:142` (fill), `:205` (focus), `:290` (select). - -> **FIXED** in `77086bb`. `runOnElement` now checks `exceptionDetails` and returns -> `cdp_error` carrying the exception text. Per the ruling, the error is -> distinguishable from both neighbours: `cdp_error` for a throw, `invalid_state` -> "No element matched" for a genuine miss, `invalid_state` "unknown or stale" for a -> bad ref — all three asserted in `test/domains/form-errors-test.ts`, which failed -> first with `invalid_state` for all three tools. The fix deliberately did **not** -> route the ref path through `resolveAndCall`, which would also have silently fixed -> the out-of-scope handle leak (F29). - -*Pre-confirmed item — I agree it is real. I do not rate it the single most severe item; see -"Ranking" below.* - -**What breaks.** The ref path calls `cdpCall(client, "Runtime.callFunctionOn", …)`. `cdpCall` -(`src/domains/cdp-call.ts:11-13`) only inspects transport success. A CDP command that *succeeds* -while the page function *throws* returns `{ result: {type:"undefined"}, exceptionDetails: {…} }`. -`runOnElement` returns `ok(r.data.result.value)` = `ok(undefined)`; the handler's -`isFillResult(undefined)` is false, so it takes the `res === undefined` branch and reports -`` `No element matched: ${target}` ``. - -**Trigger.** Any exception inside `FILL_FN` / `FOCUS_FN` / `SELECT_FN` on the ref path. Concretely: -`SELECT_FN` starts with `arg.wv` — if the argument ever arrives as `undefined` the very first -statement throws; `FILL_FN` calls `el.dispatchEvent`, which throws if a page overrode it or if a -CSP/permissions error surfaces; a cross-origin `contenteditable` focus can throw. Also every -future edit to those function bodies. - -**Impact** A **wrong diagnosis**. The agent is told the ref is bad and re-snapshots a ref that was -fine, then loops. The real error text is discarded. Compare `resolveAndCall` -(`form.ts:361-363`), which does check `exceptionDetails` and reports -`` `page function threw: …` `` — the two code paths in the same file disagree. - -**Severity** wrong result (misdiagnosis, agent-visible loop). - -**Proposed fix.** Check `r.data.exceptionDetails` in `runOnElement` before reading -`result.value` and return `{ kind: "cdp_error", message: \`page function threw: …\` }`. Better: -delete `runOnElement`'s ref branch and route it through `resolveAndCall`, which already checks -`exceptionDetails` *and* releases the object handle (F29). - -**Observable behaviour change.** Yes — a previously-mislabelled failure now returns -`cdp_error` with the page's own message. - -**Reproducible in a test?** Yes. Needs a fake `BrowserClient` whose `session().call` returns -`ok({ result: { type: "undefined" }, exceptionDetails: { text: "boom" } })`. No such fake exists -yet; the stub transport in `test/cdp/session-call-test.ts` plus a thin client shim gets there. - ---- - -## F3 — `switchTo` back to a previously-visited tab silently stops collecting console, network, and page events - -> **FIXED** in `d10e729`. `enableDomains` is now called on every attach, not only a -> tab's first visit. Tests: `test/cdp/session-switch-test.ts`, which failed first -> with **zero** `.enable` calls on the returned-to tab's new session. - -**Location** `src/cdp/session.ts:350-382`, specifically `368-377`. - -**What breaks.** `switchTo` calls `attachTo` (`Target.attachToTarget` with `flatten: true`), -which mints a **new** `sessionId` — the code knows this: `:369` says *"Each -Target.attachToTarget produces a new sessionId — update it."* CDP domain enablement is -**per-session**. But `enableDomains(attachedSessionId)` runs only in the `else` branch, i.e. only -the first time a tab is visited. On a revisit the new session is left with `Page`, `DOM`, -`Runtime`, `Network`, `Accessibility`, and `Log` all **disabled**. - -**Trigger.** `browser_switch_tab` to A → `browser_switch_tab` to B → `browser_switch_tab` back -to A. From that moment, on tab A: `Runtime.consoleAPICalled`, `Log.entryAdded`, and every -`Network.*` event stop arriving, so `browser_console` and `browser_network_requests` return -"_No matching … captured yet._" for a tab that is generating traffic. `Page.loadEventFired` / -`Page.frameNavigated` also stop, so `pageInfoDirty` is never set and `client.pageInfo()` serves -the 1-second-TTL cache without invalidation. `Page.javascriptDialogOpening` stops too, so a -dialog on the re-visited tab is never captured and `browser_page_info` cannot warn about it. - -**Impact** Silent, permanent-for-that-tab loss of all diagnostic data, plus a missed dialog -(which blocks every subsequent interaction with no explanation). The tool descriptions promise -the opposite: *"Buffer is reset on tab switch"* implies collection resumes after a switch. - -**Severity** wrong result (silent data loss) — with a plausible path to a hard hang via the -missed dialog. - -**Proposed fix.** Call `enableDomains(attachedSessionId)` unconditionally in `switchTo`, for the -`existing` branch as well. Re-enabling an already-enabled domain is a no-op in CDP, so hoisting -the call above the if/else is safe and simplest. - -**Observable behaviour change.** Yes — events resume; up to six extra CDP round-trips per -re-visit (the `TODO(perf)` at `:207` already notes these are sequential). - -**Reproducible in a test?** Yes, cleanly, with the existing stub transport: `switchTo("a")`, -`switchTo("b")`, `switchTo("a")`, then assert `calls` contains `Log.enable` for the third -`sessionId`. This is the single easiest finding in the report to pin down. - -**Confidence.** High on the code shape. The premise "a second `attachToTarget` mints a new -sessionId and domain state is per-session" is standard CDP behaviour and is asserted by the -code's own comment, but I did not confirm it against a live browser. - ---- - -## F4 — The event loop discards every event from non-active tabs, defeating the per-tab buffers - -> **FIXED** in `7c6b3fd`. Handled with the extra care the ruling asked for: before -> widening the filter I traced every `switch` arm against a background-tab event. -> Three arms needed to change together, and widening the filter alone would have -> introduced a new bug — -> - `Page.javascriptDialogOpening` used the *active* `targetId`, so a background -> tab's dialog would have been recorded against the foreground tab and -> `browser_page_info` would have pointed the agent at the wrong tab. Now -> `resolveTab(ev.sessionId)`. -> - `Page.frameNavigated` / `loadEventFired` had the same shape: the wrong tab's -> page-info cache invalidated and the right tab's left stale. Now `resolveTab`. -> - `Target.targetCreated` / `targetDestroyed` are browser-level with no sessionId, -> so they are unaffected either way; the `targetDestroyed` exemption was kept. -> - The `Network.*` / console arms already used `resolveTab` and simply start -> receiving events. -> -> One ordering hazard the widening exposed: with the filter keyed on -> `sessionIdToTargetId`, events arriving during the six domain-enable round trips -> would have been dropped as "unknown", so tab registration moved ahead of -> `enableDomains` in both `attachFirstPage` and `switchTo`. -> Tests: `test/cdp/session-events-test.ts` — four failed first (console, network, -> dialog and load events from a background tab all discarded); the fifth, asserting -> that a session this instance never attached is still ignored, passed before and -> after and is the guard that the filter did not become a free-for-all. -> No design decision turned out to be needed, so I did not stop. - -**Location** `src/cdp/session.ts:88-94`. - -```ts -if (ev.method !== "Target.targetDestroyed") { - if (ev.sessionId && ev.sessionId !== sessionId) continue; -} -``` - -**What breaks.** `sessionId` is the *active* tab's session. Every other attached tab's events are -dropped at the top of the loop. Yet the same loop then routes through -`resolveTab(ev.sessionId)` (`:151-178`), and `TabSession` carries a per-tab `networkBuffer` and -`consoleBuffer` explicitly so that *"switching tabs preserves the previous tab's data in its -TabSession instead of clearing it"* (`:13-16`). Those two designs contradict each other: the -per-tab buffers can only ever hold data captured while their tab was the active one. - -Tabs stay attached after a switch (`tabs` and `sessionIdToTargetId` are never pruned on switch), -and `browser_open_urls` / `browser_read_page` attach extra sessions of their own, so there is -real traffic being thrown away. - -**Trigger.** Switch to tab B while tab A is loading; switch back to A; call `browser_console` or -`browser_network_requests`. Everything A logged or fetched while B was active is absent, with no -`bufferOverflowed` flag or any other signal that data is missing. - -**Impact** wrong result. Silently incomplete diagnostics — the failure mode the console and -network tools exist to prevent. Note this **masks F3**: with the filter in place, the missing -domain-enable on a revisit is partly invisible, and fixing either one alone gives a partial fix. - -**Severity** wrong result (silent data loss). - -**Proposed fix.** Narrow the filter to what it is actually for. Keep dropping events for -sessions this `CdpSession` does not know (`!sessionIdToTargetId.has(ev.sessionId)`), and let -known sessions through to `resolveTab`. Then move the three handlers that use -`targetId ? tabs.get(targetId)` (`javascriptDialogOpening` at `:97`, `frameNavigated` / -`loadEventFired` at `:117`) onto `resolveTab(ev.sessionId)` too, or a background tab's dialog -will be recorded against the foreground tab. - -**Observable behaviour change.** Yes — background tabs accumulate console/network records, and -a background tab's dialog is attributed to the right tab. - -**Reproducible in a test?** Yes. The stub transport's `events()` is a generator; replace it with -a pushable queue, `switchTo("a")`, `switchTo("b")`, push a `Runtime.consoleAPICalled` carrying -tab A's sessionId, `switchTo("a")`, assert `drainConsoleBuffer` returns it. - ---- - -## F5 — `browser_fill` on a checkbox sets `value`, not `checked`, and reports success - -> **FIXED** in `16e7a30`, together with F10 — one engine in -> `src/domains/fill-engine.ts`, used by both tools. A checkbox now goes through the -> click/`checked` path instead of the `value` setter. -> Tests: `test/domains/fill-engine-test.ts` executes the real in-page engine source -> in Node against fake DOM elements — the "pure branch-decision seam" this report -> recommended. 9 of its 16 assertions failed first, including the headline one: -> after `browser_fill(checkbox, "true")`, `checked` was still `false`. - -**Location** `src/domains/form.ts:90-105` (`FILL_FN`). - -*Pre-confirmed item — verified, and I agree it is the worst row of the two-engines table.* - -**What breaks.** `FILL_FN` branches on `el instanceof HTMLInputElement` and, for any input, -fires `HTMLInputElement.prototype.value`'s setter. For `type="checkbox"` / `type="radio"` that -writes the element's *`value` attribute* — the string submitted when the box is ticked — and -leaves `checked` untouched. It then dispatches `input` + `change` (so a framework listener runs -and reads the *unchanged* checked state) and returns -`{ status: "ok", kind: "checkbox", value: el.value }`, which the handler renders as -`` Filled e7 = "true" ``. - -**Trigger.** `browser_fill({ ref: <a checkbox>, value: "true" })`. There is nothing in the tool -description or `promptGuidelines` steering the agent away from it — `browser_fill` is -described as the field-filling default, and `browser_snapshot` gives checkboxes refs like every -other interactive element. - -**Impact** A **silent no-op reported as success**, plus corruption of the checkbox's submit -value. The agent reads `Filled e7 = "true"`, believes the box is ticked, and moves on. The -appended `interactiveDiff` does compare `state`, so a careful reader might notice no -`checked` transition — but the primary result line says success. - -**Severity** wrong result — the most damaging failure mode in the report, because it is silent -and the agent has no reason to doubt it. - -**Proposed fix.** Add a checkbox/radio branch to `FILL_FN` that either delegates to the -`CHECK_FN` logic (`form.ts:379-394`, which clicks and falls back to `el.checked = want`) or -returns `{ status: "not_fillable", tag }` with a message pointing at `browser_set_checked`. -Failing loudly is strictly better than the current silence; delegating is better still, since -`browser_fill_form` already accepts a boolean for the same element (F10). - -**Observable behaviour change.** Yes, and deliberately: a call that reported success now either -works or errors. - -**Reproducible in a test?** The JS body is a string with no direct unit-test seam. Two options: -(a) reproduce in `test/manual/forms-test.ts` against live Chrome — proves it, doesn't guard it; -(b) restructure so the branch decision is a pure function of `{ tag, type, isContentEditable }` -and unit-test that. I recommend (b) — it is the only way this class of bug stays fixed, and it -covers F7 in the same seam. - ---- - -## F6 — Every mutation silently renumbers the ref map, so refs held by the agent can point at different elements - -> **FIXED** in `fe90a9c`. `interactiveDiff` now carries each ref over by -> `backendNodeId` (via a new `session.refMappings()`) and only mints a new `eN` for an -> element it has not seen; the counter starts above every existing number so a new -> element can never take a live ref. -> -> **Does this make refs genuinely stable, or narrow the window?** The ruling asked for -> an explicit answer. **Genuinely stable for any element that persists across the -> mutation** — which is the case the contract is about and the case that was silently -> wrong. Two honest residuals: -> - An element that disappears and later returns gets a **new** ref, because its old -> ref is dropped from the map when it vanishes. The agent sees it as new (`*[eN]`), -> which is truthful. -> - `browser_snapshot` still renumbers from scratch, by design: an explicit -> re-snapshot is the documented way to get a fresh ref set. -> -> So the F6 divergence between `browser_snapshot`'s `interestingOnly`/`maxNodes` and -> `interactiveDiff`'s hardcoded values no longer *renumbers* anything — a node absent -> from the diff's narrower tree simply keeps no ref until the next snapshot. -> Tests: `test/domains/ref-stability-test.ts`. Two of four failed first, with `e1` -> resolving to backendNodeId 999 (the newly inserted element) instead of 101. -> While writing them I found my own first fixture was wrong, not the product: real -> CDP AX nodes carry `parentId`, and without it `buildTree` treats every child as an -> additional root and collects it twice. Fixture corrected; no product bug there. - -**Location** `src/domains/ref-resolve.ts:75-94` (`interactiveDiff`), called from -`form.ts:160` (fill), `form.ts:307` (select), `click.ts:83` (click). -Compare `src/domains/snapshot.ts:332-344,371-376`. - -**What breaks.** After each mutation, `interactiveDiff` re-reads the AX tree, re-runs -`collectInteractiveTargets`, and assigns refs by **positional index** — `e1, e2, …` in tree -order — then calls `session.setRefMap(refMap, refSig)`, replacing the map wholesale. If the -mutation changed the interactive element set (an error message with a link appears, a -conditional field renders, a button unmounts), every ref at or after the insertion point now -resolves to a **different element**. Not stale — *wrong*. `resolveRefToObjectId` cannot detect -this: it looks up `e6`, gets a live `backendNodeId`, resolves it, and fills the wrong field. - -There is a second, independent divergence: `browser_snapshot` builds the tree with the -caller's `args.interestingOnly` / `args.maxNodes`, while `interactiveDiff` hardcodes -`{ interestingOnly: true, maxNodes: 1000 }`. A snapshot taken with `interestingOnly: false` or -`maxNodes: 5000` produces a *different numbering*, which the first mutation then replaces — -so refs shift even when the DOM did not change at all. - -**Trigger.** The documented happy path in the system prompt (`src/prompt.ts:64-72`): -snapshot once, then `browser_fill(e7)`, `browser_select_option(e9)`, `browser_click(e12)`. -Any validation message or conditional field appearing after the first fill shifts `e9` and -`e12`. Reproducible deterministically with `browser_snapshot({ interestingOnly: false })` -followed by any fill. - -**Impact** wrong result, silent, and it contradicts the contract the prompt and all seven tool -descriptions state: *"refs survive re-renders"*, *"refs are keyed to element identity"*. -What actually survives is the *backendNodeId behind a ref*, not the ref's meaning. - -**Severity** wrong result. High: it targets the wrong element while reporting success, and it -sits directly on the recommended workflow. - -**Proposed fix.** Make refs identity-keyed rather than positional. Keep a per-tab -`backendNodeId → ref` table and, when re-publishing in `interactiveDiff`, reuse the existing -ref for any backendNodeId already known, minting `eN` only for genuinely new nodes. That -preserves the diff (which already keys on `role|name`) and makes the "refs survive" claim true. -Minimum viable alternative: have `interactiveDiff` *not* replace the map — only add newly -appeared nodes — and use the same `buildTree` options the last snapshot used. - -**Observable behaviour change.** Yes — ref numbers stay stable across mutations, and the -`*[eN]` markers in the diff change to reflect stable numbering. - -**Reproducible in a test?** Yes, at the pure level and without a browser: -`buildTree(fixtureA)` → `collectInteractiveTargets` → refs; `buildTree(fixtureB)` where B is A -plus one interactive node near the top → assert the trailing refs still map to the same -`backendDOMNodeId`s. `test/deep-research/` already keeps AX-shaped JSON fixtures, so the -pattern exists. - ---- - -## F7 — `browser_fill` and `browser_select_option` write through a **disabled** control and report `status: "ok"` - -**Location** `src/domains/form.ts:90-122` (`FILL_FN`), `:254-268` (`SELECT_FN`). - -*Pre-confirmed item — verified.* - -**What breaks.** Neither function body checks `el.disabled`. `FILL_FN` fires the prototype -`value` setter, which works fine on a disabled input; `SELECT_FN` assigns `el.value` on a -disabled `<select>`. Both dispatch `input`/`change` and return `ok`. A real user cannot type -into either control, so the page's own state machine never expected the value, and the value is -not submitted. - -**Trigger.** `browser_fill` on a field the form disables until a precondition is met — a "Submit" -step that stays disabled until terms are accepted, a dependent select that unlocks after a -country is chosen. - -**Impact** wrong result. Same silent-success class as F5. `browser_fill_form` on the very same -element returns `{ ok: false, reason: "element is disabled" }` (`form-batch.ts:10`), so the two -tools give opposite answers about the same DOM. - -**Severity** wrong result. - -**Proposed fix.** Add `if (el.disabled) return { status: "not_fillable", tag }` — or a dedicated -`{ status: "disabled" }` so the message can say *why* — to both bodies, matching -`form-batch.ts:10` and `CHECK_FN` (`form.ts:385`), which both already do this. - -**Observable behaviour change.** Yes: a previously "successful" write now errors. That is the -point, and it is the behaviour `browser_fill_form` already has. - -**Reproducible in a test?** Same seam as F5 — pure branch-decision function, or live Chrome. - ---- - -## F8 — `browser_fill` reports `verified` without ever comparing it to the requested value - -**Location** `src/domains/form.ts:160-164`. - -*Pre-confirmed item — verified, and I rate it lower than the pre-confirmed list implies.* - -**What breaks.** The handler returns `details: { value: args.value, verified: res.value }` and -text `` Filled ${target} = ${JSON.stringify(res.value)} ``. `res.value` is `el.value` read back -after the write, but nothing compares it to `args.value`, and no mismatch is flagged. The tool -description promises *"Returns the field's value after writing, plus a compact diff"* — literally -true — while `promptGuidelines` says *"confirm both match what you intended"*, i.e. it delegates -verification to the agent's own reading of a string. - -**Trigger.** Any input that transforms its value: `<input type="number">` given `"abc"` (becomes -`""`), a masked phone input, `maxlength` truncation, an `<input type="date">` given a -non-ISO string, or a controlled component that rejects and reverts. - -**Impact** wrong result, but **mitigated**: the correct value *is* in the output text, so an -attentive agent can catch it. `browser_fill_form` computes an explicit `mismatch` flag -(`form-batch.ts:100-106`) and surfaces it in the summary line, which is what `browser_fill` -should do. - -**Severity** wrong result (mild — the data is present but unjudged). - -**Proposed fix.** Mirror `form-batch.ts`: compute `mismatch = res.value !== args.value`, put it -in `details`, and prefix the text (e.g. `` Filled … = "12" (requested "abc" — MISMATCH) ``). -Do not turn it into an error: legitimate normalisation (trimming, date reformatting) would -start failing. - -**Observable behaviour change.** Yes — an extra `mismatch` field and a warning in the text. - -**Reproducible in a test?** Yes, and this one is easy: the mismatch computation is pure over -`(args.value, res.value)`. Extract it (or share `form-batch`'s) and unit-test it. - ---- - -## F9 — `browser_type`'s focus guard is fail-open when the probe itself fails - -**Location** `src/domains/keyboard.ts:32-50`. - -*Pre-confirmed as a coverage hole. My independent call: it is also a live fail-open, of exactly -the shape the brief warns about.* - -```ts -const focused = await client.evaluateJs(safeJs`…`); -if (focused.success) { // ← probe failure skips the guard entirely - const f = isFocusProbe(focused.data) ? focused.data : { ok: false, tag: null }; - if (!f.ok) return err({ kind: "invalid_state", message: "No editable element is focused — typed text would be lost." }); -} -const r = await cdpCall(client, "Input.insertText", { text: args.text }); -``` - -The guard exists because typed text goes nowhere when nothing editable is focused. When the -probe cannot answer — the case where we know *least* about focus — the code proceeds to type -anyway and returns `` Typed: "…" ``. Note the inner fallback is correctly fail-closed -(`{ ok: false }` on a bad shape), which makes the outer fail-open look unintended rather than -chosen. - -**Trigger.** Anything that makes `client.evaluateJs` fail: a page-level CSP that blocks -evaluation, a detached/navigating frame, a transient timeout, or a JS dialog freezing the page -(the most likely one — a dialog is exactly when typed text is lost). - -**Impact** wrong result. Text silently discarded and reported as typed. Same class as F5, -narrower trigger. - -**Severity** wrong result. - -**Proposed fix.** Treat probe failure as "not focused": add an `else` returning -`invalid_state` with the probe's error message. If the team prefers not to block on a flaky -probe, keep typing but say so in the result text — never return an unqualified success. - -**Observable behaviour change.** Yes — `browser_type` starts failing in a case where it -previously claimed success. - -**Reproducible in a test?** Yes, but it needs a fake `BrowserClient` (the check runs through -`client.evaluateJs`). Return `err(cdpError("timeout", …))` from the fake and assert the handler -returns `invalid_state` rather than calling `Input.insertText`. A `makeFakeClient()` helper is -the missing piece; several other findings here (F2, F14) want the same helper. - ---- - -## F10 — Two fill engines coexist and disagree on the same element - -**Location** `src/domains/form.ts:90-122` (`browser_fill`) vs `src/domains/form-batch.ts:6-58` -(`browser_fill_form`). - -> **FIXED** in `16e7a30`, per the ruling: one engine, `browser_fill` delegates, and its -> documented `<select>` rejection survives as an explicit `rejectSelect` pre-check -> rather than as a happy accident of the old code falling through to "not fillable". -> Per-tool focus behaviour is a `focusFirst` parameter, so `browser_fill`'s documented -> "does not focus" promise still holds and `browser_fill_form` still focuses. -> `browser_fill` keeps appending `interactiveDiff` and republishing refs; -> `browser_fill_form` still does neither. The merged engine is the **union** of the two -> columns, so `browser_fill_form` gained the `execCommand` contenteditable path — the -> third behaviour change listed under Resolution status. - -*Pre-confirmed item — verified; the table in the brief matches the code exactly.* Both are -registered and agent-callable (`src/registry.ts:29-31`). - -| case | `browser_fill` | `browser_fill_form` | -|---|---|---| -| disabled input | writes, `ok` (F7) | `{ ok:false, reason:"element is disabled" }` | -| `<select>` | error, "use browser_select_option" | matches by `value`/`label`/`text`, fills | -| **checkbox** | prototype `value` setter, `ok` (F5) | flips `checked` via `click()` | -| contenteditable | `execCommand("insertText")` — drives ProseMirror/Slack | `el.textContent =` — does not | -| result | appends `interactiveDiff`, republishes refs | neither | -| verification | reports `verified`, no comparison (F8) | explicit `mismatch` flag | - -**Impact** The agent cannot reason about which tool to use, and the tool descriptions do not -warn it. `browser_fill_form` is documented as *"Preferred for forms"*, but it is the **weaker** -engine for contenteditable (the `textContent` assignment does not drive rich-text editors, -which is the one thing `FILL_FN`'s `execCommand` path exists for) and it returns no page diff. -Meanwhile `browser_fill` is the weaker engine for checkboxes, selects, and disabled controls. - -**Severity** wrong result (umbrella; F5, F7, F8 are its individually-testable instances). - -**Proposed fix.** One engine. Extract the per-element-type decision + write into a single JS -body used by both tools, with `browser_fill` as the single-field entry point (keeping the diff) -and `browser_fill_form` as the batch entry point (keeping `mismatch`). The union of the two -tables is strictly better than either column. This is a bigger change than the other fixes and -should be planned, not folded into a bug-fix commit. - -**Observable behaviour change.** Yes, substantially, in both tools. Needs the user's sign-off on -the merged semantics, not just on "is it a bug". - -**Reproducible in a test?** Once the decision logic is a pure function, yes — one table-driven -test per row. Today, no: both bodies are opaque strings and neither tool has any automated -coverage (F30). - ---- - -## F11 — Concurrent liveness probes can spawn a second daemon that unlinks the live one's socket - -**Location** `src/daemon/spawn.ts:41-101` (`isDaemonRunning`), `src/daemon/server.ts:83-89` -(`unlinkSync`), `:121-135` (duplicate-clientId rejection), -`src/daemon/protocol.ts:165` (`DAEMON_STALE_SOCKET_CLEANUP = true`). - -**What breaks.** Three facts compose into a bad outcome: - -1. `isDaemonRunning` registers with the **fixed** clientId `"_liveness_probe"` (`spawn.ts:70`). -2. `server.ts:126-134` rejects a registration whose clientId is already connected — it writes a - `shutdown` control and destroys the socket. -3. On rejection the probe's `close` handler fires `done(false)` (`spawn.ts:96-99`), so - `isDaemonRunning` reports **"not running"** for a daemon that is running and healthy. - `ensureDaemon` then spawns a second daemon, whose `start()` unconditionally - `unlinkSync(DAEMON_SOCKET_PATH)` (`server.ts:83-89`) — deleting the *live* daemon's socket - inode — and listens on a fresh one. - -The `close` handler's comment says *"daemon alive but rejected us (e.g. max clients); don't -unlink"* — so the author anticipated rejection, but the return value still says "not running", -which is what drives the spawn. - -**Trigger.** Two pi sessions calling `/browser-setup` (or `browser_setup`, or any first tool -call) within the ~2 s probe window. The `_liveness_probe` slot is also never freed on the -reject path, so a probe that races itself in one process can do it too. - -**Impact** Two daemons, two WebSockets to Chrome, two "Allow Remote Debugging" prompts — the -exact thing the daemon exists to eliminate. Sessions already connected keep working (their FD -survives the unlink), so the split is invisible until the user is prompted again. The orphaned -daemon lingers until its 30-minute idle timeout, if it ever reaches zero clients. - -**Severity** resource leak, with user-visible fallout (repeat Chrome prompts). - -**Proposed fix.** Two independent parts: -- Make the probe clientId unique: `` `_liveness_probe-${randomUUID()}` ``. Removes the collision - entirely and costs nothing. -- Distinguish "rejected by a live daemon" from "nothing listening". A received `shutdown` - control means the daemon is alive — `isDaemonRunning` should return `true`. And gate - `unlinkSync` on the socket not being live rather than doing it unconditionally. - -**Observable behaviour change.** No, in the normal single-session path. In the race, one daemon -instead of two. - -**Reproducible in a test?** Yes — `test/manual/ipc-server-test.ts` and -`live-daemon-test.ts` already drive the real server over a real socket. A test that registers -`_liveness_probe` and then calls `isDaemonRunning()` and asserts `true` is straightforward. -It lands in `test/manual/`, which the `npm test` glob excludes, so it would not run in CI -without the daemon-test infrastructure F30 asks for. - ---- - -## F12 — `openerId` is probably not a `Target.createTarget` parameter, so unpinned new tabs are placed by focus, not by ownership - -**Location** `src/cdp/commands.ts:90-99` (schema), `src/cdp/target-factory.ts:205-214` -(`openHarnessTab`, unpinned path). - -> **LEFT UNFIXED BY RULING — this is the highest-value thing a human with a browser can -> check.** Neither I nor the reviewer can run live Chrome. The experiment: call -> `Target.createTarget` with `openerId` set to an owned tab, then read the new target's -> `TargetInfo.openerId` and which window it joined. If Chrome ignores the parameter, -> then unpinned `browser_open_urls` / `browser_new_tab` tabs land in whichever window has -> **focus** — the user's window, not the harness's — and the session-boundary promise in -> `src/prompt.ts:98-105` is false. The fix would then be to pass `windowId: -> ownership.harnessWindowId()`, a documented parameter the harness already tracks for -> exactly this purpose and already has in its schema. - -*Pre-confirmed item, flagged for verification. My finding: it looks wrong, and I could not -confirm it either way from documentation.* - -**What breaks.** The command table declares `openerId: Type.Optional(Type.String())` as a -`Target.createTarget` parameter and `openHarnessTab` passes it to place a new tab "beside" -`openerTargetId`. Querying the CDP protocol definitions, `openerId` appears on **`TargetInfo`** -("Opener target Id") and I found no `openerId` in `CreateTargetRequest`. The documented -`Target.createTarget` parameters are `url`, `left`, `top`, `width`, `height`, `windowId`, -`browserContextId`, `enableBeginFrameControl`, `newWindow`, `background`, `forTab`. - -If it is not a parameter, Chrome ignores the unknown field and the new tab lands wherever a -bare `createTarget` lands — the focus-derived window. `target-factory.ts`'s own header says -*"`openerId` for siblings inside it"* while also stating (verified, for the pinned case) -that *"`openerId` does NOT inherit the opener's context"* — which is consistent with the field -being ignored rather than honoured. That the unpinned path appears to work in practice is -explained by the harness window usually being the focused one. - -**Trigger.** Unpinned profile, `browser_open_urls` or `browser_new_tab`, while a user window -has focus. The tabs would open in the user's window — still `ownership.add`-ed, so the harness -believes they are in its dedicated window. - -**Impact** wrong result, and it breaks the session-boundary promise in `prompt.ts:98-105` -(*"you operate only inside this session's own Chrome window"*). Also `windowId` — an actual, -documented parameter — is in the schema and available (`ownership.harnessWindowId()`) but is -never passed. - -**Severity** wrong result. **Unverified** — needs one live-Chrome experiment. - -**Proposed fix.** First verify: call `Target.createTarget` with `openerId` against live Chrome -and check whether the new target's `TargetInfo.openerId` is set and which window it joins. If -ignored, replace it with `windowId: ownership.harnessWindowId()`, which is exactly the -documented mechanism for "put this tab in that window" and which the harness already tracks -for this purpose. If it turns out to be honoured, delete the stale comment and note it. - -**Observable behaviour change.** Yes if changed — new tabs would reliably land in the harness -window rather than the focused one. - -**Reproducible in a test?** Not as a unit test — the question is about Chrome's behaviour. A -`test/manual/window-binding-test.ts` addition (that file already exists) is the right home. -The *schema* claim can be tested trivially once known: assert the table matches the protocol. - ---- - -## F13 — A `session_tree` event before client creation resets the namespace to `"default"` - -**Location** `src/index.ts:195-198`; `src/state.ts:51-68` (`restoreState`). - -*Pre-confirmed item, flagged "confirm whether the fallback makes these equivalent". It does -not, in one specific case.* - -**What breaks.** `session_tree` passes `client?.namespace`, which is `undefined` before the -client exists (the client is created in `session_start`). Inside `restoreState`: - -```ts -const fallback = defaultState(currentNamespace); // namespace = undefined ?? "default" -… -namespace: currentNamespace ?? data.namespace ?? fallback.namespace -``` - -With a persisted entry present, `data.namespace` wins and the two call sites *are* equivalent. -With **no** persisted entry, the loop falls through to `return fallback` — whose namespace is -`"default"`, not the `pi-<random>` value `index.ts:40` generated. `persistState` then writes -`"default"`, and the subsequent `session_start` reads it back and creates the client (and the -daemon transport) with `namespace: "default"`. - -**Trigger.** A `session_tree` event arriving before `session_start` in a session with no prior -`browser-harness-state` entry — a fresh session on a fresh branch. Whether pi actually orders -the events that way I could not determine from this repo; the ordering is the host's. - -**Impact** Every affected pi session shares the daemon clientId `"default"`, so the second one -to connect is rejected by `server.ts:126-134` ("clientId default is already connected") and -gets its socket destroyed. Multi-session browser control breaks, and the error surfaces as a -generic transport failure. If `--browser-namespace` was passed, `flagNs` is not consulted here -either, so an explicit user choice is discarded. - -**Severity** wrong result (conditional on host event ordering; **unverified** that the ordering -occurs). - -**Proposed fix.** Pass the namespace that is authoritative, not the one that happens to be -reachable: `restoreState(ctx, client?.namespace ?? state.namespace)`. `state.namespace` is -initialised from the flag or the generated value at `:40-42`, so it is never `undefined` — which -also makes the two call sites genuinely equivalent and removes the trap. - -**Observable behaviour change.** None in the ordering where `session_start` runs first. In the -other ordering, the namespace stops being clobbered. - -**Reproducible in a test?** Yes, and cheaply — `restoreState` takes only an `ExtensionContext` -whose `sessionManager.getBranch()` returns an array. A test asserting -`restoreState(ctxWithNoEntries, undefined).namespace` is not silently `"default"` when the -caller had a real namespace pins the contract. Reproducing the *event ordering* needs a pi -host fake, which does not exist here. - ---- - -## F14 — Both `AbortController` time budgets are dead code: the pre-`await` check can never fire - -**Location** `src/domains/snapshot.ts:346-366` (1 500 ms box budget) and -`src/domains/network.ts:249-266` (5 000 ms `BODY_BUDGET_MS`). - -*This is the "signal.aborted checked before an await, not re-checked after" item from the -brief's list — present in two places, in a slightly worse form than described.* - -**What breaks.** Both sites build the whole array of promises with `.map(async …)` and then -await them together. `.map` starts every async function **synchronously**, so the guard at the -top of each callback runs before the timer can possibly have fired: - -```ts -const timer = setTimeout(() => ac.abort(), BODY_BUDGET_MS); -const fills = drained.records.map(async (r) => { - if (ac.signal.aborted) return; // always false — same tick - const bodyRes = await session.call("Network.getResponseBody", …); // never re-checked - … -}); -await Promise.allSettled(fills); -``` - -`session.call` takes no `AbortSignal`, so aborting cannot cancel an in-flight request either. -Net effect in `network.ts`: the 5 s budget bounds nothing at all — all N `getResponseBody` -calls are issued, and the tool waits for every one (each up to the 15 s transport timeout). -In `snapshot.ts` the *first* check is likewise dead, so no `DOM.getBoxModel` is ever skipped; -only the **second** check (`:360`, after the `boxOf` await) does anything, and it only skips the -`liveValue` follow-up. - -**Trigger.** `browser_network_requests({ includeResponseBodies: true })` matching many records -on a slow page. `browser_snapshot` on a page with many interactive elements. - -**Impact** wrong result via latency: a tool documented as budgeted can block far past its -budget, and the caller sees a plain timeout with no partial data. Not a crash; nothing is -mis-stated in the output. - -**Severity** wrong result (latency/robustness). Low-to-medium. - -**Proposed fix.** Bound the whole batch instead of guarding inside each task: -`await Promise.race([Promise.allSettled(tasks), sleep(BUDGET_MS)])`, and record per-record -completion so partial results are reported honestly. If per-task cancellation is wanted, thread -a `timeoutMs` (already supported by `session.call`) derived from the remaining budget — -`src/util/time.ts` already exports a `deadline()` helper built for exactly this and currently -has no callers in `src/`. - -**Observable behaviour change.** Yes — the tool returns within its budget with partial bodies -instead of eventually returning all of them. - -**Reproducible in a test?** Yes for `network.ts`, with a fake client whose -`session().call("Network.getResponseBody")` resolves after a controllable delay: assert the -handler returns within ~`BODY_BUDGET_MS` for 50 records at 200 ms each. Needs the same -`makeFakeClient()` helper as F2/F9. - ---- - -## F15 — `asArrayOf` is all-or-nothing, so one malformed record blanks an entire TUI render - -**Location** `src/util/guards.ts:13-25`; consumers `src/domains/console.ts:139` and -`src/domains/network.ts:209`. - -*Pre-confirmed item — verified. Severity: cosmetic, not wrong-result, because it is -render-only.* - -**What breaks.** `asArrayOf` returns `undefined` if **any** element fails its item guard. In -`asConsoleDetails` / `asNetworkDetails` a single bad record makes the whole `records` array -`undefined`, which makes the details `undefined`, which makes `renderResult` return -`"console: no details"` / `"network: no details"` — hiding all the good records. - -**Trigger.** A record that fails the item guard. Both item guards require few fields -(`asConsoleRecord` needs a valid `level` and a string `text`; `asNetworkRecord` needs `method`, -`url`, `type`), and the producing buffers always populate them, so this is hard to hit from -inside the harness. It becomes reachable if a transcript is replayed after a schema change, or -if `details` is round-tripped through a lossy serializer. - -**Impact** cosmetic — the LLM's copy of the data comes from `ToolOk.text`, which is built -directly from `drained.records` and unaffected. Only the human's TUI panel goes blank. - -**Severity** cosmetic. - -**Proposed fix.** Add a lenient sibling — `asArrayOfLenient` / `compactMap`, which drops -unparseable elements and keeps the rest — and use it in the two render paths only. Leave -`asArrayOf` strict where a partial array would be wrong (`state.ts:39`, `ownedTargetIds`: -silently dropping a target id there would un-own a live tab, so strict is correct and should -stay). - -**Observable behaviour change.** Only in the failure case: a partial render instead of a blank one. - -**Reproducible in a test?** Yes, directly — `test/util/guards-test.ts` already exists and -`asArrayOf` is pure. - ---- - -## F16 — A strict `AxNode` schema means one bad node fails the whole `browser_snapshot` - -**Location** `src/cdp/commands.ts:55-76,239-242`; consumed by `snapshot.ts:313` and -`ref-resolve.ts:79`. - -*Pre-confirmed item — verified. My judgement: leave it, add a guard rail.* - -**What breaks.** `Accessibility.getFullAXTree`'s result is now `Type.Array(AxNode)`. A single -node failing validation makes `decodeResult` reject the entire response, so `browser_snapshot` -returns `invalid_response` and `interactiveDiff` silently returns `""`. Before the schema work -this was `Array(Unknown())` and could not fail. - -**Trigger.** A node where `nodeId` is absent or not a string — the only required field. CDP -always sends `nodeId` as a string, and everything else is optional with -`additionalProperties: true`, so I could not construct a realistic trigger. A future Chrome -change to the AX payload could. - -**Impact** If it ever fires, the harness's **default** page-inspection tool stops working -entirely, and the `interactiveDiff` failure is invisible (returns `""`, callers append nothing). -The blast radius is large; the probability is low. - -**Severity** cosmetic today (unreachable), crash-class if it ever fires. - -**Proposed fix.** The low-cost mitigation is per-node leniency at the consumer, not a looser -schema: keep `Type.Array(AxNode)`, but if the decode fails, fall back to filtering nodes that -individually validate and proceed with those, noting the count in `details`. Alternatively add -a `depth` cap. I would **not** revert to `Array(Unknown())` — that discards the type safety -Task 13 bought for a risk that has no demonstrated trigger. - -**Observable behaviour change.** Only in the failure case. - -**Reproducible in a test?** Yes — `test/cdp/commands-test.ts` exists; `decodeResult` is pure. -Feed a nodes array with one `{ nodeId: 42 }` and assert the whole decode fails today. - ---- - -## F17 — Every `Input.*` command's parameters are unvalidated - -**Location** `src/cdp/commands.ts:228-231`; worst consumer `src/domains/drag.ts:39-59`. - -*Pre-confirmed item — verified.* - -**What breaks.** `Input.dispatchMouseEvent`, `Input.dispatchKeyEvent`, and -`Input.dispatchDragEvent` are all `cmd(Type.Object({}, { additionalProperties: true }), Empty)`. -Any object satisfies them, so `ParamsOf<M>` is effectively `Record<string, unknown>` and a typo -(`deltaY` → `delatY`, `clickCount` → `clickcount`) typechecks, passes the boundary scanner, and -reaches Chrome, which ignores the unknown field. `drag.ts`'s `InputCall` tuple types its second -element as `Record<string, unknown>` for the same reason, and `type InputCall` is declared inside -the handler body rather than at module scope. - -**Trigger.** A typo or a wrong-typed value in any of the ~10 `Input.*` call sites -(`click.ts:42-51`, `scroll.ts:48-55`, `keyboard.ts:90-108`, `drag.ts:41-58`). Nothing catches it; -the action silently does the wrong thing or nothing. - -**Impact** No live bug found — I checked every call site against the CDP parameter names and -they are correct. This is a missing guard rail, not a defect. - -**Severity** cosmetic (latent). - -**Proposed fix.** Write out the real parameter sets: `type`, `x`, `y`, `button`, `buttons`, -`clickCount`, `deltaX`, `deltaY`, `modifiers`, `pointerType` for `dispatchMouseEvent`; `type`, -`key`, `code`, `text`, `unmodifiedText`, `windowsVirtualKeyCode`, `nativeVirtualKeyCode`, -`modifiers` for `dispatchKeyEvent`; `type`, `x`, `y`, `data`, `modifiers` for -`dispatchDragEvent`. Keep `additionalProperties: true` so a future CDP field is not a hard -error. Then `drag.ts`'s tuple can be typed properly and `InputCall` hoisted to module scope. - -**Observable behaviour change.** None — same wire payloads. - -**Reproducible in a test?** Not as a runtime test (the current code is correct). It is a -compile-time guarantee: a `// @ts-expect-error`-style negative test, or simply the typecheck. - ---- - -## F18 — `browser_snapshot` silently omits `screenshotPath` when the screenshot fails - -**Location** `src/domains/snapshot.ts:383-390`, with `:399` and the renderer at `:419`. - -*Pre-confirmed item — verified.* - -**What breaks.** - -```ts -if (args.includeScreenshot) { - const shot = await session.call("Page.captureScreenshot", …); - if (shot.success) { shotPath = …; writeFileSync(…); } // no else -} -``` - -A failed capture leaves `shotPath` undefined, `details.screenshotPath` is omitted, and the -renderer prints `"screenshot omitted"` — the identical wording it uses when the caller never -asked for one. The caller who explicitly passed `includeScreenshot: true` is told nothing. - -**Trigger.** `browser_snapshot({ includeScreenshot: true })` where `Page.captureScreenshot` -fails — a detached target, a page mid-navigation, or a GPU/compositor hiccup. - -**Impact** cosmetic: the AX tree, the refs, and everything else the tool exists for are correct. -Only the screenshot's absence is unexplained. - -**Severity** cosmetic. - -**Proposed fix.** Add `details.screenshotError = shot.error.message` on the failure path and -render `"screenshot failed: …"` instead of `"screenshot omitted"`. Do not fail the tool — the -snapshot itself succeeded. - -**Observable behaviour change.** Yes — a new `details` field and a distinct message. - -**Reproducible in a test?** Yes with a fake client (`Page.captureScreenshot` → `err`). Same -missing helper as F2/F9/F14. - ---- - -## F19 — A bad `outputPath` throws out of the handler instead of returning `io_error` - -**Location** `src/domains/files.ts:212-221` (`browser_print_to_pdf`); -also `src/domains/snapshot.ts:388` (`writeFileSync`) and -`src/domains/screenshot.ts:40` (`writeFile`). - -**What breaks.** `printToPdfTool` does `await writeFile(path, …)` with `path = args.outputPath ?? pdfPath(…)` -— caller-supplied, unvalidated, and outside any try/catch. An `ENOENT`/`EACCES`/`EISDIR` -rejection escapes the handler as a thrown exception instead of the `Result<ToolOk, ToolErr>` -every tool is contracted to return. `registerBrowserTool`'s `execute` has no try/catch around -`def.handler` (`util/tool.ts:178-196`) — only a `finally` that releases the mutex — so the -rejection propagates into pi's tool runner. The mutex *is* released, so there is no deadlock. - -The other two sites write to `screenshotPath()`/`pdfPath()` paths under `tmpdir()`, so they are -only reachable on a full or read-only tmpdir — but they are the same unguarded shape. - -**Trigger.** `browser_print_to_pdf({ outputPath: "/nonexistent/dir/out.pdf" })`, or a path the -process cannot write, or a directory path. - -**Impact** The agent gets a raw host-level error instead of a typed `io_error` it can act on. -Every sibling in `files.ts` (`downloadTool`, `verifyReadable`) validates its path carefully and -returns `io_error` — this one path does not. - -**Severity** crash (of the tool call), no data loss. - -**Proposed fix.** Wrap the three writes and map to `{ kind: "io_error", message }`. Separately, -add a try/catch around `def.handler` in `util/tool.ts:178` that maps an unexpected throw to -`{ kind: "internal" }` — a defence-in-depth fix that covers all 39 tools at once and is the -better first move. - -**Observable behaviour change.** Yes — a typed tool error instead of a thrown exception. - -**Reproducible in a test?** Yes for the `util/tool.ts` guard: register a tool whose handler -throws and assert `execute` resolves to an `isError` result. For `printToPdf` specifically it -needs the fake client. - ---- - -## F20 — Aborting `browser_wait_for` throws instead of returning a typed error - -**Location** `src/domains/page.ts:132` (`await sleep(50, signal)`); `src/util/time.ts:1-16`. - -**What breaks.** `sleep` **rejects** when the signal aborts. `waitTool` wraps its `sleep` in -try/catch (`page.ts:52-57`); `waitForTool` calls it bare inside the poll loop. The loop's own -`if (signal?.aborted)` check runs at the top of each iteration, so aborting *during* the 50 ms -sleep rejects out of the handler. Same unguarded-throw path as F19. - -Note `waitForLoadTool` (`:87`) uses a raw `new Promise(setTimeout)` rather than `sleep`, so it -is not affected — the three wait tools use three different sleep strategies. - -**Trigger.** Cancelling `browser_wait_for` (Esc / agent cancellation) within a 50 ms window. - -**Impact** A thrown exception instead of the `{ kind: "internal", message: "aborted" }` the -same function returns when the check wins the race. Inconsistent, not damaging. - -**Severity** cosmetic. - -**Proposed fix.** Wrap the loop body, or make the three wait tools share one abortable-sleep -helper that returns a `Result` rather than throwing. The blanket handler try/catch from F19 -also covers it. - -**Observable behaviour change.** Yes — a typed error on cancellation. - -**Reproducible in a test?** Yes — `sleep` is pure and needs no client; the handler needs a fake -client for `evaluateJs`. The `sleep`-rejects half is testable today. - ---- - -## F21 — `openIsolatedTab` can adopt and then close the harness window's only tab - -**Location** `src/domains/isolated-tab.ts:35-63` and `:112-114` (`closeIsolatedTab`); -`src/cdp/target-factory.ts:144-182`. - -**What breaks.** When `ensureHarnessWindow` reports `freshlyCreated: true`, -`openIsolatedTab` uses the **seed tab of the newly created harness window** as its isolated tab. -`ensureHarnessWindow` has, by then, recorded that same target as `harnessWindow` and added it to -ownership. `closeIsolatedTab` closes it in the `finally`, so the window that was just -established is destroyed and the window binding is dropped (`ownership.remove` clears -`harnessWindow` when the ids match, `ownership.ts:54-57`). - -**Trigger.** `browser_web_search` or `browser_read_page({ url })` at a moment when no owned tab -is live. `ensureAlive` normally runs first and seeds a window, so the window usually already -exists and `freshlyCreated` is false — the exposure is the narrow window where the user closes -the harness tabs between `ensureAlive` and the isolated-tab call. Also reachable when the -concurrency coordinator hands `freshlyCreated: true` to the creating caller while a second -concurrent `browser_read_page` is in flight. - -**Impact** With a pinned profile, the next tool call re-runs `seedPinnedProfileWindow`, which -**launches a browser window via the command line** — visible, slow (up to 15 s of sentinel -polling), and disruptive. If the session was attached to that seed tab, the next call also gets -`session_not_found` and re-attaches. It contradicts the tool's documented promise: -*"Runs in its own isolated tab and never disturbs the user's current tab."* - -**Severity** resource leak / lifecycle churn (no wrong data). - -**Proposed fix.** Never let an isolated tab *be* the harness window's seed tab: after -`ensureHarnessWindow` returns `freshlyCreated: true`, still call `openHarnessTab` to get a -sibling, and leave the seed alone. That is a two-line change and makes the seed tab's role -(a durable window anchor) consistent across all callers. - -**Observable behaviour change.** One extra tab open+close per isolated read in the fresh-window -case; the harness window survives. - -**Reproducible in a test?** Only with a fake client that can report "no owned tabs" and record -`Target.closeTarget` calls. Fiddly but doable; the race variant is not deterministically -reproducible. **Partly unverified** — I traced it in the code but did not observe it. - ---- - -## F22 — The daemon never releases session ownership on `Target.detachFromTarget` - -**Location** `src/daemon/bridge.ts:386-391`; caller `src/client.ts:375-383`. - -**What breaks.** The bridge does `if (isDetach && req.sessionId) router.release(req.sessionId)`. -But `client.detach()` sends `Target.detachFromTarget` with the session id in **`params`**, at -browser level (`{ sessionId: null }` transport option), so `req.sessionId` on the envelope is -`undefined` and the release never runs. `EventRouter.owners` keeps the mapping for a session -that no longer exists. - -**Trigger.** `session_shutdown` (which calls `client.detach()`), every session, every time. - -**Impact** A slow leak in a long-lived daemon: `owners` grows by one entry per detached session -and is only pruned by an `Inspector.detached` event (`bridge.ts:199-201`) or `removeClient` on -disconnect. Since the transport deliberately survives session shutdown -(`index.ts:184-187`), `removeClient` does not fire. Events for a dead session would also still -be routed to the stale owner — harmless, since Chrome stops sending them. - -**Severity** resource leak (small, bounded by daemon lifetime and the 30-minute idle exit). - -**Proposed fix.** Read the id from either place: -`const sid = req.sessionId ?? asString(req.params?.["sessionId"])`. `asString` is already -imported in `bridge.ts`. - -**Observable behaviour change.** None observable; the map shrinks. - -**Reproducible in a test?** Yes — `handleRequest` is a plain function taking a `WireRequest` -and a `send` callback; `getSessionOwner` is exported for the assertion. It lands in -`test/manual/bridge-test.ts`, outside the CI glob (F30). - ---- - -## F23 — `handleRequest`'s promise is dropped with no `.catch` - -**Location** `src/daemon/index.ts:24-32`. - -**What breaks.** `cdpBridge.handleRequest(msg, client.id, send)` returns a `Promise<void>` that -is neither awaited nor `.catch`-ed, inside a `readline` `"line"` handler. Any rejection is an -unhandled rejection, which on Node ≥ 15 terminates the process by default — taking the daemon, -and every connected pi session's browser control, with it. - -**Trigger.** No reachable throw found. `handleRequest`'s only `await` is a `setTimeout` sleep; -`JSON.stringify(payload)` and `ws.send` are inside a try/catch; `mux.allocate` cannot throw. -A future edit that adds an unguarded `await` in that function makes it reachable. - -**Impact** None today. This is the same latent shape as the `bridge.ts:156` `JSON.parse` bug -that Task 4 fixed — an unguarded failure path inside a socket handler with nothing above it — -and it sits in the same file's blast radius. - -**Severity** cosmetic today, crash-class if a throw is ever introduced. - -**Proposed fix.** `void cdpBridge.handleRequest(msg, client.id, send).catch((e) => { … send an -error response … });` — or make it explicit with `process.on("unhandledRejection")` logging in -the daemon entry point. Cheap insurance for a process whose whole job is to stay up. - -**Reproducible in a test?** Yes at the seam — inject a bridge whose `handleRequest` rejects and -assert the process/handler survives. Needs the daemon test infrastructure from F30. - ---- - -## F24 — `interactiveDiff` keys the diff on `role|name`, so duplicate-named elements collapse - -**Location** `src/domains/ref-resolve.ts:96-126`. - -**What breaks.** The diff builds `prevByKey` / `curByKey` maps keyed on `` `${role}|${name}` ``. -Any two interactive elements sharing a role and an accessible name overwrite each other, so only -the last one is compared. The comment explains the choice (*"Compare by stable identity -(role|name) rather than ref position, since refs renumber"*) — which is itself the symptom of F6. - -**Trigger.** Very common: a table with a "Delete" button per row; several unlabelled textboxes -(all `textbox|`); repeated "Add to cart" buttons. - -**Impact** The "Page changes" block under-reports. A value change on row 3's field can be -attributed to row 7's, or dropped. Since the whole point of the diff is *"read it to confirm the -change landed"* (`prompt.ts:30-31`), an agent trusting it can be misled — but the primary result -line (`Filled e7 = "…"`) is still correct, so nothing depends solely on the diff. - -**Severity** cosmetic (misleading advisory output). - -**Proposed fix.** Falls out of F6's fix for free. With identity-stable refs, key the diff on -`backendNodeId` and the collapse disappears. Not worth fixing independently. - -**Reproducible in a test?** Yes, pure, from two AX fixtures with duplicate `role|name` pairs. - ---- - -## F25 — `limit: 0` would make both buffers return everything (investigated — not a live bug) - -**Location** `src/cdp/console-buffer.ts:227-228`, `src/cdp/network-buffer.ts:174-175`. - -```ts -const limit = Math.min(filter.limit ?? 50, 500); -const limited = matched.slice(-limit); -``` - -With `limit === 0`, `-0` is `0` and `matched.slice(0)` returns the **entire** array — the exact -opposite of "return nothing". - -**Why it is not a live bug.** Both tool schemas set `minimum: 1` on `limit` -(`console.ts:35`, `network.ts:80`), so a validated `0` cannot arrive. `drainConsoleBuffer` / -`drainNetworkBuffer` have no other callers in `src/`. - -**Note for the record.** This is a **guard made dead by a schema constraint** — the category the -brief flags. Here the guard is fine and the schema is right; the fragility is that the buffer's -own contract does not hold without the schema. If the buffers are ever reused (a script via -`browser_run_script` gets `daemon` = the full `BrowserClient` and can reach -`client.session().drainConsoleBuffer({ limit: 0 })` **today**), it fires. - -**Severity** cosmetic (latent; reachable only via `browser_run_script`). - -**Proposed fix.** One line in each: `if (limit <= 0) return { records: [], total, bufferOverflowed }` -— or `matched.slice(Math.max(0, matched.length - limit))`. - -**Reproducible in a test?** Yes, trivially — both buffers are pure and dependency-free, and -this is a 5-line test. Worth doing purely because it is so cheap. - ---- - -## F26 — `"Provide either \`ref\` or \`selector\`."` appears three times; at most two can share a helper - -**Location** `src/domains/form.ts:46-48`, `src/domains/keyboard.ts:167-169`, -`src/domains/files.ts:127-129`. - -*Pre-confirmed item — verified, including the caveat.* `form.ts` and `keyboard.ts` both resolve -a ref to an **objectId** via `resolveRefToObjectId` and both fall back to -`document.querySelector`, so those two can share. `files.ts` resolves to a **backendNodeId** -(`resolveRefToBackendId`, for `DOM.setFileInputFiles`) and its selector path is a genuinely -different three-stage fallback (`DOM.querySelector` → CDP set → JS `DataTransfer`). Unifying all -three would force the file-upload shape into a mould that does not fit. - -Also note `src/domains/target.ts:34` carries the sibling message -`"Provide either \`ref\` or both \`x\` and \`y\`."` for the coordinate-based tools — already -correctly factored into one place, which is the pattern to copy. - -**Severity** cosmetic. - -**Proposed fix.** Extract the shared guard + message as a constant or a small -`requireRefOrSelector(args)` helper used by `form.ts` and `keyboard.ts`. Leave `files.ts` -referencing the same message constant but keeping its own control flow. - -**Observable behaviour change.** None. - -**Reproducible in a test?** Not a bug; no test needed. A test asserting all three produce the -identical message would lock the wording. - ---- - -## F27 — The boundary scanner has no rule for `: any` - -**Location** `scripts/check-boundaries.ts:13-19` (`RULES`). - -*Pre-confirmed item — verified, including the false-positive hazard.* The five rules cover -`as any`, `@ts-ignore`/`expect-error`/`nocheck`, non-null assertions, `as {`, and -`as unknown as`. Nothing covers a bare annotation: `const x: any = …`, `(p: any) => …`, -`Promise<any>`. - -`src/` has no `: any` today, so nothing is broken — but the whole point of the scanner is that -the property cannot silently regress, and this one can. - -**The false-positive hazard is real.** A naive `/:\s*any\b/` matches the long prose -`description` strings throughout `src/domains/` — e.g. `snapshot.ts:296` -(*"or (b) browser_snapshot and browser_execute_js cannot answer your question"* is safe, but -`js.ts:48` contains *"is this element visible / enabled / checked / focused"* and -`network.ts:74`'s resource-type list and several `promptGuidelines` lines contain `any` after a -colon in ordinary English). Anchoring on a terminator is required. - -**Severity** cosmetic (process guard rail). - -**Proposed fix.** Add `{ rule: "any-annotation", re: /:\s*any\s*(?:[=;,)\]>|&]|$)/ }` — a colon, -`any`, then a type-position terminator. Verify against the current tree: the rule must report -zero violations on `src/` as it stands. `test/util/check-boundaries-test.ts` already exists and -`checkSource` takes injected files, so both the positive and negative cases are directly -testable. - -**Observable behaviour change.** None to runtime; `npm run boundaries` gains a rule. - ---- - -## F28 — `sharp-shim.ts`'s indirection has invisible intent and invites a breaking "simplification" - -**Location** `src/util/sharp-shim.ts:23,34`. - -*Pre-confirmed item — verified. Functionally correct; a maintenance trap.* - -`const SHARP_SPECIFIER: string = "sharp";` followed by `await import(SHARP_SPECIFIER)` exists so -TypeScript cannot statically resolve the specifier and therefore does not require `sharp`'s -types — which is what removed the `@ts-ignore`. Nothing in the file says so. The explicit -`: string` annotation (redundant on a string literal) is the entire mechanism: it widens the -literal type so the import is dynamic to the compiler. - -A reader applying an "inline the constant" cleanup — plausibly Task 17, which strips comments — -restores a statically-resolvable `await import("sharp")` and breaks the build in any workspace -where the optional dependency is absent. - -**Severity** cosmetic (maintainability). - -**Proposed fix.** This is the one place in the file that needs a comment to survive Task 17. -Alternatively make the intent structural and self-evident: -`const specifier = ["sh", "arp"].join("")` is worse; a named export -`const OPTIONAL_SHARP_SPECIFIER` plus one line of explanation is the honest answer. Flag it as -comment-exempt for Task 17 either way. - -**Reproducible in a test?** Partly. A test asserting `loadSharp()` returns `{ kind: "missing" }` -rather than throwing when `sharp` is absent guards the runtime behaviour, but nothing can -prevent the compile-time regression except the comment. - ---- - -## F29 — Ref-based fill/focus/select leak a CDP object handle on every call - -**Location** `src/domains/form.ts:30-45` (`runOnElement`, ref branch); -also `src/domains/keyboard.ts:152-165` (`browser_dispatch_key`, ref branch) and -`src/domains/snapshot.ts:263-281` (`liveValue`). - -*Pre-confirmed item — verified, and it is in **three** places, not one.* - -**What breaks.** `resolveRefToObjectId` produces a `Runtime.RemoteObject` handle via -`DOM.resolveNode`. Chrome retains that object (it cannot be GC'd) until -`Runtime.releaseObject` is called or the execution context is destroyed. `runOnElement` never -releases it. `resolveAndCall` in the same file **does** (`form.ts:369-371`, in a `finally`), -so the correct pattern already exists two hundred lines away, and `Runtime.releaseObject` is -already in the typed command table (`commands.ts:193`). - -`liveValue` in `snapshot.ts` is the worst site by volume: it resolves one handle **per -interactive form control per snapshot** and releases none. - -**Trigger.** Every `browser_fill`, `browser_focus`, `browser_select_option`, and -`browser_dispatch_key` on the ref path, plus every `browser_snapshot` (once per -`VALUE_ROLES` control). - -**Impact** Renderer memory growth in the user's real browser tab, proportional to how long the -session runs. Released on navigation (context destroyed), so it is bounded per page. A -long-running SPA session doing hundreds of snapshots and fills is the bad case. - -**Severity** resource leak. - -**Proposed fix.** Wrap all three in `try { … } finally { await session.call("Runtime.releaseObject", { objectId }) }`, -exactly as `resolveAndCall` does. For `runOnElement` specifically, F2's suggested fix -(route the ref branch through `resolveAndCall`) fixes the leak and the missing -`exceptionDetails` check in one change. - -**Observable behaviour change.** One extra CDP round-trip per ref operation; no user-visible -difference. - -**Reproducible in a test?** Yes with a fake client that records calls: assert -`Runtime.releaseObject` appears after `Runtime.callFunctionOn`. Same missing `makeFakeClient()` -helper as F2. - ---- - -## F30 — Coverage holes where the type system cannot help - -*Pre-confirmed item — verified against `package.json` and the test tree.* - -The `npm test` glob is `test/{profile,deep-research,domains,cdp,util}/**/*-test.ts`. Confirmed -gaps: - -- **`test/manual/` is excluded entirely** — 10 files, including `bridge-test.ts`, - `ipc-server-test.ts`, `daemon-transport-test.ts`, `socket-guard-test.ts`, - `window-binding-test.ts`, `forms-test.ts`. All the daemon and forms coverage that exists is - outside CI. -- **`test/daemon/` does not exist.** `daemon/bridge.ts:156` (the `isCdpRawMessage` guard that - stops a `null` CDP frame from killing the daemon) has no test and no type protection — - `onChromeMessage` is a closure reachable only through a live WebSocket. Fix direction: extract - `onChromeMessage`'s body as a pure `handleChromeFrame(raw, deps)` and test it directly. That - also makes F22 and F23 testable. -- **`domains/` has three test files** (`box`, `registry`, `target`). `form.ts` (425 lines), - `form-batch.ts`, `keyboard.ts`, `snapshot.ts` (477 lines), and `ref-resolve.ts` have **zero** - automated coverage — i.e. F2, F5, F6, F7, F8, F9, F24, F29 all live in untested files. - `fillFormTool` and `setCheckedTool` are newly agent-callable with no coverage at all. -- **`test/manual/forms-test.ts` does not drive `browser_fill_form` at a `<select>`**, so it - would not have caught the select-by-label class of bug (`dcfe4f3`) even if it were run. -- **The missing enabler is a `makeFakeClient()` test helper.** Eight findings above need one and - none can be reached without it. `test/cdp/session-call-test.ts`'s stub transport is the - precedent; a `BrowserClient` shim over it is the single highest-leverage piece of test - infrastructure this codebase is missing. - -**Severity** cosmetic (process), but it is the reason most findings in this report are latent -rather than caught. - ---- - -## Investigated — not bugs - -Recorded because the reasoning is worth as much as a finding. - -**F31 — `spawn.ts` resolving the daemon entry as `index.ts`.** The comment at `spawn.ts:107-112` -says the entry is `dist/daemon/index.js` when running compiled, but the code only ever builds -`join(moduleDir, "index.ts")`. Checked `package.json`: `files` ships `src/` (no `dist/`) and -`pi.extensions` is `./src/index.ts`. The package runs from TypeScript source via `tsx` in every -distribution path, so the code is right and only the comment is stale. - -**F32 — `picker.ts:61` mapping a selected label back to a profile by `options.indexOf`.** Looked -for an off-by-one or a collision: `rows[i]` is built from `labels[i]` which is built from -`profiles[i]`, and `CLEAR_SELECTION_LABEL` is appended last and handled before the `indexOf`. -`disambiguate` guarantees distinct labels, and the pinned row's `"✓ "` prefix keeps it distinct -from an unpinned duplicate. Correct. - -**F33 — `restoreState`'s fallback making the two call sites equivalent.** They *are* equivalent -whenever a persisted entry exists. The non-equivalence is narrow and specific — see F13, which -is the real finding this investigation produced. - -**F34 — `util/mutex.ts` and `util/result.ts`.** Read closely for a lost-release or a chain that -can reject. The mutex's queue is only ever advanced by `resolve`, `registerBrowserTool` releases -in a `finally`, and no handler holds the lock across a `return` without it. Correct. - -**F35 — `console-buffer.clear()` resetting `nextSeq` to 1.** This would invalidate every -outstanding `nextCursor` (a stale cursor would suppress new records whose seq restarted below -it). Grepped: `clear()` has **no caller** in `src/`. Dead code, not a bug. Worth deleting or -making seq-monotonic if it is ever wired up. - -**F36 — `bind.ts:111` `if (seed.browserContextId) client.setProfileContextId(...)`.** If a -sentinel page arrived without a `browserContextId`, `profileContextId()` stays undefined, -`canReuse` in `ensureHarnessWindowUncoordinated` stays `false` forever, and **every** -`ensureHarnessWindow` call would command-line-launch a new profile window. That would be severe. -But `TargetInfo.browserContextId` is populated for targets in a non-default browser context — -which a pinned non-default profile's window is by construction — and the launched window is -always non-default relative to the connected browser's default context. Classified **not a bug, -low residual risk**: if the pinned profile happens to *be* the browser's default context, the -field may be omitted and the degenerate loop becomes reachable. Unverified; a one-line -`else` returning a clear error would remove the doubt cheaply. - ---- - -## Ranking: is `exceptionDetails` the most severe item? - -**No — I put it fourth.** F2 is real and should be fixed, but its failure is *loud*: the agent -gets an error, the wrong error. It re-snapshots and retries. Nothing downstream is corrupted. - -Above it I place three findings whose failure is **silent**: - -1. **F1** — the harness does not work at all on Windows. Nothing else matters on that platform. -2. **F5** — `browser_fill` on a checkbox reports success for a no-op. The agent proceeds on a - false belief, submits a form with the box unticked, and reports the task done. No error is - ever surfaced. This is the single worst *behaviour* in the codebase. -3. **F6** — refs renumber under the agent's feet, so a mutation can hit the wrong element while - reporting success — on the exact workflow the system prompt recommends. - -F3 and F4 are close behind for the same reason (silent data loss), and they are the two easiest -findings in the report to reproduce and fix. - -The ordering principle: **silence beats severity**. A wrong error message costs a retry; a false -success costs correctness with no signal. - ---- - -## Coverage and confidence - -**Read thoroughly, line by line** — `cdp/` (all 17), `daemon/` (all 5), `util/` (all 10), -`schemas/` (both), the 7 root files, and in `domains/`: `form.ts`, `form-batch.ts`, -`keyboard.ts`, `ref-resolve.ts`, `snapshot.ts`, `cdp-call.ts`, `click.ts`, `navigate.ts`, -`tabs.ts`, `page.ts`, `files.ts`, `js.ts`, `network.ts`, `console.ts`, `drag.ts`, -`isolated-tab.ts`, `screenshot.ts`, `target.ts`, `box.ts`. - -**Read, lower scrutiny** — `profile/` (7 files: read in full, but the platform-specific process -detection in `browser-process.ts` (437 lines of `ps`/PowerShell/registry parsing) was assessed -for shape rather than probed per-platform; it also has the best test coverage in the repo, -`test/profile/browser-process-test.ts`). Same for `domains/search/` and `domains/readpage/` — -read in full, and their extraction/scoring logic is already pure and fixture-tested, so I looked -for integration bugs (F21) rather than re-auditing the parsers. - -**Skimmed** — the long `description` / `promptGuidelines` string blocks in every tool -definition, except where a claim contradicted the code (F5, F6, F12, F21 all turn on such a -contradiction, so the ones that mattered were read). - -**Not covered** — `test/` beyond its layout and `session-call-test.ts`'s stub; `skills/`; -`docs/`. All out of scope for a `src/` sweep. - -**Unverified, called out in place** — F1 (needs Windows), F12 (needs live Chrome), F13 (needs -the host's event ordering), F21 (traced, not observed), F36 (needs a live pinned profile in the -default browser context). F3's premise (per-session domain enablement) is standard CDP behaviour -asserted by the code's own comment but not confirmed against a browser here. - ---- - -## Summary table - -| # | Finding | Location | Severity | Pre-confirmed? | Reproducible in a test? | Fix changes behaviour? | -|---|---|---|---|---|---|---| -| **F1 ✔ fixed** | `access()` on a Windows named pipe fails ⇒ **every** browser tool returns "not initialized" | `util/tool.ts:145` | crash (platform-wide) | no | after extracting the check; real Windows behaviour needs Windows | yes (Windows only) | -| **F5 ✔ fixed** | `browser_fill` on a checkbox sets `value` not `checked`, returns `ok` | `domains/form.ts:90-105` | wrong result (silent no-op) | yes — agree, worst behaviour | needs a pure branch-decision seam, or live Chrome | yes | -| **F6 ✔ fixed** | Every mutation renumbers the ref map ⇒ held refs can target the wrong element | `domains/ref-resolve.ts:85-94` | wrong result (silent) | no | yes, pure, from AX fixtures | yes | -| **F2 ✔ fixed** | `runOnElement` ignores `exceptionDetails` ⇒ a page throw reports "No element matched" | `domains/form.ts:37-44` (3 sites) | wrong result (misdiagnosis) | yes — agree; I rank it 4th, not 1st | yes, needs a `makeFakeClient()` helper | yes | -| **F3 ✔ fixed** | `switchTo` back to a visited tab skips `enableDomains` ⇒ console/network/page events stop | `cdp/session.ts:368-377` | wrong result (silent data loss) | no | **yes — easiest in the report**, existing stub transport | yes | -| **F4 ✔ fixed** | Event loop drops all non-active-tab events, defeating the per-tab buffers | `cdp/session.ts:93` | wrong result (silent data loss) | no | yes, stub transport + pushable event queue | yes | -| F7 | Fill/select write through a **disabled** control and report `ok` | `domains/form.ts:90,254` | wrong result (silent) | yes — agree | same seam as F5 | yes | -| F9 | `browser_type` focus guard is **fail-open** when the probe itself fails | `domains/keyboard.ts:40` | wrong result (silent) | yes (as a coverage hole) — I also call it a live fail-open | yes, needs the fake client | yes | -| **F10 ✔ fixed** | Two fill engines diverge on disabled / select / **checkbox** / contenteditable / diff | `form.ts` vs `form-batch.ts` | wrong result (umbrella) | yes — table verified exactly | per-row, once the decision logic is pure | yes, substantially — needs design sign-off | -| F8 | `verified` is reported but never compared to the requested value | `domains/form.ts:161-164` | wrong result (mild — data present, unjudged) | yes — agree it's real, rank it lower | yes, the comparison is pure | yes | -| F11 | Concurrent liveness probes ⇒ second daemon unlinks the live socket, two Chrome prompts | `daemon/spawn.ts:70` + `server.ts:83` | resource leak (user-visible) | no | yes, but lands in excluded `test/manual/` | no (normal path) | -| **F12 ⚠ needs live Chrome** | `openerId` is likely not a `createTarget` param ⇒ unpinned tabs placed by focus | `cdp/commands.ts:96`, `target-factory.ts:210` | wrong result — **unverified** | yes (flagged to verify) — looks wrong; could not confirm | no; needs one live-Chrome experiment | yes if changed | -| F13 | `session_tree` before client creation resets namespace to `"default"` | `index.ts:196` | wrong result — **unverified ordering** | yes — not equivalent, in one narrow case | `restoreState` yes; the event ordering no | no (normal ordering) | -| F19 | Bad `outputPath` throws out of the handler instead of `io_error` | `domains/files.ts:219` (+2) | crash (tool call) | no | yes, via a handler-level try/catch test | yes | -| F14 | Both abort budgets are dead — `.map` starts every task before the timer can fire | `snapshot.ts:351`, `network.ts:253` | wrong result (latency) | yes (as the abort-path item) | yes for `network.ts`, needs the fake client | yes | -| F29 | Ref fill/focus/select/dispatch_key + `liveValue` leak CDP object handles (3 sites) | `form.ts:37`, `keyboard.ts:155`, `snapshot.ts:273` | resource leak | yes — agree; it is 3 sites, not 1 | yes, assert `releaseObject` is called | no | -| F21 | `openIsolatedTab` can adopt then close the harness window's only tab | `domains/isolated-tab.ts:41-47` | resource leak / churn | no | partly — race variant not deterministic | yes (one extra tab open/close) | -| F22 | Daemon misses `detachFromTarget` session release (id is in `params`, not the envelope) | `daemon/bridge.ts:389` | resource leak (small) | no | yes, but in excluded `test/manual/` | no | -| F15 | `asArrayOf` all-or-nothing blanks a whole TUI render | `util/guards.ts:13` | cosmetic (render only) | yes — agree it's real; **render-only, not wrong result** | yes, pure, test file exists | only in the failure case | -| F18 | `screenshotPath` silently omitted when the screenshot fails | `domains/snapshot.ts:384-390` | cosmetic | yes — agree | yes, needs the fake client | yes (new `details` field) | -| F16 | Strict `AxNode` array ⇒ one bad node fails all of `browser_snapshot` | `cdp/commands.ts:241` | cosmetic today; crash-class if it fires | yes — no realistic trigger found; **mitigate, don't revert** | yes, `decodeResult` is pure | only in the failure case | -| F17 | All `Input.*` params unvalidated; `drag.ts` `InputCall` untyped + declared in a handler | `cdp/commands.ts:228-231` | cosmetic (latent) | yes — verified; **no live bug**, every call site is correct | compile-time only | no | -| F23 | `handleRequest` promise dropped with no `.catch` in a socket handler | `daemon/index.ts:31` | cosmetic today; crash-class if a throw appears | no | yes, with daemon test infra | no | -| F24 | Diff keyed on `role\|name` collapses duplicate-named elements | `domains/ref-resolve.ts:98-105` | cosmetic (misleading advisory) | no | yes, pure — but fixed for free by F6 | no (subsumed) | -| F20 | Aborting `browser_wait_for` throws instead of returning a typed error | `domains/page.ts:132` | cosmetic | no | yes (`sleep` half is testable today) | yes | -| F25 | `limit: 0` ⇒ `slice(-0)` returns everything | `console-buffer.ts:228`, `network-buffer.ts:175` | cosmetic (latent) | no | **yes, 5-line pure test** — reachable via `browser_run_script` | no | -| F26 | `"Provide either ref or selector"` ×3; only 2 can share a helper | `form.ts:47`, `keyboard.ts:168`, `files.ts:128` | cosmetic | yes — agree, including the caveat | n/a | no | -| F27 | Boundary scanner has no `: any` rule; a naive regex hits prose `description`s | `scripts/check-boundaries.ts:13-19` | cosmetic (guard rail) | yes — agree, anchoring needed | yes, `checkSource` takes injected files | no | -| F28 | `sharp-shim.ts` indirection's intent is invisible; a "simplification" breaks the build | `util/sharp-shim.ts:23,34` | cosmetic (maintainability) | yes — agree; **flag comment-exempt for Task 17** | runtime half only | no | -| F30 | Coverage holes: `test/manual/` excluded, no `test/daemon/`, `form`/`snapshot`/`keyboard` untested, no `makeFakeClient()` | `package.json` test glob | cosmetic (process) | yes — verified and expanded | n/a | no | -| F31–F36 | **Investigated, not bugs**: stale `dist/` comment in `spawn.ts`; `picker.ts` index mapping; `restoreState` equivalence (⇒ F13); `mutex`/`result`; `console-buffer.clear()` is uncalled dead code; `bind.ts` missing-`browserContextId` degenerate loop (low residual risk, unverified) | various | — | — | — | — | - -**Fixed in this pass (2026-07-28 ruling):** F1, F2, F3, F4, F5+F10, F6 — six findings in -six commits, each preceded by a test that failed for the reason recorded above. Test suite -206 → 251 passing, 8 skipped, 0 failing; `npm run check` green. See *Resolution status* at -the top for commits and the three shipped behaviour changes. **F12 remains open and needs -one live-Chrome experiment.** Everything else is documented only, by ruling. - -**Counts.** 30 findings + 6 investigated-not-bugs. -By severity: **crash 2** (F1, F19) plus 2 crash-class-if-triggered (F16, F23) · -**wrong result 11** (F2, F3, F4, F5, F6, F7, F8, F9, F10, F12, F13, F14 — 12 including F14's -latency class) · **resource leak 4** (F11, F21, F22, F29) · **cosmetic 11**. -By verification: 25 verified by reading · **5 unverified** (F1 needs Windows, F12 needs live -Chrome, F13 needs the host's event ordering, F21 traced-not-observed, F36 needs a live pinned -default-context profile). -Pre-confirmed items: 12 reviewed, **12 agreed real**, 0 rejected — but three re-rated -(F8 down, F15 down to cosmetic/render-only, F17 confirmed as latent-only with no live bug), -one broadened (F29 is 3 sites, not 1), and one, F12, remains **unverified and probably wrong -as written**. |