fix: address codex review — path traversal, retry logic, type safety

- 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 <noreply@anthropic.com>
This commit is contained in:
Prateek 2026-02-13 18:00:05 +05:30
parent d6ec95402d
commit c49649c2c1
4 changed files with 59 additions and 21 deletions

View File

@ -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

View File

@ -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 },
})
);

View File

@ -43,8 +43,18 @@ function serializeMetadata(data: Record<string, string>): 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);
}

View File

@ -68,6 +68,19 @@ function safeJsonParse<T>(str: string): T | null {
}
}
/** Valid session statuses for validation. */
const VALID_STATUSES: ReadonlySet<string> = 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, {