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. Tool files hold tools; shared logic
                    lives in helper-only modules beside them (cdp-call, ax-tree,
                    element-call, box, ref-resolve, fill-engine, isolated-tab,
                    screenshot-capture, target). The only layer expected to grow.
  |
domains/cdp-call.ts the ONLY door from a tool to the browser: cdpCall,
                    cdpCallOnTarget, cdpCallBrowser, evalJs, cdpErrToToolErr
  |
client.ts           BrowserClient facade: session, ownership, mutation mutex,
                    profile pin
  |
cdp/session.ts      typed call/callOnTarget/callBrowser, event routing,
                    console + network buffers
  |
daemon/transport.ts -> daemon/*        out-of-process daemon owning the Chrome socket
  |
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/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.

The import rule itself is **not** machine-enforced — verify it by eye in review. What *is*
enforced is the narrower rule that matters most: two boundary-scanner rules, `raw-cdp-call` and
`raw-evaluate`, are scoped to `src/domains/` and fail the build when any file but `cdp-call.ts`
reaches `session.call*` or `evaluateJs` directly. The scope matters — `client.ts` and `cdp/` are
the layer that legitimately owns the session, so the rules deliberately do not apply there.

The rules match the two call shapes that exist today (`client.session().call…` and a `CdpSession`
held in a local named `session`). They are a guardrail, not a proof: renaming that local would
slip past them.

---

## 2. The CDP boundary

`src/cdp/commands.ts` (34 commands) and `src/cdp/events.ts` (12 events) are the single source
of truth for **both** the TypeScript types and the runtime validation. One typebox schema per
entry generates both, so they cannot drift.

```ts
const cmd = <P extends TSchema, R extends TSchema>(params: P, result: R) => ({
  params,
  result,
  validate: Compile(result),
});

export const COMMANDS = {
  "Target.attachToTarget": cmd(
    Type.Object({ targetId: Type.String(), flatten: Type.Optional(Type.Boolean()) }),
    Type.Object({ sessionId: Type.String() }),
  ),
  // ...
};

export type CdpMethod = keyof typeof COMMANDS;
export type ParamsOf<M extends CdpMethod> = Static<(typeof COMMANDS)[M]["params"]>;
export type ResultOf<M extends CdpMethod> = Static<(typeof COMMANDS)[M]["result"]>;
```

`decodeResult` validates every response before resolving; `decodeEvent` does the same for
subscribed events. A malformed payload becomes `err({ kind: "invalid_response", ... })` instead
of a `TypeError` three frames later.

### Adding a command

1. Add one `cmd(params, result)` entry to `COMMANDS`, keyed by the CDP method name.
2. Call it. `session.call(method, params)` and `cdpCall(client, method, params)` are already
   typed against the table. **No cast anywhere.**

### `additionalProperties: true` — the part that bites

`additionalProperties: true` makes the **runtime validator** permissive. It does **not** widen
the derived `Static<>` **type**. The consequence differs by direction, and getting it backwards
produces two different bugs:

| Schema | Under-specify | Over-specify |
|---|---|---|
| `params` | **compile error at the call site** — the field is not in `ParamsOf<M>`, so you cannot pass it | harmless |
| `result` / event params | field is simply unavailable to read | **silently dropped event or failed decode** when a future Chrome changes that field |

So:

- **Enumerate every field a call site passes.** Four of the six compile errors this refactor
  had to fix were exactly this: `Runtime.callFunctionOn`'s params schema omitted `arguments`,
  `returnByValue`, and `awaitPromise`, which the code genuinely sends.
- **On results and events, enumerate only what a consumer actually reads.** `Page.frameNavigated`
  and `Page.loadEventFired` are declared as fully-open objects with no enumerated fields
  (`ev(Anything)`) and never call `decodeEvent` at all — the session only needs to know they
  fired.

### How a tool reaches the browser

`src/domains/cdp-call.ts` is the only door, and the scanner enforces it (section 1):

| Helper | Use for |
|---|---|
| `cdpCall(client, method, params, opts?)` | the current tab |
| `cdpCallOnTarget(client, method, params, sessionId, opts?)` | a specific attached session |
| `cdpCallBrowser(client, method, params?, opts?)` | browser-level, no session |
| `evalJs(client, expression, sessionId?)` | evaluating JS, returns `unknown` |
| `cdpErrToToolErr(e, method)` | mapping a `CdpError` you got from elsewhere |

Each wraps the session call and maps `CdpError -> ToolErr`, so the
`if (!r.success) return err({ kind: "cdp_error", ... })` couplet is gone — a failed call is
returned as-is (`if (!r.success) return r;`).

`params` is **required** on `cdpCall`, unlike `session.call`. A no-argument CDP method needs an
explicit `{}`:

```ts
const r = await cdpCall(client, "Page.reload", {});
```

**A tool that wants to swallow a failure still goes through these helpers** — it just ignores the
error instead of returning it. `liveValue` in `src/domains/ax-tree.ts` returns `undefined` so the
caller falls back to the accessibility-tree value, and `snapshot`'s optional-screenshot block
simply omits the path. Both call `cdpCall` and discard the error. Reaching past the helper to
suppress an error is never the reason to do it.

One honest gap: `session.attach()` and `session.windowId()` are session methods that do perform a
CDP call, and `domains/navigate.ts` calls `attach` directly. The scanner's rules match `call*` and
`evaluateJs`, not these two.

---

## 3. Keeping the type safety

### A green `tsc` proves nothing about unchecked casts

`as { … }` between structurally compatible object types compiles **silently**. At one point this
codebase held 33 such casts and produced only 6 compile errors when they were removed. Strict
flags do not see them.

Likewise, **`JSON.parse` returns `any`, so an annotated assignment is an invisible cast.**
`let msg: CdpRawMessage = JSON.parse(raw)` typechecked cleanly in `src/daemon/bridge.ts` while
being completely unguarded — a `null` frame from Chrome would have thrown inside a `ws` handler
with no `try`/`catch` above it and killed the daemon. That is why `src/schemas/parse.ts` exists:
`parseJson(raw, validator)` parses to `unknown`, validates, and returns a `Result`. Every parse
boundary goes through it or an equivalent guard.

### The boundary scanner

`npm run boundaries` runs `scripts/check-boundaries.ts` over every `.ts` file under `src/` and
fails the build on five line-level patterns:

| Rule | Matches |
|---|---|
| `as-any` | `as any` |
| `ts-ignore` | `@ts-ignore`, `@ts-expect-error`, `@ts-nocheck` |
| `non-null-assertion` | `x!.`, `x!(`, `x!;` … |
| `inline-object-cast` | `as {` |
| `double-cast` | `as unknown as` |

Exemptions live in an **external allowlist**, `scripts/boundary-allowlist.ts`, as
`{ path, rule, count, reason }` entries. The `count` is a budget, not a flag: a *second*
violation of the same rule in an already-exempt file still fails the build.

The allowlist is deliberately external rather than a comment marker in the source, because the
zero-comments policy would eventually delete an in-source marker and turn CI red for an
unrecognisable reason. `unusedExemptions` also reports an entry whose code is gone, so the list
cannot rot.

**Limitations, stated rather than implied:** the scan is **line-based**, so a cast split across
two lines evades every rule. And it only walks `src/` — `test/` and `scripts/` are unscanned.

### The five surviving assertions in `src/`

All five are boundary assertions where a runtime check has already established the shape and
TypeScript cannot express the correlation.

| Location | Why |
|---|---|
| `decodeResult` in `src/cdp/commands.ts` | `validate.Check(raw)` proves `raw` matches `COMMANDS[method].result`; TS cannot correlate a generic key to its mapped value |
| `decodeEvent` in `src/cdp/events.ts` | same, for `EVENTS[name].params` |
| `loadSharp` in `src/util/sharp-shim.ts` (probe for `.default`) | the module comes from a dynamic import of an optional dependency, so its shape is unknown at compile time; the cast only claims an optional `.default`, and every value read off it is re-checked with `typeof` |
| `loadSharp` in `src/util/sharp-shim.ts` (`candidate` to `SharpFactory`) | `typeof candidate === "function"` was checked immediately above |
| `registerBrowserTool` in `src/util/tool.ts` (`renderCall` re-widening) | `AnyBrowserToolDefinition` is type-erased so the registry can hold a heterogeneous array; `defineBrowserTool` guarantees handler, parameters, and `renderCall` were built from the same `S` |

Only the `sharp-shim.ts` `.default` probe needs an allowlist entry, because it is the one written
as `as { … }`. The other four are `as SomeNamedType`, which no scanner rule matches. **Do not add
allowlist entries for them** — they are not required, and `unusedExemptions` would flag them.

---

## 4. The tool authoring contract

Nine rules. Every `domains/*` file follows them.

| # | Rule | Exemplar |
|---|---|---|
| 1 | **One domain per file, exporting only `<name>Tool` consts.** Anything a second file needs goes in a helper-only module beside it (`ax-tree.ts`, `element-call.ts`, `screenshot-capture.ts`, …) — never exported from a tool file. Past roughly 250 lines, split by capability. | `src/domains/history.ts` |
| 2 | **Args schema at the top of the file**, named `<Name>Args`, built with typebox, with a `description` on every field. Those strings are the agent's only documentation of the tool. | `src/domains/keyboard.ts` (`TypeArgs`, `PressKeyArgs`) |
| 3 | **`defineBrowserTool({ … })` holds all metadata, including `concurrency`.** A tool's concurrency class must not live in a different file from the tool. | every domain file |
| 4 | **Fixed handler signature** `(args, ctx) => Promise<Result<ToolOk, ToolErr>>`. Never throw, never `console.log`, never touch `process`. Errors are values. | `src/util/tool.ts` (`ToolHandler`) |
| 5 | **Reach Chrome only via `ctx.client`.** No domain imports the transport. | see section 1 |
| 6 | **Element targeting goes through the shared `resolveTarget(client, args)`** in `src/domains/target.ts` — never a hand-rolled ref-vs-x/y branch. | `src/domains/click.ts` |
| 7 | **CDP calls go through `src/domains/cdp-call.ts`** — `cdpCall`, `cdpCallOnTarget`, `cdpCallBrowser`, `evalJs`. Machine-enforced by the `raw-cdp-call` and `raw-evaluate` scanner rules. | `src/domains/history.ts` |
| 8 | **Naming:** tool name `browser_<verb>`, `label` title-case, `promptSnippet` one line, `promptGuidelines` at most six agent-facing bullets. | pinned by `test/domains/registry-test.ts` |
| 9 | **Every tool has a test** covering its schema and its error paths, written against a fake client so no browser is needed. | `test/domains/form-errors-test.ts` |

Rule 1's two standing exceptions are largely settled: `snapshot.ts` went from 477 lines to 256
once the tree model moved to `ax-tree.ts`, and `form.ts` from 409 to 283 once the element-call
machinery moved to `element-call.ts`. Both sit at the guidance rather than far past it. Neither
is licence to add a third.

Rule 6 currently binds only `src/domains/click.ts`, because it is the sole tool taking
*either* a ref *or* coordinates. `drag.ts` takes explicit coordinates only and `scroll.ts`
defaults to the viewport centre; neither resolves refs. Any *new* ref-or-coords tool must use
`resolveTarget`.

Rules 8 and 9 are partly machine-checked. `test/domains/registry-test.ts` asserts every tool
declares a concurrency class, that names are unique, that every name is `browser_`-prefixed, and
that the set of `serialized` tools is **exactly** a pinned list — so adding a serialized tool
means adding its name to `SERIALIZED` in that file.

---

## 5. How to add a tool

`src/domains/history.ts` (80 lines, three tools) is the whole pattern in one file. A minimal
tool, abridged from `reloadTool` in that file — the real one also branches on an open JS dialog:

```ts
import { Type } from "typebox";
import { type Result, err, ok } from "../util/result";
import { defineBrowserTool, type ToolErr, type ToolOk } from "../util/tool";
import { cdpCall } from "./cdp-call";

export const reloadTool = defineBrowserTool({
  name: "browser_reload",
  label: "Browser Reload",
  description: "Reload the current page.",
  promptSnippet: "Reload the page",
  promptGuidelines: ["Use to refresh the page, e.g. after server-side changes."],
  parameters: Type.Object({}),
  concurrency: "serialized",
  async handler(_a, { client }): Promise<Result<ToolOk, ToolErr>> {
    const r = await cdpCall(client, "Page.reload", {});
    if (!r.success) return r;
    const info = await client.pageInfo();
    if (!info.success) return err({ kind: "cdp_error", message: info.error.message });
    return ok({ text: `Reloaded: ${info.data.title}`, details: { page: info.data } });
  },
});
```

Note `if (!r.success) return r;` — a failed `cdpCall` is already a `Result<_, ToolErr>`, so it
returns unchanged. No re-wrapping.

Steps:

1. Create `src/domains/foo.ts`. Declare `FooArgs` at the top with a `description` on every field.
2. If the CDP method you need is not in `COMMANDS`, add a table entry first (section 2).
3. Call `defineBrowserTool` with an explicit `concurrency`. Pick `serialized` if the tool mutates
   page or session state; `parallel` only for pure reads.
4. Add the export to `ALL_TOOLS` in `src/registry.ts`.
5. If it is `serialized`, add its name to `SERIALIZED` in `test/domains/registry-test.ts`.
6. Write `test/domains/foo-test.ts`.

### Testing a handler

Use **`createFakeClient()`** from `test/domains/fake-client.ts`. It builds a real `CdpSession`
over a **stub transport** — canned per-method results, recorded calls, and an injectable event
stream:

```ts
const ctxFor = (fake: FakeClient): HandlerContext => ({
  client: fake.client,
  signal: undefined,
  onUpdate: () => {},
  extensionCtx: undefined as never,
});

const fake = await createFakeClient({
  refs: [{ ref: "e1", backendId: 101 }],
  canned: {
    "DOM.resolveNode": ok({ object: { type: "object", objectId: "obj-1" } }),
    "Runtime.callFunctionOn": ok({
      result: { type: "undefined" },
      exceptionDetails: { text: "Uncaught TypeError: el.dispatchEvent is not a function" },
    }),
  },
});
const r = await fillTool.handler({ ref: "e1", value: "x" }, ctxFor(fake));
assert.equal(r.success, false);
if (r.success) return;
assert.equal(r.error.kind, "cdp_error");
assert.match(r.error.message, /dispatchEvent is not a function/);
```

`canned` maps a CDP method to a `Result` (or a queue of them, consumed in order); `refs` seeds the
session's ref map so a `ref`-taking tool resolves without a snapshot; `evaluate` stubs
`client.evaluateJs`. `fake.calls` / `fake.callsTo(method)` are the recorded requests, and
`fake.emit(event)` injects a CDP event. `test/domains/form-errors-test.ts` is the reference usage.

**Do not mock the `BrowserClient` instead.** A mocked client bypasses the real session, the real
decoders, and the real error mapping, which produces a test that cannot fail. Two tasks on this
refactor shipped exactly that before `createFakeClient` existed.

It also exports `createStubTransport` (for session-level tests) and `axNode` (for
accessibility-tree fixtures).

---

## 6. Conventions that look like mistakes

Each of these has been, or would be, "fixed" by someone who did not know why it was written
that way.

**`src/util/tool.ts` gates the lock on `if (def.concurrency !== "parallel")`, not
`=== "serialized"`.** Deliberate fail-closed default: an unrecognised or missing concurrency
value **over-serialises** rather than racing on shared CDP session and page state. A test in
`test/domains/registry-test.ts` ("a definition with no concurrency class takes the mutation
mutex") pins it.

**`src/util/sharp-shim.ts` routes its dynamic import through
`const SHARP_SPECIFIER: string = "sharp"`** so TypeScript cannot resolve the specifier
statically. `sharp` is an **optional** dependency. **Anyone who "simplifies" this back to a
static `import("sharp")` breaks the build wherever sharp is absent.** This is the single most
likely thing in the codebase to be broken by a well-meaning cleanup.

**`npm test` passes `--conditions=import`.** A test can import a tool definition *only* because
of it: `@mariozechner/pi-coding-agent` publishes no `"require"` condition, and this repo is not
`"type": "module"`, so tsx would otherwise compile `src/` to CJS and the specifier would be
unresolvable. It also *matches* production — pi loads the extension with jiti and aliases that
package to `dist/index.js` by absolute path, which is what the `import` condition selects.

**The test glob is `test/{profile,domains,cdp,daemon,util}/**/*-test.ts`.** A test placed
in any other directory — `test/schemas/`, say — **silently never runs**, so adding a directory
means adding it here too. `test/manual/` is excluded on purpose: those need live Chrome.

**`Box` (`src/domains/box.ts`) has six fields — `x, y, width, height, cx, cy` — and all six are
consumed.** `cx`/`cy` are the click point. `width`/`height` reach the model through
`browser_snapshot` with `format: "json"`, which serialises each node's `box` wholesale.
Narrowing the type would silently change tool output.

---

## 7. The failure mode that cost the most rework

**Adopting a type guard where a loose truthiness check lived can invert a safety default.**

`if (guard(x)) { if (!x.ok) fail }` is **not** equivalent to `if (!x.ok) fail`. With an unchecked
cast, a malformed payload left the field `undefined`, so `!field` was **true** and the failure
path fired. Fold the guard into the boolean gate and a rejected shape now **skips the check
entirely** — the code proceeds in exactly the case where it knows least.

The structural tell, verified across six sites:

- **SAFE** — the guard produces a **value** that inherits a pre-existing failure path:
  ```ts
  const v = guard(x) ? x : undefined;
  if (v === undefined || !v.ok) return err(…);
  ```
- **UNSAFE** — the guard is **ANDed into a boolean gate**:
  ```ts
  if (ok && guard(x)) { … }
  ```

`typeTool` in `src/domains/keyboard.ts` has both halves within six lines. The inner line is the
safe form — `isFocusProbe(focused.data) ? focused.data : { ok: false, tag: null }` turns a bad
shape into a **value** meaning "not focused", which the existing `if (!f.ok)` then rejects. The
outer `if (focused.success)` is the same mistake in its other guise: a gate, so when the focus
probe itself fails the entire check is skipped and the tool types anyway. That is a known open bug
(F9 in the bug audit), left in place here as the clearest illustration in the tree.

Rule of thumb: **a check must default to the failure path when its input cannot be understood.**
If adding a guard moved a case from "fails loudly" to "not checked", the guard is in the wrong
place.

---

## 8. Running the checks

```bash
npm run check     # typecheck + boundaries + test
```

- `npm run typecheck` — `tsc --noEmit` over `src/`, then `tsc -p tsconfig.test.json` over
  `src/`, `test/`, and `scripts/`. Flags live in `tsconfig.base.json`; the notable ones beyond
  `strict` are `noUncheckedIndexedAccess`, `exactOptionalPropertyTypes`,
  `noPropertyAccessFromIndexSignature`, and `verbatimModuleSyntax`.
- `npm run boundaries` — the cast scanner (section 3).
- `npm test` — `node:test` over the glob above.

`npm run check` must be green before a PR.