path: root/docs/ARCHITECTURE.md

# 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.