docs: document the layer rules and tool authoring contract

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

1 files changed, 400 insertions, 0 deletions

diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md
new file mode 100644
index 0000000..c1efae2
--- /dev/null
+++ b/docs/ARCHITECTURE.md
@@ -0,0 +1,400 @@
+# 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. 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:
+
+| 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`, `time`, `keycodes`, `truncate`, `paths`, `js-template`, `sharp-shim`) | 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/attach`,
+`cdp/window`, `cdp/evaluate`, `cdp/target-factory`, the buffers, the `CdpSession` type).
+
+**It must never import from `daemon/`, `cdp/transport`, 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.
+
+This currently holds across all of `src/domains/`. It is **not** machine-enforced —
+`scripts/check-boundaries.ts` checks casts, not imports. Verify it by eye in review.
+
+---
+
+## 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.
+
+### `cdpCall` vs `session.call`
+
+`cdpCall(client, method, params)` (`src/domains/cdp-call.ts`) is `session().call` plus the
+`CdpError -> ToolErr` mapping, retiring the `if (!r.success) return err({ kind: "cdp_error" })`
+couplet after every call. Domains use it by default.
+
+`params` is **required**, unlike `session.call`. A no-argument CDP method needs an explicit `{}`:
+
+```ts
+const r = await cdpCall(client, "Page.reload", {});
+```
+
+`src/domains/snapshot.ts` **deliberately mixes both styles.** Its three `session.call` sites —
+two in `liveValue`, one in the optional-screenshot block — swallow the failure on purpose:
+`liveValue` returns `undefined` so the caller falls back to the accessibility-tree value, and a
+failed screenshot just omits the path. Neither should surface as a tool error. This is
+intentional, not drift — do not convert them to `cdpCall`.
+
+---
+
+## 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 plus helpers it owns. 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 `cdpCall(client, method, params)`.** | `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` |
+
+Standing exceptions to rule 1: `src/domains/snapshot.ts` (477) and `src/domains/form.ts` (409)
+are over the 250-line guidance. Both are single coherent capabilities; splitting them is
+tracked work, not 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,deep-research,domains,cdp,util}/**/*-test.ts`.** A test placed
+in any other directory — `test/schemas/`, `test/daemon/` — **silently never runs.** `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.