From 3c2e89f427174d5d25495c76a41a8448e5efb813 Mon Sep 17 00:00:00 2001 From: arkon Date: Mon, 26 Jan 2026 13:58:34 +0100 Subject: [PATCH] fix: address medium severity issues from code review - Fix inconsistent null handling in state-store (use ?? instead of ||) - Add JSON.stringify error handling with specific messages - Add error handling for SSE init event in app.js - Fix spawn orchestrator silent failures (add logging) - Fix statSync race condition in subagent-watcher - Fix unbounded _taskNumberToContent Map in ralph-tracker - Fix YAML pattern case sensitivity in ralph-config Co-Authored-By: Claude Opus 4.5 --- CLAUDE.md | 2 +- package.json | 2 +- src/ralph-config.ts | 4 +-- src/ralph-tracker.ts | 20 +++++++++++++++ src/session.ts | 16 ++++++++++++ src/spawn-orchestrator.ts | 52 ++++++++++++++++++++++++--------------- src/state-store.ts | 28 +++++++++++++++------ src/subagent-watcher.ts | 10 ++++++-- src/web/public/app.js | 6 ++++- 9 files changed, 106 insertions(+), 34 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index cc819e1d..9ceb2955 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -21,7 +21,7 @@ When user says "COM": 1) Increment version in BOTH `package.json` AND `CLAUDE.md Claudeman is a Claude Code session manager with a web interface and autonomous Ralph Loop. It spawns Claude CLI processes via PTY, streams output in real-time via SSE, and supports scheduled/timed runs. -**Version**: 0.1382 (must match `package.json`) +**Version**: 0.1383 (must match `package.json`) **Tech Stack**: TypeScript (ES2022/NodeNext, strict mode), Node.js, Fastify, Server-Sent Events, node-pty diff --git a/package.json b/package.json index b773e894..5b987408 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "claudeman", - "version": "0.1382", + "version": "0.1383", "description": "The missing control plane for Claude Code - run 20 autonomous agents with real-time monitoring and session persistence", "type": "module", "main": "dist/index.js", diff --git a/src/ralph-config.ts b/src/ralph-config.ts index 5f480519..ad42ac26 100644 --- a/src/ralph-config.ts +++ b/src/ralph-config.ts @@ -18,7 +18,7 @@ const CLAUDE_MD_PROMISE_PATTERN = /\s*([A-Z0-9_-]+)\s*<\/promise>/gi; // Pattern to parse YAML frontmatter from ralph-loop.local.md // Extracts key: value pairs from between --- markers const YAML_FRONTMATTER_PATTERN = /^---\s*\n([\s\S]*?)\n---/; -const YAML_LINE_PATTERN = /^([a-z-]+):\s*"?([^"\n]+)"?\s*$/gm; +const YAML_LINE_PATTERN = /^([a-zA-Z_-]+):\s*"?([^"\n]+)"?\s*$/gm; /** * Ralph Loop configuration from .claude/ralph-loop.local.md @@ -91,7 +91,7 @@ export function parseRalphLoopConfigFromContent(content: string): RalphLoopConfi switch (key) { case 'enabled': - config.enabled = value === 'true'; + config.enabled = value.toLowerCase() === 'true' || value.toLowerCase() === 'yes'; break; case 'iteration': config.iteration = parseInt(value, 10) || 0; diff --git a/src/ralph-tracker.ts b/src/ralph-tracker.ts index 3b9eee56..4f47c7aa 100644 --- a/src/ralph-tracker.ts +++ b/src/ralph-tracker.ts @@ -217,6 +217,9 @@ const TASK_DONE_PATTERN = /(?:task|item|todo)\s*(?:#?\d+|"\s*[^"]+\s*")?\s*(?:is */ const ANSI_ESCAPE_PATTERN = /\x1b\[[0-9;]*[A-Za-z]/g; +/** Maximum number of task number to content mappings to track */ +const MAX_TASK_MAPPINGS = 100; + // ========== Event Types ========== /** @@ -1159,6 +1162,7 @@ export class RalphTracker extends EventEmitter { const content = match[2].trim(); if (content.length >= 5) { this._taskNumberToContent.set(taskNum, content); + this.enforceTaskMappingLimit(); this.upsertTodo(content, 'pending'); updated = true; } @@ -1173,6 +1177,7 @@ export class RalphTracker extends EventEmitter { // Only register if not already known from a "created" line if (!this._taskNumberToContent.has(taskNum)) { this._taskNumberToContent.set(taskNum, content); + this.enforceTaskMappingLimit(); } this.upsertTodo(this._taskNumberToContent.get(taskNum) || content, 'pending'); updated = true; @@ -1456,6 +1461,21 @@ export class RalphTracker extends EventEmitter { this.emit('loopUpdate', this.loopState); } + /** + * Enforce size limit on _taskNumberToContent map. + * Removes lowest task numbers (oldest tasks) when limit exceeded. + */ + private enforceTaskMappingLimit(): void { + if (this._taskNumberToContent.size <= MAX_TASK_MAPPINGS) return; + + // Sort keys and remove lowest (oldest) task numbers + const sortedKeys = Array.from(this._taskNumberToContent.keys()).sort((a, b) => a - b); + const keysToRemove = sortedKeys.slice(0, this._taskNumberToContent.size - MAX_TASK_MAPPINGS); + for (const key of keysToRemove) { + this._taskNumberToContent.delete(key); + } + } + /** * Clear all state and disable the tracker. * diff --git a/src/session.ts b/src/session.ts index f256d588..641567b1 100644 --- a/src/session.ts +++ b/src/session.ts @@ -1250,6 +1250,9 @@ export class Session extends EventEmitter { } private processOutput(data: string): void { + // Early return if session is stopped to prevent any processing or timer creation + if (this._isStopped) return; + // Try to extract JSON from output (Claude may output JSON in stream mode) this._lineBuffer += data; @@ -1391,8 +1394,12 @@ export class Session extends EventEmitter { } } + /** Maximum number of task descriptions to keep */ + private static readonly MAX_TASK_DESCRIPTIONS = 100; + /** * Remove task descriptions older than TASK_DESCRIPTION_MAX_AGE_MS. + * Also enforces MAX_TASK_DESCRIPTIONS size limit. */ private cleanupOldTaskDescriptions(): void { const cutoff = Date.now() - Session.TASK_DESCRIPTION_MAX_AGE_MS; @@ -1401,6 +1408,15 @@ export class Session extends EventEmitter { this._recentTaskDescriptions.delete(timestamp); } } + + // Enforce size limit by removing oldest entries + if (this._recentTaskDescriptions.size > Session.MAX_TASK_DESCRIPTIONS) { + const sortedKeys = Array.from(this._recentTaskDescriptions.keys()).sort((a, b) => a - b); + const keysToRemove = sortedKeys.slice(0, this._recentTaskDescriptions.size - Session.MAX_TASK_DESCRIPTIONS); + for (const key of keysToRemove) { + this._recentTaskDescriptions.delete(key); + } + } } /** diff --git a/src/spawn-orchestrator.ts b/src/spawn-orchestrator.ts index 36be93b4..3d656221 100644 --- a/src/spawn-orchestrator.ts +++ b/src/spawn-orchestrator.ts @@ -408,7 +408,8 @@ export class SpawnOrchestrator extends EventEmitter { try { const content = readFileSync(progressPath, 'utf-8'); return JSON.parse(content) as AgentProgress; - } catch { + } catch (err) { + console.warn(`[spawn-orchestrator] Failed to read progress for agent ${agentId}: ${getErrorMessage(err)}`); return null; } } @@ -423,26 +424,37 @@ export class SpawnOrchestrator extends EventEmitter { const messagesDir = join(agent.commsDir, 'messages'); if (!existsSync(messagesDir)) return []; - const files = readdirSync(messagesDir) - .filter(f => f.endsWith('.md')) - .sort(); + try { + const files = readdirSync(messagesDir) + .filter(f => f.endsWith('.md')) + .sort(); - const messages: SpawnMessage[] = []; - for (const file of files) { - const match = file.match(/^(\d+)-(parent|agent)\.md$/); - if (!match) continue; + const messages: SpawnMessage[] = []; + for (const file of files) { + const match = file.match(/^(\d+)-(parent|agent)\.md$/); + if (!match) continue; - const content = readFileSync(join(messagesDir, file), 'utf-8'); - messages.push({ - sequence: parseInt(match[1]), - sender: match[2] as 'parent' | 'agent', - content, - sentAt: statSync(join(messagesDir, file)).mtimeMs, - read: true, - }); + try { + const filePath = join(messagesDir, file); + const content = readFileSync(filePath, 'utf-8'); + messages.push({ + sequence: parseInt(match[1]), + sender: match[2] as 'parent' | 'agent', + content, + sentAt: statSync(filePath).mtimeMs, + read: true, + }); + } catch (err) { + console.warn(`[spawn-orchestrator] Failed to read message file ${file}: ${getErrorMessage(err)}`); + // Continue processing other messages + } + } + + return messages; + } catch (err) { + console.warn(`[spawn-orchestrator] Failed to read messages for agent ${agentId}: ${getErrorMessage(err)}`); + return []; } - - return messages; } /** @@ -839,8 +851,8 @@ export class SpawnOrchestrator extends EventEmitter { if (agent.sessionId && this._sessionCreator) { try { await this._sessionCreator.stopSession(agent.sessionId); - } catch { - // Ignore cleanup errors + } catch (err) { + console.warn(`[spawn-orchestrator] Failed to stop session for agent ${agentId}: ${getErrorMessage(err)}`); } } diff --git a/src/state-store.ts b/src/state-store.ts index 5f30a9c2..a4d2ded1 100644 --- a/src/state-store.ts +++ b/src/state-store.ts @@ -123,11 +123,18 @@ export class StateStore { this.ensureDir(); // Atomic write: write to temp file, then rename (atomic on POSIX) const tempPath = this.filePath + '.tmp'; + let json: string; try { - writeFileSync(tempPath, JSON.stringify(this.state, null, 2), 'utf-8'); + json = JSON.stringify(this.state, null, 2); + } catch (err) { + console.error('[StateStore] Failed to serialize state (circular reference or invalid data):', err); + throw err; + } + try { + writeFileSync(tempPath, json, 'utf-8'); renameSync(tempPath, this.filePath); } catch (err) { - console.error('[StateStore] Failed to save state:', err); + console.error('[StateStore] Failed to write state file:', err); // Try to clean up temp file on error try { if (existsSync(tempPath)) { @@ -156,7 +163,7 @@ export class StateStore { /** Returns a session state by ID, or null if not found. */ getSession(id: string) { - return this.state.sessions[id] || null; + return this.state.sessions[id] ?? null; } /** Sets a session state and triggers a debounced save. */ @@ -178,7 +185,7 @@ export class StateStore { /** Returns a task state by ID, or null if not found. */ getTask(id: string) { - return this.state.tasks[id] || null; + return this.state.tasks[id] ?? null; } /** Sets a task state and triggers a debounced save. */ @@ -457,11 +464,18 @@ export class StateStore { const data = Object.fromEntries(this.ralphStates); // Atomic write: write to temp file, then rename (atomic on POSIX) const tempPath = this.ralphStatePath + '.tmp'; + let json: string; try { - writeFileSync(tempPath, JSON.stringify(data, null, 2), 'utf-8'); + json = JSON.stringify(data, null, 2); + } catch (err) { + console.error('[StateStore] Failed to serialize Ralph state (circular reference or invalid data):', err); + throw err; + } + try { + writeFileSync(tempPath, json, 'utf-8'); renameSync(tempPath, this.ralphStatePath); } catch (err) { - console.error('[StateStore] Failed to save Ralph state:', err); + console.error('[StateStore] Failed to write Ralph state file:', err); // Try to clean up temp file on error try { if (existsSync(tempPath)) { @@ -475,7 +489,7 @@ export class StateStore { /** Returns inner state for a session, or null if not found. */ getRalphState(sessionId: string): RalphSessionState | null { - return this.ralphStates.get(sessionId) || null; + return this.ralphStates.get(sessionId) ?? null; } /** Sets inner state for a session and triggers a debounced save. */ diff --git a/src/subagent-watcher.ts b/src/subagent-watcher.ts index 60ce54f7..00f081ab 100644 --- a/src/subagent-watcher.ts +++ b/src/subagent-watcher.ts @@ -798,8 +798,14 @@ export class SubagentWatcher extends EventEmitter { const agentId = basename(filePath).replace('agent-', '').replace('.jsonl', ''); - // Initial info - const stat = statSync(filePath); + // Initial info - handle race condition where file may be deleted between discovery and stat + let stat; + try { + stat = statSync(filePath); + } catch { + // File was deleted between discovery and stat - skip this agent + return; + } // Extract description - prefer reading from parent transcript (most reliable) // The parent transcript has the exact Task tool call with description parameter diff --git a/src/web/public/app.js b/src/web/public/app.js index 08256340..50cf008f 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -985,7 +985,11 @@ class ClaudemanApp { }; this.eventSource.addEventListener('init', (e) => { - this.handleInit(JSON.parse(e.data)); + try { + this.handleInit(JSON.parse(e.data)); + } catch (err) { + console.error('[SSE] Failed to parse init event:', err); + } }); this.eventSource.addEventListener('session:created', (e) => {