From f2f1a4ddb6453bd0ed88eacdc557bfdfba125943 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Thu, 16 Apr 2026 23:00:20 +0530 Subject: [PATCH] fix: address follow-up stage 2 review feedback (#95) --- .../src/__tests__/lifecycle-manager.test.ts | 19 ++++ .../src/__tests__/recovery-validator.test.ts | 99 ++++++++++++++++++- packages/core/src/lifecycle-manager.ts | 26 +---- packages/core/src/lifecycle-state.ts | 19 +--- packages/core/src/recovery/validator.ts | 13 ++- 5 files changed, 131 insertions(+), 45 deletions(-) diff --git a/packages/core/src/__tests__/lifecycle-manager.test.ts b/packages/core/src/__tests__/lifecycle-manager.test.ts index 6b09a5dd6..006553729 100644 --- a/packages/core/src/__tests__/lifecycle-manager.test.ts +++ b/packages/core/src/__tests__/lifecycle-manager.test.ts @@ -702,6 +702,25 @@ describe("check (single session)", () => { expect(lm.getStates().get("app-1")).toBe("merged"); }); + it("treats closed PRs as done when the canonical lifecycle marks them complete", async () => { + const mockSCM = createMockSCM({ getPRState: vi.fn().mockResolvedValue("closed") }); + const registry = createMockRegistry({ + runtime: plugins.runtime, + agent: plugins.agent, + scm: mockSCM, + }); + + const lm = setupCheck("app-1", { + session: makeSession({ status: "pr_open", pr: makePR() }), + registry, + }); + + await lm.check("app-1"); + expect(lm.getStates().get("app-1")).toBe("done"); + const meta = readMetadataRaw(env.sessionsDir, "app-1"); + expect(meta?.["status"]).toBe("done"); + }); + it("detects mergeable when approved + CI green", async () => { const mockSCM = createMockSCM({ getReviewDecision: vi.fn().mockResolvedValue("approved"), diff --git a/packages/core/src/__tests__/recovery-validator.test.ts b/packages/core/src/__tests__/recovery-validator.test.ts index 63dc5e66c..5af943dbf 100644 --- a/packages/core/src/__tests__/recovery-validator.test.ts +++ b/packages/core/src/__tests__/recovery-validator.test.ts @@ -202,7 +202,7 @@ describe("recovery validator", () => { project: config.projects.app, sessionsDir: getSessionsDir(config.configPath, projectPath), rawMetadata: { - worktree: projectPath, + worktree: join(rootDir, "missing-worktree"), status: "working", runtimeHandle: JSON.stringify({ id: "rt-1", runtimeName: "tmux", data: {} }), }, @@ -297,7 +297,7 @@ describe("recovery validator", () => { project: config.projects.app, sessionsDir: getSessionsDir(config.configPath, projectPath), rawMetadata: { - worktree: projectPath, + worktree: join(rootDir, "missing-worktree"), status: "working", runtimeHandle: JSON.stringify({ id: "rt-1", runtimeName: "tmux", data: {} }), }, @@ -381,4 +381,99 @@ describe("recovery validator", () => { expect(assessment.recoveryRule).toBe("skip"); expect(assessment.action).toBe("skip"); }); + + it("respects escalatePartial=false for non-disagreement partial sessions", async () => { + rootDir = join(tmpdir(), `ao-recovery-validator-${randomUUID()}`); + mkdirSync(rootDir, { recursive: true }); + const projectPath = join(rootDir, "project"); + mkdirSync(projectPath, { recursive: true }); + writeFileSync(join(rootDir, "agent-orchestrator.yaml"), "projects: {}\n", "utf-8"); + + const mockRuntime: Runtime = { + name: "tmux", + create: vi.fn(), + destroy: vi.fn(), + sendMessage: vi.fn(), + getOutput: vi.fn(), + isAlive: vi.fn().mockResolvedValue(true), + }; + const mockWorkspace: Workspace = { + name: "worktree", + create: vi.fn(), + destroy: vi.fn(), + list: vi.fn(), + exists: vi.fn().mockResolvedValue(false), + }; + const mockAgent: Agent = { + name: "mock-agent", + processName: "mock-agent", + getLaunchCommand: vi.fn(), + getEnvironment: vi.fn(), + detectActivity: vi.fn(), + getActivityState: vi.fn(), + isProcessRunning: vi.fn().mockResolvedValue(true), + getSessionInfo: vi.fn(), + }; + const registry: PluginRegistry = { + register: vi.fn(), + get: vi.fn().mockImplementation((slot: string) => { + if (slot === "runtime") return mockRuntime; + if (slot === "workspace") return mockWorkspace; + if (slot === "agent") return mockAgent; + return null; + }), + list: vi.fn().mockReturnValue([]), + loadBuiltins: vi.fn().mockResolvedValue(undefined), + loadFromConfig: vi.fn().mockResolvedValue(undefined), + }; + const config: OrchestratorConfig = { + configPath: join(rootDir, "agent-orchestrator.yaml"), + port: 3000, + readyThresholdMs: 300_000, + power: { preventIdleSleep: false }, + defaults: { + runtime: "tmux", + agent: "mock-agent", + workspace: "worktree", + notifiers: ["desktop"], + }, + projects: { + app: { + name: "app", + repo: "org/repo", + path: projectPath, + defaultBranch: "main", + sessionPrefix: "app", + }, + }, + notifiers: {}, + notificationRouting: { + urgent: ["desktop"], + action: ["desktop"], + warning: [], + info: [], + }, + reactions: {}, + }; + const scanned: ScannedSession = { + sessionId: "app-1", + projectId: "app", + project: config.projects.app, + sessionsDir: getSessionsDir(config.configPath, projectPath), + rawMetadata: { + worktree: join(rootDir, "missing-worktree"), + status: "working", + runtimeHandle: JSON.stringify({ id: "rt-1", runtimeName: "tmux", data: {} }), + }, + }; + + const assessment = await validateSession(scanned, config, registry, { + escalatePartial: false, + }); + + expect(assessment.classification).toBe("partial"); + expect(assessment.signalDisagreement).toBe(false); + expect(assessment.recoveryRule).toBe("auto"); + expect(assessment.action).toBe("cleanup"); + }); }); diff --git a/packages/core/src/lifecycle-manager.ts b/packages/core/src/lifecycle-manager.ts index f5914edd1..d88cc068e 100644 --- a/packages/core/src/lifecycle-manager.ts +++ b/packages/core/src/lifecycle-manager.ts @@ -387,26 +387,6 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan return "stale"; } - function buildDetectingResult( - session: Session, - evidence: string, - fallbackStatus: SessionStatus = SESSION_STATUS.STUCK, - ): DeterminedStatus { - const attempts = parseAttemptCount(session.metadata["detectingAttempts"]) + 1; - if (attempts > DETECTING_MAX_ATTEMPTS) { - return { - status: fallbackStatus, - evidence, - detectingAttempts: attempts, - }; - } - return { - status: SESSION_STATUS.DETECTING, - evidence, - detectingAttempts: attempts, - }; - } - /** Determine current status for a session by polling plugins. */ async function determineStatus(session: Session): Promise { const project = config.projects[session.projectId]; @@ -444,7 +424,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan detectingAttempts = parseAttemptCount(session.metadata["detectingAttempts"]), ): DeterminedStatus => { session.lifecycle = lifecycle; - session.status = deriveLegacyStatus(lifecycle, session.status); + session.status = status; return { status, evidence, @@ -699,7 +679,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan lifecycle.pr.state = "closed"; lifecycle.pr.reason = "closed_unmerged"; setSessionState("done", "research_complete"); - return commit(SESSION_STATUS.KILLED, "pr_closed", 0); + return commit(SESSION_STATUS.DONE, "pr_closed", 0); } lifecycle.pr.state = "open"; @@ -753,7 +733,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan lifecycle.pr.state = "closed"; lifecycle.pr.reason = "closed_unmerged"; setSessionState("done", "research_complete"); - return commit(SESSION_STATUS.KILLED, "pr_closed", 0); + return commit(SESSION_STATUS.DONE, "pr_closed", 0); } lifecycle.pr.state = "open"; diff --git a/packages/core/src/lifecycle-state.ts b/packages/core/src/lifecycle-state.ts index ce171572f..a347d768b 100644 --- a/packages/core/src/lifecycle-state.ts +++ b/packages/core/src/lifecycle-state.ts @@ -266,15 +266,7 @@ export function deriveLegacyStatus( case "idle": return lifecycle.pr.state === "merged" ? "merged" : "idle"; case "detecting": - if ( - lifecycle.session.reason === "runtime_lost" || - lifecycle.session.reason === "agent_process_exited" || - lifecycle.runtime.state === "missing" || - lifecycle.runtime.state === "exited" - ) { - return "killed"; - } - return TERMINAL_COMPATIBILITY_STATUS.has(previousStatus) ? "working" : previousStatus; + return "detecting"; case "working": if (lifecycle.pr.reason === "ci_failing") return "ci_failed"; if (lifecycle.pr.reason === "changes_requested") return "changes_requested"; @@ -286,15 +278,6 @@ export function deriveLegacyStatus( } } -const TERMINAL_COMPATIBILITY_STATUS: ReadonlySet = new Set([ - "killed", - "terminated", - "done", - "cleanup", - "errored", - "merged", -]); - export function buildLifecycleMetadataPatch( lifecycle: CanonicalSessionLifecycle, previousStatus?: SessionStatus, diff --git a/packages/core/src/recovery/validator.ts b/packages/core/src/recovery/validator.ts index f2eb6fed1..aaadd467d 100644 --- a/packages/core/src/recovery/validator.ts +++ b/packages/core/src/recovery/validator.ts @@ -110,7 +110,12 @@ export async function validateSession( runtimeProbeSucceeded && processProbeSucceeded && ((runtimeAlive && !agentProcessRunning) || (!runtimeAlive && agentProcessRunning)); - const recoveryRule = determineRecoveryRule(classification, signalDisagreement, metadataStatus); + const recoveryRule = determineRecoveryRule( + classification, + signalDisagreement, + metadataStatus, + recoveryConfig, + ); const action = determineAction(classification, metadataStatus, recoveryConfig, recoveryRule); return { @@ -186,11 +191,15 @@ function determineRecoveryRule( classification: RecoveryClassification, signalDisagreement: boolean, metadataStatus: SessionStatus, + recoveryConfig: RecoveryConfig = DEFAULT_RECOVERY_CONFIG, ): "auto" | "human" | "skip" { if (classification === "unrecoverable") return "skip"; - if (metadataStatus === "detecting" || signalDisagreement || classification === "partial") { + if (metadataStatus === "detecting" || signalDisagreement) { return "human"; } + if (classification === "partial") { + return recoveryConfig.escalatePartial ? "human" : "auto"; + } if (classification === "live" || classification === "dead") { return "auto"; }