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