From c3bed919b8e030ea0fe788d1f85dd1024f5ff5fe Mon Sep 17 00:00:00 2001 From: Junmo Kim Date: Tue, 4 Aug 2026 12:15:42 +0900 Subject: [PATCH] fix(opencode): verify persisted compaction results (#1357) --- .../agent/backends/acp/AcpSdkBackend.test.ts | 82 +++++ .../opencode/opencodeRemoteLauncher.test.ts | 272 +++++++++++---- cli/src/opencode/opencodeRemoteLauncher.ts | 181 ++++++---- .../utils/opencodeCompactBridge.test.ts | 321 +++++++----------- .../opencode/utils/opencodeCompactBridge.ts | 196 +++++++---- 5 files changed, 647 insertions(+), 405 deletions(-) diff --git a/cli/src/agent/backends/acp/AcpSdkBackend.test.ts b/cli/src/agent/backends/acp/AcpSdkBackend.test.ts index 1c0e442d..4fcdb2de 100644 --- a/cli/src/agent/backends/acp/AcpSdkBackend.test.ts +++ b/cli/src/agent/backends/acp/AcpSdkBackend.test.ts @@ -1272,4 +1272,86 @@ describe('AcpSdkBackend', () => { emitPlanUpdate(); expect(turn1.some((m) => m.type === 'plan')).toBe(true); }); + + it('does not let compact thought/text chunks escape through the next prompt pre-swap drain, while preserving the new prompt response', async () => { + // The reported duplicate was not emitted during /compact itself. In + // the pre-suppression implementation those chunks stayed in the old + // handler and prompt()'s next pre-swap drain emitted them as an + // ordinary assistant reply. This drives that exact backend path. + backendStatics.UPDATE_QUIET_PERIOD_MS = 1; + backendStatics.UPDATE_DRAIN_TIMEOUT_MS = 20; + backendStatics.PRE_PROMPT_UPDATE_QUIET_PERIOD_MS = 1; + backendStatics.PRE_PROMPT_UPDATE_DRAIN_TIMEOUT_MS = 20; + backendStatics.LATE_FLUSH_INTERVAL_MS = 1; + backendStatics.LATE_FLUSH_QUIET_PERIOD_MS = 1; + backendStatics.LATE_FLUSH_WINDOW_MS = 20; + + const backend = new AcpSdkBackend({ command: 'opencode' }); + const backendInternal = backend as unknown as { + transport: { + sendRequest: (method: string, params: unknown, options?: unknown) => Promise; + close: () => Promise; + } | null; + handleSessionUpdate: (params: unknown) => void; + }; + let promptRequestCount = 0; + backendInternal.transport = { + sendRequest: async (method) => { + if (method === 'session/prompt') { + promptRequestCount += 1; + if (promptRequestCount === 2) { + backendInternal.handleSessionUpdate({ + sessionId: 'session-1', + update: { + sessionUpdate: ACP_SESSION_UPDATE_TYPES.agentThoughtChunk, + content: { type: 'text', text: 'new prompt thought' } + } + }); + backendInternal.handleSessionUpdate({ + sessionId: 'session-1', + update: { + sessionUpdate: ACP_SESSION_UPDATE_TYPES.agentMessageChunk, + content: { type: 'text', text: 'new prompt answer' } + } + }); + } + return { stopReason: 'end_turn' }; + } + return null; + }, + close: async () => {} + }; + + const previousTurn: AgentMessage[] = []; + await backend.prompt('session-1', [{ type: 'text', text: 'before compact' }], (message) => previousTurn.push(message)); + + await backend.suppressUpdatesDuring(async () => { + backendInternal.handleSessionUpdate({ + sessionId: 'session-1', + update: { + sessionUpdate: ACP_SESSION_UPDATE_TYPES.agentThoughtChunk, + content: { type: 'text', text: 'compact-only thought' } + } + }); + backendInternal.handleSessionUpdate({ + sessionId: 'session-1', + update: { + sessionUpdate: ACP_SESSION_UPDATE_TYPES.agentMessageChunk, + content: { type: 'text', text: 'compact-only summary' } + } + }); + }); + + const nextTurn: AgentMessage[] = []; + await backend.prompt('session-1', [{ type: 'text', text: 'after compact' }], (message) => nextTurn.push(message)); + + expect(previousTurn).toEqual([ + { type: 'turn_complete', stopReason: 'end_turn' } + ]); + expect(nextTurn).toEqual([ + { type: 'reasoning', text: 'new prompt thought' }, + { type: 'text', text: 'new prompt answer' }, + { type: 'turn_complete', stopReason: 'end_turn' } + ]); + }); }); diff --git a/cli/src/opencode/opencodeRemoteLauncher.test.ts b/cli/src/opencode/opencodeRemoteLauncher.test.ts index eeebed37..416f8c12 100644 --- a/cli/src/opencode/opencodeRemoteLauncher.test.ts +++ b/cli/src/opencode/opencodeRemoteLauncher.test.ts @@ -150,17 +150,28 @@ vi.mock('@/ui/ink/OpencodeDisplay', () => ({ const compactHarness = vi.hoisted(() => ({ calls: [] as Array<{ baseUrl: string; sessionId: string; providerId: string; modelId: string; signal?: AbortSignal }>, + operationEvents: [] as string[], 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 }, + markerSnapshotCalls: [] as Array<{ baseUrl: string; sessionId: string; signal?: AbortSignal }>, + markerSnapshot: { markerIds: ['before-this-request'] } as { markerIds: string[] } | null, + // Lets tests hold the pre-POST snapshot GET until plain Stop aborts it. + snapshotImpl: null as null | ((opts: { baseUrl: string; sessionId: string; signal?: AbortSignal }) => Promise<{ markerIds: string[] } | null>), + resultCalls: [] as Array<{ baseUrl: string; sessionId: string; markerIdsBefore: string[] | null; signal?: AbortSignal }>, + compactionResult: { status: 'success', text: '## Objective\n- Did the thing' } as + | { status: 'success'; text: string } + | { status: 'failed'; reason: string } + | { status: 'unverified'; reason: string }, // 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 }>) + // Same idea, for semantic-result GET that runs after a successful POST. + resultImpl: null as null | ((opts: { baseUrl: string; sessionId: string; markerIdsBefore: string[] | null; signal?: AbortSignal }) => Promise< + | { status: 'success'; text: string } + | { status: 'failed'; reason: string } + | { status: 'unverified'; reason: string } + >) })); vi.mock('./utils/opencodeCompactBridge', () => ({ @@ -171,18 +182,28 @@ vi.mock('./utils/opencodeCompactBridge', () => ({ 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.operationEvents.push('trigger'); 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); + captureCompactionMarkerSnapshot: vi.fn(async (opts: { baseUrl: string; sessionId: string; signal?: AbortSignal }) => { + compactHarness.operationEvents.push('snapshot'); + compactHarness.markerSnapshotCalls.push(opts); + if (compactHarness.snapshotImpl) { + return compactHarness.snapshotImpl(opts); } - return compactHarness.summaryResult; + return compactHarness.markerSnapshot; + }), + fetchCompactionResult: vi.fn(async (opts: { baseUrl: string; sessionId: string; markerIdsBefore: string[] | null; signal?: AbortSignal }) => { + compactHarness.operationEvents.push('result'); + compactHarness.resultCalls.push(opts); + if (compactHarness.resultImpl) { + return compactHarness.resultImpl(opts); + } + return compactHarness.compactionResult; }) })); @@ -330,11 +351,15 @@ describe('opencodeRemoteLauncher inline model switch', () => { harness.setConfigOptionImpl = null; harness.thoughtLevelOption = null; compactHarness.calls = []; + compactHarness.operationEvents = []; compactHarness.result = { ok: true }; - compactHarness.summaryCalls = []; - compactHarness.summaryResult = { found: false }; + compactHarness.markerSnapshotCalls = []; + compactHarness.markerSnapshot = { markerIds: ['before-this-request'] }; + compactHarness.snapshotImpl = null; + compactHarness.resultCalls = []; + compactHarness.compactionResult = { status: 'success', text: '## Objective\n- Did the thing' }; compactHarness.triggerImpl = null; - compactHarness.summaryImpl = null; + compactHarness.resultImpl = null; harness.promptImpl = null; harness.sessionModelsMetadata = undefined; harness.cancelPromptImpl = null; @@ -635,9 +660,14 @@ describe('opencodeRemoteLauncher inline model switch', () => { 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 () => { + it('runs the compact POST callback inside suppression before reporting a verified completion', async () => { const opencodeBackendModule = await import('./utils/opencodeBackend'); const factory = (opencodeBackendModule as unknown as { createOpencodeBackend: ReturnType }).createOpencodeBackend; + let inSuppressionCallback = false; + compactHarness.triggerImpl = async () => { + expect(inSuppressionCallback).toBe(true); + return { ok: true }; + }; factory.mockImplementationOnce(() => ({ initialize: vi.fn(async () => {}), newSession: vi.fn(async () => 'acp-session-1'), @@ -659,7 +689,14 @@ describe('opencodeRemoteLauncher inline model switch', () => { currentModelId: 'ollama/qwen3.6:35b-a3b-q8_0-mtp', availableModels: [] })), - suppressUpdatesDuring: vi.fn(async (fn: () => Promise): Promise => fn()) + suppressUpdatesDuring: vi.fn(async (fn: () => Promise): Promise => { + inSuppressionCallback = true; + try { + return await fn(); + } finally { + inSuppressionCallback = false; + } + }) })); const { session, sessionEvents } = createSessionStub([ @@ -668,6 +705,8 @@ describe('opencodeRemoteLauncher inline model switch', () => { await opencodeRemoteLauncher(session as never); + expect(compactHarness.operationEvents).toEqual(['snapshot', 'trigger', 'result']); + expect(compactHarness.calls).toEqual([ { baseUrl: 'http://127.0.0.1:48273', @@ -750,21 +789,101 @@ describe('opencodeRemoteLauncher inline model switch', () => { ]); }); - it('switch-to-local also interrupts an in-flight fetchCompactionSummary GET (not just the triggerOpencodeCompact POST)', async () => { + it('a plain Stop aborts the pre-POST marker snapshot GET and skips summarize because no server-side compaction has started', async () => { + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + let capturedSignal: AbortSignal | undefined; + compactHarness.snapshotImpl = (opts) => new Promise((resolve) => { + capturedSignal = opts.signal; + opts.signal?.addEventListener('abort', () => resolve(null)); + }); + + const { session, sessionEvents, rpcHandlers } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/x') } + ]); + const launcherPromise = opencodeRemoteLauncher(session as never, { + onCompactAvailabilityChange: () => {} + }); + + while (compactHarness.markerSnapshotCalls.length === 0) { + await new Promise((resolve) => setImmediate(resolve)); + } + expect(capturedSignal?.aborted).toBe(false); + + const abortHandler = rpcHandlers.get('abort') as (() => Promise) | undefined; + expect(abortHandler).toBeDefined(); + await Promise.race([ + abortHandler!(), + new Promise((_, reject) => setTimeout(() => reject(new Error('plain Stop did not settle during snapshot GET')), 2000)) + ]); + + expect(capturedSignal?.aborted).toBe(true); + expect(compactHarness.calls).toEqual([]); + expect(session.thinking).toBe(false); + expect(sessionEvents.filter((event) => event.type === 'message').map((event) => event.message)) + .toEqual(['📦 Compaction started']); + session.queue.close(); + await Promise.race([ + launcherPromise, + new Promise((_, reject) => setTimeout(() => reject(new Error('launcher did not finish after snapshot GET abort')), 2000)) + ]); + }); + + it('a plain Stop aborts the post-POST semantic-result GET because summarize has already completed', async () => { + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + let capturedSignal: AbortSignal | undefined; + compactHarness.resultImpl = (opts) => new Promise((resolve) => { + capturedSignal = opts.signal; + opts.signal?.addEventListener('abort', () => { + resolve({ status: 'unverified', reason: 'Compaction result could not be verified.' }); + }); + }); + + const { session, sessionEvents, rpcHandlers } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/x') } + ]); + const launcherPromise = opencodeRemoteLauncher(session as never, { + onCompactAvailabilityChange: () => {} + }); + + while (compactHarness.resultCalls.length === 0) { + await new Promise((resolve) => setImmediate(resolve)); + } + expect(capturedSignal?.aborted).toBe(false); + expect(compactHarness.calls).toHaveLength(1); + + const abortHandler = rpcHandlers.get('abort') as (() => Promise) | undefined; + expect(abortHandler).toBeDefined(); + await Promise.race([ + abortHandler!(), + new Promise((_, reject) => setTimeout(() => reject(new Error('plain Stop did not settle during result GET')), 2000)) + ]); + + expect(capturedSignal?.aborted).toBe(true); + expect(session.thinking).toBe(false); + expect(sessionEvents.filter((event) => event.type === 'message').map((event) => event.message)) + .toEqual(['📦 Compaction started']); + session.queue.close(); + await Promise.race([ + launcherPromise, + new Promise((_, reject) => setTimeout(() => reject(new Error('launcher did not finish after result GET abort')), 2000)) + ]); + }); + + it('switch-to-local also interrupts an in-flight semantic-result 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 + // The semantic-result 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) => { + compactHarness.resultImpl = (opts) => new Promise((resolve) => { capturedSignal = opts.signal; opts.signal?.addEventListener('abort', () => { - resolve({ found: false }); + resolve({ status: 'unverified', reason: 'Compaction result could not be verified.' }); }); }); @@ -778,7 +897,7 @@ describe('opencodeRemoteLauncher inline model switch', () => { // Wait until the summary GET is actually in flight (i.e. the POST // already resolved successfully). - while (compactHarness.summaryCalls.length === 0) { + while (compactHarness.resultCalls.length === 0) { await new Promise((resolve) => setImmediate(resolve)); } expect(capturedSignal?.aborted).toBe(false); @@ -958,8 +1077,8 @@ describe('opencodeRemoteLauncher inline model switch', () => { } }); - it('sends the fetched compaction summary as a reasoning-type agent message', async () => { - compactHarness.summaryResult = { found: true, text: '## Objective\n- Did the thing' }; + it('sends the verified compaction summary as a reasoning-type agent message', async () => { + compactHarness.compactionResult = { status: 'success', text: '## Objective\n- Did the thing' }; harness.sessionModelsMetadata = { currentModelId: 'ollama/qwen3.6:35b-a3b-q8_0-mtp', availableModels: [] }; const { session, sentAgentMessages } = createSessionStub([ @@ -968,14 +1087,61 @@ describe('opencodeRemoteLauncher inline model switch', () => { 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(compactHarness.resultCalls).toEqual([ + { + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'acp-session-1', + markerIdsBefore: ['before-this-request'], + signal: expect.any(AbortSignal) + } ]); expect(sentAgentMessages).toEqual([ { type: 'reasoning', message: '## Objective\n- Did the thing', id: expect.any(String) } ]); }); + it('does not report completion when HTTP success resolves to the observed empty terminal summary failure', async () => { + compactHarness.compactionResult = { + status: 'failed', + reason: 'OpenCode returned an empty compaction summary.' + }; + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + + const { session, sessionEvents, sentAgentMessages } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/x') } + ]); + + await opencodeRemoteLauncher(session as never); + + const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); + expect(messages).toEqual([ + '📦 Compaction started', + '📦 Compaction failed: OpenCode returned an empty compaction summary.' + ]); + expect(sentAgentMessages).toEqual([]); + }); + + it('reports an unverified result rather than optimistically completing when association is unavailable', async () => { + compactHarness.compactionResult = { + status: 'unverified', + reason: 'Compaction result could not be verified.' + }; + harness.sessionModelsMetadata = { currentModelId: 'ollama/x', availableModels: [] }; + + const { session, sessionEvents, sentAgentMessages } = createSessionStub([ + { message: '', mode: createCompactMode('ollama/x') } + ]); + + await opencodeRemoteLauncher(session as never); + + const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); + expect(messages).toEqual([ + '📦 Compaction started', + '📦 Compaction result could not be verified.' + ]); + expect(sentAgentMessages).toEqual([]); + }); + 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 @@ -1150,7 +1316,7 @@ describe('opencodeRemoteLauncher inline model switch', () => { await opencodeRemoteLauncher(session as never); - expect(compactHarness.summaryCalls).toEqual([]); + expect(compactHarness.resultCalls).toEqual([]); const messages = sessionEvents.filter((event) => event.type === 'message').map((event) => event.message); expect(messages).toEqual(['📦 Compaction started', '📦 Compaction failed: boom']); }); @@ -1200,80 +1366,40 @@ describe('opencodeRemoteLauncher inline model switch', () => { ]); }); - 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. + it('a switch-to-local firing during the inline model switch prevents the later snapshot/POST from starting', async () => { + // The controller is created before inline model switching so terminal + // exit is remembered. The cancellation check before the marker GET + // must then prevent any delayed compact HTTP work after the switch. 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.markerSnapshotCalls).toEqual([]); 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)) ]); + expect(compactHarness.markerSnapshotCalls).toEqual([]); + expect(compactHarness.calls).toEqual([]); }); 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 () => { diff --git a/cli/src/opencode/opencodeRemoteLauncher.ts b/cli/src/opencode/opencodeRemoteLauncher.ts index 5a72c743..0f657288 100644 --- a/cli/src/opencode/opencodeRemoteLauncher.ts +++ b/cli/src/opencode/opencodeRemoteLauncher.ts @@ -11,7 +11,7 @@ import type { OpencodeSession } from './session'; import type { OpencodeMode, PermissionMode } from './types'; import { RPC_METHODS } from '@hapi/protocol/rpcMethods'; import { allocateFreePort, createOpencodeBackend } from './utils/opencodeBackend'; -import { fetchCompactionSummary, splitProviderModel, triggerOpencodeCompact } from './utils/opencodeCompactBridge'; +import { captureCompactionMarkerSnapshot, fetchCompactionResult, 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'; @@ -45,6 +45,8 @@ export type AbortStatusDecision = { shouldClearThinking: boolean; }; +type CompactOperationPhase = 'idle' | 'snapshot' | 'summarize' | 'post-summarize' | 'verification'; + /** * Pure decision logic for handleAbort()'s final step: which status message * to show, and whether `thinking` should be cleared. Extracted out of the @@ -102,31 +104,15 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { // 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. + // A plain Stop must keep waiting only while the summarize POST is + // actually in flight. That POST can outlive a client-side abort while + // continuing to mutate the shared OpenCode session, so advancing to a + // prompt would violate FIFO. The pre-POST marker snapshot and post-POST + // result verification are read-only GETs; Stop aborts those immediately. + // `compactOperationPhase` makes that distinction explicit for + // handleAbort(), while this flag suppresses every eventual compact result. private compactResultSuppressed = false; + private compactOperationPhase: CompactOperationPhase = 'idle'; private displayPermissionMode: PermissionMode | null = null; private instructionsSent = false; private currentBackendModel: string | null = null; @@ -340,6 +326,7 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { const compactAbortController = isCompactBatch ? new AbortController() : null; if (compactAbortController) { this.compactAbortController = compactAbortController; + this.compactOperationPhase = 'idle'; // 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 @@ -520,6 +507,7 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { if (cancelledBeforeStart) { if (this.compactAbortController === compactAbortController) { this.compactAbortController = null; + this.compactOperationPhase = 'idle'; } // A 10th PR-review round found this skip path never // calls session.onThinkingChange(true) (that's the @@ -545,7 +533,11 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { if (compactLocalId) { session.client.emitMessagesConsumed([compactLocalId], { clearQueuedThinkingGrace: true }); } - session.onThinkingChange(false); + // Plain Stop before summarize already emitted this + // keepalive in handleAbort(); localId cancellation did not. + if (!this.compactResultSuppressed) { + session.onThinkingChange(false); + } if (session.queue.size() === 0 && !this.shouldExit) { sendReady(); } @@ -692,13 +684,11 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { * 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. + * eventual result the same way. A plain Stop aborts only the read-only + * snapshot/verification GET phases; while the summarize POST itself is + * in flight it leaves the signal alone and waits for real server-side + * completion, preserving the shared-session FIFO invariant. Switch/exit + * aborts every phase because teardown disconnects the session. * * `compactAbortController` is created by the caller (the dequeue loop), * not here, and passed in — deliberately, before the loop's model/effort @@ -747,57 +737,94 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { return; } + if (isCancelled()) { + logger.debug('[opencode-remote] /compact skipped before marker snapshot: cancelled or aborted'); + return; + } + + // The pre-POST marker snapshot is a read-only GET. A plain Stop + // aborts it because no summarize request has started yet. + this.compactOperationPhase = 'snapshot'; + const markerSnapshot = await captureCompactionMarkerSnapshot({ + baseUrl, + sessionId: acpSessionId, + signal: compactAbortController.signal + }); + if (isCancelled()) { + logger.debug('[opencode-remote] /compact skipped after marker snapshot: cancelled or aborted'); + 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). + // explicit summary we show below (from fetchCompactionResult). // 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) { + // The summarize POST can keep mutating the shared session after a + // client abort, so a plain Stop deliberately waits only in this + // phase. It has no deadline because real compaction can take minutes. + this.compactOperationPhase = 'summarize'; + const requestResult = await backend.suppressUpdatesDuring(async () => { + try { + return await triggerOpencodeCompact({ + baseUrl, + sessionId: acpSessionId, + providerId: split.providerId, + modelId: split.modelId, + signal: compactAbortController.signal + }); + } finally { + // suppressUpdatesDuring may still quiet-drain after the + // POST settles. It is no longer server-side compaction. + this.compactOperationPhase = 'post-summarize'; + } + }); + if (!requestResult.ok) { if (!isCancelled()) { - session.sendSessionEvent({ type: 'message', message: `📦 Compaction failed: ${result.error}` }); + session.sendSessionEvent({ type: 'message', message: `📦 Compaction failed: ${requestResult.error}` }); } else { logger.debug('[opencode-remote] /compact failure suppressed: cancelled or aborted before it resolved'); } return; } + if (isCancelled()) { + logger.debug('[opencode-remote] /compact verification skipped: cancelled or aborted after summarize'); + 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 }); + // The persisted-result lookup is also read-only: once summarize + // returned, a plain Stop can abort it without concurrent session work. + this.compactOperationPhase = 'verification'; + const result = await fetchCompactionResult({ + baseUrl, + sessionId: acpSessionId, + markerIdsBefore: markerSnapshot?.markerIds ?? null, + 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); + switch (result.status) { + case 'success': { + session.sendSessionEvent({ type: 'message', message: '📦 Compaction completed' }); + const converted = convertAgentMessage({ type: 'reasoning', text: result.text, id: randomUUID() }); + if (converted) { + session.sendAgentMessage(converted); + } + return; } + case 'failed': + session.sendSessionEvent({ type: 'message', message: `📦 Compaction failed: ${result.reason}` }); + return; + case 'unverified': + session.sendSessionEvent({ type: 'message', message: '📦 Compaction result could not be verified.' }); + return; } } finally { // Defensive: only clear if this is still the controller we set — @@ -808,6 +835,7 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { // with another runCompactOperation call). if (this.compactAbortController === compactAbortController) { this.compactAbortController = null; + this.compactOperationPhase = 'idle'; } } } @@ -867,13 +895,11 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { * 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. + * reasoning). Plain Stop aborts only read-only marker/result GETs. It + * leaves an in-flight summarize POST running so the dequeue loop waits + * for its real server-side completion; only that phase can still mutate + * the shared session. Switch/exit aborts every phase because cleanup() + * disconnects the ACP subprocess right after. */ private async handleAbort(leavingRemote = false): Promise { // A hostile-review sweep found that a plain Stop during an in-flight @@ -891,7 +917,15 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { const compactAbortController = this.compactAbortController; if (compactAbortController) { this.compactResultSuppressed = true; - if (leavingRemote) { + // Only summarize can still mutate the shared OpenCode session. + // Plain Stop aborts snapshot/verification reads, but deliberately + // waits for an in-flight POST to complete server-side. + if ( + leavingRemote + || this.compactOperationPhase === 'snapshot' + || this.compactOperationPhase === 'post-summarize' + || this.compactOperationPhase === 'verification' + ) { compactAbortController.abort(); } } @@ -911,10 +945,11 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase { // 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 activeCompactAbortController = this.compactAbortController; const decision = selectAbortStatusMessage({ - hasCompactInFlight: compactAbortController !== null, + hasCompactInFlight: activeCompactAbortController !== null && this.compactOperationPhase === 'summarize', leavingRemote, - compactAborted: compactAbortController?.signal.aborted ?? false + compactAborted: activeCompactAbortController?.signal.aborted ?? false }); if (decision.shouldClearThinking) { this.session.onThinkingChange(false); diff --git a/cli/src/opencode/utils/opencodeCompactBridge.test.ts b/cli/src/opencode/utils/opencodeCompactBridge.test.ts index 04482621..902e1e08 100644 --- a/cli/src/opencode/utils/opencodeCompactBridge.test.ts +++ b/cli/src/opencode/utils/opencodeCompactBridge.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi } from 'vitest'; -import { fetchCompactionSummary, splitProviderModel, triggerOpencodeCompact } from './opencodeCompactBridge'; +import { captureCompactionMarkerSnapshot, fetchCompactionResult, 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 @@ -177,242 +177,179 @@ describe('triggerOpencodeCompact', () => { }); }); -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) => { +const marker = (id: string) => ({ + info: { id, role: 'user' }, + parts: [{ type: 'compaction', auto: false }] +}); + +const summary = (id: string, parentID: string, overrides: Record = {}, text: string | null = '## Objective\n- Did the thing') => ({ + info: { id, role: 'assistant', parentID, summary: true, finish: 'provider-terminal', ...overrides }, + parts: text === null ? [{ type: 'step-start' }, { type: 'step-finish' }] : [{ type: 'text', text }] +}); + +describe('captureCompactionMarkerSnapshot', () => { + it('records only pre-existing manual marker IDs before POST so a later result cannot reuse them', async () => { + const fetchImpl = vi.fn(async (url: string, init?: RequestInit) => { 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 }); + expect(init).toMatchObject({ method: 'GET', signal: noSignal }); + return new Response(JSON.stringify([marker('old-marker')]), { 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' }); + await expect(captureCompactionMarkerSnapshot({ + baseUrl: 'http://127.0.0.1:48273', sessionId: 'ses_abc', fetchImpl, signal: noSignal + })).resolves.toEqual({ markerIds: ['old-marker'] }); }); - - 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' }); +}); + +describe('fetchCompactionResult', () => { + const options = (messages: unknown[]) => ({ + baseUrl: 'http://127.0.0.1:48273', + sessionId: 'ses_abc', + markerIdsBefore: ['old-marker'], + fetchImpl: vi.fn(async () => new Response(JSON.stringify(messages), { status: 200 })), + signal: noSignal }); - 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 - }); + it('succeeds only for the new marker\'s exactly parent-linked terminal summary with nonblank text', async () => { + const result = await fetchCompactionResult(options([ + marker('old-marker'), + summary('old-summary', 'old-marker', {}, 'old summary'), + marker('this-request-marker'), + summary('this-request-summary', 'this-request-marker') + ])); - expect(result).toEqual({ found: false }); + expect(result).toEqual({ status: 'success', text: '## Objective\n- Did the thing' }); }); - 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 })); + it('does not guess between two post-snapshot manual markers', async () => { + const result = await fetchCompactionResult(options([ + marker('old-marker'), + marker('this-request-marker'), + summary('this-request-summary', 'this-request-marker'), + marker('later-manual-marker'), + summary('later-summary', 'later-manual-marker', {}, 'later summary') + ])); - 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' }); + expect(result).toEqual({ status: 'unverified', reason: 'Compaction result could not be verified.' }); }); - 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 - }); + it('does not fall back to adjacent text when the new marker has no exact parent-linked assistant result', async () => { + const result = await fetchCompactionResult(options([ + marker('old-marker'), + marker('this-request-marker'), + summary('unrelated-summary', 'another-marker') + ])); - expect(result).toEqual({ found: false }); + expect(result).toEqual({ status: 'unverified', reason: 'Compaction result could not be verified.' }); }); - - 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({ + it('keeps a malformed result GET unverified', async () => { + const result = await fetchCompactionResult({ baseUrl: 'http://127.0.0.1:48273', sessionId: 'ses_abc', - fetchImpl, + markerIdsBefore: ['old-marker'], + fetchImpl: vi.fn(async () => new Response('not json', { status: 200 })), signal: noSignal }); - expect(result).toEqual({ found: false }); + expect(result).toEqual({ status: 'unverified', reason: 'Compaction result could not be verified.' }); }); - 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('classifies the observed HTTP-200, finish-unknown, empty linked summary as failed', async () => { + const result = await fetchCompactionResult(options([ + marker('old-marker'), + marker('this-request-marker'), + summary('this-request-summary', 'this-request-marker', { + finish: 'unknown', + tokens: { input: 0, output: 0, reasoning: 0 } + }, null) + ])); + + expect(result).toEqual({ status: 'failed', reason: 'OpenCode returned an empty compaction summary.' }); }); - it('returns found:false on a non-ok response', async () => { - const fetchImpl = vi.fn(async () => new Response(null, { status: 500 })); + it('does not use a finish allowlist when terminal evidence and a valid summary are present', async () => { + const result = await fetchCompactionResult(options([ + marker('old-marker'), + marker('this-request-marker'), + summary('this-request-summary', 'this-request-marker', { finish: 'provider-specific-finish' }) + ])); - const result = await fetchCompactionSummary({ - baseUrl: 'http://127.0.0.1:48273', - sessionId: 'ses_abc', - fetchImpl, - signal: noSignal - }); - - expect(result).toEqual({ found: false }); + expect(result).toEqual({ status: 'success', text: '## Objective\n- Did the thing' }); }); - 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 - }); + it('concatenates every text part from the exact linked summary', async () => { + const result = await fetchCompactionResult(options([ + marker('old-marker'), + marker('this-request-marker'), + { + info: { + id: 'this-request-summary', + role: 'assistant', + parentID: 'this-request-marker', + summary: true, + finish: 'provider-terminal' + }, + parts: [ + { type: 'text', text: '## Objective\n' }, + { type: 'step-finish' }, + { type: 'text', text: '- Did the thing' } + ] + } + ])); - expect(result).toEqual({ found: false }); + expect(result).toEqual({ status: 'success', text: '## Objective\n- Did the thing' }); }); - 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 })); + it('keeps an associated but non-terminal result unverified instead of treating token-less text as a failure', async () => { + const result = await fetchCompactionResult(options([ + marker('old-marker'), + marker('this-request-marker'), + summary('this-request-summary', 'this-request-marker', { finish: undefined }, '') + ])); - 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' }); + expect(result).toEqual({ status: 'unverified', reason: 'Compaction result could not be verified.' }); }); - - 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('uses OpenCode APIError.data.message as a normalized, truncated safe failure reason without serializing metadata', async () => { + const providerMessage = ` provider\n unavailable ${'x'.repeat(220)} `; + const result = await fetchCompactionResult(options([ + marker('old-marker'), + marker('this-request-marker'), + summary('this-request-summary', 'this-request-marker', { + error: { + name: 'APIError', + data: { + message: providerMessage, + apiKey: 'super-secret-api-key', + requestHeaders: { authorization: 'Bearer super-secret-token' } + } + } + }, null) + ])); + + expect(result).toEqual({ status: 'failed', reason: `provider unavailable ${'x'.repeat(179)}` }); + expect((result as { reason: string }).reason).not.toContain('super-secret'); }); - 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. + it('returns unverified when the semantic-result GET is aborted', async () => { 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')); - }); + expect(init?.signal).toBe(controller.signal); + init?.signal?.addEventListener('abort', () => reject(new DOMException('The operation was aborted.', 'AbortError'))); })); - const resultPromise = fetchCompactionSummary({ + const resultPromise = fetchCompactionResult({ baseUrl: 'http://127.0.0.1:48273', sessionId: 'ses_abc', + markerIdsBefore: ['old-marker'], fetchImpl, signal: controller.signal }); - controller.abort(); - const result = await resultPromise; - expect(result).toEqual({ found: false }); + await expect(resultPromise).resolves.toEqual({ + status: 'unverified', reason: 'Compaction result could not be verified.' + }); }); }); diff --git a/cli/src/opencode/utils/opencodeCompactBridge.ts b/cli/src/opencode/utils/opencodeCompactBridge.ts index 3e288410..ca121661 100644 --- a/cli/src/opencode/utils/opencodeCompactBridge.ts +++ b/cli/src/opencode/utils/opencodeCompactBridge.ts @@ -2,9 +2,14 @@ export type OpencodeCompactResult = | { ok: true; summaryText?: string } | { ok: false; error: string }; -export type CompactionSummaryResult = - | { found: true; text: string } - | { found: false }; +export type CompactionResult = + | { status: 'success'; text: string } + | { status: 'failed'; reason: string } + | { status: 'unverified'; reason: string }; + +export type CompactionMarkerSnapshot = { + markerIds: string[]; +}; /** 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; @@ -31,7 +36,7 @@ type BunFetchInit = RequestInit & { timeout?: false }; * 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 + * been wired up but the result-verification GET (which 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 @@ -128,9 +133,16 @@ export async function triggerOpencodeCompact(opts: OpencodeCompactCallOpts & { } } -type OpencodeMessagePart = { type?: unknown; text?: unknown }; +type OpencodeMessagePart = { type?: unknown; text?: unknown; auto?: unknown }; type OpencodeMessageEntry = { - info?: { id?: unknown; role?: unknown; parentID?: unknown; summary?: unknown }; + info?: { + id?: unknown; + role?: unknown; + parentID?: unknown; + summary?: unknown; + finish?: unknown; + error?: unknown; + }; parts?: unknown; }; @@ -138,16 +150,7 @@ 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 { +function isAssistant(entry: OpencodeMessageEntry | undefined): boolean { return entry?.info?.role === 'assistant'; } @@ -160,62 +163,121 @@ function extractTextPart(entry: OpencodeMessageEntry | undefined): string | null 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 { +function isManualCompactionMarker(entry: OpencodeMessageEntry): boolean { + return Array.isArray(entry.parts) + && entry.parts.some((part) => isObjectRecord(part) && part.type === 'compaction' && part.auto === false); +} + +function getManualCompactionMarkerIds(entries: OpencodeMessageEntry[]): string[] | null { + const markerIds: string[] = []; + for (const entry of entries) { + if (!isManualCompactionMarker(entry)) continue; + if (typeof entry.info?.id !== 'string') return null; + markerIds.push(entry.info.id); + } + return markerIds; +} + +function isTerminal(entry: OpencodeMessageEntry): boolean { + // OpenCode/provider finish strings are not an enum HAPI owns. Any + // nonblank string is terminal evidence; an allowlist would reject valid + // provider-specific values such as the observed `unknown` finish. + return typeof entry.info?.finish === 'string' && entry.info.finish.trim().length > 0; +} + +function safeErrorReason(value: unknown): string | null { + if (value === undefined || value === null) return null; + // OpenCode persists provider failures as `{ name, data: { message, ... } }`. + // Read only that scalar message: serializing error/data would risk exposing + // request headers, provider metadata, or other sensitive fields. + const message = typeof value === 'string' + ? value + : isObjectRecord(value) && isObjectRecord(value.data) && typeof value.data.message === 'string' + ? value.data.message + : null; + if (!message) return 'OpenCode reported a compaction error.'; + const normalized = message.trim().replace(/\s+/g, ' '); + return normalized ? normalized.slice(0, 200) : 'OpenCode reported a compaction error.'; +} + +async function fetchSessionMessages(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 }; - + if (!response.ok) return null; 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 }; + return Array.isArray(data) ? data as OpencodeMessageEntry[] : null; } catch { - return { found: false }; + return null; } } + +/** + * Captures manual compaction markers before the summarize POST. The + * post-request verifier uses this boundary instead of selecting the newest + * marker, which would misattribute an older or later manual compaction. + */ +export async function captureCompactionMarkerSnapshot(opts: OpencodeCompactCallOpts & { + fetchImpl?: FetchLike; +}): Promise { + const entries = await fetchSessionMessages(opts); + if (!entries) return null; + const markerIds = getManualCompactionMarkerIds(entries); + return markerIds ? { markerIds } : null; +} + +/** + * Resolves only the persisted assistant result associated with this exact + * summarize request. HTTP 200 merely proves the endpoint returned; it is not + * success evidence. Unknown shapes, absent association, and non-terminal + * messages remain unverified rather than becoming a false completion. + */ +export async function fetchCompactionResult(opts: OpencodeCompactCallOpts & { + markerIdsBefore: readonly string[] | null; + fetchImpl?: FetchLike; +}): Promise { + if (!opts.markerIdsBefore) { + return { status: 'unverified', reason: 'Compaction result could not be verified.' }; + } + + const entries = await fetchSessionMessages(opts); + if (!entries) { + return { status: 'unverified', reason: 'Compaction result could not be verified.' }; + } + + const markerIds = getManualCompactionMarkerIds(entries); + if (!markerIds) { + return { status: 'unverified', reason: 'Compaction result could not be verified.' }; + } + + const before = new Set(opts.markerIdsBefore); + const newMarkerIds = markerIds.filter((markerId) => !before.has(markerId)); + if (newMarkerIds.length !== 1) { + return { status: 'unverified', reason: 'Compaction result could not be verified.' }; + } + + const linkedResults = entries.filter((entry) => + isAssistant(entry) && entry.info?.parentID === newMarkerIds[0] + ); + if (linkedResults.length !== 1) { + return { status: 'unverified', reason: 'Compaction result could not be verified.' }; + } + + const result = linkedResults[0]!; + const error = safeErrorReason(result.info?.error); + if (error) { + return { status: 'failed', reason: error }; + } + + const text = extractTextPart(result); + if (isTerminal(result) && (!text || text.trim().length === 0)) { + return { status: 'failed', reason: 'OpenCode returned an empty compaction summary.' }; + } + + if (result.info?.summary === true && isTerminal(result) && text && text.trim().length > 0) { + return { status: 'success', text }; + } + + return { status: 'unverified', reason: 'Compaction result could not be verified.' }; +}