mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
docs: archive stale findings docs (work completed)
Both findings docs described a codebase that no longer exists — their
headline 'Critical'/'P0' items (server.ts/app.js/types.ts splits, the
{WORKING_DIR} placeholder bug) are all resolved. Moved to docs/archive/
with dated banners so they read as history, not a live TODO.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,111 @@
|
||||
> **⚠️ ARCHIVED 2026-05-21 — superseded, kept for history.**
|
||||
> The headline items here were verified resolved: the P0 `{WORKING_DIR}` placeholder
|
||||
> is now replaced (`plan-orchestrator.ts:431`), and the "~66 dead functions in app.js"
|
||||
> are gone (app.js was modularized 15K→3K LOC). A fresh `npm run knip` sweep on
|
||||
> 2026-05-21 found only a handful of unused test helpers. Do not treat this as a live TODO.
|
||||
|
||||
# Codebase Cleanup Findings
|
||||
|
||||
Compiled from parallel analysis of the entire Codeman codebase by 3 research agents (2026-02-19).
|
||||
|
||||
## P0 — Bug Fix
|
||||
|
||||
### 1. `{WORKING_DIR}` placeholder never replaced in plan-orchestrator.ts
|
||||
- **File:** `src/plan-orchestrator.ts:409`
|
||||
- `RESEARCH_AGENT_PROMPT` has `{WORKING_DIR}` placeholder but only `{TASK}` is replaced
|
||||
- The literal string `{WORKING_DIR}` gets sent to the AI model
|
||||
- **Fix:** Add `.replace('{WORKING_DIR}', this.workingDir)` after the `{TASK}` replacement
|
||||
|
||||
## P1 — Dead Code Removal (High Impact)
|
||||
|
||||
### 2. ~66 dead functions in app.js
|
||||
- Functions never called: `clearAll()`, `toggleSubagentDropdown()`, `goHome()`, `showRalphWizard()`, `minimizeRalphWizard()`, `restoreRalphWizard()`, `ralphWizardNext()`, `ralphWizardBack()`, `skipPlanGeneration()`, `regeneratePlan()`, `incrementTabCount()`, `decrementTabCount()`, `incrementShellCount()`, `decrementShellCount()`, `stopClaude()`, and ~50 more
|
||||
- Many are remnants of abandoned features (Ralph wizard, plan version history)
|
||||
- **Estimated savings:** 300-500 lines
|
||||
|
||||
### 3. ~74 dead CSS selectors in styles.css
|
||||
- Major dead blocks: Task Panel System (`.task-panel`), Process Panel System (`.process-panel`), Monitor Tabs (`.monitor-tabs`), Ralph Metadata (`.ralph-progress-section`, `.ralph-meta`), Plan Editor Toolbar, Plan Version History
|
||||
- Plus ~30 minor unused utility/component selectors
|
||||
- **Estimated savings:** ~400 lines
|
||||
|
||||
### 4. 13 dead type definitions in types.ts (~150 lines)
|
||||
- Dead request interfaces (superseded by Zod schemas): `CreateSessionRequest`, `RunPromptRequest`, `SessionInputRequest`, `ResizeRequest`, `CreateCaseRequest`, `QuickStartRequest`, `CreateScheduledRunRequest`, `QuickRunRequest`, `HookEventRequest`
|
||||
- Other dead types: `TaskAssignment`, `MemoryMetrics`, `RalphStateRecord`
|
||||
- Dead function: `createSuccessResponse` (exported, never imported)
|
||||
- **Estimated savings:** ~150 lines
|
||||
|
||||
### 5. 9 unused constants in map-limits.ts
|
||||
- `MAX_PENDING_HOOKS`, `MAX_SESSION_HISTORY`, `MAX_SSE_CLIENTS_PER_SESSION`, `MAX_TOTAL_SSE_CLIENTS`, `FILE_WATCHER_WARNING_THRESHOLD`, `MAX_QUEUED_TASKS`, `MAX_COMPLETED_TASKS_HISTORY`, `COMPLETED_TODO_TTL_MS`, `MAX_CONCURRENT_SESSIONS`
|
||||
- 9 of 14 exports are dead — only 5 are actually imported
|
||||
|
||||
### 6. Dead `SessionInputSchema` in schemas.ts
|
||||
- `SessionInputSchema` (line 87) is defined/exported but never imported
|
||||
- `SessionInputWithLimitSchema` is the one actually used
|
||||
|
||||
### 7. Dead `code-reviewer.ts` prompt file
|
||||
- `src/prompts/code-reviewer.ts` — entire file is dead, `CODE_REVIEWER_PROMPT` never imported
|
||||
- Re-exported in `src/prompts/index.ts` but no consumer
|
||||
|
||||
### 8. Dead utility exports
|
||||
- **Default exports** (4 files): `lru-map.ts`, `cleanup-manager.ts`, `stale-expiration-map.ts`, `buffer-accumulator.ts` — all have `export default` that's never used
|
||||
- **`stripAnsiSimple`** in `regex-patterns.ts` — exported, never imported (only `stripAnsi` used)
|
||||
- **String similarity**: `isSimilar`, `isSimilarByDistance`, `stringSimilarity`, `levenshteinDistance` — none imported externally
|
||||
- **LRUMap methods**: `oldest()`, `newest()`, `peek()`, `expireOlderThan()`, `valuesInOrder()`, `maxEntries`, `freeSlots` — never called
|
||||
- **StaleExpirationMap methods**: `touch()`, `getAge()`, `getRemainingTtl()`, `peek()` — never called
|
||||
- **CleanupManager methods**: `registerWatcher()`, `registerListener()`, `registerStream()`, `getRegistrations()`, `resourceCounts` — never called
|
||||
|
||||
### 9. Dead backend functions
|
||||
- `resetSessionManager()` in session-manager.ts:300 — never imported
|
||||
- `getStoredTasks()` in task-queue.ts:264 — never called
|
||||
- `start()` in session.ts:1918 — no-op legacy method
|
||||
- Empty `updateStatsFromEvent()` in run-summary.ts:397 — called every event, does nothing
|
||||
|
||||
### 10. Dead TS type exports
|
||||
- `AiCheckerEvents<R>`, `AiIdleCheckerEvents`, `AiPlanCheckerEvents` — never imported
|
||||
- `AiCheckStatus`, `AiPlanCheckStatus` — backwards compat aliases, never imported
|
||||
|
||||
## P2 — Performance & Efficiency
|
||||
|
||||
### 11. task-queue.ts `getCount()` iterates all tasks 5 times
|
||||
- Called every Ralph Loop tick — creates array from Map, then filters 4 times
|
||||
- **Fix:** Single-pass counting like `TaskTracker.getStats()` does
|
||||
|
||||
### 12. transcript-watcher.ts double file read
|
||||
- `readNewEntries()` reads the file twice: once for CRLF detection, once for parsing
|
||||
- `crlfDelay: Infinity` already handles both line endings
|
||||
- **Fix:** Remove the raw buffer CRLF check, read once
|
||||
|
||||
### 13. tmux-manager.ts `saveSessions()` no debounce
|
||||
- Rapid calls can overlap; no in-flight guard unlike `StateStore`
|
||||
- **Fix:** Add debouncing or in-flight tracking
|
||||
|
||||
## P3 — Consolidation & Consistency
|
||||
|
||||
### 14. Duplicate `SAFE_PATH_PATTERN` regex
|
||||
- `schemas.ts:15` and `tmux-manager.ts:81` — identical regex
|
||||
- **Fix:** Share from one location
|
||||
|
||||
### 15. Duplicate `MAX_CONCURRENT_SESSIONS`
|
||||
- `map-limits.ts:57` (dead) vs `server.ts:131` (used, hardcoded)
|
||||
- **Fix:** server.ts should import from map-limits
|
||||
|
||||
### 16. Duplicate cache TTLs in server.ts
|
||||
- `SESSIONS_LIST_CACHE_TTL` and `LIGHT_STATE_CACHE_TTL_MS` — both 1000ms
|
||||
- **Fix:** Consolidate into one constant
|
||||
|
||||
### 17. Inconsistent path import in server.ts
|
||||
- Imports both `path` default and destructured `{ join, dirname, resolve, relative, isAbsolute }`
|
||||
- 3 lines use `path.join()` while everywhere else uses `join()`
|
||||
- **Fix:** Remove default import, use `join()` consistently
|
||||
|
||||
### 18. Re-export indirection for `getAugmentedPath`
|
||||
- `session.ts:89` re-exports from `claude-cli-resolver.ts` for backwards compat
|
||||
- `ai-checker-base.ts` should import directly from source
|
||||
|
||||
### 19. `cliInfoUpdated` event missing from SessionEvents interface
|
||||
- Emitted in `session.ts:1742`, handled in `server.ts:4214`, but not in the interface
|
||||
- Type safety gap — handlers aren't type-checked
|
||||
|
||||
### 20. Array instead of Set for `_childAgentIds` in session.ts
|
||||
- Uses `includes()`/`indexOf()` for lookups (O(n))
|
||||
- Small lists in practice, but Set is more appropriate
|
||||
@@ -0,0 +1,983 @@
|
||||
> **⚠️ ARCHIVED 2026-05-21 — superseded, kept for history.**
|
||||
> The "Critical" structural items here are done: `server.ts` 6,736→2,065 LOC,
|
||||
> `app.js` 15,196→3,083 LOC, `types.ts` 1,443→12 LOC (now a barrel → `src/types/`).
|
||||
> The phase plans that executed this work are in `docs/archive/phase*-plan.md`.
|
||||
> Do not treat this as a live TODO; see CLAUDE.md for current architecture.
|
||||
|
||||
# Code Structure & Quality Findings
|
||||
|
||||
**Date**: 2026-02-28
|
||||
**Scope**: Full codebase analysis across 5 dimensions: frontend, backend, TypeScript, testing, and utilities/config.
|
||||
|
||||
This document contains detailed findings for agent teams to write implementation plans and execute improvements. Each section includes severity, specific locations, and recommended fixes.
|
||||
|
||||
---
|
||||
|
||||
## Table of Contents
|
||||
|
||||
1. [Critical: server.ts God Object (6,736 LOC)](#1-critical-serverts-god-object)
|
||||
2. [Critical: app.js Monolith (15,196 LOC)](#2-critical-appjs-monolith)
|
||||
3. [Critical: CleanupManager Unused Despite Existing](#3-critical-cleanupmanager-unused)
|
||||
4. [High: Duplicated Debounce/Timer Patterns](#4-high-duplicated-debouncetimer-patterns)
|
||||
5. [High: Large Domain Files Need Splitting](#5-high-large-domain-files-need-splitting)
|
||||
6. [High: types.ts God File (1,443 LOC)](#6-high-typests-god-file)
|
||||
7. [High: Zod Schemas Duplicate TypeScript Types](#7-high-zod-schemas-duplicate-typescript-types)
|
||||
8. [High: Test Coverage Gaps](#8-high-test-coverage-gaps)
|
||||
9. [High: Duplicated Test Mocks](#9-high-duplicated-test-mocks)
|
||||
10. [Medium: Hardcoded Magic Values](#10-medium-hardcoded-magic-values)
|
||||
11. [Medium: Frontend Global State Monolith](#11-medium-frontend-global-state-monolith)
|
||||
12. [Medium: Frontend Code Duplication](#12-medium-frontend-code-duplication)
|
||||
13. [Medium: Inconsistent Logging](#13-medium-inconsistent-logging)
|
||||
14. [Medium: Utils Barrel Export Gaps](#14-medium-utils-barrel-export-gaps)
|
||||
15. [Medium: Non-Null Assertion Risks](#15-medium-non-null-assertion-risks)
|
||||
16. [Low: Dead Utility Functions](#16-low-dead-utility-functions)
|
||||
17. [Low: No Dependency Injection for File I/O](#17-low-no-dependency-injection-for-file-io)
|
||||
18. [Scorecard & Prioritized Roadmap](#18-scorecard--prioritized-roadmap)
|
||||
|
||||
---
|
||||
|
||||
## 1. Critical: server.ts God Object
|
||||
|
||||
**File**: `src/web/server.ts` (6,736 lines)
|
||||
**Severity**: CRITICAL
|
||||
**Impact**: Hardest file to maintain, test, and extend. Imports 38 modules.
|
||||
|
||||
### Problem
|
||||
|
||||
The `WebServer` class handles everything: HTTP routing (~110 routes), authentication, SSE broadcasting, terminal data batching, state persistence, session lifecycle, respawn orchestration, file serving, tunnel management, plan orchestration, and subagent coordination.
|
||||
|
||||
**Key metrics**:
|
||||
- 40+ private properties (Maps, timers, caches)
|
||||
- 70+ methods
|
||||
- `setupRoutes()` is 2,000+ LOC of inline route handlers
|
||||
- Zero test coverage
|
||||
|
||||
### Current Structure (Bad)
|
||||
|
||||
```
|
||||
WebServer class (6,736 LOC)
|
||||
├── Auth session management (lines 469, 668-698)
|
||||
├── SSE client management (lines 407-408, 5843-5880)
|
||||
├── Terminal data batching (lines 414-416, 5909-5966)
|
||||
├── Task update batching (line 426, 5995-6028)
|
||||
├── State persistence batching (lines 429-430, 6028-6061)
|
||||
├── Respawn lifecycle (lines 445-451, 5425-5534)
|
||||
├── Session cleanup (lines 4769-4961)
|
||||
├── Listener setup (lines 544-643)
|
||||
└── setupRoutes() (lines 645+, 2000+ LOC)
|
||||
├── /api/sessions/* (30+ routes inline)
|
||||
├── /api/respawn/* (7 routes inline)
|
||||
├── /api/subagents/* (7 routes inline)
|
||||
├── /api/plan/* (5 routes inline)
|
||||
├── /api/push/* (4 routes inline)
|
||||
└── ... 60+ more inline
|
||||
```
|
||||
|
||||
### Recommended Structure
|
||||
|
||||
```
|
||||
src/web/
|
||||
├── server.ts (~500 LOC - HTTP setup, route registration only)
|
||||
├── routes/
|
||||
│ ├── session-routes.ts (session CRUD, input, resize)
|
||||
│ ├── respawn-routes.ts (respawn control endpoints)
|
||||
│ ├── subagent-routes.ts (background agent tracking)
|
||||
│ ├── plan-routes.ts (plan generation & management)
|
||||
│ ├── push-routes.ts (web push subscriptions)
|
||||
│ ├── mux-routes.ts (tmux management)
|
||||
│ ├── case-routes.ts (case management)
|
||||
│ ├── file-routes.ts (file browsing/serving)
|
||||
│ └── system-routes.ts (status, stats, config, settings)
|
||||
├── middleware/
|
||||
│ ├── auth.ts (Basic Auth + session cookies)
|
||||
│ └── error-handler.ts (centralized error responses)
|
||||
└── services/
|
||||
├── sse-manager.ts (SSE client + broadcast)
|
||||
├── terminal-batcher.ts (60fps terminal batching)
|
||||
└── session-lifecycle.ts (listener setup/teardown)
|
||||
```
|
||||
|
||||
### Duplication in server.ts
|
||||
|
||||
**Error response pattern** repeated 189 times:
|
||||
```typescript
|
||||
return createErrorResponse(ApiErrorCode.NOT_FOUND, 'Session not found');
|
||||
```
|
||||
|
||||
**Fix**: Extract `findSessionOrFail()` middleware:
|
||||
```typescript
|
||||
const findSessionOrFail = (sessionId: string) => {
|
||||
const session = this.sessions.get(sessionId);
|
||||
if (!session) throw new NotFoundError('Session not found');
|
||||
return session;
|
||||
};
|
||||
```
|
||||
|
||||
**Event listener setup** copy-pasted for subagent watcher, image watcher, and team watcher (lines 544-643). Same attach/detach pattern duplicated 3 times.
|
||||
|
||||
---
|
||||
|
||||
## 2. Critical: app.js Monolith
|
||||
|
||||
**File**: `src/web/public/app.js` (15,196 lines)
|
||||
**Severity**: CRITICAL
|
||||
**Impact**: Untestable, hard to navigate, tightly coupled systems.
|
||||
|
||||
### Extractable Modules (by priority)
|
||||
|
||||
| Module | Lines | Current Location | Impact |
|
||||
|--------|-------|------------------|--------|
|
||||
| Mobile handlers (MobileDetection, KeyboardHandler, SwipeHandler) | ~300 | lines 168-620 | High |
|
||||
| Voice input (DeepgramProvider, VoiceInput) | ~830 | lines 631-1471 | High |
|
||||
| NotificationManager | ~450 | lines 2218-2663 | High |
|
||||
| xterm-zerolag-input (inlined copy from packages/) | ~400 | lines 1756-2153 | High |
|
||||
| KeyboardAccessoryBar | ~195 | lines 1480-1680 | Medium |
|
||||
| FocusTrap | ~60 | lines 1690-1748 | Medium |
|
||||
|
||||
### CodemanApp Class (12,000+ LOC)
|
||||
|
||||
The main `CodemanApp` class starting at line 2665 has:
|
||||
- **60+ Maps/Sets** in the constructor (lines 2667-2805)
|
||||
- **18 Map instances** with complex cross-references (subagents, parents, teams, windows)
|
||||
- **10+ monolithic methods** exceeding 100 lines each
|
||||
|
||||
**Largest methods**:
|
||||
| Method | Lines | Size |
|
||||
|--------|-------|------|
|
||||
| `renderAppSettings()` | 14400-14700 | ~300 LOC |
|
||||
| `selectSession()` | 6028-6250 | ~220 LOC |
|
||||
| `batchTerminalWrite()` | 7482-7700 | ~200 LOC |
|
||||
| `renderSessionTabs()` | 5814-6000 | ~180 LOC |
|
||||
| `openSubagentWindow()` | 11927-12100 | ~170 LOC |
|
||||
| `handleInit()` | 5183-5350 | ~170 LOC |
|
||||
|
||||
### Recommended Split
|
||||
|
||||
```
|
||||
src/web/public/
|
||||
├── app.js (~4000 LOC - core app, session mgmt, SSE)
|
||||
├── mobile.js (~300 LOC - MobileDetection, KeyboardHandler, SwipeHandler)
|
||||
├── voice.js (~830 LOC - DeepgramProvider, VoiceInput)
|
||||
├── notifications.js (~450 LOC - NotificationManager)
|
||||
├── keyboard-accessory.js (~200 LOC - KeyboardAccessoryBar)
|
||||
├── api-client.js (~100 LOC - fetch wrapper with error handling)
|
||||
└── config.js (~50 LOC - magic numbers, z-index layers)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 3. Critical: CleanupManager Unused
|
||||
|
||||
**File**: `src/utils/cleanup-manager.ts` (320 lines)
|
||||
**Severity**: CRITICAL
|
||||
**Impact**: Memory leak risk. Well-designed utility exists but is never used. Every file manages cleanup manually.
|
||||
|
||||
### Current State
|
||||
|
||||
`CleanupManager` is exported from the utils barrel but has **0 instantiations** in production code. Instead, every file implements manual cleanup:
|
||||
|
||||
**respawn-controller.ts** (worst offender):
|
||||
```typescript
|
||||
// 11 timer properties, manually cleared in stop()
|
||||
private stepTimer: NodeJS.Timeout | null = null;
|
||||
private completionConfirmTimer: NodeJS.Timeout | null = null;
|
||||
private noOutputTimer: NodeJS.Timeout | null = null;
|
||||
// ... 8 more
|
||||
|
||||
stop() {
|
||||
if (this.stepTimer) clearTimeout(this.stepTimer);
|
||||
if (this.completionConfirmTimer) clearTimeout(this.completionConfirmTimer);
|
||||
// ... 9 more clearTimeout/clearInterval calls
|
||||
}
|
||||
```
|
||||
|
||||
**Files that should use CleanupManager**:
|
||||
| File | Timer/Listener Count | Current Cleanup |
|
||||
|------|---------------------|-----------------|
|
||||
| `respawn-controller.ts` | 11 timers + intervals | 11 manual clearTimeout/clearInterval |
|
||||
| `web/server.ts` | 6+ timers, debounce map | Manual in stop(), some may leak |
|
||||
| `state-store.ts` | 2 debounce timers | Manual clearTimeout |
|
||||
| `push-store.ts` | 1 save timer | Manual clearTimeout |
|
||||
| `subagent-watcher.ts` | debounce map + watchers | Manual clear + close |
|
||||
| `ralph-tracker.ts` | 3 debounce timers | Manual clear |
|
||||
| `bash-tool-parser.ts` | 1 debounce timer | Manual clear |
|
||||
| `image-watcher.ts` | 1 debounce map | Manual clear |
|
||||
|
||||
### Fix
|
||||
|
||||
Migrate all timer management to use `CleanupManager`. Example for respawn-controller.ts:
|
||||
|
||||
```typescript
|
||||
// Before: 11 fields + 11 clearTimeout calls
|
||||
private stepTimer: NodeJS.Timeout | null = null;
|
||||
// ...
|
||||
|
||||
// After: 1 field, auto-cleanup
|
||||
private cleanup = new CleanupManager();
|
||||
|
||||
startStep() {
|
||||
this.cleanup.setTimeout(() => { ... }, 5000, 'step');
|
||||
}
|
||||
|
||||
stop() {
|
||||
this.cleanup.dispose(); // Clears everything
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 4. High: Duplicated Debounce/Timer Patterns
|
||||
|
||||
**Severity**: HIGH
|
||||
**Impact**: 8+ files implement debounce independently. Bug fixes need to be applied everywhere.
|
||||
|
||||
### Pattern Inventory
|
||||
|
||||
```typescript
|
||||
// Pattern 1: Manual timer ref (used in 6 files)
|
||||
private saveTimer: NodeJS.Timeout | null = null;
|
||||
debouncedSave() {
|
||||
if (this.saveTimer) clearTimeout(this.saveTimer);
|
||||
this.saveTimer = setTimeout(() => this.save(), 500);
|
||||
}
|
||||
|
||||
// Pattern 2: Timer Map (used in 3 files)
|
||||
private fileDebouncers = new Map<string, NodeJS.Timeout>();
|
||||
debounce(key: string) {
|
||||
const existing = this.fileDebouncers.get(key);
|
||||
if (existing) clearTimeout(existing);
|
||||
this.fileDebouncers.set(key, setTimeout(() => { ... }, 100));
|
||||
}
|
||||
|
||||
// Pattern 3: State flag (used in 2 files)
|
||||
private isSaving = false;
|
||||
```
|
||||
|
||||
### Locations
|
||||
|
||||
| File | Debounce Vars | Delay (ms) |
|
||||
|------|---------------|------------|
|
||||
| `state-store.ts` | `saveTimeout`, `ralphStateSaveTimeout` | 500 |
|
||||
| `push-store.ts` | `saveTimer` | 500 |
|
||||
| `web/server.ts` | `persistDebounceTimers` (Map) | 500 |
|
||||
| `subagent-watcher.ts` | `fileDebouncers` (Map) | 100 |
|
||||
| `ralph-tracker.ts` | 3 debounce timers | 50, 30000 |
|
||||
| `bash-tool-parser.ts` | `EVENT_DEBOUNCE_MS` | 50 |
|
||||
| `image-watcher.ts` | debounce map | 200 |
|
||||
| `respawn-controller.ts` | 11 timer fields | various |
|
||||
|
||||
### Fix
|
||||
|
||||
Create a `Debouncer` utility:
|
||||
|
||||
```typescript
|
||||
// src/utils/debouncer.ts
|
||||
export class Debouncer {
|
||||
private timer: NodeJS.Timeout | null = null;
|
||||
|
||||
constructor(private readonly delayMs: number) {}
|
||||
|
||||
run(fn: () => void): void {
|
||||
if (this.timer) clearTimeout(this.timer);
|
||||
this.timer = setTimeout(fn, this.delayMs);
|
||||
}
|
||||
|
||||
cancel(): void {
|
||||
if (this.timer) clearTimeout(this.timer);
|
||||
this.timer = null;
|
||||
}
|
||||
}
|
||||
|
||||
// Usage:
|
||||
private saveDeb = new Debouncer(500);
|
||||
this.saveDeb.run(() => this.save());
|
||||
// cleanup: this.saveDeb.cancel();
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 5. High: Large Domain Files Need Splitting
|
||||
|
||||
**Severity**: HIGH
|
||||
**Impact**: Complex state machines spanning 3,000+ lines are hard to understand and test.
|
||||
|
||||
### ralph-tracker.ts (3,905 LOC)
|
||||
|
||||
**5 responsibilities mixed**:
|
||||
1. Output Parsing (~900 LOC) - Line-by-line parsing, state extraction
|
||||
2. Todo Management (~700 LOC) - Parsing, dedup, expiry
|
||||
3. Plan Tracking (~800 LOC) - Enhanced plan tasks, checkpoints
|
||||
4. Circuit Breaker (~400 LOC) - State machine for stuck detection
|
||||
5. File Watching (~300 LOC) - Monitor external state files
|
||||
|
||||
**Recommended split**:
|
||||
```
|
||||
ralph-tracker.ts (core output parsing, ~1200 LOC)
|
||||
ralph-todo-manager.ts (todo parsing + management, ~700 LOC)
|
||||
ralph-plan-tracker.ts (plan tasks + checkpoints, ~800 LOC)
|
||||
ralph-circuit-breaker.ts (circuit breaker logic, ~400 LOC)
|
||||
```
|
||||
|
||||
### respawn-controller.ts (3,611 LOC)
|
||||
|
||||
**6 responsibilities mixed**:
|
||||
1. State Machine (~1,000 LOC) - 6+ states, transitions
|
||||
2. Idle Detection (~800 LOC) - 5 layers + multi-signal combining
|
||||
3. AI Checkers (~600 LOC) - Idle + plan checkers integration
|
||||
4. Health Scoring (~500 LOC) - Metrics, circuit breaker, scoring
|
||||
5. Action Logging (~300 LOC) - Timeline, detection status
|
||||
6. Stuck-State Detection (~250 LOC) - Timeout tracking
|
||||
|
||||
**Recommended split**:
|
||||
```
|
||||
respawn-controller.ts (state machine core, ~1000 LOC)
|
||||
respawn-idle-detection.ts (all 5 idle detection layers, ~800 LOC)
|
||||
respawn-health-scorer.ts (metrics & health scoring, ~500 LOC)
|
||||
```
|
||||
|
||||
### session.ts (2,418 LOC)
|
||||
|
||||
**8 responsibilities mixed**:
|
||||
1. PTY Management (~600 LOC)
|
||||
2. Terminal I/O (~400 LOC)
|
||||
3. Token Tracking (~200 LOC)
|
||||
4. Task Tracking (~250 LOC)
|
||||
5. Ralph Integration (~200 LOC)
|
||||
6. Auto-Clear/Compact (~300 LOC)
|
||||
7. Image Watching (~100 LOC)
|
||||
8. CLI Detection (~150 LOC)
|
||||
|
||||
**Recommended split**:
|
||||
```
|
||||
session.ts (PTY + terminal I/O core, ~1000 LOC)
|
||||
session-tracking.ts (token + task + Ralph, ~500 LOC)
|
||||
session-auto-ops.ts (auto-clear/compact + image, ~300 LOC)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 6. High: types.ts God File
|
||||
|
||||
**File**: `src/types.ts` (1,443 lines, 72 exported definitions)
|
||||
**Severity**: HIGH
|
||||
**Impact**: Every file imports from types.ts. Hard to find relevant types.
|
||||
|
||||
### Current Contents
|
||||
|
||||
- 46 interfaces
|
||||
- 25 types
|
||||
- 1 enum (ApiErrorCode)
|
||||
- 9 factory functions (createInitialState, etc.)
|
||||
|
||||
### Recommended Split
|
||||
|
||||
```
|
||||
src/types/
|
||||
├── index.ts (barrel export - transparent migration)
|
||||
├── session.ts (SessionState, SessionConfig, SessionMode, SessionColor)
|
||||
├── task.ts (TaskState, TaskDefinition, TaskStatus)
|
||||
├── respawn.ts (RespawnConfig, RespawnState, CircuitBreakerStatus)
|
||||
├── ralph.ts (RalphLoopState, RalphTrackerState, RalphTodoItem)
|
||||
├── api.ts (ApiResponse, ApiErrorCode, HookEventType, all route types)
|
||||
├── lifecycle.ts (LifecycleEventType, LifecycleEntry)
|
||||
└── common.ts (Disposable, BufferConfig, CleanupResourceType)
|
||||
```
|
||||
|
||||
The barrel export makes this a transparent refactor - existing `import from './types'` continues to work.
|
||||
|
||||
---
|
||||
|
||||
## 7. High: Zod Schemas Duplicate TypeScript Types
|
||||
|
||||
**File**: `src/web/schemas.ts` (508 lines)
|
||||
**Severity**: HIGH
|
||||
**Impact**: When a type changes, the Zod schema must be manually updated too. Source of bugs.
|
||||
|
||||
### Problem
|
||||
|
||||
Zod schemas manually duplicate TypeScript interfaces. **Zero `z.infer` usage found.**
|
||||
|
||||
```typescript
|
||||
// types.ts (manual interface)
|
||||
export interface CreateSessionRequest {
|
||||
workingDir?: string;
|
||||
mode?: SessionMode;
|
||||
name?: string;
|
||||
}
|
||||
|
||||
// schemas.ts (manual Zod schema - duplicated!)
|
||||
export const CreateSessionSchema = z.object({
|
||||
workingDir: safePathSchema.optional(),
|
||||
mode: z.enum(['claude', 'shell', 'opencode']).optional(),
|
||||
name: z.string().max(100).optional(),
|
||||
});
|
||||
```
|
||||
|
||||
### Fix
|
||||
|
||||
Use `z.infer` to derive TypeScript types from Zod schemas (single source of truth):
|
||||
|
||||
```typescript
|
||||
// schemas.ts
|
||||
export const CreateSessionSchema = z.object({
|
||||
workingDir: safePathSchema.optional(),
|
||||
mode: z.enum(['claude', 'shell', 'opencode']).optional(),
|
||||
name: z.string().max(100).optional(),
|
||||
});
|
||||
|
||||
// types.ts (auto-derived)
|
||||
export type CreateSessionRequest = z.infer<typeof CreateSessionSchema>;
|
||||
```
|
||||
|
||||
**Affected schemas** (~10):
|
||||
- CreateSessionSchema
|
||||
- RunPromptSchema
|
||||
- ResizeSchema
|
||||
- CreateCaseSchema
|
||||
- QuickStartSchema
|
||||
- HookEventSchema
|
||||
- RespawnConfigSchema
|
||||
- ConfigUpdateSchema
|
||||
- SettingsUpdateSchema
|
||||
|
||||
---
|
||||
|
||||
## 8. High: Test Coverage Gaps
|
||||
|
||||
**Severity**: HIGH
|
||||
**Impact**: Critical code paths untested. Regressions go unnoticed.
|
||||
|
||||
### Untested Source Files
|
||||
|
||||
| File | Lines | Risk |
|
||||
|------|-------|------|
|
||||
| `src/web/server.ts` | 6,736 | CRITICAL - Core REST API, 280+ routes |
|
||||
| `src/plan-orchestrator.ts` | ~500 | HIGH - Multi-agent plan generation |
|
||||
| `src/tunnel-manager.ts` | ~200 | MEDIUM - Cloudflare tunnel |
|
||||
| `src/session-lifecycle-log.ts` | ~150 | MEDIUM - JSONL audit log |
|
||||
| `src/ai-plan-checker.ts` | ~300 | MEDIUM - Plan completion detection |
|
||||
| `src/templates/claude-md.ts` | ~200 | LOW - CLAUDE.md generation |
|
||||
| `src/utils/claude-cli-resolver.ts` | ~100 | LOW - CLI path resolution |
|
||||
| `src/utils/opencode-cli-resolver.ts` | ~100 | LOW - OpenCode CLI support |
|
||||
| `src/utils/regex-patterns.ts` | ~100 | LOW - Used everywhere! |
|
||||
| `src/utils/token-validation.ts` | ~50 | LOW - Token counting |
|
||||
|
||||
### Test Quality Issues
|
||||
|
||||
**10 "not.toThrow()" tests without behavior verification**:
|
||||
```typescript
|
||||
// BAD: Only checks it doesn't crash
|
||||
expect(() => tracker.processMessage(null)).not.toThrow();
|
||||
|
||||
// GOOD: Also verify defensive behavior
|
||||
expect(() => tracker.processMessage(null)).not.toThrow();
|
||||
expect(tracker.getAllTasks().size).toBe(0);
|
||||
```
|
||||
|
||||
Locations:
|
||||
- `task-tracker.test.ts` - 5 instances
|
||||
- `image-watcher.test.ts` - 1 instance
|
||||
- `task-queue.test.ts` - 1 instance
|
||||
- Others scattered
|
||||
|
||||
---
|
||||
|
||||
## 9. High: Duplicated Test Mocks
|
||||
|
||||
**Severity**: HIGH
|
||||
**Impact**: Mock changes need updating in 4 places. Inconsistent mock behavior.
|
||||
|
||||
### MockSession Defined 4 Times
|
||||
|
||||
| File | Usage |
|
||||
|------|-------|
|
||||
| `test/respawn-controller.test.ts` | Full mock with event emitter |
|
||||
| `test/session-manager.test.ts` | Simpler mock |
|
||||
| `test/respawn-team-awareness.test.ts` | Copy of respawn-controller mock |
|
||||
| `test/respawn-test-utils.ts` | **Comprehensive mock - UNUSED!** |
|
||||
|
||||
### MockStateStore Defined 2 Times
|
||||
|
||||
| File | Usage |
|
||||
|------|-------|
|
||||
| `test/session-manager.test.ts` | Basic mock |
|
||||
| `test/ralph-loop.test.ts` | Separate implementation |
|
||||
|
||||
### Unused Test Utilities
|
||||
|
||||
`test/respawn-test-utils.ts` exports these utilities that **no test file imports**:
|
||||
- `createTimeController()` - Abstraction over vitest fake timers
|
||||
- `MockAiIdleChecker` - Fully mocked AI idle checker
|
||||
- `MockAiPlanChecker` - Fully mocked plan checker
|
||||
- Factory functions for pre-configured controllers
|
||||
|
||||
### Fix
|
||||
|
||||
Create `test/mocks/` directory:
|
||||
```
|
||||
test/
|
||||
├── mocks/
|
||||
│ ├── mock-session.ts (single MockSession, used everywhere)
|
||||
│ ├── mock-state-store.ts (single MockStateStore)
|
||||
│ └── index.ts (barrel export)
|
||||
├── utils/
|
||||
│ └── time-controller.ts (from respawn-test-utils.ts)
|
||||
└── ... test files
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 10. Medium: Hardcoded Magic Values
|
||||
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Hard to tune, inconsistent when same value appears in multiple places.
|
||||
|
||||
### Already Centralized (Good)
|
||||
|
||||
- `src/config/buffer-limits.ts` - All buffer sizes
|
||||
- `src/config/map-limits.ts` - All collection limits
|
||||
|
||||
### NOT Centralized (40+ values scattered)
|
||||
|
||||
**In server.ts** (lines 145-194):
|
||||
```typescript
|
||||
const TASK_UPDATE_BATCH_INTERVAL = 100;
|
||||
const STATE_UPDATE_DEBOUNCE_INTERVAL = 500;
|
||||
const SESSIONS_LIST_CACHE_TTL = 1000;
|
||||
const SCHEDULED_CLEANUP_INTERVAL = 5 * 60 * 1000;
|
||||
const SSE_HEALTH_CHECK_INTERVAL = 30 * 1000;
|
||||
const MAX_TERMINAL_COLS = 500;
|
||||
const MAX_TERMINAL_ROWS = 200;
|
||||
const AUTH_SESSION_TTL_MS = 24 * 60 * 60 * 1000;
|
||||
const MAX_AUTH_SESSIONS = 100;
|
||||
const AUTH_FAILURE_WINDOW_MS = 15 * 60 * 1000;
|
||||
const STATS_COLLECTION_INTERVAL_MS = 2000;
|
||||
const MAX_INPUT_LENGTH = 64 * 1024;
|
||||
```
|
||||
|
||||
**In hooks-config.ts**: `timeout: 10000` hardcoded 6 times.
|
||||
|
||||
**In respawn-controller.ts** (lines 538-565): 10 timing constants.
|
||||
|
||||
**In utils**: `EXEC_TIMEOUT_MS = 5000` duplicated in both `claude-cli-resolver.ts` and `opencode-cli-resolver.ts`.
|
||||
|
||||
**In app.js**:
|
||||
```javascript
|
||||
// line 27: 600000 - stuck detection threshold
|
||||
// line 24: 5000 - default scrollback
|
||||
// lines 34-35: 128*1024, 256*1024 - chunk sizes
|
||||
// lines 152-155: 150, 100 - keyboard detection thresholds
|
||||
// lines 573-575: 80, 300, 100 - swipe detection params
|
||||
```
|
||||
|
||||
### Fix
|
||||
|
||||
Create additional config files:
|
||||
```
|
||||
src/config/
|
||||
├── buffer-limits.ts (existing)
|
||||
├── map-limits.ts (existing)
|
||||
├── server-config.ts (NEW - web server intervals, auth, caching)
|
||||
├── timing-config.ts (NEW - debounce delays, check intervals)
|
||||
└── terminal-config.ts (NEW - max cols/rows, batch intervals)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 11. Medium: Frontend Global State Monolith
|
||||
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: All state in single CodemanApp class. Tight coupling between unrelated systems.
|
||||
|
||||
### 60+ State Variables in CodemanApp Constructor (lines 2667-2805)
|
||||
|
||||
```javascript
|
||||
this.sessions = new Map(); // Session data
|
||||
this.subagents = new Map(); // Agent tracking
|
||||
this.subagentActivity = new Map(); // Tool call tracking
|
||||
this.subagentToolResults = new Map(); // Result caching
|
||||
this.subagentParentMap = new Map(); // Agent-to-session mapping
|
||||
this.teams = new Map(); // Team tracking
|
||||
this.teamTasks = new Map(); // Team task state
|
||||
this.planSubagents = new Map(); // Plan agent tracking
|
||||
this.pendingWrites = []; // Terminal write queue
|
||||
this.terminalBufferCache = new Map(); // Buffer caching (unbounded!)
|
||||
this.projectInsights = new Map(); // Bash tool insights
|
||||
// ... 40+ more
|
||||
```
|
||||
|
||||
### Problems
|
||||
|
||||
1. **18 Map instances** with complex cross-references (no garbage collection strategy)
|
||||
2. **No domain separation**: Session, subagent, notification, UI, and network state mixed
|
||||
3. **Implicit dependencies**: `selectSession()` requires 5+ Maps to be in consistent state
|
||||
4. **`terminalBufferCache`** has no max size - can grow unbounded with many sessions
|
||||
|
||||
### Recommended Domain Split
|
||||
|
||||
```javascript
|
||||
// Instead of 60+ flat properties:
|
||||
class SessionState {
|
||||
sessions = new Map();
|
||||
sessionOrder = [];
|
||||
terminalBuffers = new Map();
|
||||
tabAlerts = new Map();
|
||||
}
|
||||
|
||||
class SubagentState {
|
||||
subagents = new Map();
|
||||
activity = new Map();
|
||||
parentMap = new Map();
|
||||
windows = new Map();
|
||||
minimized = new Map();
|
||||
}
|
||||
|
||||
class TeamState {
|
||||
teams = new Map();
|
||||
tasks = new Map();
|
||||
teammates = new Map();
|
||||
}
|
||||
|
||||
class UIState {
|
||||
activeSessionId = null;
|
||||
draggedTabId = null;
|
||||
isLoadingBuffer = false;
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 12. Medium: Frontend Code Duplication
|
||||
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Repeated patterns increase maintenance burden and inconsistency risk.
|
||||
|
||||
### Duplicated Patterns
|
||||
|
||||
**API fetch calls** (~50 instances):
|
||||
```javascript
|
||||
// Repeated everywhere:
|
||||
fetch(`/api/sessions/${sessionId}/...`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({...})
|
||||
}).catch(() => {})
|
||||
```
|
||||
**Fix**: Extract `ApiClient` class.
|
||||
|
||||
**`innerHTML` usage** (104 instances):
|
||||
- Mix of template strings, createElement chains, and direct innerHTML
|
||||
- Some with manual XSS escaping (`text.replace(/</g, '<')`), some without
|
||||
- No consistent DOM creation pattern
|
||||
|
||||
**`typeof app !== 'undefined'` checks** (20+ instances):
|
||||
- Lines 458, 467, 481, 614, 617, 1549, etc.
|
||||
- **Fix**: Ensure `app` is always defined as global singleton.
|
||||
|
||||
**Element visibility toggling** (212+ occurrences):
|
||||
```javascript
|
||||
element.classList.add('active')
|
||||
element.classList.remove('active')
|
||||
```
|
||||
**Fix**: Create `toggleClass(el, className, condition)` utility.
|
||||
|
||||
### Event Listener Issues
|
||||
|
||||
- **152 `addEventListener` calls** with fragile cleanup
|
||||
- **Mix of inline (`onclick="app.method()"`) and addEventListener** - hard to track
|
||||
- **Element cache (`_elemCache`) never invalidated** if DOM elements are recreated (line 2808)
|
||||
- **Tab drag-and-drop listeners** may not clean up if user switches tabs mid-drag
|
||||
|
||||
---
|
||||
|
||||
## 13. Medium: Inconsistent Logging
|
||||
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Hard to debug in production. Can't filter by severity or component.
|
||||
|
||||
### Current State
|
||||
|
||||
- **345 console calls** across source files
|
||||
- **No structured logging** - all `console.log/error` directly
|
||||
- **No log levels** (DEBUG, INFO, WARN, ERROR)
|
||||
|
||||
### Inconsistent Prefixes
|
||||
|
||||
```typescript
|
||||
// Some files use brackets:
|
||||
console.log('[Session] Starting interactive...');
|
||||
console.log('[RalphLoop] Task assigned...');
|
||||
console.log('[TunnelManager] Tunnel started');
|
||||
|
||||
// Others use no prefix:
|
||||
console.error('Failed to spawn PTY:', err);
|
||||
console.log('Server listening on port', port);
|
||||
```
|
||||
|
||||
### Positive: CleanupManager Has Debug Mode
|
||||
|
||||
`src/utils/cleanup-manager.ts` has a `debugMode` flag for conditional debug logging - good pattern not replicated elsewhere.
|
||||
|
||||
### Fix
|
||||
|
||||
Either:
|
||||
1. Enforce consistent `[ComponentName]` prefixes via lint rule
|
||||
2. Create lightweight logger abstraction (not a heavy framework)
|
||||
|
||||
---
|
||||
|
||||
## 14. Medium: Utils Barrel Export Gaps
|
||||
|
||||
**File**: `src/utils/index.ts`
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Forces deep imports, unclear public API.
|
||||
|
||||
### Missing Exports
|
||||
|
||||
These functions are defined but NOT exported from the barrel:
|
||||
- `createAnsiPatternFull()` and `createAnsiPatternSimple()` (factory functions from `regex-patterns.ts`)
|
||||
- `SAFE_PATH_PATTERN` (from `regex-patterns.ts`)
|
||||
- `validateTokenCounts()` and `validateTokensAndCost()` (from `token-validation.ts`)
|
||||
- `isSimilar()`, `isSimilarByDistance()`, `levenshteinDistance()`, `normalizePhrase()` (from `string-similarity.ts` - though some are dead code, see finding #16)
|
||||
|
||||
### Deep Import Anti-Pattern (16 instances)
|
||||
|
||||
Some files bypass the barrel unnecessarily:
|
||||
```typescript
|
||||
// Could use barrel:
|
||||
import { BufferAccumulator } from './utils/buffer-accumulator.js';
|
||||
import { LRUMap } from './utils/lru-map.js';
|
||||
|
||||
// Must deep import (not in barrel):
|
||||
import { SAFE_PATH_PATTERN } from './utils/regex-patterns.js';
|
||||
```
|
||||
|
||||
### Fix
|
||||
|
||||
Add missing exports to `src/utils/index.ts` and update import sites.
|
||||
|
||||
---
|
||||
|
||||
## 15. Medium: Non-Null Assertion Risks
|
||||
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Runtime crashes if assumptions violated. 37 instances found.
|
||||
|
||||
### Distribution
|
||||
|
||||
| File | Count | Risk Level |
|
||||
|------|-------|------------|
|
||||
| `src/web/server.ts` | 10 | Low (auth flow verified) |
|
||||
| `src/session.ts` | 6 | **High** (mux/terminal refs) |
|
||||
| `src/respawn-controller.ts` | 4 | Low (config validated) |
|
||||
| `src/lru-map.ts` | 3 | Low (checked lookups) |
|
||||
| `src/subagent-watcher.ts` | 2 | Low (pending tool calls) |
|
||||
| Others | 12 | Low |
|
||||
|
||||
### High-Risk Examples (session.ts)
|
||||
|
||||
```typescript
|
||||
// Line 915 - _mux could be null if startInteractive called during cleanup
|
||||
`[Session] Starting interactive (with ${this._mux!.backend})`
|
||||
|
||||
// Line 954 - _muxSession could be null in race condition
|
||||
this._muxSession!.muxName
|
||||
```
|
||||
|
||||
### Fix
|
||||
|
||||
Add null guards before assertions, or document invariants:
|
||||
```typescript
|
||||
// Before:
|
||||
this._mux!.backend
|
||||
|
||||
// After:
|
||||
if (!this._mux) throw new Error('Invariant: _mux must be initialized before startInteractive');
|
||||
this._mux.backend
|
||||
```
|
||||
|
||||
### Positive Notes
|
||||
|
||||
- **0 instances of `as any`**
|
||||
- **0 instances of `@ts-ignore` or `@ts-expect-error`**
|
||||
- TypeScript overall score: 8.5/10
|
||||
|
||||
---
|
||||
|
||||
## 16. Low: Dead Utility Functions
|
||||
|
||||
**File**: `src/utils/string-similarity.ts`
|
||||
**Severity**: LOW
|
||||
**Impact**: Code clutter, confusion about what's actually used.
|
||||
|
||||
### Unused Functions
|
||||
|
||||
These are defined and exported but **never imported anywhere**:
|
||||
- `isSimilar(a, b, threshold)` - similarity check with threshold
|
||||
- `isSimilarByDistance(a, b, maxDistance)` - Levenshtein-based check
|
||||
- `levenshteinDistance(a, b)` - raw edit distance
|
||||
- `normalizePhrase(phrase)` - phrase normalization
|
||||
|
||||
### Actually Used
|
||||
|
||||
Only these are imported from the barrel:
|
||||
- `stringSimilarity()` - used in ralph-tracker.ts
|
||||
- `fuzzyPhraseMatch()` - used in ralph-tracker.ts
|
||||
- `todoContentHash()` - used in ralph-tracker.ts
|
||||
|
||||
### Fix
|
||||
|
||||
Delete unused functions or mark as `@internal` if kept for future use.
|
||||
|
||||
---
|
||||
|
||||
## 17. Low: No Dependency Injection for File I/O
|
||||
|
||||
**Severity**: LOW (practical impact limited at current scale)
|
||||
**Impact**: Can't mock filesystem for unit tests. 68+ hard-coded filesystem calls.
|
||||
|
||||
### Examples
|
||||
|
||||
```typescript
|
||||
// state-store.ts - directly imports and uses fs
|
||||
import { readFileSync, writeFileSync, existsSync, mkdirSync } from 'node:fs';
|
||||
|
||||
// push-store.ts - hard-coded paths
|
||||
const KEYS_FILE = join(DATA_DIR, 'push-keys.json');
|
||||
const SUBS_FILE = join(DATA_DIR, 'push-subscriptions.json');
|
||||
|
||||
// ai-checker-base.ts - direct execSync
|
||||
execSync(`tmux kill-session -t "${this.checkMuxName}"`, { timeout: 3000 });
|
||||
```
|
||||
|
||||
### Why This Is Lower Priority
|
||||
|
||||
- The codebase uses integration tests (spawning real processes/tmux sessions) rather than unit tests
|
||||
- Most filesystem operations are in infrastructure code, not business logic
|
||||
- Adding DI would be a large refactor with limited near-term benefit
|
||||
|
||||
---
|
||||
|
||||
## 18. Scorecard & Prioritized Roadmap
|
||||
|
||||
### Overall Scores (Post-Implementation)
|
||||
|
||||
| Category | Before | After | Notes |
|
||||
|----------|--------|-------|-------|
|
||||
| TypeScript Safety | 8.5/10 | 9/10 | 0 `any`, 0 `@ts-ignore`, Zod `z.infer` eliminates type drift |
|
||||
| Error Handling | 8/10 | 8/10 | Unchanged — already strong |
|
||||
| Async/Promise Safety | 9.5/10 | 9.5/10 | Unchanged — already strong |
|
||||
| Resource Cleanup | 7/10 | 8/10 | CleanupManager adopted in server.ts, subagent-watcher, bash-tool-parser; Debouncer in 6 files. **Gaps**: respawn-controller (10+ manual timers) and ralph-tracker (2 manual timers) not migrated |
|
||||
| Module Organization | 5/10 | 8/10 | Routes extracted (12 modules), types split (14 domain files), domain files split (ralph: 7, respawn: 5, session: 6) |
|
||||
| Test Coverage | 6/10 | 7.5/10 | Shared mock infrastructure, 12 route test files, MockSession/MockStateStore consolidated |
|
||||
| Config Centralization | 6/10 | 9/10 | 9 config files, ~65 constants centralized, 0 cross-file duplicates |
|
||||
| Frontend Architecture | 4/10 | 7/10 | 8 extracted modules (3,453 LOC), app.js reduced 24% (15.2K → 11.5K), xterm-zerolag-input vendor build |
|
||||
| Code Duplication | 5/10 | 8/10 | Debouncer utility, shared test mocks, barrel exports, config consolidation |
|
||||
|
||||
### Implementation Phases
|
||||
|
||||
**Phase 1 - Quick Wins (1-2 days)** ✅ COMPLETE
|
||||
1. ✅ Export missing functions from utils barrel (~30 min) — `createAnsiPatternFull`, `createAnsiPatternSimple`, `SAFE_PATH_PATTERN`, `validateTokenCounts`, `validateTokensAndCost` all now exported from `src/utils/index.ts`
|
||||
2. ✅ Delete dead utility functions (~15 min) — `isSimilar()` removed from `string-similarity.ts`; `levenshteinDistance()`, `isSimilarByDistance()`, `normalizePhrase()` made private (used internally by `fuzzyPhraseMatch`/`stringSimilarity`)
|
||||
3. ✅ Consolidate duplicated `EXEC_TIMEOUT_MS` constant (~15 min) — Created `src/config/exec-timeout.ts` as single source of truth; `claude-cli-resolver.ts`, `opencode-cli-resolver.ts`, and `tmux-manager.ts` all import from it
|
||||
4. ✅ Add `z.infer` to Zod schemas (~2 hours) — `src/web/schemas.ts` now has 36 `z.infer` type exports (lines 512-547) covering all schemas
|
||||
5. ✅ Fix 10 weak "not.toThrow()" tests (~1 hour) — All `not.toThrow()` calls now have behavior assertions: `task-tracker.test.ts` (6 instances all followed by state checks), `image-watcher.test.ts` (1 instance followed by length check), `session-manager.test.ts` (1 instance followed by count check)
|
||||
|
||||
**Phase 2 - CleanupManager & Debounce (2-3 days)** ✅ COMPLETE
|
||||
1. ✅ Create `Debouncer` utility class (~1 hour) — Created `src/utils/debouncer.ts` with `Debouncer` and `KeyedDebouncer` classes; exported from `src/utils/index.ts`
|
||||
2. ✅ Migrate all 8 files from manual debounce to Debouncer — `state-store.ts` (2 Debouncers), `push-store.ts` (1 Debouncer), `bash-tool-parser.ts` (1 Debouncer), `image-watcher.ts` (1 KeyedDebouncer), `subagent-watcher.ts` (2 KeyedDebouncers), `server.ts` (1 KeyedDebouncer for persist timers), `ralph-tracker.ts` (2 Debouncers replacing 4 manual fields: `_todoUpdateTimer`, `_loopUpdateTimer`, `_todoUpdatePending`, `_loopUpdatePending`)
|
||||
3. ✅ Migrate respawn-controller to CleanupManager — 10 manual timer fields replaced with single `CleanupManager` instance + `timerIds` Map. `startTrackedTimer()`/`cancelTrackedTimer()` preserved as wrappers for UI countdown display and timer events. `clearTimers()` uses dispose-and-recreate pattern for state transitions.
|
||||
4. ✅ Migrate server.ts timer cleanup to CleanupManager (~2 hours) — `private cleanup = new CleanupManager()` present; terminal batch timers and pending respawn starts left as manual Maps (complex lifecycle)
|
||||
5. ✅ Migrate remaining files — `bash-tool-parser.ts` (CleanupManager ✅), `subagent-watcher.ts` (CleanupManager ✅), `ralph-tracker.ts` (Debouncer ✅)
|
||||
|
||||
**Phase 3 - server.ts Route Extraction (3-4 days)** ✅ COMPLETE
|
||||
1. ✅ Created `src/web/routes/` with 12 domain route modules + index barrel (4,090 LOC total): session (909), system (768), ralph (533), plan (459), respawn (315), case, file, hook-event, mux, push, scheduled, team
|
||||
2. ✅ Created `src/web/middleware/auth.ts` (193 LOC) — Basic Auth, session cookies, rate limiting, security headers, CORS
|
||||
3. ✅ Created `src/web/ports/` with 7 typed port interfaces (142 LOC) — SessionPort, EventPort, RespawnPort, ConfigPort, InfraPort, AuthPort; routes declare dependencies via intersection types
|
||||
4. ✅ Created `src/web/route-helpers.ts` (154 LOC) — `findSessionOrFail()`, `formatUptime()`, `sanitizeHookData()`, `autoConfigureRalph()`
|
||||
5. ✅ Reduced `server.ts` from 6,736 → 2,697 LOC (60% reduction). Remaining LOC is justified infrastructure: session lifecycle, SSE broadcast engine, terminal batching, respawn integration, resource cleanup
|
||||
|
||||
**Phase 4 - Domain File Splitting (2-3 days)** ✅ COMPLETE
|
||||
1. ✅ Split `types.ts` into `src/types/` directory — 14 domain files (1,469 LOC total): common, session, task, app-state, respawn, ralph, api, lifecycle, run-summary, tools, teams, push, plan + index barrel. Original `types.ts` is now a 1-line re-export
|
||||
2. ✅ Split `ralph-tracker.ts` into 7 files (exceeded plan of 4) — ralph-tracker (2,391), ralph-plan-tracker (477), ralph-status-parser (552), ralph-fix-plan-watcher (366), ralph-stall-detector (166), ralph-config (153), ralph-loop (522)
|
||||
3. ✅ Split `respawn-controller.ts` into 5 files (exceeded plan of 3) — respawn-controller (3,228), respawn-health (229), respawn-metrics (229), respawn-patterns (131), respawn-adaptive-timing (134)
|
||||
4. ✅ Split `session.ts` into 6 files (exceeded plan of 3) — session (2,168), session-manager (298), session-auto-ops (284), session-cli-builder (132), session-task-cache (101), session-lifecycle-log (114)
|
||||
|
||||
**Phase 5 - Frontend Modularization (3-4 days)** ✅ COMPLETE
|
||||
1. ✅ Extracted `constants.js` (238 LOC) — shared constants, timing values, Z-index layers, `escapeHtml()`, `extractSyncSegments()`
|
||||
2. ✅ Extracted `mobile-handlers.js` (449 LOC) — `MobileDetection`, `KeyboardHandler`, `SwipeHandler`
|
||||
3. ✅ Extracted `voice-input.js` (853 LOC) — `DeepgramProvider`, `VoiceInput`
|
||||
4. ✅ Extracted `notification-manager.js` (445 LOC) — `NotificationManager` class (5-layer system)
|
||||
5. ✅ Extracted `keyboard-accessory.js` (279 LOC) — `KeyboardAccessoryBar`, `FocusTrap`
|
||||
6. ✅ Extracted `api-client.js` (70 LOC) — `_api()`, `_apiJson()`, `_apiPost()`, `_apiPut()`
|
||||
7. ✅ Extracted `subagent-windows.js` (1,119 LOC) — 13 subagent window methods
|
||||
8. ✅ Removed inlined xterm-zerolag-input copy → built to `vendor/xterm-zerolag-input.js` from `packages/xterm-zerolag-input/`
|
||||
9. ✅ Reduced `app.js` from ~15,200 → 11,473 LOC (24% reduction). All scripts loaded in correct dependency order in `index.html`
|
||||
|
||||
**Phase 6 - Config Consolidation (1 day)** ✅ COMPLETE
|
||||
1. ✅ Created 6 new domain-focused config files (better than plan's 2 generic files): `server-timing.ts` (13 constants), `auth-config.ts` (5 constants), `tunnel-config.ts` (8 constants), `terminal-limits.ts` (4 constants), `ai-defaults.ts` (3 constants), `team-config.ts` (3 constants)
|
||||
2. ✅ Total: 9 config files in `src/config/`, ~65 constants centralized
|
||||
3. ✅ Eliminated all cross-file duplicates: `STATS_COLLECTION_INTERVAL_MS` (was in 2 files), `timeout: 10000` (was 6× inline in hooks-config.ts → `HOOK_TIMEOUT_MS`), AI model string (was in 5 files → `AI_CHECK_MODEL`), `MAX_TRACKED_AGENTS` (was shadowed in subagent-watcher.ts)
|
||||
4. ✅ CLAUDE.md updated with config files table, import conventions, resource limits references
|
||||
|
||||
**Phase 7 - Test Infrastructure (2-3 days)** ✅ COMPLETE
|
||||
1. ✅ Created `test/mocks/` directory with 5 files (541 LOC): `mock-session.ts` (312), `mock-state-store.ts` (60), `mock-route-context.ts` (121), `test-helpers.ts` (37), `index.ts` (11 — barrel export)
|
||||
2. ✅ Consolidated MockSession into single shared definition — no duplicate class definitions remain (2 `vi.mock()`-based copies intentionally left in session-manager.test.ts and ralph-loop.test.ts)
|
||||
3. ✅ `respawn-test-utils.ts` converted to backward-compatibility shim — re-exports from `test/mocks/`, retains respawn-specific utilities (MockAiIdleChecker, TimeController, etc.)
|
||||
4. ✅ Created initial 3 route test files with 58 total tests: `session-routes.test.ts` (34 tests), `respawn-routes.test.ts` (13 tests), `system-routes.test.ts` (11 tests). Route test harness uses `app.inject()` — no real ports needed
|
||||
5. ✅ All 12 route modules now have dedicated test files in `test/routes/`: session, respawn, system, ralph, plan, push, team, mux, file, scheduled, hook-event, case
|
||||
|
||||
---
|
||||
|
||||
## Appendix: File Size Inventory (Post-Implementation)
|
||||
|
||||
### Before vs After
|
||||
|
||||
| File | Before | After | Change |
|
||||
|------|--------|-------|--------|
|
||||
| `src/web/server.ts` | 6,736 | 2,697 | **−60%** (routes, auth, ports extracted) |
|
||||
| `src/web/public/app.js` | 15,196 | 11,473 | **−24%** (8 modules extracted) |
|
||||
| `src/ralph-tracker.ts` | 3,905 | 2,391 | **−39%** (6 companion files extracted) |
|
||||
| `src/respawn-controller.ts` | 3,611 | 3,228 | **−11%** (4 companion files extracted) |
|
||||
| `src/session.ts` | 2,418 | 2,168 | **−10%** (5 companion files extracted) |
|
||||
| `src/types.ts` | 1,443 | 1 | **−99%** (14 domain files in `src/types/`) |
|
||||
|
||||
### New Infrastructure Created
|
||||
|
||||
| Directory | Files | Total LOC | Purpose |
|
||||
|-----------|-------|-----------|---------|
|
||||
| `src/web/routes/` | 13 | 4,090 | Domain route modules |
|
||||
| `src/web/ports/` | 7 | 142 | Port interfaces for DI |
|
||||
| `src/web/middleware/` | 1 | 193 | Auth middleware |
|
||||
| `src/types/` | 14 | 1,469 | Domain type files |
|
||||
| `src/config/` | 9 | ~450 | Centralized config |
|
||||
| `test/mocks/` | 5 | 541 | Shared test mocks |
|
||||
| `test/routes/` | 4 | ~500 | Route handler tests |
|
||||
|
||||
### Extracted Frontend Modules
|
||||
|
||||
| Module | Lines | Purpose |
|
||||
|--------|-------|---------|
|
||||
| `subagent-windows.js` | 1,119 | Subagent window management |
|
||||
| `voice-input.js` | 853 | DeepgramProvider, VoiceInput |
|
||||
| `mobile-handlers.js` | 449 | MobileDetection, KeyboardHandler, SwipeHandler |
|
||||
| `notification-manager.js` | 445 | 5-layer notification system |
|
||||
| `keyboard-accessory.js` | 279 | KeyboardAccessoryBar, FocusTrap |
|
||||
| `constants.js` | 238 | Shared constants, timing, Z-index |
|
||||
| `api-client.js` | 70 | API fetch wrapper |
|
||||
|
||||
### What's Working Well
|
||||
|
||||
These patterns should be **preserved, not refactored**:
|
||||
- Clean one-way dependency graph (no circular deps)
|
||||
- EventEmitter-based decoupling between domain models
|
||||
- Proper `import type` usage (19 files, consistent)
|
||||
- Utility type adoption (101 instances of Record, Partial, Omit, etc.)
|
||||
- `assertNever()` for exhaustive switch checking
|
||||
- `StaleExpirationMap` and `LRUMap` for bounded collections
|
||||
- State persistence circuit breaker pattern
|
||||
- TypeScript strict mode with all safety flags enabled
|
||||
- `CleanupManager` for centralized timer/watcher disposal
|
||||
- `Debouncer`/`KeyedDebouncer` for consistent debounce patterns
|
||||
- Port interfaces for route module dependency injection
|
||||
- `Object.assign(CodemanApp.prototype, ...)` for frontend module composition
|
||||
Reference in New Issue
Block a user