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/superpowers/specs/2026-07-26-codebase-refactor-design.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/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/`. |
