fix: protect orchestrators from session cleanup (#453)

harsh-batheja · sisyphus-dev-ai · web-flow · commit 1d960feb6ba8 · 2026-03-13T09:39:49.000+05:30
* fix: harden orchestrator cleanup selection Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * fix: suppress orchestrators in cleanup command output Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * fix: remove unreachable cleanup guard branch Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> --------- Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
diff --git a/packages/cli/__tests__/commands/session.test.ts b/packages/cli/__tests__/commands/session.test.ts
@@ -565,6 +565,77 @@ describe("session cleanup", () => {
     expect(output).toContain("Cleaned: app-2");
   });
 
+  it("suppresses orchestrator cleanup output while preserving worker cleanup output", async () => {
+    mockSessionManager.cleanup.mockResolvedValue({
+      killed: ["app-orchestrator", "app-2"],
+      skipped: [],
+      errors: [{ sessionId: "app-orchestrator", error: "should never surface" }],
+    } satisfies CleanupResult);
+
+    await program.parseAsync(["node", "test", "session", "cleanup"]);
+
+    const output = consoleSpy.mock.calls.map((c) => String(c[0])).join("\n");
+    const errOutput = vi
+      .mocked(console.error)
+      .mock.calls.map((c) => String(c[0]))
+      .join("\n");
+
+    expect(output).toContain("Cleaned: app-2");
+    expect(output).not.toContain("app-orchestrator");
+    expect(output).toContain("Cleanup complete. 1 sessions cleaned");
+    expect(errOutput).not.toContain("app-orchestrator");
+  });
+
+  it("treats orchestrator-only cleanup results as no-op output", async () => {
+    mockSessionManager.cleanup.mockResolvedValue({
+      killed: ["app-orchestrator"],
+      skipped: [],
+      errors: [],
+    } satisfies CleanupResult);
+
+    await program.parseAsync(["node", "test", "session", "cleanup"]);
+
+    const output = consoleSpy.mock.calls.map((c) => String(c[0])).join("\n");
+    expect(output).toContain("No sessions to clean up");
+    expect(output).not.toContain("app-orchestrator");
+  });
+
+  it("suppresses orchestrators in cleanup dry-run output", async () => {
+    mockSessionManager.cleanup.mockResolvedValue({
+      killed: ["app-orchestrator", "app-3"],
+      skipped: [],
+      errors: [],
+    } satisfies CleanupResult);
+
+    await program.parseAsync(["node", "test", "session", "cleanup", "--dry-run"]);
+
+    const output = consoleSpy.mock.calls.map((c) => String(c[0])).join("\n");
+    expect(output).toContain("Would kill app-3");
+    expect(output).not.toContain("app-orchestrator");
+    expect(output).toContain("1 session would be cleaned");
+  });
+
+  it("suppresses project-prefixed orchestrator cleanup results", async () => {
+    mockSessionManager.cleanup.mockResolvedValue({
+      killed: ["my-app:app-orchestrator", "my-app:app-4"],
+      skipped: [],
+      errors: [{ sessionId: "my-app:app-orchestrator", error: "should never surface" }],
+    } satisfies CleanupResult);
+
+    await program.parseAsync(["node", "test", "session", "cleanup"]);
+
+    const output = consoleSpy.mock.calls.map((c) => String(c[0])).join("\n");
+    const errOutput = vi
+      .mocked(console.error)
+      .mock.calls.map((c) => String(c[0]))
+      .join("\n");
+
+    expect(output).toContain("Cleaned: my-app:app-4");
+    expect(output).not.toContain("my-app:app-orchestrator");
+    expect(output).toContain("Cleanup complete. 1 sessions cleaned");
+    expect(errOutput).not.toContain("my-app:app-orchestrator");
+  });
+
   it("skips sessions without metadata", async () => {
     // No metadata files exist — list returns empty, cleanup returns empty
     mockSessionManager.cleanup.mockResolvedValue({
diff --git a/packages/cli/__tests__/lib/session-utils.test.ts b/packages/cli/__tests__/lib/session-utils.test.ts
@@ -1,5 +1,10 @@
 import { describe, it, expect } from "vitest";
-import { escapeRegex, matchesPrefix, findProjectForSession } from "../../src/lib/session-utils.js";
+import {
+  escapeRegex,
+  matchesPrefix,
+  findProjectForSession,
+  isOrchestratorSessionName,
+} from "../../src/lib/session-utils.js";
 import type { OrchestratorConfig } from "@composio/ao-core";
 
 describe("escapeRegex", () => {
@@ -103,3 +108,42 @@ describe("findProjectForSession", () => {
     expect(findProjectForSession(config, "b-2")).toBe("beta");
   });
 });
+
+describe("isOrchestratorSessionName", () => {
+  const makeConfig = (projects: Record<string, { sessionPrefix?: string }>): OrchestratorConfig =>
+    ({
+      dataDir: "/tmp",
+      worktreeDir: "/tmp/wt",
+      port: 3000,
+      defaults: { runtime: "tmux", agent: "claude-code", workspace: "worktree", notifiers: [] },
+      projects: Object.fromEntries(
+        Object.entries(projects).map(([id, p]) => [
+          id,
+          { name: id, repo: "", path: "", defaultBranch: "main", ...p },
+        ]),
+      ),
+      notifiers: {},
+      notificationRouting: {},
+      reactions: {},
+    }) as OrchestratorConfig;
+
+  it("matches the canonical orchestrator ID for a known project", () => {
+    const config = makeConfig({ "my-app": { sessionPrefix: "app" } });
+    expect(isOrchestratorSessionName(config, "app-orchestrator", "my-app")).toBe(true);
+  });
+
+  it("matches orchestrator IDs across configured projects without an explicit project", () => {
+    const config = makeConfig({ "my-app": { sessionPrefix: "app" } });
+    expect(isOrchestratorSessionName(config, "app-orchestrator")).toBe(true);
+  });
+
+  it("falls back to suffix detection for legacy orchestrator names", () => {
+    const config = makeConfig({ "my-app": { sessionPrefix: "app" } });
+    expect(isOrchestratorSessionName(config, "legacy-orchestrator")).toBe(true);
+  });
+
+  it("does not classify worker session IDs as orchestrators", () => {
+    const config = makeConfig({ "my-app": { sessionPrefix: "app" } });
+    expect(isOrchestratorSessionName(config, "app-12", "my-app")).toBe(false);
+  });
+});
diff --git a/packages/cli/src/commands/session.ts b/packages/cli/src/commands/session.ts
@@ -5,6 +5,7 @@ import { loadConfig, SessionNotRestorableError, WorkspaceMissingError } from "@c
 import { git, getTmuxActivity, tmux } from "../lib/shell.js";
 import { formatAge } from "../lib/format.js";
 import { getSessionManager } from "../lib/create-session-manager.js";
+import { isOrchestratorSessionName } from "../lib/session-utils.js";
 
 export function registerSession(program: Command): void {
   const session = program
@@ -145,10 +146,31 @@ export function registerSession(program: Command): void {
 
       const sm = await getSessionManager(config);
 
+      const filterCleanupIds = (ids: string[]): string[] =>
+        ids.filter((entry) => {
+          const separator = entry.indexOf(":");
+          const entryProjectId = separator === -1 ? opts.project : entry.slice(0, separator);
+          const sessionId = separator === -1 ? entry : entry.slice(separator + 1);
+          return !isOrchestratorSessionName(config, sessionId, entryProjectId);
+        });
+
+      const filterCleanupErrors = (errors: Array<{ sessionId: string; error: string }>) =>
+        errors.filter(({ sessionId }) => {
+          const separator = sessionId.indexOf(":");
+          const entryProjectId = separator === -1 ? opts.project : sessionId.slice(0, separator);
+          const normalizedSessionId = separator === -1 ? sessionId : sessionId.slice(separator + 1);
+          return !isOrchestratorSessionName(config, normalizedSessionId, entryProjectId);
+        });
+
       if (opts.dryRun) {
         // Dry-run delegates to sm.cleanup() with dryRun flag so it uses the
         // same live checks (PR state, runtime alive, tracker) as actual cleanup.
-        const result = await sm.cleanup(opts.project, { dryRun: true });
+        const rawResult = await sm.cleanup(opts.project, { dryRun: true });
+        const result = {
+          ...rawResult,
+          killed: filterCleanupIds(rawResult.killed),
+          errors: filterCleanupErrors(rawResult.errors),
+        };
 
         if (result.errors.length > 0) {
           for (const { sessionId, error } of result.errors) {
@@ -171,7 +193,12 @@ export function registerSession(program: Command): void {
           }
         }
       } else {
-        const result = await sm.cleanup(opts.project);
+        const rawResult = await sm.cleanup(opts.project);
+        const result = {
+          ...rawResult,
+          killed: filterCleanupIds(rawResult.killed),
+          errors: filterCleanupErrors(rawResult.errors),
+        };
 
         if (result.killed.length === 0 && result.errors.length === 0) {
           console.log(chalk.dim("  No sessions to clean up."));
diff --git a/packages/cli/src/lib/session-utils.ts b/packages/cli/src/lib/session-utils.ts
@@ -14,11 +14,37 @@ export function findProjectForSession(
   config: OrchestratorConfig,
   sessionName: string,
 ): string | null {
-  for (const [id, project] of Object.entries(config.projects)) {
+  for (const [id, project] of Object.entries(config.projects) as Array<
+    [string, OrchestratorConfig["projects"][string]]
+  >) {
     const prefix = project.sessionPrefix || id;
     if (matchesPrefix(sessionName, prefix)) {
       return id;
     }
   }
   return null;
 }
+
+export function isOrchestratorSessionName(
+  config: OrchestratorConfig,
+  sessionName: string,
+  projectId?: string,
+): boolean {
+  if (projectId) {
+    const project = config.projects[projectId];
+    if (project && sessionName === `${project.sessionPrefix || projectId}-orchestrator`) {
+      return true;
+    }
+  }
+
+  for (const [id, project] of Object.entries(config.projects) as Array<
+    [string, OrchestratorConfig["projects"][string]]
+  >) {
+    const prefix = project.sessionPrefix || id;
+    if (sessionName === `${prefix}-orchestrator`) {
+      return true;
+    }
+  }
+
+  return sessionName.endsWith("-orchestrator");
+}
diff --git a/packages/core/src/__tests__/session-manager.test.ts b/packages/core/src/__tests__/session-manager.test.ts
@@ -2289,6 +2289,101 @@ describe("cleanup", () => {
     expect(result.skipped).toContain("app-orchestrator");
   });
 
+  it("never cleans the canonical orchestrator session even with stale worker-like metadata", async () => {
+    const deleteLogPath = join(tmpDir, "opencode-delete-orchestrator.log");
+    const mockBin = installMockOpencode("[]", deleteLogPath);
+    process.env.PATH = `${mockBin}:${originalPath ?? ""}`;
+
+    const deadRuntime: Runtime = {
+      ...mockRuntime,
+      isAlive: vi.fn().mockResolvedValue(false),
+    };
+    const mockSCM: SCM = {
+      name: "mock-scm",
+      detectPR: vi.fn(),
+      getPRState: vi.fn().mockResolvedValue("merged"),
+      mergePR: vi.fn(),
+      closePR: vi.fn(),
+      getCIChecks: vi.fn(),
+      getCISummary: vi.fn(),
+      getReviews: vi.fn(),
+      getReviewDecision: vi.fn(),
+      getPendingComments: vi.fn(),
+      getAutomatedComments: vi.fn(),
+      getMergeability: vi.fn(),
+    };
+    const mockTracker: Tracker = {
+      name: "mock-tracker",
+      getIssue: vi.fn().mockResolvedValue({
+        id: "INT-42",
+        title: "Issue",
+        description: "",
+        url: "https://example.com/INT-42",
+        state: "closed",
+        labels: [],
+      }),
+      isCompleted: vi.fn().mockResolvedValue(true),
+      issueUrl: vi.fn().mockReturnValue("https://example.com/INT-42"),
+      branchName: vi.fn().mockReturnValue("feat/INT-42"),
+      generatePrompt: vi.fn().mockResolvedValue(""),
+    };
+    const registryWithSignals: PluginRegistry = {
+      ...mockRegistry,
+      get: vi.fn().mockImplementation((slot: string) => {
+        if (slot === "runtime") return deadRuntime;
+        if (slot === "agent") return mockAgent;
+        if (slot === "workspace") return mockWorkspace;
+        if (slot === "scm") return mockSCM;
+        if (slot === "tracker") return mockTracker;
+        return null;
+      }),
+    };
+
+    writeMetadata(sessionsDir, "app-orchestrator", {
+      worktree: "/tmp",
+      branch: "main",
+      status: "ci_failed",
+      project: "my-app",
+      issue: "INT-42",
+      pr: "https://github.com/org/repo/pull/10",
+      agent: "opencode",
+      opencodeSessionId: "ses_orchestrator_active",
+      runtimeHandle: JSON.stringify(makeHandle("rt-orchestrator")),
+    });
+
+    const sm = createSessionManager({ config, registry: registryWithSignals });
+    const result = await sm.cleanup();
+
+    expect(result.killed).not.toContain("app-orchestrator");
+    expect(result.skipped).toContain("app-orchestrator");
+    expect(existsSync(deleteLogPath)).toBe(false);
+  });
+
+  it("never cleans archived orchestrator mappings even when metadata looks stale", async () => {
+    const deleteLogPath = join(tmpDir, "opencode-delete-archived-orchestrator.log");
+    const mockBin = installMockOpencode("[]", deleteLogPath);
+    process.env.PATH = `${mockBin}:${originalPath ?? ""}`;
+
+    writeMetadata(sessionsDir, "app-orchestrator", {
+      worktree: "/tmp",
+      branch: "main",
+      status: "killed",
+      project: "my-app",
+      agent: "opencode",
+      opencodeSessionId: "ses_orchestrator_archived",
+      pr: "https://github.com/org/repo/pull/88",
+      runtimeHandle: JSON.stringify(makeHandle("rt-orchestrator")),
+    });
+    deleteMetadata(sessionsDir, "app-orchestrator", true);
+
+    const sm = createSessionManager({ config, registry: mockRegistry });
+    const result = await sm.cleanup();
+
+    expect(result.killed).not.toContain("app-orchestrator");
+    expect(result.skipped).toContain("app-orchestrator");
+    expect(existsSync(deleteLogPath)).toBe(false);
+  });
+
   it("kills sessions with dead runtimes", async () => {
     const deadRuntime: Runtime = {
       ...mockRuntime,
diff --git a/packages/core/src/session-manager.ts b/packages/core/src/session-manager.ts
@@ -363,6 +363,18 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM
     return raw["role"] === "orchestrator" || sessionId.endsWith("-orchestrator");
   }
 
+  function isCleanupProtectedSession(
+    project: ProjectConfig,
+    sessionId: string,
+    metadata?: Record<string, string> | null,
+  ): boolean {
+    const canonicalOrchestratorId = `${project.sessionPrefix}-orchestrator`;
+    return (
+      sessionId === canonicalOrchestratorId ||
+      isOrchestratorSession({ id: sessionId, metadata: metadata ?? undefined })
+    );
+  }
+
   function applyMetadataUpdatesToRaw(
     raw: Record<string, string>,
     updates: Partial<Record<string, string>>,
@@ -1643,16 +1655,13 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM
 
     for (const session of sessions) {
       try {
-        // Never clean up orchestrator sessions — they manage the lifecycle.
-        // Check explicit role metadata first, fall back to naming convention
-        // for pre-existing sessions spawned before the role field was added.
-        if (isOrchestratorSession(session)) {
+        const project = config.projects[session.projectId];
+        if (!project) {
           pushSkipped(session.projectId, session.id);
           continue;
         }
 
-        const project = config.projects[session.projectId];
-        if (!project) {
+        if (isCleanupProtectedSession(project, session.id, session.metadata)) {
           pushSkipped(session.projectId, session.id);
           continue;
         }
@@ -1718,7 +1727,7 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM
         const archived = readArchivedMetadataRaw(sessionsDir, archivedId);
         if (!archived) continue;
 
-        if (isOrchestratorSession({ id: archivedId, metadata: archived })) {
+        if (isCleanupProtectedSession(project, archivedId, archived)) {
           pushSkipped(projectKey, archivedId);
           continue;
         }
