diff --git a/src/features/tmux-subagent/session-created-handler.test.ts b/src/features/tmux-subagent/session-created-handler.test.ts new file mode 100644 index 000000000..8b0fdd4aa --- /dev/null +++ b/src/features/tmux-subagent/session-created-handler.test.ts @@ -0,0 +1,209 @@ +import { describe, test, expect, mock, afterEach } from "bun:test" + +// --------------------------------------------------------------------------- +// Module-level mocks — must be registered BEFORE importing the handler so the +// handler picks up the mocked exports instead of the real implementations. +// queryWindowState and executeActions hit real tmux/spawn subprocesses; we +// replace them with spies so the spawn path can actually be exercised in tests. +// --------------------------------------------------------------------------- + +const mockQueryWindowState = mock(async (_paneId: string) => ({ + windowWidth: 244, + windowHeight: 44, + mainPane: { paneId: "%0", width: 130, height: 44, left: 0, top: 0, title: "main", isActive: true }, + agentPanes: [], +})) + +const mockExecuteActions = mock(async (_actions: unknown[], _ctx: unknown) => ({ + success: true, + spawnedPaneId: "%99", + results: [], +})) + +mock.module("./pane-state-querier", () => ({ queryWindowState: mockQueryWindowState })) +mock.module("./action-executor", () => ({ executeActions: mockExecuteActions })) + +import type { SessionCreatedHandlerDeps } from "./session-created-handler" +import { handleSessionCreated } from "./session-created-handler" +import type { SessionCreatedEvent } from "./session-created-event" + +afterEach(() => { + mockQueryWindowState.mockClear() + mockExecuteActions.mockClear() +}) + +function makeEvent(sessionId: string, parentID = "parent-session"): SessionCreatedEvent { + return { + type: "session.created", + properties: { + info: { id: sessionId, parentID, title: "TestAgent" }, + }, + } +} + +// --------------------------------------------------------------------------- +// Factory – returns fresh mocks + deps for each test +// --------------------------------------------------------------------------- + +function makeDeps(overrides: Partial = {}): { + deps: SessionCreatedHandlerDeps + mockExecuteActions: ReturnType + mockWaitForSessionReady: ReturnType +} { + const mockExecuteActions = mock(async () => ({ + success: true, + spawnedPaneId: "%99", + results: [], + })) + + const mockWaitForSessionReady = mock(async (_sessionId: string) => true) + + const deps: SessionCreatedHandlerDeps = { + client: {} as never, + tmuxConfig: { enabled: true } as never, + directory: "/tmp/test", + serverUrl: "http://127.0.0.1:42000", + sourcePaneId: "%0", + sessions: new Map(), + pendingSessions: new Set(), + isInsideTmux: () => true, + isEnabled: () => true, + getCapacityConfig: () => ({ mainPaneMinWidth: 130, agentPaneWidth: 52 }), + getSessionMappings: () => [], + waitForSessionReady: mockWaitForSessionReady, + startPolling: mock(() => {}), + ...overrides, + } + + return { deps, mockExecuteActions, mockWaitForSessionReady } +} + +// --------------------------------------------------------------------------- +// Inject executeActions via module mock +// --------------------------------------------------------------------------- + +// We test ordering by observing call order via a shared call-log array. + +describe("handleSessionCreated – #3505 session readiness race", () => { + test("#given session not yet ready #when session.created fires #then pane is NOT spawned", async () => { + const callLog: string[] = [] + + const waitForSessionReady = mock(async (_id: string) => { + callLog.push("waitForSessionReady") + return false // session never becomes ready + }) + + const { deps } = makeDeps({ waitForSessionReady }) + + // Patch executeActions on the module after import — use the real module path + // but intercept via deps indirection through action-executor by spying on + // startPolling (it must NOT be called if spawn is skipped). + const startPolling = mock(() => { callLog.push("startPolling") }) + deps.startPolling = startPolling + + const event = makeEvent("ses_notready") + // queryWindowState will return null if no real tmux — skip through by + // providing sourcePaneId=undefined so the handler returns early after readiness. + // Instead, test the readiness gate directly by bypassing window-state with + // a paneId that queryWindowState can handle gracefully. + // Since queryWindowState hits real tmux, we override sourcePaneId-less path: + deps.sourcePaneId = undefined + + await handleSessionCreated(deps, event) + + // No pane spawned, no polling started + expect(startPolling).not.toHaveBeenCalled() + expect(waitForSessionReady).not.toHaveBeenCalled() // short-circuits at sourcePaneId check + }) + + test("#given spawn path reached #when waitForSessionReady is pending #then executeActions is deferred until readiness resolves", async () => { + // Regression test for #3505: the handler must `await waitForSessionReady` + // BEFORE calling executeActions. This test actually exercises the spawn + // path (mocked queryWindowState returns a valid window state, mocked + // executeActions is a spy) so the readiness-then-spawn ordering is + // observable and asserted, not assumed. + const callLog: string[] = [] + let resolveReadiness: ((ready: boolean) => void) | undefined + const readinessGate = new Promise((resolve) => { resolveReadiness = resolve }) + + const waitForSessionReady = mock(async (_id: string): Promise => { + callLog.push("waitForSessionReady:start") + const ready = await readinessGate + callLog.push("waitForSessionReady:end") + return ready + }) + mockExecuteActions.mockImplementation(async (_actions, _ctx) => { + callLog.push("executeActions") + return { success: true, spawnedPaneId: "%99", results: [] } + }) + + const { deps } = makeDeps({ waitForSessionReady }) + const handlerPromise = handleSessionCreated(deps, makeEvent("ses_race")) + + // Yield so the handler reaches the readiness gate; executeActions must NOT + // have been invoked yet because waitForSessionReady has not resolved. + await Promise.resolve() + await Promise.resolve() + expect(waitForSessionReady).toHaveBeenCalledTimes(1) + expect(mockExecuteActions).not.toHaveBeenCalled() + + resolveReadiness!(true) + await handlerPromise + + // Now executeActions must have fired exactly once, AFTER waitForSessionReady. + expect(mockExecuteActions).toHaveBeenCalledTimes(1) + expect(callLog).toEqual(["waitForSessionReady:start", "waitForSessionReady:end", "executeActions"]) + }) + + test("#given spawn path reached #when waitForSessionReady resolves false #then executeActions is never called", async () => { + const waitForSessionReady = mock(async (_id: string) => false) + const { deps } = makeDeps({ waitForSessionReady }) + + await handleSessionCreated(deps, makeEvent("ses_notready_spawn")) + + expect(waitForSessionReady).toHaveBeenCalledTimes(1) + expect(mockExecuteActions).not.toHaveBeenCalled() + }) + + test("#given duplicate session.created events #when first is pending #then second is deduplicated", async () => { + const { deps, mockWaitForSessionReady } = makeDeps() + deps.pendingSessions.add("ses_dup") + + const event = makeEvent("ses_dup") + await handleSessionCreated(deps, event) + + // Should bail out at the duplicate guard, never reaching readiness check + expect(mockWaitForSessionReady).not.toHaveBeenCalled() + }) + + test("#given non session.created event #when handler called #then no action taken", async () => { + const { deps, mockWaitForSessionReady } = makeDeps() + + const event: SessionCreatedEvent = { + type: "session.idle", + properties: { info: { id: "ses_idle", parentID: "parent" } }, + } + + await handleSessionCreated(deps, event as never) + expect(mockWaitForSessionReady).not.toHaveBeenCalled() + }) + + test("#given session already tracked #when session.created fires again #then idempotent", async () => { + const { deps, mockWaitForSessionReady } = makeDeps() + // Pre-populate sessions map as if pane was already spawned + deps.sessions.set("ses_existing", { + sessionId: "ses_existing", + paneId: "%5", + description: "TestAgent", + createdAt: new Date(), + lastSeenAt: new Date(), + closePending: false, + closeRetryCount: 0, + }) + + const event = makeEvent("ses_existing") + await handleSessionCreated(deps, event) + + expect(mockWaitForSessionReady).not.toHaveBeenCalled() + }) +}) diff --git a/src/features/tmux-subagent/session-created-handler.ts b/src/features/tmux-subagent/session-created-handler.ts index a80cdd546..1d806ecc7 100644 --- a/src/features/tmux-subagent/session-created-handler.ts +++ b/src/features/tmux-subagent/session-created-handler.ts @@ -102,6 +102,19 @@ export async function handleSessionCreated( return } + // Wait for the child session to be registered in the opencode server's status + // map BEFORE spawning the tmux pane. If we spawn first, `opencode attach` + // exits immediately (session not yet visible), tmux auto-closes the pane, and + // the subagent runs invisibly in the background — the bug described in #3505. + const sessionReady = await deps.waitForSessionReady(sessionId) + if (!sessionReady) { + log("[tmux-session-manager] session readiness failed before spawn", { + sessionId, + stage: "session.created", + }) + return + } + const result = await executeActions(decision.actions, { config: deps.tmuxConfig, directory: deps.directory, @@ -137,26 +150,6 @@ export async function handleSessionCreated( return } - const sessionReady = await deps.waitForSessionReady(sessionId) - if (!sessionReady) { - log("[tmux-session-manager] session not ready after timeout, closing spawned pane", { - sessionId, - paneId: result.spawnedPaneId, - }) - - await executeActions( - [{ type: "close", paneId: result.spawnedPaneId, sessionId }], - { - config: deps.tmuxConfig, - directory: deps.directory, - serverUrl: deps.serverUrl, - windowState: state, - }, - ) - - return - } - deps.sessions.set( sessionId, createTrackedSession({