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/ARCHITECTURE.md | |
| 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/ARCHITECTURE.md')
| -rw-r--r-- | docs/ARCHITECTURE.md | 429 |
1 files changed, 0 insertions, 429 deletions
diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md deleted file mode 100644 index 92ea16f..0000000 --- a/docs/ARCHITECTURE.md +++ /dev/null @@ -1,429 +0,0 @@ -# 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 - (CdpTransport is declared in cdp/types.ts) - | -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`, `truncate`, `paths`, `js-template`, `sharp-shim`, `debug`) | 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/target-factory`, -the buffers, and `cdp/session` for the `CdpSession` type and `evaluateJson`). - -**It must never import from `daemon/` 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. |
