docs: add codebase simplification spec

2026-04-11 04:47:01 +09:00
parent e88073f2f8
commit f10833e818
1 changed files with 741 additions and 0 deletions
--- a/docs/codebase-simplification-quality-improvement-spec.md
+++ b/docs/codebase-simplification-quality-improvement-spec.md
@@ -0,0 +1,741 @@
+# EJClaw Codebase Simplification & Quality Improvement Spec
+
+**Status:** Proposed
+**Audience:** Maintainers / developers implementing cleanup work
+**Language:** English
+**Review basis:** Static repository review on 2026-04-10. This review did **not** execute Bun builds or tests in the analysis environment, so all behavior changes must be validated after the quality gate is fixed.
+
+## 1. Objective
+
+Raise codebase quality by **removing unnecessary complexity**, not by adding new architecture.
+
+The target end state is:
+
+- one trustworthy quality gate,
+- one source of truth per concept,
+- less legacy compatibility surface,
+- fewer hidden global states,
+- smaller lifecycle-oriented modules in the few places where complexity is truly concentrated,
+- no speculative abstractions.
+
+This spec is intentionally biased toward **simple implementation**. It is not a request for a rewrite.
+
+## 2. Guiding Rules
+
+1. **Prefer deletion over abstraction.** If a new abstraction does not remove real duplication or real branching, do not add it.
+2. **Prefer one real shared module over “keep in sync” comments.** Manual SSOT is not SSOT.
+3. **Prefer plain TypeScript modules and small factories over frameworks.** No DI container, no service locator, no “clean architecture” ceremony.
+4. **Keep the existing single-service + file-IPC shape unless a change clearly reduces complexity.** Do not introduce Redis, sockets, queues, or microservices as part of this cleanup.
+5. **Refactor only the hotspots.** Do not split stable small modules just to reduce file count.
+6. **Quality gate first.** Until CI reflects reality, all other cleanup work is lower-confidence.
+7. **Do not re-open runtime compatibility paths that were already intentionally removed.** Finish deletion instead.
+
+## 3. Executive Summary
+
+The codebase is not primarily suffering from “too little architecture.” It is suffering from:
+
+- a **non-trustworthy quality gate**,
+- a handful of **stateful orchestration hotspots** that grew too large,
+- **manual protocol duplication** between host and runners,
+- **unfinished legacy compatibility cleanup** in setup/tooling/test paths,
+- **hidden mutable module state**,
+- and a **giant schema/migration surface** that is too hard to reason about safely.
+
+There are also several signs that the code is already moving in the right direction and should **not** be churned unnecessarily:
+
+- `src/message-runtime-*` is already split by responsibility.
+- `src/provider-retry.ts` is a good example of extracting real duplication.
+- `src/paired-execution-context-shared.ts` is already acting as a central status-transition guard.
+- `setup/register.ts` is already much simpler than older compatibility assumptions would suggest.
+
+So the right move is **targeted simplification**, not broad reorganization.
+
+## 4. Current Findings
+
+### 4.1 The quality gate is currently unreliable
+
+The repository declares Bun as the package manager:
+
+- root `package.json`: `"packageManager": "bun@1.3.11"`
+- runner packages also declare Bun
+- repository contains `bun.lock`
+
+But important automation still uses npm:
+
+- `.github/workflows/ci.yml` uses `npm ci`, `npm run format:check`, and `npx ...`
+- `.husky/pre-commit` runs `npm run format:fix`
+
+This means the repo’s actual development/runtime toolchain and its merge gate are not aligned.
+
+### 4.2 Format/type/build coverage is incomplete
+
+Current root scripts only format `src/**/*.ts`.
+
+That means formatting does **not** cover, at minimum:
+
+- `setup/**/*.ts`
+- `runners/**/*.ts`
+- `test/**/*.ts`
+- `scripts/**/*.ts`
+- `vitest*.ts`
+
+Current root `tsconfig.json` only includes `src/**/*`, so root typecheck does **not** cover:
+
+- `setup/**/*`
+- `runners/**/*`
+- top-level scripts/config files outside `src`
+
+CI also does not build the runners as part of the default merge gate.
+
+### 4.3 Complexity is concentrated in a small number of files
+
+Approximate non-test LOC hotspots:
+
+| File | Approx. LOC | Problem shape | Required response |
+| --- | ---: | --- | --- |
+| `src/db/schema.ts` | 3156 | Giant imperative migration surface | Break into versioned migrations |
+| `runners/agent-runner/src/index.ts` | 1170 | Multiple unrelated responsibilities in one entry file | Split by lifecycle boundary |
+| `src/group-queue.ts` | 1060 | Stateful queue/process orchestration hotspot | Split state model vs scheduler vs process lifecycle |
+| `src/message-agent-executor.ts` | 1031 | Target resolution, execution, retry/failover, delivery are mixed | Split into focused executor pieces |
+| `src/db.ts` | 970 | God facade / singleton entry to many DB domains | Freeze growth and narrow usage |
+| `src/ipc.ts` | 854 | File claiming, routing, watcher loop, task IPC all mixed | Split IPC concerns without changing architecture |
+| `src/db/paired-state.ts` | 853 | Large paired-task data surface | Keep domain-specific, but stop routing everything through `db.ts` |
+| `src/channels/discord.ts` | 853 | Channel implementation likely doing too much in one place | Reduce hot-path sync work and isolate channel concerns |
+| `src/paired-workspace-manager.ts` | 850 | Workspace lifecycle complexity | Keep, but isolate expensive filesystem work |
+| `src/unified-dashboard.ts` | 761 | Large UI/render orchestration | Lower priority; simplify only after core runtime |
+| `src/message-turn-controller.ts` | 747 | Turn orchestration complexity | Lower priority than protocol/IPC/executor |
+| `runners/agent-runner/src/ipc-mcp-stdio.ts` | 696 | Mixed protocol/runtime concerns | Simplify after shared protocol extraction |
+| `src/agent-runner-environment.ts` | 674 | Environment/bootstrap logic with heavy sync IO | Keep bootstrap-oriented, but isolate and simplify |
+| `src/task-scheduler.ts` | 612 | Scheduling loop + execution + state in one module | Extract scheduler runtime state + execution unit |
+| `src/message-runtime.ts` | 588 | Orchestration wrapper, but already partially split | Touch only if still needed after other changes |
+| `setup/legacy-room-registrations.ts` | 545 | Legacy compatibility logic still alive | Keep temporary or delete after migration completion |
+
+Important nuance: the answer is **not** “split everything.” The answer is to split only the files where multiple lifecycles or domains are still mixed together.
+
+### 4.4 Host ↔ runner protocol is duplicated manually
+
+`src/agent-protocol.ts` is explicitly documented as the SSOT for host ↔ runner constants, but runners keep local copies and comments such as “keep in sync”.
+
+Examples duplicated in runner entry files:
+
+- output markers
+- image tag regex
+- IPC poll interval
+- structured output normalization logic
+
+This is exactly the kind of complexity that should be removed, not documented.
+
+### 4.5 Legacy runtime cleanup is already mostly done — finish it, do not reverse it
+
+The runtime now explicitly rejects unexpected legacy room-binding state during database initialization, e.g. in `src/db/database-lifecycle.ts`.
+
+That is the correct direction.
+
+However, a significant compatibility surface still exists in setup/migration/test code, especially around:
+
+- `setup/legacy-room-registrations.ts`
+- `setup/migrate-room-registrations.ts`
+- `setup/room-registration-state.ts`
+- `setup/environment.ts`
+- several setup tests that still focus on legacy scenarios
+- broad runtime carrier types like `RegisteredGroup` that still mix room-binding data with wider runtime usage
+
+The cleanup goal should be:
+
+- keep runtime strict,
+- provide an explicit migration/backfill path,
+- then delete the remaining compatibility layer.
+
+### 4.6 Hidden mutable module state is still common
+
+Examples:
+
+- `src/service-routing.ts` keeps lease cache and global failover state in module globals.
+- `src/task-scheduler.ts` keeps scheduler state in module globals.
+- `src/ipc.ts` keeps watcher state in module globals.
+- token rotation modules also keep module-level mutable state.
+
+This is not a reason to add a framework. It is a reason to wrap state in **small factory-created runtime objects**.
+
+### 4.7 Config/env access is not fully centralized
+
+There are still many direct `process.env` reads outside a clear config boundary (at least dozens in production code, with the heaviest concentration in runner entrypoints and environment/bootstrap files).
+
+That makes behavior harder to audit, test, and reason about.
+
+### 4.8 The database/migration story is too large for safe iteration
+
+`src/db/base-schema.ts` and especially `src/db/schema.ts` represent a very large schema/migration surface.
+
+The current style makes it too easy to:
+
+- add one more conditional migration branch,
+- accidentally couple unrelated schema changes,
+- and make future reviews expensive.
+
+This is a good candidate for simplification through **versioned migrations**, not through an ORM rewrite.
+
+### 4.9 Some runtime registration is still “magic”
+
+`src/channels/index.ts` uses side-effect imports for channel registration.
+
+This is not a catastrophic issue, but it is unnecessary indirection for a codebase that is trying to become simpler.
+
+### 4.10 Test volume is high, but test focus is mixed
+
+Approximate TypeScript volume:
+
+- production TS: ~40.8k LOC
+- test TS: ~38.1k LOC
+
+The problem is not “too many tests.” The problem is that some tests still spend significant surface area preserving behavior that should be deleted.
+
+## 5. Mandatory Workstreams
+
+### 5.1 Workstream A — Fix the quality gate first
+
+### Required changes
+
+1. Replace npm-based CI/hook paths with Bun-based ones.
+2. Add **one** canonical root command for merge validation.
+3. Expand format/type/build coverage to all real code paths.
+4. Ensure runners are part of the gate.
+
+### Minimum script shape
+
+The exact script names can vary, but the repository should end with something equivalent to:
+
+```json
+{
+  "scripts": {
+    "format:check": "prettier --check \"{src,setup,runners,shared,test,scripts}/**/*.{ts,js}\" \"vitest*.ts\"",
+    "format:fix": "prettier --write \"{src,setup,runners,shared,test,scripts}/**/*.{ts,js}\" \"vitest*.ts\"",
+    "typecheck:all": "bunx tsc --noEmit -p tsconfig.check.json && bunx tsc --noEmit -p runners/shared/tsconfig.json && bunx tsc --noEmit -p runners/agent-runner/tsconfig.json && bunx tsc --noEmit -p runners/codex-runner/tsconfig.json",
+    "build:all": "bun run build && bun run --cwd runners/shared build && bun run build:runners",
+    "check": "bun run format:check && bun run typecheck:all && bun run test && bun run build:all"
+  }
+}
+```
+
+If the root build `tsconfig.json` currently assumes `rootDir=src` / `outDir=dist`, prefer a separate `tsconfig.check.json` for broad no-emit validation rather than widening the build config itself.
+
+### CI requirement
+
+CI should reduce to one truthful flow:
+
+1. install with Bun,
+2. run `bun run check`.
+
+Do **not** keep parallel npm and Bun paths unless there is a documented hard requirement.
+
+### Pre-commit requirement
+
+`.husky/pre-commit` must use Bun, not npm.
+
+### Acceptance criteria
+
+- A fresh checkout passes locally and in CI with the same top-level command.
+- Setup code, runners, and tests are covered by formatting and typecheck.
+- Runner builds are part of the merge gate.
+- There is no npm-vs-Bun ambiguity in routine contributor workflow.
+- After this lands, every follow-up cleanup PR must pass the same canonical `bun run check` gate before merge.
+
+### 5.2 Workstream B — Finish deleting remaining legacy compatibility
+
+### Required changes
+
+1. Treat the runtime’s current strictness as intentional and correct.
+2. Keep migration helpers only as **explicit migration tooling**, not as long-term compatibility logic.
+3. Narrow broad runtime carrier types after migration completion so room-binding-specific fields stop leaking across unrelated runtime paths.
+4. Move any remaining legacy behavior coverage into a narrowly-scoped migration suite.
+
+### Files in scope
+
+Primary:
+
+- `setup/legacy-room-registrations.ts`
+- `setup/migrate-room-registrations.ts`
+- `setup/room-registration-state.ts`
+- `setup/environment.ts`
+- `setup/verify.ts`
+- legacy-heavy setup tests
+
+Secondary:
+
+- `src/types.ts` (`RegisteredGroup` usage and shape leakage)
+- any code path still branching on legacy room-registration semantics
+
+### Important rule
+
+Do **not** reintroduce runtime fallback behavior for legacy room registrations. The runtime should remain strict. Migration should happen before startup, not silently during runtime.
+
+### Acceptance criteria
+
+- No runtime code path depends on legacy room-registration tables/files.
+- Legacy-only fields are removed, and room-binding-specific fields no longer need to travel through umbrella runtime types where they are unrelated.
+- Legacy behavior is covered only in dedicated migration tests/tools, not general runtime logic.
+- Setup verification reports current-state issues clearly without acting as a permanent compatibility layer.
+
+### 5.3 Workstream C — Unify the host ↔ runner protocol and reviewer runtime policy
+
+### Required changes
+
+1. Extend the existing `runners/shared` package as the default shared boundary for host ↔ runner protocol constants/types/helpers unless a narrower shared module is clearly simpler.
+2. Delete manual copies in runner entrypoints.
+3. Centralize reviewer runtime capability differences.
+4. Make capability gaps explicit in code, not hidden in comments.
+
+### Protocol items that must be shared for real
+
+At minimum:
+
+- output start/end markers,
+- image tag regex,
+- IPC polling constants,
+- input/output envelope types,
+- structured output normalization/parsing helpers.
+
+### Reviewer runtime policy
+
+Current reviewer policy is split awkwardly across runner-specific files. Codex limitations are documented in comments rather than represented as explicit capability rules.
+
+Replace that with a small capability model, e.g.:
+
+- supports shell preflight hook,
+- supports readonly sandboxing,
+- supports git-write guard only,
+- supports hard mutation blocking.
+
+This must remain simple. A plain exported object or discriminated union is enough.
+
+The capability model should live with the shared runner boundary by default. If host code needs awareness of those differences, expose a thin serialized capability surface instead of making `src/` depend deeply on runner-only internals.
+
+### Files in scope
+
+- `src/agent-protocol.ts`
+- `runners/agent-runner/src/index.ts`
+- `runners/codex-runner/src/index.ts`
+- `runners/agent-runner/src/reviewer-runtime.ts`
+- `runners/codex-runner/src/reviewer-runtime.ts`
+- existing shared runner package (`runners/shared/...`)
+
+### Acceptance criteria
+
+- No protocol constant is duplicated manually between host and runners.
+- No “keep in sync” comments remain for protocol items.
+- Reviewer capability differences are represented in one place.
+- Both runners consume the same protocol/policy definitions where applicable.
+
+### 5.4 Workstream D — Finish config/env centralization and remove hidden mutable state
+
+### Required changes
+
+1. Restrict direct `process.env` access to explicit config/bootstrap boundaries.
+2. Create small `loadXConfig()` / `createXRuntime()` entrypoints where needed.
+3. Replace module-global runtime state with plain-object state holders.
+
+### Scope rules
+
+Allowed places for direct env access:
+
+- config loaders,
+- bootstrap files,
+- true process entrypoints,
+- runner startup config loaders.
+
+Not allowed in normal domain logic.
+
+### State wrapping targets
+
+High priority:
+
+- `src/service-routing.ts`
+- `src/task-scheduler.ts`
+- `src/ipc.ts`
+
+Medium priority:
+
+- token rotation modules,
+- other modules with nontrivial mutable singleton state.
+
+### Implementation rule
+
+Use **small factory functions**, not a DI framework.
+
+Good:
+
+```ts
+const serviceRouting = createServiceRoutingRuntime(...)
+```
+
+Bad:
+
+- container registration frameworks,
+- deep dependency graphs introduced only for “purity”,
+- passing giant service bags everywhere.
+
+### Acceptance criteria
+
+- Direct `process.env` reads outside config/bootstrap/entrypoint code are either removed or explicitly justified.
+- Stateful modules can be initialized/reset without relying on hidden module-global mutation.
+- Test setup becomes simpler because state can be created explicitly.
+
+### 5.5 Workstream E — Split only the highest-risk orchestration hotspots
+
+This is the most important “simplify without overengineering” section.
+
+#### 5.5.1 `src/message-agent-executor.ts`
+
+Current file mixes:
+
+- execution target resolution,
+- paired-context preparation,
+- runner execution,
+- retry/session recovery,
+- failover behavior,
+- delivery/handoff side effects.
+
+#### Required split
+
+Keep one coordinator file, but extract:
+
+- **target/context resolution**,
+- **attempt execution**,
+- **failure classification + retry/session recovery**,
+- **delivery/handoff side effects**.
+
+Do not invent a new class hierarchy.
+
+#### 5.5.2 `src/group-queue.ts`
+
+Current file mixes:
+
+- state model,
+- run-phase transitions,
+- concurrency scheduling,
+- process close/termination behavior,
+- retry timing.
+
+#### Required split
+
+Extract into small plain modules:
+
+- group state / transitions,
+- waiting/drain scheduler,
+- process shutdown handling,
+- IPC follow-up helpers if still needed.
+
+Keep `GroupQueue` as the public coordinator if that remains simplest.
+
+#### 5.5.3 `src/ipc.ts`
+
+Current file mixes:
+
+- file claiming/quarantine,
+- inbound message forwarding,
+- task IPC processing,
+- watcher loop lifecycle.
+
+#### Required split
+
+Keep file-based IPC, but isolate:
+
+- file claim/quarantine utilities,
+- message forwarding/authorization,
+- task command handling,
+- watcher runtime.
+
+Do **not** replace this with a new platform.
+
+#### 5.5.4 `runners/agent-runner/src/index.ts`
+
+Current file mixes:
+
+- stdin parsing,
+- output protocol writing,
+- transcript parsing/markdown generation,
+- compact memory logic,
+- IPC polling/draining,
+- main runner lifecycle.
+
+#### Required split
+
+Extract by lifecycle:
+
+- input/output protocol,
+- transcript/memory helpers,
+- IPC polling/drain,
+- runner main.
+
+#### 5.5.5 `src/db.ts`
+
+This file is too large and too central.
+
+#### Required response
+
+- Stop adding new behavior to `src/db.ts`.
+- Treat it as a temporary compatibility facade.
+- New code should import from domain modules under `src/db/*` directly where practical.
+- Over time, shrink `db.ts` instead of growing it.
+
+#### 5.5.6 `src/index.ts`
+
+The entrypoint should become a clearer composition root.
+
+#### Required changes
+
+- Replace side-effect channel registration with explicit startup registration.
+- Keep startup wiring visible in one place.
+- Avoid pushing more domain logic into the entrypoint.
+
+### Important non-targets
+
+- `setup/register.ts` is already reasonably simple. Do not churn it.
+- `src/message-runtime.ts` is already partially decomposed into `message-runtime-*` files. Only trim it further if complexity still leaks after other workstreams land.
+
+### Acceptance criteria
+
+- Hotspot files are reduced because responsibilities were actually separated, not because helper wrappers were added around the same logic.
+- No new framework-style abstraction layer is introduced.
+- Public entrypoints remain obvious and easy to trace.
+
+### 5.6 Workstream F — Simplify persistence and migrations
+
+### Required changes
+
+1. Keep `applyBaseSchema()` only for fresh database creation.
+2. Freeze `src/db/schema.ts` for new schema work except emergency fixes.
+3. Route new schema changes into ordered versioned migrations first, then backfill older conditional branches incrementally.
+4. Make each migration small, isolated, and testable.
+5. Reduce reliance on broad compatibility-shaped types at runtime.
+
+### Suggested shape
+
+- `src/db/migrations/001_*.ts`
+- `src/db/migrations/002_*.ts`
+- etc.
+
+The implementation can be plain TypeScript or SQL files. No ORM is required.
+
+### Type boundary cleanup
+
+`RegisteredGroup` is still carrying room-binding data across too many unrelated paths.
+
+Fields such as `requiresTrigger` may still be canonical room-binding data; the goal is not to delete valid semantics, but to stop leaking them through one umbrella type everywhere.
+
+Introduce narrower runtime boundary types where needed, such as:
+
+- `RoomBinding`
+- `ExecutionLease`
+- `TaskSnapshot`
+
+This is not a request to rewrite all domain types. It is a request to stop overloading one broad type across unrelated paths.
+
+### Status transition rule
+
+All paired-task status writes should continue converging on `src/paired-execution-context-shared.ts`. Do not allow status mutation rules to scatter again.
+
+### Acceptance criteria
+
+- New schema changes are added as small versioned migrations.
+- Fresh DB bootstrap and incremental migration are clearly separated.
+- New runtime code does not need to import a giant umbrella `db.ts` unless there is a strong reason.
+- Canonical runtime types are narrower, and room-binding-only semantics are no longer spread through unrelated runtime surfaces by default.
+
+### 5.7 Workstream G — Reduce hot-path sync I/O without overengineering
+
+There are many synchronous filesystem/process calls across production code, with especially high concentrations in:
+
+- `src/agent-runner-environment.ts`
+- `src/paired-workspace-manager.ts`
+- `runners/agent-runner/src/index.ts`
+- `src/workspace-package-manager.ts`
+- `src/ipc.ts`
+- `src/channels/discord.ts`
+- `src/token-refresh.ts`
+
+### Important rule
+
+This does **not** mean “convert everything to async.”
+
+Synchronous I/O is acceptable in:
+
+- setup commands,
+- one-shot bootstrap,
+- CLI-style preparatory work,
+- failure-path diagnostics.
+
+It is more problematic in:
+
+- repeated polling loops,
+- repeated watcher ticks,
+- per-message hot paths,
+- runner input-drain loops.
+
+### Required changes
+
+1. Identify repeated sync scans/reads/writes in hot loops.
+2. Move hot-path file operations to async equivalents when they execute on every poll/watch tick, per-message hot path, or runner drain iteration, unless there is an explicit documented reason not to.
+3. Keep semantics and architecture simple.
+4. Do not replace file IPC with a new platform.
+
+Minimum decision rule:
+
+- repeated sync `readdirSync` / `statSync` / `readFileSync` / `writeFileSync` / `renameSync` / process probes inside polling loops must be removed or explicitly justified,
+- one-shot bootstrap/setup/failure-path sync calls may stay if they keep the code simpler.
+
+### Initial priority targets
+
+- `src/ipc.ts`
+- `runners/agent-runner/src/index.ts`
+- `src/channels/discord.ts`
+
+### Acceptance criteria
+
+- Repeated poll/watch loops no longer do avoidable synchronous filesystem work on every tick.
+- Setup/bootstrap paths may remain sync where simpler.
+- No new infra dependency is introduced just to avoid sync I/O.
+
+### 5.8 Workstream H — Simplify tests around the current architecture
+
+### Required changes
+
+1. Keep strong coverage, but re-focus it on current behavior.
+2. Move legacy-compat verification into dedicated migration suites.
+3. Remove tests that only preserve behavior the codebase intends to delete.
+4. Reduce fixture duplication with a few shared builders.
+
+### Suggested test buckets
+
+- canonical runtime behavior,
+- migration / legacy import behavior,
+- host ↔ runner protocol,
+- reviewer capability policy,
+- setup orchestration,
+- database migration behavior.
+
+### Acceptance criteria
+
+- Tests document the current architecture, not historical debris.
+- Deleting legacy compatibility also deletes corresponding legacy tests.
+- Protocol/policy behavior is protected with focused tests so duplication does not return.
+
+## 6. File-by-File Action Map
+
+| Priority | File / Area | Action |
+| --- | --- | --- |
+| P0 | `.github/workflows/ci.yml` | Replace npm flow with Bun and one truthful `bun run check` path |
+| P0 | `.husky/pre-commit` | Switch to Bun |
+| P0 | `package.json` | Add `check`, broaden format/type/build coverage |
+| P0 | `tsconfig.check.json` / new `typecheck:all` path | Ensure setup/runners/shared are typechecked without widening the build tsconfig |
+| P1 | `src/agent-protocol.ts` + runners/shared + runners | Move to a real shared import boundary using the existing shared runner package |
+| P1 | runner reviewer runtime files | Centralize reviewer capability policy |
+| P1 | `setup/legacy-room-registrations.ts` and related setup files | Limit to explicit migration tooling, then delete as possible |
+| P1 | `src/types.ts` | Narrow `RegisteredGroup` leakage and introduce more focused runtime boundary types |
+| P1 | `src/message-agent-executor.ts` | Split by execution lifecycle |
+| P1 | `src/group-queue.ts` | Split state/transitions from scheduling and shutdown |
+| P1 | `src/ipc.ts` | Split watcher/claiming/forwarding/task IPC |
+| P1 | `runners/agent-runner/src/index.ts` | Split IO protocol, transcript/memory, IPC poll, main |
+| P2 | `src/db/schema.ts` | Freeze new growth and shift schema changes to versioned migrations |
+| P2 | `src/db.ts` | Freeze growth; move new code to domain modules |
+| P2 | `src/service-routing.ts` | Wrap global state in runtime object |
+| P2 | `src/task-scheduler.ts` | Wrap scheduler state in runtime object |
+| P2 | `src/index.ts` + `src/channels/index.ts` | Replace side-effect registration with explicit registration |
+| P3 | `src/channels/discord.ts` | Trim hot-path sync IO and reduce mixed responsibilities |
+| P3 | `src/unified-dashboard.ts` | Simplify only after runtime/core cleanup |
+| P3 | `src/message-turn-controller.ts` | Revisit after protocol/executor cleanup |
+
+## 7. Recommended Implementation Order
+
+Do **not** attempt this as one giant PR.
+
+### PR 1 — Trustworthy quality gate
+
+- Bun-only CI/hook flow
+- broaden formatting/typecheck/build coverage
+- add one `check` command
+- establish the gate that every later cleanup PR must continue to pass
+
+### PR 2 — Real shared protocol + reviewer capability model
+
+- remove manual protocol duplication
+- extend `runners/shared` instead of creating a second shared boundary
+- centralize reviewer capability logic there
+- add focused protocol/policy tests
+
+### PR 3 — Legacy cleanup completion
+
+- narrow setup migration tooling
+- remove legacy compatibility branches where migration is complete
+- narrow `RegisteredGroup` leakage instead of deleting valid room-binding semantics blindly
+- move remaining legacy tests into migration-only coverage
+
+After PR 1, PR 2 and PR 3 can proceed in parallel if team bandwidth allows. They are logically related, but neither should depend on the other to start.
+
+### PR 4 — Hotspot splits
+
+- `message-agent-executor.ts`
+- `group-queue.ts`
+- `ipc.ts`
+- `runners/agent-runner/src/index.ts`
+
+### PR 5 — Persistence simplification
+
+- freeze `src/db/schema.ts` growth
+- route new work to versioned migrations first
+- stop growing `db.ts`
+- introduce narrower runtime DTOs where needed
+
+### PR 6 — Follow-up cleanup
+
+- service-routing/task-scheduler runtime state factories
+- explicit channel registration
+- lower-priority large-file cleanup
+
+## 8. Definition of Done
+
+This effort is done when all of the following are true:
+
+1. CI, local checks, and pre-commit all use the same toolchain and the same expectations.
+2. There is a single truthful validation command for contributors.
+3. Host ↔ runner protocol is defined in one real shared module.
+4. Reviewer runtime capability differences are encoded once, not in comments or duplicated conditionals.
+5. Legacy room-registration compatibility is no longer part of normal runtime logic.
+6. The worst orchestration hotspots are split by lifecycle, without introducing framework complexity.
+7. Database migration changes are versioned and small.
+8. Hidden mutable module state is reduced in the main runtime hotspots.
+9. Hot-path sync I/O is reduced where it actually matters.
+10. Tests primarily protect the current system, not deprecated behavior.
+
+## 9. Explicit Non-Goals
+
+The following are **not** part of this spec:
+
+- rewriting the app into microservices,
+- replacing file-based IPC with Redis/sockets/queues,
+- introducing a DI container,
+- introducing Nx/Turborepo or other heavy build orchestration just for cleanup,
+- converting the persistence layer to an ORM,
+- broad “clean architecture” layering,
+- mass file splitting with no behavioral simplification,
+- refactoring already-simple modules just to satisfy style preferences.
+
+## 10. Final Direction
+
+The implementation principle for this cleanup is simple:
+
+> **Delete duplication. Delete compatibility branches. Isolate the few real hotspots. Keep the architecture understandable.**
+
+A successful implementation should make the codebase feel **smaller**, not more “enterprisey”.
+
+## 11. Companion Notes
+
+There is already a detailed legacy-focused document in:
+
+- `docs/legacy-compat-removal-spec.md`
+
+That document can still be used as historical context, but implementation should follow the **current-state rules in this spec**, especially:
+
+- do not reintroduce runtime compatibility,
+- prefer deletion over accommodation,
+- and keep the cleanup implementation as plain and direct as possible.