diff --git a/docs/code-structure-findings.md b/docs/code-structure-findings.md new file mode 100644 index 00000000..e4aa314b --- /dev/null +++ b/docs/code-structure-findings.md @@ -0,0 +1,951 @@ +# 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(); +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; +``` + +**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(/100KB). With 500 agents discovered at once, that's 50MB of file reads. -- **Fix**: Only read last 10KB for display (tail). Full file on-demand only (e.g., when user opens transcript viewer). -- **Impact**: Reduces file I/O from 50MB to 5MB for bulk agent discovery. +### 4.4 Stream transcript files instead of full reads — DONE +- **Files**: `src/subagent-watcher.ts` (`tailFile()`, `findDescriptionInAgentFile()`, parent transcript lookup) +- **Change**: Multiple streaming strategies implemented: + - **Live monitoring**: Position-based `tailFile()` with `createReadStream({ start: fromPosition })` — only reads new content + - **Parent transcript lookup**: Streams only last 16KB (`createReadStream({ start: offset })`) + - **Description extraction**: Streams only first 8KB, exits early after 5 lines + - **Full read**: Only for on-demand transcript review panel (with optional `limit` parameter) +- **Impact**: File I/O for bulk agent discovery reduced from ~50MB to ~5MB. --- -## Phase 5: Long-Term Architectural (Optional) +## Phase 5: Long-Term Architectural (Optional) — NOT STARTED + +These items are deferred until scaling demands justify the complexity. ### 5.1 Worker thread for PTY processing - **Files**: `src/session.ts` @@ -134,42 +136,23 @@ All Phase 1 items were found to already exist in the codebase during verificatio --- -## Priority Matrix (Remaining Work) +## Completion Summary -| # | Item | Impact | Risk | Effort | -|---|------|--------|------|--------| -| 3.1 | State diff broadcasts | **Very High** | Medium | 3-4h | -| 3.2 | Fix session cache invalidation | **High** | Low | 1h | -| 3.3 | Skip PTY processing for hidden sessions | **High** | Medium | 2-3h | -| 3.5 | Throttle detection broadcasts | **Medium** | Low | 1h | -| 3.4 | Batch liveness checks | **Medium** | Low | 1-2h | -| 4.1 | Incremental state persistence | **Medium** | Medium | 3-4h | -| 4.2 | Team watcher fs events | **Low-Med** | Medium | 2h | -| 4.3 | Consolidate file watchers | **Low-Med** | Medium | 2h | -| 4.4 | Stream transcripts | **Low-Med** | Low | 1h | -| 5.1 | Worker thread PTY | **Med** (at scale) | High | 8h | -| 5.2 | Per-session SSE subs | **Med** (at scale) | High | 4h | -| 5.3 | O(1) LRUMap | **Very Low** | Medium | 2h | +| Phase | Scope | Status | Items | +|-------|-------|--------|-------| +| 1 | Quick Wins | **Complete** | 5/5 (all pre-existing) | +| 2 | Frontend Responsiveness | **Complete** | 3/3 actionable done, 2 skipped | +| 3 | Backend Hot Paths | **Complete** | 4/4 actionable done, 1 deferred | +| 4 | System-Level | **Complete** | 4/4 done | +| 5 | Long-Term Architectural | **Not started** | 0/3 — deferred until needed | ---- - -## Recommended Execution Order - -**Sprint 1** (Phase 3 — Backend Hot Paths): Items 3.1, 3.2, 3.3, 3.5 -- Backend serialization and broadcast efficiency -- Highest remaining impact; requires careful testing with multiple active sessions - -**Sprint 2** (Phase 4 — System Level): Items 4.1, 3.4, 4.3, 4.4 -- State persistence, liveness checks, watcher consolidation -- Medium-complexity refactors - -**Sprint 3** (Phase 5 — Architectural): Items 5.1, 5.2 — only if scaling demands it +**Overall**: 16/16 actionable items complete. 3 optional items deferred. --- ## Measurement -Before starting implementation, establish baselines: +Before starting Phase 5, establish baselines: 1. **Frontend**: Record Chrome DevTools Performance trace with 10 sessions open. Measure: - Frame rate during rapid terminal output diff --git a/docs/phase1-implementation-plan.md b/docs/phase1-implementation-plan.md new file mode 100644 index 00000000..ce7c06eb --- /dev/null +++ b/docs/phase1-implementation-plan.md @@ -0,0 +1,738 @@ +# Phase 1 Implementation Plan: Quick Wins + +**Source**: `docs/code-structure-findings.md` (Phase 1 - Quick Wins section) +**Estimated effort**: 1-2 days +**Tasks**: 5 independent tasks (can be done in parallel unless noted) + +--- + +## Safety Constraints + +Before starting ANY work, read and follow these rules: + +1. **Never run `npx vitest run`** (full suite) -- it kills tmux sessions. You are running inside a Codeman-managed tmux session. +2. **Run individual tests only**: `npx vitest run test/.test.ts` +3. **Never test on port 3000** -- the live dev server runs there. Tests use ports 3150+. +4. **After TypeScript changes**: Run `tsc --noEmit` to verify type checking passes. +5. **Before considering done**: Run `npm run lint` and `npm run format:check` to ensure CI passes. +6. **Never kill tmux sessions** -- check `echo $CODEMAN_MUX` first. + +--- + +## Task Dependencies + +All 5 tasks are independent and can be done in parallel. However: +- Task 1 (barrel exports) is a prerequisite if you want to update import sites to use the barrel after Task 3 (consolidate EXEC_TIMEOUT_MS). The EXEC_TIMEOUT_MS consolidation creates a new export that should be added to the barrel. +- Task 2 (delete dead functions) removes functions that Task 1 would otherwise need to add to the barrel. Do Task 2 first or simultaneously with Task 1 to avoid adding exports for dead code. + +**Recommended order**: Task 2 -> Task 1 -> Task 3 -> Task 4 -> Task 5 + +--- + +## Task 1: Export Missing Functions from Utils Barrel + +**File**: `src/utils/index.ts` +**Time**: ~30 minutes + +### Problem + +The barrel file (`src/utils/index.ts`) is missing exports for several functions that are defined in util modules, forcing consumers to use deep imports or preventing usage entirely. + +### Missing Exports + +From `src/utils/regex-patterns.ts`: +- `createAnsiPatternFull()` -- factory for fresh ANSI regex (documented in CLAUDE.md) +- `createAnsiPatternSimple()` -- factory for fresh ANSI regex (documented in CLAUDE.md) +- `stripAnsi()` -- ANSI stripping utility +- `SAFE_PATH_PATTERN` -- regex for safe file paths (currently deep-imported by `schemas.ts` and `tmux-manager.ts`) + +From `src/utils/token-validation.ts`: +- `validateTokenCounts()` -- token count validation (documented in CLAUDE.md) +- `validateTokensAndCost()` -- token + cost validation (documented in CLAUDE.md) + +**Note**: Do NOT export `isSimilar`, `isSimilarByDistance`, `levenshteinDistance`, or `normalizePhrase` from `string-similarity.ts` -- these are dead code (see Task 2). + +### Edit 1: Add missing regex-patterns exports + +**File**: `src/utils/index.ts` + +**Old code** (lines 13-18): +```typescript +export { + ANSI_ESCAPE_PATTERN_FULL, + ANSI_ESCAPE_PATTERN_SIMPLE, + TOKEN_PATTERN, + SPINNER_PATTERN, +} from './regex-patterns.js'; +``` + +**New code**: +```typescript +export { + ANSI_ESCAPE_PATTERN_FULL, + ANSI_ESCAPE_PATTERN_SIMPLE, + TOKEN_PATTERN, + SPINNER_PATTERN, + createAnsiPatternFull, + createAnsiPatternSimple, + stripAnsi, + SAFE_PATH_PATTERN, +} from './regex-patterns.js'; +``` + +### Edit 2: Add missing token-validation exports + +**File**: `src/utils/index.ts` + +**Old code** (line 19): +```typescript +export { MAX_SESSION_TOKENS } from './token-validation.js'; +``` + +**New code**: +```typescript +export { MAX_SESSION_TOKENS, validateTokenCounts, validateTokensAndCost } from './token-validation.js'; +``` + +### Optional follow-up: Update deep imports to use barrel + +These files currently deep-import `SAFE_PATH_PATTERN` and could be updated to use the barrel instead: + +- `src/web/schemas.ts` line 11: `import { SAFE_PATH_PATTERN } from '../utils/regex-patterns.js';` could become `import { SAFE_PATH_PATTERN } from '../utils/index.js';` +- `src/tmux-manager.ts` line 44: `import { SAFE_PATH_PATTERN } from './utils/regex-patterns.js';` could become part of existing barrel import + +This is a low-priority cosmetic change. The barrel export itself is the important fix. + +### Verification + +```bash +tsc --noEmit +npm run lint +``` + +--- + +## Task 2: Delete Dead Utility Functions + +**File**: `src/utils/string-similarity.ts` +**Time**: ~15 minutes + +### Problem + +Four exported functions in `string-similarity.ts` are never imported anywhere in the codebase: +- `levenshteinDistance()` (lines 27-69) +- `isSimilar()` (lines 106-108) +- `isSimilarByDistance()` (lines 123-125) +- `normalizePhrase()` (lines 139-144) + +Only three functions are actually used (all by `ralph-tracker.ts` via the barrel): +- `stringSimilarity()` -- uses `levenshteinDistance()` internally +- `fuzzyPhraseMatch()` -- uses `normalizePhrase()` and `isSimilarByDistance()` internally +- `todoContentHash()` + +### Strategy + +`levenshteinDistance()` is called by `stringSimilarity()`, and `normalizePhrase()` and `isSimilarByDistance()` are called by `fuzzyPhraseMatch()`. So they cannot be deleted -- they just need to be un-exported (made private to the module). + +`isSimilar()` is truly dead -- not called by anything. Delete it entirely. + +### Edit 1: Remove `export` from `levenshteinDistance` + +**File**: `src/utils/string-similarity.ts` + +**Old code** (line 27): +```typescript +export function levenshteinDistance(a: string, b: string): number { +``` + +**New code**: +```typescript +function levenshteinDistance(a: string, b: string): number { +``` + +### Edit 2: Delete `isSimilar` function entirely + +**File**: `src/utils/string-similarity.ts` + +**Old code** (lines 94-108): +```typescript +/** + * Check if two strings are similar within a given threshold. + * + * @param a - First string + * @param b - Second string + * @param threshold - Minimum similarity ratio (default: 0.85 = 85% similar) + * @returns True if similarity >= threshold + * + * @example + * isSimilar('COMPLETE', 'COMPLET', 0.85) // true (87.5% similar) + * isSimilar('COMPLETE', 'DONE', 0.85) // false (0% similar) + */ +export function isSimilar(a: string, b: string, threshold = 0.85): boolean { + return stringSimilarity(a, b) >= threshold; +} +``` + +**New code**: (delete entirely -- replace with empty string) + +### Edit 3: Remove `export` from `isSimilarByDistance` + +**File**: `src/utils/string-similarity.ts` + +**Old code** (line 123): +```typescript +export function isSimilarByDistance(a: string, b: string, maxDistance = 2): boolean { +``` + +**New code**: +```typescript +function isSimilarByDistance(a: string, b: string, maxDistance = 2): boolean { +``` + +### Edit 4: Remove `export` from `normalizePhrase` + +**File**: `src/utils/string-similarity.ts` + +**Old code** (line 139): +```typescript +export function normalizePhrase(phrase: string): string { +``` + +**New code**: +```typescript +function normalizePhrase(phrase: string): string { +``` + +### Verification + +```bash +tsc --noEmit +npx vitest run test/string-utilities.test.ts +npm run lint +``` + +Note: If `test/string-utilities.test.ts` imports any of the now-unexported functions, those test imports will fail. Check the test file and remove tests for `isSimilar` (deleted) and update any direct tests for `levenshteinDistance`, `isSimilarByDistance`, `normalizePhrase` to test them indirectly through the public API (`stringSimilarity`, `fuzzyPhraseMatch`), or remove those tests. + +--- + +## Task 3: Consolidate Duplicated `EXEC_TIMEOUT_MS` Constant + +**Files**: +- `src/utils/claude-cli-resolver.ts` (line 17) +- `src/utils/opencode-cli-resolver.ts` (line 16) +- `src/tmux-manager.ts` (line 63) -- also has its own copy + +**Time**: ~15 minutes + +### Problem + +`EXEC_TIMEOUT_MS = 5000` is defined identically in three files. Changes need to happen in all three places. + +### Strategy + +Create a shared constant and export it. The natural home is a new config file since the existing config files (`buffer-limits.ts`, `map-limits.ts`) follow this pattern. However, to keep it minimal, we can add it to an existing config file or create a small one. + +**Recommended approach**: Add to `src/config/timing-config.ts` (new file) as a single constant. This file can grow later in Phase 6 to hold other timing constants. + +Alternatively, the simplest approach: export from one of the existing utils and import in the others. Since both CLI resolvers are in `src/utils/`, the cleanest approach is to put it in a shared location. + +### Option A: Add to existing config (simpler) + +Create `src/config/exec-timeout.ts`: + +**New file**: `src/config/exec-timeout.ts` +```typescript +/** + * Timeout for child process exec commands (e.g., `which claude`, `which opencode`, tmux commands). + * Used across CLI resolvers and tmux manager. + */ +export const EXEC_TIMEOUT_MS = 5000; +``` + +### Edit 1: Update `claude-cli-resolver.ts` + +**File**: `src/utils/claude-cli-resolver.ts` + +**Old code** (lines 11-17): +```typescript +import { execSync } from 'node:child_process'; +import { existsSync } from 'node:fs'; +import { delimiter, dirname, join } from 'node:path'; +import { homedir } from 'node:os'; + +/** Timeout for exec commands (5 seconds) */ +const EXEC_TIMEOUT_MS = 5000; +``` + +**New code**: +```typescript +import { execSync } from 'node:child_process'; +import { existsSync } from 'node:fs'; +import { delimiter, dirname, join } from 'node:path'; +import { homedir } from 'node:os'; +import { EXEC_TIMEOUT_MS } from '../config/exec-timeout.js'; +``` + +### Edit 2: Update `opencode-cli-resolver.ts` + +**File**: `src/utils/opencode-cli-resolver.ts` + +**Old code** (lines 10-16): +```typescript +import { execSync } from 'node:child_process'; +import { existsSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { homedir } from 'node:os'; + +/** Timeout for exec commands (5 seconds) */ +const EXEC_TIMEOUT_MS = 5000; +``` + +**New code**: +```typescript +import { execSync } from 'node:child_process'; +import { existsSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { homedir } from 'node:os'; +import { EXEC_TIMEOUT_MS } from '../config/exec-timeout.js'; +``` + +### Edit 3: Update `tmux-manager.ts` + +**File**: `src/tmux-manager.ts` + +**Old code** (line 63): +```typescript +const EXEC_TIMEOUT_MS = 5000; +``` + +**New code**: +```typescript +import { EXEC_TIMEOUT_MS } from './config/exec-timeout.js'; +``` + +Note: `tmux-manager.ts` already has many imports at the top of the file. Add this import near the other local imports (around lines 43-56). The `const EXEC_TIMEOUT_MS = 5000;` on line 63 should be deleted entirely (replaced with the import). + +### Verification + +```bash +tsc --noEmit +npm run lint +``` + +--- + +## Task 4: Add `z.infer` to Zod Schemas + +**Files**: +- `src/web/schemas.ts` (add type exports) +- `src/types.ts` (replace manual interfaces with `z.infer` re-exports where applicable) + +**Time**: ~2 hours + +### Problem + +All 30+ Zod schemas in `schemas.ts` define validation rules, but zero use `z.infer` to derive TypeScript types. Instead, `types.ts` manually duplicates interfaces that match the schemas. When a schema changes, the type must be manually updated too. + +### Strategy + +Add `z.infer` type exports to `schemas.ts` for each exported schema. This creates derived types as the single source of truth. For schemas that have corresponding manual interfaces in `types.ts`, the manual interface can be replaced with a re-export of the inferred type. + +**Important**: Not all schemas have matching interfaces in `types.ts`. The `RespawnConfig` interface in `types.ts` (line 395) has all required fields, while `RespawnConfigSchema` has all optional fields (it's for partial updates). These are NOT the same type and should NOT be unified. + +### Edit 1: Add inferred type exports to `schemas.ts` + +**File**: `src/web/schemas.ts` + +After each schema definition, add a corresponding type export. Add the following lines at the **end of the file** (after line 509): + +**Old code** (end of file, lines 506-509): +```typescript + .optional(), +}); +``` + +Wait -- the end of file is actually at line 509 after the `RalphLoopStartSchema`. Add the type exports after the last schema: + +**Append to end of file** `src/web/schemas.ts`: + +```typescript + +// ========== Inferred Types ========== +// Derive TypeScript types from Zod schemas (single source of truth) + +export type CreateSessionInput = z.infer; +export type RunPromptInput = z.infer; +export type ResizeInput = z.infer; +export type CreateCaseInput = z.infer; +export type QuickStartInput = z.infer; +export type HookEventInput = z.infer; +export type RespawnConfigInput = z.infer; +export type ConfigUpdateInput = z.infer; +export type SettingsUpdateInput = z.infer; +export type SessionInputWithLimitInput = z.infer; +export type SessionNameInput = z.infer; +export type SessionColorInput = z.infer; +export type RalphConfigInput = z.infer; +export type FixPlanImportInput = z.infer; +export type RalphPromptWriteInput = z.infer; +export type AutoClearInput = z.infer; +export type AutoCompactInput = z.infer; +export type ImageWatcherInput = z.infer; +export type FlickerFilterInput = z.infer; +export type QuickRunInput = z.infer; +export type ScheduledRunInput = z.infer; +export type LinkCaseInput = z.infer; +export type GeneratePlanInput = z.infer; +export type GeneratePlanDetailedInput = z.infer; +export type CancelPlanInput = z.infer; +export type PlanTaskUpdateInput = z.infer; +export type PlanTaskAddInput = z.infer; +export type CpuLimitInput = z.infer; +export type SubagentWindowStatesInput = z.infer; +export type SubagentParentMapInput = z.infer; +export type InteractiveRespawnInput = z.infer; +export type RespawnEnableInput = z.infer; +export type PushSubscribeInput = z.infer; +export type PushPreferencesUpdateInput = z.infer; +export type RalphLoopStartInput = z.infer; +``` + +### What NOT to do + +Do NOT replace the `RespawnConfig` interface in `types.ts` with `z.infer`. The schema has all optional fields (for partial config updates), but the interface has required fields (for the full config object). These are intentionally different shapes. + +Similarly, do NOT try to unify every interface in `types.ts` with a schema -- most interfaces in `types.ts` represent internal domain objects (SessionState, TaskState, etc.) that have no corresponding Zod schema. The schemas only exist for API request validation. + +### Future opportunity + +In a future phase, route handlers in `server.ts` can use these inferred types for request body typing: +```typescript +const body = CreateSessionSchema.parse(request.body) as CreateSessionInput; +``` +This task only adds the type exports. Migrating route handlers to use them is out of scope. + +### Verification + +```bash +tsc --noEmit +npm run lint +npm run format:check +``` + +--- + +## Task 5: Fix Weak `not.toThrow()` Tests with Behavioral Assertions + +**Files**: +- `test/task-tracker.test.ts` -- 6 instances +- `test/image-watcher.test.ts` -- 1 instance +- `test/task-queue.test.ts` -- 1 instance +- `test/hooks-config.test.ts` -- 1 instance +- `test/session-manager.test.ts` -- 1 instance + +**Time**: ~1 hour + +### Problem + +10 tests only assert `not.toThrow()` without verifying the actual defensive behavior. These tests prove the code doesn't crash but don't verify it does the right thing. + +### Fix Strategy + +After each `not.toThrow()`, add a behavioral assertion that verifies the state is correct (e.g., no tasks were created, no side effects occurred). + +### Edit 1: `task-tracker.test.ts` -- null message (line 566) + +**File**: `test/task-tracker.test.ts` + +**Old code**: +```typescript + it('should handle null message', () => { + expect(() => tracker.processMessage(null)).not.toThrow(); + }); +``` + +**New code**: +```typescript + it('should handle null message', () => { + expect(() => tracker.processMessage(null)).not.toThrow(); + expect(tracker.getAllTasks().size).toBe(0); + expect(tracker.getRunningCount()).toBe(0); + }); +``` + +### Edit 2: `task-tracker.test.ts` -- message without content (line 569-571) + +**File**: `test/task-tracker.test.ts` + +**Old code**: +```typescript + it('should handle message without content', () => { + expect(() => tracker.processMessage({ message: {} })).not.toThrow(); + }); +``` + +**New code**: +```typescript + it('should handle message without content', () => { + expect(() => tracker.processMessage({ message: {} })).not.toThrow(); + expect(tracker.getAllTasks().size).toBe(0); + }); +``` + +### Edit 3: `task-tracker.test.ts` -- empty content array (line 573-575) + +**File**: `test/task-tracker.test.ts` + +**Old code**: +```typescript + it('should handle empty content array', () => { + expect(() => tracker.processMessage({ message: { content: [] } })).not.toThrow(); + }); +``` + +**New code**: +```typescript + it('should handle empty content array', () => { + expect(() => tracker.processMessage({ message: { content: [] } })).not.toThrow(); + expect(tracker.getAllTasks().size).toBe(0); + }); +``` + +### Edit 4: `task-tracker.test.ts` -- tool_result for unknown task (lines 577-590) + +**File**: `test/task-tracker.test.ts` + +**Old code**: +```typescript + it('should handle tool_result for unknown task', () => { + expect(() => { + tracker.processMessage({ + message: { + content: [{ + type: 'tool_result', + tool_use_id: 'unknown-task', + is_error: false, + content: 'Done', + }], + }, + }); + }).not.toThrow(); + }); +``` + +**New code**: +```typescript + it('should handle tool_result for unknown task', () => { + expect(() => { + tracker.processMessage({ + message: { + content: [{ + type: 'tool_result', + tool_use_id: 'unknown-task', + is_error: false, + content: 'Done', + }], + }, + }); + }).not.toThrow(); + expect(tracker.getTask('unknown-task')).toBeUndefined(); + expect(tracker.getAllTasks().size).toBe(0); + }); +``` + +### Edit 5: `task-tracker.test.ts` -- empty terminal output (lines 592-595) + +**File**: `test/task-tracker.test.ts` + +**Old code**: +```typescript + it('should handle empty terminal output', () => { + expect(() => tracker.processTerminalOutput('')).not.toThrow(); + expect(() => tracker.processTerminalOutput(' ')).not.toThrow(); + }); +``` + +**New code**: +```typescript + it('should handle empty terminal output', () => { + expect(() => tracker.processTerminalOutput('')).not.toThrow(); + expect(() => tracker.processTerminalOutput(' ')).not.toThrow(); + expect(tracker.getAllTasks().size).toBe(0); + expect(tracker.getRunningCount()).toBe(0); + }); +``` + +### Edit 6: `image-watcher.test.ts` -- unwatchSession for non-watched session (line 123) + +**File**: `test/image-watcher.test.ts` + +**Old code**: +```typescript + it('should be safe to call for non-watched session', () => { + expect(() => watcher.unwatchSession('nonexistent')).not.toThrow(); + }); +``` + +**New code**: +```typescript + it('should be safe to call for non-watched session', () => { + expect(() => watcher.unwatchSession('nonexistent')).not.toThrow(); + expect(watcher.getWatchedSessions()).toHaveLength(0); + }); +``` + +### Edit 7: `task-queue.test.ts` -- dependencies on non-existent tasks (lines 538-542) + +**File**: `test/task-queue.test.ts` + +**Old code**: +```typescript + it('should allow dependencies on non-existent tasks (just unsatisfied, not a cycle)', () => { + // Dependencies on non-existent tasks are valid - they just won't be satisfied + expect(() => { + queue.addTask({ prompt: 'Task D', dependencies: ['non-existent-id'] }); + }).not.toThrow(); + }); +``` + +**New code**: +```typescript + it('should allow dependencies on non-existent tasks (just unsatisfied, not a cycle)', () => { + // Dependencies on non-existent tasks are valid - they just won't be satisfied + let task: ReturnType | undefined; + expect(() => { + task = queue.addTask({ prompt: 'Task D', dependencies: ['non-existent-id'] }); + }).not.toThrow(); + expect(task).toBeDefined(); + expect(task!.dependencies).toEqual(['non-existent-id']); + // Task should be pending but blocked (dependency unsatisfied) + expect(queue.next()?.prompt).toBeUndefined(); + }); +``` + +Wait -- `queue.next()` returns `null` when no next task is available (all blocked). Let me adjust: + +**New code** (corrected): +```typescript + it('should allow dependencies on non-existent tasks (just unsatisfied, not a cycle)', () => { + // Dependencies on non-existent tasks are valid - they just won't be satisfied + let task: ReturnType | undefined; + expect(() => { + task = queue.addTask({ prompt: 'Task D', dependencies: ['non-existent-id'] }); + }).not.toThrow(); + expect(task).toBeDefined(); + expect(task!.dependencies).toEqual(['non-existent-id']); + // Task exists but is blocked (dependency unsatisfied), so next() skips it + expect(queue.getAllTasks()).toHaveLength(1); + expect(queue.next()).toBeNull(); + }); +``` + +### Edit 8: `hooks-config.test.ts` -- valid JSON check (line 129) + +**File**: `test/hooks-config.test.ts` + +**Old code**: +```typescript + it('should write valid JSON', () => { + writeHooksConfig(testDir); + const settingsPath = join(testDir, '.claude', 'settings.local.json'); + const content = readFileSync(settingsPath, 'utf-8'); + expect(() => JSON.parse(content)).not.toThrow(); + }); +``` + +**New code**: +```typescript + it('should write valid JSON', () => { + writeHooksConfig(testDir); + const settingsPath = join(testDir, '.claude', 'settings.local.json'); + const content = readFileSync(settingsPath, 'utf-8'); + const parsed = JSON.parse(content); + expect(parsed).toBeDefined(); + expect(typeof parsed).toBe('object'); + expect(parsed.hooks).toBeDefined(); + }); +``` + +### Edit 9: `session-manager.test.ts` -- stopSession for non-existent (line 216) + +**File**: `test/session-manager.test.ts` + +**Old code**: +```typescript + it('should handle non-existent session gracefully', async () => { + await expect(manager.stopSession('non-existent')).resolves.not.toThrow(); + }); +``` + +**New code**: +```typescript + it('should handle non-existent session gracefully', async () => { + await expect(manager.stopSession('non-existent')).resolves.not.toThrow(); + expect(manager.getSessionCount()).toBe(0); + }); +``` + +### Verification + +Run each test file individually: + +```bash +npx vitest run test/task-tracker.test.ts +npx vitest run test/image-watcher.test.ts +npx vitest run test/task-queue.test.ts +npx vitest run test/hooks-config.test.ts +npx vitest run test/session-manager.test.ts +``` + +**Important**: `hooks-config.test.ts` and `session-manager.test.ts` spawn real servers on ports 3130-3131. Only run them if you are NOT running other tests that use those ports. + +--- + +## Final Verification Checklist + +After all 5 tasks are complete, run the following in order: + +```bash +# 1. TypeScript type checking +tsc --noEmit + +# 2. Linting +npm run lint + +# 3. Formatting +npm run format:check + +# 4. Run affected test files individually (NOT the full suite) +npx vitest run test/string-utilities.test.ts +npx vitest run test/task-tracker.test.ts +npx vitest run test/image-watcher.test.ts +npx vitest run test/task-queue.test.ts +npx vitest run test/session-manager.test.ts +npx vitest run test/hooks-config.test.ts +``` + +If any formatting issues arise, fix with: +```bash +npm run format +``` + +If any lint issues arise, fix with: +```bash +npm run lint:fix +``` + +### Summary of Changes + +| Task | Files Modified | Files Created | +|------|---------------|---------------| +| 1. Barrel exports | `src/utils/index.ts` | -- | +| 2. Dead functions | `src/utils/string-similarity.ts` | -- | +| 3. EXEC_TIMEOUT_MS | `src/utils/claude-cli-resolver.ts`, `src/utils/opencode-cli-resolver.ts`, `src/tmux-manager.ts` | `src/config/exec-timeout.ts` | +| 4. z.infer types | `src/web/schemas.ts` | -- | +| 5. Weak tests | `test/task-tracker.test.ts`, `test/image-watcher.test.ts`, `test/task-queue.test.ts`, `test/hooks-config.test.ts`, `test/session-manager.test.ts` | -- | + +**Total files modified**: 10 +**Total files created**: 1 diff --git a/docs/phase2-implementation-plan.md b/docs/phase2-implementation-plan.md new file mode 100644 index 00000000..9785a606 --- /dev/null +++ b/docs/phase2-implementation-plan.md @@ -0,0 +1,1026 @@ +# Phase 2 Implementation Plan: CleanupManager Adoption & Debounce Consolidation + +**Source**: `docs/code-structure-findings.md` (Phase 2 — CleanupManager & Debounce) +**Estimated effort**: 2-3 days +**Tasks**: 5 tasks with dependencies (see dependency graph below) + +--- + +## Safety Constraints + +Before starting ANY work, read and follow these rules: + +1. **Never run `npx vitest run`** (full suite) — it kills tmux sessions. You are running inside a Codeman-managed tmux session. +2. **Run individual tests only**: `npx vitest run test/.test.ts` +3. **Never test on port 3000** — the live dev server runs there. Tests use ports 3150+. +4. **After TypeScript changes**: Run `tsc --noEmit` to verify type checking passes. +5. **Before considering done**: Run `npm run lint` and `npm run format:check` to ensure CI passes. +6. **Never kill tmux sessions** — check `echo $CODEMAN_MUX` first. + +--- + +## Task Dependencies + +``` +Task 1 (Debouncer utility) + └──> Task 2 (Migrate 8 files to Debouncer) + └──> Task 3 (Migrate respawn-controller to CleanupManager) + └──> Task 4 (Migrate server.ts to CleanupManager) + └──> Task 5 (Migrate remaining files to CleanupManager) +``` + +**Task 1** must complete first — the other tasks depend on the `Debouncer` class. +**Tasks 3, 4, 5** are independent of each other and can run in parallel after Task 2. + +--- + +## Task 1: Create `Debouncer` Utility Class + +**File to create**: `src/utils/debouncer.ts` +**File to edit**: `src/utils/index.ts` (add barrel export) +**Time**: ~1 hour + +### Problem + +8+ files implement debounce independently with an identical 3-step pattern: + +```typescript +// Repeated everywhere: +private saveTimer: NodeJS.Timeout | null = null; +debouncedSave() { + if (this.saveTimer) clearTimeout(this.saveTimer); + this.saveTimer = setTimeout(() => this.save(), 500); +} +stop() { + if (this.saveTimer) { clearTimeout(this.saveTimer); this.saveTimer = null; } +} +``` + +Two variants exist: +1. **Single debouncer** — one timer field per operation (state-store, push-store, ralph-tracker) +2. **Keyed debouncer** — a `Map` for per-key debouncing (image-watcher, subagent-watcher, server.ts terminal batching, server.ts persist debounce) + +### Implementation + +Create two classes: `Debouncer` for single-key and `KeyedDebouncer` for per-key patterns. + +**New file**: `src/utils/debouncer.ts` + +```typescript +/** + * @fileoverview Debounce utilities to replace manual timer management. + * + * Two variants: + * - `Debouncer` — single debounced operation (replaces timer + clearTimeout pattern) + * - `KeyedDebouncer` — per-key debouncing (replaces Map pattern) + * + * Both integrate with CleanupManager via dispose(). + * + * @module utils/debouncer + */ + +/** + * Single-operation debouncer. + * + * Replaces the common pattern of: + * ``` + * private timer: NodeJS.Timeout | null = null; + * debounce(fn) { if (this.timer) clearTimeout(this.timer); this.timer = setTimeout(fn, delay); } + * cancel() { if (this.timer) { clearTimeout(this.timer); this.timer = null; } } + * ``` + * + * @example + * ```typescript + * private saveDeb = new Debouncer(500); + * + * onChange() { + * this.saveDeb.schedule(() => this.save()); + * } + * + * stop() { + * this.saveDeb.dispose(); + * } + * ``` + */ +export class Debouncer { + private timer: NodeJS.Timeout | null = null; + + constructor(private readonly delayMs: number) {} + + /** + * Schedule a debounced callback. Resets the timer on each call. + * If a previous call is pending, it is cancelled. + */ + schedule(fn: () => void): void { + this.cancel(); + this.timer = setTimeout(() => { + this.timer = null; + fn(); + }, this.delayMs); + } + + /** Cancel any pending execution without invoking the callback. */ + cancel(): void { + if (this.timer) { + clearTimeout(this.timer); + this.timer = null; + } + } + + /** Whether a callback is currently pending. */ + get isPending(): boolean { + return this.timer !== null; + } + + /** + * Cancel pending callback and flush immediately. + * Useful for shutdown: cancel the timer but run the action now. + * + * @param fn - The flush function to run (typically the same function passed to schedule) + */ + flush(fn: () => void): void { + this.cancel(); + fn(); + } + + /** Alias for cancel() — matches CleanupManager/Disposable convention. */ + dispose(): void { + this.cancel(); + } +} + +/** + * Per-key debouncer for operations that need independent timers per resource. + * + * Replaces the common pattern of: + * ``` + * private timers = new Map(); + * debounce(key, fn) { + * const existing = this.timers.get(key); + * if (existing) clearTimeout(existing); + * this.timers.set(key, setTimeout(() => { this.timers.delete(key); fn(); }, delay)); + * } + * ``` + * + * @example + * ```typescript + * private fileDebouncers = new KeyedDebouncer(100); + * + * onFileChange(path: string) { + * this.fileDebouncers.schedule(path, () => this.processFile(path)); + * } + * + * stop() { + * this.fileDebouncers.dispose(); + * } + * ``` + */ +export class KeyedDebouncer { + private timers = new Map(); + + constructor(private readonly delayMs: number) {} + + /** + * Schedule a debounced callback for a specific key. + * Each key has its own independent timer. + */ + schedule(key: string, fn: () => void): void { + this.cancelKey(key); + this.timers.set( + key, + setTimeout(() => { + this.timers.delete(key); + fn(); + }, this.delayMs) + ); + } + + /** Cancel a pending callback for a specific key. */ + cancelKey(key: string): void { + const existing = this.timers.get(key); + if (existing) { + clearTimeout(existing); + this.timers.delete(key); + } + } + + /** Whether a callback is pending for a specific key. */ + has(key: string): boolean { + return this.timers.has(key); + } + + /** Number of active timers. */ + get size(): number { + return this.timers.size; + } + + /** Get all currently active keys. */ + keys(): IterableIterator { + return this.timers.keys(); + } + + /** Cancel all pending callbacks. */ + dispose(): void { + for (const timer of this.timers.values()) { + clearTimeout(timer); + } + this.timers.clear(); + } + + /** + * Cancel all pending callbacks and run a flush function for each active key. + * Useful for shutdown: cancel timers but run the action for each pending key. + * + * @param fn - Called once per active key with the key as argument + */ + flushAll(fn: (key: string) => void): void { + const keys = Array.from(this.timers.keys()); + this.dispose(); + for (const key of keys) { + fn(key); + } + } +} +``` + +### Edit: Add barrel export + +**File**: `src/utils/index.ts` + +Add after the `CleanupManager` export line: + +```typescript +export { Debouncer, KeyedDebouncer } from './debouncer.js'; +``` + +### Verification + +```bash +tsc --noEmit +npm run lint +npm run format:check +``` + +--- + +## Task 2: Migrate 8 Files from Manual Debounce to Debouncer + +**Time**: ~3 hours + +Migrate all manual debounce patterns to use the new `Debouncer` and `KeyedDebouncer` classes. Each file migration is independent — verify with `tsc --noEmit` after each one. + +### 2.1: `src/state-store.ts` — 2 Debouncers + +**Current** (lines 63, 71, 151-160, 245-248): +```typescript +private saveTimeout: NodeJS.Timeout | null = null; +private ralphStateSaveTimeout: NodeJS.Timeout | null = null; + +save(): void { + this.dirty = true; + if (this.saveTimeout) return; + this.saveTimeout = setTimeout(() => { ... }, SAVE_DEBOUNCE_MS); +} + +// In _doSaveAsync(): +if (this.saveTimeout) { clearTimeout(this.saveTimeout); this.saveTimeout = null; } +``` + +**New**: +```typescript +import { Debouncer } from './utils/index.js'; + +private saveDeb = new Debouncer(SAVE_DEBOUNCE_MS); +private ralphStateSaveDeb = new Debouncer(SAVE_DEBOUNCE_MS); +``` + +**Edits required**: + +1. **Replace `saveTimeout` field** (line 63): Delete `private saveTimeout: NodeJS.Timeout | null = null;`, replace with `private saveDeb = new Debouncer(SAVE_DEBOUNCE_MS);` +2. **Replace `ralphStateSaveTimeout` field** (line 71): Delete `private ralphStateSaveTimeout: NodeJS.Timeout | null = null;`, replace with `private ralphStateSaveDeb = new Debouncer(SAVE_DEBOUNCE_MS);` +3. **Update `save()` method** (lines 151-161): Replace the manual timer logic: + ```typescript + save(): void { + this.dirty = true; + this.saveDeb.schedule(() => { + this.saveNowAsync().catch((err) => { + console.error('[StateStore] Async save failed:', err); + }); + }); + } + ``` + Note: The original pattern uses "if already scheduled, return" (leading-edge debounce). The Debouncer uses trailing-edge (reschedule). State-store's `save()` uses leading-edge: once scheduled, subsequent calls are no-ops until the timer fires. To preserve this behavior exactly, keep the `if (this.saveDeb.isPending) return;` guard: + ```typescript + save(): void { + this.dirty = true; + if (this.saveDeb.isPending) return; + this.saveDeb.schedule(() => { + this.saveNowAsync().catch((err) => { + console.error('[StateStore] Async save failed:', err); + }); + }); + } + ``` +4. **Update `_doSaveAsync()`** (line 245-248): Replace `if (this.saveTimeout) { clearTimeout(this.saveTimeout); this.saveTimeout = null; }` with `this.saveDeb.cancel();` +5. **Update `saveNow()`** (lines 334-338): Replace `clearTimeout(this.saveTimeout)` logic with `this.saveDeb.cancel();` +6. **Update Ralph state save** methods similarly — find all `ralphStateSaveTimeout` references and replace with `this.ralphStateSaveDeb.schedule(...)` / `this.ralphStateSaveDeb.cancel()` +7. **Add import**: `import { Debouncer } from './utils/index.js';` + +**Search for all references**: `grep -n 'saveTimeout\|ralphStateSaveTimeout' src/state-store.ts` — update every hit. + +### 2.2: `src/push-store.ts` — 1 Debouncer + +**Current** (lines 23, 150-156, 169-178): +```typescript +private saveTimer: NodeJS.Timeout | null = null; + +private scheduleSave(): void { + if (this._disposed) return; + if (this.saveTimer) clearTimeout(this.saveTimer); + this.saveTimer = setTimeout(() => { this.flushSave(); }, SAVE_DEBOUNCE_MS); +} + +dispose(): void { + if (this._disposed) return; + this._disposed = true; + if (this.saveTimer) { clearTimeout(this.saveTimer); this.saveTimer = null; } + this.flushSave(); +} +``` + +**New**: +```typescript +import { Debouncer } from './utils/index.js'; + +private saveDeb = new Debouncer(SAVE_DEBOUNCE_MS); +``` + +**Edits required**: + +1. **Replace `saveTimer` field** (line 23): `private saveDeb = new Debouncer(SAVE_DEBOUNCE_MS);` +2. **Update `scheduleSave()`** (lines 150-156): + ```typescript + private scheduleSave(): void { + if (this._disposed) return; + this.saveDeb.schedule(() => this.flushSave()); + } + ``` +3. **Update `dispose()`** (lines 169-178): + ```typescript + dispose(): void { + if (this._disposed) return; + this._disposed = true; + this.saveDeb.flush(() => this.flushSave()); + } + ``` +4. **Add import**: `import { Debouncer } from './utils/index.js';` + +### 2.3: `src/ralph-tracker.ts` — 2 Debouncers + 2 standalone timers + +The ralph-tracker has 4 timer fields. Two follow the debounce pattern (`_todoUpdateTimer`, `_loopUpdateTimer`) and two are standalone timers (`_fixPlanReloadTimer`, `_iterationStallTimer`). + +**Current** (lines 562-566, 624-625, 668-669, 1025-1078): +```typescript +private _todoUpdateTimer: NodeJS.Timeout | null = null; +private _loopUpdateTimer: NodeJS.Timeout | null = null; +private _fixPlanReloadTimer: NodeJS.Timeout | null = null; +private _iterationStallTimer: NodeJS.Timeout | null = null; +``` + +**Edits required**: + +1. **Replace debounce timer fields** (lines 562-572): Replace `_todoUpdateTimer`, `_loopUpdateTimer`, `_todoUpdatePending`, `_loopUpdatePending` with: + ```typescript + private _todoUpdateDeb = new Debouncer(EVENT_DEBOUNCE_MS); + private _loopUpdateDeb = new Debouncer(EVENT_DEBOUNCE_MS); + ``` + The `_*Pending` flags are no longer needed — `Debouncer.isPending` replaces them. + +2. **Rewrite `emitTodoUpdateDebounced()`** (lines 1043-1057): + ```typescript + private emitTodoUpdateDebounced(): void { + this._todoUpdateDeb.schedule(() => { + this.emit('todoUpdate', this.todos); + }); + } + ``` + +3. **Rewrite `emitLoopUpdateDebounced()`** (lines 1064-1078): + ```typescript + private emitLoopUpdateDebounced(): void { + this._loopUpdateDeb.schedule(() => { + this.emit('loopUpdate', this.loopState); + }); + } + ``` + +4. **Rewrite `clearDebounceTimers()`** (lines 1025-1036): + ```typescript + private clearDebounceTimers(): void { + this._todoUpdateDeb.cancel(); + this._loopUpdateDeb.cancel(); + } + ``` + +5. **Leave `_fixPlanReloadTimer` and `_iterationStallTimer` as-is** for now — they are standalone timers, not debounce patterns. They will be migrated to `CleanupManager` in Task 5. + +6. **Add import**: `import { Debouncer } from './utils/index.js';` + +**Search for all references**: `grep -n '_todoUpdateTimer\|_loopUpdateTimer\|_todoUpdatePending\|_loopUpdatePending' src/ralph-tracker.ts` — update every hit. + +### 2.4: `src/bash-tool-parser.ts` — 1 Debouncer + +**Current** (lines 31, 156, 699-701): +```typescript +const EVENT_DEBOUNCE_MS = 50; +private _updateTimer: ReturnType | null = null; + +// In destroy(): +if (this._updateTimer) { clearTimeout(this._updateTimer); this._updateTimer = null; } +``` + +**Edits required**: + +1. **Replace `_updateTimer` field** (line 156): `private _updateDeb = new Debouncer(EVENT_DEBOUNCE_MS);` +2. **Update all `_updateTimer` usage** — find with `grep -n '_updateTimer' src/bash-tool-parser.ts` and replace: + - `if (this._updateTimer) clearTimeout(this._updateTimer);` + `this._updateTimer = setTimeout(...)` → `this._updateDeb.schedule(...)` + - Cleanup in `destroy()`: → `this._updateDeb.dispose();` +3. **Leave `_autoRemoveTimers` Set as-is** — these are per-tool auto-remove timers with individual delays, not debounce. They'll be migrated to CleanupManager in Task 5. +4. **Add import**: `import { Debouncer } from './utils/index.js';` + +### 2.5: `src/image-watcher.ts` — 1 KeyedDebouncer + +**Current** (lines 36, 69, 72, 121-124, 202-228): +```typescript +const DEBOUNCE_DELAY_MS = 200; +private debounceTimers: Map = new Map(); +private timerToSession: Map = new Map(); // tracks which session owns each timer + +// In stop(): +for (const timer of this.debounceTimers.values()) clearTimeout(timer); +this.debounceTimers.clear(); +this.timerToSession.clear(); +``` + +**Edits required**: + +1. **Replace `debounceTimers` and `timerToSession` fields** (lines 69, 72): Replace both with: + ```typescript + private fileDeb = new KeyedDebouncer(DEBOUNCE_DELAY_MS); + private fileToSession = new Map(); // tracks which session owns each debounced file + ``` + Note: We still need `fileToSession` to look up which session a file belongs to (used in `unwatchSession()` to selectively cancel timers). Rename from `timerToSession` to `fileToSession` for clarity since the key is the file path, not a timer ID. + +2. **Update debounce call sites** — find with `grep -n 'debounceTimers' src/image-watcher.ts`: + - Where a file change is detected and debounced, replace: + ```typescript + const existing = this.debounceTimers.get(filePath); + if (existing) clearTimeout(existing); + this.debounceTimers.set(filePath, setTimeout(() => { ... }, DEBOUNCE_DELAY_MS)); + ``` + with: + ```typescript + this.fileDeb.schedule(filePath, () => { ... }); + ``` + - Also update `timerToSession` references to `fileToSession`. + +3. **Update `stop()`** (lines 121-124): Replace timer cleanup with `this.fileDeb.dispose();` + +4. **Update `unwatchSession()`** (lines 202-228): This method selectively cancels timers for a specific session. Replace: + ```typescript + // Collect timers to cancel (avoid modifying map during iteration) + const toCancel: string[] = []; + for (const [file, sessionId] of this.timerToSession) { + if (sessionId === id) toCancel.push(file); + } + for (const file of toCancel) { + const timer = this.debounceTimers.get(file); + if (timer) clearTimeout(timer); + this.debounceTimers.delete(file); + this.timerToSession.delete(file); + } + ``` + with: + ```typescript + const toCancel: string[] = []; + for (const [file, sessionId] of this.fileToSession) { + if (sessionId === id) toCancel.push(file); + } + for (const file of toCancel) { + this.fileDeb.cancelKey(file); + this.fileToSession.delete(file); + } + ``` + +5. **Add import**: `import { KeyedDebouncer } from './utils/index.js';` + +### 2.6: `src/subagent-watcher.ts` — 1 KeyedDebouncer + +The subagent-watcher has a `fileDebouncers` Map for per-file debouncing. + +**Edits required**: + +1. Find the `fileDebouncers` Map field and replace with `private fileDeb = new KeyedDebouncer(100);` (100ms delay — check the actual constant in the file) +2. Update all `fileDebouncers.get()`/`.set()`/`clearTimeout()` call sites to use `this.fileDeb.schedule(key, fn)` and `this.fileDeb.cancelKey(key)` +3. Update `stop()` to use `this.fileDeb.dispose()` instead of the manual map iteration +4. **Add import**: `import { KeyedDebouncer } from './utils/index.js';` + +**Search**: `grep -n 'fileDebouncers' src/subagent-watcher.ts` + +### 2.7: `src/web/server.ts` — 1 KeyedDebouncer for persist timers + +Server.ts has `persistDebounceTimers: Map>` (line 449) — a per-session debounce map. + +**Edits required**: + +1. **Replace `persistDebounceTimers` field** (line 449): `private persistDeb = new KeyedDebouncer(500);` + Check the actual debounce delay used when `persistDebounceTimers` is populated — search for where timers are added to this map. +2. **Update timer creation sites** — `grep -n 'persistDebounceTimers' src/web/server.ts`: + - Replace `this.persistDebounceTimers.set(id, setTimeout(...))` with `this.persistDeb.schedule(id, () => ...)` + - Replace `clearTimeout(this.persistDebounceTimers.get(id))` with `this.persistDeb.cancelKey(id)` +3. **Update `stop()` method** (lines 6590-6597): Replace the flush loop: + ```typescript + // Old: + for (const [sessionId, timer] of this.persistDebounceTimers) { + clearTimeout(timer); + const session = this.sessions.get(sessionId); + if (session) this._persistSessionStateNow(session); + } + this.persistDebounceTimers.clear(); + + // New: + this.persistDeb.flushAll((sessionId) => { + const session = this.sessions.get(sessionId); + if (session) this._persistSessionStateNow(session); + }); + ``` +4. **Add import**: `import { KeyedDebouncer } from '../../utils/index.js';` (or adjust path for server.ts location) + +**Note**: Do NOT migrate `terminalBatchTimers` to `KeyedDebouncer` — terminal batching uses variable delays (16-50ms adaptive) and stores batch data in separate Maps. The `KeyedDebouncer` with a fixed delay isn't a good fit. Leave terminal batching as-is. + +### 2.8: Verify `Debouncer` is not needed in `src/respawn-controller.ts` + +The respawn-controller has 10 timer fields, but **none** follow the debounce pattern. They are all one-shot timers or intervals started/stopped at state transitions. The correct tool for these is `CleanupManager`, not `Debouncer`. This is handled in Task 3. + +### Verification (after all 2.x edits) + +```bash +tsc --noEmit +npm run lint +npm run format:check + +# Run tests for affected modules (where tests exist): +npx vitest run test/image-watcher.test.ts +npx vitest run test/task-tracker.test.ts # tests bash-tool-parser indirectly +``` + +--- + +## Task 3: Migrate `respawn-controller.ts` to CleanupManager + +**File**: `src/respawn-controller.ts` +**Time**: ~3 hours + +This is the biggest single migration (10 timer fields + 1 interval field + `activeTimers` tracking map). + +### Current State (lines 644-745, 1773-1816) + +```typescript +// 10 Timeout fields: +private stepTimer: NodeJS.Timeout | null = null; +private completionConfirmTimer: NodeJS.Timeout | null = null; +private noOutputTimer: NodeJS.Timeout | null = null; +private detectionUpdateTimer: NodeJS.Timeout | null = null; // actually setInterval +private autoAcceptTimer: NodeJS.Timeout | null = null; +private preFilterTimer: NodeJS.Timeout | null = null; +private hookConfirmTimer: NodeJS.Timeout | null = null; +private clearFallbackTimer: NodeJS.Timeout | null = null; +private stepConfirmTimer: NodeJS.Timeout | null = null; +private stuckStateTimer: NodeJS.Timeout | null = null; // actually setInterval + +// UI tracking map: +private activeTimers: Map; +``` + +The `clearTimers()` method (lines 1773-1816) has 10 individual if-clearTimeout-null blocks plus a `clearInterval` for the two interval timers. + +### Strategy + +Replace all 10 timer fields with a single `CleanupManager` instance. Use the `description` option to give each timer a human-readable name. The `activeTimers` Map for UI countdown display must be preserved since it serves a different purpose (user-facing timer list). + +**Key constraint**: Several methods cancel specific timers by name before rescheduling (e.g., `resetNoOutputTimer()` clears `noOutputTimer` then sets a new one). `CleanupManager.setTimeout()` returns a registration ID — store these IDs in named fields to support cancel-and-reschedule. + +### Implementation + +1. **Add field** at the class level: + ```typescript + private cleanup = new CleanupManager(); + ``` + +2. **Replace individual timer fields** with registration ID fields: + ```typescript + // Old: + private stepTimer: NodeJS.Timeout | null = null; + // New: + private stepTimerId: string | null = null; + ``` + + Apply to all 10 timer fields. The naming convention is `*Id` suffix to indicate these hold CleanupManager registration IDs, not raw `NodeJS.Timeout` handles. + +3. **Replace timer creation**: Everywhere a timer is started, replace: + ```typescript + // Old: + if (this.stepTimer) clearTimeout(this.stepTimer); + this.stepTimer = setTimeout(() => { ... }, delay); + + // New: + if (this.stepTimerId) this.cleanup.unregister(this.stepTimerId); + this.stepTimerId = this.cleanup.setTimeout(() => { + if (this.cleanup.isStopped) return; + ... + }, delay, { description: 'step delay' }); + ``` + + For intervals (`detectionUpdateTimer`, `stuckStateTimer`): + ```typescript + // Old: + this.detectionUpdateTimer = setInterval(() => { ... }, 2000); + // New: + this.detectionUpdateTimerId = this.cleanup.setInterval(() => { ... }, 2000, { description: 'detection status updates' }); + ``` + +4. **Replace `clearTimers()`** (lines 1773-1816): + ```typescript + private clearTimers(): void { + this.activeTimers.clear(); + this.cleanup.dispose(); + // Reinitialize for reuse (controller can be stopped and restarted) + this.cleanup = new CleanupManager(); + // Null out IDs + this.stepTimerId = null; + this.completionConfirmTimerId = null; + this.noOutputTimerId = null; + this.detectionUpdateTimerId = null; + this.autoAcceptTimerId = null; + this.preFilterTimerId = null; + this.hookConfirmTimerId = null; + this.clearFallbackTimerId = null; + this.stepConfirmTimerId = null; + this.stuckStateTimerId = null; + } + ``` + + **Important**: The respawn controller is reusable — `stop()` can be followed by `start()`. Since `CleanupManager.dispose()` sets `isDisposed = true` permanently, we must create a new instance after disposing. This is safe and cheap. + +5. **Replace individual timer cancels** throughout the file — search for each timer name pattern: + ```bash + grep -n 'stepTimer\|completionConfirmTimer\|noOutputTimer\|detectionUpdateTimer\|autoAcceptTimer\|preFilterTimer\|hookConfirmTimer\|clearFallbackTimer\|stepConfirmTimer\|stuckStateTimer' src/respawn-controller.ts + ``` + Each `if (this.xxxTimer) { clearTimeout(this.xxxTimer); this.xxxTimer = null; }` becomes `if (this.xxxTimerId) { this.cleanup.unregister(this.xxxTimerId); this.xxxTimerId = null; }`. + +6. **Update `stop()` method** to ensure cleanup is called: + ```typescript + stop(): void { + // ... existing stop logic ... + this.clearTimers(); + // clearTimers() now handles all cleanup via CleanupManager.dispose() + } + ``` + +7. **Add import**: `import { CleanupManager } from './utils/index.js';` + +### activeTimers Map (UI Display) + +The `activeTimers` Map (line 733) tracks timers for UI countdown display. This is a **separate concern** from lifecycle cleanup and must be preserved alongside the CleanupManager migration. Continue updating it when timers are started/stopped as before. + +### Methods to Update + +Find each of these methods and update their timer management: + +| Method | Timer(s) Used | +|--------|--------------| +| `step()` / internal step functions | `stepTimer` | +| `startCompletionConfirm()` | `completionConfirmTimer` | +| `resetNoOutputTimer()` | `noOutputTimer` | +| `startDetectionUpdates()` / `stopDetectionUpdates()` | `detectionUpdateTimer` (interval) | +| `resetAutoAcceptTimer()` | `autoAcceptTimer` | +| `resetPreFilterTimer()` | `preFilterTimer` | +| `startHookConfirmTimer()` | `hookConfirmTimer` | +| `startClearFallbackTimer()` | `clearFallbackTimer` | +| `startStepConfirmTimer()` | `stepConfirmTimer` | +| `startStuckStateTimer()` | `stuckStateTimer` (interval) | + +### Verification + +```bash +tsc --noEmit +npm run lint +npx vitest run test/respawn-controller.test.ts +``` + +--- + +## Task 4: Migrate `server.ts` Timer Cleanup to CleanupManager + +**File**: `src/web/server.ts` +**Time**: ~2 hours + +### Current State (lines 424-449, 6529-6660) + +Server.ts has 6 standalone timers (not counting the Maps migrated in Task 2): + +```typescript +private scheduledCleanupTimer: NodeJS.Timeout | null = null; // setInterval +private taskUpdateBatchTimer: NodeJS.Timeout | null = null; // setTimeout +private stateUpdateTimer: NodeJS.Timeout | null = null; // setTimeout +private sseHealthCheckTimer: NodeJS.Timeout | null = null; // setInterval +private tokenRecordingTimer: NodeJS.Timeout | null = null; // setInterval +``` + +Plus one Map already migrated to `KeyedDebouncer` in Task 2: +```typescript +private persistDebounceTimers → persistDeb // Done in Task 2 +``` + +And Maps that should NOT be migrated (variable delay, complex batch logic): +```typescript +private terminalBatchTimers: Map // Leave as-is +private pendingRespawnStarts: Map // Leave as-is +``` + +### Strategy + +Add a `CleanupManager` to handle the 5 standalone timers. Leave `terminalBatchTimers` and `pendingRespawnStarts` as manual Maps (they have complex lifecycle requirements that don't fit CleanupManager cleanly). + +### Implementation + +1. **Add field**: + ```typescript + private cleanup = new CleanupManager(); + ``` + +2. **Replace 5 standalone timer fields** with CleanupManager registration IDs. For timers that are set once during `start()` and never reset: + ```typescript + // Old (in startServer or setupRoutes): + this.sseHealthCheckTimer = setInterval(() => { ... }, SSE_HEALTH_CHECK_INTERVAL); + // New: + this.cleanup.setInterval(() => { ... }, SSE_HEALTH_CHECK_INTERVAL, { description: 'SSE health check' }); + ``` + + For these "start once" timers, we don't even need to store the registration ID since they're never individually cancelled. The timers in this category: + - `sseHealthCheckTimer` — started once, cleared on stop + - `scheduledCleanupTimer` — started once, cleared on stop + - `tokenRecordingTimer` — started once, cleared on stop + + For timers that are reset during operation: + - `taskUpdateBatchTimer` — reset on each task update batch + - `stateUpdateTimer` — reset on each state change + + These need stored registration IDs: + ```typescript + private taskUpdateBatchTimerId: string | null = null; + private stateUpdateTimerId: string | null = null; + ``` + +3. **Update `stop()` method** (lines 6529-6660): Replace individual timer clears with: + ```typescript + // Replace lines 6535-6584 (5 individual timer clears) with: + this.cleanup.dispose(); + ``` + + Keep the remaining cleanup that isn't timer-related: + - SSE client graceful close (lines 6541-6551) — keep + - Terminal batch timer Map clear (lines 6554-6559) — keep (not migrated) + - Pending respawn starts Map clear (lines 6604-6607) — keep (not migrated) + - Persist debouncer flush (migrated in Task 2 to `this.persistDeb.flushAll(...)`) — keep + - Everything else below (respawn controllers, sessions, listeners) — keep + +4. **Add import**: Add `CleanupManager` to the imports from utils (likely already imported but unused). + +### What NOT to Migrate + +- **`terminalBatchTimers`**: Per-session batch timers with adaptive 16-50ms delays. Complex lifecycle, variable delays, performance-critical. Keep as manual Map. +- **`pendingRespawnStarts`**: Grace period timers for restored sessions. Set once per session on startup, cleared individually when sessions start. Keep as manual Map. +- **SSE client management**: Not timer-based. Keep as-is. +- **Session listener refs**: EventEmitter cleanup, not timers. Keep as-is. + +### Verification + +```bash +tsc --noEmit +npm run lint +npm run format:check +``` + +No dedicated server.ts tests exist, so verify manually: +```bash +# Start dev server and confirm it runs without errors +npx tsx src/index.ts web & +sleep 3 +curl -s http://localhost:3000/api/status | jq '.status' +# Should output "ok" +# Then kill the background process +``` + +--- + +## Task 5: Migrate Remaining Files to CleanupManager + +**Files**: `src/ralph-tracker.ts`, `src/bash-tool-parser.ts`, `src/image-watcher.ts`, `src/subagent-watcher.ts` +**Time**: ~2 hours + +After Task 2 migrates debounce patterns to `Debouncer`/`KeyedDebouncer`, some files still have standalone timers, watchers, and interval resources that should use `CleanupManager`. + +### 5.1: `src/ralph-tracker.ts` — Watcher + 2 standalone timers + +After Task 2, the debounce timers (`_todoUpdateDeb`, `_loopUpdateDeb`) are handled. Remaining: + +**Still manual**: +- `_fixPlanWatcher: FSWatcher | null` (line 620) — file system watcher +- `_fixPlanWatcherErrorHandler` (line 622) — stored error handler ref +- `_fixPlanReloadTimer: NodeJS.Timeout | null` (line 625) — debounce for file changes +- `_iterationStallTimer: NodeJS.Timeout | null` (line 669) — stall detection interval + +**Edits required**: + +1. **Add field**: `private cleanup = new CleanupManager();` +2. **Migrate `_fixPlanReloadTimer`** to a `Debouncer` (it IS a debounce pattern — file change events are debounced): + ```typescript + private _fixPlanReloadDeb = new Debouncer(500); // check actual delay + ``` +3. **Migrate `_fixPlanWatcher`**: When the watcher is created, register it: + ```typescript + this.cleanup.registerWatcher(watcher, 'fix plan file watcher'); + ``` + Remove the manual `_fixPlanWatcher` and `_fixPlanWatcherErrorHandler` fields. +4. **Migrate `_iterationStallTimer`**: When started, use: + ```typescript + this._iterationStallTimerId = this.cleanup.setInterval(() => { ... }, interval, { description: 'iteration stall detection' }); + ``` +5. **Update `destroy()`**: Add `this.cleanup.dispose();` and `this._fixPlanReloadDeb.dispose();`. Remove manual timer/watcher cleanup that's now handled by CleanupManager. +6. **Update `stopWatchingFixPlan()`**: Use `this.cleanup.unregister(watcherId)` instead of manual `watcher.close()`. +7. **Update `stopIterationStallDetection()`**: Use `this.cleanup.unregister(this._iterationStallTimerId)` instead of manual `clearInterval()`. + +### 5.2: `src/bash-tool-parser.ts` — Auto-remove timer Set + +After Task 2, `_updateDeb` handles the event debounce. Remaining: + +**Still manual**: +- `_autoRemoveTimers: Set>` (line 149) — Set of auto-remove timeouts per tool + +**Edits required**: + +1. **Add field**: `private cleanup = new CleanupManager();` +2. **Replace `_autoRemoveTimers` Set**: When a tool auto-remove timer is created: + ```typescript + // Old: + const timer = setTimeout(() => { this.removeTool(id); }, AUTO_REMOVE_MS); + this._autoRemoveTimers.add(timer); + // New: + this.cleanup.setTimeout(() => { this.removeTool(id); }, AUTO_REMOVE_MS, { description: `auto-remove tool ${id}` }); + ``` + Remove the `_autoRemoveTimers` Set field entirely. +3. **Update `destroy()`**: Replace `for (const t of this._autoRemoveTimers) clearTimeout(t); this._autoRemoveTimers.clear();` with `this.cleanup.dispose();` +4. **Add `if (this.cleanup.isStopped) return;` guard** in callbacks that fire after potential disposal. + +### 5.3: `src/image-watcher.ts` — Chokidar watchers + +After Task 2, `fileDeb` (KeyedDebouncer) handles per-file debouncing. Remaining: + +**Still manual**: +- `watchers: Map` — chokidar file watchers per session + +**Edits required**: + +1. **Add field**: `private cleanup = new CleanupManager();` +2. **Register watchers on creation**: When a watcher is created for a session: + ```typescript + const watcherId = this.cleanup.registerWatcher(watcher, `image watcher for ${sessionId}`); + // Store watcherId → sessionId mapping if needed for selective cleanup + ``` +3. **Update `stop()`**: Replace manual watcher close loop with `this.cleanup.dispose();` +4. **Update `unwatchSession()`**: Need to selectively unregister watchers for a specific session. This requires storing the registration ID alongside the session mapping. Add a `watcherIds: Map` to map `sessionId → cleanupRegistrationId`, then: + ```typescript + const regId = this.watcherIds.get(sessionId); + if (regId) this.cleanup.unregister(regId); + ``` + +**Note**: If `unwatchSession()` selective cleanup makes CleanupManager usage awkward (needing a parallel tracking Map), it may be simpler to keep the watcher Map manual and only migrate the overall `stop()` cleanup. Use judgment — the goal is reducing boilerplate, not adding complexity. + +### 5.4: `src/subagent-watcher.ts` — Intervals + watchers + +After Task 2, `fileDeb` (KeyedDebouncer) handles file debouncing. Remaining: + +**Still manual**: +- Poll interval +- Liveness check interval +- Idle timers Map +- Directory watchers +- Error handler references + +**Edits required**: + +1. **Add field**: `private cleanup = new CleanupManager();` +2. **Register poll interval**: + ```typescript + this.cleanup.setInterval(() => this.poll(), POLL_INTERVAL_MS, { description: 'subagent poll' }); + ``` +3. **Register liveness interval**: + ```typescript + this.cleanup.setInterval(() => this.checkLiveness(), LIVENESS_INTERVAL_MS, { description: 'subagent liveness check' }); + ``` +4. **Migrate idle timers Map**: Replace `idleTimers: Map` with `KeyedDebouncer` or individual `CleanupManager.setTimeout()` calls. If idle timers have the same delay, use `KeyedDebouncer`. If variable delay, use CleanupManager and store registration IDs. +5. **Register directory watchers**: When fs.watch watchers are created: + ```typescript + this.cleanup.registerWatcher(watcher, `subagent dir watcher for ${sessionId}`); + ``` +6. **Update `stop()`**: Replace the long manual cleanup with: + ```typescript + this.cleanup.dispose(); + this.fileDeb.dispose(); // From Task 2 + // Clear data Maps (not timer/resource related): + this.filePositions.clear(); + this.agentInfo.clear(); + // ... etc + ``` + +### Verification + +```bash +tsc --noEmit +npm run lint +npm run format:check +npx vitest run test/image-watcher.test.ts +npx vitest run test/task-tracker.test.ts +``` + +--- + +## Final Verification Checklist + +After all 5 tasks are complete, run the following: + +```bash +# 1. TypeScript type checking +tsc --noEmit + +# 2. Linting +npm run lint + +# 3. Formatting +npm run format:check + +# 4. Run affected test files individually (NOT the full suite) +npx vitest run test/respawn-controller.test.ts +npx vitest run test/image-watcher.test.ts +npx vitest run test/task-tracker.test.ts +npx vitest run test/task-queue.test.ts +npx vitest run test/session-manager.test.ts +``` + +If any formatting issues arise: +```bash +npm run format +``` + +If any lint issues arise: +```bash +npm run lint:fix +``` + +--- + +## Summary of Changes + +| Task | Files Modified | Files Created | Key Change | +|------|---------------|---------------|------------| +| 1. Debouncer utility | `src/utils/index.ts` | `src/utils/debouncer.ts` | `Debouncer` + `KeyedDebouncer` classes | +| 2. Debounce migrations | 7 files (state-store, push-store, ralph-tracker, bash-tool-parser, image-watcher, subagent-watcher, server.ts) | — | Replace manual timer patterns with Debouncer/KeyedDebouncer | +| 3. Respawn CleanupManager | `src/respawn-controller.ts` | — | 10 timer fields → CleanupManager | +| 4. Server CleanupManager | `src/web/server.ts` | — | 5 standalone timers → CleanupManager | +| 5. Remaining CleanupManager | 4 files (ralph-tracker, bash-tool-parser, image-watcher, subagent-watcher) | — | Watchers, intervals, auto-remove timers → CleanupManager | + +**Total files modified**: 10 +**Total files created**: 1 +**Timer fields eliminated**: ~30 manual timer fields → Debouncer/KeyedDebouncer/CleanupManager +**Lines of boilerplate removed**: ~200+ lines of if-clearTimeout-null patterns + +--- + +## Risk Assessment + +| Risk | Likelihood | Mitigation | +|------|-----------|------------| +| Timer behavior changes (leading vs trailing edge) | Medium | Preserve `isPending` guard for state-store's leading-edge pattern | +| Respawn controller restart after dispose | Low | Reinitialize CleanupManager in `clearTimers()` | +| Selective cleanup in image/subagent watchers | Low | Keep parallel tracking Maps where CleanupManager doesn't fit | +| TypeScript strict mode violations | Low | `tsc --noEmit` after each file migration | +| Test regressions | Low | Existing tests cover timer-dependent behavior | + +### What NOT to Touch + +- **Terminal batch timers** (`server.ts:terminalBatchTimers`) — performance-critical adaptive batching with variable delays +- **Pending respawn starts** (`server.ts:pendingRespawnStarts`) — one-shot timers with individual lifecycle +- **SSE client management** — not timer-based +- **Session listener refs** — EventEmitter cleanup, not timers +- **Frontend `app.js`** — separate concern (Phase 5 of the overall roadmap) diff --git a/docs/phase3-implementation-plan.md b/docs/phase3-implementation-plan.md new file mode 100644 index 00000000..30628352 --- /dev/null +++ b/docs/phase3-implementation-plan.md @@ -0,0 +1,1045 @@ +# Phase 3 Implementation Plan: server.ts Route Extraction + +**Source**: `docs/code-structure-findings.md` (Phase 3 — server.ts Route Extraction) +**Estimated effort**: 3-4 days +**Tasks**: 8 tasks with dependencies (see dependency graph below) + +--- + +## Safety Constraints + +Before starting ANY work, read and follow these rules: + +1. **Never run `npx vitest run`** (full suite) — it kills tmux sessions. You are running inside a Codeman-managed tmux session. +2. **Run individual tests only**: `npx vitest run test/.test.ts` +3. **Never test on port 3000** — the live dev server runs there. Tests use ports 3150+. +4. **After TypeScript changes**: Run `tsc --noEmit` to verify type checking passes. +5. **Before considering done**: Run `npm run lint` and `npm run format:check` to ensure CI passes. +6. **Never kill tmux sessions** — check `echo $CODEMAN_MUX` first. +7. **Verify the dev server starts**: After each task, run `npx tsx src/index.ts web --port 3099 &` on a non-production port, confirm `curl -s http://localhost:3099/api/status | jq .status` returns `"ok"`, then kill the background process. This is essential — there are zero server.ts tests. + +--- + +## Goal + +Reduce `src/web/server.ts` from ~6,710 LOC to ~1,500 LOC by extracting route handlers into domain-specific route modules, auth logic into middleware, and SSE/batching into service modules. The WebServer class retains orchestration (start/stop, session lifecycle, listener wiring) but delegates all HTTP route definitions to separate files. + +--- + +## Task Dependencies + +``` +Task 1 (RouteContext interface) + ├──> Task 2 (Auth middleware) + ├──> Task 3 (Session routes) + ├──> Task 4 (Respawn routes) + ├──> Task 5 (Subagent + mux + team routes) + ├──> Task 6 (Plan + case + ralph routes) + ├──> Task 7 (System + settings + push + misc routes) + └──> Task 8 (Final cleanup — reduce server.ts) +``` + +**Task 1** must complete first — it defines the shared interface all route modules use to access server state. +**Tasks 2-7** are independent and can run in parallel. +**Task 8** depends on all prior tasks. + +--- + +## Target Directory Structure + +``` +src/web/ +├── server.ts (~1,500 LOC — orchestration, start/stop, listeners, SSE infra) +├── route-context.ts (~80 LOC — RouteContext interface + helpers) +├── middleware/ +│ └── auth.ts (~120 LOC — Basic Auth + session cookies + rate limiting) +├── routes/ +│ ├── session-routes.ts (~800 LOC — CRUD, input, resize, terminal buffer, auto-ops) +│ ├── respawn-routes.ts (~400 LOC — status, config, start/stop, enable, interactive) +│ ├── subagent-routes.ts (~250 LOC — list, transcript, kill, cleanup, window states) +│ ├── plan-routes.ts (~500 LOC — generate, detailed, cancel, tasks, checkpoint, rollback) +│ ├── case-routes.ts (~400 LOC — CRUD, link, fix-plan, ralph wizard) +│ ├── ralph-routes.ts (~450 LOC — config, status, circuit-breaker, fix-plan, prompts, loop start) +│ ├── system-routes.ts (~350 LOC — status, stats, config, debug, lifecycle, settings, model) +│ ├── file-routes.ts (~400 LOC — file tree, preview, raw, tail, screenshots) +│ ├── push-routes.ts (~100 LOC — VAPID key, subscribe, update prefs, unsubscribe) +│ ├── mux-routes.ts (~80 LOC — list, delete, reconcile, stats) +│ ├── team-routes.ts (~50 LOC — list teams, team tasks) +│ └── scheduled-routes.ts (~120 LOC — CRUD for scheduled runs, quick-start, quick-run) +└── schemas.ts (existing — unchanged) +``` + +--- + +## Key Design Decision: RouteContext Pattern + +Routes need access to server state (sessions, respawn controllers, store, mux, etc.) without importing WebServer directly. We use a **context object pattern** where the WebServer exposes a `RouteContext` interface that route modules receive. + +This avoids: +- Circular dependencies (routes importing server, server importing routes) +- Exposing all 40+ private WebServer fields +- Making route modules aware of server internals + +Each route module exports a `register(app, ctx)` function that Fastify calls during setup. + +### Why NOT Fastify plugins/decorators + +Fastify plugins with `fastify.decorate()` would work but: +- Requires TypeScript module augmentation for type safety (brittle) +- Each decorator is on the Fastify instance, not a typed interface — easy to misspell +- The context pattern is simpler, standard in large Fastify apps, and plays well with `strictNullChecks` + +--- + +## Task 1: Create RouteContext Interface + +**Files to create**: `src/web/route-context.ts` +**Files to edit**: `src/web/server.ts` +**Time**: ~2 hours + +### Problem + +Route modules need access to server state (sessions, controllers, store) and helper methods (broadcast, persistSessionState, cleanupSession). A typed interface provides this without coupling routes to WebServer internals. + +### Implementation + +Create `src/web/route-context.ts` that defines the interface: + +```typescript +/** + * @fileoverview Shared context interface for route modules. + * + * Route handlers receive a RouteContext to access server state and + * helper methods without directly depending on the WebServer class. + * + * @module web/route-context + */ + +import type { FastifyInstance } from 'fastify'; +import type { Session } from '../session.js'; +import type { RespawnController, RespawnConfig } from '../respawn-controller.js'; +import type { TerminalMultiplexer } from '../mux-interface.js'; +import type { StateStore } from '../state-store.js'; +import type { PlanOrchestrator } from '../plan-orchestrator.js'; +import type { RunSummaryTracker } from '../run-summary.js'; +import type { TranscriptWatcher } from '../transcript-watcher.js'; +import type { TeamWatcher } from '../team-watcher.js'; +import type { TunnelManager } from '../tunnel-manager.js'; +import type { PushSubscriptionStore } from '../push-store.js'; +import type { + ApiErrorCode, + ApiResponse, + PersistedRespawnConfig, + NiceConfig, +} from '../types.js'; + +/** + * Context object passed to route modules. + * Provides access to server state and helper methods. + */ +export interface RouteContext { + // === Core State === + readonly sessions: Map; + readonly respawnControllers: Map; + readonly respawnTimers: Map; + readonly runSummaryTrackers: Map; + readonly activePlanOrchestrators: Map; + readonly scheduledRuns: Map; // Type imported from server.ts or moved + readonly store: StateStore; + readonly mux: TerminalMultiplexer; + readonly teamWatcher: TeamWatcher; + readonly tunnelManager: TunnelManager; + readonly pushStore: PushSubscriptionStore; + + // === Config === + readonly port: number; + readonly https: boolean; + readonly testMode: boolean; + readonly serverStartTime: number; + + // === Methods === + + /** Broadcast an SSE event to all connected clients */ + broadcast(event: string, data: unknown): void; + + /** Debounced session state persistence */ + persistSessionState(session: Session): void; + + /** Immediate session state persistence */ + persistSessionStateNow(session: Session): void; + + /** Clean up all resources for a session */ + cleanupSession(sessionId: string, killMux?: boolean, reason?: string): Promise; + + /** Set up event listeners for a session */ + setupSessionListeners(session: Session): void; + + /** Remove all event listeners for a session */ + removeSessionListeners(sessionId: string): void; + + /** Set up respawn controller event listeners */ + setupRespawnListeners(sessionId: string, controller: RespawnController): void; + + /** Set up a timed respawn duration */ + setupTimedRespawn(sessionId: string, durationMinutes: number): void; + + /** Restore a respawn controller from persisted config */ + restoreRespawnController(session: Session, config: PersistedRespawnConfig, source: string): void; + + /** Save respawn config to mux metadata */ + saveRespawnConfig(sessionId: string, config: RespawnConfig, durationMinutes?: number): void; + + /** Start transcript watcher for a session */ + startTranscriptWatcher(sessionId: string, transcriptPath: string): void; + + /** Stop transcript watcher for a session */ + stopTranscriptWatcher(sessionId: string): void; + + /** Get session state enriched with respawn info */ + getSessionStateWithRespawn(session: Session): unknown; + + /** Get default CLAUDE.md template path from settings */ + getDefaultClaudeMdPath(): Promise; + + /** Batch terminal data for 60fps streaming */ + batchTerminalData(sessionId: string, data: string): void; + + /** Broadcast debounced session state update */ + broadcastSessionStateDebounced(sessionId: string): void; + + /** Batch a task update for SSE broadcasting */ + batchTaskUpdate(sessionId: string, task: unknown): void; +} + +/** + * Signature for a route registration function. + * Each route module exports a function with this signature. + */ +export type RegisterRoutes = (app: FastifyInstance, ctx: RouteContext) => void; +``` + +### Expose context from WebServer + +In `server.ts`, add a private method that creates the context object. This is called once during `setupRoutes()` and passed to each route module: + +```typescript +private createRouteContext(): RouteContext { + return { + sessions: this.sessions, + respawnControllers: this.respawnControllers, + respawnTimers: this.respawnTimers, + runSummaryTrackers: this.runSummaryTrackers, + activePlanOrchestrators: this.activePlanOrchestrators, + scheduledRuns: this.scheduledRuns, + store: this.store, + mux: this.mux, + teamWatcher: this.teamWatcher, + tunnelManager: this.tunnelManager, + pushStore: this.pushStore, + port: this.port, + https: this.https, + testMode: this.testMode, + serverStartTime: this.serverStartTime, + broadcast: this.broadcast.bind(this), + persistSessionState: this.persistSessionState.bind(this), + persistSessionStateNow: this._persistSessionStateNow.bind(this), + cleanupSession: this.cleanupSession.bind(this), + setupSessionListeners: this.setupSessionListeners.bind(this), + removeSessionListeners: this.removeSessionListeners.bind(this), + setupRespawnListeners: this.setupRespawnListeners.bind(this), + setupTimedRespawn: this.setupTimedRespawn.bind(this), + restoreRespawnController: this.restoreRespawnController.bind(this), + saveRespawnConfig: this.saveRespawnConfig.bind(this), + startTranscriptWatcher: this.startTranscriptWatcher.bind(this), + stopTranscriptWatcher: this.stopTranscriptWatcher.bind(this), + getSessionStateWithRespawn: this.getSessionStateWithRespawn.bind(this), + getDefaultClaudeMdPath: this.getDefaultClaudeMdPath.bind(this), + batchTerminalData: this.batchTerminalData.bind(this), + broadcastSessionStateDebounced: this.broadcastSessionStateDebounced.bind(this), + batchTaskUpdate: this.batchTaskUpdate.bind(this), + }; +} +``` + +### Helper: findSessionOrFail + +Add a shared helper to `route-context.ts` (replaces 189 repetitions of the session-not-found pattern): + +```typescript +import { createErrorResponse, ApiErrorCode } from '../types.js'; + +/** + * Look up a session by ID, throwing a structured error if not found. + * Route handlers call this to avoid 189 repetitions of the NOT_FOUND pattern. + */ +export function findSessionOrFail(ctx: RouteContext, sessionId: string): Session { + const session = ctx.sessions.get(sessionId); + if (!session) { + throw Object.assign( + new Error('Session not found'), + { statusCode: 404, response: createErrorResponse(ApiErrorCode.NOT_FOUND, 'Session not found') } + ); + } + return session; +} +``` + +In each route module, use it as: +```typescript +const session = findSessionOrFail(ctx, sessionId); +// If we get here, session is guaranteed non-null +``` + +For Fastify error handling, register a global error handler in `setupRoutes()` that catches these structured errors: +```typescript +this.app.setErrorHandler((error, _req, reply) => { + if ('response' in error && 'statusCode' in error) { + return reply.code((error as any).statusCode).send((error as any).response); + } + reply.code(500).send(createErrorResponse(ApiErrorCode.INTERNAL_ERROR, error.message)); +}); +``` + +### ScheduledRun type + +The `ScheduledRun` type is currently defined locally in `server.ts` (around line 372-389). Move it to `route-context.ts` or a shared types location since route modules need it. + +### Verification + +```bash +tsc --noEmit +npm run lint +npm run format:check +``` + +--- + +## Task 2: Extract Auth Middleware + +**File to create**: `src/web/middleware/auth.ts` +**File to edit**: `src/web/server.ts` +**Time**: ~1 hour + +### Problem + +Auth logic (HTTP Basic Auth, session cookies, rate limiting) is inline in `setupRoutes()` at lines 659-780. This is ~120 LOC of self-contained logic that doesn't depend on any route handlers. + +### Implementation + +Extract the `onRequest` hook registration into a standalone function: + +**New file**: `src/web/middleware/auth.ts` + +```typescript +/** + * @fileoverview HTTP Basic Auth middleware with session cookies and rate limiting. + * + * Extracted from server.ts setupRoutes() auth section. + * Only active when CODEMAN_PASSWORD environment variable is set. + * + * @module web/middleware/auth + */ + +import type { FastifyInstance } from 'fastify'; +import { randomBytes, timingSafeEqual } from 'node:crypto'; +import { StaleExpirationMap } from '../../utils/index.js'; + +// Auth configuration constants +const AUTH_SESSION_TTL_MS = 24 * 60 * 60 * 1000; // 24 hours +const MAX_AUTH_SESSIONS = 100; +const AUTH_FAILURE_WINDOW_MS = 15 * 60 * 1000; // 15 minutes +const AUTH_FAILURE_MAX = 10; +const AUTH_COOKIE_NAME = 'codeman_session'; + +/** + * Register HTTP Basic Auth with session cookies and rate limiting. + * + * Does nothing if CODEMAN_PASSWORD is not set. + * Exempts /api/hook-event from localhost (Claude Code hooks curl this). + */ +export function registerAuthMiddleware(app: FastifyInstance, https: boolean): { + authSessions: StaleExpirationMap | null; + authFailures: StaleExpirationMap | null; +} { + const authPassword = process.env.CODEMAN_PASSWORD; + if (!authPassword) { + return { authSessions: null, authFailures: null }; + } + + const authUsername = process.env.CODEMAN_USERNAME || 'admin'; + const expectedHeader = 'Basic ' + Buffer.from(`${authUsername}:${authPassword}`).toString('base64'); + + const authSessions = new StaleExpirationMap({ + ttlMs: AUTH_SESSION_TTL_MS, + refreshOnGet: true, + }); + + const authFailures = new StaleExpirationMap({ + ttlMs: AUTH_FAILURE_WINDOW_MS, + refreshOnGet: false, + }); + + app.addHook('onRequest', (req, reply, done) => { + // Hook events from localhost bypass auth + if (req.url === '/api/hook-event' && req.method === 'POST') { + const ip = req.ip; + if (ip === '127.0.0.1' || ip === '::1' || ip === '::ffff:127.0.0.1') { + done(); + return; + } + } + + const clientIp = req.ip; + + // Rate limit check + const failures = authFailures.get(clientIp) ?? 0; + if (failures >= AUTH_FAILURE_MAX) { + reply.code(429).send('Too Many Requests — try again later'); + return; + } + + // Session cookie check + const sessionToken = req.cookies[AUTH_COOKIE_NAME]; + if (sessionToken && authSessions.get(sessionToken) !== undefined) { + done(); + return; + } + + // Basic Auth header check (timing-safe) + const auth = req.headers.authorization; + const authBuf = Buffer.from(auth ?? ''); + const expectedBuf = Buffer.from(expectedHeader); + if (authBuf.length === expectedBuf.length && timingSafeEqual(authBuf, expectedBuf)) { + const token = randomBytes(32).toString('hex'); + if (authSessions.size >= MAX_AUTH_SESSIONS) { + const oldestKey = authSessions.keys().next().value; + if (oldestKey !== undefined) authSessions.delete(oldestKey); + } + authSessions.set(token, clientIp); + authFailures.delete(clientIp); + reply.setCookie(AUTH_COOKIE_NAME, token, { + httpOnly: true, + secure: https, + sameSite: 'lax', + maxAge: AUTH_SESSION_TTL_MS / 1000, + path: '/', + }); + done(); + return; + } + + // Failed — track and reject + authFailures.set(clientIp, failures + 1); + reply.code(401).header('WWW-Authenticate', 'Basic realm="Codeman"').send('Unauthorized'); + }); + + return { authSessions, authFailures }; +} +``` + +### Security headers + +Also extract the security headers hook (CSP, X-Frame-Options, X-Content-Type-Options, HSTS) from `setupRoutes()` into `auth.ts` as a separate function `registerSecurityHeaders(app, https)`, since it's closely related to the auth/security concern. + +### Edit server.ts + +In `setupRoutes()`, replace the inline auth block (~lines 659-780) with: + +```typescript +import { registerAuthMiddleware, registerSecurityHeaders } from './middleware/auth.js'; + +// In setupRoutes(): +const { authSessions, authFailures } = registerAuthMiddleware(this.app, this.https); +this.authSessions = authSessions; +this.authFailures = authFailures; +registerSecurityHeaders(this.app, this.https); +``` + +Move the auth constants (`AUTH_SESSION_TTL_MS`, `MAX_AUTH_SESSIONS`, `AUTH_FAILURE_WINDOW_MS`, `AUTH_FAILURE_MAX`, `AUTH_COOKIE_NAME`) from server.ts to the middleware file. + +### Verification + +```bash +tsc --noEmit +npm run lint +# Start server and verify auth still works: +CODEMAN_PASSWORD=test npx tsx src/index.ts web --port 3099 & +sleep 3 +# Should get 401 without credentials: +curl -s -o /dev/null -w "%{http_code}" http://localhost:3099/api/status +# Should get 200 with credentials: +curl -s -u admin:test http://localhost:3099/api/status | jq .status +kill %1 +``` + +--- + +## Task 3: Extract Session Routes + +**File to create**: `src/web/routes/session-routes.ts` +**File to edit**: `src/web/server.ts` +**Time**: ~3 hours (largest route group) + +### Problem + +Session routes are the largest group (~30 handlers, ~800 LOC) covering CRUD, input, resize, terminal buffer access, auto-clear/compact, and image watcher toggling. + +### Routes to Extract + +| Method | Path | Current Line | Purpose | +|--------|------|-------------|---------| +| GET | `/api/sessions` | 1015 | List all sessions (cached) | +| POST | `/api/sessions` | 1017 | Create session | +| PUT | `/api/sessions/:id/name` | 1094 | Rename session | +| PUT | `/api/sessions/:id/color` | 1117 | Change session color | +| DELETE | `/api/sessions/:id` | 1141 | Delete single session | +| DELETE | `/api/sessions` | 1155 | Kill all sessions | +| GET | `/api/sessions/:id` | 1169 | Get session details | +| GET | `/api/sessions/:id/output` | 1182 | Get session output text | +| GET | `/api/sessions/:id/ralph-state` | 1201 | Get Ralph tracker state | +| GET | `/api/sessions/:id/run-summary` | 1220 | Get run summary timeline | +| GET | `/api/sessions/:id/active-tools` | 1243 | Get active bash tools | +| POST | `/api/sessions/:id/run` | 1915 | Run a prompt | +| POST | `/api/sessions/:id/interactive` | 1942 | Start interactive mode | +| POST | `/api/sessions/:id/shell` | 1985 | Start shell mode | +| POST | `/api/sessions/:id/input` | 2015 | Send input to session | +| POST | `/api/sessions/:id/resize` | 2059 | Resize terminal | +| GET | `/api/sessions/:id/terminal` | 2087 | Get terminal buffer | +| POST | `/api/sessions/:id/auto-clear` | 2444 | Toggle auto-clear | +| POST | `/api/sessions/:id/auto-compact` | 2473 | Toggle auto-compact | +| POST | `/api/sessions/:id/image-watcher` | 2503 | Toggle image watcher | +| POST | `/api/sessions/:id/flicker-filter` | 2535 | Toggle flicker filter | +| GET | `/api/sessions/:id/cpu-limit` | 4120 | Get CPU limit | +| POST | `/api/sessions/:id/cpu-limit` | 4134 | Set CPU limit | + +### Implementation Pattern + +```typescript +// src/web/routes/session-routes.ts +import type { FastifyInstance } from 'fastify'; +import type { RouteContext } from '../route-context.js'; +import { findSessionOrFail } from '../route-context.js'; +import { CreateSessionSchema, RunPromptSchema, /* ... */ } from '../schemas.js'; +import { createErrorResponse, ApiErrorCode } from '../../types.js'; +import { Session } from '../../session.js'; + +export function registerSessionRoutes(app: FastifyInstance, ctx: RouteContext): void { + app.get('/api/sessions', async () => /* moved from server.ts */); + + app.post('/api/sessions', async (req) => { + const data = CreateSessionSchema.parse(req.body); + // ... handler body moved verbatim from server.ts + // Replace `this.sessions` with `ctx.sessions` + // Replace `this.broadcast(...)` with `ctx.broadcast(...)` + // Replace `this.persistSessionState(...)` with `ctx.persistSessionState(...)` + }); + + // ... remaining routes +} +``` + +### Migration Strategy for Each Route + +1. **Copy** the route handler body from server.ts to the new file +2. **Replace** all `this.xxx` references with `ctx.xxx` equivalents +3. **Replace** inline session lookups with `findSessionOrFail(ctx, id)` where applicable +4. **Import** schemas, types, and utilities used by the handler +5. **Delete** the route from server.ts +6. **Verify** with `tsc --noEmit` after each batch of routes + +### State Access Patterns in Session Routes + +These routes access WebServer state that must be exposed via RouteContext: + +| State | Used By | +|-------|---------| +| `this.sessions` | All session routes | +| `this.store` | Create, delete, settings | +| `this.mux` | Create (spawn tmux), delete (kill) | +| `this.broadcast()` | Most routes (SSE events) | +| `this.persistSessionState()` | Name, color, auto-ops, cpu-limit | +| `this.cleanupSession()` | Delete | +| `this.setupSessionListeners()` | Create, interactive, shell | +| `this.getSessionStateWithRespawn()` | Get session details | +| `this.batchTerminalData()` | (Indirectly via session listeners) | +| `this.runSummaryTrackers` | Run summary GET | +| `imageWatcher` | Image watcher toggle | + +### Session Creation Helper + +The `POST /api/sessions` handler at line 1017 is ~75 LOC and does complex work (spawn PTY, setup listeners, persist state, broadcast). It uses several internal methods. Consider extracting the create logic into a `createSession()` method on the RouteContext rather than inlining all of it in the route module. + +### Verification + +```bash +tsc --noEmit +npm run lint +# Verify session CRUD works: +npx tsx src/index.ts web --port 3099 & +sleep 3 +# Create session: +curl -s -X POST http://localhost:3099/api/sessions -H 'Content-Type: application/json' \ + -d '{"mode":"shell"}' | jq .id +# List sessions: +curl -s http://localhost:3099/api/sessions | jq length +# Delete session (use ID from create): +curl -s -X DELETE http://localhost:3099/api/sessions/ | jq .success +kill %1 +``` + +--- + +## Task 4: Extract Respawn Routes + +**File to create**: `src/web/routes/respawn-routes.ts` +**File to edit**: `src/web/server.ts` +**Time**: ~1.5 hours + +### Routes to Extract + +| Method | Path | Current Line | Purpose | +|--------|------|-------------|---------| +| GET | `/api/sessions/:id/respawn` | 2142 | Get respawn status | +| GET | `/api/sessions/:id/respawn/config` | 2157 | Get respawn config | +| POST | `/api/sessions/:id/respawn/start` | 2175 | Start respawn | +| POST | `/api/sessions/:id/respawn/stop` | 2221 | Stop respawn | +| PUT | `/api/sessions/:id/respawn/config` | 2256 | Update respawn config | +| POST | `/api/sessions/:id/interactive-respawn` | 2313 | Start interactive respawn | +| POST | `/api/sessions/:id/respawn/enable` | 2388 | Enable/disable respawn | + +### Key Dependencies + +These routes heavily use: +- `ctx.respawnControllers` — get/create/delete controllers +- `ctx.respawnTimers` — timed respawn duration management +- `ctx.setupRespawnListeners()` — wire events for new controllers +- `ctx.setupTimedRespawn()` — set duration timer +- `ctx.saveRespawnConfig()` — persist to mux metadata +- `ctx.persistSessionState()` — update state.json +- `ctx.broadcast()` — SSE events +- `RespawnController` constructor — instantiated in start/interactive-respawn routes + +### Respawn Start Route Complexity + +The `POST /api/sessions/:id/respawn/start` handler (line 2175, ~45 LOC) creates a new `RespawnController`, calls `setupRespawnListeners`, starts it, and optionally sets up timed respawn. This is complex but self-contained — it can move to the route module as-is, with `ctx.setupRespawnListeners()` and `ctx.setupTimedRespawn()` as the bridge back to server.ts. + +### Interactive Respawn Complexity + +`POST /api/sessions/:id/interactive-respawn` (line 2313, ~75 LOC) is the most complex respawn route. It stops existing controllers, creates a new one with different config, and handles the "sendInit" flow. All logic can move to the route module since it only needs `ctx` methods. + +### Verification + +```bash +tsc --noEmit +npm run lint +npx vitest run test/respawn-controller.test.ts # Ensure respawn logic still works +``` + +--- + +## Task 5: Extract Subagent, Mux, and Team Routes + +**File to create**: `src/web/routes/subagent-routes.ts`, `src/web/routes/mux-routes.ts`, `src/web/routes/team-routes.ts` +**File to edit**: `src/web/server.ts` +**Time**: ~2 hours + +### Subagent Routes + +| Method | Path | Line | Purpose | +|--------|------|------|---------| +| GET | `/api/subagents` | 4271 | List all subagents | +| GET | `/api/sessions/:id/subagents` | 4280 | List session subagents | +| GET | `/api/subagents/:agentId` | 4291 | Get single subagent | +| GET | `/api/subagents/:agentId/transcript` | 4301 | Get transcript | +| DELETE | `/api/subagents/:agentId` | 4316 | Kill subagent | +| POST | `/api/subagents/cleanup` | 4331 | Cleanup completed | +| DELETE | `/api/subagents` | 4337 | Kill all subagents | +| GET | `/api/subagent-window-states` | 4162 | Get window positions | +| PUT | `/api/subagent-window-states` | 4174 | Save window positions | +| GET | `/api/subagent-parents` | 4197 | Get parent map | +| PUT | `/api/subagent-parents` | 4209 | Save parent map | + +These routes primarily use the `subagentWatcher` singleton (imported directly, not via ctx) and `ctx.store` for window state persistence. + +### Mux Routes + +| Method | Path | Line | Purpose | +|--------|------|------|---------| +| GET | `/api/mux-sessions` | 4230 | List tmux sessions | +| DELETE | `/api/mux-sessions/:sessionId` | 4239 | Kill tmux session | +| POST | `/api/mux-sessions/reconcile` | 4246 | Reconcile sessions | +| POST | `/api/mux-sessions/stats/start` | 4252 | Start stats polling | +| POST | `/api/mux-sessions/stats/stop` | 4258 | Stop stats polling | +| GET | `/api/system/stats` | 4264 | System CPU/memory | + +These routes only need `ctx.mux` and `getSystemStats()` (move the helper to the route module or route-context). + +### Team Routes + +| Method | Path | Line | Purpose | +|--------|------|------|---------| +| GET | `/api/teams` | 4345 | List teams | +| GET | `/api/teams/:name/tasks` | 4350 | Get team tasks | + +These routes only need `ctx.teamWatcher`. + +### Verification + +```bash +tsc --noEmit +npm run lint +# Verify subagent listing works: +curl -s http://localhost:3099/api/subagents | jq length +``` + +--- + +## Task 6: Extract Plan, Case, and Ralph Routes + +**File to create**: `src/web/routes/plan-routes.ts`, `src/web/routes/case-routes.ts`, `src/web/routes/ralph-routes.ts` +**File to edit**: `src/web/server.ts` +**Time**: ~3 hours + +### Plan Routes + +| Method | Path | Line | Purpose | +|--------|------|------|---------| +| POST | `/api/generate-plan` | 3380 | Generate plan | +| POST | `/api/generate-plan-detailed` | 3574 | Generate detailed plan | +| POST | `/api/cancel-plan-generation` | 3682 | Cancel plan generation | +| PATCH | `/api/sessions/:id/plan/task/:taskId` | 3877 | Update plan task | +| POST | `/api/sessions/:id/plan/checkpoint` | 3909 | Create plan checkpoint | +| GET | `/api/sessions/:id/plan/history` | 3927 | Get plan history | +| POST | `/api/sessions/:id/plan/rollback/:version` | 3943 | Rollback plan | +| POST | `/api/sessions/:id/plan/task` | 3965 | Add plan task | + +These routes use `ctx.activePlanOrchestrators`, `PlanOrchestrator` constructor, and `ctx.broadcast()`. The generate-plan handlers are the most complex (~200 LOC each) because they set up orchestrator event listeners and manage the async plan generation lifecycle. + +### Case Routes + +| Method | Path | Line | Purpose | +|--------|------|------|---------| +| GET | `/api/cases` | 2676 | List cases | +| POST | `/api/cases` | 2718 | Create case | +| POST | `/api/cases/link` | 2760 | Link existing dir as case | +| GET | `/api/cases/:name` | 2818 | Get case details | +| GET | `/api/cases/:name/fix-plan` | 2853 | Get case fix plan | +| GET | `/api/cases/:caseName/ralph-wizard/files` | 3716 | Wizard file listing | +| GET | `/api/cases/:caseName/ralph-wizard/file/:filePath` | 3777 | Wizard file content | + +Case routes use filesystem operations (`existsSync`, `mkdirSync`, `readFileSync`, `writeFileSync`) and the `generateClaudeMd` template function. They're self-contained — the only ctx dependency is `ctx.store` for the cases directory path. + +### Ralph Routes + +| Method | Path | Line | Purpose | +|--------|------|------|---------| +| POST | `/api/sessions/:id/ralph-config` | 1653 | Update Ralph config | +| POST | `/api/sessions/:id/ralph-circuit-breaker/reset` | 1733 | Reset circuit breaker | +| GET | `/api/sessions/:id/ralph-status` | 1746 | Get Ralph status | +| GET | `/api/sessions/:id/fix-plan` | 1766 | Get fix plan | +| POST | `/api/sessions/:id/fix-plan/import` | 1785 | Import fix plan | +| POST | `/api/sessions/:id/fix-plan/write` | 1811 | Write fix plan tasks | +| POST | `/api/sessions/:id/fix-plan/read` | 1842 | Trigger fix plan re-read | +| POST | `/api/sessions/:id/ralph-prompt/write` | 1880 | Write Ralph prompt file | +| POST | `/api/ralph-loop/start` | 3128 | Start Ralph Loop | + +Ralph routes access `session.ralphTracker` methods and `ctx.respawnControllers` for circuit breaker operations. The Ralph Loop start route (line 3128, ~250 LOC) is the most complex — it creates sessions, sets up Ralph Loop mode, and configures respawn. + +### Verification + +```bash +tsc --noEmit +npm run lint +# Verify cases endpoint: +curl -s http://localhost:3099/api/cases | jq length +``` + +--- + +## Task 7: Extract System, Settings, Push, File, and Scheduled Routes + +**Files to create**: `src/web/routes/system-routes.ts`, `src/web/routes/file-routes.ts`, `src/web/routes/push-routes.ts`, `src/web/routes/scheduled-routes.ts` +**File to edit**: `src/web/server.ts` +**Time**: ~3 hours + +### System Routes + +| Method | Path | Line | Purpose | +|--------|------|------|---------| +| GET | `/api/status` | 842 | Full app status (cached) | +| GET | `/api/tunnel/status` | 844 | Tunnel status | +| GET | `/api/tunnel/qr` | 846 | Tunnel QR code | +| GET | `/api/opencode/status` | 862 | OpenCode CLI check | +| POST | `/api/cleanup-state` | 871 | Clean stale sessions from state | +| GET | `/api/session-lifecycle` | 877 | Lifecycle audit log | +| GET | `/api/stats` | 895 | App statistics | +| GET | `/api/token-stats` | 913 | Token usage stats | +| GET | `/api/config` | 931 | Get app config | +| PUT | `/api/config` | 935 | Update app config | +| GET | `/api/debug/memory` | 947 | Debug memory usage | +| POST | `/api/logout` | 833 | Clear auth cookie | +| GET | `/api/settings` | 3991 | Get global settings | +| PUT | `/api/settings` | 4003 | Update global settings | +| GET | `/api/execution/model-config` | 4074 | Get model config | +| PUT | `/api/execution/model-config` | 4087 | Update model config | + +The `getLightState()` and `getLightSessionsState()` cached methods remain in server.ts (they access the cache fields) and are exposed via RouteContext. The system routes just call them. + +The `getSystemStats()` helper (lines 4710-4753) moves to `system-routes.ts` since it has no server state dependencies (only `os` module calls). + +### File Routes + +| Method | Path | Line | Purpose | +|--------|------|------|---------| +| GET | `/api/sessions/:id/files` | 1260 | File tree browser | +| GET | `/api/sessions/:id/file-content` | 1388 | File content preview | +| GET | `/api/sessions/:id/file-raw` | 1498 | Raw file download | +| GET | `/api/sessions/:id/tail-file` | 1575 | Live file tail (SSE) | +| DELETE | `/api/sessions/:id/tail-file/:streamId` | 1640 | Stop file tail | +| POST | `/api/screenshots` | 4455 | Upload screenshot | +| GET | `/api/screenshots` | 4542 | List screenshots | +| GET | `/api/screenshots/:name` | 4556 | Serve screenshot | + +File routes are the most self-contained group — they use `fs` operations, path validation, and `fileStreamManager`. The only ctx dependencies are `ctx.sessions` (to verify session exists and get working dir) and `ctx.broadcast()` (for screenshot upload notification). + +**TOCTOU security**: The file-raw route has a critical `realpathSync()` double-check for symlink TOCTOU protection. Preserve this exactly when moving. + +### Push Routes + +| Method | Path | Line | Purpose | +|--------|------|------|---------| +| GET | `/api/push/vapid-key` | 4410 | Get VAPID public key | +| POST | `/api/push/subscribe` | 4414 | Register push subscription | +| PUT | `/api/push/subscribe/:id` | 4431 | Update subscription prefs | +| DELETE | `/api/push/subscribe/:id` | 4444 | Unsubscribe | + +Push routes only need `ctx.pushStore`. Very self-contained. + +### Scheduled Routes + +| Method | Path | Line | Purpose | +|--------|------|------|---------| +| POST | `/api/run` | 2561 | Quick run (create + run prompt) | +| GET | `/api/scheduled` | 2620 | List scheduled runs | +| POST | `/api/scheduled` | 2624 | Create scheduled run | +| DELETE | `/api/scheduled/:id` | 2650 | Cancel scheduled run | +| GET | `/api/scheduled/:id` | 2662 | Get scheduled run | +| POST | `/api/quick-start` | 2965 | Quick start (case + session + hooks) | + +Quick-start (line 2965, ~160 LOC) is complex: it creates a case directory, writes CLAUDE.md, configures hooks, creates a session, and starts respawn. It uses many ctx methods. Consider keeping it intact as a single large handler in the route module. + +### Hook Event Route + +| Method | Path | Line | Purpose | +|--------|------|------|---------| +| POST | `/api/hook-event` | 4357 | Receive Claude Code hook events | + +This route is special: it's exempt from auth (localhost-only), validates with `HookEventSchema`, and broadcasts `hook:{eventName}` events. It also handles Web Push notifications. Place it in `system-routes.ts` or its own `hook-routes.ts`. + +### SSE Route + +| Method | Path | Line | Purpose | +|--------|------|------|---------| +| GET | `/api/events` | 803 | SSE connection endpoint | + +The SSE endpoint (line 803, ~30 LOC) sets up headers, adds the client to `sseClients`, sends initial state, and handles cleanup. This can stay in server.ts since it directly manages the SSE client set, or move to system-routes with `sseClients` exposed via RouteContext. + +**Recommendation**: Keep the SSE endpoint in server.ts — it's tightly coupled to the broadcast infrastructure and only ~30 LOC. + +### Verification + +```bash +tsc --noEmit +npm run lint +npm run format:check +# Comprehensive verification: +npx tsx src/index.ts web --port 3099 & +sleep 3 +curl -s http://localhost:3099/api/status | jq .status +curl -s http://localhost:3099/api/settings | jq 'keys' +curl -s http://localhost:3099/api/push/vapid-key | jq .publicKey +kill %1 +``` + +--- + +## Task 8: Final Cleanup — Reduce server.ts + +**File to edit**: `src/web/server.ts` +**Time**: ~2 hours + +### What Remains in server.ts + +After Tasks 2-7, server.ts should contain only: + +1. **Class fields** (~60 LOC) — Maps, timers, config +2. **Constructor** (~50 LOC) — Fastify init, mux creation, watcher setup +3. **`setupRoutes()`** (~50 LOC) — Plugin registration + route module imports: + ```typescript + private async setupRoutes(): Promise { + // Plugins + await this.app.register(fastifyCompress, { threshold: 1024 }); + await this.app.register(fastifyCookie); + this.app.addContentTypeParser('multipart/form-data', (_req, _payload, done) => done(null)); + + // Auth + const { authSessions, authFailures } = registerAuthMiddleware(this.app, this.https); + this.authSessions = authSessions; + this.authFailures = authFailures; + registerSecurityHeaders(this.app, this.https); + + // Error handler + this.app.setErrorHandler((error, _req, reply) => { /* ... */ }); + + // Static files + await this.app.register(fastifyStatic, { /* ... */ }); + + // Route modules + const ctx = this.createRouteContext(); + registerSessionRoutes(this.app, ctx); + registerRespawnRoutes(this.app, ctx); + registerSubagentRoutes(this.app, ctx); + registerMuxRoutes(this.app, ctx); + registerTeamRoutes(this.app, ctx); + registerPlanRoutes(this.app, ctx); + registerCaseRoutes(this.app, ctx); + registerRalphRoutes(this.app, ctx); + registerSystemRoutes(this.app, ctx); + registerFileRoutes(this.app, ctx); + registerPushRoutes(this.app, ctx); + registerScheduledRoutes(this.app, ctx); + + // SSE endpoint (kept here — tightly coupled to broadcast infra) + this.app.get('/api/events', (req, reply) => { /* ... */ }); + + // Service worker route (kept here — 20 LOC) + this.app.get('/sw.js', async (_req, reply) => { /* ... */ }); + } + ``` +4. **`createRouteContext()`** (~40 LOC) — Build context object +5. **`start()`** (~90 LOC) — Server startup, session restoration +6. **`stop()`** (~170 LOC) — Graceful shutdown +7. **Session listener setup/teardown** (~200 LOC) — `setupSessionListeners`, `removeSessionListeners` +8. **Respawn lifecycle** (~200 LOC) — `setupRespawnListeners`, `setupTimedRespawn`, `restoreRespawnController`, `saveRespawnConfig` +9. **Watcher setup** (~100 LOC) — `setupSubagentWatcherListeners`, `setupImageWatcherListeners`, `setupTeamWatcherListeners` +10. **SSE infrastructure** (~100 LOC) — `broadcast`, `sendSSE`, `sendSSEPreformatted` +11. **Terminal batching** (~80 LOC) — `batchTerminalData`, `flushSessionTerminalBatch` +12. **Task/state update batching** (~60 LOC) — `batchTaskUpdate`, `broadcastSessionStateDebounced` +13. **State persistence** (~60 LOC) — `persistSessionState`, `_persistSessionStateNow` +14. **Session cleanup** (~120 LOC) — `cleanupSession`, `_doCleanupSession` +15. **Transcript watchers** (~50 LOC) — `startTranscriptWatcher`, `stopTranscriptWatcher` +16. **State caching** (~60 LOC) — `getLightState`, `getLightSessionsState` + +**Estimated total**: ~1,300-1,500 LOC + +### Cleanup Steps + +1. **Remove all extracted route handlers** from `setupRoutes()` — after all route modules are imported and registered, the remaining inline routes should be zero (except SSE and sw.js). +2. **Remove unused imports** — many imports at the top of server.ts were only used by route handlers (e.g., `generateClaudeMd`, `parseRalphLoopConfig`). Delete them. +3. **Move constants** that only route modules use to the route modules. Constants used by server core (batching intervals, cache TTLs) stay. +4. **Remove the `ScheduledRun` type** from server.ts if it was moved to route-context.ts in Task 1. +5. **Verify no dead code** remains — run `tsc --noEmit` with strict unused-variable checking. + +### Verification (Comprehensive) + +```bash +# 1. TypeScript +tsc --noEmit + +# 2. Lint + format +npm run lint +npm run format:check + +# 3. Line count verification +wc -l src/web/server.ts +# Should be ~1,300-1,500 LOC + +# 4. Route count verification (should match original ~110) +grep -c "app\.\(get\|post\|put\|patch\|delete\)(" src/web/routes/*.ts src/web/server.ts + +# 5. Integration test — start server, verify key endpoints +npx tsx src/index.ts web --port 3099 & +sleep 3 + +# System +curl -s http://localhost:3099/api/status | jq .status +curl -s http://localhost:3099/api/config | jq .version + +# Sessions +SESS=$(curl -s -X POST http://localhost:3099/api/sessions -H 'Content-Type: application/json' \ + -d '{"mode":"shell"}' | jq -r .id) +curl -s http://localhost:3099/api/sessions | jq length +curl -s http://localhost:3099/api/sessions/$SESS | jq .id + +# Subagents +curl -s http://localhost:3099/api/subagents | jq length + +# Cases +curl -s http://localhost:3099/api/cases | jq length + +# Settings +curl -s http://localhost:3099/api/settings | jq 'keys' + +# Push +curl -s http://localhost:3099/api/push/vapid-key | jq .publicKey + +# Mux +curl -s http://localhost:3099/api/mux-sessions | jq length + +# Cleanup +curl -s -X DELETE http://localhost:3099/api/sessions/$SESS | jq .success +kill %1 + +# 6. Run any existing tests +npx vitest run test/respawn-controller.test.ts +npx vitest run test/session-manager.test.ts +``` + +--- + +## Risk Assessment + +| Risk | Likelihood | Impact | Mitigation | +|------|-----------|--------|------------| +| `this` binding lost when methods passed via RouteContext | Medium | High (runtime crash) | Use `.bind(this)` in `createRouteContext()` for all methods | +| Circular dependency between server.ts and route modules | Low | High (import crash) | Route modules only import `route-context.ts`, never `server.ts` | +| Route handler accesses private field not in RouteContext | High | Medium (compile error) | Audit each route's `this.xxx` usage before moving; add to RouteContext as needed | +| Auth middleware ordering changes | Low | High (security) | Keep `addHook('onRequest')` registration before all routes | +| SSE endpoint moved loses access to sseClients | Low | Medium | Keep SSE endpoint in server.ts | +| Forgotten import after extraction | Medium | Low (compile error) | `tsc --noEmit` catches immediately | +| Performance regression from context indirection | Very Low | Low | Context is a plain object; zero overhead vs `this.xxx` | + +--- + +## What NOT to Touch + +- **`app.js`** (frontend monolith) — that's Phase 5 +- **`schemas.ts`** — validation schemas stay as-is +- **Terminal batching internals** — `batchTerminalData`, `flushSessionTerminalBatch` stay in server.ts +- **SSE infrastructure** — `broadcast`, `sendSSE`, `sendSSEPreformatted` stay in server.ts +- **Session listener wiring** — complex event handler setup stays in server.ts +- **Respawn lifecycle methods** — `setupRespawnListeners`, `restoreRespawnController` stay in server.ts +- **Session cleanup** — `cleanupSession`, `_doCleanupSession` stay in server.ts (complex cross-cutting logic) +- **State caching** — `getLightState`, `getLightSessionsState` stay in server.ts + +The goal is to extract **route definitions** (HTTP handler logic), not **orchestration** (lifecycle, events, batching). + +--- + +## Summary of Changes + +| Task | Files Created | Key Change | +|------|-------------|------------| +| 1. RouteContext | `src/web/route-context.ts` | Shared interface + `findSessionOrFail` helper | +| 2. Auth middleware | `src/web/middleware/auth.ts` | Auth hooks + security headers extracted | +| 3. Session routes | `src/web/routes/session-routes.ts` | ~23 routes, ~800 LOC extracted | +| 4. Respawn routes | `src/web/routes/respawn-routes.ts` | ~7 routes, ~400 LOC extracted | +| 5. Subagent/mux/team | `src/web/routes/subagent-routes.ts`, `mux-routes.ts`, `team-routes.ts` | ~18 routes, ~380 LOC extracted | +| 6. Plan/case/ralph | `src/web/routes/plan-routes.ts`, `case-routes.ts`, `ralph-routes.ts` | ~24 routes, ~1350 LOC extracted | +| 7. System/file/push/sched | `src/web/routes/system-routes.ts`, `file-routes.ts`, `push-routes.ts`, `scheduled-routes.ts` | ~28 routes, ~1500 LOC extracted | +| 8. Final cleanup | `src/web/server.ts` reduced | ~5,200 LOC removed from server.ts | + +**Total files created**: 14 (1 interface + 1 middleware + 12 route modules) +**Total files modified**: 1 (server.ts) +**Net LOC change**: ~0 (moved, not deleted — but server.ts drops from ~6,710 to ~1,500) +**Route count preserved**: ~110 routes (verified by grep after extraction) diff --git a/src/bash-tool-parser.ts b/src/bash-tool-parser.ts index eee92db0..cb72435c 100644 --- a/src/bash-tool-parser.ts +++ b/src/bash-tool-parser.ts @@ -15,6 +15,7 @@ import { EventEmitter } from 'node:events'; import { v4 as uuidv4 } from 'uuid'; import { ActiveBashTool } from './types.js'; +import { CleanupManager, Debouncer } from './utils/index.js'; // ========== Configuration Constants ========== @@ -145,15 +146,14 @@ export class BashToolParser extends EventEmitter { private _workingDir: string; private _homeDir: string; - // Track auto-remove timers for cleanup - private _autoRemoveTimers: Set> = new Set(); + // Centralized resource cleanup for auto-remove timers + private cleanup = new CleanupManager(); // Flag to prevent operations after destroy private _destroyed: boolean = false; // Debouncing - private _pendingUpdate: boolean = false; - private _updateTimer: ReturnType | null = null; + private _updateDeb = new Debouncer(EVENT_DEBOUNCE_MS); constructor(config: BashToolParserConfig) { super(); @@ -524,13 +524,15 @@ export class BashToolParser extends EventEmitter { this.scheduleUpdate(); // Remove completed tool after a short delay to allow UI to show completion - const timer = setTimeout(() => { - this._autoRemoveTimers.delete(timer); - if (this._destroyed) return; - this._activeTools.delete(tool.id); - this.scheduleUpdate(); - }, 2000); - this._autoRemoveTimers.add(timer); + this.cleanup.setTimeout( + () => { + if (this._destroyed) return; + this._activeTools.delete(tool.id); + this.scheduleUpdate(); + }, + 2000, + { description: 'auto-remove completed tool' } + ); } this._lastToolId = null; return; @@ -562,13 +564,15 @@ export class BashToolParser extends EventEmitter { this.scheduleUpdate(); // Auto-remove suggestions after 30 seconds - const timer = setTimeout(() => { - this._autoRemoveTimers.delete(timer); - if (this._destroyed) return; - this._activeTools.delete(tool.id); - this.scheduleUpdate(); - }, 30000); - this._autoRemoveTimers.add(timer); + this.cleanup.setTimeout( + () => { + if (this._destroyed) return; + this._activeTools.delete(tool.id); + this.scheduleUpdate(); + }, + 30000, + { description: 'auto-remove suggestion tool' } + ); return; } @@ -599,13 +603,15 @@ export class BashToolParser extends EventEmitter { this.scheduleUpdate(); // Auto-remove after 60 seconds - const timer = setTimeout(() => { - this._autoRemoveTimers.delete(timer); - if (this._destroyed) return; - this._activeTools.delete(tool.id); - this.scheduleUpdate(); - }, 60000); - this._autoRemoveTimers.add(timer); + this.cleanup.setTimeout( + () => { + if (this._destroyed) return; + this._activeTools.delete(tool.id); + this.scheduleUpdate(); + }, + 60000, + { description: 'auto-remove log file tool' } + ); } } @@ -675,13 +681,9 @@ export class BashToolParser extends EventEmitter { * Schedule a debounced update emission. */ private scheduleUpdate(): void { - if (this._pendingUpdate) return; - - this._pendingUpdate = true; - this._updateTimer = setTimeout(() => { - this._pendingUpdate = false; + this._updateDeb.schedule(() => { this.emitUpdate(); - }, EVENT_DEBOUNCE_MS); + }); } /** @@ -696,15 +698,8 @@ export class BashToolParser extends EventEmitter { */ destroy(): void { this._destroyed = true; - if (this._updateTimer) { - clearTimeout(this._updateTimer); - this._updateTimer = null; - } - // Clear all auto-remove timers to prevent orphaned callbacks - for (const timer of this._autoRemoveTimers) { - clearTimeout(timer); - } - this._autoRemoveTimers.clear(); + this._updateDeb.dispose(); + this.cleanup.dispose(); this._activeTools.clear(); this.removeAllListeners(); } diff --git a/src/config/exec-timeout.ts b/src/config/exec-timeout.ts new file mode 100644 index 00000000..79c4dd4d --- /dev/null +++ b/src/config/exec-timeout.ts @@ -0,0 +1,10 @@ +/** + * @fileoverview Shared exec timeout constant. + * + * Used by CLI resolvers and tmux-manager for execSync/exec calls. + * + * @module config/exec-timeout + */ + +/** Timeout for exec commands (5 seconds) */ +export const EXEC_TIMEOUT_MS = 5000; diff --git a/src/image-watcher.ts b/src/image-watcher.ts index 84812df8..280dba9b 100644 --- a/src/image-watcher.ts +++ b/src/image-watcher.ts @@ -13,6 +13,7 @@ import { watch, type FSWatcher } from 'chokidar'; import { basename, extname, relative } from 'node:path'; import { statSync } from 'node:fs'; import type { ImageDetectedEvent } from './types.js'; +import { KeyedDebouncer } from './utils/index.js'; // ========== Types ========== @@ -65,11 +66,11 @@ export class ImageWatcher extends EventEmitter { /** Map of sessionId -> working directory path */ private sessionDirs = new Map(); - /** Debounce timers for rapid image creation (keyed by filePath) */ - private debounceTimers = new Map(); + /** Per-file debouncer for rapid image creation */ + private fileDeb = new KeyedDebouncer(DEBOUNCE_DELAY_MS); - /** Track which session owns each debounce timer (for cleanup) */ - private timerToSession = new Map(); + /** Track which session owns each debounced file (for cleanup) */ + private fileToSession = new Map(); /** Per-session burst tracking: sessionId -> { count, windowStart } */ private burstTrackers = new Map(); @@ -118,11 +119,8 @@ export class ImageWatcher extends EventEmitter { this.sessionDirs.clear(); // Clear all debounce timers - for (const timer of this.debounceTimers.values()) { - clearTimeout(timer); - } - this.debounceTimers.clear(); - this.timerToSession.clear(); + this.fileDeb.dispose(); + this.fileToSession.clear(); this.burstTrackers.clear(); } @@ -212,20 +210,15 @@ export class ImageWatcher extends EventEmitter { this.sessionDirs.delete(sessionId); // Clear any pending debounce timers for this session - // Collect keys first to avoid iterator invalidation during deletion - const toDelete: string[] = []; - for (const [filePath, ownerId] of this.timerToSession) { + const toCancel: string[] = []; + for (const [filePath, ownerId] of this.fileToSession) { if (ownerId === sessionId) { - toDelete.push(filePath); + toCancel.push(filePath); } } - for (const filePath of toDelete) { - const timer = this.debounceTimers.get(filePath); - if (timer) { - clearTimeout(timer); - this.debounceTimers.delete(filePath); - } - this.timerToSession.delete(filePath); + for (const filePath of toCancel) { + this.fileDeb.cancelKey(filePath); + this.fileToSession.delete(filePath); } this.burstTrackers.delete(sessionId); } @@ -269,22 +262,15 @@ export class ImageWatcher extends EventEmitter { } // Debounce rapid file creation (e.g., multiple screenshots quickly) - const existingTimer = this.debounceTimers.get(filePath); - if (existingTimer) { - clearTimeout(existingTimer); - } - - const timer = setTimeout(() => { - this.debounceTimers.delete(filePath); - this.timerToSession.delete(filePath); + this.fileDeb.schedule(filePath, () => { + this.fileToSession.delete(filePath); this.emitImageDetected(sessionId, filePath); // Increment burst count on actual emission (not on detection) const b = this.burstTrackers.get(sessionId); if (b) b.count++; - }, DEBOUNCE_DELAY_MS); + }); - this.debounceTimers.set(filePath, timer); - this.timerToSession.set(filePath, sessionId); + this.fileToSession.set(filePath, sessionId); } /** diff --git a/src/push-store.ts b/src/push-store.ts index 9a27ae76..3b610838 100644 --- a/src/push-store.ts +++ b/src/push-store.ts @@ -11,6 +11,7 @@ import { join } from 'node:path'; import { homedir } from 'node:os'; import webpush from 'web-push'; import type { VapidKeys, PushSubscriptionRecord } from './types.js'; +import { Debouncer } from './utils/index.js'; const DATA_DIR = join(homedir(), '.codeman'); const KEYS_FILE = join(DATA_DIR, 'push-keys.json'); @@ -20,7 +21,7 @@ const SAVE_DEBOUNCE_MS = 500; export class PushSubscriptionStore { private vapidKeys: VapidKeys | null = null; private subscriptions: Map = new Map(); - private saveTimer: NodeJS.Timeout | null = null; + private saveDeb = new Debouncer(SAVE_DEBOUNCE_MS); private _disposed = false; constructor() { @@ -149,10 +150,7 @@ export class PushSubscriptionStore { /** Schedule a debounced save */ private scheduleSave(): void { if (this._disposed) return; - if (this.saveTimer) clearTimeout(this.saveTimer); - this.saveTimer = setTimeout(() => { - this.flushSave(); - }, SAVE_DEBOUNCE_MS); + this.saveDeb.schedule(() => this.flushSave()); } /** Immediately persist subscriptions to disk */ @@ -169,11 +167,6 @@ export class PushSubscriptionStore { dispose(): void { if (this._disposed) return; this._disposed = true; - if (this.saveTimer) { - clearTimeout(this.saveTimer); - this.saveTimer = null; - } - // Final flush - this.flushSave(); + this.saveDeb.flush(() => this.flushSave()); } } diff --git a/src/ralph-tracker.ts b/src/ralph-tracker.ts index 46e5e9a6..40142ea9 100644 --- a/src/ralph-tracker.ts +++ b/src/ralph-tracker.ts @@ -34,7 +34,14 @@ import { PlanTaskStatus, TddPhase, } from './types.js'; -import { ANSI_ESCAPE_PATTERN_SIMPLE, fuzzyPhraseMatch, todoContentHash, stringSimilarity } from './utils/index.js'; +import { + ANSI_ESCAPE_PATTERN_SIMPLE, + CleanupManager, + Debouncer, + fuzzyPhraseMatch, + todoContentHash, + stringSimilarity, +} from './utils/index.js'; import { MAX_LINE_BUFFER_SIZE } from './config/buffer-limits.js'; import { MAX_TODOS_PER_SESSION } from './config/map-limits.js'; @@ -559,17 +566,14 @@ export class RalphTracker extends EventEmitter { /** Timestamp of last cleanup check for throttling */ private _lastCleanupTime: number = 0; - /** Debounce timer for todoUpdate events */ - private _todoUpdateTimer: NodeJS.Timeout | null = null; + /** Centralized resource cleanup for timers/intervals */ + private cleanup = new CleanupManager(); - /** Debounce timer for loopUpdate events */ - private _loopUpdateTimer: NodeJS.Timeout | null = null; + /** Debouncer for todoUpdate events */ + private _todoUpdateDeb = new Debouncer(EVENT_DEBOUNCE_MS); - /** Flag indicating pending todoUpdate emission */ - private _todoUpdatePending: boolean = false; - - /** Flag indicating pending loopUpdate emission */ - private _loopUpdatePending: boolean = false; + /** Debouncer for loopUpdate events */ + private _loopUpdateDeb = new Debouncer(EVENT_DEBOUNCE_MS); /** When true, prevents auto-enable on pattern detection */ private _autoEnableDisabled: boolean = true; @@ -621,8 +625,8 @@ export class RalphTracker extends EventEmitter { /** Error handler for FSWatcher (stored for cleanup to prevent memory leak) */ private _fixPlanWatcherErrorHandler: ((err: Error) => void) | null = null; - /** Debounce timer for file change events */ - private _fixPlanReloadTimer: NodeJS.Timeout | null = null; + /** Debouncer for file change events */ + private _fixPlanReloadDeb = new Debouncer(500); /** Path to the @fix_plan.md file being watched */ private _fixPlanPath: string | null = null; @@ -665,8 +669,8 @@ export class RalphTracker extends EventEmitter { /** Last observed iteration count for stall detection */ private _lastObservedIteration: number = 0; - /** Timer for iteration stall detection */ - private _iterationStallTimer: NodeJS.Timeout | null = null; + /** CleanupManager registration ID for iteration stall detection interval */ + private _iterationStallTimerId: string | null = null; /** Iteration stall warning threshold (ms) - default 10 minutes */ private _iterationStallWarningMs: number = 10 * 60 * 1000; @@ -869,14 +873,9 @@ export class RalphTracker extends EventEmitter { */ private handleFixPlanChange(): void { // Debounce rapid changes (e.g., multiple writes) - if (this._fixPlanReloadTimer) { - clearTimeout(this._fixPlanReloadTimer); - } - - this._fixPlanReloadTimer = setTimeout(() => { - this._fixPlanReloadTimer = null; + this._fixPlanReloadDeb.schedule(() => { this.loadFixPlanFromDisk(); - }, 500); // 500ms debounce + }); } /** @@ -892,10 +891,7 @@ export class RalphTracker extends EventEmitter { this._fixPlanWatcher.close(); this._fixPlanWatcher = null; } - if (this._fixPlanReloadTimer) { - clearTimeout(this._fixPlanReloadTimer); - this._fixPlanReloadTimer = null; - } + this._fixPlanReloadDeb.cancel(); } /** @@ -1023,16 +1019,8 @@ export class RalphTracker extends EventEmitter { * Called during reset/fullReset to prevent stale emissions. */ private clearDebounceTimers(): void { - if (this._todoUpdateTimer) { - clearTimeout(this._todoUpdateTimer); - this._todoUpdateTimer = null; - } - if (this._loopUpdateTimer) { - clearTimeout(this._loopUpdateTimer); - this._loopUpdateTimer = null; - } - this._todoUpdatePending = false; - this._loopUpdatePending = false; + this._todoUpdateDeb.cancel(); + this._loopUpdateDeb.cancel(); } /** @@ -1041,19 +1029,9 @@ export class RalphTracker extends EventEmitter { * The event fires after EVENT_DEBOUNCE_MS of inactivity. */ private emitTodoUpdateDebounced(): void { - this._todoUpdatePending = true; - - if (this._todoUpdateTimer) { - clearTimeout(this._todoUpdateTimer); - } - - this._todoUpdateTimer = setTimeout(() => { - if (this._todoUpdatePending) { - this._todoUpdatePending = false; - this._todoUpdateTimer = null; - this.emit('todoUpdate', this.todos); - } - }, EVENT_DEBOUNCE_MS); + this._todoUpdateDeb.schedule(() => { + this.emit('todoUpdate', this.todos); + }); } /** @@ -1062,19 +1040,9 @@ export class RalphTracker extends EventEmitter { * The event fires after EVENT_DEBOUNCE_MS of inactivity. */ private emitLoopUpdateDebounced(): void { - this._loopUpdatePending = true; - - if (this._loopUpdateTimer) { - clearTimeout(this._loopUpdateTimer); - } - - this._loopUpdateTimer = setTimeout(() => { - if (this._loopUpdatePending) { - this._loopUpdatePending = false; - this._loopUpdateTimer = null; - this.emit('loopUpdate', this.loopState); - } - }, EVENT_DEBOUNCE_MS); + this._loopUpdateDeb.schedule(() => { + this.emit('loopUpdate', this.loopState); + }); } /** @@ -1082,21 +1050,11 @@ export class RalphTracker extends EventEmitter { * Useful for testing or when immediate state sync is needed. */ flushPendingEvents(): void { - if (this._todoUpdatePending) { - this._todoUpdatePending = false; - if (this._todoUpdateTimer) { - clearTimeout(this._todoUpdateTimer); - this._todoUpdateTimer = null; - } - this.emit('todoUpdate', this.todos); + if (this._todoUpdateDeb.isPending) { + this._todoUpdateDeb.flush(() => this.emit('todoUpdate', this.todos)); } - if (this._loopUpdatePending) { - this._loopUpdatePending = false; - if (this._loopUpdateTimer) { - clearTimeout(this._loopUpdateTimer); - this._loopUpdateTimer = null; - } - this.emit('loopUpdate', this.loopState); + if (this._loopUpdateDeb.isPending) { + this._loopUpdateDeb.flush(() => this.emit('loopUpdate', this.loopState)); } } @@ -1116,18 +1074,22 @@ export class RalphTracker extends EventEmitter { this._iterationStallWarned = false; // Check every minute - this._iterationStallTimer = setInterval(() => { - this.checkIterationStall(); - }, 60 * 1000); + this._iterationStallTimerId = this.cleanup.setInterval( + () => { + this.checkIterationStall(); + }, + 60 * 1000, + { description: 'iteration stall detection' } + ); } /** * Stop iteration stall detection timer. */ stopIterationStallDetection(): void { - if (this._iterationStallTimer) { - clearInterval(this._iterationStallTimer); - this._iterationStallTimer = null; + if (this._iterationStallTimerId) { + this.cleanup.unregister(this._iterationStallTimerId); + this._iterationStallTimerId = null; } } @@ -3888,7 +3850,8 @@ export class RalphTracker extends EventEmitter { destroy(): void { this.clearDebounceTimers(); this.stopWatchingFixPlan(); - this.stopIterationStallDetection(); + this._fixPlanReloadDeb.dispose(); + this.cleanup.dispose(); this._todos.clear(); this._taskNumberToContent.clear(); this._todoStartTimes.clear(); diff --git a/src/respawn-controller.ts b/src/respawn-controller.ts index be9ee65c..ce5a611a 100644 --- a/src/respawn-controller.ts +++ b/src/respawn-controller.ts @@ -41,7 +41,7 @@ import { AiIdleChecker, type AiCheckResult, type AiCheckState } from './ai-idle- import { AiPlanChecker, type AiPlanCheckResult } from './ai-plan-checker.js'; import type { TeamWatcher } from './team-watcher.js'; import { BufferAccumulator } from './utils/buffer-accumulator.js'; -import { ANSI_ESCAPE_PATTERN_SIMPLE, TOKEN_PATTERN, assertNever } from './utils/index.js'; +import { ANSI_ESCAPE_PATTERN_SIMPLE, TOKEN_PATTERN, assertNever, CleanupManager } from './utils/index.js'; import { MAX_RESPAWN_BUFFER_SIZE, TRIM_RESPAWN_BUFFER_TO as RESPAWN_BUFFER_TRIM_SIZE } from './config/buffer-limits.js'; import type { RespawnCycleMetrics, @@ -641,26 +641,29 @@ export class RespawnController extends EventEmitter { /** Current state machine state */ private _state: RespawnState = 'stopped'; - /** Timer for step delays */ - private stepTimer: NodeJS.Timeout | null = null; + /** Centralized resource cleanup manager for timers */ + private cleanup = new CleanupManager(); - /** Timer for completion confirmation (Layer 2) */ - private completionConfirmTimer: NodeJS.Timeout | null = null; + /** Timer ID for step delays */ + private stepTimerId: string | null = null; - /** Timer for no-output fallback (Layer 5) */ - private noOutputTimer: NodeJS.Timeout | null = null; + /** Timer ID for completion confirmation (Layer 2) */ + private completionConfirmTimerId: string | null = null; - /** Timer for periodic detection status updates */ - private detectionUpdateTimer: NodeJS.Timeout | null = null; + /** Timer ID for no-output fallback (Layer 5) */ + private noOutputTimerId: string | null = null; + + /** Timer ID for periodic detection status updates */ + private detectionUpdateTimerId: string | null = null; /** Cached key fields from last emitted detection status (for dedup) */ private lastEmittedDetectionKey: string = ''; - /** Timer for auto-accepting plan mode prompts */ - private autoAcceptTimer: NodeJS.Timeout | null = null; + /** Timer ID for auto-accepting plan mode prompts */ + private autoAcceptTimerId: string | null = null; - /** Timer for pre-filter silence detection (triggers AI check) */ - private preFilterTimer: NodeJS.Timeout | null = null; + /** Timer ID for pre-filter silence detection (triggers AI check) */ + private preFilterTimerId: string | null = null; /** Whether any terminal output has been received since start/last-auto-accept */ private hasReceivedOutput: boolean = false; @@ -682,8 +685,8 @@ export class RespawnController extends EventEmitter { /** Timestamp when idle_prompt was received */ private idlePromptTime: number | null = null; - /** Timer for short confirmation after hook signal (handles race conditions) */ - private hookConfirmTimer: NodeJS.Timeout | null = null; + /** Timer ID for short confirmation after hook signal (handles race conditions) */ + private hookConfirmTimerId: string | null = null; /** Confirmation delay after hook signal before confirming idle (ms) */ private static readonly HOOK_CONFIRM_DELAY_MS = 3000; @@ -718,11 +721,11 @@ export class RespawnController extends EventEmitter { /** Unique ID for current AI check request (to detect stale results) */ private _currentAiCheckId: string | null = null; - /** Timer for /clear step fallback (sends /init if no prompt detected) */ - private clearFallbackTimer: NodeJS.Timeout | null = null; + /** Timer ID for /clear step fallback (sends /init if no prompt detected) */ + private clearFallbackTimerId: string | null = null; - /** Timer for step completion confirmation (waits for silence after completion) */ - private stepConfirmTimer: NodeJS.Timeout | null = null; + /** Timer ID for step completion confirmation (waits for silence after completion) */ + private stepConfirmTimerId: string | null = null; /** Fallback timeout for /clear step (ms) - sends /init without waiting for prompt */ private static readonly CLEAR_FALLBACK_TIMEOUT_MS = 10000; @@ -741,8 +744,8 @@ export class RespawnController extends EventEmitter { /** Timestamp when the current state was entered */ private stateEnteredAt: number = 0; - /** Timer for stuck-state detection */ - private stuckStateTimer: NodeJS.Timeout | null = null; + /** Timer ID for stuck-state detection */ + private stuckStateTimerId: string | null = null; /** Whether a stuck-state warning has been emitted for current state */ private stuckStateWarned: boolean = false; @@ -1200,31 +1203,35 @@ export class RespawnController extends EventEmitter { this.stopDetectionUpdates(); if (this._state === 'stopped') return; this.lastEmittedDetectionKey = ''; - this.detectionUpdateTimer = setInterval(() => { - try { - if (this._state !== 'stopped') { - const status = this.getDetectionStatus(); - // Only emit when status meaningfully changed (confidence, state text, or timer values) - // to avoid broadcasting identical data every 2s for stable/idle sessions. - const key = `${status.confidenceLevel}|${status.statusText}|${this._state}`; - if (key !== this.lastEmittedDetectionKey) { - this.lastEmittedDetectionKey = key; - this.emit('detectionUpdate', status); + this.detectionUpdateTimerId = this.cleanup.setInterval( + () => { + try { + if (this._state !== 'stopped') { + const status = this.getDetectionStatus(); + // Only emit when status meaningfully changed (confidence, state text, or timer values) + // to avoid broadcasting identical data every 2s for stable/idle sessions. + const key = `${status.confidenceLevel}|${status.statusText}|${this._state}`; + if (key !== this.lastEmittedDetectionKey) { + this.lastEmittedDetectionKey = key; + this.emit('detectionUpdate', status); + } } + } catch (err) { + console.error(`[RespawnController] Error in detectionUpdateTimer:`, err); } - } catch (err) { - console.error(`[RespawnController] Error in detectionUpdateTimer:`, err); - } - }, 2000); + }, + 2000, + { description: 'detection status updates' } + ); } /** * Stop periodic detection status updates. */ private stopDetectionUpdates(): void { - if (this.detectionUpdateTimer) { - clearInterval(this.detectionUpdateTimer); - this.detectionUpdateTimer = null; + if (this.detectionUpdateTimerId) { + this.cleanup.unregister(this.detectionUpdateTimerId); + this.detectionUpdateTimerId = null; } } @@ -1515,8 +1522,8 @@ export class RespawnController extends EventEmitter { this.lastWorkingPatternTime = now; // Cancel hook confirmation timer if running - this.cancelTrackedTimer('hook-confirm', this.hookConfirmTimer, 'working patterns detected'); - this.hookConfirmTimer = null; + this.cancelTrackedTimer('hook-confirm', this.hookConfirmTimerId, 'working patterns detected'); + this.hookConfirmTimerId = null; // Cancel any pending completion confirmation this.cancelCompletionConfirm(); @@ -1650,8 +1657,8 @@ export class RespawnController extends EventEmitter { */ private checkClearComplete(): void { // Clear the fallback timer since we got prompt detection - this.cancelTrackedTimer('clear-fallback', this.clearFallbackTimer, 'prompt detected'); - this.clearFallbackTimer = null; + this.cancelTrackedTimer('clear-fallback', this.clearFallbackTimerId, 'prompt detected'); + this.clearFallbackTimerId = null; this.logAction('step', '/clear completed'); this.emit('stepCompleted', 'clear'); @@ -1699,11 +1706,11 @@ export class RespawnController extends EventEmitter { this.logAction('step', 'Monitoring if /init triggered work...'); // Give Claude a moment to start working before checking for idle - this.stepTimer = this.startTrackedTimer( + this.stepTimerId = this.startTrackedTimer( 'init-monitor', 3000, () => { - this.stepTimer = null; + this.stepTimerId = null; // If still in monitoring state and no work detected, consider it idle if (this._state === 'monitoring_init' && !this.workingDetected) { this.checkMonitoringInitIdle(); @@ -1719,9 +1726,9 @@ export class RespawnController extends EventEmitter { * @fires stepCompleted - With step 'init' */ private checkMonitoringInitIdle(): void { - if (this.stepTimer) { - clearTimeout(this.stepTimer); - this.stepTimer = null; + if (this.stepTimerId) { + this.cleanup.unregister(this.stepTimerId); + this.stepTimerId = null; } this.log('/init did not trigger work, sending kickstart prompt'); this.emit('stepCompleted', 'init'); @@ -1737,11 +1744,11 @@ export class RespawnController extends EventEmitter { this.terminalBuffer.clear(); this.clearWorkingPatternWindow(); - this.stepTimer = this.startTrackedTimer( + this.stepTimerId = this.startTrackedTimer( 'step-delay', this.config.interStepDelayMs, async () => { - this.stepTimer = null; + this.stepTimerId = null; if (this._state === 'stopped') return; const prompt = this.config.kickstartPrompt!; this.logAction('command', `Sending kickstart: "${prompt.substring(0, 40)}..."`); @@ -1773,46 +1780,20 @@ export class RespawnController extends EventEmitter { private clearTimers(): void { // Clear tracked timers map first to avoid stale entries during individual cleanup this.activeTimers.clear(); - if (this.stepTimer) { - clearTimeout(this.stepTimer); - this.stepTimer = null; - } - if (this.clearFallbackTimer) { - clearTimeout(this.clearFallbackTimer); - this.clearFallbackTimer = null; - } - if (this.completionConfirmTimer) { - clearTimeout(this.completionConfirmTimer); - this.completionConfirmTimer = null; - } - if (this.stepConfirmTimer) { - clearTimeout(this.stepConfirmTimer); - this.stepConfirmTimer = null; - } - if (this.autoAcceptTimer) { - clearTimeout(this.autoAcceptTimer); - this.autoAcceptTimer = null; - } - if (this.preFilterTimer) { - clearTimeout(this.preFilterTimer); - this.preFilterTimer = null; - } - if (this.noOutputTimer) { - clearTimeout(this.noOutputTimer); - this.noOutputTimer = null; - } - if (this.hookConfirmTimer) { - clearTimeout(this.hookConfirmTimer); - this.hookConfirmTimer = null; - } - if (this.stuckStateTimer) { - clearInterval(this.stuckStateTimer); - this.stuckStateTimer = null; - } - if (this.detectionUpdateTimer) { - clearInterval(this.detectionUpdateTimer); - this.detectionUpdateTimer = null; - } + this.cleanup.dispose(); + // Reinitialize for reuse (controller can be stopped and restarted) + this.cleanup = new CleanupManager(); + // Null out IDs + this.stepTimerId = null; + this.completionConfirmTimerId = null; + this.noOutputTimerId = null; + this.detectionUpdateTimerId = null; + this.autoAcceptTimerId = null; + this.preFilterTimerId = null; + this.hookConfirmTimerId = null; + this.clearFallbackTimerId = null; + this.stepConfirmTimerId = null; + this.stuckStateTimerId = null; } // ========== Stuck-State Detection Methods ========== @@ -1826,21 +1807,25 @@ export class RespawnController extends EventEmitter { if (this._state === 'stopped') return; // Clear existing timer - if (this.stuckStateTimer) { - clearInterval(this.stuckStateTimer); - this.stuckStateTimer = null; + if (this.stuckStateTimerId) { + this.cleanup.unregister(this.stuckStateTimerId); + this.stuckStateTimerId = null; } // Check interval for stuck state const checkIntervalMs = Math.min(this.config.stuckStateWarningMs, 60000); // Check every minute max - this.stuckStateTimer = setInterval(() => { - try { - this.checkStuckState(); - } catch (err) { - console.error(`[RespawnController] Error in stuckStateTimer:`, err); - } - }, checkIntervalMs); + this.stuckStateTimerId = this.cleanup.setInterval( + () => { + try { + this.checkStuckState(); + } catch (err) { + console.error(`[RespawnController] Error in stuckStateTimer:`, err); + } + }, + checkIntervalMs, + { description: 'stuck-state detection' } + ); } /** @@ -1981,7 +1966,7 @@ export class RespawnController extends EventEmitter { * Start a tracked timer with UI countdown support. * Emits timerStarted event and tracks the timer for UI display. */ - private startTrackedTimer(name: string, durationMs: number, callback: () => void, reason?: string): NodeJS.Timeout { + private startTrackedTimer(name: string, durationMs: number, callback: () => void, reason?: string): string { const now = Date.now(); const endsAt = now + durationMs; @@ -1989,19 +1974,23 @@ export class RespawnController extends EventEmitter { this.emit('timerStarted', { name, durationMs, endsAt, reason }); this.logAction('timer', `Started ${name}: ${Math.round(durationMs / 1000)}s${reason ? ` (${reason})` : ''}`); - return setTimeout(() => { - this.activeTimers.delete(name); - this.emit('timerCompleted', name); - callback(); - }, durationMs); + return this.cleanup.setTimeout( + () => { + this.activeTimers.delete(name); + this.emit('timerCompleted', name); + callback(); + }, + durationMs, + { description: name } + ); } /** * Cancel a tracked timer and emit cancellation event. */ - private cancelTrackedTimer(name: string, timerRef: NodeJS.Timeout | null, reason?: string): void { - if (timerRef) { - clearTimeout(timerRef); + private cancelTrackedTimer(name: string, timerId: string | null, reason?: string): void { + if (timerId) { + this.cleanup.unregister(timerId); if (this.activeTimers.has(name)) { this.activeTimers.delete(name); this.emit('timerCancelled', name, reason); @@ -2097,14 +2086,14 @@ export class RespawnController extends EventEmitter { * (used when AI check is disabled or has too many errors). */ private startNoOutputTimer(): void { - this.cancelTrackedTimer('no-output-fallback', this.noOutputTimer, 'restarting'); - this.noOutputTimer = null; + this.cancelTrackedTimer('no-output-fallback', this.noOutputTimerId, 'restarting'); + this.noOutputTimerId = null; - this.noOutputTimer = this.startTrackedTimer( + this.noOutputTimerId = this.startTrackedTimer( 'no-output-fallback', this.config.noOutputTimeoutMs, () => { - this.noOutputTimer = null; + this.noOutputTimerId = null; if (this._state === 'watching' || this._state === 'confirming_idle') { const msSinceOutput = Date.now() - this.lastOutputTime; this.logAction('detection', `No-output fallback: ${Math.round(msSinceOutput / 1000)}s silence`); @@ -2137,17 +2126,17 @@ export class RespawnController extends EventEmitter { * This provides an additional path to AI check even without a completion message. */ private startPreFilterTimer(): void { - this.cancelTrackedTimer('pre-filter', this.preFilterTimer, 'restarting'); - this.preFilterTimer = null; + this.cancelTrackedTimer('pre-filter', this.preFilterTimerId, 'restarting'); + this.preFilterTimerId = null; // Only set up pre-filter when AI check is enabled if (!this.config.aiIdleCheckEnabled) return; - this.preFilterTimer = this.startTrackedTimer( + this.preFilterTimerId = this.startTrackedTimer( 'pre-filter', this.config.completionConfirmMs, () => { - this.preFilterTimer = null; + this.preFilterTimerId = null; if (this._state === 'watching') { const now = Date.now(); const msSinceOutput = now - this.lastOutputTime; @@ -2252,18 +2241,18 @@ export class RespawnController extends EventEmitter { if (result.verdict === 'IDLE') { // Cancel any pending confirmation timers - AI has spoken - this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimer, 'AI verdict: IDLE'); - this.completionConfirmTimer = null; - this.cancelTrackedTimer('pre-filter', this.preFilterTimer, 'AI verdict: IDLE'); - this.preFilterTimer = null; + this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimerId, 'AI verdict: IDLE'); + this.completionConfirmTimerId = null; + this.cancelTrackedTimer('pre-filter', this.preFilterTimerId, 'AI verdict: IDLE'); + this.preFilterTimerId = null; this.logAction('ai-check', `Verdict: IDLE - ${result.reasoning}`); this.emit('aiCheckCompleted', result); this.onIdleConfirmed(`ai-check: idle (${result.reasoning})`); } else if (result.verdict === 'WORKING') { // Cancel timers and go to cooldown - this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimer, 'AI verdict: WORKING'); - this.completionConfirmTimer = null; + this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimerId, 'AI verdict: WORKING'); + this.completionConfirmTimerId = null; this.logAction('ai-check', `Verdict: WORKING - ${result.reasoning}`); this.emit('aiCheckCompleted', result); @@ -2319,14 +2308,14 @@ export class RespawnController extends EventEmitter { * and no elicitation dialog was detected. Only handles plan mode approvals. */ private startAutoAcceptTimer(): void { - this.cancelTrackedTimer('auto-accept', this.autoAcceptTimer, 'restarting'); - this.autoAcceptTimer = null; + this.cancelTrackedTimer('auto-accept', this.autoAcceptTimerId, 'restarting'); + this.autoAcceptTimerId = null; - this.autoAcceptTimer = this.startTrackedTimer( + this.autoAcceptTimerId = this.startTrackedTimer( 'auto-accept', this.config.autoAcceptDelayMs, () => { - this.autoAcceptTimer = null; + this.autoAcceptTimerId = null; this.tryAutoAccept(); }, 'plan mode detection' @@ -2338,8 +2327,8 @@ export class RespawnController extends EventEmitter { * Called when a completion message is detected (normal idle flow handles it). */ private cancelAutoAcceptTimer(): void { - this.cancelTrackedTimer('auto-accept', this.autoAcceptTimer, 'cancelled'); - this.autoAcceptTimer = null; + this.cancelTrackedTimer('auto-accept', this.autoAcceptTimerId, 'cancelled'); + this.autoAcceptTimerId = null; } /** @@ -2500,8 +2489,8 @@ export class RespawnController extends EventEmitter { } // Cancel completion confirmation - auto-accept takes precedence - this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimer, 'auto-accept'); - this.completionConfirmTimer = null; + this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimerId, 'auto-accept'); + this.completionConfirmTimerId = null; this.completionMessageTime = null; // Ensure we're in watching state (not confirming_idle or ai_checking) @@ -2555,12 +2544,12 @@ export class RespawnController extends EventEmitter { } // Cancel completion confirm timer - hook takes precedence - this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimer, 'Stop hook received'); - this.completionConfirmTimer = null; + this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimerId, 'Stop hook received'); + this.completionConfirmTimerId = null; // Cancel pre-filter timer - hook takes precedence - this.cancelTrackedTimer('pre-filter', this.preFilterTimer, 'Stop hook received'); - this.preFilterTimer = null; + this.cancelTrackedTimer('pre-filter', this.preFilterTimerId, 'Stop hook received'); + this.preFilterTimerId = null; // Start short confirmation timer to handle race conditions // (e.g., Stop hook arrives but Claude immediately starts new work) @@ -2594,12 +2583,12 @@ export class RespawnController extends EventEmitter { } // Cancel all other detection timers - this is definitive - this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimer, 'idle_prompt received'); - this.completionConfirmTimer = null; - this.cancelTrackedTimer('pre-filter', this.preFilterTimer, 'idle_prompt received'); - this.preFilterTimer = null; - this.cancelTrackedTimer('no-output-fallback', this.noOutputTimer, 'idle_prompt received'); - this.noOutputTimer = null; + this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimerId, 'idle_prompt received'); + this.completionConfirmTimerId = null; + this.cancelTrackedTimer('pre-filter', this.preFilterTimerId, 'idle_prompt received'); + this.preFilterTimerId = null; + this.cancelTrackedTimer('no-output-fallback', this.noOutputTimerId, 'idle_prompt received'); + this.noOutputTimerId = null; // idle_prompt is an even stronger signal than Stop hook (60s+ idle) // Skip confirmation and go directly to idle @@ -2613,14 +2602,14 @@ export class RespawnController extends EventEmitter { * @param hookType - Which hook triggered this ('stop' or 'idle_prompt') */ private startHookConfirmTimer(hookType: 'stop' | 'idle_prompt'): void { - this.cancelTrackedTimer('hook-confirm', this.hookConfirmTimer, 'restarting'); - this.hookConfirmTimer = null; + this.cancelTrackedTimer('hook-confirm', this.hookConfirmTimerId, 'restarting'); + this.hookConfirmTimerId = null; - this.hookConfirmTimer = this.startTrackedTimer( + this.hookConfirmTimerId = this.startTrackedTimer( 'hook-confirm', RespawnController.HOOK_CONFIRM_DELAY_MS, () => { - this.hookConfirmTimer = null; + this.hookConfirmTimerId = null; // Verify we haven't received new output since the hook arrived const hookTime = hookType === 'stop' ? this.stopHookTime : this.idlePromptTime; @@ -2694,17 +2683,17 @@ export class RespawnController extends EventEmitter { * After completion message, waits for output silence then triggers AI check. */ private startCompletionConfirmTimer(): void { - this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimer, 'restarting'); - this.completionConfirmTimer = null; + this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimerId, 'restarting'); + this.completionConfirmTimerId = null; this.setState('confirming_idle'); this.logAction('detection', 'Completion message found in output'); - this.completionConfirmTimer = this.startTrackedTimer( + this.completionConfirmTimerId = this.startTrackedTimer( 'completion-confirm', this.config.completionConfirmMs, () => { - this.completionConfirmTimer = null; + this.completionConfirmTimerId = null; if (this._state === 'stopped') return; const msSinceOutput = Date.now() - this.lastOutputTime; if (msSinceOutput >= this.config.completionConfirmMs) { @@ -2725,8 +2714,8 @@ export class RespawnController extends EventEmitter { * Cancel completion confirmation if new activity detected. */ private cancelCompletionConfirm(): void { - this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimer, 'activity detected'); - this.completionConfirmTimer = null; + this.cancelTrackedTimer('completion-confirm', this.completionConfirmTimerId, 'activity detected'); + this.completionConfirmTimerId = null; if (this._state === 'confirming_idle') { this.setState('watching'); this.completionMessageTime = null; @@ -2739,14 +2728,14 @@ export class RespawnController extends EventEmitter { * This ensures Claude has finished processing before we send the next command. */ private startStepConfirmTimer(step: 'update' | 'init' | 'kickstart'): void { - this.cancelTrackedTimer('step-confirm', this.stepConfirmTimer, 'restarting'); - this.stepConfirmTimer = null; + this.cancelTrackedTimer('step-confirm', this.stepConfirmTimerId, 'restarting'); + this.stepConfirmTimerId = null; - this.stepConfirmTimer = this.startTrackedTimer( + this.stepConfirmTimerId = this.startTrackedTimer( 'step-confirm', this.config.completionConfirmMs, () => { - this.stepConfirmTimer = null; + this.stepConfirmTimerId = null; if (this._state === 'stopped') return; const msSinceOutput = Date.now() - this.lastOutputTime; @@ -2779,8 +2768,8 @@ export class RespawnController extends EventEmitter { * Cancel step confirmation if working patterns detected. */ private cancelStepConfirm(): void { - this.cancelTrackedTimer('step-confirm', this.stepConfirmTimer, 'working detected'); - this.stepConfirmTimer = null; + this.cancelTrackedTimer('step-confirm', this.stepConfirmTimerId, 'working detected'); + this.stepConfirmTimerId = null; } /** @@ -2942,11 +2931,11 @@ export class RespawnController extends EventEmitter { this.terminalBuffer.clear(); // Clear buffer for fresh detection this.clearWorkingPatternWindow(); // Clear rolling window - this.stepTimer = this.startTrackedTimer( + this.stepTimerId = this.startTrackedTimer( 'step-delay', this.config.interStepDelayMs, async () => { - this.stepTimer = null; + this.stepTimerId = null; if (this._state === 'stopped') return; // Use RALPH_STATUS RECOMMENDATION if available, otherwise fall back to config @@ -2983,11 +2972,11 @@ export class RespawnController extends EventEmitter { this.terminalBuffer.clear(); this.clearWorkingPatternWindow(); - this.stepTimer = this.startTrackedTimer( + this.stepTimerId = this.startTrackedTimer( 'step-delay', this.config.interStepDelayMs, async () => { - this.stepTimer = null; + this.stepTimerId = null; if (this._state === 'stopped') return; this.logAction('command', 'Sending: /clear'); await this.session.writeViaMux('/clear\r'); // \r triggers Enter in Ink/Claude CLI @@ -2996,11 +2985,11 @@ export class RespawnController extends EventEmitter { this.promptDetected = false; // Start fallback timer - if no prompt detected after 10s, proceed to /init anyway - this.clearFallbackTimer = this.startTrackedTimer( + this.clearFallbackTimerId = this.startTrackedTimer( 'clear-fallback', RespawnController.CLEAR_FALLBACK_TIMEOUT_MS, () => { - this.clearFallbackTimer = null; + this.clearFallbackTimerId = null; if (this._state === 'waiting_clear') { this.logAction('step', '/clear fallback: proceeding to /init'); this.emit('stepCompleted', 'clear'); @@ -3027,11 +3016,11 @@ export class RespawnController extends EventEmitter { this.terminalBuffer.clear(); this.clearWorkingPatternWindow(); - this.stepTimer = this.startTrackedTimer( + this.stepTimerId = this.startTrackedTimer( 'step-delay', this.config.interStepDelayMs, async () => { - this.stepTimer = null; + this.stepTimerId = null; if (this._state === 'stopped') return; this.logAction('command', 'Sending: /init'); await this.session.writeViaMux('/init\r'); // \r triggers Enter in Ink/Claude CLI diff --git a/src/state-store.ts b/src/state-store.ts index e69e0adb..8a935e92 100644 --- a/src/state-store.ts +++ b/src/state-store.ts @@ -28,7 +28,7 @@ import { TokenStats, TokenUsageEntry, } from './types.js'; -import { MAX_SESSION_TOKENS } from './utils/index.js'; +import { Debouncer, MAX_SESSION_TOKENS } from './utils/index.js'; /** Debounce delay for batching state writes (ms) */ const SAVE_DEBOUNCE_MS = 500; @@ -60,7 +60,7 @@ const MAX_CONSECUTIVE_FAILURES = 3; export class StateStore { private state: AppState; private filePath: string; - private saveTimeout: NodeJS.Timeout | null = null; + private saveDeb = new Debouncer(SAVE_DEBOUNCE_MS); private dirty: boolean = false; private dirtySessions = new Set(); private cachedSessionJsons = new Map(); @@ -68,7 +68,7 @@ export class StateStore { // Inner state storage (separate from main state to reduce write frequency) private ralphStates: Map = new Map(); private ralphStatePath: string; - private ralphStateSaveTimeout: NodeJS.Timeout | null = null; + private ralphStateSaveDeb = new Debouncer(SAVE_DEBOUNCE_MS); private ralphStateDirty: boolean = false; // Circuit breaker for save failures (prevents hammering disk on persistent errors) @@ -150,14 +150,12 @@ export class StateStore { */ save(): void { this.dirty = true; - if (this.saveTimeout) { - return; // Already scheduled - } - this.saveTimeout = setTimeout(() => { + if (this.saveDeb.isPending) return; // Already scheduled + this.saveDeb.schedule(() => { this.saveNowAsync().catch((err) => { console.error('[StateStore] Async save failed:', err); }); - }, SAVE_DEBOUNCE_MS); + }); } /** @@ -242,10 +240,7 @@ export class StateStore { } private async _doSaveAsync(): Promise { - if (this.saveTimeout) { - clearTimeout(this.saveTimeout); - this.saveTimeout = null; - } + this.saveDeb.cancel(); if (!this.dirty) { return; } @@ -332,10 +327,7 @@ export class StateStore { * Prefer saveNowAsync() for normal operation. */ saveNow(): void { - if (this.saveTimeout) { - clearTimeout(this.saveTimeout); - this.saveTimeout = null; - } + this.saveDeb.cancel(); if (!this.dirty) { return; } @@ -775,12 +767,10 @@ export class StateStore { // Debounced save for inner states private saveRalphStates(): void { this.ralphStateDirty = true; - if (this.ralphStateSaveTimeout) { - return; // Already scheduled - } - this.ralphStateSaveTimeout = setTimeout(() => { + if (this.ralphStateSaveDeb.isPending) return; // Already scheduled + this.ralphStateSaveDeb.schedule(() => { this.saveRalphStatesNow(); - }, SAVE_DEBOUNCE_MS); + }); } /** @@ -788,10 +778,7 @@ export class StateStore { * Writes to temp file first, then renames to prevent corruption on crash. */ private saveRalphStatesNow(): void { - if (this.ralphStateSaveTimeout) { - clearTimeout(this.ralphStateSaveTimeout); - this.ralphStateSaveTimeout = null; - } + this.ralphStateSaveDeb.cancel(); if (!this.ralphStateDirty) { return; } diff --git a/src/subagent-watcher.ts b/src/subagent-watcher.ts index dbc73821..f0d1eb94 100644 --- a/src/subagent-watcher.ts +++ b/src/subagent-watcher.ts @@ -14,6 +14,7 @@ import { join, basename } from 'node:path'; import { execFile } from 'node:child_process'; import { readFile, readdir, stat as statAsync } from 'node:fs/promises'; import { PENDING_TOOL_CALL_TTL_MS, MAX_PENDING_TOOL_CALLS } from './config/map-limits.js'; +import { CleanupManager, KeyedDebouncer } from './utils/index.js'; // ========== Types ========== @@ -161,12 +162,11 @@ const FILE_CONTENT_DEBOUNCE_MS = 100; // Debounce delay for file content updates export class SubagentWatcher extends EventEmitter { private filePositions = new Map(); private dirWatchers = new Map(); - // Per-file debounce timers for directory watcher (replaces per-file FSWatchers) - private fileDebouncers = new Map(); + // Per-file debouncer for directory watcher (replaces per-file FSWatchers) + private fileDeb = new KeyedDebouncer(FILE_CONTENT_DEBOUNCE_MS); private agentInfo = new Map(); - private idleTimers = new Map(); - private pollInterval: NodeJS.Timeout | null = null; - private livenessInterval: NodeJS.Timeout | null = null; + private idleDeb = new KeyedDebouncer(IDLE_TIMEOUT_MS); + private cleanup = new CleanupManager(); private _isRunning = false; private knownSubagentDirs = new Set(); // Map of agentId -> Map of toolUseId -> { toolName, timestamp } (for linking tool_result to tool_call) @@ -228,12 +228,16 @@ export class SubagentWatcher extends EventEmitter { // Periodic scan for new subagent directories // Full directory traversal only every FULL_SCAN_EVERY_N_POLLS polls (~5s) // FSWatchers handle known directories between full scans - this.pollInterval = setInterval(() => { - this._pollCount++; - if (this._pollCount % FULL_SCAN_EVERY_N_POLLS === 0) { - this.scanForSubagents().catch((err) => this.emit('subagent:error', err as Error)); - } - }, POLL_INTERVAL_MS); + this.cleanup.setInterval( + () => { + this._pollCount++; + if (this._pollCount % FULL_SCAN_EVERY_N_POLLS === 0) { + this.scanForSubagents().catch((err) => this.emit('subagent:error', err as Error)); + } + }, + POLL_INTERVAL_MS, + { description: 'subagent directory poll' } + ); // Periodic liveness check for active subagents this.startLivenessChecker(); @@ -249,54 +253,56 @@ export class SubagentWatcher extends EventEmitter { * 3. Full pgrep scan (expensive, ~500ms) — only for agents that fail tiers 1+2 */ private startLivenessChecker(): void { - if (this.livenessInterval) return; + this.cleanup.setInterval( + async () => { + // Guard: prevent concurrent liveness checks (avoids duplicate completed events) + if (this._isCheckingLiveness) return; + this._isCheckingLiveness = true; - this.livenessInterval = setInterval(async () => { - // Guard: prevent concurrent liveness checks (avoids duplicate completed events) - if (this._isCheckingLiveness) return; - this._isCheckingLiveness = true; + try { + // Collect agents that need the expensive pgrep scan + const needsFullScan: SubagentInfo[] = []; - try { - // Collect agents that need the expensive pgrep scan - const needsFullScan: SubagentInfo[] = []; - - for (const [_agentId, info] of this.agentInfo) { - if (info.status !== 'active' && info.status !== 'idle') continue; - - // Tier 1: File mtime check (~0.3ms per agent) - if (await this.checkSubagentFileAlive(info)) continue; - - // Tier 2: Cached PID check (~0.1ms per agent) - if (info.pid && (await this.checkPidAlive(info.pid))) continue; - - // Tiers 1+2 failed — need expensive scan for this agent - needsFullScan.push(info); - } - - // Tier 3: Full pgrep scan — only if any agents failed cheap checks - if (needsFullScan.length > 0) { - const pidMap = await this.getClaudePids(); - - for (const info of needsFullScan) { - // Re-check status in case another check completed this agent + for (const [_agentId, info] of this.agentInfo) { if (info.status !== 'active' && info.status !== 'idle') continue; - const alive = this.checkSubagentAliveFromPidMap(info, pidMap); - if (!alive) { - info.pid = undefined; - info.status = 'completed'; - this.pendingToolCalls.delete(info.agentId); - this.emit('subagent:completed', info); + // Tier 1: File mtime check (~0.3ms per agent) + if (await this.checkSubagentFileAlive(info)) continue; + + // Tier 2: Cached PID check (~0.1ms per agent) + if (info.pid && (await this.checkPidAlive(info.pid))) continue; + + // Tiers 1+2 failed — need expensive scan for this agent + needsFullScan.push(info); + } + + // Tier 3: Full pgrep scan — only if any agents failed cheap checks + if (needsFullScan.length > 0) { + const pidMap = await this.getClaudePids(); + + for (const info of needsFullScan) { + // Re-check status in case another check completed this agent + if (info.status !== 'active' && info.status !== 'idle') continue; + + const alive = this.checkSubagentAliveFromPidMap(info, pidMap); + if (!alive) { + info.pid = undefined; + info.status = 'completed'; + this.pendingToolCalls.delete(info.agentId); + this.emit('subagent:completed', info); + } } } - } - // Periodically clean up stale completed agents (older than 24 hours) - this.cleanupStaleAgents(); - } finally { - this._isCheckingLiveness = false; - } - }, LIVENESS_CHECK_MS); + // Periodically clean up stale completed agents (older than 24 hours) + this.cleanupStaleAgents(); + } finally { + this._isCheckingLiveness = false; + } + }, + LIVENESS_CHECK_MS, + { description: 'subagent liveness check' } + ); } /** @@ -418,21 +424,12 @@ export class SubagentWatcher extends EventEmitter { stop(): void { this._isRunning = false; - if (this.pollInterval) { - clearInterval(this.pollInterval); - this.pollInterval = null; - } - - if (this.livenessInterval) { - clearInterval(this.livenessInterval); - this.livenessInterval = null; - } + // Dispose poll and liveness intervals, then re-create for potential restart + this.cleanup.dispose(); + this.cleanup = new CleanupManager(); // Clear file debouncers - for (const timer of this.fileDebouncers.values()) { - clearTimeout(timer); - } - this.fileDebouncers.clear(); + this.fileDeb.dispose(); this.fileAgentContext.clear(); // Remove error handlers before closing watchers to prevent memory leak @@ -447,10 +444,7 @@ export class SubagentWatcher extends EventEmitter { } this.dirWatchers.clear(); - for (const timer of this.idleTimers.values()) { - clearTimeout(timer); - } - this.idleTimers.clear(); + this.idleDeb.dispose(); // Clear all state for clean restart this.filePositions.clear(); @@ -532,16 +526,8 @@ export class SubagentWatcher extends EventEmitter { this.pendingToolCalls.delete(agentId); this.filePositions.delete(info.filePath); this.fileAgentContext.delete(info.filePath); - const debounceTimer = this.fileDebouncers.get(info.filePath); - if (debounceTimer) { - clearTimeout(debounceTimer); - this.fileDebouncers.delete(info.filePath); - } - const timer = this.idleTimers.get(agentId); - if (timer) { - clearTimeout(timer); - this.idleTimers.delete(agentId); - } + this.fileDeb.cancelKey(info.filePath); + this.idleDeb.cancelKey(agentId); } } @@ -634,9 +620,9 @@ export class SubagentWatcher extends EventEmitter { return { agentCount: this.agentInfo.size, - fileDebouncerCount: this.fileDebouncers.size, + fileDebouncerCount: this.fileDeb.size, dirWatcherCount: this.dirWatchers.size, - idleTimerCount: this.idleTimers.size, + idleTimerCount: this.idleDeb.size, pendingToolCallsCount, knownDirsCount: this.knownSubagentDirs.size, filePositionsCount: this.filePositions.size, @@ -1129,13 +1115,8 @@ export class SubagentWatcher extends EventEmitter { if (!filename?.endsWith('.jsonl')) return; const filePath = join(dir, filename); - // Clear existing debounce for this file - const existing = this.fileDebouncers.get(filePath); - if (existing) clearTimeout(existing); - // Debounce 100ms to batch rapid writes - const timer = setTimeout(() => { - this.fileDebouncers.delete(filePath); + this.fileDeb.schedule(filePath, () => { if (!existsSync(filePath)) return; if (this.fileAgentContext.has(filePath)) { @@ -1145,9 +1126,7 @@ export class SubagentWatcher extends EventEmitter { // New file — register it this.registerAgentFile(filePath, projectHash, sessionId).catch(() => {}); } - }, FILE_CONTENT_DEBOUNCE_MS); - - this.fileDebouncers.set(filePath, timer); + }); }); // Handle watcher errors to prevent unhandled exceptions @@ -1602,25 +1581,13 @@ export class SubagentWatcher extends EventEmitter { * Reset idle timer for an agent */ private resetIdleTimer(agentId: string): void { - const existing = this.idleTimers.get(agentId); - if (existing) { - clearTimeout(existing); - } - - const timer = setTimeout(() => { - // Guard against race condition: agent may have been deleted before timer fires + this.idleDeb.schedule(agentId, () => { const info = this.agentInfo.get(agentId); - if (!info) { - // Agent was deleted - clean up timer reference - this.idleTimers.delete(agentId); - return; - } + if (!info) return; if (info.status === 'active') { info.status = 'idle'; } - }, IDLE_TIMEOUT_MS); - - this.idleTimers.set(agentId, timer); + }); } /** diff --git a/src/tmux-manager.ts b/src/tmux-manager.ts index 44f01d3e..1551649a 100644 --- a/src/tmux-manager.ts +++ b/src/tmux-manager.ts @@ -59,8 +59,7 @@ import { resolveOpenCodeDir } from './utils/opencode-cli-resolver.js'; // Timing Constants // ============================================================================ -/** Timeout for exec commands (5 seconds) */ -const EXEC_TIMEOUT_MS = 5000; +import { EXEC_TIMEOUT_MS } from './config/exec-timeout.js'; /** Delay after tmux session creation — enough for detached tmux to be queryable */ const TMUX_CREATION_WAIT_MS = 100; diff --git a/src/utils/claude-cli-resolver.ts b/src/utils/claude-cli-resolver.ts index 82cbe5f2..575fd9ff 100644 --- a/src/utils/claude-cli-resolver.ts +++ b/src/utils/claude-cli-resolver.ts @@ -12,9 +12,7 @@ import { execSync } from 'node:child_process'; import { existsSync } from 'node:fs'; import { delimiter, dirname, join } from 'node:path'; import { homedir } from 'node:os'; - -/** Timeout for exec commands (5 seconds) */ -const EXEC_TIMEOUT_MS = 5000; +import { EXEC_TIMEOUT_MS } from '../config/exec-timeout.js'; /** Common directories where the Claude CLI binary may be installed */ const CLAUDE_SEARCH_DIRS = [ diff --git a/src/utils/debouncer.ts b/src/utils/debouncer.ts new file mode 100644 index 00000000..87b983b2 --- /dev/null +++ b/src/utils/debouncer.ts @@ -0,0 +1,174 @@ +/** + * @fileoverview Debounce utilities to replace manual timer management. + * + * Two variants: + * - `Debouncer` — single debounced operation (replaces timer + clearTimeout pattern) + * - `KeyedDebouncer` — per-key debouncing (replaces Map pattern) + * + * Both integrate with CleanupManager via dispose(). + * + * @module utils/debouncer + */ + +/** + * Single-operation debouncer. + * + * Replaces the common pattern of: + * ``` + * private timer: NodeJS.Timeout | null = null; + * debounce(fn) { if (this.timer) clearTimeout(this.timer); this.timer = setTimeout(fn, delay); } + * cancel() { if (this.timer) { clearTimeout(this.timer); this.timer = null; } } + * ``` + * + * @example + * ```typescript + * private saveDeb = new Debouncer(500); + * + * onChange() { + * this.saveDeb.schedule(() => this.save()); + * } + * + * stop() { + * this.saveDeb.dispose(); + * } + * ``` + */ +export class Debouncer { + private timer: NodeJS.Timeout | null = null; + + constructor(private readonly delayMs: number) {} + + /** + * Schedule a debounced callback. Resets the timer on each call. + * If a previous call is pending, it is cancelled. + */ + schedule(fn: () => void): void { + this.cancel(); + this.timer = setTimeout(() => { + this.timer = null; + fn(); + }, this.delayMs); + } + + /** Cancel any pending execution without invoking the callback. */ + cancel(): void { + if (this.timer) { + clearTimeout(this.timer); + this.timer = null; + } + } + + /** Whether a callback is currently pending. */ + get isPending(): boolean { + return this.timer !== null; + } + + /** + * Cancel pending callback and flush immediately. + * Useful for shutdown: cancel the timer but run the action now. + * + * @param fn - The flush function to run (typically the same function passed to schedule) + */ + flush(fn: () => void): void { + this.cancel(); + fn(); + } + + /** Alias for cancel() — matches CleanupManager/Disposable convention. */ + dispose(): void { + this.cancel(); + } +} + +/** + * Per-key debouncer for operations that need independent timers per resource. + * + * Replaces the common pattern of: + * ``` + * private timers = new Map(); + * debounce(key, fn) { + * const existing = this.timers.get(key); + * if (existing) clearTimeout(existing); + * this.timers.set(key, setTimeout(() => { this.timers.delete(key); fn(); }, delay)); + * } + * ``` + * + * @example + * ```typescript + * private fileDebouncers = new KeyedDebouncer(100); + * + * onFileChange(path: string) { + * this.fileDebouncers.schedule(path, () => this.processFile(path)); + * } + * + * stop() { + * this.fileDebouncers.dispose(); + * } + * ``` + */ +export class KeyedDebouncer { + private timers = new Map(); + + constructor(private readonly delayMs: number) {} + + /** + * Schedule a debounced callback for a specific key. + * Each key has its own independent timer. + */ + schedule(key: string, fn: () => void): void { + this.cancelKey(key); + this.timers.set( + key, + setTimeout(() => { + this.timers.delete(key); + fn(); + }, this.delayMs) + ); + } + + /** Cancel a pending callback for a specific key. */ + cancelKey(key: string): void { + const existing = this.timers.get(key); + if (existing) { + clearTimeout(existing); + this.timers.delete(key); + } + } + + /** Whether a callback is pending for a specific key. */ + has(key: string): boolean { + return this.timers.has(key); + } + + /** Number of active timers. */ + get size(): number { + return this.timers.size; + } + + /** Get all currently active keys. */ + keys(): IterableIterator { + return this.timers.keys(); + } + + /** Cancel all pending callbacks. */ + dispose(): void { + for (const timer of this.timers.values()) { + clearTimeout(timer); + } + this.timers.clear(); + } + + /** + * Cancel all pending callbacks and run a flush function for each active key. + * Useful for shutdown: cancel timers but run the action for each pending key. + * + * @param fn - Called once per active key with the key as argument + */ + flushAll(fn: (key: string) => void): void { + const activeKeys = Array.from(this.timers.keys()); + this.dispose(); + for (const key of activeKeys) { + fn(key); + } + } +} diff --git a/src/utils/index.ts b/src/utils/index.ts index e75d8940..ebe137b8 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -9,14 +9,19 @@ export { BufferAccumulator } from './buffer-accumulator.js'; export { LRUMap, type LRUMapOptions } from './lru-map.js'; export { CleanupManager, type TimerOptions } from './cleanup-manager.js'; +export { Debouncer, KeyedDebouncer } from './debouncer.js'; export { StaleExpirationMap, type StaleExpirationMapOptions } from './stale-expiration-map.js'; export { ANSI_ESCAPE_PATTERN_FULL, ANSI_ESCAPE_PATTERN_SIMPLE, TOKEN_PATTERN, SPINNER_PATTERN, + createAnsiPatternFull, + createAnsiPatternSimple, + stripAnsi, + SAFE_PATH_PATTERN, } from './regex-patterns.js'; -export { MAX_SESSION_TOKENS } from './token-validation.js'; +export { MAX_SESSION_TOKENS, validateTokenCounts, validateTokensAndCost } from './token-validation.js'; export { stringSimilarity, fuzzyPhraseMatch, todoContentHash } from './string-similarity.js'; export { assertNever } from './type-safety.js'; export { wrapWithNice } from './nice-wrapper.js'; diff --git a/src/utils/opencode-cli-resolver.ts b/src/utils/opencode-cli-resolver.ts index 57124306..b144699e 100644 --- a/src/utils/opencode-cli-resolver.ts +++ b/src/utils/opencode-cli-resolver.ts @@ -11,9 +11,7 @@ import { execSync } from 'node:child_process'; import { existsSync } from 'node:fs'; import { dirname, join } from 'node:path'; import { homedir } from 'node:os'; - -/** Timeout for exec commands (5 seconds) */ -const EXEC_TIMEOUT_MS = 5000; +import { EXEC_TIMEOUT_MS } from '../config/exec-timeout.js'; /** Common directories where the OpenCode CLI binary may be installed */ const OPENCODE_SEARCH_DIRS = [ diff --git a/src/utils/string-similarity.ts b/src/utils/string-similarity.ts index 27ea785d..2cd62fa9 100644 --- a/src/utils/string-similarity.ts +++ b/src/utils/string-similarity.ts @@ -24,7 +24,7 @@ * levenshteinDistance('hello', 'helo') // 1 (one deletion) * levenshteinDistance('COMPLETE', 'COMPLET') // 1 (one deletion) */ -export function levenshteinDistance(a: string, b: string): number { +function levenshteinDistance(a: string, b: string): number { // Ensure a is the shorter string for space efficiency if (a.length > b.length) { [a, b] = [b, a]; @@ -91,22 +91,6 @@ export function stringSimilarity(a: string, b: string): number { return 1 - distance / maxLength; } -/** - * Check if two strings are similar within a given threshold. - * - * @param a - First string - * @param b - Second string - * @param threshold - Minimum similarity ratio (default: 0.85 = 85% similar) - * @returns True if similarity >= threshold - * - * @example - * isSimilar('COMPLETE', 'COMPLET', 0.85) // true (87.5% similar) - * isSimilar('COMPLETE', 'DONE', 0.85) // false (0% similar) - */ -export function isSimilar(a: string, b: string, threshold = 0.85): boolean { - return stringSimilarity(a, b) >= threshold; -} - /** * Check if two strings are similar with edit distance tolerance. * More intuitive for short strings than percentage-based threshold. @@ -120,7 +104,7 @@ export function isSimilar(a: string, b: string, threshold = 0.85): boolean { * isSimilarByDistance('COMPLETE', 'COMPLET', 2) // true (distance 1) * isSimilarByDistance('COMPLETE', 'COMP', 2) // false (distance 4) */ -export function isSimilarByDistance(a: string, b: string, maxDistance = 2): boolean { +function isSimilarByDistance(a: string, b: string, maxDistance = 2): boolean { return levenshteinDistance(a, b) <= maxDistance; } @@ -136,7 +120,7 @@ export function isSimilarByDistance(a: string, b: string, maxDistance = 2): bool * normalizePhrase('TASK-DONE') // 'TASKDONE' * normalizePhrase('Task Done') // 'TASKDONE' */ -export function normalizePhrase(phrase: string): string { +function normalizePhrase(phrase: string): string { return phrase .toUpperCase() .replace(/[\s_\-.]+/g, '') // Remove whitespace, underscores, hyphens, dots diff --git a/src/web/schemas.ts b/src/web/schemas.ts index 0b96f671..00edf28e 100644 --- a/src/web/schemas.ts +++ b/src/web/schemas.ts @@ -506,3 +506,42 @@ export const RalphLoopStartSchema = z.object({ ) .optional(), }); + +// ========== Inferred Types ========== + +export type CreateSessionInput = z.infer; +export type RunPromptInput = z.infer; +export type ResizeInput = z.infer; +export type CreateCaseInput = z.infer; +export type QuickStartInput = z.infer; +export type HookEventInput = z.infer; +export type RespawnConfigInput = z.infer; +export type ConfigUpdateInput = z.infer; +export type SettingsUpdateInput = z.infer; +export type SessionInputWithLimitInput = z.infer; +export type SessionNameInput = z.infer; +export type SessionColorInput = z.infer; +export type RalphConfigInput = z.infer; +export type FixPlanImportInput = z.infer; +export type RalphPromptWriteInput = z.infer; +export type AutoClearInput = z.infer; +export type AutoCompactInput = z.infer; +export type ImageWatcherInput = z.infer; +export type FlickerFilterInput = z.infer; +export type QuickRunInput = z.infer; +export type ScheduledRunInput = z.infer; +export type LinkCaseInput = z.infer; +export type GeneratePlanInput = z.infer; +export type GeneratePlanDetailedInput = z.infer; +export type CancelPlanInput = z.infer; +export type PlanTaskUpdateInput = z.infer; +export type PlanTaskAddInput = z.infer; +export type CpuLimitInput = z.infer; +export type ModelConfigUpdateInput = z.infer; +export type SubagentWindowStatesInput = z.infer; +export type SubagentParentMapInput = z.infer; +export type InteractiveRespawnInput = z.infer; +export type RespawnEnableInput = z.infer; +export type PushSubscribeInput = z.infer; +export type PushPreferencesUpdateInput = z.infer; +export type RalphLoopStartInput = z.infer; diff --git a/src/web/server.ts b/src/web/server.ts index 7f56005c..a96c3542 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -123,7 +123,7 @@ import { RalphLoopStartSchema, isValidWorkingDir, } from './schemas.js'; -import { StaleExpirationMap } from '../utils/index.js'; +import { CleanupManager, KeyedDebouncer, StaleExpirationMap } from '../utils/index.js'; import { MAX_CONCURRENT_SESSIONS, MAX_SSE_CLIENTS } from '../config/map-limits.js'; const __dirname = dirname(fileURLToPath(import.meta.url)); @@ -420,16 +420,14 @@ export class WebServer extends EventEmitter { ttlMs: 5 * 60 * 1000, // 5 minutes - auto-expire stale session timing data refreshOnGet: false, // Don't refresh on reads, only on explicit sets }); - // Scheduled runs cleanup timer - private scheduledCleanupTimer: NodeJS.Timeout | null = null; + // Centralized cleanup for standalone timers (intervals + resettable timeouts) + private cleanup = new CleanupManager(); // SSE event batching private taskUpdateBatches: Map = new Map(); - private taskUpdateBatchTimer: NodeJS.Timeout | null = null; + private taskUpdateBatchTimerId: string | null = null; // State update batching (reduce expensive toDetailedState() serialization) private stateUpdatePending: Set = new Set(); - private stateUpdateTimer: NodeJS.Timeout | null = null; - // SSE client health check timer - private sseHealthCheckTimer: NodeJS.Timeout | null = null; + private stateUpdateTimerId: string | null = null; // Flag to prevent new timers during shutdown private _isStopping: boolean = false; // Cached light state for SSE init (avoids rebuilding on every reconnect) @@ -439,14 +437,13 @@ export class WebServer extends EventEmitter { private cachedSessionsList: { data: unknown[]; timestamp: number } | null = null; // Token recording for daily stats (track what's been recorded to avoid double-counting) private lastRecordedTokens: Map = new Map(); - private tokenRecordingTimer: NodeJS.Timeout | null = null; // Server startup time for respawn grace period calculation private readonly serverStartTime: number = Date.now(); // Pending respawn start timers (for cleanup on shutdown) private pendingRespawnStarts: Map = new Map(); // Active plan orchestrators (for cancellation via API) private activePlanOrchestrators: Map = new Map(); - private persistDebounceTimers: Map> = new Map(); + private persistDeb = new KeyedDebouncer(100); // Grace period before starting restored respawn controllers (2 minutes) private static readonly RESPAWN_RESTORE_GRACE_PERIOD_MS = 2 * 60 * 1000; // Stored listener handlers for cleanup @@ -4646,18 +4643,12 @@ NOW: Generate the implementation plan for the task above. Think step by step.`; /** Debounced wrapper — coalesces rapid persistSessionState calls per session */ private persistSessionState(session: Session): void { - const existing = this.persistDebounceTimers.get(session.id); - if (existing) clearTimeout(existing); - this.persistDebounceTimers.set( - session.id, - setTimeout(() => { - this.persistDebounceTimers.delete(session.id); - // Session may have been removed during debounce - if (this.sessions.has(session.id)) { - this._persistSessionStateNow(session); - } - }, 100) - ); + this.persistDeb.schedule(session.id, () => { + // Session may have been removed during debounce + if (this.sessions.has(session.id)) { + this._persistSessionStateNow(session); + } + }); } /** Persists full session state including respawn config to state.json */ @@ -4845,11 +4836,7 @@ NOW: Generate the implementation plan for the task above. Think step by step.`; } // Clear pending persist-debounce timer (prevents stale closure holding session ref) - const pendingPersist = this.persistDebounceTimers.get(sessionId); - if (pendingPersist) { - clearTimeout(pendingPersist); - this.persistDebounceTimers.delete(sessionId); - } + this.persistDeb.cancelKey(sessionId); // Clear batches, per-session timers, and pending state updates this.terminalBatches.delete(sessionId); @@ -5079,11 +5066,7 @@ NOW: Generate the implementation plan for the task above. Think step by step.`; this.lastTerminalEventTime.delete(session.id); // Clear pending persist-debounce timer - const pendingPersist = this.persistDebounceTimers.get(session.id); - if (pendingPersist) { - clearTimeout(pendingPersist); - this.persistDebounceTimers.delete(session.id); - } + this.persistDeb.cancelKey(session.id); // Close any active file streams fileStreamManager.closeSessionStreams(session.id); @@ -6000,11 +5983,15 @@ NOW: Generate the implementation plan for the task above. Think step by step.`; const key = `${sessionId}:${task.id}`; this.taskUpdateBatches.set(key, { sessionId, task }); - if (!this.taskUpdateBatchTimer) { - this.taskUpdateBatchTimer = setTimeout(() => { - this.flushTaskUpdateBatches(); - this.taskUpdateBatchTimer = null; - }, TASK_UPDATE_BATCH_INTERVAL); + if (!this.taskUpdateBatchTimerId) { + this.taskUpdateBatchTimerId = this.cleanup.setTimeout( + () => { + this.taskUpdateBatchTimerId = null; + this.flushTaskUpdateBatches(); + }, + TASK_UPDATE_BATCH_INTERVAL, + { description: 'task update batch flush' } + ); } } @@ -6031,11 +6018,15 @@ NOW: Generate the implementation plan for the task above. Think step by step.`; this.stateUpdatePending.add(sessionId); - if (!this.stateUpdateTimer) { - this.stateUpdateTimer = setTimeout(() => { - this.flushStateUpdates(); - this.stateUpdateTimer = null; - }, STATE_UPDATE_DEBOUNCE_INTERVAL); + if (!this.stateUpdateTimerId) { + this.stateUpdateTimerId = this.cleanup.setTimeout( + () => { + this.stateUpdateTimerId = null; + this.flushStateUpdates(); + }, + STATE_UPDATE_DEBOUNCE_INTERVAL, + { description: 'state update debounce flush' } + ); } } @@ -6226,21 +6217,30 @@ NOW: Generate the implementation plan for the task above. Think step by step.`; process.env.CODEMAN_API_URL = `${protocol}://localhost:${this.port}`; // Start scheduled runs cleanup timer - this.scheduledCleanupTimer = setInterval(() => { - this.cleanupScheduledRuns(); - }, SCHEDULED_CLEANUP_INTERVAL); + this.cleanup.setInterval( + () => { + this.cleanupScheduledRuns(); + }, + SCHEDULED_CLEANUP_INTERVAL, + { description: 'scheduled runs cleanup' } + ); // Start SSE client health check timer (prevents memory leaks from dead connections) - this.sseHealthCheckTimer = setInterval(() => { - this.cleanupDeadSSEClients(); - }, SSE_HEALTH_CHECK_INTERVAL); + this.cleanup.setInterval( + () => { + this.cleanupDeadSSEClients(); + }, + SSE_HEALTH_CHECK_INTERVAL, + { description: 'SSE client health check' } + ); // Start token recording timer (every 5 minutes for long-running sessions) - this.tokenRecordingTimer = setInterval( + this.cleanup.setInterval( () => { this.recordPeriodicTokenUsage(); }, - 5 * 60 * 1000 + 5 * 60 * 1000, + { description: 'periodic token recording' } ); // Start subagent watcher for Claude Code background agent visibility (if enabled) @@ -6531,11 +6531,8 @@ NOW: Generate the implementation plan for the task above. Think step by step.`; // Set stopping flag to prevent new timer creation during shutdown this._isStopping = true; - // Clear SSE health check timer - if (this.sseHealthCheckTimer) { - clearInterval(this.sseHealthCheckTimer); - this.sseHealthCheckTimer = null; - } + // Dispose all managed timers (intervals + resettable timeouts) + this.cleanup.dispose(); // Gracefully close all SSE connections before clearing for (const client of this.sseClients) { @@ -6558,43 +6555,20 @@ NOW: Generate the implementation plan for the task above. Think step by step.`; this.terminalBatches.clear(); this.terminalBatchSizes.clear(); - if (this.taskUpdateBatchTimer) { - clearTimeout(this.taskUpdateBatchTimer); - this.taskUpdateBatchTimer = null; - } this.taskUpdateBatches.clear(); - - if (this.stateUpdateTimer) { - clearTimeout(this.stateUpdateTimer); - this.stateUpdateTimer = null; - } this.stateUpdatePending.clear(); - - // Clear token recording timer - if (this.tokenRecordingTimer) { - clearInterval(this.tokenRecordingTimer); - this.tokenRecordingTimer = null; - } this.lastRecordedTokens.clear(); - // Clear scheduled cleanup timer - if (this.scheduledCleanupTimer) { - clearInterval(this.scheduledCleanupTimer); - this.scheduledCleanupTimer = null; - } - // Stop multiplexer and flush pending saves this.mux.destroy(); // Flush any pending persist-debounce timers and persist dirty sessions - for (const [sessionId, timer] of this.persistDebounceTimers) { - clearTimeout(timer); + this.persistDeb.flushAll((sessionId) => { const session = this.sessions.get(sessionId); if (session) { this._persistSessionStateNow(session); } - } - this.persistDebounceTimers.clear(); + }); // Clear cached state this.cachedLightState = null; diff --git a/test/hooks-config.test.ts b/test/hooks-config.test.ts index f10b6dcf..4e544ad8 100644 --- a/test/hooks-config.test.ts +++ b/test/hooks-config.test.ts @@ -126,7 +126,10 @@ describe('writeHooksConfig', () => { writeHooksConfig(testDir); const settingsPath = join(testDir, '.claude', 'settings.local.json'); const content = readFileSync(settingsPath, 'utf-8'); - expect(() => JSON.parse(content)).not.toThrow(); + const parsed = JSON.parse(content); + expect(parsed).toHaveProperty('hooks'); + expect(parsed.hooks).toHaveProperty('Notification'); + expect(parsed.hooks).toHaveProperty('Stop'); }); it('should include hooks config in output', () => { diff --git a/test/image-watcher.test.ts b/test/image-watcher.test.ts index 907934ab..19832091 100644 --- a/test/image-watcher.test.ts +++ b/test/image-watcher.test.ts @@ -121,6 +121,7 @@ describe('ImageWatcher', () => { it('should be safe to call for non-watched session', () => { expect(() => watcher.unwatchSession('nonexistent')).not.toThrow(); + expect(watcher.getWatchedSessions()).toHaveLength(0); }); it('should clear pending debounce timers for the session', () => { diff --git a/test/session-manager.test.ts b/test/session-manager.test.ts index bb31cddf..c68e7764 100644 --- a/test/session-manager.test.ts +++ b/test/session-manager.test.ts @@ -214,6 +214,7 @@ describe('SessionManager', () => { it('should handle non-existent session gracefully', async () => { await expect(manager.stopSession('non-existent')).resolves.not.toThrow(); + expect(manager.getSessionCount()).toBe(0); }); it('should update stored session to stopped', async () => { diff --git a/test/task-queue.test.ts b/test/task-queue.test.ts index b9152128..40c49919 100644 --- a/test/task-queue.test.ts +++ b/test/task-queue.test.ts @@ -536,9 +536,11 @@ describe('circular dependency detection', () => { it('should allow dependencies on non-existent tasks (just unsatisfied, not a cycle)', () => { // Dependencies on non-existent tasks are valid - they just won't be satisfied - expect(() => { - queue.addTask({ prompt: 'Task D', dependencies: ['non-existent-id'] }); - }).not.toThrow(); + const task = queue.addTask({ prompt: 'Task D', dependencies: ['non-existent-id'] }); + expect(queue.getAllTasks()).toHaveLength(1); + expect(task.dependencies).toEqual(['non-existent-id']); + // Task is blocked by unsatisfied dependency, so next() should return null + expect(queue.next()).toBeNull(); }); it('should detect self-dependency when task references itself', () => { diff --git a/test/task-tracker.test.ts b/test/task-tracker.test.ts index dcc5db33..a856d0f9 100644 --- a/test/task-tracker.test.ts +++ b/test/task-tracker.test.ts @@ -564,14 +564,18 @@ describe('TaskTracker', () => { describe('Edge Cases', () => { it('should handle null message', () => { expect(() => tracker.processMessage(null)).not.toThrow(); + expect(tracker.getAllTasks().size).toBe(0); + expect(tracker.getRunningCount()).toBe(0); }); it('should handle message without content', () => { expect(() => tracker.processMessage({ message: {} })).not.toThrow(); + expect(tracker.getAllTasks().size).toBe(0); }); it('should handle empty content array', () => { expect(() => tracker.processMessage({ message: { content: [] } })).not.toThrow(); + expect(tracker.getAllTasks().size).toBe(0); }); it('should handle tool_result for unknown task', () => { @@ -587,11 +591,15 @@ describe('TaskTracker', () => { }, }); }).not.toThrow(); + expect(tracker.getTask('unknown-task')).toBeUndefined(); + expect(tracker.getAllTasks().size).toBe(0); }); it('should handle empty terminal output', () => { expect(() => tracker.processTerminalOutput('')).not.toThrow(); expect(() => tracker.processTerminalOutput(' ')).not.toThrow(); + expect(tracker.getAllTasks().size).toBe(0); + expect(tracker.getRunningCount()).toBe(0); }); });