diff options
Diffstat (limited to 'docs/superpowers/reports/2026-07-26-bug-audit.md')
| -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**. |