From c49649c2c1ce7df7da8d1faead54b8ef1abf5887 Mon Sep 17 00:00:00 2001 From: Prateek Date: Fri, 13 Feb 2026 18:00:05 +0530 Subject: [PATCH] =?UTF-8?q?fix:=20address=20codex=20review=20=E2=80=94=20p?= =?UTF-8?q?ath=20traversal,=20retry=20logic,=20type=20safety?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - metadata: validate sessionId against ^[a-zA-Z0-9_-]+$ to prevent path traversal attacks - lifecycle-manager: fix reaction retry/escalation to count attempts (not just successes), escalate after maxRetries attempts; map session.killed to agent-exited reaction (was unreachable session.exited) - session-manager: validate status against known SessionStatus union (default to "spawning" for unknown values); clean up workspace on runtime creation failure to prevent resource leaks - event-bus: guard appendFileSync in try/catch so disk errors don't crash event emitters Co-Authored-By: Claude Opus 4.6 --- packages/core/src/event-bus.ts | 8 +++-- packages/core/src/lifecycle-manager.ts | 18 ++++++----- packages/core/src/metadata.ts | 10 ++++++ packages/core/src/session-manager.ts | 44 +++++++++++++++++++------- 4 files changed, 59 insertions(+), 21 deletions(-) diff --git a/packages/core/src/event-bus.ts b/packages/core/src/event-bus.ts index b5143375b..8d6560bce 100644 --- a/packages/core/src/event-bus.ts +++ b/packages/core/src/event-bus.ts @@ -117,9 +117,13 @@ export function createEventBus(logPath: string | null): EventBus { return { emit(event: OrchestratorEvent): void { - // Persist to JSONL + // Persist to JSONL (non-fatal on disk errors) if (logPath) { - appendFileSync(logPath, serializeEvent(event) + "\n", "utf-8"); + try { + appendFileSync(logPath, serializeEvent(event) + "\n", "utf-8"); + } catch { + // Disk error — event still propagated in-memory + } } // Store in memory diff --git a/packages/core/src/lifecycle-manager.ts b/packages/core/src/lifecycle-manager.ts index b78625340..5ab6bd2a7 100644 --- a/packages/core/src/lifecycle-manager.ts +++ b/packages/core/src/lifecycle-manager.ts @@ -79,7 +79,7 @@ function eventToReactionKey(eventType: EventType): string | null { case "merge.ready": return "approved-and-green"; case "session.stuck": return "agent-stuck"; case "session.needs_input": return "agent-needs-input"; - case "session.exited": return "agent-exited"; + case "session.killed": return "agent-exited"; case "summary.all_complete": return "all-complete"; default: return null; } @@ -92,9 +92,9 @@ export interface LifecycleManagerDeps { eventBus: EventBus; } -/** Track retry counts for reactions per session. */ +/** Track attempt counts for reactions per session. */ interface ReactionTracker { - retries: number; + attempts: number; firstTriggered: Date; } @@ -210,16 +210,19 @@ export function createLifecycleManager( let tracker = reactionTrackers.get(trackerKey); if (!tracker) { - tracker = { retries: 0, firstTriggered: new Date() }; + tracker = { attempts: 0, firstTriggered: new Date() }; reactionTrackers.set(trackerKey, tracker); } + // Increment attempts before checking escalation + tracker.attempts++; + // Check if we should escalate const maxRetries = reactionConfig.retries ?? Infinity; const escalateAfter = reactionConfig.escalateAfter; let shouldEscalate = false; - if (tracker.retries >= maxRetries) { + if (tracker.attempts > maxRetries) { shouldEscalate = true; } @@ -230,7 +233,7 @@ export function createLifecycleManager( } } - if (typeof escalateAfter === "number" && tracker.retries >= escalateAfter) { + if (typeof escalateAfter === "number" && tracker.attempts > escalateAfter) { shouldEscalate = true; } @@ -253,14 +256,13 @@ export function createLifecycleManager( if (reactionConfig.message) { try { await sessionManager.send(event.sessionId, reactionConfig.message); - tracker.retries++; eventBus.emit( createEvent("reaction.triggered", { sessionId: event.sessionId, projectId: event.projectId, message: `Reaction '${reactionKey}' sent message to agent`, - data: { reactionKey, retries: tracker.retries }, + data: { reactionKey, attempts: tracker.attempts }, }) ); diff --git a/packages/core/src/metadata.ts b/packages/core/src/metadata.ts index dd98211ec..351023b6b 100644 --- a/packages/core/src/metadata.ts +++ b/packages/core/src/metadata.ts @@ -43,8 +43,18 @@ function serializeMetadata(data: Record): string { .join("\n") + "\n"; } +/** Validate sessionId to prevent path traversal. */ +const VALID_SESSION_ID = /^[a-zA-Z0-9_-]+$/; + +function validateSessionId(sessionId: SessionId): void { + if (!VALID_SESSION_ID.test(sessionId)) { + throw new Error(`Invalid session ID: ${sessionId}`); + } +} + /** Get the metadata file path for a session. */ function metadataPath(dataDir: string, sessionId: SessionId): string { + validateSessionId(sessionId); return join(dataDir, "sessions", sessionId); } diff --git a/packages/core/src/session-manager.ts b/packages/core/src/session-manager.ts index e88a913f7..d5d847518 100644 --- a/packages/core/src/session-manager.ts +++ b/packages/core/src/session-manager.ts @@ -68,6 +68,19 @@ function safeJsonParse(str: string): T | null { } } +/** Valid session statuses for validation. */ +const VALID_STATUSES: ReadonlySet = new Set([ + "spawning", "working", "pr_open", "ci_failed", "review_pending", + "changes_requested", "approved", "mergeable", "merged", + "needs_input", "stuck", "errored", "killed", +]); + +/** Validate and normalize a status string. */ +function validateStatus(raw: string | undefined): SessionStatus { + if (raw && VALID_STATUSES.has(raw)) return raw as SessionStatus; + return "spawning"; +} + /** Reconstruct a Session object from raw metadata key=value pairs. */ function metadataToSession( sessionId: SessionId, @@ -76,7 +89,7 @@ function metadataToSession( return { id: sessionId, projectId: meta["project"] ?? "", - status: (meta["status"] ?? "unknown") as SessionStatus, + status: validateStatus(meta["status"]), activity: "idle", branch: meta["branch"] || null, issueId: meta["issue"] || null, @@ -201,16 +214,25 @@ export function createSessionManager(deps: SessionManagerDeps): SessionManager { const launchCommand = plugins.agent.getLaunchCommand(agentLaunchConfig); const environment = plugins.agent.getEnvironment(agentLaunchConfig); - // Create runtime session - const handle = await plugins.runtime.create({ - sessionId, - workspacePath, - launchCommand, - environment: { - ...environment, - AO_SESSION: sessionId, - }, - }); + // Create runtime session — clean up workspace on failure + let handle: RuntimeHandle; + try { + handle = await plugins.runtime.create({ + sessionId, + workspacePath, + launchCommand, + environment: { + ...environment, + AO_SESSION: sessionId, + }, + }); + } catch (err) { + // Clean up workspace if runtime creation failed + if (plugins.workspace && workspacePath !== project.path) { + try { await plugins.workspace.destroy(workspacePath); } catch { /* best effort */ } + } + throw err; + } // Write metadata writeMetadata(config.dataDir, sessionId, {