diff --git a/cli/src/agent/backends/acp/AcpSdkBackend.test.ts b/cli/src/agent/backends/acp/AcpSdkBackend.test.ts index 8b3e8fb5..096e9dee 100644 --- a/cli/src/agent/backends/acp/AcpSdkBackend.test.ts +++ b/cli/src/agent/backends/acp/AcpSdkBackend.test.ts @@ -1139,4 +1139,135 @@ describe('AcpSdkBackend', () => { expect(registered.get('cursor/ask_question')).toBe(handler); }); + + it('suppressUpdatesDuring drops session/update notifications that would otherwise leak into the previous turn\'s onUpdate, then restores normal forwarding', async () => { + // Reproduces the real /compact duplicate-summary bug: OpenCode keeps + // streaming session/update notifications (over the same ACP + // transport) while a raw-HTTP /compact call is in flight outside + // prompt(), and handleSessionUpdate forwards them unconditionally to + // whatever messageHandler is still installed from the last prompt() + // turn — rendering the same content a second time alongside the + // compact bridge's own explicit summary message. + // + // Fast quiet-drain timing so this test doesn't pay the real + // (production) 200ms/1200ms PRE_PROMPT_* delay suppressUpdatesDuring + // now waits through before restoring the handler. + backendStatics.PRE_PROMPT_UPDATE_QUIET_PERIOD_MS = 5; + backendStatics.PRE_PROMPT_UPDATE_DRAIN_TIMEOUT_MS = 50; + + const backend = new AcpSdkBackend({ command: 'opencode' }); + const backendInternal = backend as unknown as { + transport: { + sendRequest: (...args: unknown[]) => Promise; + close: () => Promise; + } | null; + handleSessionUpdate: (params: unknown) => void; + messageHandler: unknown; + }; + backendInternal.transport = { + sendRequest: async () => ({ stopReason: 'end_turn' }), + close: async () => {} + }; + + const turn1: AgentMessage[] = []; + await backend.prompt('session-1', [{ type: 'text', text: 'hi' }], (m) => turn1.push(m)); + + const emitPlanUpdate = () => backendInternal.handleSessionUpdate({ + sessionId: 'session-1', + update: { + sessionUpdate: ACP_SESSION_UPDATE_TYPES.plan, + entries: [{ content: 'leaked plan step', priority: 'medium', status: 'pending' }] + } + }); + + const handlerBeforeSuppression = backendInternal.messageHandler; + expect(handlerBeforeSuppression).not.toBeNull(); + + let handlerDuringSuppression: unknown = 'not-checked'; + const result = await backend.suppressUpdatesDuring(async () => { + handlerDuringSuppression = backendInternal.messageHandler; + emitPlanUpdate(); + return 'compact result'; + }); + + expect(result).toBe('compact result'); + expect(handlerDuringSuppression).toBeNull(); + expect(turn1.some((m) => m.type === 'plan')).toBe(false); + + // The previous turn's handler must be back in place afterward so + // ordinary straggler-forwarding (covered elsewhere) is unaffected. + expect(backendInternal.messageHandler).toBe(handlerBeforeSuppression); + emitPlanUpdate(); + expect(turn1.some((m) => m.type === 'plan')).toBe(true); + }); + + it('waits for a quiet period (reusing the same drain prompt() uses before swapping handlers) before restoring the handler after suppressUpdatesDuring, so a late server-side straggler from an already-aborted operation cannot leak', async () => { + // Reproduces a hostile-review finding: aborting the client-side HTTP + // call (e.g. compactAbortController) does not mean the OpenCode + // server actually stopped the operation — session/update is a + // separate notification channel from that HTTP request's lifecycle. + // If suppressUpdatesDuring restored the handler the instant `fn` + // resolved, a straggler notification arriving moments later (while + // the server is still winding the operation down) would leak + // straight into the restored handler. + backendStatics.PRE_PROMPT_UPDATE_QUIET_PERIOD_MS = 30; + backendStatics.PRE_PROMPT_UPDATE_DRAIN_TIMEOUT_MS = 300; + + const backend = new AcpSdkBackend({ command: 'opencode' }); + const backendInternal = backend as unknown as { + transport: { + sendRequest: (...args: unknown[]) => Promise; + close: () => Promise; + } | null; + handleSessionUpdate: (params: unknown) => void; + messageHandler: unknown; + }; + backendInternal.transport = { + sendRequest: async () => ({ stopReason: 'end_turn' }), + close: async () => {} + }; + + const turn1: AgentMessage[] = []; + await backend.prompt('session-1', [{ type: 'text', text: 'hi' }], (m) => turn1.push(m)); + + const emitPlanUpdate = () => backendInternal.handleSessionUpdate({ + sessionId: 'session-1', + update: { + sessionUpdate: ACP_SESSION_UPDATE_TYPES.plan, + entries: [{ content: 'late server-side straggler', priority: 'medium', status: 'pending' }] + } + }); + + const handlerBeforeSuppression = backendInternal.messageHandler; + + const suppressPromise = backend.suppressUpdatesDuring(async () => { + // Client gives up almost immediately (mirrors compactAbortController + // firing), but the server keeps streaming for a little longer — + // one update right away, one more 15ms later. + emitPlanUpdate(); + setTimeout(emitPlanUpdate, 15); + return 'aborted-early'; + }); + + // Sampled while suppressUpdatesDuring's own returned promise is + // still pending (fn already resolved, but the quiet-drain in its + // `finally` has not) — this is what actually proves restoration is + // *deferred*, not merely eventually correct. + await sleep(20); + const handlerDuringDrainWindow = backendInternal.messageHandler; + + const result = await suppressPromise; + + expect(result).toBe('aborted-early'); + expect(handlerDuringDrainWindow).toBeNull(); + // Neither the immediate update nor the +15ms straggler leaked — + // messageHandler was null (suppressed) for both. + expect(turn1.some((m) => m.type === 'plan')).toBe(false); + + expect(backendInternal.messageHandler).toBe(handlerBeforeSuppression); + + // Normal forwarding resumes once actually restored. + emitPlanUpdate(); + expect(turn1.some((m) => m.type === 'plan')).toBe(true); + }); }); diff --git a/cli/src/agent/backends/acp/AcpSdkBackend.ts b/cli/src/agent/backends/acp/AcpSdkBackend.ts index 6c5dd384..06d32db5 100644 --- a/cli/src/agent/backends/acp/AcpSdkBackend.ts +++ b/cli/src/agent/backends/acp/AcpSdkBackend.ts @@ -604,6 +604,68 @@ export class AcpSdkBackend implements AgentBackend { this.stderrErrorHandler = handler; } + /** + * Runs `fn` with `session/update` notifications temporarily prevented + * from reaching whatever `messageHandler` is currently installed (i.e. + * the last prompt() turn's handler), restoring it once `fn` settles. + * + * Needed for out-of-band calls that don't go through `prompt()` at all — + * e.g. OpenCode's /compact bridge, which triggers native compaction via + * a raw HTTP request to the agent subprocess instead of `session/prompt`. + * The agent keeps streaming `session/update` notifications (thought + * chunks etc.) over the same ACP transport while that HTTP call runs, + * and `handleSessionUpdate` forwards them unconditionally — with no + * prompt() turn in flight to own them, they'd otherwise land on the + * previous turn's now-stale `messageHandler` and render as a duplicate + * assistant message alongside whatever the caller explicitly displays + * from the HTTP response. + * + * `captureAvailableCommands` / `forwardSessionInfoUpdate` / + * `captureUsageUpdate` in `handleSessionUpdate` are untouched by this — + * only the `messageHandler.handleUpdate` forwarding is suppressed. + * + * Session-agnostic: this is a pure prompt()-adjacent utility with no + * Gemini/OpenCode-specific behavior, so it's safe on the shared + * AcpSdkBackend class — nothing calls it unless a caller opts in. + * + * The `this.messageHandler === null` guard on restore is defense in + * depth: normal serialization (compact and prompts run through the same + * single dequeue loop — see opencodeRemoteLauncher.ts) means `fn` should + * never overlap with a real prompt() turn, but if `disconnect()` or a + * new `prompt()` did run concurrently and changed `messageHandler` + * during `fn`, this avoids clobbering whatever it set. + * + * Restoring the handler waits for the same quiet-drain `prompt()` already + * uses before installing a *new* handler for the next turn (see its + * `PRE_PROMPT_UPDATE_QUIET_PERIOD_MS`/`_DRAIN_TIMEOUT_MS` call) — the + * same class of race, just on the way back in instead of the way out. + * Aborting `fn()` client-side (e.g. OpenCode's compact bridge aborting + * its HTTP call) does not necessarily stop the agent from continuing the + * operation server-side: `session/update` is a separate notification + * channel from that HTTP request's lifecycle (confirmed while building + * the /compact bridge — see runCompactOperation's doc comment). Without + * this wait, late notifications from a still-running server-side + * operation would immediately leak into whichever handler gets restored + * (or into a brand new one prompt() installs right after) the instant + * `fn()` returns. `messageHandler` stays null (suppression still in + * effect) for the whole drain, so nothing leaks during it either. + */ + async suppressUpdatesDuring(fn: () => Promise): Promise { + const previousHandler = this.messageHandler; + this.messageHandler = null; + try { + return await fn(); + } finally { + await this.waitForSessionUpdateQuiet( + AcpSdkBackend.PRE_PROMPT_UPDATE_QUIET_PERIOD_MS, + AcpSdkBackend.PRE_PROMPT_UPDATE_DRAIN_TIMEOUT_MS + ); + if (this.messageHandler === null) { + this.messageHandler = previousHandler; + } + } + } + /** * Returns true if currently processing a message (prompt in progress). * Useful for checking if it's safe to perform session operations. diff --git a/cli/src/modules/common/remote/RemoteLauncherBase.ts b/cli/src/modules/common/remote/RemoteLauncherBase.ts index bce64e0d..c473a0d5 100644 --- a/cli/src/modules/common/remote/RemoteLauncherBase.ts +++ b/cli/src/modules/common/remote/RemoteLauncherBase.ts @@ -91,10 +91,41 @@ export abstract class RemoteLauncherBase { rpcHandlerManager.registerHandler(RPC_METHODS.Switch, async () => {}); } + /** + * Hook for flavor-specific "we are leaving remote mode" bookkeeping. + * No-op by default — override only if a flavor has state that must stop + * being valid the instant remote mode starts tearing down (OpenCode's + * /compact availability flag is the motivating case; see + * OpencodeRemoteLauncher's override). + * + * Called from two places, both intentionally, since neither alone covers + * every way a launcher can stop being "in remote mode": + * 1. `requestExit()`, synchronously, as its very first action — before + * `shouldExit`/`exitReason` are even set, and long before the + * `handler` it's about to await (e.g. OpenCode's `handleAbort()`, + * which does real async teardown work like cancelling the ACP + * prompt) gets a chance to run. This is what actually closes a race + * window: anything gated on flavor state this hook resets can no + * longer slip through between "a switch/exit was requested" and + * "the async teardown for it finished". + * 2. `start()`'s `finally` block, unconditionally, as a backstop for + * every other way `runMainLoop()` can end — a thrown exception, for + * instance, never goes through `requestExit()` at all. Firing here + * too is what guarantees the hook always runs by the time this + * launcher's promise settles, not just on the two deliberate exit + * paths. + * + * Must stay synchronous and idempotent — it can run twice per exit (once + * from each call site above) and must never assume `handler`/`cleanup()` + * have run yet. + */ + protected onLeavingRemote(): void {} + protected async requestExit( reason: RemoteLauncherExitReason, handler: () => void | Promise ): Promise { + this.onLeavingRemote(); if (!this.exitReason) { this.exitReason = reason; } @@ -121,6 +152,9 @@ export abstract class RemoteLauncherBase { try { await this.runMainLoop(); } finally { + // Backstop call — see onLeavingRemote()'s doc comment for why + // this needs to run here too, not just from requestExit(). + this.onLeavingRemote(); await this.cleanup(); this.finalizeTerminal(); } diff --git a/cli/src/opencode/loop.test.ts b/cli/src/opencode/loop.test.ts new file mode 100644 index 00000000..b2bd8a24 --- /dev/null +++ b/cli/src/opencode/loop.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, it, vi } from 'vitest'; + +const harness = vi.hoisted(() => ({ + runLocalRemoteArgs: [] as Array>, + localCalls: [] as Array<{ opts: unknown }>, + remoteCalls: [] as Array<{ opts: unknown }> +})); + +vi.mock('@/agent/loopBase', () => ({ + runLocalRemoteSession: vi.fn(async (opts: Record) => { + harness.runLocalRemoteArgs.push(opts); + }) +})); + +vi.mock('./opencodeLocalLauncher', () => ({ + opencodeLocalLauncher: vi.fn(async (_instance: unknown, opts: unknown) => { + harness.localCalls.push({ opts }); + return 'exit'; + }) +})); + +vi.mock('./opencodeRemoteLauncher', () => ({ + opencodeRemoteLauncher: vi.fn(async (_instance: unknown, opts: unknown) => { + harness.remoteCalls.push({ opts }); + return 'exit'; + }) +})); + +// loop.ts constructs a real OpencodeSession internally (not injectable) — +// mock it so this test exercises only opencodeLoop's own glue logic (option +// forwarding + the compact-availability reset below), not the full +// AgentSessionBase construction contract. +vi.mock('./session', () => ({ + OpencodeSession: vi.fn().mockImplementation(function (this: { onSessionFound: () => void }) { + this.onSessionFound = vi.fn(); + }) +})); + +vi.mock('@/ui/logger', () => ({ + logger: { + debug: vi.fn(), + getLogPath: () => '/tmp/hapi-loop-test.log' + } +})); + +import { opencodeLoop } from './loop'; + +function baseOpts(overrides: Record = {}) { + return { + path: '/tmp/hapi-loop-test', + messageQueue: {} as never, + session: { rpcHandlerManager: {} } as never, + api: {} as never, + onModeChange: vi.fn(), + hookServer: { port: 1234, stop: vi.fn() } as never, + hookUrl: 'http://127.0.0.1:1234/hook/opencode', + ...overrides + }; +} + +describe('opencodeLoop compact availability wiring', () => { + // Resetting availability to false used to be loop.ts's job, done here in + // runLocal right before every local-mode entry. That left a window + // between "a switch/exit was requested" and "runLocal actually ran" + // where availability was still stale-true — a PR-review round found a + // /compact slash command arriving in that window could still queue and + // (via local mode bouncing straight back to remote to drain a non-empty + // queue) end up running despite the user having already asked to leave + // remote mode. The reset now happens as early as possible on the + // *leaving-remote* side instead (OpencodeRemoteLauncher's + // onLeavingRemote() override, called from RemoteLauncherBase's + // requestExit()/start()) — see opencodeRemoteLauncher.test.ts's + // "flips /compact availability to false synchronously..." test for that + // half of the contract. runLocal here must NOT also reset it: by the + // time runLocal ever runs, the prior remote launcher's promise (and + // therefore its onLeavingRemote() call) has already resolved. + it('does not call onCompactAvailabilityChange from runLocal — availability is already false by the time runLocal runs, reset earlier by the remote launcher leaving', async () => { + const events: boolean[] = []; + + await opencodeLoop(baseOpts({ + startingMode: 'local', + onCompactAvailabilityChange: (available: boolean) => events.push(available) + }) as Parameters[0]); + + const opts = harness.runLocalRemoteArgs[0] as { runLocal: (instance: unknown) => Promise }; + expect(opts.runLocal).toBeDefined(); + + await opts.runLocal({}); + + expect(events).toEqual([]); + expect(harness.localCalls.length).toBe(1); + }); + + it('forwards onCompactAvailabilityChange unchanged to the remote launcher', async () => { + const onCompactAvailabilityChange = vi.fn(); + + await opencodeLoop(baseOpts({ + startingMode: 'remote', + onCompactAvailabilityChange + }) as Parameters[0]); + + const opts = harness.runLocalRemoteArgs.at(-1) as { runRemote: (instance: unknown) => Promise }; + await opts.runRemote({}); + + expect(harness.remoteCalls.length).toBe(1); + const remoteOpts = harness.remoteCalls[0]?.opts as { onCompactAvailabilityChange?: unknown }; + expect(remoteOpts.onCompactAvailabilityChange).toBe(onCompactAvailabilityChange); + }); +}); diff --git a/cli/src/opencode/loop.ts b/cli/src/opencode/loop.ts index a9c5d1e9..71b6e0ce 100644 --- a/cli/src/opencode/loop.ts +++ b/cli/src/opencode/loop.ts @@ -24,6 +24,13 @@ interface OpencodeLoopOptions { hookUrl: string; onSessionReady?: (session: OpencodeSession) => void; onReasoningEffortRollback?: (effort: string | null) => void; + onCompactAvailabilityChange?: (available: boolean) => void; + // Consumes (delete-and-return) whether the given localId was cancelled + // after already being dequeued — needed because a queued /compact can + // still be running (its REST call can take minutes) by the time a + // cancel arrives, well past the point `messageQueue.cancelByLocalId` + // can do anything about it. + isLocalIdCancelled?: (localId: string) => boolean; } export async function opencodeLoop(opts: OpencodeLoopOptions): Promise { @@ -54,12 +61,26 @@ export async function opencodeLoop(opts: OpencodeLoopOptions): Promise { session, startingMode: opts.startingMode, logTag: 'opencode-loop', + // /compact only exists in remote mode (it needs the ACP backend + + // internal HTTP baseUrl that only opencodeRemoteLauncher owns). + // Availability is reset to false as part of *leaving* remote mode, + // not on *entering* local mode — see OpencodeRemoteLauncher's + // onLeavingRemote() override — so it's already false by the time + // runLocal below ever runs; no reset needed here. That decoupling is + // deliberate: resetting on local-entry left a window between "a + // switch/exit was requested" and "the next runLocal() call actually + // happened" where availability was still stale-true, which a + // PR-review round found could let a /compact queued in that window + // run anyway once local mode bounced straight back to remote to + // drain a non-empty queue. runLocal: (instance) => opencodeLocalLauncher(instance, { hookServer: opts.hookServer, hookUrl: opts.hookUrl }), runRemote: (instance) => opencodeRemoteLauncher(instance, { - onReasoningEffortRollback: opts.onReasoningEffortRollback + onReasoningEffortRollback: opts.onReasoningEffortRollback, + onCompactAvailabilityChange: opts.onCompactAvailabilityChange, + isLocalIdCancelled: opts.isLocalIdCancelled }), onSessionReady: opts.onSessionReady }); diff --git a/cli/src/opencode/opencodeRemoteLauncher.test.ts b/cli/src/opencode/opencodeRemoteLauncher.test.ts index 81f16ee7..03284b18 100644 --- a/cli/src/opencode/opencodeRemoteLauncher.test.ts +++ b/cli/src/opencode/opencodeRemoteLauncher.test.ts @@ -12,13 +12,53 @@ const harness = vi.hoisted(() => ({ events: [] as string[], setModelImpl: null as null | ((sessionId: string, modelId: string) => Promise), setConfigOptionImpl: null as null | ((sessionId: string, configId: string, value: string) => Promise), - thoughtLevelOption: null as null | { id: string; currentValue?: string; options: Array<{ value: string; name?: string }> } + thoughtLevelOption: null as null | { id: string; currentValue?: string; options: Array<{ value: string; name?: string }> }, + // Lets a test take full manual control of when a given prompt() call + // resolves, instead of the fixed-one-tick setImmediate delay below — + // needed to deterministically test ordering against /compact without + // guessing tick counts. + promptImpl: null as null | (() => Promise), + sessionModelsMetadata: undefined as undefined | { currentModelId: string; availableModels: unknown[] }, + // Lets a test hold handleAbort()'s cancelPrompt() call pending, so it + // can assert something happened *before* handleAbort()'s async teardown + // finished rather than merely by the time it eventually settles. + cancelPromptImpl: null as null | (() => Promise), + // Lets a test hold backend.newSession() pending, to simulate a + // terminal switch-to-local/exit landing during session initialization + // — before RPC 'abort'/'switch' handlers even exist (they're only + // registered once initialization finishes), so that race can only be + // reproduced via the terminal UI's onExit/onSwitchToLocal callbacks, + // not rpcHandlers. + newSessionImpl: null as null | (() => Promise) +})); + +// Captures the RemoteLauncherDisplayContext (including onExit/ +// onSwitchToLocal) that RemoteLauncherBase.setupTerminal() passes to +// OpencodeDisplay via ink's render() — but only when `hasTTY` is true, since +// setupTerminal() gates the real render() call on it. None of the other +// tests in this file force isTTY, so this mock is inert for them (render() +// is simply never called) and this hoisted state stays untouched. +const inkHarness = vi.hoisted(() => ({ + lastRenderProps: null as null | { onExit?: () => void | Promise; onSwitchToLocal?: () => void | Promise } +})); + +vi.mock('ink', () => ({ + render: vi.fn((element: { props?: { onExit?: () => void | Promise; onSwitchToLocal?: () => void | Promise } }) => { + inkHarness.lastRenderProps = element.props ?? null; + return { unmount: () => {} }; + }) })); vi.mock('./utils/opencodeBackend', () => ({ + allocateFreePort: vi.fn(async () => 48273), createOpencodeBackend: vi.fn(() => ({ initialize: vi.fn(async () => {}), - newSession: vi.fn(async () => 'acp-session-1'), + newSession: vi.fn(async () => { + if (harness.newSessionImpl) { + return harness.newSessionImpl(); + } + return 'acp-session-1'; + }), loadSession: vi.fn(async () => 'acp-session-1'), setModel: vi.fn(async (sessionId: string, modelId: string, opts?: { flavor?: string }) => { harness.events.push(`setModel:${modelId}`); @@ -26,6 +66,14 @@ vi.mock('./utils/opencodeBackend', () => ({ if (harness.setModelImpl) { await harness.setModelImpl(sessionId, modelId); } + // Mirror AcpSdkBackend's optimistic currentModelId update for the + // opencode flavor (see updateCurrentModelOptimistic) so a + // subsequent getSessionModelsMetadata() call in the same test + // reflects the switch — needed to verify /compact runs under the + // model a batch just switched to, not a stale cached one. + if (harness.sessionModelsMetadata) { + harness.sessionModelsMetadata = { ...harness.sessionModelsMetadata, currentModelId: modelId }; + } }), setConfigOption: vi.fn(async (sessionId: string, configId: string, value: string) => { harness.events.push(`setConfigOption:${value}`); @@ -41,10 +89,18 @@ vi.mock('./utils/opencodeBackend', () => ({ harness.promptContents.push(content); harness.events.push('prompt:start'); harness.promptCount++; - await new Promise((resolve) => setImmediate(resolve)); + if (harness.promptImpl) { + await harness.promptImpl(); + } else { + await new Promise((resolve) => setImmediate(resolve)); + } harness.events.push('prompt:end'); }), - cancelPrompt: vi.fn(async () => {}), + cancelPrompt: vi.fn(async () => { + if (harness.cancelPromptImpl) { + await harness.cancelPromptImpl(); + } + }), respondToPermission: vi.fn(async () => {}), onStderrError: vi.fn(), setSessionInfoUpdateListener: vi.fn(), @@ -53,8 +109,13 @@ vi.mock('./utils/opencodeBackend', () => ({ }), onPermissionRequest: vi.fn(), disconnect: vi.fn(async () => {}), - getSessionModelsMetadata: vi.fn(() => undefined), - getThoughtLevelConfigOption: vi.fn(() => harness.thoughtLevelOption ?? undefined) + getSessionModelsMetadata: vi.fn(() => harness.sessionModelsMetadata), + getThoughtLevelConfigOption: vi.fn(() => harness.thoughtLevelOption ?? undefined), + // Real AcpSdkBackend.suppressUpdatesDuring swaps out the message + // handler around `fn`; that detail is irrelevant to these + // launcher-level tests (which never assert on ACP session/update + // forwarding), so the stub is a transparent pass-through. + suppressUpdatesDuring: vi.fn(async (fn: () => Promise): Promise => fn()) })) })); @@ -78,6 +139,44 @@ vi.mock('@/ui/ink/OpencodeDisplay', () => ({ OpencodeDisplay: () => null })); +const compactHarness = vi.hoisted(() => ({ + calls: [] as Array<{ baseUrl: string; sessionId: string; providerId: string; modelId: string; signal?: AbortSignal }>, + result: { ok: true } as { ok: true } | { ok: false; error: string }, + summaryCalls: [] as Array<{ baseUrl: string; sessionId: string; signal?: AbortSignal }>, + summaryResult: { found: false } as { found: true; text: string } | { found: false }, + // Lets a test simulate a REST call that only settles once its signal is + // aborted (mirroring how a real fetch() behaves under AbortSignal) — + // needed to test that handleAbort() actually unblocks an in-flight + // /compact instead of the default immediate-resolve behavior below. + triggerImpl: null as null | ((opts: { baseUrl: string; sessionId: string; providerId: string; modelId: string; signal?: AbortSignal }) => Promise<{ ok: true } | { ok: false; error: string }>), + // Same idea, for the GET that runs right after a successful POST — this + // is what a PR-review round found was missing a signal entirely. + summaryImpl: null as null | ((opts: { baseUrl: string; sessionId: string; signal?: AbortSignal }) => Promise<{ found: true; text: string } | { found: false }>) +})); + +vi.mock('./utils/opencodeCompactBridge', () => ({ + splitProviderModel: (combined: string | null | undefined) => { + if (!combined) return null; + const idx = combined.indexOf('/'); + if (idx <= 0 || idx === combined.length - 1) return null; + return { providerId: combined.slice(0, idx), modelId: combined.slice(idx + 1) }; + }, + triggerOpencodeCompact: vi.fn(async (opts: { baseUrl: string; sessionId: string; providerId: string; modelId: string; signal?: AbortSignal }) => { + compactHarness.calls.push(opts); + if (compactHarness.triggerImpl) { + return compactHarness.triggerImpl(opts); + } + return compactHarness.result; + }), + fetchCompactionSummary: vi.fn(async (opts: { baseUrl: string; sessionId: string; signal?: AbortSignal }) => { + compactHarness.summaryCalls.push(opts); + if (compactHarness.summaryImpl) { + return compactHarness.summaryImpl(opts); + } + return compactHarness.summaryResult; + }) +})); + vi.mock('@/ui/logger', () => ({ logger: { debug: vi.fn(), @@ -86,7 +185,7 @@ vi.mock('@/ui/logger', () => ({ } })); -import { opencodeRemoteLauncher } from './opencodeRemoteLauncher'; +import { opencodeRemoteLauncher, selectAbortStatusMessage } from './opencodeRemoteLauncher'; function createMode(model?: string): OpencodeMode { return { @@ -117,21 +216,32 @@ function createResetMode(): OpencodeMode { }; } -function createSessionStub(items: Array<{ message: string; mode: OpencodeMode }>) { +function createSessionStub( + items: Array<{ message: string; mode: OpencodeMode; localId?: string }>, + opts: { keepOpen?: boolean } = {} +) { const queue = new MessageQueue2((mode) => JSON.stringify(mode)); - items.forEach(({ message, mode }, index) => { + items.forEach(({ message, mode, localId }, index) => { if (index === 0 && items.length > 1) { - queue.pushIsolateAndClear(message, mode); + queue.pushIsolateAndClear(message, mode, localId); } else { - queue.push(message, mode); + queue.push(message, mode, localId); } }); - queue.close(); + // A test simulating a message arriving mid-run (e.g. /compact reaching + // the queue while an earlier item is still executing) needs to push to + // this queue after createSessionStub returns, so it can't be closed yet. + if (!opts.keepOpen) { + queue.close(); + } const sessionEvents: Array<{ type: string; [key: string]: unknown }> = []; + const sentAgentMessages: unknown[] = []; const rpcHandlers = new Map unknown>(); const setModelReasoningEffort = vi.fn(); const pushKeepAlive = vi.fn(); + const emitMessagesConsumedCalls: Array<{ localIds: string[]; options?: { clearQueuedThinkingGrace?: boolean } }> = []; + const thinkingChangeCalls: boolean[] = []; const client = { rpcHandlerManager: { @@ -144,6 +254,9 @@ function createSessionStub(items: Array<{ message: string; mode: OpencodeMode }> sendUserMessage(_text: string) {}, sendSessionEvent(event: { type: string; [key: string]: unknown }) { sessionEvents.push(event); + }, + emitMessagesConsumed(localIds: string[], options?: { clearQueuedThinkingGrace?: boolean }) { + emitMessagesConsumedCalls.push({ localIds, options }); } }; @@ -162,18 +275,29 @@ function createSessionStub(items: Array<{ message: string; mode: OpencodeMode }> pushKeepAlive, onThinkingChange(thinking: boolean) { session.thinking = thinking; + thinkingChangeCalls.push(thinking); }, onSessionFound(id: string) { session.sessionId = id; }, - sendAgentMessage(_message: unknown) {}, + sendAgentMessage(message: unknown) { + sentAgentMessages.push(message); + }, sendSessionEvent(event: { type: string; [key: string]: unknown }) { client.sendSessionEvent(event); }, sendUserMessage(_text: string) {} }; - return { session, sessionEvents, rpcHandlers, setModelReasoningEffort, pushKeepAlive }; + return { session, sessionEvents, sentAgentMessages, rpcHandlers, setModelReasoningEffort, pushKeepAlive, emitMessagesConsumedCalls, thinkingChangeCalls }; +} + +function createCompactMode(model?: string): OpencodeMode { + return { + permissionMode: 'default' as PermissionMode, + model, + operation: 'compact' + }; } describe('opencodeRemoteLauncher inline model switch', () => { @@ -188,6 +312,1013 @@ describe('opencodeRemoteLauncher inline model switch', () => { harness.setModelImpl = null; harness.setConfigOptionImpl = null; harness.thoughtLevelOption = null; + compactHarness.calls = []; + compactHarness.result = { ok: true }; + compactHarness.summaryCalls = []; + compactHarness.summaryResult = { found: false }; + compactHarness.triggerImpl = null; + compactHarness.summaryImpl = null; + harness.promptImpl = null; + harness.sessionModelsMetadata = undefined; + harness.cancelPromptImpl = null; + harness.newSessionImpl = null; + inkHarness.lastRenderProps = null; + }); + + it('processes a queued /compact operation only after an earlier queued prompt has finished', async () => { + let resolvePrompt: (() => void) | null = null; + harness.promptImpl = () => new Promise((resolve) => { + resolvePrompt = resolve; + }); + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + + // The compact item is queued right behind the prompt from the start + // (both pre-populated via createSessionStub) — this is the exact + // "message A generating, message B (compact) already queued" race a + // prior design got wrong by running /compact through an + // externally-invoked trigger instead of this same queue. + const { session } = createSessionStub([ + { message: 'first', mode: createMode('ollama/x') }, + { message: '', mode: createCompactMode('ollama/x') } + ]); + + const launcherPromise = opencodeRemoteLauncher(session as never, { + onCompactAvailabilityChange: () => {} + }); + + // Deterministically wait until the prompt is confirmed in-flight + // (it will not resolve until we call resolvePrompt below). + while (!harness.events.includes('prompt:start')) { + await new Promise((resolve) => setImmediate(resolve)); + } + + // Give the loop several ticks to (incorrectly) run the already-queued + // compact item ahead of the still-running prompt, if the fix weren't + // in place. + for (let i = 0; i < 5; i++) { + await new Promise((resolve) => setImmediate(resolve)); + } + expect(compactHarness.calls).toEqual([]); + expect(harness.events).toEqual(['prompt:start']); + + resolvePrompt!(); + await launcherPromise; + + expect(harness.events).toEqual(['prompt:start', 'prompt:end']); + expect(compactHarness.calls.length).toBe(1); + }); + + it('processes a queued prompt only after an earlier queued /compact operation has finished', async () => { + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + + const { triggerOpencodeCompact } = await import('./utils/opencodeCompactBridge'); + const triggerMock = triggerOpencodeCompact as unknown as ReturnType; + let resolveCompact: (() => void) | null = null; + triggerMock.mockImplementationOnce((opts: { baseUrl: string; sessionId: string; providerId: string; modelId: string }) => { + compactHarness.calls.push(opts); + return new Promise((resolve) => { + resolveCompact = () => resolve({ ok: true }); + }); + }); + + // Compact is queued first this time, with a prompt right behind it. + const { session } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/x') }, + { message: 'second', mode: createMode('ollama/x') } + ]); + + const launcherPromise = opencodeRemoteLauncher(session as never, { + onCompactAvailabilityChange: () => {} + }); + + // Give the main loop plenty of ticks to (incorrectly) start the + // queued prompt while compact is still in flight. + for (let i = 0; i < 10; i++) { + await new Promise((resolve) => setImmediate(resolve)); + } + expect(compactHarness.calls.length).toBe(1); + expect(harness.promptCount).toBe(0); + + resolveCompact!(); + await launcherPromise; + + expect(harness.promptCount).toBe(1); + expect(harness.events).toEqual(['prompt:start', 'prompt:end']); + }); + + it('runs the exact 3-stage scenario reported by HAPI Bot: prompt A generating, prompt B already queued, /compact arrives after — final order is A, B, compact', async () => { + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + const resolvers: Array<() => void> = []; + harness.promptImpl = () => new Promise((resolve) => { + resolvers.push(resolve); + }); + + // Prompt A and prompt B are both already queued up front. Keep the + // queue open so /compact can be pushed onto it mid-run, exactly like + // runOpencode.ts's messageQueue.pushIsolated(...) call would while A + // is still generating. + const { session } = createSessionStub([ + { message: 'A', mode: createMode('ollama/x') }, + { message: 'B', mode: createMode('ollama/x') } + ], { keepOpen: true }); + + const launcherPromise = opencodeRemoteLauncher(session as never, { + onCompactAvailabilityChange: () => {} + }); + + // Wait until prompt A is confirmed in-flight. + while (!harness.events.includes('prompt:start')) { + await new Promise((resolve) => setImmediate(resolve)); + } + expect(harness.promptContents).toEqual([[{ type: 'text', text: expect.stringContaining('A') }]]); + + // /compact arrives now — after B was already queued, while A is + // still generating. + session.queue.pushIsolated('', { ...createMode('ollama/x'), operation: 'compact' }); + session.queue.close(); + + // Resolve A; B must run to completion before compact fires, even + // though /compact arrived before B had a chance to be dequeued. + resolvers[0]!(); + while (harness.promptCount < 2) { + await new Promise((resolve) => setImmediate(resolve)); + } + expect(compactHarness.calls).toEqual([]); + + resolvers[1]!(); + await launcherPromise; + + expect(harness.promptContents).toEqual([ + [{ type: 'text', text: expect.stringContaining('A') }], + [{ type: 'text', text: expect.stringContaining('B') }] + ]); + expect(compactHarness.calls.length).toBe(1); + expect(harness.events).toEqual(['prompt:start', 'prompt:end', 'prompt:start', 'prompt:end']); + }); + + it('cancelling a /compact operation while it is still queued behind a running prompt keeps the REST bridge from ever being called', async () => { + // Reproduces the exact scenario a PR reviewer bot reported: prompt A + // is already generating, /compact is queued behind it (not yet + // dequeued), and the user cancels /compact before A finishes. + // + // Note on what this test does and doesn't prove: `queue.cancelByLocalId` + // removing a still-queued item and the dequeue loop never reaching a + // removed item both already worked at this (launcher + MessageQueue2) + // level before the runOpencode.ts fix below — this test would pass + // either way, since it drives session.queue directly and never goes + // through runOpencode.ts's onUserMessage/onCancelQueuedMessage + // handlers. What actually changed with the fix — runOpencode.ts no + // longer calling session.emitMessagesConsumed([localId]) synchronously + // the instant /compact is queued, a leftover from when /compact ran + // via a trigger function outside the queue entirely — is that the hub + // would otherwise mark the message "invoked" before it was ever + // dequeued and never ask the CLI to cancel it at all, so the cancel + // request this test simulates (queue.cancelByLocalId) would never + // have been *made* in the first place. That RED/GREEN is covered in + // runOpencode.test.ts ("queues a /compact request..." — asserts + // emitMessagesConsumed is not called at queue time). This test locks + // in the launcher-side half of the contract that fix depends on: once + // a cancel *does* reach the CLI for a still-queued /compact behind a + // running prompt, the bridge must never be called. + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + const resolvers: Array<() => void> = []; + harness.promptImpl = () => new Promise((resolve) => { + resolvers.push(resolve); + }); + + const { session } = createSessionStub([ + { message: 'A', mode: createMode('ollama/x') } + ], { keepOpen: true }); + + const launcherPromise = opencodeRemoteLauncher(session as never, { + onCompactAvailabilityChange: () => {} + }); + + // Wait until prompt A is confirmed in-flight. + while (!harness.events.includes('prompt:start')) { + await new Promise((resolve) => setImmediate(resolve)); + } + + // /compact is queued behind A while A is still generating — mirrors + // runOpencode.ts's messageQueue.pushIsolated(...) call for a + // /compact slash command. + session.queue.pushIsolated('', { ...createMode('ollama/x'), operation: 'compact' }, 'local-compact'); + + // The user cancels /compact before A finishes. It's still sitting + // in the queue (never dequeued), so this must remove it cleanly — + // the same call runOpencode.ts's onCancelQueuedMessage makes for any + // other still-queued item. + expect(session.queue.cancelByLocalId('local-compact')).toBe(true); + session.queue.close(); + + resolvers[0]!(); + await launcherPromise; + + expect(compactHarness.calls).toEqual([]); + expect(harness.events).toEqual(['prompt:start', 'prompt:end']); + }); + + it('a queued /compact operation posts to the REST bridge using the session baseUrl and current model, and reports started/completed', async () => { + const opencodeBackendModule = await import('./utils/opencodeBackend'); + const factory = (opencodeBackendModule as unknown as { createOpencodeBackend: ReturnType }).createOpencodeBackend; + factory.mockImplementationOnce(() => ({ + initialize: vi.fn(async () => {}), + newSession: vi.fn(async () => 'acp-session-1'), + loadSession: vi.fn(async () => 'acp-session-1'), + setModel: vi.fn(async () => {}), + prompt: vi.fn(async () => {}), + cancelPrompt: vi.fn(async () => {}), + respondToPermission: vi.fn(async () => {}), + onStderrError: vi.fn(), + setSessionInfoUpdateListener: vi.fn(), + refreshSessionInfo: vi.fn(async () => {}), + onPermissionRequest: vi.fn(), + disconnect: vi.fn(async () => {}), + getSessionModelsMetadata: vi.fn(() => ({ + currentModelId: 'ollama/qwen3.6:35b-a3b-q8_0-mtp', + availableModels: [] + })), + suppressUpdatesDuring: vi.fn(async (fn: () => Promise): Promise => fn()) + })); + + const { session, sessionEvents } = createSessionStub([ + { message: '', mode: createCompactMode() } + ]); + + await opencodeRemoteLauncher(session as never); + + expect(compactHarness.calls).toEqual([ + { + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'acp-session-1', + providerId: 'ollama', + modelId: 'qwen3.6:35b-a3b-q8_0-mtp', + signal: expect.any(AbortSignal) + } + ]); + const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); + expect(messages).toEqual(['📦 Compaction started', '📦 Compaction completed']); + + // The REST bridge call must run inside suppressUpdatesDuring so any + // session/update notifications OpenCode streams while it's in + // flight don't leak into the previous turn's onUpdate and render as + // a duplicate assistant message (see AcpSdkBackend.suppressUpdatesDuring). + const backendInstance = factory.mock.results[0]?.value as { suppressUpdatesDuring: ReturnType }; + expect(backendInstance.suppressUpdatesDuring).toHaveBeenCalledTimes(1); + }); + + it('switch-to-local (which reuses handleAbort()) interrupts an in-flight /compact REST call instead of blocking on it until it settles on its own', async () => { + // Reproduces the exact bug a PR reviewer bot reported: triggerOpencodeCompact + // is awaited with no way to interrupt it, so Stop/switch-to-local had + // to wait out the REST call (which is deliberately unbounded — see + // its doc comment) before the launcher could do anything else. Here + // the mock REST call only ever settles if its AbortSignal fires, + // exactly like a real fetch() under AbortSignal — so if handleAbort() + // (invoked here via the 'switch' RPC, which routes through it before + // exiting remote mode) doesn't actually abort it, this test times out. + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + let capturedSignal: AbortSignal | undefined; + // Mirrors the real triggerOpencodeCompact's contract (never rejects + // — an aborted fetch() is caught internally and turned into a + // structured `{ ok: false }`), just driven by a signal instead of a + // real network call. + compactHarness.triggerImpl = (opts) => new Promise((resolve) => { + capturedSignal = opts.signal; + opts.signal?.addEventListener('abort', () => { + resolve({ ok: false, error: 'The operation was aborted.' }); + }); + }); + + const { session, sessionEvents, rpcHandlers } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/x') } + ]); + + const launcherPromise = opencodeRemoteLauncher(session as never, { + onCompactAvailabilityChange: () => {} + }); + + // Wait until the compact REST call is actually in flight. + while (compactHarness.calls.length === 0) { + await new Promise((resolve) => setImmediate(resolve)); + } + expect(capturedSignal?.aborted).toBe(false); + + const switchHandler = rpcHandlers.get('switch') as (() => Promise) | undefined; + expect(switchHandler).toBeDefined(); + + // Racing against a short timeout is the actual assertion: without + // the fix, this promise (and therefore the whole launcher) never + // settles, since the mock REST call above only resolves on abort. + await Promise.race([ + switchHandler!(), + new Promise((_, reject) => setTimeout(() => reject(new Error('switch handler (handleAbort) did not return in time')), 2000)) + ]); + expect(capturedSignal?.aborted).toBe(true); + + // The interrupted operation must not surface a stale result. + const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); + expect(messages).toEqual(['📦 Compaction started']); + + // The launcher must actually be able to leave remote mode — 'switch' + // sets shouldExit before calling handleAbort(), so once that + // interruption unblocks runCompactOperation(), the main loop should + // exit on its own without any further input. + await Promise.race([ + launcherPromise, + new Promise((_, reject) => setTimeout(() => reject(new Error('launcher did not exit remote mode in time')), 2000)) + ]); + }); + + it('switch-to-local also interrupts an in-flight fetchCompactionSummary GET (not just the triggerOpencodeCompact POST)', async () => { + // Reproduces a second PR-review round's finding: the fix above only + // wired the abort signal through triggerOpencodeCompact (the POST). + // fetchCompactionSummary (the GET runCompactOperation() calls right + // after a successful POST) still had no way to be interrupted, so + // Stop/switch-to-local could still block for as long as *that* call + // took even after the POST-side fix landed. Here the POST resolves + // immediately (ok:true) and the GET is the one that only settles on + // abort. + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + let capturedSignal: AbortSignal | undefined; + compactHarness.summaryImpl = (opts) => new Promise((resolve) => { + capturedSignal = opts.signal; + opts.signal?.addEventListener('abort', () => { + resolve({ found: false }); + }); + }); + + const { session, sessionEvents, rpcHandlers } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/x') } + ]); + + const launcherPromise = opencodeRemoteLauncher(session as never, { + onCompactAvailabilityChange: () => {} + }); + + // Wait until the summary GET is actually in flight (i.e. the POST + // already resolved successfully). + while (compactHarness.summaryCalls.length === 0) { + await new Promise((resolve) => setImmediate(resolve)); + } + expect(capturedSignal?.aborted).toBe(false); + + const switchHandler = rpcHandlers.get('switch') as (() => Promise) | undefined; + expect(switchHandler).toBeDefined(); + + await Promise.race([ + switchHandler!(), + new Promise((_, reject) => setTimeout(() => reject(new Error('switch handler (handleAbort) did not return in time')), 2000)) + ]); + expect(capturedSignal?.aborted).toBe(true); + + // No stale "Compaction completed" for an interrupted GET. + const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); + expect(messages).toEqual(['📦 Compaction started']); + + await Promise.race([ + launcherPromise, + new Promise((_, reject) => setTimeout(() => reject(new Error('launcher did not exit remote mode in time')), 2000)) + ]); + }); + + it('flips /compact availability to false synchronously the instant switch-to-local begins, before handleAbort()\'s async teardown (e.g. cancelPrompt) finishes', async () => { + // Reproduces a fourth PR-review round's finding: availability used + // to only reset on the *next* local-mode entry (loop.ts's + // `runLocal:` callback), leaving a window between "switch was + // requested" and "local mode actually started running" where + // availability was still stale-true. A /compact slash command + // arriving in that window would still queue normally + // (runOpencode.ts's `compactSupported` flag hadn't flipped yet) — + // and since local mode immediately hands back to remote when it + // finds a non-empty queue, that queued compact could end up running + // anyway despite the user having already asked to leave remote mode. + // + // cancelPrompt() is held pending here specifically so the assertion + // below happens *during* handleAbort()'s async teardown, not merely + // by the time the whole thing eventually settles — proving + // availability flips at the earliest possible synchronous point + // (requestExit()'s onLeavingRemote() call), not somewhere later in + // the same unwind. + let resolveCancelPrompt: (() => void) | null = null; + harness.cancelPromptImpl = () => new Promise((resolve) => { + resolveCancelPrompt = resolve; + }); + + const availabilityEvents: boolean[] = []; + const { session, rpcHandlers } = createSessionStub([], { keepOpen: true }); + + const launcherPromise = opencodeRemoteLauncher(session as never, { + onCompactAvailabilityChange: (available) => availabilityEvents.push(available) + }); + + // Wait until remote signals /compact is available (backend ready, + // dequeue loop now idle waiting on the empty queue). + while (!availabilityEvents.includes(true)) { + await new Promise((resolve) => setImmediate(resolve)); + } + expect(availabilityEvents).toEqual([true]); + + const switchHandler = rpcHandlers.get('switch') as (() => Promise) | undefined; + expect(switchHandler).toBeDefined(); + + const switchPromise = switchHandler!(); + + // Let the synchronous prefix of the switch/requestExit/handleAbort + // call chain run, then check availability *before* releasing the + // held cancelPrompt() — i.e. before handleAbort() can possibly have + // finished. + await new Promise((resolve) => setImmediate(resolve)); + expect(resolveCancelPrompt).not.toBeNull(); + expect(availabilityEvents).toEqual([true, false]); + + resolveCancelPrompt!(); + session.queue.close(); + await Promise.race([ + switchPromise, + new Promise((_, reject) => setTimeout(() => reject(new Error('switch handler did not settle in time')), 2000)) + ]); + await Promise.race([ + launcherPromise, + new Promise((_, reject) => setTimeout(() => reject(new Error('launcher did not exit remote mode in time')), 2000)) + ]); + }); + + it('never emits a trailing /compact availability(true) if a terminal switch-to-local/exit already ran while session initialization (newSession) was still pending', async () => { + // A 9th PR-review round found the mirror-image bug to the test + // above: onCompactAvailabilityChange(true) (right after + // newSession/loadSession resolves) fires unconditionally — with no + // way to know a switch/exit already happened *during* that pending + // ACP round trip. That race can only be reached via the terminal + // UI's onExit/onSwitchToLocal callbacks (wired up by + // setupTerminal() before runMainLoop() even starts) — the RPC + // 'abort'/'switch' handlers below don't exist yet at this point in + // the sequence (setupAbortHandlers() only runs after + // newSession/loadSession resolve), so they can't be used to + // reproduce this specific window. + // + // requestExit() sets `this.shouldExit = true` synchronously, before + // awaiting its handler (see RemoteLauncherBase.requestExit) — so by + // the time the pending newSession() resolves and this code reaches + // `onCompactAvailabilityChange?.(true)`, `this.shouldExit` already + // reflects the switch/exit that happened in between. The bug: that + // line used to fire regardless, resurrecting availability (and + // transitively runOpencode.ts's compactSupported) even though the + // session is on its way out — see that gate's compactTeardownInProgress + // comment for why compactSupported flipping true makes it get + // ignored entirely. + let resolveNewSession: ((id: string) => void) | null = null; + harness.newSessionImpl = () => new Promise((resolve) => { + resolveNewSession = resolve; + }); + + const originalStdoutIsTTY = process.stdout.isTTY; + const originalStdinIsTTY = process.stdin.isTTY; + Object.defineProperty(process.stdout, 'isTTY', { configurable: true, value: true }); + Object.defineProperty(process.stdin, 'isTTY', { configurable: true, value: true }); + const originalSetRawMode = (process.stdin as unknown as { setRawMode?: (mode: boolean) => void }).setRawMode; + const setRawModeStub = vi.fn(); + Object.defineProperty(process.stdin, 'setRawMode', { configurable: true, value: setRawModeStub }); + + try { + const availabilityEvents: boolean[] = []; + const { session } = createSessionStub([], { keepOpen: true }); + + const launcherPromise = opencodeRemoteLauncher(session as never, { + onCompactAvailabilityChange: (available) => availabilityEvents.push(available) + }); + + // setupTerminal() runs synchronously as the very first thing + // start() does, before runMainLoop() (and hence before the + // pending newSession()) gets a chance to run — so by the time + // control returns here, ink's render() (mocked above) has + // already captured onExit/onSwitchToLocal. + expect(inkHarness.lastRenderProps?.onSwitchToLocal).toBeDefined(); + expect(resolveNewSession).toBeNull(); + expect(availabilityEvents).toEqual([]); + + await inkHarness.lastRenderProps!.onSwitchToLocal!(); + // requestExit()'s onLeavingRemote() fires synchronously — but + // availability was never true yet, so this is the only event + // so far. + expect(availabilityEvents).toEqual([false]); + + // Now let the previously-pending newSession() resolve. + resolveNewSession!('acp-session-late'); + + for (let i = 0; i < 10; i++) { + await new Promise((resolve) => setImmediate(resolve)); + } + + // The fix: no trailing `true` ever gets appended once the + // previously-pending newSession() resolves. + expect(availabilityEvents).not.toContain(true); + + session.queue.close(); + await Promise.race([ + launcherPromise, + new Promise((_, reject) => setTimeout(() => reject(new Error('launcher did not exit remote mode in time')), 2000)) + ]); + + // Final check once the launcher has actually settled — still no + // `true` anywhere, regardless of how many times the (idempotent, + // by design — see onLeavingRemote's doc comment) backstop in + // start()'s finally re-fired `false` along the way. + expect(availabilityEvents).not.toContain(true); + expect(availabilityEvents.length).toBeGreaterThan(0); + expect(availabilityEvents.every((value) => value === false)).toBe(true); + } finally { + Object.defineProperty(process.stdout, 'isTTY', { configurable: true, value: originalStdoutIsTTY }); + Object.defineProperty(process.stdin, 'isTTY', { configurable: true, value: originalStdinIsTTY }); + if (originalSetRawMode) { + Object.defineProperty(process.stdin, 'setRawMode', { configurable: true, value: originalSetRawMode }); + } else { + delete (process.stdin as unknown as { setRawMode?: unknown }).setRawMode; + } + } + }); + + it('sends the fetched compaction summary as a reasoning-type agent message', async () => { + compactHarness.summaryResult = { found: true, text: '## Objective\n- Did the thing' }; + harness.sessionModelsMetadata = { currentModelId: 'ollama/qwen3.6:35b-a3b-q8_0-mtp', availableModels: [] }; + + const { session, sentAgentMessages } = createSessionStub([ + { message: '', mode: createCompactMode() } + ]); + + await opencodeRemoteLauncher(session as never); + + expect(compactHarness.summaryCalls).toEqual([ + { baseUrl: 'http://127.0.0.1:48273', sessionId: 'acp-session-1', signal: expect.any(AbortSignal) } + ]); + expect(sentAgentMessages).toEqual([ + { type: 'reasoning', message: '## Objective\n- Did the thing', id: expect.any(String) } + ]); + }); + + it('never starts the compact at all if isLocalIdCancelled already reports the item cancelled the moment it is dequeued', async () => { + // isLocalIdCancelled's backing Set (runOpencode.ts's + // cancelledBeforeEnqueue) can only ever be populated during the + // brief network round trip between the CLI emitting a queued + // /compact item's "invoked" ack and the hub recording it — never + // while the compact REST call is actually running (see that file's + // doc comment for the full mechanism). So by the time the dequeue + // loop gets here, a true result unconditionally means this compact + // was cancelled before its REST request was ever sent — an 8th + // PR-review round found the pre-start check round 7 added for + // compactResultSuppressed needed the same treatment here: skip + // starting the operation entirely rather than sending "📦 + // Compaction started" for a request that's about to be thrown away. + // + // A 10th PR-review round found this skip path also never told the + // hub the queued item was done — session.onThinkingChange(true) is + // never called here (that's the whole point of skipping), so + // without an explicit clearQueuedThinkingGrace ack + a final + // thinking=false keepalive, the web UI spinner could sit stuck for + // the hub's full 15s queued-thinking grace window. + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + const isLocalIdCancelled = vi.fn((id: string) => id === 'compact-1'); + + const { session, sessionEvents, sentAgentMessages, emitMessagesConsumedCalls, thinkingChangeCalls } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/x'), localId: 'compact-1' } + ]); + + await opencodeRemoteLauncher(session as never, { isLocalIdCancelled }); + + expect(isLocalIdCancelled).toHaveBeenCalledWith('compact-1'); + expect(compactHarness.calls).toEqual([]); + const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); + expect(messages).toEqual([]); + expect(sentAgentMessages).toEqual([]); + expect(emitMessagesConsumedCalls).toEqual([ + { localIds: ['compact-1'], options: { clearQueuedThinkingGrace: true } } + ]); + expect(thinkingChangeCalls).toEqual([false]); + }); + + it('never starts the compact (not even the REST bridge call itself) if isLocalIdCancelled already reports the item cancelled the moment it is dequeued, regardless of localId', async () => { + // Sibling of the test above using an unconditional isLocalIdCancelled + // (vs. one keyed to a specific id) — doesn't mock triggerOpencodeCompact + // at all, since the whole point is that it must never be called; doing + // so also avoids leaking a mockImplementationOnce() that would never + // get consumed (skip means it's never invoked) into a later test. + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + const isLocalIdCancelled = vi.fn(() => true); + + const { session, sessionEvents } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/x'), localId: 'compact-2' } + ]); + + await opencodeRemoteLauncher(session as never, { isLocalIdCancelled }); + + expect(compactHarness.calls).toEqual([]); + const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); + expect(messages).toEqual([]); + }); + + it('does not suppress the result when isLocalIdCancelled reports false', async () => { + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + const isLocalIdCancelled = vi.fn(() => false); + + const { session, sessionEvents } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/x'), localId: 'compact-3' } + ]); + + await opencodeRemoteLauncher(session as never, { isLocalIdCancelled }); + + const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); + expect(messages).toEqual(['📦 Compaction started', '📦 Compaction completed']); + }); + + it('a plain Stop during an in-flight compact does not unblock the dequeue loop until the operation actually settles server-side, and suppresses the eventual result', async () => { + // Reproduces the exact scenario a 6th PR-review round reported (and + // that an earlier round's fix — always aborting compactAbortController + // on any abort — was rejected for): Stop only interrupts the + // *client's* HTTP request. The OpenCode server can still be + // compacting the same session well after that, since session/update + // notifications are a separate channel from that HTTP request's + // lifecycle (see AcpSdkBackend.suppressUpdatesDuring's doc comment). + // If the dequeue loop moved on to the next queued prompt as soon as + // the client gave up, that prompt could run concurrently with a + // compaction still touching the same session — breaking the "compact + // and prompt never touch the session at once" invariant this + // feature's whole queue-based redesign depends on. This mock's + // triggerImpl only ever settles when the test explicitly resolves + // it (standing in for "the server is still working"), never when + // the client-side signal aborts — so if the fix regressed back to + // unconditionally aborting on plain Stop, this test would hang/time + // out rather than merely assert wrong. + // (handleAbort()'s existing session.queue.reset() call clears any + // still-queued items regardless of leavingRemote, so this + // deliberately doesn't rely on a prompt queued behind the compact + // surviving Stop — that's an orthogonal, pre-existing behavior. + // Instead it uses the dequeue loop's 'ready' session event — only + // ever sent from the loop's own finally block, once + // runCompactOperation() actually returns — as the direct signal that + // the loop advanced past this operation.) + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + let resolveServerSideCompaction: (() => void) | null = null; + let capturedSignal: AbortSignal | undefined; + compactHarness.triggerImpl = (opts) => { + capturedSignal = opts.signal; + return new Promise((resolve) => { + resolveServerSideCompaction = () => resolve({ ok: true }); + // Mirrors real triggerOpencodeCompact/fetch() semantics: an + // aborted signal settles the call too (as a failure) — this + // is what makes the test meaningfully distinguish "plain + // Stop leaves the signal alone" from "plain Stop aborts it", + // rather than both cases merely hanging identically. + opts.signal?.addEventListener('abort', () => resolve({ ok: false, error: 'aborted' })); + }); + }; + + const { session, sessionEvents, rpcHandlers } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/x') } + ]); + + const launcherPromise = opencodeRemoteLauncher(session as never, { + onCompactAvailabilityChange: () => {} + }); + + while (compactHarness.calls.length === 0) { + await new Promise((resolve) => setImmediate(resolve)); + } + + const abortHandler = rpcHandlers.get('abort') as (() => Promise) | undefined; + expect(abortHandler).toBeDefined(); + await abortHandler!(); + + // Plain Stop must NOT abort the client-side signal. + expect(capturedSignal?.aborted).toBe(false); + + // Several ticks pass — the loop must still be blocked inside + // runCompactOperation(): no 'ready' event yet, and `thinking` must + // stay true — nothing has actually stopped yet from the user's + // perspective, so flipping it false here (as handleAbort used to, + // unconditionally) would misleadingly suggest otherwise while the + // server keeps compacting for real. + for (let i = 0; i < 10; i++) { + await new Promise((resolve) => setImmediate(resolve)); + } + expect(sessionEvents.some((event) => event.type === 'ready')).toBe(false); + expect(session.thinking).toBe(true); + + // The server genuinely finishes now. + resolveServerSideCompaction!(); + + while (!sessionEvents.some((event) => event.type === 'ready')) { + await new Promise((resolve) => setImmediate(resolve)); + } + + // The result must be suppressed — no stale "Compaction completed" + // for an action the user already asked to abort. + const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); + expect(messages).toEqual(['📦 Compaction started']); + + session.queue.close(); + await launcherPromise; + }); + + it('does not look up a summary when the compact REST call itself failed', async () => { + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + const { triggerOpencodeCompact } = await import('./utils/opencodeCompactBridge'); + (triggerOpencodeCompact as unknown as ReturnType).mockImplementationOnce(async () => ({ ok: false, error: 'boom' })); + + const { session, sessionEvents } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/x') } + ]); + + await opencodeRemoteLauncher(session as never); + + expect(compactHarness.summaryCalls).toEqual([]); + const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); + expect(messages).toEqual(['📦 Compaction started', '📦 Compaction failed: boom']); + }); + + it('reports a clear failure when the session has no model metadata', async () => { + // Default harness mock's getSessionModelsMetadata returns undefined + // (harness.sessionModelsMetadata stays undefined). + const { session, sessionEvents } = createSessionStub([ + { message: '', mode: createCompactMode() } + ]); + + await opencodeRemoteLauncher(session as never); + + expect(compactHarness.calls).toEqual([]); + const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); + expect(messages).toEqual([ + '📦 Compaction started', + '📦 Compaction failed: OpenCode model metadata is not available; cannot determine provider/model for compaction.' + ]); + }); + + it('switches the model for a queued /compact operation before running it, same as a prompt turn', async () => { + // Addresses the reviewer's secondary concern: model/effort switching + // must apply to a compact batch too, in its actual queue position — + // not be skipped or applied "outside" the ordering guarantee. + harness.sessionModelsMetadata = { currentModelId: 'ollama/launch-default', availableModels: [] }; + + const { session } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/switched') } + ]); + + await opencodeRemoteLauncher(session as never); + + expect(harness.setModelArgs).toEqual([ + { sessionId: 'acp-session-1', modelId: 'ollama/switched', flavor: 'opencode' } + ]); + // The compact REST call must reflect the just-switched model, not the + // launch-time default it replaced. + expect(compactHarness.calls).toEqual([ + { + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'acp-session-1', + providerId: 'ollama', + modelId: 'switched', + signal: expect.any(AbortSignal) + } + ]); + }); + + it('a switch-to-local firing during the inline model switch that precedes a compact batch still interrupts that compact once it runs, instead of the abort landing on a not-yet-created controller', async () => { + // Reproduces a hostile-review whole-feature-sweep finding: the + // dequeue loop applies the inline model/effort switch to every batch + // (including operation:'compact' ones) *before* branching into + // runCompactOperation() — which is where compactAbortController used + // to get created. backend.setModel()/setConfigOption() are real + // async ACP round-trips that yield to the event loop, so an abort + // firing in that window used to hit a still-null + // compactAbortController (a no-op), and by the time the switch + // resolved and runCompactOperation() created a *fresh* controller, + // all memory of the abort was gone — the unbounded compact REST call + // then ran to completion uninterrupted. + // + // Uses 'switch' (not plain 'abort'/Stop) since a later round split + // handleAbort()'s behavior: only switch-to-local/exit + // (leavingRemote=true) actually aborts compactAbortController.signal + // — plain Stop now only suppresses the result and deliberately + // leaves the signal alone (see compactResultSuppressed's doc + // comment). The controller-must-already-exist regression this test + // protects against still applies identically to switch/exit. + harness.sessionModelsMetadata = { currentModelId: 'ollama/launch-default', availableModels: [] }; + let resolveSetModel: (() => void) | null = null; + harness.setModelImpl = () => new Promise((resolve) => { + resolveSetModel = resolve; + }); + + let capturedSignal: AbortSignal | undefined; + compactHarness.triggerImpl = (opts) => { + capturedSignal = opts.signal; + return Promise.resolve({ ok: true }); + }; + + const { session, rpcHandlers } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/switched') } + ]); + + const launcherPromise = opencodeRemoteLauncher(session as never, { + onCompactAvailabilityChange: () => {} + }); + + // Wait until the model switch is actually in flight. setModelArgs is + // pushed synchronously before setModelImpl() is awaited (see the + // base mock above), so by the time this is non-empty, resolveSetModel + // is already assigned too. + while (harness.setModelArgs.length === 0) { + await new Promise((resolve) => setImmediate(resolve)); + } + expect(compactHarness.calls).toEqual([]); + + const switchHandler = rpcHandlers.get('switch') as (() => Promise) | undefined; + expect(switchHandler).toBeDefined(); + await switchHandler!(); + + // Release the switch; the loop now proceeds into the compact batch. + // (Cast re-widens the type: TS narrows `resolveSetModel` to `never` + // here otherwise, since its only visible assignment is inside the + // nested Promise executor above and TS's control-flow analysis + // doesn't account for that closure running before this point.) + (resolveSetModel as (() => void) | null)?.(); + + while (compactHarness.calls.length === 0) { + await new Promise((resolve) => setImmediate(resolve)); + } + + // The controller the switch acted on during the model switch must be + // the SAME one threaded into the compact REST call — not a fresh, + // never-aborted one created after the fact. + expect(capturedSignal?.aborted).toBe(true); + + session.queue.close(); + await Promise.race([ + launcherPromise, + new Promise((_, reject) => setTimeout(() => reject(new Error('launcher did not exit in time')), 2000)) + ]); + }); + + it('a plain Stop firing during the inline model switch that precedes a compact batch prevents that compact from ever starting once the switch finishes, instead of unconditionally launching it anyway', async () => { + // Reproduces a 7th PR-review round finding: compactAbortController + // is created before the model/effort switch specifically so a + // Stop/switch/exit landing during that switch has something to act + // on (see its field doc comment). But a plain Stop only sets + // compactResultSuppressed — that flag suppresses the eventual + // RESULT of a request that's already in flight (see + // runCompactOperation()'s isCancelled()), it does not stop + // runCompactOperation() itself from being called in the first + // place. Once the switch resolved, the dequeue loop used to call + // runCompactOperation() unconditionally regardless — so a compact + // cancelled *before* its REST request was ever sent would still + // start a brand new one the instant the switch finished, blocking + // the dequeue loop for however long that takes despite the user + // having already cancelled before anything went out. Round 6's + // "wait for a request that's actually in flight to really finish" + // invariant only makes sense once a request has actually been sent + // — there's nothing server-side to wait for here. + // + // A 10th PR-review round found this skip path also never told the + // hub the queued item was done — same fix, same assertions, as the + // isLocalIdCancelled sibling test above. + harness.sessionModelsMetadata = { currentModelId: 'ollama/launch-default', availableModels: [] }; + let resolveSetModel: (() => void) | null = null; + harness.setModelImpl = () => new Promise((resolve) => { + resolveSetModel = resolve; + }); + + const { session, rpcHandlers, sessionEvents, emitMessagesConsumedCalls, thinkingChangeCalls } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/switched'), localId: 'compact-switch-1' } + ]); + + const launcherPromise = opencodeRemoteLauncher(session as never, { + onCompactAvailabilityChange: () => {} + }); + + // Wait until the model switch is actually in flight. + while (harness.setModelArgs.length === 0) { + await new Promise((resolve) => setImmediate(resolve)); + } + expect(compactHarness.calls).toEqual([]); + + const abortHandler = rpcHandlers.get('abort') as (() => Promise) | undefined; + expect(abortHandler).toBeDefined(); + await abortHandler!(); + + // Release the switch — the loop now decides what to do with the + // compact batch. (Cast re-widens the type: see the sibling test + // above for why TS narrows this to `never` otherwise.) + (resolveSetModel as (() => void) | null)?.(); + + // Give the loop several ticks to (incorrectly) start the compact + // anyway, if the fix weren't in place. + for (let i = 0; i < 10; i++) { + await new Promise((resolve) => setImmediate(resolve)); + } + + expect(compactHarness.calls).toEqual([]); + // No "Compaction started/completed/failed" at all — the operation + // never actually began. + const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); + expect(messages).toEqual([]); + expect(emitMessagesConsumedCalls).toEqual([ + { localIds: ['compact-switch-1'], options: { clearQueuedThinkingGrace: true } } + ]); + expect(thinkingChangeCalls).toEqual([false]); + + // The loop went back to waiting on the (now-empty) queue after + // skipping the cancelled compact — close it so the launcher can + // exit, same as every other test in this file that reaches the + // dequeue loop's steady state. + session.queue.close(); + await Promise.race([ + launcherPromise, + new Promise((_, reject) => setTimeout(() => reject(new Error('launcher did not exit in time')), 2000)) + ]); + }); + + it('creates a fresh compactAbortController for each sequential compact operation — no leak or cross-clearing between them', async () => { + // Backs up the "still same controller" guard in runCompactOperation()'s + // finally block (which only clears this.compactAbortController if it's + // still the instance this call created) with an executable check, not + // just the code comment's claim that two runCompactOperation calls can + // never overlap. Two isolated /compact items dequeued back-to-back + // must each get their own independent controller. + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + const capturedSignals: AbortSignal[] = []; + compactHarness.triggerImpl = (opts) => { + capturedSignals.push(opts.signal!); + return Promise.resolve({ ok: true }); + }; + + const { session } = createSessionStub([], { keepOpen: true }); + session.queue.pushIsolated('', createCompactMode('ollama/x'), 'compact-1'); + session.queue.pushIsolated('', createCompactMode('ollama/x'), 'compact-2'); + session.queue.close(); + + await opencodeRemoteLauncher(session as never, { onCompactAvailabilityChange: () => {} }); + + expect(capturedSignals.length).toBe(2); + expect(capturedSignals[0]).not.toBe(capturedSignals[1]); + // Neither should be left in an aborted state by the other's cleanup. + expect(capturedSignals[0].aborted).toBe(false); + expect(capturedSignals[1].aborted).toBe(false); + }); + + it('does not leak compactResultSuppressed into a later compact operation — a Stop-suppressed compact #1 does not silence a normally-completed compact #2', async () => { + // Companion to the controller-freshness test above: compactAbortController + // isn't the only piece of per-operation state runCompactOperation() + // reads — compactResultSuppressed (set by a plain Stop, see + // handleAbort()'s doc comment) must also be scoped to the operation + // that was actually Stopped, not linger and silence an unrelated + // later compact that completes normally. + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + let resolveFirstCompaction: (() => void) | null = null; + compactHarness.triggerImpl = (opts) => { + const callIndex = compactHarness.calls.length; + if (callIndex === 1) { + // First call: hangs until the test explicitly resolves it, + // standing in for "the server is still compacting" — same + // pattern as the plain-Stop-blocking test above. + return new Promise((resolve) => { + resolveFirstCompaction = () => resolve({ ok: true }); + }); + } + return Promise.resolve({ ok: true }); + }; + + // compact #2 is deliberately pushed *after* Stop below, not + // upfront: handleAbort() unconditionally calls session.queue.reset(), + // which would otherwise clear it before it's ever dequeued (an + // orthogonal, pre-existing behavior — see the plain-Stop-blocking + // test above's comment on the same point). + const { session, sessionEvents, rpcHandlers } = createSessionStub([], { keepOpen: true }); + session.queue.pushIsolated('', createCompactMode('ollama/x'), 'compact-1'); + + const launcherPromise = opencodeRemoteLauncher(session as never, { onCompactAvailabilityChange: () => {} }); + + while (compactHarness.calls.length === 0) { + await new Promise((resolve) => setImmediate(resolve)); + } + + const abortHandler = rpcHandlers.get('abort') as (() => Promise) | undefined; + expect(abortHandler).toBeDefined(); + await abortHandler!(); + + session.queue.pushIsolated('', createCompactMode('ollama/x'), 'compact-2'); + session.queue.close(); + + // Compact #1 genuinely finishes now — its result must stay suppressed. + resolveFirstCompaction!(); + + // Wait for compact #2 to actually run and finish too. + while (compactHarness.calls.length < 2) { + await new Promise((resolve) => setImmediate(resolve)); + } + await launcherPromise; + + const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); + expect(messages).toEqual([ + '📦 Compaction started', // compact #1 + '📦 Compaction started', // compact #2 + '📦 Compaction completed' // compact #2's result, NOT suppressed by #1's Stop + ]); }); it('injects the skill lookup instruction only on the first prompt', async () => { @@ -205,6 +1336,20 @@ describe('opencodeRemoteLauncher inline model switch', () => { expect(JSON.stringify(harness.promptContents[1])).not.toContain('skill_lookup'); }); + it('spawns the ACP backend with an explicit --port/--hostname from allocateFreePort', async () => { + const { session } = createSessionStub([ + { message: 'first', mode: createMode() } + ]); + + await opencodeRemoteLauncher(session as never); + + const opencodeBackendModule = await import('./utils/opencodeBackend'); + const factory = (opencodeBackendModule as unknown as { createOpencodeBackend: ReturnType }).createOpencodeBackend; + const lastCall = factory.mock.calls.at(-1)?.[0] as { cwd?: string; port?: number; hostname?: string }; + expect(lastCall.port).toBe(48273); + expect(lastCall.hostname).toBe('127.0.0.1'); + }); + it('calls setModel with opencode flavor between turns when the queued model differs', async () => { const { session } = createSessionStub([ { message: 'first', mode: createMode('ollama/exaone:4.5-33b-q8') }, @@ -554,3 +1699,69 @@ describe('opencodeRemoteLauncher inline model switch', () => { ]); }); }); + +describe('selectAbortStatusMessage', () => { + // Pure-logic unit tests for handleAbort()'s final decision, extracted + // specifically because opencodeRemoteLauncher.test.ts's harness has no + // way to observe MessageBuffer/Ink content — the launcher instance + // itself is never exposed to tests, only the session stub and the exit + // reason. These exercise the exact same decision handleAbort() makes, + // driven by re-read (not snapshotted) state, without needing any of + // that launcher/Ink machinery. + + it('a plain Stop with a compact still running (not yet aborted) reports the waiting message and does not clear thinking', () => { + const decision = selectAbortStatusMessage({ + hasCompactInFlight: true, + leavingRemote: false, + compactAborted: false + }); + + expect(decision.shouldClearThinking).toBe(false); + // Must actually tell the user how to leave, not just that they're stuck. + expect(decision.message).toContain('waiting for the in-progress compaction'); + expect(decision.message.toLowerCase()).toMatch(/switch|exit/); + }); + + it('switch-to-local/exit (leavingRemote=true) with a compact in flight reports "Turn aborted" and clears thinking, even before the abort() call is reflected in the signal', () => { + // Mirrors handleAbort()'s actual call: it reads leavingRemote + // directly (not compactAborted) to decide this branch, since the + // real call chain always aborts the controller synchronously before + // reaching this decision when leavingRemote is true — this input + // combination (leavingRemote=true, compactAborted=false) simply + // proves the decision doesn't depend on compactAborted once + // leavingRemote is true. + const decision = selectAbortStatusMessage({ + hasCompactInFlight: true, + leavingRemote: true, + compactAborted: false + }); + + expect(decision).toEqual({ message: 'Turn aborted', shouldClearThinking: true }); + }); + + it('no compact in flight reports "Turn aborted" regardless of leavingRemote', () => { + expect(selectAbortStatusMessage({ hasCompactInFlight: false, leavingRemote: false, compactAborted: false })) + .toEqual({ message: 'Turn aborted', shouldClearThinking: true }); + expect(selectAbortStatusMessage({ hasCompactInFlight: false, leavingRemote: true, compactAborted: false })) + .toEqual({ message: 'Turn aborted', shouldClearThinking: true }); + }); + + it('a compact already aborted by an interleaved leavingRemote=true call reports "Turn aborted" for a subsequent plain-Stop continuation reading the re-checked state — this is the RPC-overlap message-ordering fix', () => { + // Reproduces the exact scenario the previous round's fix addressed: + // Stop's continuation resumes (leavingRemote=false, as originally + // called) *after* an interleaved switch-to-local call already + // aborted the same controller. Re-reading `compactAborted` (true + // here) rather than trusting a stale "was it aborted when I + // started" snapshot is what makes this resolve to "Turn aborted" + // instead of a now-inaccurate "still waiting" message that would + // appear confusingly after switch's own "Turn aborted" already + // printed. + const decision = selectAbortStatusMessage({ + hasCompactInFlight: true, + leavingRemote: false, + compactAborted: true + }); + + expect(decision).toEqual({ message: 'Turn aborted', shouldClearThinking: true }); + }); +}); diff --git a/cli/src/opencode/opencodeRemoteLauncher.ts b/cli/src/opencode/opencodeRemoteLauncher.ts index 4ec0c360..3dea38e0 100644 --- a/cli/src/opencode/opencodeRemoteLauncher.ts +++ b/cli/src/opencode/opencodeRemoteLauncher.ts @@ -1,4 +1,5 @@ import React from 'react'; +import { randomUUID } from 'node:crypto'; import { registerAcpSessionTitleSync } from '@/agent/acpSessionTitle'; import { logger } from '@/ui/logger'; import { buildHapiMcpBridge } from '@/codex/utils/buildHapiMcpBridge'; @@ -9,21 +10,113 @@ import { OpencodeDisplay } from '@/ui/ink/OpencodeDisplay'; import type { OpencodeSession } from './session'; import type { OpencodeMode, PermissionMode } from './types'; import { RPC_METHODS } from '@hapi/protocol/rpcMethods'; -import { createOpencodeBackend } from './utils/opencodeBackend'; +import { allocateFreePort, createOpencodeBackend } from './utils/opencodeBackend'; +import { fetchCompactionSummary, splitProviderModel, triggerOpencodeCompact } from './utils/opencodeCompactBridge'; import { OpencodePermissionHandler } from './utils/permissionHandler'; import { OPENCODE_NATIVE_TOOL_INSTRUCTION, PLAN_MODE_INSTRUCTION } from './utils/systemPrompt'; import { resolveThoughtLevelEffort } from './thoughtLevelEffort'; type OpencodeRemoteLauncherOptions = { onReasoningEffortRollback?: (effort: string | null) => void; + // Called with `true` once the ACP backend + internal HTTP baseUrl are + // ready (so /compact can actually run) and with `false` whenever this + // session leaves remote mode. runOpencode.ts uses this to decide whether + // a `/compact` message should be queued or immediately answered with a + // "not yet supported" reply — see its `slash.kind === 'compact'` branch. + onCompactAvailabilityChange?: (available: boolean) => void; + // Consumes (delete-and-return) whether the queued item with this localId + // was cancelled via runOpencode.ts's `onCancelQueuedMessage` fallback + // branch (see the comment on `cancelledDequeuedLocalIds` there for what + // that actually covers — in practice a narrow ack-vs-hub-DB-write race, + // not "cancel while the REST call is running"). Checked once the REST + // call (and summary lookup) settles, so a cancelled request's result + // doesn't surface for an action the user no longer expects a reply from. + isLocalIdCancelled?: (localId: string) => boolean; }; +export type AbortStatusDecision = { + message: string; + shouldClearThinking: boolean; +}; + +/** + * Pure decision logic for handleAbort()'s final step: which status message + * to show, and whether `thinking` should be cleared. Extracted out of the + * method itself (which calls this with freshly re-read state, not a + * snapshot from before its awaits — see the call site) so it's unit + * testable without needing to observe `MessageBuffer`/Ink rendering, which + * this file's test harness (`opencodeRemoteLauncher.test.ts`) has no + * infrastructure for. + * + * A compact operation left deliberately running after a plain Stop (see + * `compactResultSuppressed`'s field doc comment on the class) is the one + * case where nothing has actually stopped yet — Stop alone cannot leave + * this remote session, only switch-to-local/exit can, so the message says + * so explicitly rather than leaving the user wondering why the UI still + * looks busy. + */ +export function selectAbortStatusMessage(opts: { + hasCompactInFlight: boolean; + leavingRemote: boolean; + compactAborted: boolean; +}): AbortStatusDecision { + const compactStillWaiting = opts.hasCompactInFlight && !opts.leavingRemote && !opts.compactAborted; + if (compactStillWaiting) { + return { + message: 'Stop requested — waiting for the in-progress compaction to finish on the server. Switch to local or exit to leave immediately.', + shouldClearThinking: false + }; + } + return { message: 'Turn aborted', shouldClearThinking: true }; +} + class OpencodeRemoteLauncher extends RemoteLauncherBase { private readonly session: OpencodeSession; private backend: ReturnType | null = null; + /** Loopback base URL of the OpenCode ACP subprocess's internal HTTP API, set once the backend is spawned with an explicit --port/--hostname. */ + private baseUrl: string | null = null; private permissionHandler: OpencodePermissionHandler | null = null; private happyServer: { stop: () => void } | null = null; private abortController = new AbortController(); + // Set by the dequeue loop as soon as a batch is identified as a + // `operation:'compact'` one — deliberately *before* that batch's inline + // model/effort switch runs, not only once runCompactOperation()'s + // triggerOpencodeCompact() REST call actually starts (a hostile-review + // sweep found that creating it any later left a window during that + // switch — a real async ACP round-trip — where an abort had nothing to + // act on yet). Null whenever no compact batch is in flight. Unlike + // `abortController` above (which governs the dequeue loop's + // wait-for-next-message signal), `handleAbort()` needs this to actually + // interrupt the compact's HTTP call(s) — without it, Stop/switch-to-local + // has no way to unblock a dequeued /compact whose REST call is + // deliberately unbounded (see triggerOpencodeCompact's doc comment) and + // the launcher stays wedged until it eventually settles on its own. + private compactAbortController: AbortController | null = null; + // True from the moment handleAbort() observes a compact operation in + // flight until the dequeue loop creates the next one. A 6th PR-review + // round found that unconditionally aborting `compactAbortController` on + // *plain* Stop (not just switch/exit) broke a core invariant this + // feature's whole redesign (see the FIFO-queue comment on the dequeue + // loop) depends on: compact and a prompt must never touch the same + // OpenCode session at once. Aborting only unblocks the *client's* fetch + // — `session/update` notifications are a separate channel from that + // HTTP request's lifecycle (see AcpSdkBackend.suppressUpdatesDuring's + // doc comment), so the agent can still be compacting server-side well + // after the client gives up, and the quiet-drain there (bounded at + // ~1.2s) is not a real guarantee that a multi-minute server-side + // compaction has actually finished. If the dequeue loop moved on to a + // prompt as soon as the client-side abort settled, that prompt could + // run concurrently with a compaction still touching the same session. + // + // The fix: plain Stop only sets this flag (suppressing the eventual + // result) and leaves `compactAbortController` alone, so + // runCompactOperation()'s own awaits keep blocking the dequeue loop + // until the *real* HTTP response arrives — i.e. until the server + // actually finishes. Switch-to-local/exit still abort the controller for + // real (see handleAbort's `leavingRemote` parameter) because cleanup() + // is about to disconnect the whole ACP subprocess regardless, so there's + // no session left to protect. + private compactResultSuppressed = false; private displayPermissionMode: PermissionMode | null = null; private instructionsSent = false; private currentBackendModel: string | null = null; @@ -62,8 +155,20 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { }); this.happyServer = happyServer; + // Pre-select a loopback port for the ACP subprocess's internal HTTP + // API and pass it explicitly via --port/--hostname. opencode does not + // announce the bound port anywhere (stdout/stderr/ACP responses) when + // launched with --port 0, so HAPI must choose it up front to be able + // to reach that HTTP API later (e.g. for /compact — see + // opencodeCompactBridge.ts). + const hostname = '127.0.0.1'; + const port = await allocateFreePort(hostname); + this.baseUrl = `http://${hostname}:${port}`; + const backend = createOpencodeBackend({ - cwd: session.path + cwd: session.path, + port, + hostname }); this.backend = backend; registerAcpSessionTitleSync(backend, session.client); @@ -115,6 +220,34 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { this.currentBackendEffort = thoughtLevelOption?.currentValue ?? null; this.defaultBackendEffort = this.currentBackendEffort; + // Let the caller (runOpencode.ts) know native /compact can actually + // run now that the ACP backend + internal HTTP baseUrl exist. The + // dequeue loop below (not an externally-invoked trigger) is what + // executes it, in its actual FIFO queue position. + // + // A 9th PR-review round found a race here: a terminal + // switch-to-local/exit can land *during* the newSession/loadSession + // await above (setupTerminal() wires up onExit/onSwitchToLocal + // before runMainLoop() even starts, so this is reachable well + // before setupAbortHandlers() below registers the RPC + // 'abort'/'switch' handlers). RemoteLauncherBase.requestExit() + // already fired onLeavingRemote() (availability(false)) and set + // `this.shouldExit = true` synchronously for that switch/exit, + // before awaiting its handler — but this line used to run + // regardless once initialization finished, resurrecting + // availability(true) even though the session is already on its way + // out. runOpencode.ts's compactSupported/compactTeardownInProgress + // gate treats compactSupported flipping true as reason enough to + // ignore compactTeardownInProgress entirely (see that gate's doc + // comment), so this stray true could let a /compact arriving right + // after slip into the queue mid-teardown. Checking `shouldExit` + // here — the same flag requestExit() already set — keeps + // availability from ever un-flipping once a switch/exit is + // underway. + if (!this.shouldExit) { + this.options.onCompactAvailabilityChange?.(true); + } + // Expose the cached models metadata via per-session RPC so the hub can // forward it to the web UI's model selector without round-tripping ACP. session.client.rpcHandlerManager.registerHandler(RPC_METHODS.ListOpencodeModels, async () => { @@ -149,7 +282,10 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { this.applyDisplayMode(session.getPermissionMode() as PermissionMode); this.setupAbortHandlers(session.client.rpcHandlerManager, { - onAbort: () => this.handleAbort(), + // Explicit `false`: plain Stop stays in this remote session, so + // an in-flight compact must not be aborted client-side — see + // handleAbort's `leavingRemote` doc comment. + onAbort: () => this.handleAbort(false), onSwitch: () => this.handleSwitchRequest() }); @@ -167,6 +303,29 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { break; } + // Created here — before the model/effort switch below — rather + // than inside runCompactOperation(), so it already exists for + // handleAbort() to act on during that switch. backend.setModel()/ + // setConfigOption() are real async ACP round-trips that yield to + // the event loop; a hostile-review whole-feature sweep found + // that an abort landing in that window used to hit a still-null + // compactAbortController (a no-op) and then get silently + // forgotten once runCompactOperation() created a *fresh* + // controller afterward — the compact's unbounded REST call would + // then run to completion with no way to interrupt it, despite + // the user having already pressed Stop/switch/exit. + const isCompactBatch = batch.mode.operation === 'compact'; + const compactAbortController = isCompactBatch ? new AbortController() : null; + if (compactAbortController) { + this.compactAbortController = compactAbortController; + // Reset here (as early as the controller itself — see its + // sibling field's doc comment for why that timing matters) + // rather than inside runCompactOperation(), so a plain Stop + // landing during the model/effort switch below already has + // something to suppress. + this.compactResultSuppressed = false; + } + // Inline model change via ACP RPC (session/set_model — see ACP SDK // schema `x-method: session/set_model`). Mirrors the Gemini pattern // from PR #543: if the running OpenCode build does not implement the @@ -271,6 +430,118 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { this.applyDisplayMode(batch.mode.permissionMode); messageBuffer.addMessage(batch.message, 'user'); + // /compact reaches here through the exact same dequeue loop as + // any prompt — it was pushed via messageQueue.pushIsolated(...) + // in runOpencode.ts, so it occupies its real FIFO position + // relative to prompts queued before or after it (fixes a prior + // design where /compact ran via an externally-invoked trigger + // and could execute ahead of an already-queued prompt). The + // model/effort switch above already ran for this batch just like + // any other, so compaction runs under whatever model this batch + // resolved to. + if (isCompactBatch && compactAbortController) { + // A compact batch is always a single isolated item (pushed + // via pushIsolated), so its own localId is exactly + // batch.items[0]?.localId. + const compactLocalId = batch.items[0]?.localId; + + // A 7th PR-review round found that a plain Stop landing + // *during the model/effort switch above* — before this + // compact's REST request has ever actually been sent — was + // silently ignored here. Plain Stop's handleAbort(false) only + // sets `compactResultSuppressed = true`; it deliberately + // leaves compactAbortController.signal alone (see that + // field's doc comment — Round 6 needs the real HTTP request + // to keep running so the dequeue loop can wait for genuine + // server-side completion). But that logic assumed a request + // was already in flight to wait for. Here, mid-switch, none + // has been sent yet — so this branch used to call + // runCompactOperation() unconditionally once the switch + // resolved anyway, starting a brand new REST request the + // instant a cancelled compact's turn came up and blocking + // the dequeue loop for however long that takes. + // + // The fix is narrow on purpose: skip starting the operation + // only when a plain Stop landed (compactResultSuppressed) + // AND the controller was never actually aborted. If the + // controller WAS aborted, that means switch/exit's + // handleAbort(true) ran instead — and Round 5's test + // (below) established that runCompactOperation() must still + // be called in that case, threading the pre-aborted signal + // through so the fetch call rejects immediately without any + // network I/O, rather than being skipped here. + // + // An 8th PR-review round found this same reasoning also + // applies to isLocalIdCancelled, which round 7 had + // deliberately left out of this check (see runOpencode.ts's + // preparingLocalIds/cancelledBeforeEnqueue doc comment for + // the full mechanism): the localId-keyed cancel Set it reads + // can *only* ever be populated during the brief network + // round trip between the CLI emitting the /compact item's + // "invoked" ack and the hub recording it — never while a + // compact REST call is actually running. So if + // isLocalIdCancelled(compactLocalId) is already true here, + // that unconditionally means this compact was cancelled + // before its REST request was ever sent, exactly like the + // compactResultSuppressed case above — there's no + // in-flight server-side work to preserve by starting the + // operation anyway. (isLocalIdCancelled is a delete-and- + // return, one-shot callback, so checking it here consumes + // the same entry runCompactOperation()'s own isCancelled() + // would otherwise have consumed — it isn't checked twice.) + const compactCancelledByLocalId = compactLocalId + ? (this.options.isLocalIdCancelled?.(compactLocalId) ?? false) + : false; + const cancelledBeforeStart = + (this.compactResultSuppressed && !compactAbortController.signal.aborted) + || compactCancelledByLocalId; + if (cancelledBeforeStart) { + if (this.compactAbortController === compactAbortController) { + this.compactAbortController = null; + } + // A 10th PR-review round found this skip path never + // calls session.onThinkingChange(true) (that's the + // whole point of skipping) but also never told the hub + // this queued item is done, leaving the web UI spinner + // stuck: markMessageQueued's 15s "queued thinking" + // grace (hub/src/sync/sessionCache.ts) keeps thinking + // pinned true regardless of keepalives until either the + // grace expires or a messages-consumed ack with + // `clearQueuedThinkingGrace` arrives. Same situation, + // same fix, as the synchronous slash.kind === 'handled' + // path in runOpencode.ts (e.g. /model — see its + // `clearQueuedThinkingGrace` comment there): ack with + // the grace-clearing flag, then push an immediate + // thinking=false keepalive so the spinner clears + // without waiting on the grace. (This is on top of, not + // instead of, the queue's own unflagged + // onBatchConsumed ack — a second ack for an + // already-invoked localId is a no-op on the hub's + // first-write-wins queued-message protocol, and + // clearQueuedThinkingGrace itself is keyed by session, + // not by localId, so it's idempotent too.) + if (compactLocalId) { + session.client.emitMessagesConsumed([compactLocalId], { clearQueuedThinkingGrace: true }); + } + session.onThinkingChange(false); + if (session.queue.size() === 0 && !this.shouldExit) { + sendReady(); + } + continue; + } + + session.onThinkingChange(true); + try { + await this.runCompactOperation(acpSessionId, compactAbortController, compactLocalId); + } finally { + session.onThinkingChange(false); + if (session.queue.size() === 0 && !this.shouldExit) { + sendReady(); + } + } + continue; + } + // Inject title instructions on first prompt let messageText = batch.message; if (batch.mode.permissionMode === 'plan') { @@ -310,6 +581,24 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { } } + /** + * /compact must stop being offered the instant remote mode starts + * leaving — not merely by the time it's actually torn down, and + * critically not only on the *next* local-mode entry (the previous + * mechanism, in loop.ts's `runLocal:` callback). That gap between "a + * switch/exit was requested" and "the next runLocal() call reset this" + * is exactly the window a PR-review round found: a /compact slash + * command arriving in it still queues normally (runOpencode.ts's + * `compactSupported` flag hadn't flipped yet), and since local mode + * immediately hands back to remote when it finds a non-empty queue, that + * queued compact can end up running anyway — despite the user having + * already asked to leave remote mode. See onLeavingRemote()'s doc + * comment on RemoteLauncherBase for exactly when this fires. + */ + protected onLeavingRemote(): void { + this.options.onCompactAvailabilityChange?.(false); + } + protected async cleanup(): Promise { this.clearAbortHandlers(this.session.client.rpcHandlerManager); @@ -336,6 +625,147 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { this.options.onReasoningEffortRollback?.(effort); } + /** + * Executes the /compact operation for a queued `operation:'compact'` + * batch. Reached only through the main dequeue loop (so it never runs + * concurrently with a prompt turn — see the loop's doc comment), which + * is also why this needs no timeout/mutex of its own despite the REST + * call it makes potentially taking several minutes. + * + * `localId` is used to detect a cancel that runOpencode.ts's + * `isLocalIdCancelled` reports for this item (see its declaration there + * for the real — and narrow — race window that covers) — checked at each + * point below right before a result would be shown, same as the + * pre-redesign behavior where this was a single `wasCancelled()` check + * after one combined async trigger(). "Compaction started" itself is + * never suppressed (it wasn't before either). + * + * Separately, `compactAbortController`/`compactResultSuppressed` cover a + * different case: Stop/switch-to-local firing *while the REST call is + * actually in flight*, which `isLocalIdCancelled` cannot — that + * mechanism only ever observes a cancel for this item's *queue message*, + * and by this point the item has already been dequeued. `isCancelled()` + * below checks all three, so any kind of cancellation suppresses the + * eventual result the same way — but only switch/exit (`leavingRemote` + * in handleAbort()) actually aborts `compactAbortController.signal`; a + * plain Stop sets `compactResultSuppressed` alone and deliberately + * leaves the signal un-aborted, so this function's own awaits below keep + * blocking the dequeue loop until the operation *really* finishes + * server-side — see `compactResultSuppressed`'s field doc comment for + * why that invariant matters. + * + * `compactAbortController` is created by the caller (the dequeue loop), + * not here, and passed in — deliberately, before the loop's model/effort + * switch for this batch runs, not after. A hostile-review whole-feature + * sweep found that creating it in here (i.e. only once this function was + * actually entered) left a window during that switch — a real async ACP + * round-trip — where an abort had nothing to act on yet (`this + * .compactAbortController` was still null) and was silently lost by the + * time this function created a *fresh* controller afterward. + */ + private async runCompactOperation( + acpSessionId: string, + compactAbortController: AbortController, + localId?: string + ): Promise { + const session = this.session; + session.sendSessionEvent({ type: 'message', message: '📦 Compaction started' }); + + try { + const isCancelled = (): boolean => + (localId ? (this.options.isLocalIdCancelled?.(localId) ?? false) : false) + || compactAbortController.signal.aborted + || this.compactResultSuppressed; + + const backend = this.backend; + const baseUrl = this.baseUrl; + if (!baseUrl || !backend) { + if (!isCancelled()) { + session.sendSessionEvent({ + type: 'message', + message: '📦 Compaction failed: OpenCode internal HTTP API base URL is not available.' + }); + } + return; + } + + const metadata = backend.getSessionModelsMetadata?.(acpSessionId); + const split = splitProviderModel(metadata?.currentModelId ?? this.currentBackendModel); + if (!split) { + if (!isCancelled()) { + session.sendSessionEvent({ + type: 'message', + message: '📦 Compaction failed: OpenCode model metadata is not available; cannot determine provider/model for compaction.' + }); + } + return; + } + + // Suppressed: OpenCode keeps streaming session/update notifications + // (agent_thought_chunk etc.) over the ACP transport while this raw + // HTTP call runs — with no prompt() turn in flight to own them, they + // would otherwise leak into the previous turn's still-installed + // onUpdate and render as a duplicate assistant message alongside the + // explicit summary we show below (from fetchCompactionSummary). + // See AcpSdkBackend.suppressUpdatesDuring's doc comment. + // + // `signal` lets handleAbort() interrupt this specific call (see + // compactAbortController's field doc comment) — triggerOpencodeCompact + // otherwise has no deadline by design, since a real compaction can + // legitimately take minutes. + const result = await backend.suppressUpdatesDuring(() => triggerOpencodeCompact({ + baseUrl, + sessionId: acpSessionId, + providerId: split.providerId, + modelId: split.modelId, + signal: compactAbortController.signal + })); + if (!result.ok) { + if (!isCancelled()) { + session.sendSessionEvent({ type: 'message', message: `📦 Compaction failed: ${result.error}` }); + } else { + logger.debug('[opencode-remote] /compact failure suppressed: cancelled or aborted before it resolved'); + } + return; + } + + // Best-effort: fetch the actual summary text OpenCode generated + // before the final cancellation check, so a cancel landing anywhere + // during this whole operation (REST call or summary lookup) + // suppresses "Compaction completed" and the Reasoning block + // together — this mirrors the pre-redesign behavior, where both were + // produced by one combined async step checked once. `signal` is + // required on this call (see OpencodeCompactCallOpts) for exactly + // the reason a prior PR-review round flagged as missing here: the + // POST above being interruptible isn't enough on its own if this + // GET can still block Stop/switch-to-local for as long as it takes. + const summary = await fetchCompactionSummary({ baseUrl, sessionId: acpSessionId, signal: compactAbortController.signal }); + + if (isCancelled()) { + logger.debug('[opencode-remote] /compact result suppressed: cancelled or aborted before it resolved'); + return; + } + + session.sendSessionEvent({ type: 'message', message: '📦 Compaction completed' }); + if (summary.found) { + const converted = convertAgentMessage({ type: 'reasoning', text: summary.text, id: randomUUID() }); + if (converted) { + session.sendAgentMessage(converted); + } + } + } finally { + // Defensive: only clear if this is still the controller we set — + // mirrors the same "don't clobber a newer value" guard as + // AcpSdkBackend.suppressUpdatesDuring's restore. In practice this + // is always still the same instance, since compact runs + // serialized through the single dequeue loop (never concurrently + // with another runCompactOperation call). + if (this.compactAbortController === compactAbortController) { + this.compactAbortController = null; + } + } + } + private handleAgentMessage(message: AgentMessage): void { const converted = convertAgentMessage(message); if (converted) { @@ -383,29 +813,79 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { } } - private async handleAbort(): Promise { + /** + * `leavingRemote` distinguishes plain Stop (`false`, the default — stays + * in the same remote session) from switch-to-local/exit (`true` — the + * session is being torn down). A 6th PR-review round rejected an earlier + * fix (always aborting `compactAbortController` here) because it broke + * this feature's core invariant: compact and a prompt must never touch + * the same OpenCode session concurrently (see + * `compactResultSuppressed`'s field doc comment for the full + * reasoning). Plain Stop now only suppresses the eventual result and + * leaves the compact operation's REST call running for real — the + * dequeue loop stays blocked on it until the server actually finishes, + * exactly as it does for an un-aborted turn. Switch/exit still abort it + * for real: `cleanup()` disconnects the whole ACP subprocess right + * after, so there is no shared-session invariant left to protect and + * responsiveness (fixed in an earlier round) matters more. + */ + private async handleAbort(leavingRemote = false): Promise { + // A hostile-review sweep found that a plain Stop during an in-flight + // compact — which deliberately leaves the operation running for real + // (see compactResultSuppressed's doc comment) — still unconditionally + // flipped `thinking` off and reported "Turn aborted" below, telling + // the user the turn had stopped while the dequeue loop was actually + // still blocked inside runCompactOperation() for however long the + // real server-side compaction takes (potentially minutes). Track + // that specific case so the messaging stays honest: nothing has + // actually stopped yet from the user's perspective, and the dequeue + // loop's own `finally` (once runCompactOperation() genuinely + // returns) remains the sole source of truth for when this turn is + // done. + const compactAbortController = this.compactAbortController; + if (compactAbortController) { + this.compactResultSuppressed = true; + if (leavingRemote) { + compactAbortController.abort(); + } + } const backend = this.backend; if (backend && this.session.sessionId) { await backend.cancelPrompt(this.session.sessionId); } await this.permissionHandler?.cancelAll('User aborted'); this.session.queue.reset(); - this.session.onThinkingChange(false); this.abortController.abort(); this.abortController = new AbortController(); - this.messageBuffer.addMessage('Turn aborted', 'status'); + // Re-read here (not the snapshot taken above, before the awaits) in + // case a concurrent leavingRemote=true call for the same compact + // interleaved with this one and already aborted it — RPC dispatch + // doesn't serialize handleAbort() calls against each other, so a + // Stop immediately followed by a switch-to-local can genuinely + // overlap. Without this, the (now-stale) plain-Stop continuation + // could append its "still waiting" message after the switch's + // "Turn aborted" already ran, showing the two in a confusing order. + const decision = selectAbortStatusMessage({ + hasCompactInFlight: compactAbortController !== null, + leavingRemote, + compactAborted: compactAbortController?.signal.aborted ?? false + }); + if (decision.shouldClearThinking) { + this.session.onThinkingChange(false); + } + this.messageBuffer.addMessage(decision.message, 'status'); } private async handleExitFromUi(): Promise { - await this.requestExit('exit', () => this.handleAbort()); + await this.requestExit('exit', () => this.handleAbort(true)); } private async handleSwitchFromUi(): Promise { - await this.requestExit('switch', () => this.handleAbort()); + await this.requestExit('switch', () => this.handleAbort(true)); } private async handleSwitchRequest(): Promise { - await this.requestExit('switch', () => this.handleAbort()); + await this.requestExit('switch', () => this.handleAbort(true)); } } diff --git a/cli/src/opencode/runOpencode.test.ts b/cli/src/opencode/runOpencode.test.ts index 4537ca0a..0f8bc7c1 100644 --- a/cli/src/opencode/runOpencode.test.ts +++ b/cli/src/opencode/runOpencode.test.ts @@ -6,7 +6,17 @@ const mockOpencodeSession = vi.hoisted(() => ({ setModelReasoningEffort: vi.fn(), pushKeepAlive: vi.fn(), thinking: false, - stopKeepAlive: vi.fn() + stopKeepAlive: vi.fn(), + onThinkingChange: vi.fn(), + // Mirrors AgentSessionBase's own `mode` field ('local' | 'remote', + // flipped synchronously by onModeChange before either launcher + // starts/finishes) — settable per test to simulate a session that + // started in remote mode and is still initializing (ACP backend not + // ready yet, so onCompactAvailabilityChange(true) hasn't fired), as + // opposed to a genuinely local-mode session. This becomes + // sessionWrapperRef.current in runOpencode.ts via the mocked + // opencodeLoop's onSessionReady callback below. + mode: 'local' as 'local' | 'remote' })); const harness = vi.hoisted(() => ({ @@ -18,7 +28,12 @@ const harness = vi.hoisted(() => ({ onUserMessage: vi.fn(), onCancelQueuedMessage: vi.fn(), sendAgentMessage: vi.fn(), + sendSessionEvent: vi.fn(), emitMessagesConsumed: vi.fn(), + // Needed for createModeChangeHandler(session) (real, unmocked) to + // run without throwing when a test invokes the real onModeChange + // wrapper passed to opencodeLoop. + updateAgentState: vi.fn(), rpcHandlerManager: { registerHandler: vi.fn() } @@ -100,10 +115,14 @@ describe('runOpencode set-session-config handler', () => { mockOpencodeSession.setPermissionMode.mockReset(); mockOpencodeSession.setModelReasoningEffort.mockReset(); mockOpencodeSession.pushKeepAlive.mockReset(); + mockOpencodeSession.onThinkingChange.mockReset(); + mockOpencodeSession.mode = 'local'; harness.session.onUserMessage.mockReset(); harness.session.onCancelQueuedMessage.mockReset(); harness.session.sendAgentMessage.mockReset(); + harness.session.sendSessionEvent.mockReset(); harness.session.emitMessagesConsumed.mockReset(); + harness.session.updateAgentState.mockReset(); harness.session.rpcHandlerManager.registerHandler.mockReset(); harness.listSlashCommands.mockReset(); harness.listSlashCommands.mockResolvedValue([]); @@ -257,6 +276,234 @@ describe('runOpencode set-session-config handler', () => { expect(harness.session.sendAgentMessage).toHaveBeenCalled(); }); + it('queues a /compact request (isolated, with operation:"compact") once compact becomes available', async () => { + await runOpencode({}); + + const onCompactAvailabilityChange = harness.opencodeLoopArgs[0]?.onCompactAvailabilityChange as + ((available: boolean) => void) | undefined; + expect(onCompactAvailabilityChange).toBeDefined(); + onCompactAvailabilityChange!(true); + + const messageQueue = harness.opencodeLoopArgs[0]?.messageQueue as + { queue: Array<{ message: string; mode: { operation?: string }; localId?: string; isolate?: boolean }> }; + + const userMessageHandler = harness.session.onUserMessage.mock.calls[0]?.[0] as + ((msg: { content: { text: string; attachments?: unknown[] } }, localId?: string) => void) + | undefined; + expect(userMessageHandler).toBeDefined(); + + userMessageHandler!({ content: { text: '/compact' } }, 'local-compact'); + // Drain microtasks across the async chain: listSlashCommands -> slash + // resolve -> messageQueue.pushIsolated(...). + for (let i = 0; i < 5; i++) { + await new Promise((resolve) => setTimeout(resolve, 0)); + } + + // No manual emitMessagesConsumed call here (unlike the synchronous + // 'handled' branch) — the ack now happens automatically at dequeue + // time via MessageQueue2's onBatchConsumed (wired in + // AgentSessionBase's constructor onto session.queue, same as any + // regular prompt), not synchronously when queuing. A manual call + // right here used to exist and fire immediately regardless of FIFO + // position — that's what let the hub mark a still-queued /compact + // "invoked" before it was actually dequeued, breaking cancellation + // of it while queued (see the comment on this branch in + // runOpencode.ts and opencodeRemoteLauncher.test.ts's "cancelling a + // /compact operation while it is still queued behind another + // prompt" test for the fix this enables). + expect(harness.session.emitMessagesConsumed).not.toHaveBeenCalled(); + // The actual REST call, "Compaction started/completed" status events, + // and Reasoning-block summary now all happen inside + // opencodeRemoteLauncher.ts's dequeue loop once this item reaches the + // front of the queue (covered by opencodeRemoteLauncher.test.ts) — + // runOpencode.ts's job for a supported /compact is only to queue it + // in its correct FIFO position, never to run it directly. + expect(messageQueue.queue).toEqual([ + { + message: '', + mode: expect.objectContaining({ operation: 'compact' }), + modeHash: expect.any(String), + localId: 'local-compact', + isolate: true + } + ]); + expect(harness.session.sendSessionEvent).not.toHaveBeenCalled(); + expect(harness.session.sendAgentMessage).not.toHaveBeenCalled(); + }); + + it('queues /compact like a prompt while a remote-mode session is still initializing (ACP backend not ready yet), instead of rejecting it as not-yet-supported', async () => { + // Reproduces a hostile-review finding: compactSupported alone + // conflates "genuinely local mode" with "remote mode, but ACP + // initialize + session load/new hasn't finished yet" — a regular + // prompt sent in that exact same startup window queues normally and + // just waits, but /compact used to get an immediate not-yet-supported + // reply instead, even though the session is (or is about to be) in + // remote mode. sessionWrapperRef.current?.mode (mocked here via + // mockOpencodeSession.mode, delivered through onSessionReady) is what + // now distinguishes the two — deliberately never call + // onCompactAvailabilityChange(true) here, since the whole point is + // that this must queue even while it's still false. + mockOpencodeSession.mode = 'remote'; + await runOpencode({}); + + const messageQueue = harness.opencodeLoopArgs[0]?.messageQueue as + { queue: Array<{ message: string; mode: { operation?: string }; localId?: string; isolate?: boolean }> }; + + const userMessageHandler = harness.session.onUserMessage.mock.calls[0]?.[0] as + ((msg: { content: { text: string; attachments?: unknown[] } }, localId?: string) => void) + | undefined; + expect(userMessageHandler).toBeDefined(); + + userMessageHandler!({ content: { text: '/compact' } }, 'local-compact-pending'); + for (let i = 0; i < 5; i++) { + await new Promise((resolve) => setTimeout(resolve, 0)); + } + + // Must not get the not-yet-supported reply. + expect(harness.session.sendAgentMessage).not.toHaveBeenCalled(); + // Must be queued exactly like the compactSupported===true case above. + expect(messageQueue.queue).toEqual([ + { + message: '', + mode: expect.objectContaining({ operation: 'compact' }), + modeHash: expect.any(String), + localId: 'local-compact-pending', + isolate: true + } + ]); + }); + + it('falls back to a not-yet-supported message for /compact when compact is not available (e.g. local mode)', async () => { + await runOpencode({}); + // Deliberately do not call onCompactAvailabilityChange(true) — this + // is the state a local-mode session stays in (loop.ts resets it to + // false on every local entry and opencodeLocalLauncher never sets it + // true). + + const userMessageHandler = harness.session.onUserMessage.mock.calls[0]?.[0] as + ((msg: { content: { text: string; attachments?: unknown[] } }, localId?: string) => void) + | undefined; + userMessageHandler!({ content: { text: '/compact' } }, 'local-compact-none'); + for (let i = 0; i < 5; i++) { + await new Promise((resolve) => setTimeout(resolve, 0)); + } + + const messages = harness.session.sendAgentMessage.mock.calls.map((call) => (call[0] as { message: string }).message); + expect(messages).toEqual(['/compact is not yet supported in HAPI OpenCode sessions.']); + + const messageQueue = harness.opencodeLoopArgs[0]?.messageQueue as { queue: unknown[] }; + expect(messageQueue.queue).toEqual([]); + }); + + it('stops queuing /compact once availability is reset to false (e.g. a remote->local handoff mid-session)', async () => { + await runOpencode({}); + + // Faithful to real production timing: session.mode stays 'remote' + // throughout the whole teardown window (onLeavingRemote firing + // synchronously as the very first action of requestExit(), long + // before runMainLoop() actually returns and mode flips back to + // 'local') — see AgentSessionBase.onModeChange and + // OpencodeRemoteLauncher.onLeavingRemote's doc comments. A hostile + // review found that omitting this from the mock let a real + // regression slip through: the mode-based /compact queuing fix + // for the *startup* window (mode:'remote' but not yet ready) also + // accidentally re-opened queuing during *this* teardown window, + // since both look identical if you only check `mode !== 'remote'`. + mockOpencodeSession.mode = 'remote'; + + const onCompactAvailabilityChange = harness.opencodeLoopArgs[0]?.onCompactAvailabilityChange as + ((available: boolean) => void) | undefined; + onCompactAvailabilityChange!(true); + onCompactAvailabilityChange!(false); + + const userMessageHandler = harness.session.onUserMessage.mock.calls[0]?.[0] as + ((msg: { content: { text: string; attachments?: unknown[] } }, localId?: string) => void) + | undefined; + userMessageHandler!({ content: { text: '/compact' } }, 'local-compact-reset'); + for (let i = 0; i < 5; i++) { + await new Promise((resolve) => setTimeout(resolve, 0)); + } + + const messages = harness.session.sendAgentMessage.mock.calls.map((call) => (call[0] as { message: string }).message); + expect(messages).toEqual(['/compact is not yet supported in HAPI OpenCode sessions.']); + + const messageQueue = harness.opencodeLoopArgs[0]?.messageQueue as { queue: unknown[] }; + expect(messageQueue.queue).toEqual([]); + }); + + it('resumes queuing /compact once a torn-down session re-enters remote mode (compactTeardownInProgress resets on the next remote entry)', async () => { + // Locks in the other half of compactTeardownInProgress's contract: + // it must not get stuck true forever after one teardown, or every + // later remote re-entry's startup window (the case the "queues + // /compact like a prompt while..." test above covers) would + // incorrectly reject /compact too. + // + // Round 2 of the review-cycle that produced this test found the + // first version didn't actually discriminate the fix: it left + // `mockOpencodeSession.mode` at 'remote' throughout, so the gate's + // `mode !== 'remote'` clause was permanently false and the test + // would have passed identically even if compactTeardownInProgress + // never reset (or didn't exist at all). Faithfully modeling the + // real local interlude between the two remote attempts — mode + // actually flips to 'local' once the first remote launcher's + // runMainLoop() fully unwinds, per AgentSessionBase.onModeChange — + // is what makes this test sensitive to the reset specifically. + mockOpencodeSession.mode = 'remote'; + + await runOpencode({}); + + const onCompactAvailabilityChange = harness.opencodeLoopArgs[0]?.onCompactAvailabilityChange as + ((available: boolean) => void) | undefined; + const onModeChange = harness.opencodeLoopArgs[0]?.onModeChange as + ((mode: 'local' | 'remote') => void) | undefined; + expect(onModeChange).toBeDefined(); + + const messageQueue = harness.opencodeLoopArgs[0]?.messageQueue as + { queue: Array<{ message: string; mode: { operation?: string }; localId?: string; isolate?: boolean }> }; + const userMessageHandler = harness.session.onUserMessage.mock.calls[0]?.[0] as + ((msg: { content: { text: string; attachments?: unknown[] } }, localId?: string) => void) + | undefined; + + // First remote attempt becomes ready, then tears down. + onCompactAvailabilityChange!(true); + onCompactAvailabilityChange!(false); + + // Local interlude — mode genuinely flips to 'local' here in + // production. Sanity-check /compact is still correctly rejected + // during it (proves the interlude is real, not cosmetic). + mockOpencodeSession.mode = 'local'; + userMessageHandler!({ content: { text: '/compact' } }, 'local-compact-interlude'); + for (let i = 0; i < 5; i++) { + await new Promise((resolve) => setTimeout(resolve, 0)); + } + expect(harness.session.sendAgentMessage).toHaveBeenCalledTimes(1); + expect(messageQueue.queue).toEqual([]); + + // The next remote attempt begins: mode flips back to 'remote' and + // onModeChange fires on that exact transition — this is what + // compactTeardownInProgress's reset actually depends on. + mockOpencodeSession.mode = 'remote'; + onModeChange!('remote'); + + userMessageHandler!({ content: { text: '/compact' } }, 'local-compact-reentry'); + for (let i = 0; i < 5; i++) { + await new Promise((resolve) => setTimeout(resolve, 0)); + } + + // Still only the one not-yet-supported reply from the interlude + // above — the re-entry /compact must queue, not get a second reply. + expect(harness.session.sendAgentMessage).toHaveBeenCalledTimes(1); + expect(messageQueue.queue).toEqual([ + { + message: '', + mode: expect.objectContaining({ operation: 'compact' }), + modeHash: expect.any(String), + localId: 'local-compact-reentry', + isolate: true + } + ]); + }); + it('cancels a slash command that is cancelled before listSlashCommands resolves', async () => { let releaseListSlashCommands: () => void = () => {}; const slashCommandsPromise = new Promise((resolve) => { @@ -288,4 +535,36 @@ describe('runOpencode set-session-config handler', () => { expect(harness.session.sendAgentMessage).not.toHaveBeenCalled(); expect(harness.session.emitMessagesConsumed).not.toHaveBeenCalled(); }); + + it('bounds the unmatched-cancel tracking Set so it cannot grow unboundedly over a long session', async () => { + await runOpencode({}); + + const cancelHandler = harness.session.onCancelQueuedMessage.mock.calls[0]?.[0] as + ((localId: string) => boolean) | undefined; + const isLocalIdCancelled = harness.opencodeLoopArgs[0]?.isLocalIdCancelled as + ((localId: string) => boolean) | undefined; + expect(cancelHandler).toBeDefined(); + expect(isLocalIdCancelled).toBeDefined(); + + // None of these localIds are in the queue or in the pre-enqueue + // preparing window, so every call falls into the fallback branch + // that records it as a possible dequeued-compact cancel. Simulate + // far more of these than could ever realistically be in flight at + // once (see the comment on `cancelledDequeuedLocalIds` in + // runOpencode.ts for why this branch is only reachable during a + // brief per-message ack race) to prove the tracking Set evicts its + // oldest entries instead of growing forever. + const localIds = Array.from({ length: 200 }, (_, i) => `unmatched-${i}`); + for (const localId of localIds) { + cancelHandler!(localId); + } + + // The earliest entries must have been evicted... + expect(isLocalIdCancelled!('unmatched-0')).toBe(false); + // ...while a recent one is still tracked (delete-and-return: true + // once, then gone). + const lastLocalId = localIds[localIds.length - 1]!; + expect(isLocalIdCancelled!(lastLocalId)).toBe(true); + expect(isLocalIdCancelled!(lastLocalId)).toBe(false); + }); }); diff --git a/cli/src/opencode/runOpencode.ts b/cli/src/opencode/runOpencode.ts index 3e89e02a..668fd59f 100644 --- a/cli/src/opencode/runOpencode.ts +++ b/cli/src/opencode/runOpencode.ts @@ -79,10 +79,43 @@ export async function runOpencode(opts: { // batches with different intent don't merge — the launcher uses null // to mean "switch back to defaultBackendModel". model: mode.model === null ? '__reset__' : mode.model ?? null, - modelReasoningEffort: mode.modelReasoningEffort ?? null + modelReasoningEffort: mode.modelReasoningEffort ?? null, + // Defense in depth: a compact item is always pushed via + // `pushIsolated` (never batches with siblings regardless of mode + // hash), but including `operation` here too means a prompt and a + // compact request could never be merged into one batch even if that + // isolation guard were ever bypassed. + operation: mode.operation ?? null })); const sessionWrapperRef: { current: OpencodeSession | null } = { current: null }; + // Set by opencodeRemoteLauncher once the ACP backend + internal HTTP + // baseUrl are actually ready (remote mode only), and reset to false as + // early as possible whenever this session leaves remote mode + // (OpencodeRemoteLauncher's onLeavingRemote() override — see its doc + // comment on RemoteLauncherBase for exactly when that fires). While + // false, the `slash.kind === 'compact'` branch below must tell apart two + // situations that both look like "compactSupported is false, mode is + // 'remote'": a session that just entered remote mode and hasn't finished + // ACP initialize+session load/new yet (should queue /compact like a + // prompt), versus a session whose remote launcher is already tearing + // down (onLeavingRemote fired, `mode` hasn't flipped back to 'local' yet + // because that only happens once the whole launcher unwinds — see + // AgentSessionBase.onModeChange). `compactTeardownInProgress` below is + // what distinguishes them — a hostile-review sweep found that gating on + // `mode` alone (added to fix the first case) silently re-opened the + // second: it let /compact queue during the exact teardown window + // onLeavingRemote exists to protect, since `mode` stays 'remote' + // throughout it. + let compactSupported = false; + // True from the moment onCompactAvailabilityChange(false) fires (which, + // per onLeavingRemote's contract, only ever happens because remote mode + // is being left — never because remote just started) until this session + // next re-enters remote mode (see the wrapped `onModeChange` below). + // Only meaningful while `sessionWrapperRef.current?.mode === 'remote'`; + // harmless/stale otherwise since the mode check alone already rejects a + // genuinely local-mode session regardless of this flag's value. + let compactTeardownInProgress = false; let currentPermissionMode: PermissionMode = opts.permissionMode ?? 'default'; let sessionModel: string | null = initialModel; let sessionModelReasoningEffort: string | null = initialModelReasoningEffort; @@ -134,6 +167,52 @@ export async function runOpencode(opts: { // short-circuit when it resumes. const preparingLocalIds = new Set(); const cancelledBeforeEnqueue = new Set(); + // Mirrors `cancelledBeforeEnqueue` above, but for the other side of the + // queued-compact ack: `onCancelQueuedMessage` below can still fire for a + // localId that's neither in the queue nor in `preparingLocalIds`. Track + // it here and let the launcher consume it via `isLocalIdCancelled` + // (passed through opencodeLoop) so it can suppress the eventual + // "Compaction completed/failed" + Reasoning-block result if this really + // was that localId. + // + // Note on how narrow this window actually is: the hub only calls back + // into the CLI's `onCancelQueuedMessage` when its own DB lookup still + // finds the row queued (invoked_at IS NULL) — see + // `cancelQueuedMessage`'s Phase 1 in hub/src/sync/messageService.ts. + // `session.emitMessagesConsumed([localId])` a few lines below fires the + // "invoked" ack for the /compact message *before* it's pushed onto + // `messageQueue`, i.e. long before the launcher ever dequeues it and + // starts the REST call. So once that ack's DB write lands, every later + // cancel request short-circuits on the hub side and never reaches the + // CLI at all — this Set can only ever be populated during the brief + // network round trip between the CLI emitting that ack and the hub + // recording it, not while the compact REST call is actually running. + // That's an existing characteristic of the hub's first-write-wins + // queued-message protocol (present since Phase 1 of this feature, not + // something this change introduced or is trying to fix — a hub-side + // redesign of that protocol is out of scope here since it would affect + // cancel behavior for every flavor, not just OpenCode /compact). + // + // Nothing here distinguishes "this localId was actually a /compact + // message" from any other queued message whose cancel happened to land + // in that race window — the fallback branch below has no way to know. + // For a real /compact race, `isLocalIdCancelled` reads (and deletes) the + // entry once `runCompactOperation` checks it; for anything else, the + // entry would sit here unread for the rest of the process's life. Cap + // the Set (oldest-first eviction, relying on Set's insertion-order + // iteration) so a long-running session can't accumulate these forever — + // realistically at most a handful of entries would ever coexist, so this + // cap is a defensive bound, not something expected to trigger. + const MAX_CANCELLED_DEQUEUED_LOCAL_IDS = 50; + const cancelledDequeuedLocalIds = new Set(); + const addCancelledDequeuedLocalId = (localId: string): void => { + cancelledDequeuedLocalIds.add(localId); + while (cancelledDequeuedLocalIds.size > MAX_CANCELLED_DEQUEUED_LOCAL_IDS) { + const oldest = cancelledDequeuedLocalIds.values().next().value; + if (oldest === undefined) break; + cancelledDequeuedLocalIds.delete(oldest); + } + }; let userMessageChain: Promise = Promise.resolve(); session.onUserMessage((message, localId) => { @@ -167,6 +246,88 @@ export async function runOpencode(opts: { modelReasoningEffort: sessionModelReasoningEffort }); + if (slash.kind === 'compact') { + // `compactSupported` alone conflates two different + // situations: a genuinely local-mode session (compact + // fundamentally can't run — there's no ACP backend to + // run it against) versus a session that's already in + // remote mode but hasn't finished ACP initialize + + // session load/new yet (onCompactAvailabilityChange(true) + // hasn't fired *yet*, but will shortly). A hostile-review + // sweep found the old code treated both the same way — + // an immediate not-yet-supported reply — even though a + // regular prompt sent in that exact same startup window + // queues normally and just waits. + // + // `sessionWrapperRef.current?.mode` (not the `session` + // variable in this closure, which is the lower-level + // ApiSessionClient without a `mode` field) is the actual + // OpencodeSession instance's mode — 'local' | 'remote', + // synced synchronously by onModeChange before either + // launcher starts (see AgentSessionBase). `undefined` + // (not yet set) falls through to the safe + // not-yet-supported default below, same as genuinely + // local mode. + // + // `mode !== 'remote'` alone isn't enough, though: + // `mode` stays 'remote' for the *entire* teardown + // window too (it only flips back to 'local' once the + // whole remote launcher has fully unwound), so without + // also checking `compactTeardownInProgress`, this would + // re-open queuing during exactly the window + // onLeavingRemote() exists to protect — a hostile-review + // sweep found this the first time this branch checked + // `mode` alone (see compactTeardownInProgress's + // declaration comment for the full distinction). + if (!compactSupported && (sessionWrapperRef.current?.mode !== 'remote' || compactTeardownInProgress)) { + if (localId) { + session.emitMessagesConsumed([localId], { clearQueuedThinkingGrace: true }); + } + session.sendAgentMessage({ + type: 'message', + message: '/compact is not yet supported in HAPI OpenCode sessions.', + id: randomUUID() + }); + sessionWrapperRef.current?.onThinkingChange(false); + return; + } + // No manual emitMessagesConsumed here (unlike the + // synchronous 'handled' branch below): `messageQueue` + // (== session.queue, wired in AgentSessionBase's + // constructor — see sessionBase.ts) already acks + // automatically at dequeue time via `onBatchConsumed`, + // exactly like any regular prompt — `collectBatch()` in + // MessageQueue2.ts calls it right after shifting an + // item off the queue, which for /compact happens in + // opencodeRemoteLauncher.ts's dequeue loop. An earlier + // version of this branch (from when /compact ran via a + // trigger function invoked directly from this chain, + // bypassing the queue entirely) called + // `session.emitMessagesConsumed([localId])` manually + // right here, before the item was even queued — that + // stopped mattering for FIFO ordering once /compact + // moved to `pushIsolated` below, but it kept firing the + // hub ack immediately regardless, which is what actually + // broke cancellation of an already-queued-but-not-yet- + // dequeued /compact: the hub marked it invoked the + // instant it was queued, so `cancelByLocalId` in + // `onCancelQueuedMessage` below always found it already + // gone from the queue and could never remove it before + // that premature ack landed. Removing the manual call + // lets the automatic dequeue-time ack (and therefore + // `messageQueue.cancelByLocalId`) work the same way it + // already does for prompts — a queued /compact can now + // actually be cancelled before opencodeRemoteLauncher.ts + // dequeues it and calls `runCompactOperation()`. + // + // pushIsolated (not push): must never batch with a + // sibling prompt, but must still occupy its real FIFO + // position relative to prompts already queued ahead of + // it. + messageQueue.pushIsolated('', { ...buildMode(), operation: 'compact' }, localId); + return; + } + if (slash.kind !== 'passthrough') { if (slash.updates) { if (slash.updates.permissionMode !== undefined) { @@ -246,7 +407,20 @@ export async function runOpencode(opts: { logger.debug(`[opencode] cancelByLocalId(${localId}): marked for cancellation before enqueue`); return true; } - logger.debug(`[opencode] cancelByLocalId(${localId}): not found (best-effort)`); + // Not in the queue and not in the pre-enqueue preparing window. As + // explained where `cancelledDequeuedLocalIds` is declared above, the + // hub only calls this at all while its own row is still queued, so + // reaching this branch means we're in the brief race between our + // /compact ack (`emitMessagesConsumed`) being sent and the hub + // recording it — not, as the name might suggest, the compact REST + // call itself running. Remember it so the launcher can suppress the + // result if that's what this turns out to be; harmless if it doesn't + // match anything (just an unread entry that never gets consumed). + // Return value is unchanged from before this tracking existed — we + // don't actually know whether this cancelled anything real, so this + // stays "best-effort: not found". + addCancelledDequeuedLocalId(localId); + logger.debug(`[opencode] cancelByLocalId(${localId}): not found in queue; marked in case it lands in the compact ack race window (best-effort)`); return false; }); @@ -270,6 +444,7 @@ export async function runOpencode(opts: { }); let crashed = false; + const notifyHubModeChange = createModeChangeHandler(session); try { await opencodeLoop({ @@ -285,14 +460,38 @@ export async function runOpencode(opts: { resumeSessionId: opts.resumeSessionId, hookServer, hookUrl, - onModeChange: createModeChangeHandler(session), + onModeChange: (mode) => { + if (mode === 'remote') { + // A fresh remote entry is beginning (first-ever, or a + // local interlude ending) — whatever the previous + // remote attempt's teardown state was, it no longer + // applies. (The very first entry into remote mode, + // when `startingMode` is already 'remote', never calls + // onModeChange at all — see loopBase.ts — but this + // flag's initial `false` already covers that case.) + compactTeardownInProgress = false; + } + notifyHubModeChange(mode); + }, onReasoningEffortRollback: (effort) => { sessionModelReasoningEffort = effort; }, onSessionReady: (instance) => { sessionWrapperRef.current = instance; syncSessionMode(); - } + }, + onCompactAvailabilityChange: (available) => { + compactSupported = available; + if (!available) { + // onCompactAvailabilityChange(false) only ever fires + // from OpencodeRemoteLauncher's onLeavingRemote() (the + // old reset-on-next-local-entry was removed — see its + // declaration comment) — so reaching here always means + // "leaving remote", never "not ready yet". + compactTeardownInProgress = true; + } + }, + isLocalIdCancelled: (localId) => cancelledDequeuedLocalIds.delete(localId) }); } catch (error) { crashed = true; diff --git a/cli/src/opencode/types.ts b/cli/src/opencode/types.ts index 29283de2..0382c5cc 100644 --- a/cli/src/opencode/types.ts +++ b/cli/src/opencode/types.ts @@ -9,6 +9,14 @@ export interface OpencodeMode { // "no change requested for this batch". model?: string | null; modelReasoningEffort?: string | null; + // Marks this queued item as a /compact request rather than a regular + // prompt turn. Pushed via `messageQueue.pushIsolated(...)` so it never + // batches with sibling prompts but still occupies its actual FIFO + // position — the launcher's dequeue loop branches on this instead of + // calling `backend.prompt()`, which keeps /compact from "cutting in + // line" ahead of prompts that were already queued when it arrived. + // `undefined` for normal prompts. + operation?: 'compact'; } export type OpencodeHookEvent = { diff --git a/cli/src/opencode/utils/opencodeBackend.test.ts b/cli/src/opencode/utils/opencodeBackend.test.ts new file mode 100644 index 00000000..15b70d6d --- /dev/null +++ b/cli/src/opencode/utils/opencodeBackend.test.ts @@ -0,0 +1,55 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const constructorCalls: Array<{ command: string; args?: string[]; env?: Record }> = []; + +vi.mock('@/agent/backends/acp', () => ({ + AcpSdkBackend: vi.fn().mockImplementation(function ( + this: unknown, + opts: { command: string; args?: string[]; env?: Record } + ) { + constructorCalls.push(opts); + return { __opts: opts }; + }) +})); + +import { allocateFreePort, createOpencodeBackend } from './opencodeBackend'; + +describe('allocateFreePort', () => { + it('resolves a bindable loopback port number', async () => { + const port = await allocateFreePort('127.0.0.1'); + expect(typeof port).toBe('number'); + expect(Number.isInteger(port)).toBe(true); + expect(port).toBeGreaterThan(0); + expect(port).toBeLessThan(65536); + }); + + it('releases the port so it can be reused by a later caller', async () => { + // Each call must close its probe socket before resolving, otherwise a + // consumer that immediately tries to bind the returned port (e.g. the + // spawned `opencode acp --port ` process) would collide with it. + const first = await allocateFreePort('127.0.0.1'); + const second = await allocateFreePort('127.0.0.1'); + expect(typeof second).toBe('number'); + // Not asserting first !== second (OS may reuse immediately-freed ports), + // only that a second allocation does not hang or throw EADDRINUSE. + expect(first).toBeGreaterThan(0); + }); +}); + +describe('createOpencodeBackend', () => { + beforeEach(() => { + constructorCalls.length = 0; + }); + + it('passes --port and --hostname args when provided', () => { + createOpencodeBackend({ cwd: '/tmp/x', port: 5555, hostname: '127.0.0.1' }); + expect(constructorCalls[0]?.args).toEqual([ + 'acp', '--cwd', '/tmp/x', '--port', '5555', '--hostname', '127.0.0.1' + ]); + }); + + it('omits --port/--hostname when not provided (backward compatible)', () => { + createOpencodeBackend({ cwd: '/tmp/x' }); + expect(constructorCalls[0]?.args).toEqual(['acp', '--cwd', '/tmp/x']); + }); +}); diff --git a/cli/src/opencode/utils/opencodeBackend.ts b/cli/src/opencode/utils/opencodeBackend.ts index 7776cd6c..45fc774b 100644 --- a/cli/src/opencode/utils/opencodeBackend.ts +++ b/cli/src/opencode/utils/opencodeBackend.ts @@ -1,3 +1,4 @@ +import { createServer } from 'node:net'; import { AcpSdkBackend } from '@/agent/backends/acp'; import { buildOpencodeEnv } from './config'; import { getInvokedCwd } from '@/utils/invokedCwd'; @@ -12,11 +13,52 @@ function filterEnv(env: NodeJS.ProcessEnv): Record { return result; } +/** + * Reserves a free TCP port on the given loopback host by binding an + * ephemeral probe socket (`listen(0)`) and immediately closing it before + * resolving. `opencode acp` does not announce the port it actually bound + * when launched with `--port 0` (verified 2026-07-30 — no port appears in + * stdout/stderr even at DEBUG log level), so HAPI must pick the port itself + * and hand it to the subprocess explicitly via `--port`. There is a + * theoretical reuse race between this function releasing the port and the + * subprocess binding it, but both sides are loopback-only local processes, + * so the practical risk is low. + */ +export function allocateFreePort(hostname = '127.0.0.1'): Promise { + return new Promise((resolve, reject) => { + const probe = createServer(); + probe.once('error', reject); + probe.listen(0, hostname, () => { + const address = probe.address(); + const port = address && typeof address === 'object' ? address.port : null; + probe.close((closeError) => { + if (closeError) { + reject(closeError); + return; + } + if (port === null) { + reject(new Error('Failed to allocate a free port for the OpenCode ACP server')); + return; + } + resolve(port); + }); + }); + }); +} + export function createOpencodeBackend(opts: { cwd?: string; + port?: number; + hostname?: string; }): AcpSdkBackend { const env = buildOpencodeEnv(); const args = ['acp', '--cwd', opts.cwd ?? getInvokedCwd()]; + if (opts.port !== undefined) { + args.push('--port', String(opts.port)); + } + if (opts.hostname !== undefined) { + args.push('--hostname', opts.hostname); + } return new AcpSdkBackend({ command: 'opencode', diff --git a/cli/src/opencode/utils/opencodeCompactBridge.test.ts b/cli/src/opencode/utils/opencodeCompactBridge.test.ts new file mode 100644 index 00000000..04482621 --- /dev/null +++ b/cli/src/opencode/utils/opencodeCompactBridge.test.ts @@ -0,0 +1,418 @@ +import { describe, expect, it, vi } from 'vitest'; +import { fetchCompactionSummary, splitProviderModel, triggerOpencodeCompact } from './opencodeCompactBridge'; + +// `signal` is a required field (see OpencodeCompactCallOpts's doc comment) — +// most tests below don't exercise abort behavior at all, so this is a +// signal that's simply never aborted, just satisfying the type. +const noSignal = new AbortController().signal; + +describe('splitProviderModel', () => { + it('splits a combined "provider/model" wire id on the first slash', () => { + expect(splitProviderModel('ollama/qwen3.6:35b-a3b-q8_0-mtp')).toEqual({ + providerId: 'ollama', + modelId: 'qwen3.6:35b-a3b-q8_0-mtp' + }); + }); + + it('keeps everything after the first slash as the modelId (model ids may contain slashes)', () => { + expect(splitProviderModel('openrouter/anthropic/claude-sonnet-4-5')).toEqual({ + providerId: 'openrouter', + modelId: 'anthropic/claude-sonnet-4-5' + }); + }); + + it('returns null for null/undefined input', () => { + expect(splitProviderModel(null)).toBeNull(); + expect(splitProviderModel(undefined)).toBeNull(); + }); + + it('returns null when there is no slash', () => { + expect(splitProviderModel('no-slash-here')).toBeNull(); + }); + + it('returns null for a leading or trailing slash (empty provider or model)', () => { + expect(splitProviderModel('/model-only')).toBeNull(); + expect(splitProviderModel('provider-only/')).toBeNull(); + }); +}); + +describe('triggerOpencodeCompact', () => { + it('posts to /session/:id/summarize with the required providerID/modelID payload and no artificial timeout', async () => { + const fetchImpl = vi.fn(async (url: string, init?: RequestInit) => { + expect(url).toBe('http://127.0.0.1:48273/session/ses_abc/summarize'); + expect(init?.method).toBe('POST'); + expect(JSON.parse(init?.body as string)).toEqual({ + providerID: 'ollama', + modelID: 'qwen3.6:35b-a3b-q8_0-mtp' + }); + // `signal` is always attached now (required — see + // OpencodeCompactCallOpts), but it must not act as a deadline on + // its own: the caller here never aborts it, so the request must + // run to completion regardless of how long it legitimately takes + // (90s+ verified against SER8, 2026-07-30). + expect(init?.signal).toBe(noSignal); + // Bun's global fetch() hardcodes a 5-minute idle timeout that + // fires even with no AbortSignal at all (verified via isolated + // E2E against SER8, 2026-07-30 — a real ~250s compaction call + // failed with "The operation timed out"; see oven-sh/bun#16682). + // The only documented workaround is this Bun-specific, + // non-standard `timeout: false` fetch option — needed regardless + // of whether the caller's own `signal` ever fires. + expect((init as unknown as { timeout?: boolean })?.timeout).toBe(false); + return new Response(null, { status: 204 }); + }); + + const result = await triggerOpencodeCompact({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + providerId: 'ollama', + modelId: 'qwen3.6:35b-a3b-q8_0-mtp', + fetchImpl, + signal: noSignal + }); + + expect(result).toEqual({ ok: true }); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + + it('reports a structured failure when the server responds non-ok (e.g. the v2 compact stub 503)', async () => { + const fetchImpl = vi.fn(async () => new Response( + JSON.stringify({ _tag: 'ServiceUnavailableError', message: 'Session compact is not available yet', service: 'session.compact' }), + { status: 503 } + )); + + const result = await triggerOpencodeCompact({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + providerId: 'ollama', + modelId: 'qwen3.6:35b-a3b-q8_0-mtp', + fetchImpl, + signal: noSignal + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toContain('503'); + expect(result.error).toContain('not available yet'); + } + }); + + it('reports a structured failure when the network call throws', async () => { + const fetchImpl = vi.fn(async () => { + throw new Error('ECONNREFUSED'); + }); + + const result = await triggerOpencodeCompact({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + providerId: 'ollama', + modelId: 'qwen3.6:35b-a3b-q8_0-mtp', + fetchImpl, + signal: noSignal + }); + + expect(result).toEqual({ ok: false, error: 'ECONNREFUSED' }); + }); + + it('URL-encodes the sessionId in the path', async () => { + const fetchImpl = vi.fn(async (url: string) => { + expect(url).toBe('http://127.0.0.1:48273/session/ses%20with%20space/summarize'); + return new Response(null, { status: 204 }); + }); + + await triggerOpencodeCompact({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses with space', + providerId: 'ollama', + modelId: 'model-x', + fetchImpl, + signal: noSignal + }); + + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + + it('forwards an AbortSignal to fetch when provided, so a caller can interrupt an in-flight request', async () => { + const controller = new AbortController(); + const fetchImpl = vi.fn(async (_url: string, init?: RequestInit) => { + expect(init?.signal).toBe(controller.signal); + return new Response(null, { status: 204 }); + }); + + const result = await triggerOpencodeCompact({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + providerId: 'ollama', + modelId: 'qwen3.6:35b-a3b-q8_0-mtp', + fetchImpl, + signal: controller.signal + }); + + expect(result).toEqual({ ok: true }); + }); + + it('resolves with a structured failure (not a hang or uncaught rejection) when the signal aborts mid-request', async () => { + const controller = new AbortController(); + // Mirrors how a real fetch() rejects on abort: the promise only + // settles once the signal actually fires, not before. + const fetchImpl = vi.fn((_url: string, init?: RequestInit) => new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => { + reject(new DOMException('The operation was aborted.', 'AbortError')); + }); + })); + + const resultPromise = triggerOpencodeCompact({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + providerId: 'ollama', + modelId: 'qwen3.6:35b-a3b-q8_0-mtp', + fetchImpl, + signal: controller.signal + }); + + controller.abort(); + const result = await resultPromise; + + expect(result.ok).toBe(false); + }); +}); + +describe('fetchCompactionSummary', () => { + it('extracts the text part of the assistant message that follows the compaction marker (matched via parentID)', async () => { + const fetchImpl = vi.fn(async (url: string) => { + expect(url).toBe('http://127.0.0.1:48273/session/ses_abc/message'); + return new Response(JSON.stringify([ + { info: { id: 'msg_1', role: 'user' }, parts: [{ id: 'prt_1', type: 'text', text: 'hello' }] }, + { info: { id: 'msg_2', role: 'assistant' }, parts: [{ id: 'prt_2', type: 'text', text: 'hi there' }] }, + { info: { id: 'msg_3', role: 'user' }, parts: [{ id: 'prt_3', type: 'compaction', auto: false }] }, + { + info: { id: 'msg_4', role: 'assistant', parentID: 'msg_3', summary: true }, + parts: [ + { id: 'prt_4a', type: 'step-start' }, + { id: 'prt_4b', type: 'reasoning', text: 'thinking about the summary' }, + { id: 'prt_4c', type: 'text', text: '## Objective\n- Did the thing' }, + { id: 'prt_4d', type: 'step-finish' } + ] + } + ]), { status: 200 }); + }); + + const result = await fetchCompactionSummary({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + fetchImpl, + signal: noSignal + }); + + expect(result).toEqual({ found: true, text: '## Objective\n- Did the thing' }); + }); + + it('falls back to positional adjacency when the assistant message has no parentID', async () => { + const fetchImpl = vi.fn(async () => new Response(JSON.stringify([ + { info: { id: 'msg_3', role: 'user' }, parts: [{ id: 'prt_3', type: 'compaction', auto: false }] }, + { info: { id: 'msg_4', role: 'assistant' }, parts: [{ id: 'prt_4', type: 'text', text: 'summary via positional match' }] } + ]), { status: 200 })); + + const result = await fetchCompactionSummary({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + fetchImpl, + signal: noSignal + }); + + expect(result).toEqual({ found: true, text: 'summary via positional match' }); + }); + + it('rejects a parentID/positional match whose role is not assistant, even if it happens to carry a text part', async () => { + // Both the parentID-linked entry AND the positionally-adjacent entry + // have a `type:'text'` part here, but neither is role:'assistant' — + // the safe fallback (found:false) must win rather than surfacing + // whatever unrelated text these entries happen to carry. + const fetchImpl = vi.fn(async () => new Response(JSON.stringify([ + { info: { id: 'msg_3', role: 'user' }, parts: [{ id: 'prt_3', type: 'compaction', auto: false }] }, + { info: { id: 'msg_4', role: 'user', parentID: 'msg_3' }, parts: [{ id: 'prt_4', type: 'text', text: 'not actually a summary' }] } + ]), { status: 200 })); + + const result = await fetchCompactionSummary({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + fetchImpl, + signal: noSignal + }); + + expect(result).toEqual({ found: false }); + }); + + it('concatenates multiple text parts in order instead of only taking the first', async () => { + const fetchImpl = vi.fn(async () => new Response(JSON.stringify([ + { info: { id: 'msg_3', role: 'user' }, parts: [{ id: 'prt_3', type: 'compaction', auto: false }] }, + { + info: { id: 'msg_4', role: 'assistant', parentID: 'msg_3' }, + parts: [ + { id: 'prt_4a', type: 'text', text: '## Objective\n' }, + { id: 'prt_4b', type: 'step-finish' }, + { id: 'prt_4c', type: 'text', text: '- Did the thing' } + ] + } + ]), { status: 200 })); + + const result = await fetchCompactionSummary({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + fetchImpl, + signal: noSignal + }); + + expect(result).toEqual({ found: true, text: '## Objective\n- Did the thing' }); + }); + + it('returns found:false when no compaction marker exists', async () => { + const fetchImpl = vi.fn(async () => new Response(JSON.stringify([ + { info: { id: 'msg_1', role: 'user' }, parts: [{ id: 'prt_1', type: 'text', text: 'hello' }] }, + { info: { id: 'msg_2', role: 'assistant' }, parts: [{ id: 'prt_2', type: 'text', text: 'hi' }] } + ]), { status: 200 })); + + const result = await fetchCompactionSummary({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + fetchImpl, + signal: noSignal + }); + + expect(result).toEqual({ found: false }); + }); + + it('returns found:false when the marker is the last message (no following assistant message yet)', async () => { + const fetchImpl = vi.fn(async () => new Response(JSON.stringify([ + { info: { id: 'msg_3', role: 'user' }, parts: [{ id: 'prt_3', type: 'compaction', auto: false }] } + ]), { status: 200 })); + + const result = await fetchCompactionSummary({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + fetchImpl, + signal: noSignal + }); + + expect(result).toEqual({ found: false }); + }); + + it('returns found:false when the following assistant message has no text part', async () => { + const fetchImpl = vi.fn(async () => new Response(JSON.stringify([ + { info: { id: 'msg_3', role: 'user' }, parts: [{ id: 'prt_3', type: 'compaction', auto: false }] }, + { info: { id: 'msg_4', role: 'assistant', parentID: 'msg_3' }, parts: [{ id: 'prt_4', type: 'step-finish' }] } + ]), { status: 200 })); + + const result = await fetchCompactionSummary({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + fetchImpl, + signal: noSignal + }); + + expect(result).toEqual({ found: false }); + }); + + it('returns found:false on a non-ok response', async () => { + const fetchImpl = vi.fn(async () => new Response(null, { status: 500 })); + + const result = await fetchCompactionSummary({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + fetchImpl, + signal: noSignal + }); + + expect(result).toEqual({ found: false }); + }); + + it('returns found:false when the response is not valid JSON / not an array', async () => { + const fetchImpl = vi.fn(async () => new Response('not json', { status: 200 })); + + const result = await fetchCompactionSummary({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + fetchImpl, + signal: noSignal + }); + + expect(result).toEqual({ found: false }); + }); + + it('returns found:false when the network call throws', async () => { + const fetchImpl = vi.fn(async () => { + throw new Error('ECONNREFUSED'); + }); + + const result = await fetchCompactionSummary({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + fetchImpl, + signal: noSignal + }); + + expect(result).toEqual({ found: false }); + }); + + it('picks the LAST compaction marker when there are multiple (a session may be compacted more than once)', async () => { + const fetchImpl = vi.fn(async () => new Response(JSON.stringify([ + { info: { id: 'msg_1', role: 'user' }, parts: [{ id: 'prt_1', type: 'compaction', auto: false }] }, + { info: { id: 'msg_2', role: 'assistant', parentID: 'msg_1' }, parts: [{ id: 'prt_2', type: 'text', text: 'first summary' }] }, + { info: { id: 'msg_3', role: 'user' }, parts: [{ id: 'prt_3', type: 'text', text: 'more chat' }] }, + { info: { id: 'msg_4', role: 'user' }, parts: [{ id: 'prt_4', type: 'compaction', auto: false }] }, + { info: { id: 'msg_5', role: 'assistant', parentID: 'msg_4' }, parts: [{ id: 'prt_5', type: 'text', text: 'second summary' }] } + ]), { status: 200 })); + + const result = await fetchCompactionSummary({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + fetchImpl, + signal: noSignal + }); + + expect(result).toEqual({ found: true, text: 'second summary' }); + }); + + it('forwards an AbortSignal to fetch when provided, so a caller can interrupt an in-flight request', async () => { + const controller = new AbortController(); + const fetchImpl = vi.fn(async (_url: string, init?: RequestInit) => { + expect(init?.signal).toBe(controller.signal); + return new Response(JSON.stringify([]), { status: 200 }); + }); + + await fetchCompactionSummary({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + fetchImpl, + signal: controller.signal + }); + + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + + it('resolves with found:false (not a hang or uncaught rejection) when the signal aborts mid-request', async () => { + // Reproduces the exact gap a PR-review round found: the POST + // (triggerOpencodeCompact) had a signal wired through in an earlier + // round, but this GET — which runs right after it inside + // runCompactOperation() — did not, so Stop/switch-to-local could + // still block on this call even after that fix. + const controller = new AbortController(); + const fetchImpl = vi.fn((_url: string, init?: RequestInit) => new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => { + reject(new DOMException('The operation was aborted.', 'AbortError')); + }); + })); + + const resultPromise = fetchCompactionSummary({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + fetchImpl, + signal: controller.signal + }); + + controller.abort(); + const result = await resultPromise; + + expect(result).toEqual({ found: false }); + }); +}); diff --git a/cli/src/opencode/utils/opencodeCompactBridge.ts b/cli/src/opencode/utils/opencodeCompactBridge.ts new file mode 100644 index 00000000..3e288410 --- /dev/null +++ b/cli/src/opencode/utils/opencodeCompactBridge.ts @@ -0,0 +1,221 @@ +export type OpencodeCompactResult = + | { ok: true; summaryText?: string } + | { ok: false; error: string }; + +export type CompactionSummaryResult = + | { found: true; text: string } + | { found: false }; + +/** Minimal fetch-shaped function signature, kept narrower than `typeof fetch` so tests can pass a plain `vi.fn()` without matching runtime-specific extras (e.g. Bun's `fetch.preconnect`). */ +export type FetchLike = (url: string, init?: RequestInit) => Promise; + +/** + * `RequestInit` extended with Bun's non-standard `timeout` fetch option + * (absent from `bun-types` / the standard fetch typings — see the + * `triggerOpencodeCompact` doc comment for why it's needed). Narrower than + * `Record` so the cast below can't silently accept an + * unrelated typo'd option name. + */ +type BunFetchInit = RequestInit & { timeout?: false }; + +/** + * Every OpenCode-side HTTP call `runCompactOperation()` in + * opencodeRemoteLauncher.ts makes on behalf of a single /compact operation + * must extend this — `signal` is **required**, not optional. That launcher + * owns exactly one `AbortController` for the operation's whole lifecycle + * (`compactAbortController`, aborted by `handleAbort()` on Stop/switch/exit) + * and threads its `.signal` through every step so a user-initiated + * interruption actually reaches whichever HTTP call happens to be in flight + * at the time. + * + * This was previously opt-in (`signal?: AbortSignal`) on each function + * individually, which is exactly how a real regression happened: a second + * PR-review round later found that `triggerOpencodeCompact` (the POST) had + * been wired up but `fetchCompactionSummary` (the GET that runs right + * after it) had not, because nothing forced it. Making `signal` a required + * field of a shared base type means the compiler catches a future third + * HTTP step *implemented as a function in this file* without one — it can't + * stop someone from bypassing this file entirely with an inline `fetch()` + * call in `runCompactOperation()`, so this is a guardrail for the pattern + * this file establishes, not an architectural boundary enforced repo-wide. + */ +export type OpencodeCompactCallOpts = { + baseUrl: string; + sessionId: string; + signal: AbortSignal; +}; + +/** + * Splits an ACP-reported combined model id (e.g. `"ollama/qwen3.6:35b-a3b-q8_0-mtp"`) + * into the separate `providerId`/`modelId` pair required by OpenCode's internal + * `POST /session/:id/summarize` payload. Only the first `/` is treated as the + * separator — model ids may themselves contain slashes (e.g. OpenRouter-style + * `"openrouter/anthropic/claude-sonnet-4-5"`). + */ +export function splitProviderModel(combined: string | null | undefined): { providerId: string; modelId: string } | null { + if (!combined) return null; + const separatorIndex = combined.indexOf('/'); + if (separatorIndex <= 0 || separatorIndex === combined.length - 1) return null; + return { + providerId: combined.slice(0, separatorIndex), + modelId: combined.slice(separatorIndex + 1) + }; +} + +/** + * Triggers OpenCode's native AI-compaction for a session by calling the + * legacy `POST /session/:id/summarize` route on the `opencode acp` + * subprocess's internal HTTP API. + * + * This is NOT `POST /api/session/:id/compact` — that v2-API route is an + * unimplemented stub in opencode 1.18.9 and always returns 503 + * ("Session compact is not available yet"). `summarize` is the route that + * actually performs native AI compaction (verified 2026-07-30: triggering it + * appends a real `{"type":"compaction"}` message part to the session, and + * streams `agent_thought_chunk` ACP notifications while the model works). + * + * `providerID`/`modelID` are required by the endpoint (400 if omitted). + * The response can legitimately take several minutes to arrive for slow + * models, so by default no deadline is applied here, mirroring how + * `AcpSdkBackend.prompt()` uses `timeoutMs: Infinity` for `session/prompt`. + * `signal` (required — see `OpencodeCompactCallOpts`) is a caller-driven + * abort, not a deadline, so it's orthogonal to the Bun timeout workaround + * below (both apply at once). + * + * Omitting the `timeout: false` option below is NOT enough under Bun: Bun's + * global `fetch()` hardcodes its own idle timeout (~5 minutes) that fires + * independently of any AbortSignal (verified 2026-07-30 via isolated E2E + * against SER8 — a real ~250s compaction call was killed client-side with + * "The operation timed out" even with no signal attached; see upstream + * report oven-sh/bun#16682). The only documented workaround is the + * non-standard `timeout: false` fetch option Bun itself recognizes (absent + * from the standard `RequestInit` typings, hence the cast below) — kept + * unconditionally regardless of `signal` also being set. + */ +export async function triggerOpencodeCompact(opts: OpencodeCompactCallOpts & { + providerId: string; + modelId: string; + fetchImpl?: FetchLike; +}): Promise { + const fetchFn: FetchLike = opts.fetchImpl ?? fetch; + const url = `${opts.baseUrl}/session/${encodeURIComponent(opts.sessionId)}/summarize`; + + try { + const init: BunFetchInit = { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ providerID: opts.providerId, modelID: opts.modelId }), + // Bun-specific: disables Bun's hardcoded ~5min fetch timeout. + timeout: false, + signal: opts.signal + }; + const response = await fetchFn(url, init as RequestInit); + + if (!response.ok) { + const text = await response.text().catch(() => ''); + return { + ok: false, + error: `OpenCode compact request failed (${response.status}): ${text.slice(0, 300)}` + }; + } + + return { ok: true }; + } catch (error) { + return { + ok: false, + error: error instanceof Error ? error.message : String(error) + }; + } +} + +type OpencodeMessagePart = { type?: unknown; text?: unknown }; +type OpencodeMessageEntry = { + info?: { id?: unknown; role?: unknown; parentID?: unknown; summary?: unknown }; + parts?: unknown; +}; + +function isObjectRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null; +} + +/** + * Only an assistant message is a plausible summary carrier — without this + * check, an unrelated adjacent/linked entry that happens to carry a `text` + * part (e.g. another user message) could silently surface as the "summary", + * bypassing the safe "not found -> skip" fallback this function exists to + * provide. `info.summary === true` (observed on the real compaction + * response) is a stronger corroborating signal when present, but role is the + * one check we always enforce. + */ +function isAssistantSummaryCandidate(entry: OpencodeMessageEntry | undefined): boolean { + return entry?.info?.role === 'assistant'; +} + +/** Concatenates every `type:'text'` part in order — a summary can arrive as more than one text segment, and taking only the first would silently truncate it. */ +function extractTextPart(entry: OpencodeMessageEntry | undefined): string | null { + if (!entry || !Array.isArray(entry.parts)) return null; + const texts = (entry.parts as unknown[]) + .filter((part): part is OpencodeMessagePart => isObjectRecord(part) && part.type === 'text' && typeof part.text === 'string') + .map((part) => part.text as string); + return texts.length > 0 ? texts.join('') : null; +} + +/** + * After a successful `triggerOpencodeCompact`, OpenCode's session history + * contains a `{"type":"compaction"}` marker message (role `user`, no text) + * followed by an assistant message whose `text` part holds the actual + * summary OpenCode generated (verified 2026-07-30 via isolated E2E: parts + * were `['step-start','reasoning','text','step-finish']`). This fetches the + * message list and extracts that text so HAPI can show it as a "Reasoning" + * block instead of leaving the summary invisible. + * + * Looks for the assistant message via its `parentID` pointing at the marker + * first (robust to the API returning messages in an order other than + * creation order), falling back to simple positional adjacency (the very + * next array entry) if no `parentID` link is present. If a session has been + * compacted more than once, only the most recent marker is considered. + * + * Never throws — any failure (network error, unexpected response shape, no + * marker found, no text part found, or `signal` — see `OpencodeCompactCallOpts` + * — firing mid-request) resolves to `{ found: false }` so the caller can + * silently skip showing the summary rather than surfacing an error for what + * is a purely cosmetic enhancement. + */ +export async function fetchCompactionSummary(opts: OpencodeCompactCallOpts & { + fetchImpl?: FetchLike; +}): Promise { + const fetchFn: FetchLike = opts.fetchImpl ?? fetch; + const url = `${opts.baseUrl}/session/${encodeURIComponent(opts.sessionId)}/message`; + + try { + const response = await fetchFn(url, { method: 'GET', signal: opts.signal }); + if (!response.ok) return { found: false }; + + const data: unknown = await response.json().catch(() => null); + if (!Array.isArray(data)) return { found: false }; + const entries = data as OpencodeMessageEntry[]; + + let markerIndex = -1; + for (let i = entries.length - 1; i >= 0; i--) { + const parts = entries[i]?.parts; + if (Array.isArray(parts) && parts.some((part) => isObjectRecord(part) && part.type === 'compaction')) { + markerIndex = i; + break; + } + } + if (markerIndex === -1) return { found: false }; + + const markerId = entries[markerIndex]?.info?.id; + const byParentId = typeof markerId === 'string' + ? entries.find((entry) => entry.info?.parentID === markerId && isAssistantSummaryCandidate(entry)) + : undefined; + + const positionalCandidate = entries[markerIndex + 1]; + const byPosition = isAssistantSummaryCandidate(positionalCandidate) ? positionalCandidate : undefined; + + const text = extractTextPart(byParentId) ?? extractTextPart(byPosition); + return text !== null ? { found: true, text } : { found: false }; + } catch { + return { found: false }; + } +} diff --git a/cli/src/opencode/utils/slashCommands.test.ts b/cli/src/opencode/utils/slashCommands.test.ts index fe942445..75e049b6 100644 --- a/cli/src/opencode/utils/slashCommands.test.ts +++ b/cli/src/opencode/utils/slashCommands.test.ts @@ -119,15 +119,19 @@ describe('resolveOpencodeSlashCommand', () => { } }); - it('returns a not-yet-supported message for /clear and /compact', () => { + it('returns a not-yet-supported message for /clear', () => { expect(resolveOpencodeSlashCommand('/clear', state)).toEqual({ kind: 'handled', message: '/clear is not yet supported in HAPI OpenCode sessions.' }); - expect(resolveOpencodeSlashCommand('/compact', state)).toEqual({ - kind: 'handled', - message: '/compact is not yet supported in HAPI OpenCode sessions.' - }); + }); + + it('resolves /compact to a dedicated kind so the launcher can bridge to native compaction asynchronously', () => { + expect(resolveOpencodeSlashCommand('/compact', state)).toEqual({ kind: 'compact' }); + }); + + it('resolves /compact the same way regardless of trailing arguments (compaction takes no arguments)', () => { + expect(resolveOpencodeSlashCommand('/compact now please', state)).toEqual({ kind: 'compact' }); }); it('expands custom OpenCode command prompts', () => { @@ -163,6 +167,8 @@ describe('resolveOpencodeSlashCommand', () => { expect(help.message).toContain('Supported OpenCode slash commands'); expect(help.message).toContain('/plan'); expect(help.message).toContain('/permissions'); + expect(help.message).toContain('/compact` — compact (summarize) the OpenCode session context (remote sessions only)'); + expect(help.message).toContain('/clear` is not yet supported'); } }); diff --git a/cli/src/opencode/utils/slashCommands.ts b/cli/src/opencode/utils/slashCommands.ts index 8a49fc98..33230105 100644 --- a/cli/src/opencode/utils/slashCommands.ts +++ b/cli/src/opencode/utils/slashCommands.ts @@ -18,6 +18,12 @@ const OPENCODE_INIT_PROMPT = [ export type OpencodeSlashResolution = | { kind: 'passthrough' } + // /compact needs an async round trip to OpenCode's internal REST API + // (native AI compaction, can take 90s+) and a "Compaction + // started/completed/failed" event sequence, which doesn't fit the + // synchronous 'handled' shape below. The launcher (runOpencode.ts) + // intercepts this kind and drives that flow itself. + | { kind: 'compact' } | { kind: 'handled'; message: string; @@ -163,7 +169,11 @@ export function resolveOpencodeSlashCommand( }; } - if (command === 'clear' || command === 'compact') { + if (command === 'compact') { + return { kind: 'compact' }; + } + + if (command === 'clear') { return { kind: 'handled', message: `/${command} is not yet supported in HAPI OpenCode sessions.` @@ -193,11 +203,12 @@ export function resolveOpencodeSlashCommand( '- `/plan off` — return to default permission mode', '- `/default` — return to default permission mode', '- `/init [extra]` — generate or refresh AGENTS.md for this project', + '- `/compact` — compact (summarize) the OpenCode session context (remote sessions only)', '', 'Model, reasoning effort, and permission mode have dedicated buttons in the composer. ' + 'You can still type `/model`, `/reasoning`, or `/permissions` if you prefer.', '', - '`/clear` and `/compact` are not yet supported in HAPI OpenCode sessions.', + '`/clear` is not yet supported in HAPI OpenCode sessions.', '', 'Custom commands from `~/.config/opencode/command` or `.opencode/command` are expanded before sending.' ].join('\n')