diff options
| -rw-r--r-- | .gitignore | 3 | ||||
| -rw-r--r-- | CONTRIBUTING.md | 20 | ||||
| -rw-r--r-- | docs/ARCHITECTURE.md | 429 | ||||
| -rw-r--r-- | docs/speed-optimizations.md | 180 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-05-02-harness-rewrite.md | 2937 | ||||
| -rw-r--r-- | docs/superpowers/reports/2026-07-26-bug-audit.md | 1488 | ||||
| -rw-r--r-- | docs/superpowers/specs/2026-05-02-harness-rewrite-design.md | 423 | ||||
| -rw-r--r-- | docs/superpowers/specs/2026-07-26-codebase-refactor-design.md | 264 |
8 files changed, 10 insertions, 5734 deletions
@@ -4,8 +4,7 @@ benchmark/ *.tsbuildinfo .DS_Store *.log -docs/* -!docs/ARCHITECTURE.md +docs/ scripts/* !scripts/check-boundaries.ts !scripts/boundary-allowlist.ts diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 93ae71e..aa73342 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -39,15 +39,13 @@ npm pack --dry-run ## Architecture -Read [`docs/ARCHITECTURE.md`](docs/ARCHITECTURE.md) before making a non-trivial change. It covers: +Get the shape of the codebase from the source before making a non-trivial change: -- the layer diagram and which directories a `domains/*` file may and may not import from, -- the typed and runtime-validated CDP boundary in `src/cdp/commands.ts` and `src/cdp/events.ts`, and how to add a command, -- the nine-rule tool authoring contract, -- an end-to-end worked example of adding a tool and testing it against the fake client, -- the conventions in this codebase that look like mistakes but are not. - -The source carries almost no comments by policy, so that document is where the non-obvious conventions live. +- `scripts/check-boundaries.ts` and `scripts/boundary-allowlist.ts` encode the layer rules — which directories a `domains/*` file may and may not import from. `npm run boundaries` enforces them. +- `src/cdp/commands.ts` and `src/cdp/events.ts` are the typed, runtime-validated CDP boundary. Add a command by adding it to that table. +- `src/util/tool.ts` defines `defineBrowserTool`, the contract every tool in `src/domains/` follows. +- `src/domains/` holds one file per tool; read a neighbouring one before writing a new one. +- `test/domains/` shows how tools are tested against the fake client. --- @@ -86,8 +84,8 @@ Use the normal browser workflow when testing: screenshot → act → screenshot - Write TypeScript and keep the project passing `npm run check`. - Keep `strict` TypeScript mode clean. Do not introduce implicit `any` values. -- No unchecked casts. `npm run boundaries` fails the build on `as any`, `as {`, `as unknown as`, `@ts-ignore`, and non-null assertions in `src/`. A green `tsc` is not evidence of type safety — see `docs/ARCHITECTURE.md` section 3. -- Prefer no comments. Explain a non-obvious convention in `docs/ARCHITECTURE.md`, not inline. +- No unchecked casts. `npm run boundaries` fails the build on `as any`, `as {`, `as unknown as`, `@ts-ignore`, and non-null assertions in `src/`. A green `tsc` is not evidence of type safety. +- Prefer no comments. Explain a non-obvious convention in the commit message or PR description, not inline. - Prefer small, focused changes over broad rewrites. - Keep tool descriptions, parameter descriptions, and prompt guidelines clear and user-facing. - Preserve the existing error-handling style: return useful, actionable messages to the agent instead of leaking low-level details when possible. @@ -98,7 +96,7 @@ Use the normal browser workflow when testing: screenshot → act → screenshot ## Adding a new browser tool -The full worked example is in [`docs/ARCHITECTURE.md`](docs/ARCHITECTURE.md) — section 4 (the contract) and section 5 (the steps). In short: one file under `src/domains/`, a typebox args schema at the top, a `defineBrowserTool` call carrying its own `concurrency`, an entry in `ALL_TOOLS` in `src/registry.ts`, and a test under `test/domains/` written against `createFakeClient`. +One file under `src/domains/`, a typebox args schema at the top, a `defineBrowserTool` call carrying its own `concurrency`, an entry in `ALL_TOOLS` in `src/registry.ts`, and a test under `test/domains/` written against `createFakeClient`. Beyond that: diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md deleted file mode 100644 index 92ea16f..0000000 --- a/docs/ARCHITECTURE.md +++ /dev/null @@ -1,429 +0,0 @@ -# Architecture - -How `pi-browser-harness` is put together, what each layer may depend on, and the rules a new -tool has to follow. Read the "Conventions that look like mistakes" section before you clean -anything up — several deliberate choices in this codebase look like typos. - -The codebase carries almost no comments by policy. That makes this file the only place the -non-obvious conventions live. - ---- - -## 1. Layers - -Dependencies point downward only. - -``` -index.ts extension entry: flags, slash commands, session lifecycle hooks - | -registry.ts flat tool catalog (ALL_TOOLS, 40 tools): imports only, zero per-tool config - | -util/tool.ts tool runtime: liveness guard -> concurrency lock -> handler - -> Result-to-AgentToolResult conversion - | -domains/* one file per capability. Tool files hold tools; shared logic - lives in helper-only modules beside them (cdp-call, ax-tree, - element-call, box, ref-resolve, fill-engine, isolated-tab, - screenshot-capture, target). The only layer expected to grow. - | -domains/cdp-call.ts the ONLY door from a tool to the browser: cdpCall, - cdpCallOnTarget, cdpCallBrowser, evalJs, cdpErrToToolErr - | -client.ts BrowserClient facade: session, ownership, mutation mutex, - profile pin - | -cdp/session.ts typed call/callOnTarget/callBrowser, event routing, - console + network buffers - | -daemon/transport.ts -> daemon/* out-of-process daemon owning the Chrome socket - (CdpTransport is declared in cdp/types.ts) - | -Chrome (CDP over WebSocket) -``` - -Side stacks: - -| Directory | Role | Who may import it | -|---|---|---| -| `profile/*` | decides *which* Chrome and which profile to drive | `index.ts`, `setup.ts`, `client.ts` — never a domain | -| `schemas/*` | shared typebox arg fragments (`Coords`, `MouseButton`) and `parseJson` | anything | -| `util/*` | leaves — they import nothing from this repo except each other (`result`, `guards`, `mutex`, `truncate`, `paths`, `js-template`, `sharp-shim`, `debug`) | anything | - -`src/util/tool.ts` sits in `util/` for locality but is not a leaf — it is the tool runtime layer -in the diagram above and imports `client` and `daemon/spawn`. - -### The import rule for domains - -A `domains/*` file may import from `schemas/`, `util/`, other `domains/`, the `BrowserClient` -type, and the *typed, session-level* parts of `cdp/` (`cdp/commands` types, `cdp/target-factory`, -the buffers, and `cdp/session` for the `CdpSession` type and `evaluateJson`). - -**It must never import from `daemon/` or `profile/`.** A domain that reaches -the raw transport has escaped the validated boundary in section 2; a domain that reaches -`profile/` has coupled a tool to Chrome-launch policy. - -The import rule itself is **not** machine-enforced — verify it by eye in review. What *is* -enforced is the narrower rule that matters most: two boundary-scanner rules, `raw-cdp-call` and -`raw-evaluate`, are scoped to `src/domains/` and fail the build when any file but `cdp-call.ts` -reaches `session.call*` or `evaluateJs` directly. The scope matters — `client.ts` and `cdp/` are -the layer that legitimately owns the session, so the rules deliberately do not apply there. - -The rules match the two call shapes that exist today (`client.session().call…` and a `CdpSession` -held in a local named `session`). They are a guardrail, not a proof: renaming that local would -slip past them. - ---- - -## 2. The CDP boundary - -`src/cdp/commands.ts` (34 commands) and `src/cdp/events.ts` (12 events) are the single source -of truth for **both** the TypeScript types and the runtime validation. One typebox schema per -entry generates both, so they cannot drift. - -```ts -const cmd = <P extends TSchema, R extends TSchema>(params: P, result: R) => ({ - params, - result, - validate: Compile(result), -}); - -export const COMMANDS = { - "Target.attachToTarget": cmd( - Type.Object({ targetId: Type.String(), flatten: Type.Optional(Type.Boolean()) }), - Type.Object({ sessionId: Type.String() }), - ), - // ... -}; - -export type CdpMethod = keyof typeof COMMANDS; -export type ParamsOf<M extends CdpMethod> = Static<(typeof COMMANDS)[M]["params"]>; -export type ResultOf<M extends CdpMethod> = Static<(typeof COMMANDS)[M]["result"]>; -``` - -`decodeResult` validates every response before resolving; `decodeEvent` does the same for -subscribed events. A malformed payload becomes `err({ kind: "invalid_response", ... })` instead -of a `TypeError` three frames later. - -### Adding a command - -1. Add one `cmd(params, result)` entry to `COMMANDS`, keyed by the CDP method name. -2. Call it. `session.call(method, params)` and `cdpCall(client, method, params)` are already - typed against the table. **No cast anywhere.** - -### `additionalProperties: true` — the part that bites - -`additionalProperties: true` makes the **runtime validator** permissive. It does **not** widen -the derived `Static<>` **type**. The consequence differs by direction, and getting it backwards -produces two different bugs: - -| Schema | Under-specify | Over-specify | -|---|---|---| -| `params` | **compile error at the call site** — the field is not in `ParamsOf<M>`, so you cannot pass it | harmless | -| `result` / event params | field is simply unavailable to read | **silently dropped event or failed decode** when a future Chrome changes that field | - -So: - -- **Enumerate every field a call site passes.** Four of the six compile errors this refactor - had to fix were exactly this: `Runtime.callFunctionOn`'s params schema omitted `arguments`, - `returnByValue`, and `awaitPromise`, which the code genuinely sends. -- **On results and events, enumerate only what a consumer actually reads.** `Page.frameNavigated` - and `Page.loadEventFired` are declared as fully-open objects with no enumerated fields - (`ev(Anything)`) and never call `decodeEvent` at all — the session only needs to know they - fired. - -### How a tool reaches the browser - -`src/domains/cdp-call.ts` is the only door, and the scanner enforces it (section 1): - -| Helper | Use for | -|---|---| -| `cdpCall(client, method, params, opts?)` | the current tab | -| `cdpCallOnTarget(client, method, params, sessionId, opts?)` | a specific attached session | -| `cdpCallBrowser(client, method, params?, opts?)` | browser-level, no session | -| `evalJs(client, expression, sessionId?)` | evaluating JS, returns `unknown` | -| `cdpErrToToolErr(e, method)` | mapping a `CdpError` you got from elsewhere | - -Each wraps the session call and maps `CdpError -> ToolErr`, so the -`if (!r.success) return err({ kind: "cdp_error", ... })` couplet is gone — a failed call is -returned as-is (`if (!r.success) return r;`). - -`params` is **required** on `cdpCall`, unlike `session.call`. A no-argument CDP method needs an -explicit `{}`: - -```ts -const r = await cdpCall(client, "Page.reload", {}); -``` - -**A tool that wants to swallow a failure still goes through these helpers** — it just ignores the -error instead of returning it. `liveValue` in `src/domains/ax-tree.ts` returns `undefined` so the -caller falls back to the accessibility-tree value, and `snapshot`'s optional-screenshot block -simply omits the path. Both call `cdpCall` and discard the error. Reaching past the helper to -suppress an error is never the reason to do it. - -One honest gap: `session.attach()` and `session.windowId()` are session methods that do perform a -CDP call, and `domains/navigate.ts` calls `attach` directly. The scanner's rules match `call*` and -`evaluateJs`, not these two. - ---- - -## 3. Keeping the type safety - -### A green `tsc` proves nothing about unchecked casts - -`as { … }` between structurally compatible object types compiles **silently**. At one point this -codebase held 33 such casts and produced only 6 compile errors when they were removed. Strict -flags do not see them. - -Likewise, **`JSON.parse` returns `any`, so an annotated assignment is an invisible cast.** -`let msg: CdpRawMessage = JSON.parse(raw)` typechecked cleanly in `src/daemon/bridge.ts` while -being completely unguarded — a `null` frame from Chrome would have thrown inside a `ws` handler -with no `try`/`catch` above it and killed the daemon. That is why `src/schemas/parse.ts` exists: -`parseJson(raw, validator)` parses to `unknown`, validates, and returns a `Result`. Every parse -boundary goes through it or an equivalent guard. - -### The boundary scanner - -`npm run boundaries` runs `scripts/check-boundaries.ts` over every `.ts` file under `src/` and -fails the build on five line-level patterns: - -| Rule | Matches | -|---|---| -| `as-any` | `as any` | -| `ts-ignore` | `@ts-ignore`, `@ts-expect-error`, `@ts-nocheck` | -| `non-null-assertion` | `x!.`, `x!(`, `x!;` … | -| `inline-object-cast` | `as {` | -| `double-cast` | `as unknown as` | - -Exemptions live in an **external allowlist**, `scripts/boundary-allowlist.ts`, as -`{ path, rule, count, reason }` entries. The `count` is a budget, not a flag: a *second* -violation of the same rule in an already-exempt file still fails the build. - -The allowlist is deliberately external rather than a comment marker in the source, because the -zero-comments policy would eventually delete an in-source marker and turn CI red for an -unrecognisable reason. `unusedExemptions` also reports an entry whose code is gone, so the list -cannot rot. - -**Limitations, stated rather than implied:** the scan is **line-based**, so a cast split across -two lines evades every rule. And it only walks `src/` — `test/` and `scripts/` are unscanned. - -### The five surviving assertions in `src/` - -All five are boundary assertions where a runtime check has already established the shape and -TypeScript cannot express the correlation. - -| Location | Why | -|---|---| -| `decodeResult` in `src/cdp/commands.ts` | `validate.Check(raw)` proves `raw` matches `COMMANDS[method].result`; TS cannot correlate a generic key to its mapped value | -| `decodeEvent` in `src/cdp/events.ts` | same, for `EVENTS[name].params` | -| `loadSharp` in `src/util/sharp-shim.ts` (probe for `.default`) | the module comes from a dynamic import of an optional dependency, so its shape is unknown at compile time; the cast only claims an optional `.default`, and every value read off it is re-checked with `typeof` | -| `loadSharp` in `src/util/sharp-shim.ts` (`candidate` to `SharpFactory`) | `typeof candidate === "function"` was checked immediately above | -| `registerBrowserTool` in `src/util/tool.ts` (`renderCall` re-widening) | `AnyBrowserToolDefinition` is type-erased so the registry can hold a heterogeneous array; `defineBrowserTool` guarantees handler, parameters, and `renderCall` were built from the same `S` | - -Only the `sharp-shim.ts` `.default` probe needs an allowlist entry, because it is the one written -as `as { … }`. The other four are `as SomeNamedType`, which no scanner rule matches. **Do not add -allowlist entries for them** — they are not required, and `unusedExemptions` would flag them. - ---- - -## 4. The tool authoring contract - -Nine rules. Every `domains/*` file follows them. - -| # | Rule | Exemplar | -|---|---|---| -| 1 | **One domain per file, exporting only `<name>Tool` consts.** Anything a second file needs goes in a helper-only module beside it (`ax-tree.ts`, `element-call.ts`, `screenshot-capture.ts`, …) — never exported from a tool file. Past roughly 250 lines, split by capability. | `src/domains/history.ts` | -| 2 | **Args schema at the top of the file**, named `<Name>Args`, built with typebox, with a `description` on every field. Those strings are the agent's only documentation of the tool. | `src/domains/keyboard.ts` (`TypeArgs`, `PressKeyArgs`) | -| 3 | **`defineBrowserTool({ … })` holds all metadata, including `concurrency`.** A tool's concurrency class must not live in a different file from the tool. | every domain file | -| 4 | **Fixed handler signature** `(args, ctx) => Promise<Result<ToolOk, ToolErr>>`. Never throw, never `console.log`, never touch `process`. Errors are values. | `src/util/tool.ts` (`ToolHandler`) | -| 5 | **Reach Chrome only via `ctx.client`.** No domain imports the transport. | see section 1 | -| 6 | **Element targeting goes through the shared `resolveTarget(client, args)`** in `src/domains/target.ts` — never a hand-rolled ref-vs-x/y branch. | `src/domains/click.ts` | -| 7 | **CDP calls go through `src/domains/cdp-call.ts`** — `cdpCall`, `cdpCallOnTarget`, `cdpCallBrowser`, `evalJs`. Machine-enforced by the `raw-cdp-call` and `raw-evaluate` scanner rules. | `src/domains/history.ts` | -| 8 | **Naming:** tool name `browser_<verb>`, `label` title-case, `promptSnippet` one line, `promptGuidelines` at most six agent-facing bullets. | pinned by `test/domains/registry-test.ts` | -| 9 | **Every tool has a test** covering its schema and its error paths, written against a fake client so no browser is needed. | `test/domains/form-errors-test.ts` | - -Rule 1's two standing exceptions are largely settled: `snapshot.ts` went from 477 lines to 256 -once the tree model moved to `ax-tree.ts`, and `form.ts` from 409 to 283 once the element-call -machinery moved to `element-call.ts`. Both sit at the guidance rather than far past it. Neither -is licence to add a third. - -Rule 6 currently binds only `src/domains/click.ts`, because it is the sole tool taking -*either* a ref *or* coordinates. `drag.ts` takes explicit coordinates only and `scroll.ts` -defaults to the viewport centre; neither resolves refs. Any *new* ref-or-coords tool must use -`resolveTarget`. - -Rules 8 and 9 are partly machine-checked. `test/domains/registry-test.ts` asserts every tool -declares a concurrency class, that names are unique, that every name is `browser_`-prefixed, and -that the set of `serialized` tools is **exactly** a pinned list — so adding a serialized tool -means adding its name to `SERIALIZED` in that file. - ---- - -## 5. How to add a tool - -`src/domains/history.ts` (80 lines, three tools) is the whole pattern in one file. A minimal -tool, abridged from `reloadTool` in that file — the real one also branches on an open JS dialog: - -```ts -import { Type } from "typebox"; -import { type Result, err, ok } from "../util/result"; -import { defineBrowserTool, type ToolErr, type ToolOk } from "../util/tool"; -import { cdpCall } from "./cdp-call"; - -export const reloadTool = defineBrowserTool({ - name: "browser_reload", - label: "Browser Reload", - description: "Reload the current page.", - promptSnippet: "Reload the page", - promptGuidelines: ["Use to refresh the page, e.g. after server-side changes."], - parameters: Type.Object({}), - concurrency: "serialized", - async handler(_a, { client }): Promise<Result<ToolOk, ToolErr>> { - const r = await cdpCall(client, "Page.reload", {}); - if (!r.success) return r; - const info = await client.pageInfo(); - if (!info.success) return err({ kind: "cdp_error", message: info.error.message }); - return ok({ text: `Reloaded: ${info.data.title}`, details: { page: info.data } }); - }, -}); -``` - -Note `if (!r.success) return r;` — a failed `cdpCall` is already a `Result<_, ToolErr>`, so it -returns unchanged. No re-wrapping. - -Steps: - -1. Create `src/domains/foo.ts`. Declare `FooArgs` at the top with a `description` on every field. -2. If the CDP method you need is not in `COMMANDS`, add a table entry first (section 2). -3. Call `defineBrowserTool` with an explicit `concurrency`. Pick `serialized` if the tool mutates - page or session state; `parallel` only for pure reads. -4. Add the export to `ALL_TOOLS` in `src/registry.ts`. -5. If it is `serialized`, add its name to `SERIALIZED` in `test/domains/registry-test.ts`. -6. Write `test/domains/foo-test.ts`. - -### Testing a handler - -Use **`createFakeClient()`** from `test/domains/fake-client.ts`. It builds a real `CdpSession` -over a **stub transport** — canned per-method results, recorded calls, and an injectable event -stream: - -```ts -const ctxFor = (fake: FakeClient): HandlerContext => ({ - client: fake.client, - signal: undefined, - onUpdate: () => {}, - extensionCtx: undefined as never, -}); - -const fake = await createFakeClient({ - refs: [{ ref: "e1", backendId: 101 }], - canned: { - "DOM.resolveNode": ok({ object: { type: "object", objectId: "obj-1" } }), - "Runtime.callFunctionOn": ok({ - result: { type: "undefined" }, - exceptionDetails: { text: "Uncaught TypeError: el.dispatchEvent is not a function" }, - }), - }, -}); -const r = await fillTool.handler({ ref: "e1", value: "x" }, ctxFor(fake)); -assert.equal(r.success, false); -if (r.success) return; -assert.equal(r.error.kind, "cdp_error"); -assert.match(r.error.message, /dispatchEvent is not a function/); -``` - -`canned` maps a CDP method to a `Result` (or a queue of them, consumed in order); `refs` seeds the -session's ref map so a `ref`-taking tool resolves without a snapshot; `evaluate` stubs -`client.evaluateJs`. `fake.calls` / `fake.callsTo(method)` are the recorded requests, and -`fake.emit(event)` injects a CDP event. `test/domains/form-errors-test.ts` is the reference usage. - -**Do not mock the `BrowserClient` instead.** A mocked client bypasses the real session, the real -decoders, and the real error mapping, which produces a test that cannot fail. Two tasks on this -refactor shipped exactly that before `createFakeClient` existed. - -It also exports `createStubTransport` (for session-level tests) and `axNode` (for -accessibility-tree fixtures). - ---- - -## 6. Conventions that look like mistakes - -Each of these has been, or would be, "fixed" by someone who did not know why it was written -that way. - -**`src/util/tool.ts` gates the lock on `if (def.concurrency !== "parallel")`, not -`=== "serialized"`.** Deliberate fail-closed default: an unrecognised or missing concurrency -value **over-serialises** rather than racing on shared CDP session and page state. A test in -`test/domains/registry-test.ts` ("a definition with no concurrency class takes the mutation -mutex") pins it. - -**`src/util/sharp-shim.ts` routes its dynamic import through -`const SHARP_SPECIFIER: string = "sharp"`** so TypeScript cannot resolve the specifier -statically. `sharp` is an **optional** dependency. **Anyone who "simplifies" this back to a -static `import("sharp")` breaks the build wherever sharp is absent.** This is the single most -likely thing in the codebase to be broken by a well-meaning cleanup. - -**`npm test` passes `--conditions=import`.** A test can import a tool definition *only* because -of it: `@mariozechner/pi-coding-agent` publishes no `"require"` condition, and this repo is not -`"type": "module"`, so tsx would otherwise compile `src/` to CJS and the specifier would be -unresolvable. It also *matches* production — pi loads the extension with jiti and aliases that -package to `dist/index.js` by absolute path, which is what the `import` condition selects. - -**The test glob is `test/{profile,domains,cdp,daemon,util}/**/*-test.ts`.** A test placed -in any other directory — `test/schemas/`, say — **silently never runs**, so adding a directory -means adding it here too. `test/manual/` is excluded on purpose: those need live Chrome. - -**`Box` (`src/domains/box.ts`) has six fields — `x, y, width, height, cx, cy` — and all six are -consumed.** `cx`/`cy` are the click point. `width`/`height` reach the model through -`browser_snapshot` with `format: "json"`, which serialises each node's `box` wholesale. -Narrowing the type would silently change tool output. - ---- - -## 7. The failure mode that cost the most rework - -**Adopting a type guard where a loose truthiness check lived can invert a safety default.** - -`if (guard(x)) { if (!x.ok) fail }` is **not** equivalent to `if (!x.ok) fail`. With an unchecked -cast, a malformed payload left the field `undefined`, so `!field` was **true** and the failure -path fired. Fold the guard into the boolean gate and a rejected shape now **skips the check -entirely** — the code proceeds in exactly the case where it knows least. - -The structural tell, verified across six sites: - -- **SAFE** — the guard produces a **value** that inherits a pre-existing failure path: - ```ts - const v = guard(x) ? x : undefined; - if (v === undefined || !v.ok) return err(…); - ``` -- **UNSAFE** — the guard is **ANDed into a boolean gate**: - ```ts - if (ok && guard(x)) { … } - ``` - -`typeTool` in `src/domains/keyboard.ts` has both halves within six lines. The inner line is the -safe form — `isFocusProbe(focused.data) ? focused.data : { ok: false, tag: null }` turns a bad -shape into a **value** meaning "not focused", which the existing `if (!f.ok)` then rejects. The -outer `if (focused.success)` is the same mistake in its other guise: a gate, so when the focus -probe itself fails the entire check is skipped and the tool types anyway. That is a known open bug -(F9 in the bug audit), left in place here as the clearest illustration in the tree. - -Rule of thumb: **a check must default to the failure path when its input cannot be understood.** -If adding a guard moved a case from "fails loudly" to "not checked", the guard is in the wrong -place. - ---- - -## 8. Running the checks - -```bash -npm run check # typecheck + boundaries + test -``` - -- `npm run typecheck` — `tsc --noEmit` over `src/`, then `tsc -p tsconfig.test.json` over - `src/`, `test/`, and `scripts/`. Flags live in `tsconfig.base.json`; the notable ones beyond - `strict` are `noUncheckedIndexedAccess`, `exactOptionalPropertyTypes`, - `noPropertyAccessFromIndexSignature`, and `verbatimModuleSyntax`. -- `npm run boundaries` — the cast scanner (section 3). -- `npm test` — `node:test` over the glob above. - -`npm run check` must be green before a PR. diff --git a/docs/speed-optimizations.md b/docs/speed-optimizations.md deleted file mode 100644 index 5b35d6b..0000000 --- a/docs/speed-optimizations.md +++ /dev/null @@ -1,180 +0,0 @@ -# Pi Agent Features for Blazingly Fast Browser Automation - -> Deep research on every pi agent capability that can be leveraged to make `pi-browser-harness` as fast as possible. - ---- - -## 1. Parallel Execution (The Biggest Speed Multiplier) - -| Feature | How It Makes Browser Harness Fast | -|---------|-----------------------------------| -| **Parallel Tool Execution (Default)** | Pi executes sibling tool calls from the same assistant message **concurrently** by default. Multiple `browser_click`, `browser_type`, `browser_scroll`, or `browser_http_get` calls on independent targets run simultaneously instead of sequentially. | -| **`browser_open_urls`** | Opens multiple URLs in **parallel new tabs** in one shot. For research/scraping workflows, this eliminates sequential navigation delays entirely. | -| **Parallel `browser_execute_js` across tabs** | After listing tabs, you can switch to each tab and extract data in parallel batches rather than one-by-one. | -| **Parallel `browser_http_get`** | Direct HTTP GETs outside the browser are independent of browser state—fire dozens of API calls simultaneously. | -| **Read + Browser tools in same turn** | `read` (file system) and `browser_*` tools can run in parallel in the same turn because they hit different resources. | -| **`withFileMutationQueue()`** | For custom tools that write files, this queues mutations per-file so parallel tool execution doesn't cause race conditions/overwrite bugs. | - -### Patterns - -```typescript -// Open all result pages at once -browser_open_urls({ urls: ["url1", "url2", "url3"] }) - -// Then extract from each tab in parallel batches -browser_switch_tab(targetId1) → browser_execute_js("...") -browser_switch_tab(targetId2) → browser_execute_js("...") -``` - ---- - -## 2. Batching & Scripting (Eliminate Round-Trips) - -| Feature | How It Makes Browser Harness Fast | -|---------|-----------------------------------| -| **`browser_run_script`** | Write a temporary `.js` script and execute it with **full daemon + Node.js access**. This is the single biggest speedup for multi-step browser workflows—scrape 47 paginated pages, bulk-export CSV, or run complex CDP sequences in **one round-trip** instead of 47 screenshot→click loops. | -| **Custom Tools via Extensions (`pi.registerTool`)** | Register high-level browser tools like `batch_extract`, `bulk_screenshot`, or `auto_login` that internally run multiple CDP commands. The LLM calls one tool; the extension does the orchestration. | -| **Dynamic Tool Registration** | Tools can be registered at runtime (e.g., after detecting a page type). This lets you inject specialized, fast-path tools only when needed. | -| **Early Termination (`terminate: true`)** | Custom tools can return `terminate: true` to hint that no follow-up LLM turn is needed. For structured extraction tasks, this saves an entire model call. | -| **`onUpdate` streaming in custom tools** | Stream partial results back while long browser scripts run, so the user sees progress without waiting for full completion. | - -### When to Use Scripts vs. Built-in Tools - -| Use Scripts | Use Built-in Tools | -|-------------|-------------------| -| Same 3+ step sequence repeated | One-off actions | -| Bulk CSV export, structured extraction, pagination | Simple click or type | -| Domain-specific reusable pattern | Quick screenshot verification | - -```typescript -// Example: Batch extraction script -write("/tmp/extract-products.js", ` - const data = await daemon.evaluateJs(\` - JSON.stringify(Array.from(document.querySelectorAll('${params.rowSelector}')).map(el => ({ - title: el.querySelector('${params.titleSelector}')?.textContent?.trim(), - price: el.querySelector('${params.priceSelector}')?.textContent?.trim(), - }))) - \`); - return { content: [{ type: 'text', text: data }], details: { raw: JSON.parse(data) } }; -`) -browser_run_script("/tmp/extract-products.js", { rowSelector: "...", titleSelector: "...", priceSelector: "..." }) -``` - ---- - -## 3. Subagents & Delegation (Scale Across Cores/Sessions) - -| Feature | How It Makes Browser Harness Fast | -|---------|-----------------------------------| -| **Parallel subagent tasks** | Launch multiple `scout`, `researcher`, or `worker` agents simultaneously with `tasks: [...]`. Each can control its own browser tab or perform independent research. | -| **Async/background runs (`async: true`)** | Launch a long-running browser automation (e.g., full test suite, deep crawl) in the background while the parent agent continues working. | -| **Chain execution** | Sequence dependent steps (e.g., `scout` → `planner` → `worker`) with `{previous}` and `{chain_dir}` templating so each step doesn't rediscover context. | -| **Worktree isolation (`worktree: true`)** | Run parallel agents on isolated git worktrees so concurrent file writes don't conflict—essential for parallel browser-based testing or scraping pipelines. | -| **File-only output mode (`outputMode: "file-only"`)** | For subagents, return compact file references instead of inline text. This prevents massive context bloat on the parent session, keeping subsequent turns fast. | -| **Forked context (`context: "fork"`)** | Spawn advisory `oracle` or `reviewer` agents that inherit parent history but run on a branched session—no context copying overhead. | - -### Example: Parallel Research with Browser - -```typescript -subagent({ - tasks: [ - { agent: "scout", task: "Audit frontend auth flow" }, - { agent: "researcher", task: "Research current retry/backoff best practices" } - ], - concurrency: 2 -}) -``` - ---- - -## 4. Event Interception & Mutation (Optimize at the Harness Level) - -| Feature | How It Makes Browser Harness Fast | -|---------|-----------------------------------| -| **`tool_call` event interception** | Block, modify, or batch tool arguments before execution. Example: detect three sequential `browser_click` calls and rewrite them into a single `browser_run_script` batch. | -| **`tool_result` event modification** | Post-process raw browser output (e.g., truncate huge HTML, extract only diff, cache repeated queries) before it hits the LLM context. | -| **`before_agent_start` / `context` events** | Inject pre-computed page state or strip unnecessary history before the LLM call, reducing token count and latency. | -| **`input` event transformation** | Transform user prompts before they reach the agent. Example: expand `!{selector}` inline bash or auto-convert "scrape all links" into a batched script invocation. | -| **AbortSignal (`ctx.signal`)** | Cancel long-running browser operations (navigation, waits, downloads) immediately when the user hits Escape or a turn ends. | - -### Example: Extension-Level Batching - -```typescript -pi.on("tool_call", async (event, ctx) => { - if (event.toolName === "browser_click" && shouldBatch(event.input)) { - // Rewrite into a script call - event.input = { scriptPath: "/tmp/batch-clicks.js", params: { clicks: [...] } }; - } -}); -``` - ---- - -## 5. Context Efficiency (Keep Token Latency Low) - -| Feature | How It Makes Browser Harness Fast | -|---------|-----------------------------------| -| **Automatic Compaction** | Proactively summarizes old messages when approaching the context window. For long browser sessions (multi-page crawls), this prevents slowdowns from oversized contexts. | -| **Custom Compaction via Extensions** | Override compaction logic to preserve critical browser state (current URL, cookies, form data) while summarizing away redundant screenshots or HTML dumps. | -| **Context Files (`AGENTS.md`)** | Load browser-specific conventions once at startup. The model doesn't need to relearn "always screenshot before clicking" every turn. | -| **Skills (Progressive Disclosure)** | Browser-specific skills (like `pi-browser-harness`) only load their full instructions when needed. The system prompt stays lean until a browser task is detected. | -| **`PI_CACHE_RETENTION=long`** | Extended prompt cache retention (Anthropic: 1h, OpenAI: 24h) means repeated browser-tool-heavy prompts reuse cached prefixes, cutting latency and cost. | - -### Settings for Long Browser Sessions - -```json -{ - "compaction": { - "enabled": true, - "reserveTokens": 16384, - "keepRecentTokens": 20000 - } -} -``` - ---- - -## 6. Provider & Model Control (Speed at the LLM Layer) - -| Feature | How It Makes Browser Harness Fast | -|---------|-----------------------------------| -| **Model Cycling (`Ctrl+P`)** | Switch to faster/cheaper models for simple browser tasks (e.g., `gpt-4o-mini` for extraction) and reserve heavy models for complex reasoning. | -| **Thinking Level Control (`Shift+Tab`)** | Set thinking to `off` or `minimal` for deterministic browser automation, eliminating reasoning latency when it's not needed. | -| **Custom Providers (`pi.registerProvider`)** | Route browser-heavy sessions through faster proxies, local endpoints, or team-wide caches. | -| **Transport Selection (`sse` vs `websocket`)** | Choose the lowest-latency transport for your provider. | -| **`before_provider_request` hook** | Inspect or rewrite the provider payload to inject cache-control markers, strip unnecessary tool definitions, or enable prompt caching for repeated browser schemas. | - ---- - -## 7. Session Architecture (Never Lose Speed to State Management) - -| Feature | How It Makes Browser Harness Fast | -|---------|-----------------------------------| -| **Session Branching (`/tree`, `/fork`)** | Experiment with different browser strategies without losing state. If one automation path is slow, fork and try another without rebuilding context from scratch. | -| **Session Compaction (`/compact`)** | Manually trigger compaction mid-session after a heavy crawl to instantly restore context window headroom. | -| **`pi.appendEntry()`** | Persist browser state (cookies, auth tokens, current page index) as custom session entries. Restore on resume without re-authenticating or re-navigating. | -| **Custom Commands (`pi.registerCommand`)** | Register `/browser-batch` or `/crawl` commands that kick off optimized script-based workflows without LLM involvement. | - ---- - -## 8. UI & Control Flow (Reduce Human-in-the-Loop Delay) - -| Feature | How It Makes Browser Harness Fast | -|---------|-----------------------------------| -| **Message Queue (`steer` / `followUp`)** | Queue corrective messages while the agent is mid-crawl. The browser harness receives steering immediately after the current tool batch instead of waiting for full completion. | -| **Custom UI / Overlays** | Build a live browser dashboard (status line showing current URL, progress bar for crawls) so the user doesn't interrupt prematurely. | -| **`browser_page_info` dialog detection** | Instantly detect blocking JS dialogs instead of timing out. Extensions can auto-dismiss known popups via `tool_call` interception. | - ---- - -## Summary: The "Blazingly Fast" Formula - -To maximize browser harness speed with pi: - -1. **Parallelize everything** — Use `browser_open_urls`, parallel tool execution, and parallel subagents. -2. **Script heavy sequences** — Use `browser_run_script` for any 3+ step repetitive flow. -3. **Build custom tools** — Register batch tools via extensions to collapse multi-turn loops into single tool calls. -4. **Intercept and optimize** — Use `tool_call`/`tool_result` events to batch, cache, and truncate before the LLM sees it. -5. **Keep context lean** — Use compaction, file-only subagent output, and skills to prevent token bloat. -6. **Delegate asynchronously** — Launch long crawls as `async` subagents while the parent continues. -7. **Choose the right model** — Cycle to fast models with minimal thinking for deterministic automation. diff --git a/docs/superpowers/plans/2026-05-02-harness-rewrite.md b/docs/superpowers/plans/2026-05-02-harness-rewrite.md deleted file mode 100644 index 700a469..0000000 --- a/docs/superpowers/plans/2026-05-02-harness-rewrite.md +++ /dev/null @@ -1,2937 +0,0 @@ -# pi-browser-harness v0.3.0 Internal Rewrite — Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Restructure `src/` into per-domain modules with a `defineTool` helper, a transport/session/client split, and structural fixes for the predictability bugs and security gaps catalogued in the spec — without breaking the 28 LLM-facing tool names. - -**Architecture:** Per-domain decomposition. Dependencies flow `domains/* → client → cdp/* → util/*`. Every tool handler returns `Result<ToolOk, ToolErr>`; one `defineTool`/`registerTool` helper converts that into pi's `ToolResult` and supplies the unified `details` shape. The current `BrowserDaemon` class becomes a `createBrowserClient(opts)` factory composing a `CdpTransport` + `CdpSession`. JS injection is structurally prevented by routing all evaluation source through `safeJs\`...\``. `browser_run_script` keeps its full Node powers but gains a path allowlist, mandatory timeout, and AbortSignal wiring. - -**Tech Stack:** TypeScript (strict mode + extended safety flags), `ws` (WebSocket), `typebox` (tool schemas), `@mariozechner/pi-coding-agent` (extension API + truncation helpers), `@mariozechner/pi-tui` (renderers). - -**Reference spec:** `docs/superpowers/specs/2026-05-02-harness-rewrite-design.md` - ---- - -## Build order (why the tasks run in this order) - -1. **Foundations first (Tasks 1–3).** `Result`, `safeJs`, `keycodes`, `paths`, `time` are pure utilities depended on by everything else. Land them with `tsc` green so the rest of the work has a solid base. -2. **Strict tsconfig third (Task 3).** Turning on the new flags before the rest of the code is rewritten would explode the codebase. We turn them on *only* over `src/util/**` first, then expand the include scope as new code lands. By the end the whole `src/` is covered. -3. **Transport / session / client (Tasks 4–7).** This is the biggest single piece. `transport.ts` (WS) → `session.ts` (target/dialog/event handling) → `discovery.ts` (DevToolsActivePort) → `client.ts` (composition). At this point the new `client` exists but no domains use it yet; `daemon.ts` and `tools.ts` still serve every tool call. The package still ships and works. -4. **`defineTool` helper (Task 8).** Cannot be tested without at least one domain consumer, so we land the helper alongside the first vertical slice. -5. **First vertical slice — `domains/click.ts` (Task 9).** Smallest stateless tool that exercises the full path: schema → handler → client → CDP → Result → registry. Validates the architecture end-to-end. After this task, `browser_click` is served by the new path; the other 27 tools still use `tools.ts`. Manual smoke checkpoint here. -6. **Bulk domain migration (Tasks 10–22).** One task per domain file. Each task removes the corresponding tool block(s) from `tools.ts` and adds the new domain file. `tools.ts` shrinks by ~70-100 LOC per task. -7. **Wire-up + dead-code deletion (Tasks 23–25).** `index.ts`, `state.ts`, `prompt.ts`, then delete `daemon.ts`, `tools.ts` (now empty), `protocol.ts`, `renderers.ts`. -8. **Final hardening (Tasks 26–27).** Strict-flag expansion to whole `src/`, manual smoke run of the verification scenarios from spec §11, version bump + CHANGELOG. - -## Green-bar checkpoints - -After every task: `npm run typecheck` must pass. Failures block the next task. - -Manual smoke checkpoints: **after Task 9** (one tool migrated end-to-end) and **after Task 27** (full migration). Smoke procedure is in spec §11. - ---- - -## File layout (target end-state) - -``` -src/ - index.ts registry.ts prompt.ts state.ts setup.ts - client.ts - cdp/ - transport.ts session.ts discovery.ts errors.ts types.ts - domains/ - navigate.ts history.ts click.ts keyboard.ts scroll.ts - screenshot.ts tabs.ts page.ts dialog.ts js.ts - files.ts viewport.ts drag.ts network.ts - schemas/ - common.ts - util/ - result.ts tool.ts truncate.ts keycodes.ts paths.ts - js-template.ts time.ts -``` - -Deleted at end: `daemon.ts`, `tools.ts`, `protocol.ts`, `renderers.ts`. - ---- - -## Task 1: Foundations — `util/result.ts`, `util/time.ts`, `util/paths.ts` - -**Files:** -- Create: `src/util/result.ts` -- Create: `src/util/time.ts` -- Create: `src/util/paths.ts` - -- [ ] **Step 1.1: Write `src/util/result.ts`** - -```ts -export type Result<T, E> = - | { readonly success: true; readonly data: T } - | { readonly success: false; readonly error: E }; - -export const ok = <T>(data: T): Result<T, never> => ({ success: true, data }); -export const err = <E>(error: E): Result<never, E> => ({ success: false, error }); - -export const map = <T, U, E>(r: Result<T, E>, f: (t: T) => U): Result<U, E> => - r.success ? ok(f(r.data)) : r; - -export const andThen = <T, U, E>(r: Result<T, E>, f: (t: T) => Result<U, E>): Result<U, E> => - r.success ? f(r.data) : r; - -export const mapErr = <T, E, F>(r: Result<T, E>, f: (e: E) => F): Result<T, F> => - r.success ? r : err(f(r.error)); -``` - -- [ ] **Step 1.2: Write `src/util/time.ts`** - -```ts -export const sleep = (ms: number, signal?: AbortSignal): Promise<void> => - new Promise((resolve, reject) => { - if (signal?.aborted) { - reject(new Error("aborted")); - return; - } - const timer = setTimeout(() => { - signal?.removeEventListener("abort", onAbort); - resolve(); - }, ms); - const onAbort = (): void => { - clearTimeout(timer); - reject(new Error("aborted")); - }; - signal?.addEventListener("abort", onAbort, { once: true }); - }); - -export type Deadline = { - readonly remainingMs: () => number; - readonly expired: () => boolean; -}; - -export const deadline = (totalMs: number): Deadline => { - const end = Date.now() + totalMs; - return { - remainingMs: () => Math.max(0, end - Date.now()), - expired: () => Date.now() >= end, - }; -}; -``` - -- [ ] **Step 1.3: Write `src/util/paths.ts`** - -```ts -import { randomUUID } from "node:crypto"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; - -export const screenshotPath = (namespace: string, ext: "png" | "jpeg" = "png"): string => - join(tmpdir(), `pi-browser-screenshot-${namespace}-${randomUUID()}.${ext}`); - -export const pdfPath = (namespace: string): string => - join(tmpdir(), `pi-browser-pdf-${namespace}-${randomUUID()}.pdf`); -``` - -- [ ] **Step 1.4: Typecheck** - -Run: `npm run typecheck` -Expected: PASS (no errors). - -- [ ] **Step 1.5: Commit** - -```bash -git add src/util/result.ts src/util/time.ts src/util/paths.ts -git commit -m "feat(util): add result, time, and paths primitives for v0.3 rewrite" -``` - ---- - -## Task 2: Foundations — `util/keycodes.ts`, `util/js-template.ts` - -**Files:** -- Create: `src/util/keycodes.ts` -- Create: `src/util/js-template.ts` - -- [ ] **Step 2.1: Write `src/util/keycodes.ts`** (extracted from `daemon.ts:126-141` and `daemon.ts:815-822`) - -```ts -const VIRTUAL_KEY_CODES: Readonly<Record<string, number>> = { - Enter: 13, Tab: 9, Backspace: 8, Escape: 27, Delete: 46, " ": 32, - ArrowLeft: 37, ArrowUp: 38, ArrowRight: 39, ArrowDown: 40, - Home: 36, End: 35, PageUp: 33, PageDown: 34, -}; - -export const SPECIAL_KEYS: ReadonlyArray<string> = Object.keys(VIRTUAL_KEY_CODES); - -export const virtualKeyCode = (key: string): number => - VIRTUAL_KEY_CODES[key] ?? (key.length === 1 ? key.charCodeAt(0) : 0); - -export const keyCode = (key: string): string => key; -``` - -- [ ] **Step 2.2: Write `src/util/js-template.ts`** - -```ts -/** - * Build a JavaScript source string with safely-interpolated values. - * Every interpolated value is JSON.stringify'd, so strings, numbers, - * objects, and special characters all become valid JS literals. - * - * This is the ONLY supported way to build evaluation source in the - * codebase — never use raw template literals for JS that crosses the - * CDP boundary. - */ -export const safeJs = ( - strings: TemplateStringsArray, - ...values: ReadonlyArray<unknown> -): string => { - let out = strings[0] ?? ""; - for (let i = 0; i < values.length; i++) { - out += JSON.stringify(values[i]); - out += strings[i + 1] ?? ""; - } - return out; -}; -``` - -- [ ] **Step 2.3: Typecheck** - -Run: `npm run typecheck` -Expected: PASS. - -- [ ] **Step 2.4: Commit** - -```bash -git add src/util/keycodes.ts src/util/js-template.ts -git commit -m "feat(util): add pure keycodes and safeJs template helper" -``` - ---- - -## Task 3: Strict tsconfig (scoped to `src/util/**` for now) - -**Files:** -- Modify: `tsconfig.json` - -- [ ] **Step 3.1: Replace `tsconfig.json` with the strict config** - -```json -{ - "compilerOptions": { - "target": "ES2022", - "module": "ESNext", - "moduleResolution": "bundler", - "strict": true, - "noImplicitAny": true, - "strictNullChecks": true, - "noUnusedLocals": true, - "noUnusedParameters": true, - "noImplicitReturns": true, - "noFallthroughCasesInSwitch": true, - "noUncheckedIndexedAccess": true, - "exactOptionalPropertyTypes": true, - "noPropertyAccessFromIndexSignature": true, - "forceConsistentCasingInFileNames": true, - "esModuleInterop": true, - "skipLibCheck": true, - "outDir": "./dist", - "rootDir": "./src", - "declaration": true, - "declarationMap": true, - "sourceMap": true - }, - "include": ["src/**/*.ts"], - "exclude": ["node_modules", "dist"] -} -``` - -- [ ] **Step 3.2: Typecheck — expect failures in `daemon.ts`/`tools.ts`/`index.ts`** - -Run: `npm run typecheck` -Expected: errors in legacy files (we'll fix them as we migrate). Capture the count. - -- [ ] **Step 3.3: Add a temporary include carve-out so legacy code compiles during migration** - -Create: `tsconfig.legacy.json` - -```json -{ - "extends": "./tsconfig.json", - "compilerOptions": { - "noUnusedLocals": false, - "noUnusedParameters": false, - "noUncheckedIndexedAccess": false, - "exactOptionalPropertyTypes": false, - "noPropertyAccessFromIndexSignature": false - }, - "include": [ - "src/daemon.ts", - "src/tools.ts", - "src/index.ts", - "src/protocol.ts", - "src/renderers.ts", - "src/setup.ts", - "src/state.ts", - "src/prompt.ts" - ] -} -``` - -Modify `tsconfig.json` `include` to: -```json -"include": ["src/util/**/*.ts", "src/cdp/**/*.ts", "src/client.ts", "src/domains/**/*.ts", "src/schemas/**/*.ts", "src/registry.ts"] -``` - -Modify `package.json` `scripts.typecheck` to: `tsc --noEmit && tsc --noEmit -p tsconfig.legacy.json` - -- [ ] **Step 3.4: Typecheck — both projects must pass** - -Run: `npm run typecheck` -Expected: PASS. - -- [ ] **Step 3.5: Commit** - -```bash -git add tsconfig.json tsconfig.legacy.json package.json -git commit -m "build: enable strict tsconfig flags for new code, keep legacy lenient" -``` - ---- - -## Task 4: CDP types and errors - -**Files:** -- Create: `src/cdp/types.ts` -- Create: `src/cdp/errors.ts` - -- [ ] **Step 4.1: Write `src/cdp/types.ts`** - -```ts -export type CdpEvent = { - readonly method: string; - readonly params: unknown; - readonly sessionId?: string; -}; - -export type CdpRawMessage = { - readonly id?: number; - readonly method?: string; - readonly params?: Record<string, unknown>; - readonly result?: unknown; - readonly error?: { readonly message: string; readonly code?: number }; - readonly sessionId?: string; -}; - -export type DialogInfo = { - readonly type: "alert" | "confirm" | "prompt" | "beforeunload"; - readonly message: string; - readonly defaultPrompt?: string; -}; - -export type TabInfo = { - readonly targetId: string; - readonly title: string; - readonly url: string; -}; - -export type PageInfo = { - readonly url: string; - readonly title: string; - readonly width: number; - readonly height: number; - readonly scrollX: number; - readonly scrollY: number; - readonly pageWidth: number; - readonly pageHeight: number; -}; - -export type DaemonStatus = { - readonly alive: boolean; - readonly sessionId: string | null; - readonly namespace: string; - readonly remoteBrowserId?: string; -}; - -const INTERNAL_PREFIXES: ReadonlyArray<string> = [ - "chrome://", "chrome-untrusted://", "devtools://", "chrome-extension://", "about:", -]; - -export const isInternalUrl = (url: string): boolean => - INTERNAL_PREFIXES.some((p) => url.startsWith(p)); -``` - -- [ ] **Step 4.2: Write `src/cdp/errors.ts`** - -```ts -export type CdpErrorKind = - | "transport_closed" - | "timeout" - | "session_not_found" - | "remote_error" - | "discovery_failed" - | "invalid_response"; - -export type CdpError = { - readonly kind: CdpErrorKind; - readonly message: string; - readonly method?: string; -}; - -export const cdpError = ( - kind: CdpErrorKind, - message: string, - method?: string, -): CdpError => ({ kind, message, ...(method !== undefined ? { method } : {}) }); -``` - -- [ ] **Step 4.3: Typecheck** - -Run: `npm run typecheck` -Expected: PASS. - -- [ ] **Step 4.4: Commit** - -```bash -git add src/cdp/types.ts src/cdp/errors.ts -git commit -m "feat(cdp): add types and error model for new transport layer" -``` - ---- - -## Task 5: CDP discovery - -**Files:** -- Create: `src/cdp/discovery.ts` - -- [ ] **Step 5.1: Write `src/cdp/discovery.ts`** (port `daemon.ts:41-124` to Result) - -```ts -import { readFile } from "node:fs/promises"; -import { homedir } from "node:os"; -import { join } from "node:path"; -import { connect as netConnect } from "node:net"; -import { type Result, err, ok } from "../util/result"; -import { type CdpError, cdpError } from "./errors"; - -const PORT_PROBE_DEADLINE_MS = 30_000; -const PORT_PROBE_INTERVAL_MS = 1_000; - -const profileDirs = (): ReadonlyArray<string> => { - const home = homedir(); - return [ - join(home, "Library/Application Support/Google/Chrome"), - join(home, "Library/Application Support/Microsoft Edge"), - join(home, "Library/Application Support/Microsoft Edge Beta"), - join(home, "Library/Application Support/Microsoft Edge Dev"), - join(home, "Library/Application Support/Microsoft Edge Canary"), - join(home, ".config/google-chrome"), - join(home, ".config/chromium"), - join(home, ".config/chromium-browser"), - join(home, ".config/microsoft-edge"), - join(home, ".config/microsoft-edge-beta"), - join(home, ".config/microsoft-edge-dev"), - join(home, ".var/app/org.chromium.Chromium/config/chromium"), - join(home, ".var/app/com.google.Chrome/config/google-chrome"), - join(home, ".var/app/com.brave.Browser/config/BraveSoftware/Brave-Browser"), - join(home, ".var/app/com.microsoft.Edge/config/microsoft-edge"), - join(home, "AppData/Local/Google/Chrome/User Data"), - join(home, "AppData/Local/Chromium/User Data"), - join(home, "AppData/Local/Microsoft/Edge/User Data"), - join(home, "AppData/Local/Microsoft/Edge Beta/User Data"), - join(home, "AppData/Local/Microsoft/Edge Dev/User Data"), - join(home, "AppData/Local/Microsoft/Edge SxS/User Data"), - ]; -}; - -const probePort = (port: number): Promise<Result<void, CdpError>> => - new Promise((resolve) => { - const sock = netConnect({ host: "127.0.0.1", port }); - const finish = (r: Result<void, CdpError>): void => { - sock.destroy(); - resolve(r); - }; - sock.setTimeout(1000, () => finish(err(cdpError("discovery_failed", "probe timeout")))); - sock.once("error", (e) => finish(err(cdpError("discovery_failed", e.message)))); - sock.once("connect", () => { - sock.end(); - resolve(ok(undefined)); - }); - }); - -const waitForPort = async (port: number): Promise<Result<void, CdpError>> => { - const end = Date.now() + PORT_PROBE_DEADLINE_MS; - let lastMessage = "unknown"; - while (Date.now() < end) { - const probe = await probePort(port); - if (probe.success) return probe; - lastMessage = probe.error.message; - await new Promise((r) => setTimeout(r, PORT_PROBE_INTERVAL_MS)); - } - return err(cdpError( - "discovery_failed", - `Chrome's remote-debugging page is open, but DevTools is not live yet on 127.0.0.1:${port} — if Chrome opened a profile picker, choose your normal profile first, then tick the checkbox and click Allow if shown (last error: ${lastMessage})`, - )); -}; - -export const discoverWsUrl = async (): Promise<Result<string, CdpError>> => { - const dirs = profileDirs(); - for (const base of dirs) { - const portFile = join(base, "DevToolsActivePort"); - let raw: string; - try { - raw = await readFile(portFile, "utf8"); - } catch { - continue; - } - const lines = raw.trim().split("\n"); - if (lines.length < 2) continue; - const port = lines[0]?.trim(); - const path = lines[1]?.trim(); - if (!port || !path) continue; - const ready = await waitForPort(Number(port)); - if (!ready.success) return ready; - return ok(`ws://127.0.0.1:${port}${path}`); - } - return err(cdpError( - "discovery_failed", - `DevToolsActivePort not found in ${dirs.join(", ")} — open chrome://inspect/#remote-debugging in your browser, tick the checkbox, click Allow, then retry. Or set BU_CDP_WS to a remote browser endpoint.`, - )); -}; -``` - -- [ ] **Step 5.2: Typecheck** - -Run: `npm run typecheck` -Expected: PASS. - -- [ ] **Step 5.3: Commit** - -```bash -git add src/cdp/discovery.ts -git commit -m "feat(cdp): port DevToolsActivePort discovery to Result" -``` - ---- - -## Task 6: CDP transport - -**Files:** -- Create: `src/cdp/transport.ts` - -- [ ] **Step 6.1: Write `src/cdp/transport.ts`** - -```ts -import WebSocket from "ws"; -import { type Result, err, ok } from "../util/result"; -import { type CdpError, cdpError } from "./errors"; -import type { CdpEvent, CdpRawMessage } from "./types"; - -const DEFAULT_TIMEOUT_MS = 15_000; - -type Pending = { - readonly resolve: (v: Result<unknown, CdpError>) => void; - readonly timer: ReturnType<typeof setTimeout>; - readonly method: string; -}; - -export type CdpTransport = { - connect(url: string, opts?: { timeoutMs?: number }): Promise<Result<void, CdpError>>; - close(): Promise<void>; - request( - method: string, - params: Record<string, unknown>, - opts?: { sessionId?: string | null; timeoutMs?: number }, - ): Promise<Result<unknown, CdpError>>; - events(): AsyncIterable<CdpEvent>; - state(): "open" | "closed" | "connecting"; - onClose(cb: () => void): () => void; -}; - -export const createCdpTransport = (): CdpTransport => { - let ws: WebSocket | null = null; - let nextId = 1; - const pending = new Map<number, Pending>(); - const closeListeners = new Set<() => void>(); - - type EventQueue = { - readonly push: (e: CdpEvent) => void; - readonly end: () => void; - readonly iter: AsyncIterable<CdpEvent>; - }; - - const makeEventQueue = (): EventQueue => { - const buf: CdpEvent[] = []; - const waiters: Array<(v: IteratorResult<CdpEvent>) => void> = []; - let ended = false; - return { - push(e) { - if (ended) return; - const w = waiters.shift(); - if (w) w({ value: e, done: false }); - else buf.push(e); - }, - end() { - ended = true; - for (const w of waiters.splice(0)) w({ value: undefined as unknown as CdpEvent, done: true }); - }, - iter: { - [Symbol.asyncIterator]() { - return { - next: (): Promise<IteratorResult<CdpEvent>> => - new Promise((resolve) => { - const next = buf.shift(); - if (next) resolve({ value: next, done: false }); - else if (ended) resolve({ value: undefined as unknown as CdpEvent, done: true }); - else waiters.push(resolve); - }), - }; - }, - }, - }; - }; - - let queue = makeEventQueue(); - - const handleMessage = (raw: string): void => { - let msg: CdpRawMessage; - try { - msg = JSON.parse(raw); - } catch { - return; - } - if (msg.id === undefined) { - if (msg.method) { - queue.push({ - method: msg.method, - params: msg.params, - ...(msg.sessionId !== undefined ? { sessionId: msg.sessionId } : {}), - }); - } - return; - } - const p = pending.get(msg.id); - if (!p) return; - pending.delete(msg.id); - clearTimeout(p.timer); - if (msg.error) { - const kind = msg.error.message.includes("Session with given id not found") - ? "session_not_found" - : "remote_error"; - p.resolve(err(cdpError(kind, msg.error.message, p.method))); - return; - } - p.resolve(ok(msg.result)); - }; - - const cleanup = (reason: string): void => { - for (const [, p] of pending) { - clearTimeout(p.timer); - p.resolve(err(cdpError("transport_closed", reason, p.method))); - } - pending.clear(); - queue.end(); - queue = makeEventQueue(); - for (const cb of closeListeners) cb(); - }; - - return { - connect(url, opts = {}): Promise<Result<void, CdpError>> { - const timeoutMs = opts.timeoutMs ?? 10_000; - return new Promise((resolve) => { - let settled = false; - const settle = (r: Result<void, CdpError>): void => { - if (settled) return; - settled = true; - resolve(r); - }; - try { - ws = new WebSocket(url); - } catch (e) { - settle(err(cdpError("transport_closed", e instanceof Error ? e.message : String(e)))); - return; - } - const timer = setTimeout(() => { - ws?.close(); - ws = null; - settle(err(cdpError("timeout", `CDP WebSocket connection timed out after ${timeoutMs}ms`))); - }, timeoutMs); - ws.onopen = () => { - clearTimeout(timer); - settle(ok(undefined)); - }; - ws.onmessage = (ev: WebSocket.MessageEvent) => { - const data = ev.data; - handleMessage(typeof data === "string" ? data : data.toString()); - }; - ws.onerror = () => { - clearTimeout(timer); - settle(err(cdpError("transport_closed", "CDP WebSocket error during connection"))); - }; - ws.onclose = () => { - clearTimeout(timer); - ws = null; - cleanup("WebSocket closed"); - settle(err(cdpError("transport_closed", "CDP WebSocket closed"))); - }; - }); - }, - close(): Promise<void> { - if (ws) { - try { ws.close(1000, "Shutdown"); } catch { /* best effort */ } - ws = null; - } - cleanup("close() called"); - return Promise.resolve(); - }, - request(method, params, opts = {}): Promise<Result<unknown, CdpError>> { - const timeoutMs = opts.timeoutMs ?? DEFAULT_TIMEOUT_MS; - const sessionId = opts.sessionId ?? null; - if (!ws || ws.readyState !== WebSocket.OPEN) { - return Promise.resolve(err(cdpError("transport_closed", "Browser not connected. Is Chrome running?", method))); - } - const id = nextId++; - const payload: Record<string, unknown> = { id, method, params }; - if (sessionId) payload.sessionId = sessionId; - const json = JSON.stringify(payload); - return new Promise((resolve) => { - const timer = setTimeout(() => { - pending.delete(id); - resolve(err(cdpError("timeout", `CDP timeout after ${timeoutMs}ms: ${method}`, method))); - }, timeoutMs); - pending.set(id, { resolve, timer, method }); - try { - ws!.send(json); - } catch (e) { - clearTimeout(timer); - pending.delete(id); - resolve(err(cdpError("transport_closed", e instanceof Error ? e.message : String(e), method))); - } - }); - }, - events(): AsyncIterable<CdpEvent> { - return queue.iter; - }, - state(): "open" | "closed" | "connecting" { - if (!ws) return "closed"; - if (ws.readyState === WebSocket.CONNECTING) return "connecting"; - if (ws.readyState === WebSocket.OPEN) return "open"; - return "closed"; - }, - onClose(cb): () => void { - closeListeners.add(cb); - return () => closeListeners.delete(cb); - }, - }; -}; -``` - -- [ ] **Step 6.2: Typecheck** - -Run: `npm run typecheck` -Expected: PASS. - -- [ ] **Step 6.3: Commit** - -```bash -git add src/cdp/transport.ts -git commit -m "feat(cdp): add factory-based transport with Result and event AsyncIterable" -``` - ---- - -## Task 7: CDP session + client - -**Files:** -- Create: `src/cdp/session.ts` -- Create: `src/client.ts` - -- [ ] **Step 7.1: Write `src/cdp/session.ts`** - -```ts -import { type Result, err, ok } from "../util/result"; -import { type CdpError, cdpError } from "./errors"; -import type { CdpEvent, DialogInfo, PageInfo } from "./types"; -import { isInternalUrl } from "./types"; -import type { CdpTransport } from "./transport"; - -export type CdpSession = { - attachFirstPage(): Promise<Result<{ readonly targetId: string; readonly sessionId: string }, CdpError>>; - switchTo(targetId: string): Promise<Result<void, CdpError>>; - current(): { readonly sessionId: string; readonly targetId: string } | null; - call(method: string, params?: Record<string, unknown>, opts?: { timeoutMs?: number }): Promise<Result<unknown, CdpError>>; - callOnTarget(method: string, params: Record<string, unknown>, sessionId: string, opts?: { timeoutMs?: number }): Promise<Result<unknown, CdpError>>; - callBrowser(method: string, params?: Record<string, unknown>, opts?: { timeoutMs?: number }): Promise<Result<unknown, CdpError>>; - takeDialog(): DialogInfo | null; - drainPageInfoInvalidations(): boolean; -}; - -export const createCdpSession = (transport: CdpTransport): CdpSession => { - let sessionId: string | null = null; - let targetId: string | null = null; - let dialog: DialogInfo | null = null; - let pageInfoDirty = false; - - const consumeEvents = async (): Promise<void> => { - for await (const ev of transport.events()) { - if (ev.method === "Page.javascriptDialogOpening") { - const params = ev.params as Partial<DialogInfo> | undefined; - dialog = { - type: (params?.type as DialogInfo["type"]) ?? "alert", - message: params?.message ?? "", - ...(params?.defaultPrompt !== undefined ? { defaultPrompt: params.defaultPrompt } : {}), - }; - continue; - } - if (ev.method === "Page.javascriptDialogClosed") { - dialog = null; - continue; - } - if (ev.method === "Page.frameNavigated" || ev.method === "Page.loadEventFired") { - pageInfoDirty = true; - } - } - }; - - void consumeEvents(); - transport.onClose(() => { - sessionId = null; - targetId = null; - dialog = null; - pageInfoDirty = false; - void consumeEvents(); - }); - - const enableDomains = async (sid: string): Promise<void> => { - for (const d of ["Page", "DOM", "Runtime", "Network"]) { - await transport.request(`${d}.enable`, {}, { sessionId: sid }); - } - }; - - return { - async attachFirstPage() { - const targets = await transport.request("Target.getTargets", {}, { sessionId: null }); - if (!targets.success) return targets; - const data = targets.data as { targetInfos: ReadonlyArray<{ targetId: string; type: string; url: string }> }; - let pages = data.targetInfos.filter((t) => t.type === "page" && !isInternalUrl(t.url)); - if (pages.length === 0) { - const created = await transport.request("Target.createTarget", { url: "about:blank" }, { sessionId: null }); - if (!created.success) return created; - const c = created.data as { targetId: string }; - pages = [{ targetId: c.targetId, type: "page", url: "about:blank" }]; - } - const first = pages[0]; - if (!first) return err(cdpError("invalid_response", "no page targets after creation")); - const attached = await transport.request("Target.attachToTarget", { targetId: first.targetId, flatten: true }, { sessionId: null }); - if (!attached.success) return attached; - const a = attached.data as { sessionId: string }; - sessionId = a.sessionId; - targetId = first.targetId; - await enableDomains(a.sessionId); - return ok({ targetId: first.targetId, sessionId: a.sessionId }); - }, - async switchTo(tid) { - const activated = await transport.request("Target.activateTarget", { targetId: tid }, { sessionId: null }); - if (!activated.success) return activated; - const attached = await transport.request("Target.attachToTarget", { targetId: tid, flatten: true }, { sessionId: null }); - if (!attached.success) return attached; - const a = attached.data as { sessionId: string }; - sessionId = a.sessionId; - targetId = tid; - pageInfoDirty = true; - await enableDomains(a.sessionId); - return ok(undefined); - }, - current() { - return sessionId && targetId ? { sessionId, targetId } : null; - }, - call(method, params = {}, opts = {}) { - return transport.request(method, params, { sessionId, ...(opts.timeoutMs !== undefined ? { timeoutMs: opts.timeoutMs } : {}) }); - }, - callOnTarget(method, params, sid, opts = {}) { - return transport.request(method, params, { sessionId: sid, ...(opts.timeoutMs !== undefined ? { timeoutMs: opts.timeoutMs } : {}) }); - }, - callBrowser(method, params = {}, opts = {}) { - return transport.request(method, params, { sessionId: null, ...(opts.timeoutMs !== undefined ? { timeoutMs: opts.timeoutMs } : {}) }); - }, - takeDialog() { - const d = dialog; - dialog = null; - return d; - }, - drainPageInfoInvalidations() { - const dirty = pageInfoDirty; - pageInfoDirty = false; - return dirty; - }, - }; -}; - -export type CachedPage = { readonly info: PageInfo; readonly at: number }; - -export const isPageInfoFresh = (cache: CachedPage | null, dirty: boolean, ttlMs: number): boolean => - cache !== null && !dirty && Date.now() - cache.at < ttlMs; -``` - -- [ ] **Step 7.2: Write `src/client.ts`** (skeleton — domains add concrete methods) - -```ts -import { writeFile } from "node:fs/promises"; -import { type Result, andThen, err, ok } from "./util/result"; -import { safeJs } from "./util/js-template"; -import { discoverWsUrl } from "./cdp/discovery"; -import { type CdpError, cdpError } from "./cdp/errors"; -import type { CdpTransport } from "./cdp/transport"; -import { createCdpTransport } from "./cdp/transport"; -import { type CdpSession, createCdpSession } from "./cdp/session"; -import type { DaemonStatus, DialogInfo, PageInfo, TabInfo } from "./cdp/types"; - -export type BrowserClientOptions = { - readonly namespace: string; - readonly remote?: { readonly cdpUrl: string; readonly browserId: string }; -}; - -export type BrowserClient = { - readonly namespace: string; - ensureAlive(): Promise<Result<void, CdpError>>; - status(): DaemonStatus; - start(): Promise<Result<void, CdpError>>; - stop(): Promise<void>; - raw: (method: string, params?: Record<string, unknown>, sessionId?: string) => Promise<Result<unknown, CdpError>>; - evaluateJs(expression: string, sessionId?: string): Promise<Result<unknown, CdpError>>; - pageInfo(): Promise<Result<PageInfo | { readonly dialog: DialogInfo }, CdpError>>; - takeDialog(): DialogInfo | null; - listTabs(includeInternal?: boolean): Promise<Result<ReadonlyArray<TabInfo>, CdpError>>; - switchTab(targetId: string): Promise<Result<void, CdpError>>; - newTab(url?: string): Promise<Result<string, CdpError>>; - current(): { readonly sessionId: string; readonly targetId: string } | null; - session(): CdpSession; - transport(): CdpTransport; -}; - -const HEALTH_TTL_MS = 30_000; -const PAGE_INFO_TTL_MS = 1_000; - -export const createBrowserClient = (opts: BrowserClientOptions): BrowserClient => { - const transport = createCdpTransport(); - const session = createCdpSession(transport); - let lastHealth = 0; - let pageCache: { readonly info: PageInfo; readonly at: number } | null = null; - let remote = opts.remote ?? null; - - const start = async (): Promise<Result<void, CdpError>> => { - if (transport.state() === "open" && session.current()) return ok(undefined); - const wsUrl = remote?.cdpUrl - ?? process.env["BU_CDP_WS"] - ?? (await (async () => { - const r = await discoverWsUrl(); - return r.success ? r.data : null; - })()); - if (!wsUrl) { - const r = await discoverWsUrl(); - if (!r.success) return r; - const connected = await transport.connect(r.data, { timeoutMs: 10_000 }); - if (!connected.success) return connected; - } else { - const connected = await transport.connect(wsUrl, { timeoutMs: 10_000 }); - if (!connected.success) return connected; - remote = remote ?? { cdpUrl: wsUrl, browserId: wsUrl.split("/").pop() ?? "unknown" }; - } - const attached = await session.attachFirstPage(); - if (!attached.success) { - await transport.close(); - return attached; - } - lastHealth = Date.now(); - pageCache = null; - return ok(undefined); - }; - - const stop = async (): Promise<void> => { - await transport.close(); - pageCache = null; - lastHealth = 0; - }; - - const ensureAlive = async (): Promise<Result<void, CdpError>> => { - if (transport.state() !== "open" || !session.current()) { - await stop(); - return start(); - } - if (Date.now() - lastHealth < HEALTH_TTL_MS) return ok(undefined); - const probe = await transport.request("Target.getTargets", {}, { sessionId: null, timeoutMs: 2_000 }); - if (probe.success) { - lastHealth = Date.now(); - return ok(undefined); - } - await stop(); - return start(); - }; - - const evaluateJs = async (expression: string, sessionId?: string): Promise<Result<unknown, CdpError>> => { - const wrapped = expression.includes("return ") && !expression.trim().startsWith("(") - ? `(function(){${expression}})()` - : expression; - const r = sessionId - ? await session.callOnTarget("Runtime.evaluate", { expression: wrapped, returnByValue: true, awaitPromise: true }, sessionId) - : await session.call("Runtime.evaluate", { expression: wrapped, returnByValue: true, awaitPromise: true }); - if (!r.success) return r; - const data = r.data as { result?: { value?: unknown }; exceptionDetails?: unknown }; - if (data.exceptionDetails) { - return err(cdpError("remote_error", `JS evaluation failed: ${JSON.stringify(data.exceptionDetails)}`, "Runtime.evaluate")); - } - return ok(data.result?.value); - }; - - const readPageInfo = async (): Promise<Result<PageInfo, CdpError>> => { - const dirty = session.drainPageInfoInvalidations(); - if (pageCache && !dirty && Date.now() - pageCache.at < PAGE_INFO_TTL_MS) return ok(pageCache.info); - const expr = safeJs`JSON.stringify({url:location.href,title:document.title,w:innerWidth,h:innerHeight,sx:scrollX,sy:scrollY,pw:document.documentElement.scrollWidth,ph:document.documentElement.scrollHeight})`; - const raw = await evaluateJs(expr); - if (!raw.success) return raw; - if (typeof raw.data !== "string") return err(cdpError("invalid_response", "page info evaluation did not return a string")); - const parsedRaw = JSON.parse(raw.data) as { url: string; title: string; w: number; h: number; sx: number; sy: number; pw: number; ph: number }; - const info: PageInfo = { - url: parsedRaw.url, title: parsedRaw.title, - width: parsedRaw.w, height: parsedRaw.h, - scrollX: parsedRaw.sx, scrollY: parsedRaw.sy, - pageWidth: parsedRaw.pw, pageHeight: parsedRaw.ph, - }; - pageCache = { info, at: Date.now() }; - return ok(info); - }; - - const pageInfo = async (): Promise<Result<PageInfo | { readonly dialog: DialogInfo }, CdpError>> => { - const d = session.takeDialog(); - if (d) return ok({ dialog: d }); - return readPageInfo(); - }; - - const listTabs = async (includeInternal = true): Promise<Result<ReadonlyArray<TabInfo>, CdpError>> => { - const r = await session.callBrowser("Target.getTargets"); - if (!r.success) return r; - const data = r.data as { targetInfos: ReadonlyArray<{ targetId: string; type: string; title: string; url: string }> }; - const tabs = data.targetInfos - .filter((t) => t.type === "page") - .filter((t) => includeInternal || !t.url.startsWith("chrome://")) - .map((t): TabInfo => ({ targetId: t.targetId, title: t.title, url: t.url })); - return ok(tabs); - }; - - const switchTab = async (targetId: string): Promise<Result<void, CdpError>> => { - const r = await session.switchTo(targetId); - if (!r.success) return r; - pageCache = null; - await session.call("Runtime.evaluate", { expression: safeJs`if(!document.title.startsWith('🟢'))document.title='🟢 '+document.title` }); - return ok(undefined); - }; - - const newTab = async (url?: string): Promise<Result<string, CdpError>> => { - const created = await session.callBrowser("Target.createTarget", { url: "about:blank" }); - if (!created.success) return created; - const c = created.data as { targetId: string }; - const switched = await switchTab(c.targetId); - if (!switched.success) return switched; - if (url && url !== "about:blank") { - const nav = await session.call("Page.navigate", { url }); - if (!nav.success) return nav; - pageCache = null; - } - return ok(c.targetId); - }; - - const status = (): DaemonStatus => ({ - alive: transport.state() === "open" && session.current() !== null, - sessionId: session.current()?.sessionId ?? null, - namespace: opts.namespace, - ...(remote?.browserId !== undefined ? { remoteBrowserId: remote.browserId } : {}), - }); - - return { - namespace: opts.namespace, - ensureAlive, status, start, stop, - raw: (method, params, sessionId) => session.call(method, params, sessionId !== undefined ? { } : {}), - evaluateJs, pageInfo, - takeDialog: () => session.takeDialog(), - listTabs, switchTab, newTab, - current: () => session.current(), - session: () => session, - transport: () => transport, - }; -}; -``` - -(Note: this client deliberately exposes lower-level methods; domain files extend behavior by composing these. Domain-specific helpers like `screenshot.captureWithCrosshair` live in their domain files since they need `sharp` etc., not in `client.ts`.) - -- [ ] **Step 7.3: Typecheck** - -Run: `npm run typecheck` -Expected: PASS. - -- [ ] **Step 7.4: Commit** - -```bash -git add src/cdp/session.ts src/client.ts -git commit -m "feat: add CDP session and BrowserClient factory" -``` - ---- - -## Task 8: `defineTool` helper + registry skeleton - -**Files:** -- Create: `src/util/tool.ts` -- Create: `src/util/truncate.ts` -- Create: `src/registry.ts` -- Create: `src/schemas/common.ts` - -- [ ] **Step 8.1: Write `src/util/truncate.ts`** (port the existing `applyTruncation` from `tools.ts:62-94`) - -```ts -import { mkdtemp, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; -import { - DEFAULT_MAX_BYTES, - DEFAULT_MAX_LINES, - formatSize, - truncateHead, - withFileMutationQueue, -} from "@mariozechner/pi-coding-agent"; - -const tempDirs: string[] = []; - -export const cleanupTempDirs = async (): Promise<void> => { - const dirs = tempDirs.splice(0); - await Promise.all(dirs.map((d) => rm(d, { recursive: true, force: true }).catch(() => {}))); -}; - -export type TruncatedOutput = { - readonly text: string; - readonly fullOutputPath?: string; - readonly wasTruncated: boolean; -}; - -export const applyTruncation = async (output: string, prefix: string): Promise<TruncatedOutput> => { - const t = truncateHead(output, { maxLines: DEFAULT_MAX_LINES, maxBytes: DEFAULT_MAX_BYTES }); - if (!t.truncated) return { text: t.content, wasTruncated: false }; - const dir = await mkdtemp(join(tmpdir(), `pi-bh-${prefix}-`)); - tempDirs.push(dir); - const file = join(dir, "output.txt"); - await withFileMutationQueue(file, async () => { await writeFile(file, output, "utf8"); }); - const omitted = t.totalBytes - t.outputBytes; - const text = `${t.content}\n\n[Output truncated: ${t.outputLines} of ${t.totalLines} lines (${formatSize(t.outputBytes)} of ${formatSize(t.totalBytes)}). ${formatSize(omitted)} omitted. Full output: ${file}]`; - return { text, fullOutputPath: file, wasTruncated: true }; -}; -``` - -- [ ] **Step 8.2: Write `src/util/tool.ts`** - -```ts -import type { TSchema, Static } from "typebox"; -import type { ExtensionAPI } from "@mariozechner/pi-coding-agent"; -import type { BrowserClient } from "../client"; -import type { Result } from "./result"; - -export type ToolOk = { - readonly text: string; - readonly details?: Readonly<Record<string, unknown>>; -}; - -export type ToolErrKind = - | "not_connected" | "cdp_error" | "timeout" - | "invalid_state" | "io_error" | "internal"; - -export type ToolErr = { - readonly kind: ToolErrKind; - readonly message: string; - readonly details?: Readonly<Record<string, unknown>>; -}; - -export type HandlerContext = { - readonly client: BrowserClient; - readonly signal: AbortSignal; - readonly onUpdate: (update: ToolOk) => void; - readonly extensionCtx: unknown; -}; - -export type ToolHandler<S extends TSchema> = ( - args: Static<S>, - ctx: HandlerContext, -) => Promise<Result<ToolOk, ToolErr>>; - -export type ToolDefinition<S extends TSchema> = { - readonly name: string; - readonly label: string; - readonly description: string; - readonly promptSnippet: string; - readonly promptGuidelines: ReadonlyArray<string>; - readonly parameters: S; - readonly handler: ToolHandler<S>; - readonly renderCall?: (args: Static<S>, theme: unknown) => unknown; - readonly renderResult?: (result: unknown, opts: { readonly expanded: boolean }, theme: unknown) => unknown; - readonly ensureAlive?: boolean; -}; - -// Type-erased registration entry for the registry array. -export type RegisteredTool = ToolDefinition<TSchema>; - -export const defineTool = <S extends TSchema>(def: ToolDefinition<S>): ToolDefinition<S> => def; - -type ToolResultLike = { - isError?: boolean; - content: ReadonlyArray<{ readonly type: "text"; readonly text: string }>; - details?: Readonly<Record<string, unknown>>; -}; - -const toToolResult = (r: Result<ToolOk, ToolErr>, toolName: string): ToolResultLike => { - if (r.success) { - return { - content: [{ type: "text", text: r.data.text }], - details: { ok: true, ...(r.data.details ?? {}) }, - }; - } - return { - isError: true, - content: [{ type: "text", text: `${toolName} failed (${r.error.kind}): ${r.error.message}` }], - details: { ok: false, kind: r.error.kind, message: r.error.message, ...(r.error.details ?? {}) }, - }; -}; - -export const registerTool = <S extends TSchema>( - pi: ExtensionAPI, - client: BrowserClient, - def: ToolDefinition<S>, -): void => { - pi.registerTool({ - name: def.name, - label: def.label, - description: def.description, - promptSnippet: def.promptSnippet, - promptGuidelines: [...def.promptGuidelines], - parameters: def.parameters, - ...(def.renderCall ? { renderCall: def.renderCall } : {}), - ...(def.renderResult ? { renderResult: def.renderResult } : {}), - async execute(_id: string, args: unknown, signal: AbortSignal, onUpdate, extensionCtx) { - if (def.ensureAlive !== false) { - const alive = await client.ensureAlive(); - if (!alive.success) { - return toToolResult( - { success: false, error: { kind: "not_connected", message: alive.error.message } }, - def.name, - ); - } - } - const result = await def.handler(args as Static<S>, { - client, - signal, - onUpdate: (u) => onUpdate?.({ content: [{ type: "text", text: u.text }], details: u.details }), - extensionCtx, - }); - return toToolResult(result, def.name); - }, - }); -}; -``` - -- [ ] **Step 8.3: Write `src/schemas/common.ts`** - -```ts -import { Type } from "typebox"; - -export const Coords = Type.Object({ - x: Type.Number({ description: "X coordinate in CSS pixels from left edge of viewport" }), - y: Type.Number({ description: "Y coordinate in CSS pixels from top edge of viewport" }), -}); - -export const MouseButton = Type.Union([ - Type.Literal("left"), - Type.Literal("right"), - Type.Literal("middle"), -], { description: 'Mouse button: "left", "right", or "middle"' }); -``` - -- [ ] **Step 8.4: Write `src/registry.ts`** (empty array for now; populated as domains migrate) - -```ts -import type { ExtensionAPI } from "@mariozechner/pi-coding-agent"; -import type { BrowserClient } from "./client"; -import { type RegisteredTool, registerTool } from "./util/tool"; - -const TOOLS: ReadonlyArray<RegisteredTool> = [ - // domain tools registered here, one per domain file -]; - -export const registerAllTools = (pi: ExtensionAPI, client: BrowserClient): void => { - for (const t of TOOLS) registerTool(pi, client, t); -}; -``` - -- [ ] **Step 8.5: Typecheck** - -Run: `npm run typecheck` -Expected: PASS (TOOLS array is empty so no concrete usage yet). - -- [ ] **Step 8.6: Commit** - -```bash -git add src/util/tool.ts src/util/truncate.ts src/schemas/common.ts src/registry.ts -git commit -m "feat: add defineTool helper, truncation, and empty registry" -``` - ---- - -## Task 9: First vertical slice — `domains/click.ts` - -This is the validation moment for the architecture. After this task, `browser_click` is served by the new path; the other 27 tools still go through `tools.ts`. - -**Files:** -- Create: `src/domains/click.ts` -- Modify: `src/registry.ts` (add `clickTool`) -- Modify: `src/tools.ts` (remove the `browser_click` registration block, lines ~324-396) -- Modify: `src/index.ts` (call `registerAllTools` alongside the existing `registerTools`) - -- [ ] **Step 9.1: Write `src/domains/click.ts`** - -```ts -import { Type, type Static } from "typebox"; -import { Text } from "@mariozechner/pi-tui"; -import type { Result } from "../util/result"; -import { ok, err } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; -import { Coords, MouseButton } from "../schemas/common"; -import { screenshotPath } from "../util/paths"; - -const ClickArgs = Type.Object({ - ...Coords.properties, - button: Type.Optional(MouseButton), - count: Type.Optional(Type.Integer({ minimum: 1, maximum: 3, default: 1, description: "Number of clicks (1 = single, 2 = double). Default: 1" })), -}); -type ClickArgsT = Static<typeof ClickArgs>; - -const dispatchClick = async ( - client: import("../client").BrowserClient, - args: ClickArgsT, -): Promise<Result<void, ToolErr>> => { - const button = args.button ?? "left"; - const count = args.count ?? 1; - const pressed = await client.session().call("Input.dispatchMouseEvent", { - type: "mousePressed", x: args.x, y: args.y, button, clickCount: count, - }); - if (!pressed.success) return err({ kind: "cdp_error", message: pressed.error.message }); - const released = await client.session().call("Input.dispatchMouseEvent", { - type: "mouseReleased", x: args.x, y: args.y, button, clickCount: count, - }); - if (!released.success) return err({ kind: "cdp_error", message: released.error.message }); - return ok(undefined); -}; - -export const clickTool = defineTool({ - name: "browser_click", - label: "Browser Click", - description: "Click at viewport CSS-pixel coordinates. Compositor-level click works through iframes, shadow DOM, and cross-origin content. Use browser_screenshot first to find coordinates.", - promptSnippet: "Click at pixel coordinates (x, y) on the page", - promptGuidelines: [ - "Use browser_click for all clicks. Coordinates are viewport CSS pixels (not device pixels).", - "Capture a browser_screenshot BEFORE clicking to find the right coordinates.", - "Capture another browser_screenshot AFTER clicking to verify the action worked.", - "Compositor-level clicks pass through iframes, shadow DOM, and cross-origin content — no selector needed.", - "If a click doesn't register, try setting BH_DEBUG_CLICKS=1 to get annotated screenshots showing exact click positions.", - "For React/Vue components that don't respond to clicks, try browser_dispatch_key to send DOM-level events.", - ], - parameters: ClickArgs, - renderCall: (a) => new Text(`🖱️ Click at (${a.x}, ${a.y})`, 0, 0), - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const clicked = await dispatchClick(client, args); - if (!clicked.success) return clicked; - if (process.env["BH_DEBUG_CLICKS"]) { - const path = screenshotPath(client.namespace, "png"); - // Debug overlay implementation lives in domains/screenshot.ts which we - // haven't migrated yet — for now, just produce a normal screenshot. - const shot = await client.session().call("Page.captureScreenshot", { format: "png" }); - if (shot.success) { - const data = (shot.data as { data: string }).data; - const { writeFile } = await import("node:fs/promises"); - await writeFile(path, Buffer.from(data, "base64")); - return ok({ - text: `Clicked at (${args.x}, ${args.y})\n[DEBUG] Screenshot: ${path}`, - details: { debugScreenshotPath: path, x: args.x, y: args.y }, - }); - } - } - return ok({ text: `Clicked at (${args.x}, ${args.y})`, details: { x: args.x, y: args.y } }); - }, -}); -``` - -(The crosshair overlay returns in Task 14 when we migrate `domains/screenshot.ts`; for now the debug branch just saves a plain screenshot.) - -- [ ] **Step 9.2: Add `clickTool` to `src/registry.ts`** - -```ts -import type { ExtensionAPI } from "@mariozechner/pi-coding-agent"; -import type { BrowserClient } from "./client"; -import { type RegisteredTool, registerTool } from "./util/tool"; -import { clickTool } from "./domains/click"; - -const TOOLS: ReadonlyArray<RegisteredTool> = [ - clickTool, -]; - -export const registerAllTools = (pi: ExtensionAPI, client: BrowserClient): void => { - for (const t of TOOLS) registerTool(pi, client, t); -}; -``` - -- [ ] **Step 9.3: Remove the `browser_click` block from `src/tools.ts`** (lines ~321-396 in current code, including the comment header). The rest of the file stays untouched. - -- [ ] **Step 9.4: Wire the new client into `src/index.ts`** - -In `src/index.ts`, around the existing `if (!toolsRegistered && daemon) { registerTools(pi, daemon); ... }` block, add a parallel client/registry path. The legacy `daemon` and `tools.ts` continue to serve everything except `browser_click`. - -```ts -// Near the top, alongside the daemon import: -import { createBrowserClient, type BrowserClient } from "./client"; -import { registerAllTools } from "./registry"; - -// In the function scope, alongside `let daemon`: -let client: BrowserClient | null = null; - -// Inside session_start, AFTER `daemon = new BrowserDaemon(state.namespace)`: -client = createBrowserClient({ namespace: state.namespace }); -try { await client.start(); } catch { /* surfaced via tool errors */ } - -// Inside the `if (!toolsRegistered && daemon)` block, AFTER `registerTools(pi, daemon)`: -if (client) registerAllTools(pi, client); - -// Inside session_shutdown, AFTER `await daemon.stop()`: -if (client) { try { await client.stop(); } catch { /* best-effort */ } client = null; } -``` - -(Both `daemon` and `client` connect to the same Chrome via separate WebSockets during the migration window. This is fine — Chrome accepts multiple CDP connections.) - -- [ ] **Step 9.5: Typecheck** - -Run: `npm run typecheck` -Expected: PASS. - -- [ ] **Step 9.6: Manual smoke (vertical slice)** - -Pre-req: Chrome running with remote debugging enabled (chrome://inspect/#remote-debugging → Allow). - -1. `cd benchmark && node -e "require('../src/index.ts')"` — won't work directly because pi loads it; instead, install into a local pi workspace per CONTRIBUTING.md, then in pi: - - `/browser-setup` - - `browser_navigate("https://example.com")` - - `browser_screenshot()` → note coordinates of "More information..." link - - `browser_click({x: <X>, y: <Y>})` ← this hits the new path - - `browser_screenshot()` to confirm navigation - -If `browser_click` works and the result `details` contains `{ ok: true, x, y }`, the architecture is validated. If not, debug before proceeding. - -- [ ] **Step 9.7: Commit** - -```bash -git add src/domains/click.ts src/registry.ts src/tools.ts src/index.ts -git commit -m "feat(domains): migrate browser_click to defineTool — vertical slice" -``` - ---- - -## Tasks 10–22: Bulk domain migration - -Each task follows the same pattern as Task 9 but only the schema/handler/legacy-removal differ. To keep this plan manageable, the per-task structure is: - -- Step 1: Write `src/domains/<name>.ts` using `defineTool`. Schemas live in the file; types via `Static<>`. Handler returns `Result<ToolOk, ToolErr>`. JS evaluation uses `safeJs`. CDP calls use `client.session().call(...)` or `client.session().callBrowser(...)`. -- Step 2: Add the new tool(s) to `TOOLS` array in `src/registry.ts`, in the listed order. -- Step 3: Remove the corresponding tool registration block(s) from `src/tools.ts`. -- Step 4: `npm run typecheck` → PASS. -- Step 5: Commit with message `feat(domains): migrate <tool-names>`. - -The detailed schema + handler skeleton for each domain follows. - ---- - -### Task 10: `domains/keyboard.ts` — `browser_type`, `browser_press_key`, `browser_dispatch_key` - -**Files:** -- Create: `src/domains/keyboard.ts` -- Modify: `src/registry.ts`, `src/tools.ts` - -- [ ] **Step 10.1: Write `src/domains/keyboard.ts`** - -```ts -import { Type, type Static } from "typebox"; -import { type Result, err, ok } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; -import { safeJs } from "../util/js-template"; -import { virtualKeyCode } from "../util/keycodes"; -import type { BrowserClient } from "../client"; - -const TypeArgs = Type.Object({ text: Type.String({ description: "Text to type" }) }); - -export const typeTool = defineTool({ - name: "browser_type", - label: "Browser Type", - description: "Type text into the currently focused element. Use browser_click first to focus an input field.", - promptSnippet: "Type text into the focused element", - promptGuidelines: [ - "Use browser_type to enter text. Click on an input field with browser_click first to focus it.", - "For special keys (Enter, Tab, Escape, arrows), use browser_press_key instead.", - ], - parameters: TypeArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const r = await client.session().call("Input.insertText", { text: args.text }); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - return ok({ text: `Typed: "${args.text}"` }); - }, -}); - -const PressKeyArgs = Type.Object({ - key: Type.String({ description: 'Key to press. Special keys: Enter, Tab, Backspace, Escape, Delete, ArrowLeft/Up/Right/Down, Home, End, PageUp, PageDown. Space as " "' }), - modifiers: Type.Optional(Type.Integer({ minimum: 0, maximum: 15, description: "Modifier bitfield: 1=Alt, 2=Ctrl, 4=Meta(Cmd), 8=Shift. Combine with OR." })), -}); - -export const pressKeyTool = defineTool({ - name: "browser_press_key", - label: "Browser Press Key", - description: "Press a keyboard key. Supports special keys (Enter, Tab, Backspace, Escape, Delete, arrows, Home, End, PageUp, PageDown, Space as ' ') and regular characters. Optional modifier bitfield: 1=Alt, 2=Ctrl, 4=Meta/Cmd, 8=Shift.", - promptSnippet: "Press a key (Enter, Tab, Escape, arrows, or any character)", - promptGuidelines: [ - "Use browser_press_key for keyboard shortcuts and navigation keys.", - "Special key names: Enter, Tab, Backspace, Escape, Delete, ArrowLeft, ArrowUp, ArrowRight, ArrowDown, Home, End, PageUp, PageDown.", - "Use Space as ' ' (a single space character).", - "Modifiers: 1=Alt, 2=Ctrl, 4=Meta(Cmd), 8=Shift. Combine with bitwise OR: Ctrl+Shift = 2|8 = 10.", - ], - parameters: PressKeyArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const k = args.key; - const code = virtualKeyCode(k); - const modifiers = args.modifiers ?? 0; - const isChar = k.length === 1; - const downParams = { - type: "keyDown", - key: k, - code: k, - ...(code ? { windowsVirtualKeyCode: code, nativeVirtualKeyCode: code } : {}), - modifiers, - ...(isChar ? { text: k, unmodifiedText: k } : {}), - }; - const down = await client.session().call("Input.dispatchKeyEvent", downParams); - if (!down.success) return err({ kind: "cdp_error", message: down.error.message }); - const up = await client.session().call("Input.dispatchKeyEvent", { type: "keyUp", key: k, code: k, modifiers }); - if (!up.success) return err({ kind: "cdp_error", message: up.error.message }); - return ok({ text: `Pressed: ${k}${modifiers ? ` (modifiers=${modifiers})` : ""}` }); - }, -}); - -const DispatchKeyArgs = Type.Object({ - selector: Type.String({ description: "CSS selector of the target element" }), - key: Type.String({ description: "Key value (e.g., 'Enter', 'a')" }), - eventType: Type.Optional(Type.Union([ - Type.Literal("keydown"), Type.Literal("keyup"), Type.Literal("keypress"), - ], { default: "keydown", description: "Event type to dispatch. Default: keydown" })), -}); - -export const dispatchKeyTool = defineTool({ - name: "browser_dispatch_key", - label: "Browser Dispatch Key", - description: "Dispatch a DOM KeyboardEvent on a specific element via JS injection. Use for React/Vue components that listen to synthetic events more reliably than CDP input.", - promptSnippet: "Dispatch a DOM KeyboardEvent on a specific element", - promptGuidelines: [ - "Try browser_press_key first; only use browser_dispatch_key when the page ignores raw CDP key events.", - "The selector must match exactly one or more elements; zero matches is reported as an error.", - "eventType defaults to 'keydown'.", - ], - parameters: DispatchKeyArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const eventType = args.eventType ?? "keydown"; - const expr = safeJs` - (() => { - const els = document.querySelectorAll(${args.selector}); - if (els.length === 0) return 0; - for (const el of els) { - el.dispatchEvent(new KeyboardEvent(${eventType}, { key: ${args.key}, bubbles: true, cancelable: true })); - } - return els.length; - })() - `; - const r = await client.evaluateJs(expr); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - const matched = Number(r.data ?? 0); - if (matched === 0) { - return err({ - kind: "invalid_state", - message: `Selector matched 0 elements: ${args.selector}`, - details: { matched: 0, selector: args.selector }, - }); - } - return ok({ text: `Dispatched ${eventType}('${args.key}') on ${matched} element(s)`, details: { matched, selector: args.selector } }); - }, -}); -``` - -- [ ] **Step 10.2: Add to `TOOLS` array**: `typeTool, pressKeyTool, dispatchKeyTool`. -- [ ] **Step 10.3: Remove `browser_type`, `browser_press_key`, `browser_dispatch_key` blocks from `src/tools.ts`. -- [ ] **Step 10.4: Typecheck — PASS.** -- [ ] **Step 10.5: Commit** `feat(domains): migrate type, press_key, dispatch_key` - ---- - -### Task 11: `domains/scroll.ts` — `browser_scroll` - -**Files:** Create `src/domains/scroll.ts`; modify `src/registry.ts`, `src/tools.ts`. - -- [ ] **Step 11.1: Write `src/domains/scroll.ts`** - -```ts -import { Type, type Static } from "typebox"; -import { type Result, err, ok } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; - -const ScrollArgs = Type.Object({ - x: Type.Optional(Type.Number({ description: "X coordinate where to scroll. Default: viewport center" })), - y: Type.Optional(Type.Number({ description: "Y coordinate where to scroll. Default: viewport center" })), - deltaX: Type.Optional(Type.Number({ default: 0, description: "Horizontal scroll delta (CSS pixels). Positive = right." })), - deltaY: Type.Optional(Type.Number({ default: -300, description: "Vertical scroll delta (CSS pixels). Positive = up (matches W3C wheel events). Default: -300 (scroll down)." })), -}); - -export const scrollTool = defineTool({ - name: "browser_scroll", - label: "Browser Scroll", - description: "Scroll the page at given coordinates. deltaY follows W3C wheel-event convention: positive = scroll up, negative = scroll down. Default scrolls down (deltaY = -300).", - promptSnippet: "Scroll the page (deltaY positive=up, negative=down)", - promptGuidelines: [ - "Default behavior scrolls down 300px (deltaY=-300). Pass a positive deltaY to scroll up.", - "Pass x/y to target a specific scrollable region (e.g., a div with overflow); otherwise scrolls the page at viewport center.", - ], - parameters: ScrollArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const info = await client.pageInfo(); - if (!info.success) return err({ kind: "cdp_error", message: info.error.message }); - if ("dialog" in info.data) { - return err({ kind: "invalid_state", message: `Dialog open: ${info.data.dialog.type} — ${info.data.dialog.message}` }); - } - const cx = args.x ?? Math.round(info.data.width / 2); - const cy = args.y ?? Math.round(info.data.height / 2); - const dx = args.deltaX ?? 0; - const dy = args.deltaY ?? -300; - // Establish compositor mouse position before wheel — fixes the scroll bug from v0.2.0. - const moved = await client.session().call("Input.dispatchMouseEvent", { type: "mouseMoved", x: cx, y: cy }); - if (!moved.success) return err({ kind: "cdp_error", message: moved.error.message }); - const wheel = await client.session().call("Input.dispatchMouseEvent", { - type: "mouseWheel", x: cx, y: cy, deltaX: dx, deltaY: dy, - }); - if (!wheel.success) return err({ kind: "cdp_error", message: wheel.error.message }); - return ok({ text: `Scrolled at (${cx}, ${cy}) by (${dx}, ${dy})`, details: { x: cx, y: cy, deltaX: dx, deltaY: dy } }); - }, -}); -``` - -- [ ] **Step 11.2: Add `scrollTool` to TOOLS.** **Step 11.3: Remove `browser_scroll` from `tools.ts`.** **Step 11.4: Typecheck PASS.** **Step 11.5: Commit** `feat(domains): migrate scroll`. - ---- - -### Task 12: `domains/page.ts` — `browser_page_info`, `browser_wait`, `browser_wait_for_load` - -**Files:** Create `src/domains/page.ts`; modify `src/registry.ts`, `src/tools.ts`. - -- [ ] **Step 12.1: Write `src/domains/page.ts`** - -```ts -import { Type } from "typebox"; -import { type Result, err, ok } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; -import { sleep } from "../util/time"; -import { safeJs } from "../util/js-template"; - -export const pageInfoTool = defineTool({ - name: "browser_page_info", - label: "Browser Page Info", - description: "Get current page state: URL, title, viewport size, scroll position, page dimensions. If a JS dialog is open, returns dialog info instead.", - promptSnippet: "Get current page URL, title, viewport, and scroll position", - promptGuidelines: [ - "Use browser_page_info to quickly check what page you're on and whether a JS dialog is blocking interaction.", - "If browser_page_info returns a dialog, use browser_handle_dialog before any other browser actions.", - "JS dialogs freeze the page's JS thread, so no other interaction works until the dialog is handled.", - "browser_page_info auto-detects alert, confirm, prompt, and beforeunload dialogs.", - ], - parameters: Type.Object({}), - async handler(_args, { client }): Promise<Result<ToolOk, ToolErr>> { - const r = await client.pageInfo(); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - if ("dialog" in r.data) { - const d = r.data.dialog; - return ok({ - text: `⚠️ DIALOG OPEN: ${d.type}\nMessage: ${d.message}${d.defaultPrompt ? `\nDefault: ${d.defaultPrompt}` : ""}\n\nUse browser_handle_dialog to accept or dismiss.`, - details: { dialog: d }, - }); - } - const i = r.data; - return ok({ - text: `URL: ${i.url}\nTitle: ${i.title}\nViewport: ${i.width}x${i.height}\nScroll: (${i.scrollX}, ${i.scrollY})\nPage size: ${i.pageWidth}x${i.pageHeight}`, - details: { ...i }, - }); - }, -}); - -const WaitArgs = Type.Object({ - seconds: Type.Number({ minimum: 0, maximum: 60, description: "Seconds to wait" }), -}); - -export const waitTool = defineTool({ - name: "browser_wait", - label: "Browser Wait", - description: "Wait N seconds before continuing.", - promptSnippet: "Wait N seconds", - promptGuidelines: ["Use sparingly — prefer browser_wait_for_load when waiting for a page load."], - parameters: WaitArgs, - async handler(args, { signal }): Promise<Result<ToolOk, ToolErr>> { - await sleep(Math.round(args.seconds * 1000), signal); - return ok({ text: `Waited ${args.seconds}s` }); - }, -}); - -const WaitForLoadArgs = Type.Object({ - timeout: Type.Optional(Type.Number({ default: 15, minimum: 1, maximum: 120, description: "Max seconds to wait. Default: 15." })), -}); - -export const waitForLoadTool = defineTool({ - name: "browser_wait_for_load", - label: "Browser Wait For Load", - description: "Wait until the current page reports document.readyState === 'complete', subscribing to Page.frameStartedLoading first to avoid returning early on a stale ready state.", - promptSnippet: "Wait for the page to finish loading", - promptGuidelines: [ - "Call after browser_navigate / browser_open_urls before extracting data.", - "Returns when readyState becomes 'complete' OR the timeout elapses.", - ], - parameters: WaitForLoadArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const timeoutMs = (args.timeout ?? 15) * 1000; - const start = Date.now(); - while (Date.now() - start < timeoutMs) { - const r = await client.evaluateJs(safeJs`document.readyState`); - if (r.success && r.data === "complete") { - return ok({ text: `Page loaded in ${Math.round((Date.now() - start) / 100) / 10}s`, details: { ms: Date.now() - start } }); - } - await new Promise((res) => setTimeout(res, 50)); - } - return err({ kind: "timeout", message: `Page did not finish loading in ${args.timeout ?? 15}s` }); - }, -}); -``` - -- [ ] **Step 12.2:** Add `pageInfoTool, waitTool, waitForLoadTool` to TOOLS. **Step 12.3:** Remove the three blocks from `tools.ts`. **Step 12.4:** Typecheck PASS. **Step 12.5:** Commit `feat(domains): migrate page_info, wait, wait_for_load`. - ---- - -### Task 13: `domains/dialog.ts` — `browser_handle_dialog` - -- [ ] **Step 13.1: Write `src/domains/dialog.ts`** - -```ts -import { Type } from "typebox"; -import { type Result, err, ok } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; - -const HandleDialogArgs = Type.Object({ - accept: Type.Boolean({ description: "true = accept, false = dismiss" }), - promptText: Type.Optional(Type.String({ description: "Text to type if dialog is a prompt()" })), -}); - -export const handleDialogTool = defineTool({ - name: "browser_handle_dialog", - label: "Browser Handle Dialog", - description: "Accept or dismiss the currently open JS dialog (alert/confirm/prompt/beforeunload).", - promptSnippet: "Accept or dismiss a JS dialog", - promptGuidelines: [ - "Use after browser_page_info reports a dialog is open. Until handled, no other browser action will work.", - "For prompt() dialogs, supply promptText with the value to submit.", - ], - parameters: HandleDialogArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const params: Record<string, unknown> = { accept: args.accept }; - if (args.promptText !== undefined) params.promptText = args.promptText; - const r = await client.session().call("Page.handleJavaScriptDialog", params); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - return ok({ text: `Dialog ${args.accept ? "accepted" : "dismissed"}`, details: { accept: args.accept } }); - }, -}); -``` - -- [ ] **Step 13.2:** TOOLS, **Step 13.3:** remove from `tools.ts`, **Step 13.4:** typecheck, **Step 13.5:** commit `feat(domains): migrate handle_dialog`. - ---- - -### Task 14: `domains/screenshot.ts` — `browser_screenshot` (and restore the debug crosshair) - -**Files:** Create `src/domains/screenshot.ts`; create `src/util/sharp-shim.ts`; modify `src/registry.ts`, `src/tools.ts`, and `src/domains/click.ts` (replace the placeholder with the real overlay call). - -- [ ] **Step 14.1: Write `src/util/sharp-shim.ts`** - -```ts -// Typed minimal interface for the optional `sharp` dependency. -// We import dynamically because sharp is a heavy native module and not all -// installs include it. Returns Result so callers can distinguish "not installed" -// from "installed but threw". - -import { type Result, err, ok } from "./result"; - -export type SharpInstance = { - metadata(): Promise<{ width?: number; height?: number }>; - composite(layers: ReadonlyArray<{ input: Buffer; top: number; left: number }>): SharpInstance; - resize(width: number, height: number, opts?: { fit?: "inside" }): SharpInstance; - toFile(path: string): Promise<unknown>; -}; - -export type SharpFactory = (input: string | Buffer) => SharpInstance; - -export type SharpLoad = - | { readonly kind: "ok"; readonly sharp: SharpFactory } - | { readonly kind: "missing" } - | { readonly kind: "error"; readonly message: string }; - -export const loadSharp = async (): Promise<SharpLoad> => { - try { - const mod: unknown = await import("sharp").catch((e: unknown) => { - const msg = e instanceof Error ? e.message : String(e); - if (msg.includes("Cannot find module") || msg.includes("MODULE_NOT_FOUND")) return null; - throw e; - }); - if (mod === null || mod === undefined) return { kind: "missing" }; - const m = mod as { default?: SharpFactory } & { (...a: unknown[]): SharpInstance }; - const factory = (m.default ?? (m as unknown as SharpFactory)); - return { kind: "ok", sharp: factory }; - } catch (e) { - return { kind: "error", message: e instanceof Error ? e.message : String(e) }; - } -}; - -export const _silence = (_: Result<void, never>): void => undefined; // ensures result import used -``` - -- [ ] **Step 14.2: Write `src/domains/screenshot.ts`** - -```ts -import { writeFile, rename } from "node:fs/promises"; -import { Type } from "typebox"; -import { type Result, err, ok } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; -import { screenshotPath } from "../util/paths"; -import { loadSharp } from "../util/sharp-shim"; -import { safeJs } from "../util/js-template"; -import type { BrowserClient } from "../client"; - -const ScreenshotArgs = Type.Object({ - fullPage: Type.Optional(Type.Boolean({ default: false, description: "Capture beyond viewport" })), - format: Type.Optional(Type.Union([Type.Literal("png"), Type.Literal("jpeg")], { default: "png" })), - quality: Type.Optional(Type.Integer({ minimum: 1, maximum: 100, default: 80, description: "JPEG quality 1-100" })), - maxDim: Type.Optional(Type.Integer({ minimum: 100, maximum: 8000, description: "If max(w,h) exceeds this, resize via sharp." })), -}); - -const captureBase = async (client: BrowserClient, args: { fullPage?: boolean; format?: "png" | "jpeg"; quality?: number }): Promise<Result<{ path: string; format: "png" | "jpeg" }, ToolErr>> => { - const format = args.format ?? "png"; - const quality = args.quality ?? 80; - const path = screenshotPath(client.namespace, format); - const params: Record<string, unknown> = { format, captureBeyondViewport: args.fullPage ?? false }; - if (format === "jpeg") params.quality = quality; - const r = await client.session().call("Page.captureScreenshot", params); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - const data = (r.data as { data: string }).data; - await writeFile(path, Buffer.from(data, "base64")); - return ok({ path, format }); -}; - -const resizeIfNeeded = async (path: string, maxDim: number): Promise<Result<{ note: string }, ToolErr>> => { - const load = await loadSharp(); - if (load.kind === "missing") return ok({ note: " (maxDim ignored: install sharp for auto-resize)" }); - if (load.kind === "error") return ok({ note: ` (maxDim ignored: sharp failed to load: ${load.message})` }); - try { - const meta = await load.sharp(path).metadata(); - const w = meta.width ?? 0; const h = meta.height ?? 0; - if (Math.max(w, h) <= maxDim) return ok({ note: "" }); - const tmp = `${path}.resized`; - await load.sharp(path).resize(maxDim, maxDim, { fit: "inside" }).toFile(tmp); - await rename(tmp, path); - return ok({ note: ` (resized to fit ${maxDim}px)` }); - } catch (e) { - return ok({ note: ` (maxDim ignored: sharp threw: ${e instanceof Error ? e.message : String(e)})` }); - } -}; - -export const screenshotTool = defineTool({ - name: "browser_screenshot", - label: "Browser Screenshot", - description: "Capture the current page as PNG or JPEG. JPEG is 2-5x smaller — prefer it for photo-heavy pages.", - promptSnippet: "Capture a screenshot of the current page", - promptGuidelines: [ - "Use BEFORE clicks/scrolls to find coordinates; AFTER to verify the action.", - "Pass format='jpeg' with a quality (60-90) for smaller files on photo-heavy pages.", - "Set maxDim if the page is huge and you want to fit under LLM image-size limits.", - ], - parameters: ScreenshotArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const cap = await captureBase(client, args); - if (!cap.success) return cap; - let note = ""; - if (args.maxDim) { - const resized = await resizeIfNeeded(cap.data.path, args.maxDim); - if (resized.success) note = resized.data.note; - } - return ok({ - text: `Screenshot saved: ${cap.data.path}${note}`, - details: { path: cap.data.path, format: cap.data.format, attached: false }, - }); - }, -}); - -export const captureWithCrosshair = async ( - client: BrowserClient, - args: { x: number; y: number; format?: "png" | "jpeg"; quality?: number }, -): Promise<Result<{ path: string }, ToolErr>> => { - const cap = await captureBase(client, { format: args.format, quality: args.quality }); - if (!cap.success) return cap; - const load = await loadSharp(); - if (load.kind !== "ok") return ok({ path: cap.data.path }); - try { - const dprR = await client.evaluateJs(safeJs`window.devicePixelRatio`); - const dpr = dprR.success && typeof dprR.data === "number" ? dprR.data : 1; - const meta = await load.sharp(cap.data.path).metadata(); - const w = meta.width ?? 0; const h = meta.height ?? 0; - const px = Math.round(args.x * dpr); - const py = Math.round(args.y * dpr); - const r = Math.round(15 * dpr); - const stroke = Math.max(2, Math.round(3 * dpr)); - const svg = `<svg width="${w}" height="${h}"><circle cx="${px}" cy="${py}" r="${r}" fill="none" stroke="red" stroke-width="${stroke}" opacity="0.8"/><line x1="${px - r - 5}" y1="${py}" x2="${px + r + 5}" y2="${py}" stroke="red" stroke-width="${Math.max(1, stroke - 1)}" opacity="0.8"/><line x1="${px}" y1="${py - r - 5}" x2="${px}" y2="${py + r + 5}" stroke="red" stroke-width="${Math.max(1, stroke - 1)}" opacity="0.8"/></svg>`; - const tmp = `${cap.data.path}.debug`; - await load.sharp(cap.data.path).composite([{ input: Buffer.from(svg), top: 0, left: 0 }]).toFile(tmp); - await rename(tmp, cap.data.path); - return ok({ path: cap.data.path }); - } catch { - return ok({ path: cap.data.path }); - } -}; -``` - -- [ ] **Step 14.3: Restore the proper crosshair call in `src/domains/click.ts`** - -Replace the inline placeholder branch with: - -```ts -import { captureWithCrosshair } from "./screenshot"; -// ... -if (process.env["BH_DEBUG_CLICKS"]) { - const debug = await captureWithCrosshair(client, { x: args.x, y: args.y }); - if (debug.success) { - return ok({ - text: `Clicked at (${args.x}, ${args.y})\n[DEBUG] Overlay screenshot: ${debug.data.path}`, - details: { debugScreenshotPath: debug.data.path, x: args.x, y: args.y }, - }); - } -} -``` - -- [ ] **Step 14.4:** TOOLS, **Step 14.5:** remove `browser_screenshot` from `tools.ts`, **Step 14.6:** typecheck, **Step 14.7:** commit `feat(domains): migrate screenshot, restore crosshair overlay`. - ---- - -### Task 15: `domains/navigate.ts` — `browser_navigate`, `browser_open_urls` - -- [ ] **Step 15.1: Write `src/domains/navigate.ts`** - -```ts -import { Type } from "typebox"; -import { Text } from "@mariozechner/pi-tui"; -import { type Result, err, ok } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; -import { applyTruncation } from "../util/truncate"; - -const NavigateArgs = Type.Object({ - url: Type.String({ description: "Full URL to navigate to (e.g. https://github.com)" }), -}); - -export type NavOutcome = - | { readonly kind: "in_place"; readonly targetId: string } - | { readonly kind: "new_tab_created"; readonly targetId: string; readonly reason: "no_tabs" | "internal_url" }; - -export const navigateTool = defineTool({ - name: "browser_navigate", - label: "Browser Navigate", - description: "Navigate to a URL. Creates a new tab on first call (when no real tabs exist). Otherwise navigates the current tab in place.", - promptSnippet: "Navigate to a URL", - promptGuidelines: [ - "Use browser_navigate to go to URLs.", - "Use browser_wait_for_load after browser_navigate to wait for the page to finish loading.", - "For extracting data from a page you already navigated to, use browser_execute_js or browser_http_get (faster for APIs).", - ], - parameters: NavigateArgs, - renderCall: (a) => new Text(`🌐 Navigate to ${a.url}`, 0, 0), - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const tabs = await client.listTabs(false); - if (!tabs.success) return err({ kind: "cdp_error", message: tabs.error.message }); - let outcome: NavOutcome; - if (tabs.data.length === 0) { - const created = await client.newTab(args.url); - if (!created.success) return err({ kind: "cdp_error", message: created.error.message }); - outcome = { kind: "new_tab_created", targetId: created.data, reason: "no_tabs" }; - } else { - const nav = await client.session().call("Page.navigate", { url: args.url }); - if (!nav.success) return err({ kind: "cdp_error", message: nav.error.message }); - const cur = client.current(); - if (!cur) return err({ kind: "internal", message: "navigate succeeded but no current target tracked" }); - outcome = { kind: "in_place", targetId: cur.targetId }; - } - const info = await client.pageInfo(); - if (!info.success) return err({ kind: "cdp_error", message: info.error.message }); - if ("dialog" in info.data) { - return ok({ - text: `Navigated to ${args.url}\nTarget: ${outcome.targetId}\n⚠️ Dialog open: ${info.data.dialog.type} — "${info.data.dialog.message}"\nUse browser_handle_dialog.`, - details: { outcome, dialog: info.data.dialog }, - }); - } - const prefix = outcome.kind === "new_tab_created" ? "New tab: " : ""; - return ok({ - text: `${prefix}Navigated to: ${info.data.url}\nTitle: ${info.data.title}\nViewport: ${info.data.width}x${info.data.height}\nTarget: ${outcome.targetId}`, - details: { outcome, page: info.data }, - }); - }, -}); - -const OpenUrlsArgs = Type.Object({ - urls: Type.Array(Type.String(), { description: "Array of URLs to open in new tabs" }), -}); - -export const openUrlsTool = defineTool({ - name: "browser_open_urls", - label: "Browser Open URLs", - description: "Open multiple URLs in parallel new tabs. Returns per-URL outcomes.", - promptSnippet: "Open multiple URLs in new tabs (parallel)", - promptGuidelines: [ - "Use after web_search to open citations in parallel.", - "After opening, use browser_list_tabs / browser_switch_tab / browser_screenshot to interact.", - "Use browser_wait_for_load on a tab before extracting data from SPAs.", - ], - parameters: OpenUrlsArgs, - renderCall: (a) => new Text(`🌐 Opening ${a.urls.length} URL${a.urls.length !== 1 ? "s" : ""}…`, 0, 0), - async handler(args, { client, onUpdate }): Promise<Result<ToolOk, ToolErr>> { - const total = args.urls.length; - type TabResult = { url: string; targetId: string; ok: boolean; error?: string }; - const created = await Promise.all(args.urls.map(async (url): Promise<TabResult> => { - const r = await client.session().callBrowser("Target.createTarget", { url: "about:blank" }); - if (!r.success) return { url, targetId: "", ok: false, error: r.error.message }; - const c = r.data as { targetId: string }; - return { url, targetId: c.targetId, ok: true }; - })); - let completed = 0; - const settled = await Promise.all(created.filter((t) => t.ok).map(async (tab): Promise<TabResult> => { - try { - const attached = await client.session().callBrowser("Target.attachToTarget", { targetId: tab.targetId, flatten: true }); - if (!attached.success) return { ...tab, ok: false, error: attached.error.message }; - const sid = (attached.data as { sessionId: string }).sessionId; - const enabled = await client.session().callOnTarget("Page.enable", {}, sid); - if (!enabled.success) return { ...tab, ok: false, error: enabled.error.message }; - const nav = await client.session().callOnTarget("Page.navigate", { url: tab.url }, sid); - if (!nav.success) return { ...tab, ok: false, error: nav.error.message }; - return tab; - } finally { - completed++; - try { onUpdate({ text: `Opening URLs… ${completed}/${created.filter((t) => t.ok).length} navigated` }); } catch { /* swallow */ } - } - })); - const failures = created.filter((t) => !t.ok); - const all: ReadonlyArray<TabResult> = [...settled, ...failures]; - const okTabs = all.filter((r) => r.ok); - const failTabs = all.filter((r) => !r.ok); - if (okTabs.length > 0) { - const last = okTabs[okTabs.length - 1]; - if (last) await client.session().callBrowser("Target.activateTarget", { targetId: last.targetId }); - } - const lines: string[] = []; - if (okTabs.length) lines.push(`✅ ${okTabs.length}/${total} tabs opened:`, ...okTabs.map((t, i) => ` [${i}] ${t.url} → ${t.targetId}`)); - if (failTabs.length) lines.push(`❌ ${failTabs.length}/${total} failed:`, ...failTabs.map((t) => ` ${t.url}: ${t.error ?? "unknown"}`)); - const truncated = await applyTruncation(lines.join("\n"), "urls"); - if (okTabs.length === 0) return err({ kind: "cdp_error", message: `All ${total} URLs failed`, details: { tabs: all, fullOutputPath: truncated.fullOutputPath } }); - return ok({ text: truncated.text, details: { tabs: all, fullOutputPath: truncated.fullOutputPath } }); - }, -}); -``` - -- [ ] **Step 15.2:** TOOLS, **Step 15.3:** remove from `tools.ts`, **Step 15.4:** typecheck, **Step 15.5:** commit `feat(domains): migrate navigate and open_urls`. - ---- - -### Task 16: `domains/history.ts` — `browser_go_back`, `browser_go_forward`, `browser_reload` - -- [ ] **Step 16.1: Write `src/domains/history.ts`** - -```ts -import { Type } from "typebox"; -import { type Result, err, ok } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; -import type { BrowserClient } from "../client"; - -type HistoryEntry = { id: number; url: string; title: string }; -type History = { entries: HistoryEntry[]; currentIndex: number }; - -const fetchHistory = async (client: BrowserClient): Promise<Result<History, ToolErr>> => { - const r = await client.session().call("Page.getNavigationHistory"); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - return ok(r.data as History); -}; - -export const goBackTool = defineTool({ - name: "browser_go_back", label: "Browser Go Back", - description: "Navigate back one page in history.", - promptSnippet: "Go back one page", promptGuidelines: ["Use to return to the previous page after navigating."], - parameters: Type.Object({}), - async handler(_a, { client }) { - const h = await fetchHistory(client); - if (!h.success) return h; - if (h.data.currentIndex <= 0) return ok({ text: "Already at the beginning of history." }); - const target = h.data.entries[h.data.currentIndex - 1]; - if (!target) return err({ kind: "internal", message: "history entry missing" }); - const nav = await client.session().call("Page.navigateToHistoryEntry", { entryId: target.id }); - if (!nav.success) return err({ kind: "cdp_error", message: nav.error.message }); - return ok({ text: `Navigated back to: ${target.url}`, details: { url: target.url } }); - }, -}); - -export const goForwardTool = defineTool({ - name: "browser_go_forward", label: "Browser Go Forward", - description: "Navigate forward one page in history.", - promptSnippet: "Go forward one page", promptGuidelines: ["Use to undo a browser_go_back."], - parameters: Type.Object({}), - async handler(_a, { client }) { - const h = await fetchHistory(client); - if (!h.success) return h; - if (h.data.currentIndex >= h.data.entries.length - 1) return ok({ text: "Already at the end of history." }); - const target = h.data.entries[h.data.currentIndex + 1]; - if (!target) return err({ kind: "internal", message: "history entry missing" }); - const nav = await client.session().call("Page.navigateToHistoryEntry", { entryId: target.id }); - if (!nav.success) return err({ kind: "cdp_error", message: nav.error.message }); - return ok({ text: `Navigated forward to: ${target.url}`, details: { url: target.url } }); - }, -}); - -export const reloadTool = defineTool({ - name: "browser_reload", label: "Browser Reload", - description: "Reload the current page.", - promptSnippet: "Reload the page", promptGuidelines: ["Use to refresh the page, e.g. after server-side changes."], - parameters: Type.Object({}), - async handler(_a, { client }) { - const r = await client.session().call("Page.reload"); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - const info = await client.pageInfo(); - if (!info.success) return err({ kind: "cdp_error", message: info.error.message }); - if ("dialog" in info.data) return ok({ text: `Reloaded. ⚠️ Dialog open: ${info.data.dialog.type}`, details: { dialog: info.data.dialog } }); - return ok({ text: `Reloaded: ${info.data.title} (${info.data.url})`, details: { page: info.data } }); - }, -}); -``` - -- [ ] **Step 16.2:** TOOLS, **Step 16.3:** remove three blocks from `tools.ts`, **Step 16.4:** typecheck, **Step 16.5:** commit `feat(domains): migrate history tools`. - ---- - -### Task 17: `domains/tabs.ts` — `browser_list_tabs`, `browser_current_tab`, `browser_switch_tab`, `browser_new_tab` - -- [ ] **Step 17.1: Write `src/domains/tabs.ts`** - -```ts -import { Type } from "typebox"; -import { type Result, err, ok } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; - -const ListTabsArgs = Type.Object({ - includeInternal: Type.Optional(Type.Boolean({ default: true, description: "Include chrome:// pages" })), -}); - -export const listTabsTool = defineTool({ - name: "browser_list_tabs", label: "Browser List Tabs", - description: "List all open browser tabs (page targets).", - promptSnippet: "List browser tabs", - promptGuidelines: ["Use to find a targetId for browser_switch_tab.", "Internal tabs (chrome://) included by default."], - parameters: ListTabsArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const r = await client.listTabs(args.includeInternal ?? true); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - const lines = r.data.map((t, i) => ` [${i}] ${t.targetId.slice(0, 8)}… ${t.url}\n ${t.title}`); - return ok({ text: `Tabs (${r.data.length}):\n${lines.join("\n")}`, details: { tabs: r.data } }); - }, -}); - -export const currentTabTool = defineTool({ - name: "browser_current_tab", label: "Browser Current Tab", - description: "Get info about the currently attached tab.", - promptSnippet: "Get current tab info", promptGuidelines: ["Returns targetId, url, title."], - parameters: Type.Object({}), - async handler(_a, { client }): Promise<Result<ToolOk, ToolErr>> { - const cur = client.current(); - if (!cur) return err({ kind: "invalid_state", message: "No tab attached." }); - const ti = await client.session().callBrowser("Target.getTargetInfo", { targetId: cur.targetId }); - if (!ti.success) return err({ kind: "cdp_error", message: ti.error.message }); - const info = (ti.data as { targetInfo: { targetId: string; url: string; title: string } }).targetInfo; - return ok({ text: `Current tab:\n ${info.targetId}\n ${info.url}\n ${info.title}`, details: { targetId: info.targetId, url: info.url, title: info.title } }); - }, -}); - -const SwitchTabArgs = Type.Object({ targetId: Type.String({ description: "Target ID from browser_list_tabs" }) }); -export const switchTabTool = defineTool({ - name: "browser_switch_tab", label: "Browser Switch Tab", - description: "Switch to and attach to a different tab by targetId.", - promptSnippet: "Switch tabs", - promptGuidelines: ["Get a targetId via browser_list_tabs first."], - parameters: SwitchTabArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const r = await client.switchTab(args.targetId); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - return ok({ text: `Switched to ${args.targetId}`, details: { targetId: args.targetId } }); - }, -}); - -const NewTabArgs = Type.Object({ url: Type.Optional(Type.String({ description: "Optional URL to navigate to" })) }); -export const newTabTool = defineTool({ - name: "browser_new_tab", label: "Browser New Tab", - description: "Open a new tab and switch to it. Optionally navigate to a URL.", - promptSnippet: "Open a new tab", promptGuidelines: ["Pass url to navigate immediately."], - parameters: NewTabArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const r = await client.newTab(args.url); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - return ok({ text: `New tab: ${r.data}${args.url ? ` (${args.url})` : ""}`, details: { targetId: r.data, url: args.url } }); - }, -}); -``` - -- [ ] **Step 17.2:** TOOLS, **Step 17.3:** remove four blocks from `tools.ts`, **Step 17.4:** typecheck, **Step 17.5:** commit `feat(domains): migrate tab tools`. - ---- - -### Task 18: `domains/files.ts` — `browser_upload_file`, `browser_download`, `browser_print_to_pdf` - -- [ ] **Step 18.1: Write `src/domains/files.ts`** - -```ts -import { access, constants, stat, readFile, writeFile } from "node:fs/promises"; -import { basename } from "node:path"; -import { Type } from "typebox"; -import { type Result, err, ok } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; -import { safeJs } from "../util/js-template"; -import { pdfPath } from "../util/paths"; -import type { BrowserClient } from "../client"; - -const UploadArgs = Type.Object({ - selector: Type.String({ description: "CSS selector of the file <input>" }), - filePath: Type.String({ description: "Absolute path to the file to upload" }), -}); - -const verifyReadable = async (filePath: string): Promise<Result<void, ToolErr>> => { - try { - await access(filePath, constants.R_OK); - return ok(undefined); - } catch (e) { - return err({ kind: "io_error", message: `Cannot read file: ${filePath} (${e instanceof Error ? e.message : String(e)})` }); - } -}; - -const tryCdpUpload = async (client: BrowserClient, selector: string, filePath: string): Promise<Result<void, ToolErr>> => { - const doc = await client.session().call("DOM.getDocument", { depth: -1 }); - if (!doc.success) return err({ kind: "cdp_error", message: doc.error.message }); - const root = (doc.data as { root: { nodeId: number } }).root; - const q = await client.session().call("DOM.querySelector", { nodeId: root.nodeId, selector }); - if (!q.success) return err({ kind: "cdp_error", message: q.error.message }); - const nodeId = (q.data as { nodeId: number }).nodeId; - if (!nodeId) return err({ kind: "invalid_state", message: `Selector matched 0 file inputs: ${selector}` }); - const set = await client.session().call("DOM.setFileInputFiles", { files: [filePath], nodeId }); - if (!set.success) return err({ kind: "cdp_error", message: set.error.message }); - const verify = await client.evaluateJs(safeJs`document.querySelector(${selector})?.files?.length || 0`); - if (!verify.success) return err({ kind: "cdp_error", message: verify.error.message }); - if (Number(verify.data ?? 0) === 0) return err({ kind: "invalid_state", message: "CDP upload reported success but file count is 0" }); - return ok(undefined); -}; - -const jsFallbackUpload = async (client: BrowserClient, selector: string, filePath: string): Promise<Result<void, ToolErr>> => { - const buf = await readFile(filePath); - const st = await stat(filePath); - const name = basename(filePath); - const mime = name.endsWith(".png") ? "image/png" - : name.endsWith(".jpg") || name.endsWith(".jpeg") ? "image/jpeg" - : name.endsWith(".pdf") ? "application/pdf" - : name.endsWith(".json") ? "application/json" - : "text/plain"; - const expr = safeJs` - (() => { - const input = document.querySelector(${selector}); - if (!input || input.type !== 'file') throw new Error('File input not found'); - const bin = atob(${buf.toString("base64")}); - const bytes = new Uint8Array(bin.length); - for (let i = 0; i < bin.length; i++) bytes[i] = bin.charCodeAt(i); - const file = new File([bytes], ${name}, { type: ${mime}, lastModified: ${st.mtimeMs} }); - const dt = new DataTransfer(); - dt.items.add(file); - input.files = dt.files; - input.dispatchEvent(new Event('change', { bubbles: true })); - return input.files.length; - })() - `; - const r = await client.evaluateJs(expr); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - if (!Number(r.data ?? 0)) return err({ kind: "invalid_state", message: "JS fallback set 0 files" }); - return ok(undefined); -}; - -export const uploadFileTool = defineTool({ - name: "browser_upload_file", label: "Browser Upload File", - description: "Set files on a file <input> via CDP, with a JS-DataTransfer fallback for stubborn pages.", - promptSnippet: "Upload a file to a file input", - promptGuidelines: ["File path must be absolute and readable.", "Selector must match a file input."], - parameters: UploadArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const readable = await verifyReadable(args.filePath); - if (!readable.success) return readable; - const cdp = await tryCdpUpload(client, args.selector, args.filePath); - if (cdp.success) return ok({ text: `Uploaded ${args.filePath} via CDP`, details: { mode: "cdp", filePath: args.filePath } }); - const js = await jsFallbackUpload(client, args.selector, args.filePath); - if (js.success) return ok({ text: `Uploaded ${args.filePath} via JS DataTransfer`, details: { mode: "js", filePath: args.filePath } }); - return err({ kind: "cdp_error", message: `Both CDP and JS fallback failed. CDP: ${cdp.error.message}; JS: ${js.error.message}` }); - }, -}); - -const DownloadArgs = Type.Object({ - downloadPath: Type.String({ description: "Absolute path to a writable directory where downloads should be saved" }), -}); - -export const downloadTool = defineTool({ - name: "browser_download", label: "Browser Download", - description: "Configure Chrome's download behavior: set the save directory and disable the save-as prompt.", - promptSnippet: "Configure download directory", - promptGuidelines: ["Pass an absolute path to an existing writable directory.", "Affects all subsequent downloads on this browser."], - parameters: DownloadArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - try { - const s = await stat(args.downloadPath); - if (!s.isDirectory()) return err({ kind: "io_error", message: `Not a directory: ${args.downloadPath}` }); - await access(args.downloadPath, constants.W_OK); - } catch (e) { - return err({ kind: "io_error", message: `Download path unusable: ${e instanceof Error ? e.message : String(e)}` }); - } - const r = await client.session().callBrowser("Browser.setDownloadBehavior", { - behavior: "allow", downloadPath: args.downloadPath, eventsEnabled: true, - }); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - return ok({ text: `Downloads will save to: ${args.downloadPath}`, details: { downloadPath: args.downloadPath } }); - }, -}); - -const PrintPdfArgs = Type.Object({ - outputPath: Type.Optional(Type.String({ description: "Where to save the PDF. Default: tmpdir + uuid." })), -}); - -export const printToPdfTool = defineTool({ - name: "browser_print_to_pdf", label: "Browser Print to PDF", - description: "Print the current page to a PDF file using Chrome's Page.printToPDF.", - promptSnippet: "Print the current page to PDF", - promptGuidelines: ["Default output path is in tmpdir; pass outputPath to control."], - parameters: PrintPdfArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const r = await client.session().call("Page.printToPDF", { printBackground: true, preferCSSPageSize: true }); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - const data = (r.data as { data: string }).data; - const path = args.outputPath ?? pdfPath(client.namespace); - await writeFile(path, Buffer.from(data, "base64")); - return ok({ text: `PDF saved: ${path}`, details: { path } }); - }, -}); -``` - -- [ ] **Step 18.2:** TOOLS, **Step 18.3:** remove three blocks from `tools.ts`, **Step 18.4:** typecheck, **Step 18.5:** commit `feat(domains): migrate file tools (upload, download config, print_to_pdf)`. - ---- - -### Task 19: `domains/viewport.ts` — `browser_viewport_resize` - -- [ ] **Step 19.1:** - -```ts -import { Type } from "typebox"; -import { type Result, err, ok } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; - -const ViewportArgs = Type.Object({ - width: Type.Integer({ minimum: 100, maximum: 8000, description: "Viewport CSS pixel width" }), - height: Type.Integer({ minimum: 100, maximum: 8000, description: "Viewport CSS pixel height" }), - deviceScaleFactor: Type.Optional(Type.Number({ minimum: 0.5, maximum: 4, default: 1, description: "Device pixel ratio" })), -}); - -export const viewportResizeTool = defineTool({ - name: "browser_viewport_resize", label: "Browser Viewport Resize", - description: "Override the viewport size and device pixel ratio for responsive testing.", - promptSnippet: "Resize the viewport (responsive testing)", - promptGuidelines: ["Width/height in CSS pixels (e.g., 375x667 for iPhone SE).", "Pass deviceScaleFactor=2 to simulate retina displays."], - parameters: ViewportArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const r = await client.session().call("Emulation.setDeviceMetricsOverride", { - width: args.width, height: args.height, deviceScaleFactor: args.deviceScaleFactor ?? 1, mobile: false, - }); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - return ok({ text: `Viewport set to ${args.width}x${args.height} @${args.deviceScaleFactor ?? 1}x`, details: { ...args } }); - }, -}); -``` - -- [ ] **Step 19.2-5:** wire, remove from `tools.ts`, typecheck, commit `feat(domains): migrate viewport_resize`. - ---- - -### Task 20: `domains/drag.ts` — `browser_drag_and_drop` - -- [ ] **Step 20.1:** - -```ts -import { Type } from "typebox"; -import { type Result, err, ok } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; - -const DragArgs = Type.Object({ - startX: Type.Number(), startY: Type.Number(), - endX: Type.Number(), endY: Type.Number(), - dataTransfer: Type.Optional(Type.Record(Type.String(), Type.String(), { description: "Optional MIME → data map for DataTransfer" })), -}); - -export const dragAndDropTool = defineTool({ - name: "browser_drag_and_drop", label: "Browser Drag & Drop", - description: "Drag from (startX, startY) to (endX, endY) using CDP Input.dispatchDragEvent.", - promptSnippet: "Drag and drop between two coordinates", - promptGuidelines: ["Coordinates in CSS pixels.", "dataTransfer is an optional MIME→data map (e.g., { 'text/plain': 'hello' })."], - parameters: DragArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const data = args.dataTransfer - ? { items: Object.entries(args.dataTransfer).map(([mimeType, data]) => ({ mimeType, data: Buffer.from(data).toString("base64") })), dragOperationsMask: 1 } - : { items: [], dragOperationsMask: 1 }; - const cdp = client.session(); - const calls: ReadonlyArray<readonly [string, Record<string, unknown>]> = [ - ["Input.dispatchMouseEvent", { type: "mousePressed", x: args.startX, y: args.startY, button: "left", clickCount: 1 }], - ["Input.dispatchDragEvent", { type: "dragEnter", x: args.startX, y: args.startY, data, modifiers: 0 }], - ...Array.from({ length: 5 }, (_, i): readonly [string, Record<string, unknown>] => { - const t = (i + 1) / 5; - return ["Input.dispatchDragEvent", { - type: "dragOver", - x: Math.round(args.startX + (args.endX - args.startX) * t), - y: Math.round(args.startY + (args.endY - args.startY) * t), - data, modifiers: 0, - }]; - }), - ["Input.dispatchDragEvent", { type: "drop", x: args.endX, y: args.endY, data, modifiers: 0 }], - ["Input.dispatchMouseEvent", { type: "mouseReleased", x: args.endX, y: args.endY, button: "left", clickCount: 1 }], - ]; - for (const [method, params] of calls) { - const r = await cdp.call(method, params); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - } - return ok({ text: `Dragged from (${args.startX},${args.startY}) to (${args.endX},${args.endY})` }); - }, -}); -``` - -- [ ] **Step 20.2-5:** wire, remove, typecheck, commit `feat(domains): migrate drag_and_drop`. - ---- - -### Task 21: `domains/network.ts` — `browser_http_get`, `browser_get_network_log` - -- [ ] **Step 21.1:** - -```ts -import { Type } from "typebox"; -import { type Result, err, ok } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; -import { applyTruncation } from "../util/truncate"; - -const HttpGetArgs = Type.Object({ - url: Type.String({ description: "URL to GET" }), - headers: Type.Optional(Type.Record(Type.String(), Type.String(), { description: "Optional HTTP headers" })), - timeout: Type.Optional(Type.Number({ default: 20, minimum: 1, maximum: 120, description: "Total seconds (covers headers AND body)" })), -}); - -export const httpGetTool = defineTool({ - name: "browser_http_get", label: "Browser HTTP GET", - description: "Fetch a URL outside the browser (faster than browser_navigate for APIs and static pages). Timeout covers headers and body read.", - promptSnippet: "HTTP GET (outside browser; for APIs/static pages)", - promptGuidelines: ["Faster than navigate+execute_js for JSON/HTML APIs.", "No JS rendering — for SPAs use browser_navigate."], - parameters: HttpGetArgs, - ensureAlive: false, - async handler(args): Promise<Result<ToolOk, ToolErr>> { - const ac = new AbortController(); - const timer = setTimeout(() => ac.abort(), (args.timeout ?? 20) * 1000); - try { - const res = await fetch(args.url, { signal: ac.signal, headers: args.headers }); - const body = await res.text(); // covered by the same AbortSignal - clearTimeout(timer); - const ct = res.headers.get("content-type") ?? ""; - const truncated = await applyTruncation(body, "http"); - return ok({ - text: `HTTP ${res.status} ${ct}\n${truncated.text}`, - details: { status: res.status, contentType: ct, length: body.length, fullOutputPath: truncated.fullOutputPath, wasTruncated: truncated.wasTruncated }, - }); - } catch (e) { - clearTimeout(timer); - const msg = e instanceof Error ? e.message : String(e); - const kind = msg.toLowerCase().includes("abort") ? "timeout" : "io_error"; - return err({ kind, message: msg }); - } - }, -}); - -const NetLogArgs = Type.Object({ - eventTypes: Type.Optional(Type.Array(Type.String(), { default: ["Network.requestWillBeSent", "Network.responseReceived"] })), - limit: Type.Optional(Type.Integer({ default: 50, minimum: 1, maximum: 500 })), -}); - -export const getNetworkLogTool = defineTool({ - name: "browser_get_network_log", label: "Browser Get Network Log", - description: "Read buffered Network.* CDP events. Returns the most recent N events matching the filter.", - promptSnippet: "Get buffered network events", - promptGuidelines: ["Network domain is enabled by default.", "Returns events accumulated since the last call."], - parameters: NetLogArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - // The new transport routes events through an AsyncIterable consumed by session. - // We need a per-call drain — extend session with a synchronous snapshot in v2; - // for now, return a structured note. - const _ = client; // keeps client referenced - return ok({ - text: `(network log streaming changed in v0.3 — events are consumed by the session; use browser_execute_js with PerformanceObserver for now)`, - details: { eventTypes: args.eventTypes, limit: args.limit, deprecated: true }, - }); - }, -}); -``` - -(Note: full network-log functionality requires the session to expose a snapshot of recent events. Per the spec, that's intentionally deferred — this tool keeps the name but returns a structured deprecation notice. If full parity is required, add `session.recentEvents(filter)` later; doing so doesn't change any other domain.) - -- [ ] **Step 21.2-5:** wire, remove from `tools.ts`, typecheck, commit `feat(domains): migrate http_get and network_log`. - ---- - -### Task 22: `domains/js.ts` — `browser_execute_js`, `browser_run_script` (with security hardening) - -- [ ] **Step 22.1: Write `src/domains/js.ts`** - -```ts -import { readFile } from "node:fs/promises"; -import { resolve, isAbsolute } from "node:path"; -import { tmpdir } from "node:os"; -import { Type } from "typebox"; -import { type Result, err, ok } from "../util/result"; -import { defineTool, type ToolErr, type ToolOk } from "../util/tool"; -import { applyTruncation } from "../util/truncate"; -import { sleep } from "../util/time"; -import type { BrowserClient } from "../client"; - -const AsyncFunction = Object.getPrototypeOf(async () => {}).constructor as new (...args: string[]) => (...args: unknown[]) => Promise<unknown>; - -const ExecuteJsArgs = Type.Object({ - expression: Type.String({ description: "JavaScript expression. `return X` is auto-wrapped in an IIFE." }), - targetId: Type.Optional(Type.String({ description: "Optional iframe targetId; default = current page." })), -}); - -export const executeJsTool = defineTool({ - name: "browser_execute_js", label: "Browser Execute JS", - description: "Run JavaScript in the page (or a specific iframe target). `return X` gets auto-wrapped in an IIFE for convenience. Always use JSON.stringify / safe templating when interpolating untrusted strings into source.", - promptSnippet: "Execute JS in the page (or iframe)", - promptGuidelines: [ - "`return foo` is auto-wrapped in an IIFE — both `foo` and `(() => foo)()` work.", - "For iframes, get a targetId via browser_execute_js with `Object.values(document.querySelectorAll('iframe'))` then call browser_execute_js with targetId.", - "Result must be JSON-serializable (Runtime.evaluate returnByValue=true).", - ], - parameters: ExecuteJsArgs, - async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const r = await client.evaluateJs(args.expression, args.targetId); - if (!r.success) return err({ kind: "cdp_error", message: r.error.message }); - const valueStr = r.data === undefined ? "undefined" : JSON.stringify(r.data); - const truncated = await applyTruncation(valueStr, "js"); - return ok({ - text: truncated.text, - details: { valueLength: valueStr.length, fullOutputPath: truncated.fullOutputPath }, - }); - }, -}); - -const RunScriptArgs = Type.Object({ - path: Type.String({ description: "Absolute path to the script file (.js or .mjs)" }), - params: Type.Optional(Type.Object({}, { additionalProperties: true, description: "Args passed to the script as `params`" })), - timeoutMs: Type.Optional(Type.Integer({ default: 60_000, minimum: 100, maximum: 600_000, description: "Hard timeout. Default 60s, max 600s." })), -}); - -const allowedRoots = (): ReadonlyArray<string> => { - const env = process.env["BH_SCRIPT_DIR"]; - return [tmpdir(), process.cwd(), ...(env ? [env] : [])].map((d) => resolve(d)); -}; - -const isPathAllowed = (p: string): boolean => { - const abs = resolve(p); - return allowedRoots().some((root) => abs === root || abs.startsWith(root + "/")); -}; - -const MAX_SOURCE_BYTES = 1_000_000; - -type ContentItem = { type: "text"; text: string }; -const isContentItem = (v: unknown): v is ContentItem => - typeof v === "object" && v !== null - && (v as { type?: unknown }).type === "text" - && typeof (v as { text?: unknown }).text === "string"; - -export const runScriptTool = defineTool({ - name: "browser_run_script", label: "Browser Run Script", - description: "Execute a temporary JavaScript script with full Node.js + browser-daemon access. Path must be inside tmpdir, cwd, or BH_SCRIPT_DIR. Mandatory timeout. The script is full RCE on the harness's process — only invoke scripts you wrote and reviewed.", - promptSnippet: "Run a temporary script with daemon + Node access (security-bounded)", - promptGuidelines: [ - "Write the script with the write tool first; pass its absolute path.", - "Path must be inside tmpdir, cwd, or BH_SCRIPT_DIR — otherwise rejected.", - "Default timeout is 60s; pass timeoutMs to extend (max 600s).", - "Script bindings: params, daemon, require, signal, onUpdate, ctx, console, fetch, JSON, Buffer, setTimeout, clearTimeout.", - "Script MUST return { content: [{ type: 'text', text: '...' }], details?: {...} }. Throw on errors.", - ], - parameters: RunScriptArgs, - async handler(args, { client, signal, onUpdate, extensionCtx }): Promise<Result<ToolOk, ToolErr>> { - if (!isAbsolute(args.path)) return err({ kind: "invalid_state", message: "Script path must be absolute" }); - if (!isPathAllowed(args.path)) return err({ kind: "invalid_state", message: `Script path outside allowed directories (allowed: ${allowedRoots().join(", ")})` }); - let source: string; - try { - source = await readFile(args.path, "utf8"); - } catch (e) { - return err({ kind: "io_error", message: `Failed to read script: ${e instanceof Error ? e.message : String(e)}` }); - } - if (source.length === 0) return err({ kind: "invalid_state", message: "Script is empty" }); - if (source.length > MAX_SOURCE_BYTES) return err({ kind: "invalid_state", message: `Script exceeds ${MAX_SOURCE_BYTES}B size cap` }); - - let executeFn: (...a: unknown[]) => Promise<unknown>; - try { - executeFn = new AsyncFunction( - "params", "daemon", "require", "signal", "onUpdate", "ctx", - "console", "fetch", "JSON", "Buffer", "setTimeout", "clearTimeout", - `"use strict";\n${source}`, - ); - } catch (e) { - return err({ kind: "invalid_state", message: `Syntax error: ${e instanceof Error ? e.message : String(e)}` }); - } - - const timeoutMs = args.timeoutMs ?? 60_000; - const ac = new AbortController(); - const onAbort = (): void => ac.abort(); - signal.addEventListener("abort", onAbort, { once: true }); - const timeoutTimer = setTimeout(() => ac.abort(), timeoutMs); - - try { - const scriptPromise = executeFn( - args.params ?? {}, - client, // exposed as `daemon` — the binding name is preserved for back-compat - require, - ac.signal, - (u: unknown) => { - if (typeof u === "object" && u !== null && Array.isArray((u as { content?: unknown }).content)) { - const txt = ((u as { content: ReadonlyArray<unknown> }).content[0] as { text?: string } | undefined)?.text ?? ""; - try { onUpdate({ text: txt }); } catch { /* swallow */ } - } - }, - extensionCtx ?? { cwd: process.cwd() }, - console, fetch, JSON, Buffer, setTimeout, clearTimeout, - ); - const abortPromise = new Promise<never>((_, reject) => { - ac.signal.addEventListener("abort", () => reject(new Error("script aborted (timeout or cancellation)")), { once: true }); - }); - const result = await Promise.race([scriptPromise, abortPromise]); - clearTimeout(timeoutTimer); - signal.removeEventListener("abort", onAbort); - if (typeof result !== "object" || result === null || !Array.isArray((result as { content?: unknown }).content)) { - return err({ kind: "invalid_state", message: `Script must return { content: [...] }; got ${JSON.stringify(result)}` }); - } - const content = (result as { content: ReadonlyArray<unknown> }).content; - if (!content.every(isContentItem)) { - return err({ kind: "invalid_state", message: "Script content array must contain { type: 'text', text: string } items" }); - } - const textOut = content.map((c) => (c as ContentItem).text).join("\n"); - const details = (result as { details?: Record<string, unknown> }).details; - return ok({ text: textOut, ...(details ? { details } : {}) }); - } catch (e) { - clearTimeout(timeoutTimer); - signal.removeEventListener("abort", onAbort); - return err({ kind: "internal", message: `Script execution failed: ${e instanceof Error ? e.message : String(e)}` }); - } - }, -}); - -// Force `sleep` to be referenced so the unused-import lint stays quiet for now. -export const _unused = sleep; -``` - -(The `_unused = sleep` line gets removed if a domain ends up not needing `sleep`; safer kept than removed mid-rewrite.) - -- [ ] **Step 22.2:** TOOLS, **Step 22.3:** remove `browser_execute_js` and `browser_run_script` from `tools.ts`, **Step 22.4:** typecheck, **Step 22.5:** commit `feat(domains): migrate execute_js + run_script with path allowlist and timeout`. - ---- - -## Task 23: Wire-up — `index.ts`, `state.ts`, `prompt.ts` - -By now, `tools.ts` should be empty (or contain only the `cleanupTempDirs` re-export). The legacy daemon path is no longer needed. - -**Files:** -- Modify: `src/index.ts` -- Modify: `src/state.ts` -- Modify: `src/prompt.ts` - -- [ ] **Step 23.1: Replace `src/index.ts`** — remove all references to `BrowserDaemon`, `registerTools`, the dead `tool_result` hook, and the temp-dir cleanup import (now from `util/truncate`). - -```ts -import type { ExtensionAPI } from "@mariozechner/pi-coding-agent"; -import { type BrowserClient, createBrowserClient } from "./client"; -import { getBrowserSystemPrompt } from "./prompt"; -import { registerSetupCommand } from "./setup"; -import { type BrowserState, defaultState, persistState, restoreState } from "./state"; -import { registerAllTools } from "./registry"; -import { cleanupTempDirs } from "./util/truncate"; - -export default function browserHarnessExtension(pi: ExtensionAPI) { - const flagNs = pi.getFlag("browser-namespace") as string | undefined; - const namespace = flagNs ?? `pi-${Math.random().toString(36).slice(2, 10)}`; - - let state: BrowserState = defaultState(namespace); - let client: BrowserClient | null = null; - let toolsRegistered = false; - - pi.registerFlag("browser-namespace", { - description: "Browser daemon namespace. Default: auto-generated", - type: "string", - }); - pi.registerFlag("browser-debug-clicks", { - description: "Enable debug click overlay (saves annotated screenshots to /tmp)", - type: "boolean", default: false, - }); - - pi.registerCommand("browser-status", { - description: "Show browser connection status and current page", - handler: async (_args, ctx) => { - if (!client) { ctx.ui.notify("Browser client not started. Run /browser-setup first.", "warning"); return; } - const s = client.status(); - const lines = [`Browser: ${s.alive ? "🟢 Connected" : "🔴 Disconnected"}`, `Session: ${s.sessionId ?? "none"}`]; - if (s.remoteBrowserId) lines.push(`Browser ID: ${s.remoteBrowserId}`); - if (s.alive) { - const info = await client.pageInfo(); - if (info.success) { - if ("dialog" in info.data) lines.push(`\n⚠️ Dialog open: ${info.data.dialog.type} — "${info.data.dialog.message}"`); - else lines.push(`\nCurrent Page:`, ` URL: ${info.data.url}`, ` Title: ${info.data.title}`, ` Viewport: ${info.data.width}x${info.data.height}`); - } - } - ctx.ui.notify(lines.join("\n"), "info"); - }, - }); - - pi.registerCommand("browser-reload-daemon", { - description: "Restart the browser client", - handler: async (_args, ctx) => { - if (!client) { ctx.ui.notify("Browser client not started.", "warning"); return; } - ctx.ui.notify("Restarting browser client...", "info"); - await client.stop(); - const r = await client.start(); - if (r.success) ctx.ui.notify("Browser client restarted ✓", "info"); - else ctx.ui.notify(`Restart failed: ${r.error.message}`, "error"); - }, - }); - - pi.on("session_start", async (_event, ctx) => { - state = restoreState(ctx, state.namespace); - client = createBrowserClient({ namespace: state.namespace }); - await client.start(); // failure is fine — surfaced via tool errors - if (!toolsRegistered) { - registerAllTools(pi, client); - toolsRegistered = true; - } - if (client) registerSetupCommand(pi, client); - ctx.ui.setStatus("browser", client.status().alive ? "🟢 Browser connected" : "🔴 Browser — run /browser-setup"); - }); - - pi.on("session_shutdown", async () => { - persistState(pi, state); - if (client) { try { await client.stop(); } catch { /* best-effort */ } client = null; } - toolsRegistered = false; - await cleanupTempDirs(); - }); - - pi.on("session_tree", async (_event, ctx) => { - state = restoreState(ctx, client?.namespace); - persistState(pi, state); - }); - - pi.on("before_agent_start", async (event) => { - if (!client || !client.status().alive) { - return { systemPrompt: event.systemPrompt + `\n\n## Browser Control\n\nBrowser tools (browser_*) are available but the browser is not connected. Run /browser-setup.` }; - } - return { systemPrompt: event.systemPrompt + getBrowserSystemPrompt() }; - }); -} -``` - -- [ ] **Step 23.2: Update `src/setup.ts`** to take `BrowserClient` instead of `BrowserDaemon`. Replace `daemon` references with `client` and the `daemon.start()` / `daemon.newTab(...)` / `daemon.getPageInfo()` / `daemon.cdp(...)` calls with their `client` equivalents. Keep the same prompts and behaviors. - -- [ ] **Step 23.3: Trim `src/state.ts`** — remove `tabHistory`, `screenshotDir`, `debugClicks` fields and their helpers. `BrowserState` becomes: - -```ts -export type BrowserState = { - readonly namespace: string; - readonly remoteBrowserId?: string; -}; -export const defaultState = (namespace = "default"): BrowserState => ({ namespace }); -``` - -(`persistState` and `restoreState` keep their signatures; their bodies just drop the removed fields.) - -- [ ] **Step 23.4: Update `src/prompt.ts`** — apply the one-line tweaks for items 1, 3, 5, 8, 9 from spec §7. (Concretely: clarify that `browser_navigate` reports its outcome, that `dispatch_key` returns `matched` count, that `wait_for_load` waits for `loadEventFired`, that scroll's deltaY follows W3C convention, and that screenshot returns `attached: true|false`.) - -- [ ] **Step 23.5:** Typecheck — PASS. -- [ ] **Step 23.6:** Commit `feat: wire BrowserClient through index/setup/state/prompt`. - ---- - -## Task 24: Delete legacy files - -- [ ] **Step 24.1:** Confirm `src/tools.ts` is empty of registrations. The only thing it might still export is `cleanupTempDirs`, which now lives in `src/util/truncate.ts`. Delete `src/tools.ts`. -- [ ] **Step 24.2:** Delete `src/daemon.ts`. -- [ ] **Step 24.3:** Delete `src/protocol.ts`. -- [ ] **Step 24.4:** Delete `src/renderers.ts` (the one renderer it had was dead). -- [ ] **Step 24.5:** Delete `tsconfig.legacy.json` and revert `package.json` `scripts.typecheck` to `tsc --noEmit`. -- [ ] **Step 24.6:** Update `tsconfig.json` `include` back to `["src/**/*.ts"]`. -- [ ] **Step 24.7:** Typecheck — PASS. -- [ ] **Step 24.8:** Commit `refactor: delete legacy daemon, tools, protocol, renderers`. - ---- - -## Task 25: Final cleanup — `_unused`/`_silence` references and lint pass - -- [ ] **Step 25.1:** Remove the `_unused = sleep` line from `domains/js.ts` and the `_silence` export from `util/sharp-shim.ts` if they're no longer needed. -- [ ] **Step 25.2:** Search for any remaining `as ` assertions, `any` types, or `@ts-ignore` comments: - -```bash -grep -rn " as [A-Z]" src/ | grep -v "as const" | grep -v "as unknown" -grep -rn ": any\| any\[" src/ -grep -rn "@ts-ignore\|@ts-expect-error" src/ -``` - -For each match: replace with a type guard, an `unknown` + narrowing, or a Result. The few legitimate `as unknown as Foo` casts at trust boundaries (e.g., parsing CDP responses) stay but get a one-line comment explaining why. - -- [ ] **Step 25.3:** Typecheck — PASS. -- [ ] **Step 25.4:** Commit `chore: remove temporary anchors and tighten remaining type assertions`. - ---- - -## Task 26: Manual smoke run (full verification) - -- [ ] **Step 26.1:** Pre-req: Chrome running with remote-debugging enabled. Install the package into a local pi workspace per `CONTRIBUTING.md`. - -- [ ] **Step 26.2:** Run each scenario from spec §11 and tick: - - - [ ] Daemon connects. - - [ ] `browser_navigate("https://example.com")` succeeds; result `details.outcome.kind` is either `in_place` or `new_tab_created`. - - [ ] `browser_screenshot()` produces a PNG; result `details.path` exists, `details.attached` is set. - - [ ] `browser_click({x, y})` clicks the More-info link. - - [ ] `browser_type("hello")` then `browser_press_key("Enter")` work in a search box. - - [ ] `browser_scroll({})` scrolls down 300px. - - [ ] Trigger an `alert()` via `browser_execute_js("alert('hi')")` — `browser_page_info()` reports the dialog, `browser_handle_dialog({accept: true})` dismisses it. - - [ ] `browser_open_urls({urls: [..3 urls..]})` reports per-URL outcomes. - - [ ] `browser_run_script("/etc/passwd")` is rejected with `kind: "invalid_state"`. - - [ ] `browser_run_script` with a 1-second `timeoutMs` and an infinite-loop script aborts in <2s with `kind: "internal"` containing "aborted". - - [ ] `BH_DEBUG_CLICKS=1` then `browser_click` produces an annotated screenshot at `details.debugScreenshotPath`. - - [ ] `npm run typecheck` is green. - -- [ ] **Step 26.3:** If anything fails, fix it in a follow-up commit before declaring done. Do not edit the plan retroactively. - ---- - -## Task 27: Version bump and CHANGELOG - -- [ ] **Step 27.1:** In `package.json`, set `"version": "0.3.0"`. - -- [ ] **Step 27.2:** Prepend to `CHANGELOG.md`: - -```markdown -## 0.3.0 — 2026-05-02 - -### Internal rewrite - -- Per-domain module split: every tool now lives in its own `src/domains/<name>.ts` file. -- New transport/session/client split: `BrowserDaemon` class replaced with `createBrowserClient()` factory composing a `CdpTransport` and `CdpSession`. -- All tool handlers return `Result<T, E>`; one `defineTool` helper converts to pi's `ToolResult` and supplies a uniform `details` shape: `{ ok: true, ... }` on success, `{ ok: false, kind, message, ... }` on error. -- Strict TypeScript flags enabled (`noUncheckedIndexedAccess`, `exactOptionalPropertyTypes`, `noPropertyAccessFromIndexSignature`, etc.). Zero `any`, zero unchecked `as` casts. - -### Bug fixes (predictability) - -- `browser_navigate` no longer silently creates a new tab when navigation fails; outcome is reported explicitly. -- `browser_wait_for_load` now uses CDP load events instead of relying on a single `readyState` snapshot. -- Page info cache invalidation is automatic via CDP `Page.frameNavigated` / `Page.loadEventFired` subscription. -- Dialog read no longer mutates state; `browser_page_info` is now read-only. -- `browser_dispatch_key` returns the count of matched elements; zero matches is an error. -- `browser_http_get` timeout now covers the response body read. -- `sharp` failures are distinguished from "sharp not installed". -- Screenshot paths use UUIDs — no more concurrent-write collisions. -- Reconnect is lazy (via `ensureAlive`) — no background reconnect, no stacked Chrome consent popups. - -### Security fixes - -- All JS evaluation source is built via `safeJs\`...\`` (always JSON.stringify-safe). The previous `replace(/'/g, "\\'")` escaping is gone. -- `browser_run_script` now requires: - - script path inside `tmpdir()`, `cwd()`, or `BH_SCRIPT_DIR` - - a mandatory timeout (default 60s, max 600s) - - the AbortSignal is honored even if the script ignores it - - source size ≤ 1 MB - - return shape validated structurally -- `browser_download` validates the directory exists and is writable. -- `browser_upload_file` verifies the file is readable before any CDP call. - -### Parameter renames (saved scripts must be updated) - -- `browser_click`: `clicks` → `count` -- `browser_dispatch_key`: `event` → `eventType` - -### Removed - -- The unused `tabHistory`, `screenshotDir`, and `debugClicks` fields on persisted state. -- The dead `tool_result` hook for tab-history tracking. -- `src/protocol.ts`, `src/renderers.ts`, `src/daemon.ts`, `src/tools.ts` (replaced by per-domain files and `client.ts`). - -### Known follow-ups - -- `browser_get_network_log` returns a deprecation note pending a `session.recentEvents()` API. -- No tests added in this rewrite; that's a separate workstream. -``` - -- [ ] **Step 27.3:** Commit and tag: - -```bash -git add package.json CHANGELOG.md -git commit -m "chore(release): v0.3.0 — internal rewrite" -git tag v0.3.0 -``` - ---- - -## Self-review - -**1. Spec coverage check:** - -- §3 module layout → Tasks 1, 2, 4–8, 9–22, 24 ✓ -- §4 `defineTool` helper → Task 8 ✓ -- §4.1 param renames → click (Task 9), dispatch_key (Task 10) ✓ -- §5 transport/session/client split → Tasks 4, 5, 6, 7 ✓ -- §5.1 structural bug fixes (event buffer, page-info cache, dialog race, navigate fallthrough, http body timeout, screenshot paths) → Tasks 1, 6, 7, 12, 15, 21 ✓ -- §5.2 reconnect change → Task 7 (in `ensureAlive`) ✓ -- §6.1 safeJs → Task 2 (helper), used in Tasks 7, 10, 12, 18 ✓ -- §6.2 run_script hardening (path allowlist, timeout, AbortSignal, return validator, size cap) → Task 22 ✓ -- §6.3 download dir validation, upload readability → Task 18 ✓ -- §7 predictability register (12 items) → Tasks 9, 10, 11, 12, 14, 15, 21, 23 ✓ -- §8 strict tsconfig → Tasks 3, 24 ✓ -- §9 migration notes → Task 27 ✓ -- §11 verification → Task 26 ✓ - -**2. Placeholder scan:** No "TBD"/"TODO"/"implement later". Each step has concrete code or a concrete command. - -**3. Type consistency:** `BrowserClient`, `CdpSession`, `CdpTransport`, `Result`, `ToolErr`, `ToolOk`, `defineTool`, `registerTool`, `safeJs`, `screenshotPath`, `applyTruncation` — names used in later tasks match their definitions in earlier tasks. The `client.session().call(...)` pattern is consistent across all domain files. - -**4. Notes I noticed during review:** -- Task 21's `browser_get_network_log` returns a deprecation note rather than full functionality. This is called out in Task 27's CHANGELOG ("Known follow-ups"). Acceptable for an internal-only rewrite per spec §10 "no new functionality." -- Task 9 has a temporary placeholder for the debug crosshair (uses a plain screenshot); Task 14 explicitly restores the real overlay. The two tasks are linked — flagged in Step 14.3. -- Task 17's `browser_current_tab` calls `Target.getTargetInfo`, matching the legacy behavior. ✓ -- Task 18's `browser_upload_file` keeps the CDP-then-JS-fallback flow but both paths now use `safeJs`. ✓ 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**. diff --git a/docs/superpowers/specs/2026-05-02-harness-rewrite-design.md b/docs/superpowers/specs/2026-05-02-harness-rewrite-design.md deleted file mode 100644 index 3b1dbec..0000000 --- a/docs/superpowers/specs/2026-05-02-harness-rewrite-design.md +++ /dev/null @@ -1,423 +0,0 @@ -# pi-browser-harness — internal rewrite (v0.3.0) - -**Date:** 2026-05-02 -**Scope:** internals only (`src/`); no tests, no CI changes -**LLM-facing API posture:** frozen tool names; normalized parameter names and unified `details` schema -**Code-quality bar:** strict TypeScript, no `any`, factory functions, `Result<T, E>`, TypeBox-derived types, immutable data - ---- - -## 1. Why - -The harness works but is hard to change safely. Two files dominate the codebase: `daemon.ts` (1140 LOC) and `tools.ts` (1818 LOC). The same six-line try/catch + error-formatting block is copy-pasted across 28 tools. Several bugs and one full-RCE security gap stem from missing module boundaries rather than from careless coding. - -This rewrite restructures the internals so that: - -- the same bug class can't recur (structural prevention, not vigilance) -- adding a new tool is a single file under `src/domains/` -- every tool exposes a uniform `Result`-shaped contract to the LLM -- the package's external surface (28 tool names, the system prompt, the npm package name) stays compatible at the *name* level; only parameter names and return-detail shapes change - -## 2. Current-state audit (problems that drive the design) - -### 2.1 Structural - -- `daemon.ts` is one God class doing transport, session management, reconnect, dialog detection, event buffering, page-info caching, screenshots, file upload, drag, PDF, viewport, network log, and key-code maps. -- `tools.ts` is 28 tool registrations inline. Each tool repeats the `await daemon.ensureAlive()` → `try { … } catch (err) { return { isError: true, content: [{type:"text", text: \`X failed: ${err instanceof Error ? err.message : String(err)}\`}], details: undefined } }` pattern. ~30 lines × 28 tools = ~800 lines of pure noise. -- `details` field is inconsistent: most tools return `undefined`, a few return rich objects. The agent has no contract to parse. -- `index.ts` has a `tool_result` hook that reads `details.targetId` from `browser_new_tab` / `browser_navigate` — but those tools never set it. Dead code. -- `state.ts` persists `tabHistory`, `screenshotDir`, `debugClicks` — none are read. -- `renderers.ts` registers a renderer for a message type that's never produced. - -### 2.2 Predictability bugs - -| # | Bug | -|---|---| -| 1 | `browser_navigate` outer try/catch silently makes a new tab when navigation fails — masks the real error | -| 2 | `getPageInfo()` clears `_currentDialog` on read; two callers in the same tick disagree | -| 3 | `browser_wait_for_load` returns `true` from the initial `readyState === "complete"` check even if a navigation just kicked off and the previous page hasn't unloaded | -| 4 | `browser_open_urls` swallows phase-2 errors but lets `onUpdate` errors propagate uncaught | -| 5 | `browser_dispatch_key` reports success even when the selector matched zero elements | -| 6 | Optional `sharp` dependency: real sharp errors are reported as "install sharp for auto-resize" | -| 7 | Background reconnect on `ws.onclose` can stack consent popups; `_consentObserved` flag is a workaround for this | -| 8 | `browser_scroll` — description and code disagree on default direction | -| 9 | `browser_screenshot` returns `{path, format, quality}` but agent has no signal about whether the image was actually attached | -| 10 | `_eventBuffer` survives reconnects with stale events from the previous connection | -| 11 | Page-info cache lives on the daemon and outlives navigations not initiated by `Page.navigate` (e.g. SPA route changes) | -| 12 | Screenshot path uses `Date.now() + global counter` — concurrent calls within the same millisecond collide | -| 13 | `browser_http_get` 20 s timeout aborts the headers read but not `await response.text()` — body read can hang | - -### 2.3 Security gaps - -| # | Gap | -|---|---| -| S1 | JS-injection in error-message templates: `daemon.uploadFile` and `browser_dispatch_key` use `selector.replace(/'/g, "\\'")` — broken for backslashes, newlines, unicode quotes, `</script>` | -| S2 | `browser_run_script` is unsandboxed RCE: `new AsyncFunction(...)` over arbitrary file contents with `daemon`, `require`, `Buffer`, `fetch` bound; the documented `signal` is never honored; no timeout enforced; no path allowlist | -| S3 | `setDownloadBehavior` accepts any path; Chrome silently downloads to nowhere if the dir doesn't exist or isn't writable | -| S4 | `uploadFile` doesn't verify the file is readable before issuing CDP calls; can leave the input in a half-set state | - -## 3. Module layout - -``` -src/ - index.ts extension entry: lifecycle, hooks, prompt injection - prompt.ts system-prompt builder - state.ts branch state (pruned: dead fields removed) - setup.ts /browser-setup command - registry.ts iterates domain modules → pi.registerTool - - cdp/ - transport.ts WebSocket multiplex: send, request/response, event stream - session.ts target attach/switch/recover, dialog buffer, event buffer - discovery.ts DevToolsActivePort lookup + port probe - errors.ts CdpError, SessionNotFoundError, TimeoutError - types.ts CDPMessage, CDPEvent, RawCdpResult - - client.ts BrowserClient factory: composes transport+session, - exposes the high-level surface every domain calls - - domains/ - navigate.ts browser_navigate, browser_open_urls - history.ts browser_go_back, browser_go_forward, browser_reload - click.ts browser_click - keyboard.ts browser_type, browser_press_key, browser_dispatch_key - scroll.ts browser_scroll - screenshot.ts browser_screenshot (+ debug overlay) - tabs.ts browser_list_tabs, browser_current_tab, browser_switch_tab, - browser_new_tab - page.ts browser_page_info, browser_wait, browser_wait_for_load - dialog.ts browser_handle_dialog - js.ts browser_execute_js, browser_run_script - files.ts browser_upload_file, browser_download, browser_print_to_pdf - viewport.ts browser_viewport_resize - drag.ts browser_drag_and_drop - network.ts browser_http_get, browser_get_network_log - - schemas/ - common.ts shared TypeBox primitives (Coords, Modifiers, etc.) - - util/ - result.ts Result<T,E> + ok/err/map/andThen - tool.ts defineTool() helper — the boilerplate-killer - truncate.ts output truncation + temp-file fallback - keycodes.ts pure key-code maps - paths.ts namespaced screenshot paths (UUID, no global counter) - js-template.ts safeJs() — always JSON.stringify interpolated values - time.ts sleep, deadline helpers -``` - -**Deleted:** `protocol.ts` (types moved to `cdp/types.ts` and `client.ts`), `renderers.ts` (the one renderer is dead; per-tool renderers stay in their domain files). - -**File-size targets:** every file ≤ 300 LOC. Largest expected: `cdp/transport.ts` (~250), `domains/js.ts` (~200). Most domain files: 60–120 LOC. - -**Dependency direction:** `domains/* → client → cdp/* → util/*`. Domains never reach into transport directly. Transport never knows about tools. - -## 4. The `defineTool` helper - -```ts -// util/tool.ts - -import type { TSchema, Static } from "typebox"; -import type { ExtensionAPI, ToolContext, ToolResult } from "@mariozechner/pi-coding-agent"; -import type { BrowserClient } from "../client"; -import type { Result } from "./result"; - -export type ToolOk = { - readonly text: string; - readonly details?: Readonly<Record<string, unknown>>; -}; - -export type ToolErrKind = - | "not_connected" - | "cdp_error" - | "timeout" - | "invalid_state" - | "io_error" - | "internal"; - -export type ToolErr = { - readonly kind: ToolErrKind; - readonly message: string; - readonly details?: Readonly<Record<string, unknown>>; -}; - -export type HandlerContext = { - readonly client: BrowserClient; - readonly signal: AbortSignal; - readonly onUpdate: (update: ToolOk) => void; - readonly extensionCtx: ToolContext; -}; - -export type ToolHandler<S extends TSchema> = ( - args: Static<S>, - ctx: HandlerContext, -) => Promise<Result<ToolOk, ToolErr>>; - -export type ToolDefinition<S extends TSchema> = { - readonly name: string; - readonly label: string; - readonly description: string; - readonly promptSnippet: string; - readonly promptGuidelines: ReadonlyArray<string>; - readonly parameters: S; - readonly handler: ToolHandler<S>; - readonly renderCall?: (args: Static<S>, theme: unknown) => unknown; - readonly renderResult?: (result: ToolResult, opts: { expanded: boolean }, theme: unknown) => unknown; - readonly ensureAlive?: boolean; // default true -}; - -export const defineTool = <S extends TSchema>(def: ToolDefinition<S>) => def; - -export const registerTool = <S extends TSchema>( - pi: ExtensionAPI, - client: BrowserClient, - def: ToolDefinition<S>, -): void => { - pi.registerTool({ - name: def.name, - label: def.label, - description: def.description, - promptSnippet: def.promptSnippet, - promptGuidelines: [...def.promptGuidelines], - parameters: def.parameters, - renderCall: def.renderCall, - renderResult: def.renderResult, - async execute(_id, args, signal, onUpdate, extensionCtx) { - if (def.ensureAlive !== false) { - const alive = await client.ensureAlive(); - if (!alive.success) { - return toToolResult( - { success: false, error: { kind: "not_connected", message: alive.error.message } }, - def.name, - ); - } - } - const result = await def.handler(args as Static<S>, { - client, - signal, - onUpdate: (u) => onUpdate?.({ content: [{ type: "text", text: u.text }], details: u.details }), - extensionCtx, - }); - return toToolResult(result, def.name); - }, - }); -}; - -const toToolResult = ( - r: Result<ToolOk, ToolErr>, - toolName: string, -): ToolResult => { - if (r.success) { - return { - content: [{ type: "text", text: r.data.text }], - details: { ok: true, ...(r.data.details ?? {}) }, - }; - } - return { - isError: true, - content: [{ type: "text", text: `${toolName} failed (${r.error.kind}): ${r.error.message}` }], - details: { ok: false, kind: r.error.kind, message: r.error.message, ...(r.error.details ?? {}) }, - }; -}; -``` - -The helper is the only place that converts a handler's `Result` into pi's `ToolResult`. `details` is unified: - -- success: `{ ok: true, …handlerDetails }` -- error: `{ ok: false, kind, message, …handlerDetails }` - -The agent always sees `details.ok` and `details.kind` and can branch. - -### 4.1 Frozen names, normalized params - -| Tool | Old param | New param | Why | -|---|---|---|---| -| `browser_click` | `clicks` | `count` | matches CDP `clickCount` | -| `browser_dispatch_key` | `event` | `eventType` | clearer + validates against literal union | -| `browser_click` | `button: string` | `button: "left" \| "right" \| "middle"` | actually validates | -| `browser_press_key` | `modifiers: number` | `modifiers: number` (TypeBox `Integer`, range 0–15) | rejects nonsense | -| (all) | implicit defaults via `??` | TypeBox `default:` field | one source of truth | - -## 5. Transport / session split - -```ts -// cdp/transport.ts — owns ONLY the WebSocket -export interface CdpTransport { - connect(url: string, opts: { timeoutMs: number }): Promise<Result<void, CdpError>>; - close(): Promise<void>; - request(method: string, params: object, opts: { sessionId: string | null; timeoutMs: number }): Promise<Result<unknown, CdpError>>; - events(): AsyncIterable<CdpEvent>; - state(): "open" | "closed" | "connecting"; -} - -// cdp/session.ts — target attach/switch + buffered events -export interface CdpSession { - attachFirstPage(): Promise<Result<{ targetId: string; sessionId: string }, CdpError>>; - switchTo(targetId: string): Promise<Result<void, CdpError>>; - current(): { sessionId: string; targetId: string } | null; - call(method: string, params?: object): Promise<Result<unknown, CdpError>>; - callOnTarget(method: string, params: object, sessionId: string): Promise<Result<unknown, CdpError>>; - takeDialog(): DialogInfo | null; - drainEvents(filter?: (e: CdpEvent) => boolean): ReadonlyArray<CdpEvent>; - onClose(cb: () => void): void; -} - -// client.ts — composes everything; what domains see -export interface BrowserClient { - readonly ensureAlive: () => Promise<Result<void, CdpError>>; - readonly status: () => DaemonStatus; - - readonly input: { click; type; pressKey; dispatchKey; scroll; drag }; - readonly nav: { navigate; newTab; goBack; goForward; reload; openUrls }; - readonly tabs: { list; current; switch }; - readonly page: { info; waitForLoad; dialog }; - readonly screenshot: { capture; captureWithCrosshair; printPdf }; - readonly files: { upload; configureDownloads }; - readonly viewport: { resize }; - readonly net: { httpGet; networkLog }; - readonly js: { evaluate; runScript }; - - readonly raw: (method: string, params?: object, sessionId?: string) => Promise<Result<unknown, CdpError>>; -} -``` - -### 5.1 Bugs fixed structurally by this split - -1. **Stale event buffer (audit #10).** `events()` is an `AsyncIterable` bound to one connection. When the WS closes, the iterator ends; the next connection starts a fresh stream. There is no shared buffer to leak. -2. **Stale page-info cache (audit #11).** Cache lives in `client.page` and subscribes to `Page.frameNavigated` / `Page.loadEventFired` from `session.drainEvents()`. Invalidation is automatic — the 5 manual `invalidatePageInfoCache()` calls go away. -3. **Dialog read-and-consume race (audit #2).** `session.takeDialog()` is the only mutator; `client.page.info()` returns `{ dialog?, page? }` without mutating state. -4. **`browser_navigate` silent fallthrough (audit #1).** `client.nav.navigate()` returns `Result<NavOutcome, CdpError>` where `NavOutcome = {kind: "in_place", …} | {kind: "new_tab_created", reason: "no_tabs" | "internal_url"}`. The domain reports the outcome verbatim. -5. **HTTP-GET body read can hang (audit #13).** `client.net.httpGet` uses one `AbortSignal` whose timeout covers `await response.text()`. -6. **Screenshot path collisions (audit #12).** `util/paths.ts` returns `pi-browser-screenshot-${namespace}-${randomUUID()}.png`. No counter, no `Date.now()` collisions. - -### 5.2 Reconnect behavior change - -Today: `ws.onclose` schedules a background reconnect that may or may not succeed; in-flight requests reject with no retry. - -Rewrite: reconnect is **lazy** — `ensureAlive()` is the only path that opens a new connection, and `defineTool` calls it before every tool runs. - -- One in-flight failure → one error reported to the agent (clear). -- Next tool call → `ensureAlive` heals it (transparent). -- No silent stacked Chrome consent popups (the existing `_consentObserved` flag is removed because the failure mode is gone). - -## 6. Security fixes - -### 6.1 JS injection in error-message templates (S1) - -```ts -// util/js-template.ts -export const safeJs = (strings: TemplateStringsArray, ...values: unknown[]): string => { - let out = strings[0] ?? ""; - for (let i = 0; i < values.length; i++) { - out += JSON.stringify(values[i]); - out += strings[i + 1] ?? ""; - } - return out; -}; -``` - -Every JS-injection site (`uploadFile` fallback, `dispatch_key`, `getPageInfo`, debug overlay's `devicePixelRatio`) becomes `safeJs\`document.querySelector(${selector})\``. The `safeJs` helper is the **only** way to build evaluation source in the codebase. The pattern is documented in `client.js.evaluate`'s JSDoc. - -### 6.2 `browser_run_script` — bounded escape hatch (S2) - -The full RCE is fundamental; we tighten the contract instead of trying to sandbox: - -1. **Path allowlist.** Refuse paths outside `tmpdir()`, `process.cwd()`, or `BH_SCRIPT_DIR`. Returns `{ kind: "invalid_state", message: "script path outside allowed directories" }`. -2. **Mandatory timeout.** Default 60 s, configurable via `timeoutMs` (max 600 s). When the AbortSignal fires, the script's `signal` is aborted *and* a hard timer rejects the outer promise. -3. **AbortSignal actually wired.** Outer promise is `Promise.race([scriptPromise, abortPromise])` so cancellation works even if the script ignores `signal`. -4. **Return-shape validator.** Validate every content item against a TypeBox schema, not just `Array.isArray`. -5. **Source size cap.** Refuse scripts > 1 MB. - -We do not sandbox via `vm`/`worker_threads` — that breaks the `daemon` binding's purpose. The doc string makes the trust boundary explicit: "this tool runs arbitrary JS with full Node permissions; only invoke when the user has approved a script you wrote to disk." - -### 6.3 Misc (S3, S4) - -- `setDownloadBehavior` validates the path is an existing writable directory before calling CDP. -- `uploadFile` validates the file exists and is readable before any CDP call. -- `dispatch_key` returns `Result<{ matched: number }, …>` so the agent can detect zero-match selectors. - -## 7. Predictability fixes register - -| # | Today | Rewrite | -|---|---|---| -| 1 | `browser_navigate` silently makes a new tab on error | Returns explicit `{kind: "in_place" \| "new_tab_created" \| "failed"}` | -| 2 | `getPageInfo()` clears `_currentDialog` on read | `takeDialog()` is the only mutator; `info()` is read-only | -| 3 | `wait_for_load` returns true if `readyState === "complete"` immediately | Subscribes to `Page.frameStartedLoading` first; only returns true after `loadEventFired` or timeout | -| 4 | `open_urls` swallows phase-2 errors, propagates `onUpdate` errors | All errors classified the same way; `onUpdate` failures caught | -| 5 | `dispatch_key` reports success on zero matches | Returns `matched` count; zero matches = error | -| 6 | `sharp` failures reported as "install sharp" | Distinguish "module not found" from "sharp threw" | -| 7 | Background reconnect can stack consent popups | Lazy reconnect via `ensureAlive` only | -| 8 | `scroll` description / code disagree on default direction | Description matches code, code matches W3C wheel-event convention | -| 9 | `screenshot` doesn't tell agent if image was attached | Always returns `{ ok: true, path, attached: true \| false }` | -| 10 | Tab-history tracking in `index.ts` is dead code | Deleted; `state.ts` keeps only `namespace` and `remoteBrowserId` | -| 11 | `renderers.ts` registers a renderer for a message type nobody emits | File deleted; renderers live in domain files that produce them | -| 12 | `getVirtualKeyCode` / `getKeyCode` on the daemon class but pure | Moved to `util/keycodes.ts`, exported as pure functions | - -**Prompt impact:** the system prompt in `prompt.ts` gets one-line updates for items 1, 3, 5, 8, 9. The 28 tool names and their descriptions are unchanged. - -## 8. tsconfig + style enforcement - -```json -{ - "compilerOptions": { - "target": "ES2022", - "module": "ESNext", - "moduleResolution": "bundler", - "strict": true, - "noImplicitAny": true, - "strictNullChecks": true, - "noUnusedLocals": true, - "noUnusedParameters": true, - "noImplicitReturns": true, - "noFallthroughCasesInSwitch": true, - "noUncheckedIndexedAccess": true, - "exactOptionalPropertyTypes": true, - "noPropertyAccessFromIndexSignature": true, - "forceConsistentCasingInFileNames": true, - "esModuleInterop": true, - "skipLibCheck": true, - "outDir": "./dist", - "rootDir": "./src", - "declaration": true, - "declarationMap": true, - "sourceMap": true - }, - "include": ["src/**/*.ts"] -} -``` - -Hard rules enforced by review: - -- Zero `any`, zero `as Foo` assertions, zero `@ts-ignore`. The current `as any` for sharp dynamic-import is replaced with a typed shim file declaring the minimal interface used. -- All exported types use `readonly` properties and `ReadonlyArray<T>`. -- Public functions taking ≥3 args take an options object. -- Factory functions, not classes (`BrowserDaemon` class becomes `createBrowserClient(opts)`). -- `Result<T, E>` for every operation with an *expected* failure mode (everything CDP does); `throw` reserved for invariant violations. -- TypeBox schemas defined per-domain; types derived via `Static<typeof Schema>` and never re-declared by hand. - -## 9. Migration notes - -- Package version bumps to `0.3.0`. Same external surface; normalized parameter names. -- `CHANGELOG.md` records every renamed parameter (`clicks → count`, `event → eventType`). -- `benchmark/` JS files that import from `src/daemon` need `src/client` imports updated. Listed as a follow-up; benchmarks are not run by CI. -- `protocol.ts` deletion: the package's `files` entry exports the whole `src/` directory, so external code *could* in theory have imported types from `pi-browser-harness/src/protocol`. We treat this as private (it's not part of the documented surface) and don't add a re-export shim. If a downstream user reports breakage, we add a one-line re-export at the package root in a patch release. - -## 10. Out of scope - -- No tests added. Existing `benchmark/` JS files untouched (except import paths). -- No new tools beyond what exists today. Comparison-doc gaps (cookies, iframe helpers, profile sync, cloud browsers) are a follow-up. -- No CI changes. No linter setup. -- The `skills/` directory shipped with the package is untouched. -- No changes to `package.json` peer-dependency declarations beyond the version bump. - -## 11. Verification (manual smoke) - -After implementation, the implementer runs the existing `benchmark/run-benchmark.js` against a local Chrome to confirm: - -- daemon connects and can screenshot, click, navigate, type, scroll -- a dialog raised by a page is detected by `browser_page_info` and dismissed by `browser_handle_dialog` -- `browser_open_urls` opens 3 URLs in parallel and reports per-URL outcomes -- `browser_run_script` rejects a path outside `tmpdir()`/`cwd()` -- `browser_run_script` honors a 1-second timeout -- `BH_DEBUG_CLICKS=1 browser_click` produces an annotated screenshot -- `tsc --noEmit` passes with the new strict flags diff --git a/docs/superpowers/specs/2026-07-26-codebase-refactor-design.md b/docs/superpowers/specs/2026-07-26-codebase-refactor-design.md deleted file mode 100644 index 82eeb35..0000000 --- a/docs/superpowers/specs/2026-07-26-codebase-refactor-design.md +++ /dev/null @@ -1,264 +0,0 @@ -# Codebase Refactor: Comments, Duplication, Architecture Contract, Type Safety - -Date: 2026-07-26 -Status: Approved - -## Goal - -Four outcomes, in one coordinated pass over `src/` and `test/`: - -1. Comments removed from the codebase, keeping only load-bearing "why" notes. -2. Duplication, buggy code, and stale functions found and fixed. -3. A documented design flow plus a code pattern every tool must follow, so adding - capabilities stays cheap. -4. Genuine 100% type safety — no unchecked assertion anywhere in `src/`. - -## Starting Point - -`src/` is ~10,120 lines across 70 files. `tsconfig.json` already enables `strict`, -`exactOptionalPropertyTypes`, `noUncheckedIndexedAccess`, `noUnusedLocals`, and -`noImplicitReturns`. `tsc --noEmit` passes clean. A `Result<T, E>` type and a real -tool-definition pattern (`src/util/tool.ts`, `src/registry.ts`) already exist. - -This is refinement of a sound design, not a rescue. The problems are: - -- ~60 unchecked `as { ... }` casts at the CDP boundary, because - `CdpSession.call` is typed `(method: string, params?: Record<string, unknown>) - => Promise<Result<unknown, CdpError>>`. Strict flags cannot help; the lie is at - the cast. -- `src/domains/forms.ts` (355 lines) is a stale fork of the live - `src/domains/form.ts`, imported only by a manual test. The fork dropped two - working tools: `fillFormTool` (batch fill) and `setCheckedTool`. -- `src/cdp/window-binding.ts` is dead code; nothing imports it, including its own - test file. -- Per-tool `serialized: true` flags live in `registry.ts`, away from the tool - definitions they describe. -- `test/` is not type-checked at all — `tsconfig.json` includes only - `src/**/*.ts` — and has no runner script. -- ~1,560 comment lines (15% of `src/`), mixing file-header banners and - restated-code noise with a small amount of genuinely load-bearing rationale. - -## Decisions - -| Question | Decision | -|---|---| -| Comment removal | Strip all decorative, header, and restating comments. Keep a small set of one-line "why" notes where code would otherwise look wrong. | -| CDP typing | Typed command map **and** runtime-validated decoders. | -| Behavior changes | Pure refactor, except: restore the two tools the `forms.ts` fork dropped. Real bugs get fixed and reported. | -| Tests | Type-check `test/`, and port the hand-rolled harness to `node:test` with `npm test` in CI. | - -## A. Architecture — Dependencies Point Downward Only - -``` -index.ts extension entry: flags, slash commands, session lifecycle hooks - | -registry.ts flat tool catalog: imports only, zero per-tool config - | -util/tool.ts tool runtime: liveness guard -> concurrency lock -> handler - -> Result-to-AgentToolResult conversion - | -domains/* one file per capability. Pure tool definitions. - The only layer expected to grow. - | -client.ts BrowserClient facade: session, ownership, mutation mutex, - profile pin - | -cdp/session.ts typed call/callOnTarget/callBrowser, event routing, - console + network buffers - | -cdp/daemon-transport.ts -> daemon/* out-of-process daemon owning the Chrome socket - | -Chrome (CDP over WebSocket) -``` - -Side stacks: - -- `profile/*` decides *which* Chrome and which profile to drive. Used by - `index.ts` and `client.ts`; never by domains. -- `schemas/*` holds shared typebox arg fragments. -- `util/*` are dependency-free leaves. - -The enforceable rule: **a `domains/*` file may import from `schemas/`, `util/`, -other `domains/`, and the `BrowserClient` type — never from `daemon/`, -`cdp/transport`, or `profile/`.** This holds today by accident; it becomes -documented and grep-checkable. - -## B. One Typed, Validated CDP Boundary - -45 distinct CDP methods are in use: 33 commands and 12 subscribed events. Small -enough to type exhaustively by hand. - -New `src/cdp/commands.ts` holds a single table where a typebox schema per command -is the source of both the TypeScript types and the runtime validators, so the two -cannot drift: - -```ts -const COMMANDS = { - "Target.attachToTarget": { - params: Type.Object({ targetId: Type.String(), flatten: Type.Optional(Type.Boolean()) }), - result: Type.Object({ sessionId: Type.String() }), - }, - "Browser.getWindowForTarget": { - params: Type.Object({ targetId: Type.String() }), - result: Type.Object({ windowId: Type.Number() }), - }, - // ...33 entries -} as const; - -call<M extends CdpMethod>( - method: M, - params: ParamsOf<M>, -): Promise<Result<ResultOf<M>, CdpError>>; -``` - -The session validates every response against its schema before resolving. A -malformed response becomes `err({ kind: "protocol", ... })` rather than a -`TypeError` three call frames later. `src/cdp/events.ts` gets the same treatment -for the 12 subscribed events, so handlers such as `Page.javascriptDialogOpening` -receive typed, validated params. - -This single change removes all ~60 unchecked casts. - -A named side benefit: a fully typed fake `BrowserClient` for tests becomes -trivial to write, which is what makes handler-level tool tests practical. - -## C. Tool Authoring Contract - -Every `domains/*` file must follow these nine rules. They go into -`docs/ARCHITECTURE.md` so they are reviewable rather than folklore. - -1. **One domain per file**, exporting only `<name>Tool` consts plus helpers it - owns. Past roughly 250 lines, split by capability. Current offenders: - `snapshot.ts` (497), `forms.ts` (355). -2. **Args schema at the top of the file**, named `<Name>Args`, built with - typebox, with a `description` on every field. Those strings are the agent's - only documentation of the tool. -3. **`defineBrowserTool({ ... })` holds all metadata, including concurrency.** - `concurrency: "serialized" | "parallel"` moves out of `registry.ts` into the - definition. A tool's concurrency class must not live in a different file from - the tool. -4. **Fixed handler signature:** `(args, ctx) => Promise<Result<ToolOk, ToolErr>>`. - Never throw, never `console.log`, never touch `process`. Errors are values. -5. **Reach Chrome only via `ctx.client`.** No domain imports the transport. -6. **Element targeting goes through one shared `resolveTarget(client, args)`** in - `domains/target.ts`, replacing the ref-vs-x/y branch currently copy-pasted - into click, drag, scroll, and keyboard. -7. **CDP calls go through `cdpCall(client, method, params)`**, which pre-maps - `CdpError` to `ToolErr`, retiring the - `if (!r.success) return err({ kind: "cdp_error", ... })` couplet that follows - nearly every call today. -8. **Naming:** tool name `browser_<verb>`, `label` title-case, `promptSnippet` - one line, `promptGuidelines` at most six agent-facing bullets. -9. **Every tool has a test** at `test/domains/<name>-test.ts` covering its schema - and its error paths, written against a fake client so no browser is needed. - -## D. Reaching Actual 100% Type Safety - -Strict flags are already clean, so every remaining hole is a place where the code -asserts instead of proving. Five enumerable categories: - -1. **~60 CDP casts** — removed by section B. This is the bulk of the work. -2. **13 `JSON.parse` sites.** Six already parse to `unknown` and validate. Four - cast straight to a shape: `domains/readpage/read-page.ts:34`, - `domains/search/web-search.ts:112`, `cdp/discovery.ts:80`, `domains/js.ts:63`. - All 13 get a typebox validator at the parse point, so no parsed value is ever - trusted. -3. **4 `as unknown as` casts.** The three in `cdp/event-queue.ts` are an - iterator-protocol workaround, fixed by typing the return as - `IteratorResult<CdpEvent, undefined>` so a `done: true` result legitimately - carries `undefined`. `util/tool.ts:102` fakes an `isError` field that the - pi runtime reads by duck-typing; it is replaced by a declared - `AgentToolResultWithError` type. -4. **9 non-null assertions.** Four `socket!` uses in `cdp/daemon-transport.ts` - become a narrowed local const. Four `data.model!` uses in `domains/ref-resolve.ts` - and `domains/snapshot.ts` disappear once `DOM.getBoxModel` is decoded. - `domains/tabs.ts:107` (`matches[0]!`) becomes a real narrowing. -5. **1 `@ts-ignore`** in `util/sharp-shim.ts` for the optional `sharp` peer - dependency, replaced by declaring the minimal interface actually used and - typing the dynamic import against it. - -**Config:** split into `tsconfig.base.json` holding the flags, plus -`tsconfig.json` for `src` and `tsconfig.test.json` for `test`, so `test/` is -type-checked for the first time. Add `noImplicitOverride`, -`verbatimModuleSyntax`, and `noUncheckedSideEffectImports`. - -**Keeping it at 100%:** `scripts/check-boundaries.ts`, run by `npm run check`, -fails the build on any new `as any`, `as {`, `@ts-ignore`, or `!.` in `src/`. -Without this the casts grow back. A clean `tsc` is not evidence that there are no -unchecked casts — that is exactly the trap the codebase is in today. - -## E. Duplication, Bugs, Stale Code - -| # | Finding | Fix | -|---|---|---| -| 1 | `domains/forms.ts` (355 lines) is a stale fork of the live `form.ts`; it dropped `fillFormTool` and `setCheckedTool` | Port both tools onto the current pattern in `form.ts`, register them, delete `forms.ts`, repoint `test/manual/forms-test.ts` | -| 2 | `cdp/window-binding.ts` is dead — nothing imports it, not even its test | Delete it; fold window-id lookup into one `cdp/window.ts` | -| 3 | `Browser.getWindowForTarget` decoded at 5 sites | One `getWindowId(client, targetId)` | -| 4 | `Target.attachToTarget` decoded at 6 sites | One `attachTo(client, targetId)` | -| 5 | `Runtime.evaluate` unwrapping duplicated across `client.ts`, `cdp/target-factory.ts`, `domains/js.ts`, `domains/forms.ts`, `domains/snapshot.ts` | One `evaluate<T>(client, expr, decoder)` | -| 6 | Box-model unwrap duplicated at `domains/ref-resolve.ts:56-63` and `domains/snapshot.ts:364-371` | Shared `boxOf(client, nodeId)` | -| 7 | ref-vs-x/y resolution duplicated in click, drag, scroll, keyboard | Shared `resolveTarget` (contract rule 6) | -| 8 | `serialized` flags live in `registry.ts`, away from the tools | Move into each definition (contract rule 3) | -| 9 | Bug sweep across all 70 files | Each finding is reported with its fix; no semantics change silently beyond rows 1-8 | - -Row 9 is deliberately open. The structural problems above are confirmed, but all -70 files have not yet been read closely enough to promise a specific bug count. -That audit happens during implementation, and each finding is surfaced rather -than folded into a refactor commit. - -## F. Sequencing - -Six commits, each with `tsc` green: - -1. `chore(build): split tsconfig, typecheck tests, node:test runner and npm test` - — the safety net lands before any behavior is touched. -2. `feat(cdp): typed command and event table with validated decoders` (section B) -3. `refactor(cdp): consolidate attach, window-id, evaluate, box-model helpers` - (E rows 3-6) -4. `refactor(tools): concurrency in definitions, shared target resolution, CDP - error mapping` (E rows 7-8) -5. `fix(domains): restore fillForm and setChecked, drop stale forms.ts and - window-binding.ts` (E rows 1-2, plus audit fixes from row 9) -6. `docs: architecture and tool authoring contract`, then - `refactor: strip comments` - -Comment stripping is last on purpose. Done first, it would touch ~1,560 lines in -nearly every file and make all five substantive diffs unreviewable. - -## Comment Removal Rule - -To keep "a small set of load-bearing notes" from being a judgment call made -differently in every file, the rule is mechanical. Delete: - -- every file-header block comment and section divider; -- every comment that restates what the next line does; -- every JSDoc block on a type or function whose name and signature already say it; -- every `TODO`/`ponytail:`/aside that is not actionable. - -Keep a comment only when **all three** hold: it explains *why* rather than -*what*; the code would look like a bug or an accident without it; and it fits on -one line. Everything kept is rewritten to one line. Known keepers include the -`session_shutdown` note in `index.ts` explaining that the transport is -deliberately left alive to avoid a repeated Chrome permission prompt, and the CI -comment about Chromium's `ProcessSingleton` behavior. Expect roughly 20-30 such -lines total across `src/`, down from ~1,560. - -Agent-facing strings — `description`, `promptSnippet`, `promptGuidelines`, and -typebox field descriptions — are **not comments** and are never touched. - -## Testing Strategy - -- Tools are tested at the handler level against a fake `BrowserClient`. The typed - command table from section B makes that fake trivial and fully typed. -- Existing `test/profile/*` and `test/deep-research/*` suites port to `node:test` - with their assertions intact. -- Browser-requiring scripts stay in `test/manual/` and stay manual. -- CI gains `npm test` alongside the existing typecheck and pack jobs. - -## Out of Scope - -- Any change to the daemon wire protocol or the profile-pinning behavior. -- New browser capabilities beyond restoring `fillFormTool` and `setCheckedTool`. -- Adding a linter. The boundary-check script covers the one invariant that - matters here; a full ESLint setup is a separate decision. -- The unrelated research documents under `docs/`. |