diff options
| author | amankumarsingh77 <amankumarsingh7702@gmail.com> | 2026-08-02 19:17:20 +0530 |
|---|---|---|
| committer | amankumarsingh77 <amankumarsingh7702@gmail.com> | 2026-08-02 19:17:20 +0530 |
| commit | da64520016e1631d21a5e393cec6b1a51e72a2a3 (patch) | |
| tree | 538a18b2391517113a9984b8074ded13bf9dfec0 | |
| parent | f20fdf7fc4039ea71f3a8b56c7c508e46f1bbc51 (diff) | |
docs: remove the docs/ folder
The plans, specs, and audit reports under docs/superpowers/ described work
that has since landed, and ARCHITECTURE.md duplicated what the boundary
scanner and the tool contract already enforce in code. CONTRIBUTING.md now
points at the source that enforces each rule instead of at prose about it.
.gitignore keeps docs/ ignored so local scratch docs stay out of the repo.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| -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/`. |
