fix: address stage 2 PR review feedback (#95)

.changeset/stage2-evidence-recovery.md

@@ -0,0 +1,5 @@
+---
+"@aoagents/ao-core": minor
+---
+
+Improve lifecycle detection to use bounded `detecting` retries when runtime, process, and activity evidence disagree, and make recovery validation escalate probe uncertainty for human review instead of treating it as cleanup-safe death.

CHANGELOG.md

@@ -1,11 +0,0 @@
-# Changelog
-
-All notable changes to this project will be documented in this file.
-
-## [Unreleased]
-
-### Changed
-- Improve lifecycle assessment to use evidence-driven `detecting` retries before declaring sessions dead when runtime, process, and activity signals disagree.
-
-### Fixed
-- Fix recovery validation so probe uncertainty and signal disagreement escalate for human review instead of being flattened into cleanup-safe dead states.

packages/core/src/__tests__/lifecycle-manager.test.ts

@@ -370,6 +370,31 @@ describe("check (single session)", () => {
     expect(meta?.["lifecycleEvidence"]).toContain("signal_disagreement");
   });
 
+  it("enters detecting when runtime is dead but process state is unknown", async () => {
+    const registryWithoutAgent = createMockRegistry({
+      runtime: plugins.runtime,
+      agent: plugins.agent,
+    });
+    vi.mocked(registryWithoutAgent.get).mockImplementation((slot: string, name?: string) => {
+      if (slot === "runtime") return plugins.runtime;
+      if (slot === "agent") return null;
+      return null;
+    });
+    vi.mocked(plugins.runtime.isAlive).mockResolvedValue(false);
+
+    const lm = setupCheck("app-1", {
+      session: makeSession({ status: "working" }),
+      registry: registryWithoutAgent,
+    });
+
+    await lm.check("app-1");
+
+    expect(lm.getStates().get("app-1")).toBe("detecting");
+    const meta = readMetadataRaw(env.sessionsDir, "app-1");
+    expect(meta?.["lifecycleEvidence"]).toContain("runtime_dead process_unknown");
+    expect(meta?.["detectingAttempts"]).toBe("1");
+  });
+
   it("escalates detecting to stuck after bounded retries", async () => {
     vi.mocked(plugins.runtime.isAlive).mockResolvedValue(false);
     vi.mocked(plugins.agent.getActivityState).mockResolvedValue({

packages/core/src/__tests__/recovery-validator.test.ts

@@ -310,4 +310,75 @@ describe("recovery validator", () => {
     expect(assessment.action).toBe("escalate");
     expect(assessment.reason).toContain("Signal disagreement");
   });
+
+  it("keeps terminal metadata unrecoverable even when probes are uncertain", 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 mockWorkspace: Workspace = {
+      name: "worktree",
+      create: vi.fn(),
+      destroy: vi.fn(),
+      list: vi.fn(),
+      exists: vi.fn().mockResolvedValue(false),
+    };
+    const registry: PluginRegistry = {
+      register: vi.fn(),
+      get: vi.fn().mockImplementation((slot: string) => {
+        if (slot === "workspace") return mockWorkspace;
+        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: projectPath,
+        status: "merged",
+      },
+    };
+
+    const assessment = await validateSession(scanned, config, registry);
+
+    expect(assessment.classification).toBe("unrecoverable");
+    expect(assessment.recoveryRule).toBe("skip");
+    expect(assessment.action).toBe("skip");
+  });
 });

packages/core/src/lifecycle-manager.ts

@@ -635,6 +635,17 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan
       );
     }
 
+    if (
+      runtimeProbe.state === "dead" &&
+      processProbe.state === "unknown" &&
+      canProbeRuntimeIdentity
+    ) {
+      return buildDetectingAssessment(
+        `runtime_dead process_unknown activity=${activityFreshness}`,
+        "runtime_lost",
+      );
+    }
+
     if (
       runtimeProbe.state === "dead" &&
       processProbe.state === "dead" &&
@@ -1452,16 +1463,29 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan
     const lifecycleChanged = session.metadata["statePayload"] !== JSON.stringify(session.lifecycle);
     let transitionReaction: { key: string; result: ReactionResult | null } | undefined;
 
-    updateSessionMetadata(session, {
-      lifecycleEvidence: assessment.evidence,
-      detectingAttempts:
-        assessment.detectingAttempts > 0 ? String(assessment.detectingAttempts) : "",
-      detectingEscalatedAt:
-        newStatus === SESSION_STATUS.STUCK &&
-        assessment.detectingAttempts > DETECTING_MAX_ATTEMPTS
-          ? new Date().toISOString()
-          : "",
-    });
+    const nextLifecycleEvidence = assessment.evidence;
+    const nextDetectingAttempts =
+      assessment.detectingAttempts > 0 ? String(assessment.detectingAttempts) : "";
+    const isDetectingEscalated =
+      newStatus === SESSION_STATUS.STUCK &&
+      assessment.detectingAttempts > DETECTING_MAX_ATTEMPTS;
+    const nextDetectingEscalatedAt = isDetectingEscalated
+      ? (session.metadata["detectingEscalatedAt"] || new Date().toISOString())
+      : "";
+
+    const metadataUpdates: Record<string, string> = {};
+    if (session.metadata["lifecycleEvidence"] !== nextLifecycleEvidence) {
+      metadataUpdates["lifecycleEvidence"] = nextLifecycleEvidence;
+    }
+    if ((session.metadata["detectingAttempts"] || "") !== nextDetectingAttempts) {
+      metadataUpdates["detectingAttempts"] = nextDetectingAttempts;
+    }
+    if ((session.metadata["detectingEscalatedAt"] || "") !== nextDetectingEscalatedAt) {
+      metadataUpdates["detectingEscalatedAt"] = nextDetectingEscalatedAt;
+    }
+    if (Object.keys(metadataUpdates).length > 0) {
+      updateSessionMetadata(session, metadataUpdates);
+    }
 
     if (newStatus !== oldStatus) {
       const correlationId = createCorrelationId("lifecycle-transition");

packages/core/src/recovery/validator.ts

@@ -151,6 +151,10 @@ function classifySession(
   runtimeProbeSucceeded: boolean,
   processProbeSucceeded: boolean,
 ): RecoveryClassification {
+  if (TERMINAL_STATUSES_SET.has(metadataStatus)) {
+    return "unrecoverable";
+  }
+
   if (metadataStatus === "detecting" || !runtimeProbeSucceeded || !processProbeSucceeded) {
     return "partial";
   }
@@ -160,9 +164,6 @@ function classifySession(
   }
 
   if (!runtimeAlive && !workspaceExists) {
-    if (TERMINAL_STATUSES_SET.has(metadataStatus)) {
-      return "unrecoverable";
-    }
     return "dead";
   }