diff options
Diffstat (limited to 'docs/superpowers/specs/2026-07-26-codebase-refactor-design.md')
| -rw-r--r-- | docs/superpowers/specs/2026-07-26-codebase-refactor-design.md | 264 |
1 files changed, 0 insertions, 264 deletions
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/`. |