diff options
Diffstat (limited to 'docs/superpowers/specs')
| -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 |
2 files changed, 0 insertions, 687 deletions
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/`. |