diff --git a/docs/agent-lifecycle.md b/docs/agent-lifecycle.md index 3d50231fa..bf8e26948 100644 --- a/docs/agent-lifecycle.md +++ b/docs/agent-lifecycle.md @@ -83,9 +83,16 @@ only the route's explicit Unarchive or Restore action changes the archived works History navigation preserves the selected agent as an explicit recovery target. If both that agent and its workspace are archived, the workspace recovery action restores the workspace and unarchives the selected agent as one user action. Other archived agents in the restored workspace remain -recoverable from History. Opening one pins its tab and renders the archived-agent callout before any -provider timeline is loaded; **Unarchive** runs the provider's native unarchive hook (including Codex -`thread/unarchive`) before the normal agent resume and timeline hydration flow. +recoverable from History. Opening one pins its tab and renders the archived-agent callout. Authoritative +timeline catch-up may load provider history with a runtime-only `history` resume purpose, which must +leave both Paseo's `archivedAt` and the provider's native archive state unchanged. **Unarchive** remains +the only transition back to an interactive runtime: it runs the provider's native unarchive hook +(including Codex `thread/unarchive`) before the normal agent resume and timeline hydration flow. + +Provider session connection owns every process it spawns until the session is registered with +`AgentManager`. If initialization, persisted-session resume, or initial history hydration fails, +`connect()` must dispose that process before rethrowing; the manager cannot clean up a session it never +received. ## Tabs vs archive diff --git a/packages/server/src/server/agent/agent-loading.test.ts b/packages/server/src/server/agent/agent-loading.test.ts new file mode 100644 index 000000000..e4943a079 --- /dev/null +++ b/packages/server/src/server/agent/agent-loading.test.ts @@ -0,0 +1,82 @@ +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { expect, test } from "vitest"; + +import { createTestLogger } from "../../test-utils/test-logger.js"; +import { AgentManager } from "./agent-manager.js"; +import { ensureAgentLoaded } from "./agent-loading.js"; +import { AgentStorage } from "./agent-storage.js"; +import type { + AgentClient, + AgentLaunchContext, + AgentPersistenceHandle, + AgentResumeSessionOptions, + AgentSession, + AgentSessionConfig, +} from "./agent-sdk-types.js"; +import { createTestAgentClients } from "../test-utils/fake-agent-client.js"; + +test("loads archived records for history and active records with the interactive default", async () => { + const root = await mkdtemp(path.join(tmpdir(), "agent-loading-purpose-")); + const logger = createTestLogger(); + const storage = new AgentStorage(path.join(root, "agents"), logger); + const baseClient = createTestAgentClients().codex; + if (!baseClient) { + throw new Error("expected Codex test client"); + } + + const resumeOptions: Array = []; + const client: AgentClient = { + provider: baseClient.provider, + capabilities: baseClient.capabilities, + createSession: async ( + config: AgentSessionConfig, + launchContext?: AgentLaunchContext, + ): Promise => await baseClient.createSession(config, launchContext), + resumeSession: async ( + handle: AgentPersistenceHandle, + overrides?: Partial, + launchContext?: AgentLaunchContext, + options?: AgentResumeSessionOptions, + ): Promise => { + resumeOptions.push(options); + return await baseClient.resumeSession(handle, overrides, launchContext); + }, + fetchCatalog: async (options) => await baseClient.fetchCatalog(options), + isAvailable: async () => await baseClient.isAvailable(), + }; + const manager = new AgentManager({ + clients: { codex: client }, + registry: storage, + logger, + }); + + const archivedId = "00000000-0000-4000-8000-000000000301"; + const activeId = "00000000-0000-4000-8000-000000000302"; + + try { + const archived = await manager.createAgent({ provider: "codex", cwd: root }, archivedId, { + workspaceId: "workspace-archived", + }); + await manager.archiveAgent(archived.id); + + const active = await manager.createAgent({ provider: "codex", cwd: root }, activeId, { + workspaceId: "workspace-active", + }); + await manager.closeAgent(active.id); + + await ensureAgentLoaded(archived.id, { agentManager: manager, agentStorage: storage, logger }); + await ensureAgentLoaded(active.id, { agentManager: manager, agentStorage: storage, logger }); + + expect(resumeOptions).toEqual([{ purpose: "history" }, undefined]); + } finally { + await Promise.all([ + manager.closeAgent(archivedId).catch(() => undefined), + manager.closeAgent(activeId).catch(() => undefined), + ]); + await manager.flush().catch(() => undefined); + await storage.flush().catch(() => undefined); + await rm(root, { recursive: true, force: true }); + } +}); diff --git a/packages/server/src/server/agent/agent-loading.ts b/packages/server/src/server/agent/agent-loading.ts index 6f54864b3..c4a32e5f5 100644 --- a/packages/server/src/server/agent/agent-loading.ts +++ b/packages/server/src/server/agent/agent-loading.ts @@ -111,6 +111,7 @@ export async function ensureAgentLoaded( buildConfigOverrides(record), agentId, extractTimestamps(record), + record.archivedAt ? { purpose: "history" } : undefined, ); deps.logger.info({ agentId, provider: record.provider }, "Agent resumed from persistence"); } else { diff --git a/packages/server/src/server/agent/agent-manager.ts b/packages/server/src/server/agent/agent-manager.ts index cfda95c1b..3337ceca6 100644 --- a/packages/server/src/server/agent/agent-manager.ts +++ b/packages/server/src/server/agent/agent-manager.ts @@ -19,6 +19,7 @@ import { type AgentCapabilityFlags, type AgentClient, type AgentCreateSessionOptions, + type AgentResumeSessionOptions, type AgentFeature, type AgentLaunchContext, type AgentSlashCommand, @@ -1078,9 +1079,10 @@ export class AgentManager { workspaceId?: string; owner?: AgentOwner; }, + resumeOptions?: AgentResumeSessionOptions, ): Promise { return this.trackAgentRegistrationOperation( - this.resumeAgentFromPersistenceInternal(handle, overrides, agentId, options), + this.resumeAgentFromPersistenceInternal(handle, overrides, agentId, options, resumeOptions), ); } @@ -1096,6 +1098,7 @@ export class AgentManager { workspaceId?: string; owner?: AgentOwner; }, + resumeOptions?: AgentResumeSessionOptions, ): Promise { this.assertAcceptingAgentRegistrations(); const resolvedAgentId = validateAgentId( @@ -1122,7 +1125,12 @@ export class AgentManager { } const launchContext = await this.buildLaunchContext(resolvedAgentId, client); const providerLaunchConfig = this.resolveProviderLaunchConfig(launchConfig, launchContext); - const session = await client.resumeSession(handle, providerLaunchConfig, launchContext); + const session = await client.resumeSession( + handle, + providerLaunchConfig, + launchContext, + resumeOptions, + ); return this.registerSession(session, storedConfig, resolvedAgentId, { ...options, persistence: handle, diff --git a/packages/server/src/server/agent/agent-sdk-types.ts b/packages/server/src/server/agent/agent-sdk-types.ts index 6836cd8bc..e473f75ec 100644 --- a/packages/server/src/server/agent/agent-sdk-types.ts +++ b/packages/server/src/server/agent/agent-sdk-types.ts @@ -602,6 +602,12 @@ export interface AgentCreateSessionOptions { persistSession?: boolean; } +/** Runtime-only intent for a persisted-session resume. Never persist this option. */ +export interface AgentResumeSessionOptions { + /** Defaults to interactive. History loading may be read-only for archived native sessions. */ + purpose?: "interactive" | "history"; +} + /** * Returned by respondToPermission when the permission resolution requires * a follow-up turn (e.g. Codex plan approval → implementation). @@ -688,6 +694,7 @@ export interface AgentClient { handle: AgentPersistenceHandle, overrides?: Partial, launchContext?: AgentLaunchContext, + options?: AgentResumeSessionOptions, ): Promise; /** * Discover models and modes together. Implementations may use one upstream diff --git a/packages/server/src/server/agent/provider-registry.ts b/packages/server/src/server/agent/provider-registry.ts index dbdb5c076..96be83d16 100644 --- a/packages/server/src/server/agent/provider-registry.ts +++ b/packages/server/src/server/agent/provider-registry.ts @@ -408,7 +408,7 @@ function wrapClientProvider( launchContext, ), ), - resumeSession: async (handle, overrides, launchContext) => + resumeSession: async (handle, overrides, launchContext, options) => wrapSessionProvider( provider, await inner.resumeSession( @@ -423,6 +423,7 @@ function wrapClientProvider( } : undefined, launchContext, + options, ), ), fetchCatalog: async (options) => { diff --git a/packages/server/src/server/agent/providers/acp-agent.test.ts b/packages/server/src/server/agent/providers/acp-agent.test.ts index 201d4e0e4..07e392793 100644 --- a/packages/server/src/server/agent/providers/acp-agent.test.ts +++ b/packages/server/src/server/agent/providers/acp-agent.test.ts @@ -2742,6 +2742,81 @@ describe("ACPAgentSession close() tree-kill", () => { }); }); +describe("ACPAgentSession initialization cleanup", () => { + test("terminates the ACP process when session/new fails", async () => { + const terminator = new FakeTerminator(); + const child = createProbeChildStub(); + + class FailingNewSession extends ACPAgentSession { + protected override async spawnProcess(): Promise { + return { + child, + connection: { + newSession: vi.fn().mockRejectedValue(new Error("session/new failed")), + } as unknown as ClientSideConnection, + initialize: { agentCapabilities: {} }, + }; + } + } + + const session = new FailingNewSession( + { provider: "copilot", cwd: "/tmp/paseo-acp-test" }, + { + provider: "copilot", + logger: createTestLogger(), + defaultCommand: ["copilot", "--acp"], + defaultModes: [], + capabilities: { + supportsStreaming: true, + supportsSessionPersistence: true, + }, + terminateProcess: terminator.terminate, + }, + ); + + await expect(session.initializeNewSession()).rejects.toThrow("session/new failed"); + + expect(terminator.terminated).toContain(child); + }); + + test("terminates the ACP process when session/load fails", async () => { + const terminator = new FakeTerminator(); + const child = createProbeChildStub(); + + class FailingLoadSession extends ACPAgentSession { + protected override async spawnProcess(): Promise { + return { + child, + connection: { + loadSession: vi.fn().mockRejectedValue(new Error("session/load failed")), + } as unknown as ClientSideConnection, + initialize: { agentCapabilities: { loadSession: true } }, + }; + } + } + + const session = new FailingLoadSession( + { provider: "cursor", cwd: "/tmp/paseo-acp-test" }, + { + provider: "cursor", + logger: createTestLogger(), + defaultCommand: ["cursor-agent", "acp"], + defaultModes: [], + capabilities: { + supportsStreaming: true, + supportsSessionPersistence: true, + }, + handle: { provider: "cursor", sessionId: "session-1" }, + terminateProcess: terminator.terminate, + }, + ); + + await expect(session.initializeResumedSession()).rejects.toThrow("session/load failed"); + + expect(terminator.terminated).toContain(child); + }); +}); + describe("ACPAgentClient probe cleanup", () => { afterEach(() => { vi.restoreAllMocks(); diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index ddb28b01d..033e21238 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -1376,21 +1376,25 @@ export class ACPAgentSession implements AgentSession, ACPClient { } async initializeNewSession(): Promise { - const spawned = await this.spawnProcess(); - this.child = spawned.child; - this.connection = spawned.connection; - this.agentCapabilities = spawned.initialize.agentCapabilities ?? null; + try { + const spawned = await this.spawnProcess(); + this.child = spawned.child; + this.connection = spawned.connection; + this.agentCapabilities = spawned.initialize.agentCapabilities ?? null; - const response = await this.runACPRequest(() => - this.connection!.newSession({ - cwd: this.config.cwd, - mcpServers: this.acpMcpServers(), - }), - ); - this.sessionId = response.sessionId; - this.bootstrapThreadEventPending = true; - this.applySessionState(response); - await this.applyConfiguredOverrides(); + const response = await this.runACPRequest(() => + this.connection!.newSession({ + cwd: this.config.cwd, + mcpServers: this.acpMcpServers(), + }), + ); + this.sessionId = response.sessionId; + this.bootstrapThreadEventPending = true; + this.applySessionState(response); + await this.applyConfiguredOverrides(); + } catch (error) { + await this.closeAfterInitializationFailure(error); + } } /** @@ -1401,45 +1405,61 @@ export class ACPAgentSession implements AgentSession, ACPClient { * from these calls regardless of capabilities. */ async initializeResumedSession(): Promise { - const handle = this.initialHandle; - if (!handle) { - throw new Error("Resume requested without persistence handle"); + try { + const handle = this.initialHandle; + if (!handle) { + throw new Error("Resume requested without persistence handle"); + } + + const spawned = await this.spawnProcess(); + this.child = spawned.child; + this.connection = spawned.connection; + this.agentCapabilities = spawned.initialize.agentCapabilities ?? null; + this.sessionId = handle.sessionId; + this.bootstrapThreadEventPending = true; + + const sessionCapabilities = this.agentCapabilities?.sessionCapabilities; + if (this.agentCapabilities?.loadSession) { + this.replayingHistory = true; + const response = await this.runACPRequest(() => + this.connection!.loadSession({ + sessionId: handle.sessionId, + cwd: this.config.cwd, + mcpServers: this.acpMcpServers(), + }), + ); + this.replayingHistory = false; + this.historyPending = this.persistedHistory.length > 0; + this.applySessionState(response); + } else if (sessionCapabilities?.resume) { + const response = await this.runACPRequest(() => + this.connection!.unstable_resumeSession({ + sessionId: handle.sessionId, + cwd: this.config.cwd, + mcpServers: this.acpMcpServers(), + }), + ); + this.applySessionState(response); + } else { + throw new Error(`${this.provider} does not support ACP session resume`); + } + + await this.applyConfiguredOverrides(); + } catch (error) { + await this.closeAfterInitializationFailure(error); } + } - const spawned = await this.spawnProcess(); - this.child = spawned.child; - this.connection = spawned.connection; - this.agentCapabilities = spawned.initialize.agentCapabilities ?? null; - this.sessionId = handle.sessionId; - this.bootstrapThreadEventPending = true; - - const sessionCapabilities = this.agentCapabilities?.sessionCapabilities; - if (this.agentCapabilities?.loadSession) { - this.replayingHistory = true; - const response = await this.runACPRequest(() => - this.connection!.loadSession({ - sessionId: handle.sessionId, - cwd: this.config.cwd, - mcpServers: this.acpMcpServers(), - }), + private async closeAfterInitializationFailure(error: unknown): Promise { + try { + await this.close(); + } catch (closeError) { + this.logger.warn( + { err: closeError, initializationError: error }, + "Failed to close ACP process after session initialization failure", ); - this.replayingHistory = false; - this.historyPending = this.persistedHistory.length > 0; - this.applySessionState(response); - } else if (sessionCapabilities?.resume) { - const response = await this.runACPRequest(() => - this.connection!.unstable_resumeSession({ - sessionId: handle.sessionId, - cwd: this.config.cwd, - mcpServers: this.acpMcpServers(), - }), - ); - this.applySessionState(response); - } else { - throw new Error(`${this.provider} does not support ACP session resume`); } - - await this.applyConfiguredOverrides(); + throw error; } async run(prompt: AgentPromptInput, options?: AgentRunOptions): Promise { @@ -2335,6 +2355,10 @@ export class ACPAgentSession implements AgentSession, ACPClient { { logger: this.logger, provider: this.provider }, ); const connection = new ClientSideConnection(() => this, stream); + // Take ownership before initialize so the outer initialization guard can + // close the process even when the ACP handshake itself rejects. + this.child = child; + this.connection = connection; const initialize = await this.runACPRequest(() => connection.initialize({ protocolVersion: PROTOCOL_VERSION, diff --git a/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts b/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts index a9df432f4..50c3baaae 100644 --- a/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts +++ b/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts @@ -106,6 +106,37 @@ function createSession( return session; } +function createProviderWithFakeAppServer(appServer: FakeCodexAppServer): CodexAppServerAgentClient { + const provider = new CodexAppServerAgentClient(createTestLogger()); + const internals = castInternals<{ + goalsEnabledPromise: Promise | null; + autoReviewEnabledPromise: Promise | null; + spawnAppServer: () => Promise; + }>(provider); + internals.goalsEnabledPromise = Promise.resolve(false); + internals.autoReviewEnabledPromise = Promise.resolve(false); + internals.spawnAppServer = async () => appServer.child; + return provider; +} + +function archivedThreadHandle() { + return { + sessionId: "archived-thread-id", + metadata: { + cwd: "/tmp/codex-question-test", + modeId: "auto", + model: "gpt-5.4", + }, + }; +} + +function archivedThreadErrorMessage(threadId: string): string { + return ( + `session ${threadId} is archived. ` + + `Run \`codex unarchive ${threadId}\` to unarchive it first.` + ); +} + function asInternals(session: CodexTestSession): CodexSessionTestAccess { return castInternals(session); } @@ -871,6 +902,66 @@ describe("Codex app-server provider", () => { await session.close(); }); + test("loads archived Codex history without resuming the native thread", async () => { + const threadRequests: string[] = []; + const appServer = createFakeCodexAppServer({ + "thread/loaded/list": () => { + threadRequests.push("thread/loaded/list"); + return { data: [] }; + }, + "thread/resume": () => { + threadRequests.push("thread/resume"); + return Promise.reject(new Error(archivedThreadErrorMessage("archived-thread-id"))); + }, + "thread/read": () => { + threadRequests.push("thread/read"); + return { thread: { turns: [] } }; + }, + }); + const provider = createProviderWithFakeAppServer(appServer); + + const session = await provider.resumeSession(archivedThreadHandle(), undefined, undefined, { + purpose: "history", + }); + + expect(threadRequests).toEqual(["thread/loaded/list", "thread/resume", "thread/read"]); + await session.close(); + appServer.assertNoErrors(); + }); + + test("closes Codex app-server when an interactive resume fails", async () => { + const appServer = createFakeCodexAppServer({ + "thread/resume": () => + Promise.reject(new Error(archivedThreadErrorMessage("archived-thread-id"))), + }); + const killSpy = vi.spyOn(appServer.child, "kill"); + const provider = createProviderWithFakeAppServer(appServer); + + await expect(provider.resumeSession(archivedThreadHandle())).rejects.toThrow( + archivedThreadErrorMessage("archived-thread-id"), + ); + + expect(killSpy).toHaveBeenCalledWith("SIGTERM"); + appServer.assertNoErrors(); + }); + + test("closes Codex app-server when archived history hydration fails", async () => { + const appServer = createFakeCodexAppServer({ + "thread/resume": () => + Promise.reject(new Error(archivedThreadErrorMessage("archived-thread-id"))), + "thread/read": () => Promise.reject(new Error("thread history is unavailable")), + }); + const killSpy = vi.spyOn(appServer.child, "kill"); + const provider = createProviderWithFakeAppServer(appServer); + + await expect( + provider.resumeSession(archivedThreadHandle(), undefined, undefined, { purpose: "history" }), + ).rejects.toThrow("thread history is unavailable"); + + expect(killSpy).toHaveBeenCalledWith("SIGTERM"); + appServer.assertNoErrors(); + }); + test("unarchives a persisted Codex thread through app-server", async () => { const threadRequests: Array<{ method: string; params: unknown }> = []; const appServer = createFakeCodexAppServer({ @@ -1113,12 +1204,7 @@ describe("Codex app-server provider", () => { }; }, }); - const provider = new CodexAppServerAgentClient(createTestLogger()); - castInternals<{ goalsEnabledPromise: Promise | null }>(provider).goalsEnabledPromise = - Promise.resolve(false); - castInternals<{ spawnAppServer: () => Promise }>( - provider, - ).spawnAppServer = async () => appServer.child; + const provider = createProviderWithFakeAppServer(appServer); const outcome = await Promise.race([ provider diff --git a/packages/server/src/server/agent/providers/codex-app-server-agent.ts b/packages/server/src/server/agent/providers/codex-app-server-agent.ts index ac3017dac..19f691106 100644 --- a/packages/server/src/server/agent/providers/codex-app-server-agent.ts +++ b/packages/server/src/server/agent/providers/codex-app-server-agent.ts @@ -6,6 +6,7 @@ import { type AgentCreateSessionOptions, type AgentFeature, type AgentLaunchContext, + type AgentResumeSessionOptions, type AgentMode, type AgentModelDefinition, type McpServerConfig, @@ -110,6 +111,14 @@ function isRecord(value: unknown): value is Record { return value != null && typeof value === "object" && !Array.isArray(value); } +function isArchivedCodexThreadResumeError(error: unknown, threadId: string): boolean { + if (!(error instanceof Error)) return false; + const expectedMessage = + `session ${threadId} is archived. ` + + `Run \`codex unarchive ${threadId}\` to unarchive it first.`; + return error.message === expectedMessage; +} + function isCodexAlreadyUnarchivedError(error: unknown, threadId: string): boolean { const message = error instanceof Error ? error.message : String(error); return message.includes(`no archived rollout found for thread id ${threadId}`); @@ -3174,6 +3183,7 @@ export class CodexAppServerAgentSession implements AgentSession { private readonly goalsEnabled: boolean = false, private readonly autoReviewEnabled: boolean = false, private readonly agentId?: string, + private readonly initialResumePurpose: "interactive" | "history" = "interactive", ) { this.logger = logger.child({ module: "agent", @@ -3220,18 +3230,32 @@ export class CodexAppServerAgentSession implements AgentSession { this.client.setNotificationHandler((method, params) => this.handleNotification(method, params)); this.registerRequestHandlers(); - await this.client.request("initialize", buildCodexAppServerInitializeParams()); - this.client.notify("initialized", {}); + try { + await this.client.request("initialize", buildCodexAppServerInitializeParams()); + this.client.notify("initialized", {}); - await this.loadCollaborationModes(); - await this.loadSkills(); + await this.loadCollaborationModes(); + await this.loadSkills(); - if (this.currentThreadId) { - await this.ensureThreadLoaded(); - await this.loadPersistedHistory(); + if (this.currentThreadId) { + await this.ensureThreadLoaded({ + allowArchivedHistory: this.initialResumePurpose === "history", + }); + await this.loadPersistedHistory(); + } + + this.connected = true; + } catch (error) { + try { + await this.close(); + } catch (closeError) { + this.logger.warn( + { err: closeError, connectError: error }, + "Failed to close Codex app-server after connection failure", + ); + } + throw error; } - - this.connected = true; } private traceContext(): CodexAppServerTraceContext { @@ -3537,7 +3561,9 @@ export class CodexAppServerAgentSession implements AgentSession { } } - private async ensureThreadLoaded(): Promise { + private async ensureThreadLoaded( + options: { allowArchivedHistory?: boolean } = {}, + ): Promise { if (!this.client || !this.currentThreadId) return; try { const loaded = toObjectRecord(await this.client.request("thread/loaded/list", {})); @@ -3561,6 +3587,16 @@ export class CodexAppServerAgentSession implements AgentSession { } catch (error) { const threadId = this.currentThreadId; const message = error instanceof Error ? error.message : String(error); + if ( + options.allowArchivedHistory === true && + isArchivedCodexThreadResumeError(error, threadId) + ) { + this.logger.info( + { threadId }, + "Loading archived Codex thread history without resuming the native session", + ); + return; + } this.logger.warn({ error, threadId }, "Failed to resume persisted Codex thread"); throw new Error(`Failed to resume Codex thread ${threadId}: ${message}`, { cause: error }); } @@ -6303,6 +6339,7 @@ export class CodexAppServerAgentClient implements AgentClient { handle: { sessionId: string; metadata?: Record }, overrides?: Partial, launchContext?: AgentLaunchContext, + options?: AgentResumeSessionOptions, ): Promise { const storedConfig = (handle.metadata ?? {}) as Partial; const merged: AgentSessionConfig = { @@ -6324,6 +6361,7 @@ export class CodexAppServerAgentClient implements AgentClient { goalsEnabled, autoReviewEnabled, launchContext?.agentId, + options?.purpose ?? "interactive", ); await session.connect(); return session;