From 08fbea5311e61dbb98c736ad48ecfff28b36aeee Mon Sep 17 00:00:00 2001 From: Junmo Kim Date: Sat, 1 Aug 2026 18:20:07 +0900 Subject: [PATCH] feat(cli): bridge OpenCode native compaction via internal REST API (#1252) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(cli): allocate a loopback port for the OpenCode ACP subprocess opencode does not announce which port it bound when launched with --port 0 (verified: not present in stdout/stderr even at DEBUG level), so pick a free loopback port ourselves and pass it explicitly via --port/--hostname. This makes the ACP subprocess's internal HTTP API reachable at a known baseUrl for follow-up work (native /compact bridging). * feat(cli): add REST bridge for OpenCode native session compaction opencode's ACP method table has no session/compact RPC, but the opencode acp subprocess also runs an internal HTTP API. That API's POST /api/session/:id/compact route is an unimplemented v2 stub (always 503); the route that actually performs native AI compaction is the legacy POST /session/:id/summarize, which requires providerID and modelID in its payload. This adds a small client for that route plus a helper to split ACP's combined "provider/model" wire id. Not wired into the slash-command flow yet. * feat(cli): trigger native OpenCode compaction from /compact Wires the REST bridge into the OpenCode slash-command flow so /compact performs real context compaction instead of returning a "not yet supported" message. - slashCommands.ts: /compact now resolves to its own `kind: 'compact'` (the synchronous 'handled' shape can't carry an async REST round trip). /clear is unchanged. - opencodeRemoteLauncher.ts: registers a compact trigger callback once the ACP backend + internal HTTP baseUrl are ready, reading the session's current provider/model on every call so it reflects inline model switches. A `runExclusive` promise-chain mutex serializes the compact trigger against `backend.prompt()` so a still-in-flight prompt and a `/compact` sent moments later can't race against the same OpenCode session concurrently, in either arrival order. - runOpencode.ts: on /compact, emits "Compaction started" as a session event (the same `sendSessionEvent({ type: 'message', ... })` status- line channel Claude/Codex already use for compaction and other transient state, rather than a chat message), awaits the bridge with no artificial timeout (compaction on a reasoning model can take 90s+), then emits "Compaction completed" or "Compaction failed: " the same way. Falls back to the previous not-yet-supported chat message when no trigger is registered (local mode has no ACP backend, so this is unreachable there today). If the user cancels the message while compaction is queued or in flight, the eventual result is suppressed instead of surfacing a stray status message for an action the user already considered cancelled (aborting the in-flight HTTP call itself is left as follow-up scope). - help text now notes /compact is remote-sessions only, since local mode has no ACP backend to bridge through. Also fixes a runOpencode.test.ts fixture gap: the mocked OpencodeSession was missing onThinkingChange, so any exception in that call path was silently swallowed by the handler's outer catch instead of failing the test. * feat(cli): show compaction summary as a reasoning block After a native OpenCode compaction succeeds, OpenCode's session history gains a `role: "user"` marker message (a `{type:"compaction"}` part with no text) followed by a normal assistant message holding the actual generated summary in a `text` part. Left alone, neither is visible in HAPI — the bridge only checked success/failure and never looked at the resulting messages. - opencodeCompactBridge.ts: `fetchCompactionSummary()` does one more GET against the session's message list after a successful compact, finds the most recent `type:"compaction"` marker, and extracts the following assistant message's text — preferring a match via the marker's `parentID` (order-safe) and falling back to simple array adjacency, both gated on `role === 'assistant'` so an unrelated same-shaped sibling can't be silently misattributed as the summary. Concatenates every `text` part in case a summary spans more than one. Never throws: any unexpected shape or request failure just yields "not found" so the caller can skip showing anything. - opencodeRemoteLauncher.ts / runOpencode.ts: on a successful compact, forward the extracted summary as a `{ type: 'reasoning', text, id }` AgentMessage through the same `handleAgentMessage()` / `convertAgentMessage()` path OpenCode's own live thought-chunk streaming already uses, so it renders in the existing collapsible "Reasoning" block — no new UI component or schema field. The `role:"user"` marker is never constructed or forwarded at all, so there's nothing to filter (unlike Claude's compact summary, which is hidden after the fact via an `isCompactSummary` flag). * fix(cli): disable Bun's hardcoded fetch timeout for OpenCode compaction Isolated E2E against a real OpenCode session showed compaction always failing with "Compaction failed: The operation timed out." after ~250s, even though session/prompt-style unlimited waits were intended. Root cause: Bun's global fetch() hardcodes an internal ~5 minute timeout that fires independently of any AbortSignal (or its absence) — see oven-sh/bun#16682. The only documented workaround is passing the non-standard `timeout: false` fetch option that Bun itself recognizes. Cast through a local `BunFetchInit = RequestInit & { timeout?: false }` type alias rather than `Record`, so the option stays structurally checked against the rest of the fetch call's shape. * fix(cli): serialize /compact through the message queue instead of a mutex HAPI Bot flagged two Major correctness issues on the PR: - `/compact` bypassed `MessageQueue2` and entered a `runExclusive` mutex directly from the user-message handler. If prompt A was in flight and prompt B already queued behind it, `/compact` sent afterward could still reach the mutex before B did, running compaction ahead of an earlier-queued user prompt. - `compactTriggerRef` (a closure capturing the remote backend) was never cleared on a remote→local handoff, so `/compact` typed after switching to local mode could call a stale closure targeting an already-disposed backend instead of the intended local-mode fallback. Removes the mutex entirely and routes `/compact` through the same `messageQueue` regular prompts use (a new `operation?: 'compact'` field on `OpencodeMode`, dequeued by the launcher's single sequential consumer loop, which branches to a new `runCompactOperation()` instead of `backend.prompt()`). Ordering is now enforced by construction (one queue item in flight at a time) rather than a second bolted-on lock. Replaces the closure-based `onCompactTriggerReady` callback with a boolean-flag `onCompactAvailabilityChange`, reset to `false` on every entry into local mode (cold-start and handoff alike) before the remote backend is even disposed, so there is no window where the flag says available but the backend underneath it is gone. * fix(cli): let /compact's cancel ack fire at dequeue time, not on queue A follow-up HAPI Bot review caught a Major issue this PR's queue-based /compact serialization (previous commit) left open: runOpencode.ts still called session.emitMessagesConsumed manually and synchronously the instant /compact was pushed onto the queue -- a leftover from before /compact was routed through the queue at all. That beat MessageQueue2's automatic dequeue-time ack (onBatchConsumed, wired in sessionBase.ts, already used by every regular prompt) to the hub, so a /compact sitting behind other queued prompts was marked "invoked" immediately and could never actually be cancelled from the UI. Removes the manual ack; the queue's own dequeue-time ack now covers /compact exactly like any other queued operation. Adds a deterministic test for the reported scenario: prompt A generating, /compact queued behind it, cancelled before A finishes -- the compact REST bridge must never be called. * fix(cli): suppress live ACP updates while a compact REST call runs Found via live use: OpenCode keeps streaming session/update notifications (thought chunks, etc.) over the ACP transport while /compact's REST call runs outside prompt(), and AcpSdkBackend.handleSessionUpdate forwarded them unconditionally to whatever messageHandler was still installed from the last real prompt turn -- rendering the compaction summary a second time as a plain assistant message, alongside the explicit Reasoning block this PR already sends for the same content. Adds a narrow, opt-in AcpSdkBackend.suppressUpdatesDuring() (shared with Gemini, but a pure addition with no behavior change for existing prompt() callers) that temporarily swaps out the message handler for the duration of an async callback, restoring it afterward. Wraps the compact bridge's REST call with it so the live stream produces no output during that window -- the Reasoning block built from the explicit GET response becomes the only place the summary appears. * fix(cli): let Stop/switch-to-local interrupt an in-flight /compact REST call triggerOpencodeCompact's HTTP request is intentionally unbounded (a real compaction can take minutes), but the launcher awaited it with no way to interrupt it once dequeued and running. handleAbort() only cancels the ACP prompt() turn and resets the queue -- neither touches this raw HTTP call -- so Stop (and switch-to-local, which routes through the same handler) stayed blocked until the request settled on its own: a stale "Compaction completed/failed" could still surface afterward, queued prompts behind it were delayed, and remote->local handoff couldn't proceed. Add a per-call AbortController (compactAbortController) that handleAbort() aborts before cancelling the prompt, and thread it through triggerOpencodeCompact's new optional `signal` (kept separate from the existing timeout:false Bun workaround, which stays unconditional). An aborted call now folds into the same isCancelled() check that already suppresses a stale result for the existing queue-cancel race, so either kind of interruption behaves the same way. * fix(cli): structurally close remaining /compact abort/lifecycle races Two more narrow races surfaced in review, both symptoms of ad hoc per-site fixes rather than a shared mechanism: 1. The signal threaded through triggerOpencodeCompact (the POST) did not also reach fetchCompactionSummary (the GET runCompactOperation makes right after it), so Stop/switch-to-local could still block on that call alone. Introduce OpencodeCompactCallOpts with `signal` required (not optional) so every HTTP step opencodeCompactBridge.ts makes on behalf of one /compact operation is forced by the compiler to accept it, not left to remembering to wire it in per call site. 2. /compact availability (runOpencode.ts's compactSupported flag) was only reset to false on the *next* local-mode entry (loop.ts's runLocal callback), leaving a window between "switch/exit was requested" and "local mode actually started" where a /compact arriving mid-transition could still queue, and -- since local mode hands straight back to remote when it finds a non-empty queue -- run anyway despite the user having already left remote mode. Add an onLeavingRemote() hook to RemoteLauncherBase (no-op default, so the other six flavors built on it are unaffected) called synchronously as the very first action of requestExit() -- closing the race at its source -- and again unconditionally in start()'s finally block as a backstop for exit paths that never call requestExit() at all (an exception thrown from runMainLoop, for instance). OpencodeRemoteLauncher overrides it to flip availability false; loop.ts's local-entry reset is removed as redundant now that leaving remote is what's authoritative. * fix(cli): create compactAbortController before the inline model/effort switch A hostile-review whole-feature sweep of the /compact abort/lifecycle surface (round 5) found that the dequeue loop applied the per-batch inline model/effort switch (real async ACP round-trips that yield to the event loop) *before* branching into runCompactOperation(), which is where compactAbortController used to get created. An abort firing during that switch hit a still-null controller (a no-op), and by the time the switch resolved and runCompactOperation() created a fresh one, the abort was forgotten -- the compact's unbounded REST call would then run to completion despite the user having already pressed Stop/switch/exit. Move controller creation to the top of the loop iteration, as soon as the batch is known to be a compact operation, and pass it into runCompactOperation() rather than having that function create its own. Also: soften an overstated doc comment about what the required `signal` field actually guarantees, and add a regression test locking in that two sequential compact operations each get an independent controller (no leak or cross-clearing). * fix(cli): drain quietly before restoring the handler in suppressUpdatesDuring A 5th PR-review round found that suppressUpdatesDuring restored the suppressed messageHandler the instant its callback settled, but aborting the client-side HTTP call (e.g. OpenCode's compact bridge aborting via compactAbortController) does not necessarily stop the agent from continuing that operation server-side -- session/update is a separate notification channel from the HTTP request's lifecycle. A late straggler notification from a still-running server-side operation could leak straight into the restored handler (or into a new one prompt() installs right after). Reuse the same quiet-drain prompt() already runs before installing a new handler for the next turn (waitForSessionUpdateQuiet with the PRE_PROMPT_* constants) -- same class of race, same validated mechanism, just on the way back in instead of the way out. No new state or Stop-vs-switch branching: the wait is internal to suppressUpdatesDuring and the handler stays suppressed throughout it. * fix(cli): queue /compact during remote-mode initialization instead of rejecting it compactSupported alone conflated two different situations: a genuinely local-mode session (compact fundamentally can't run) 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 regular prompt sent in that same startup window queues normally and just waits its turn; /compact sent in the identical window instead got an immediate not-yet-supported reply. Check sessionWrapperRef.current?.mode (the actual OpencodeSession instance's mode, synced synchronously by onModeChange before either launcher starts) alongside compactSupported: only genuinely local mode now gets the not-yet-supported reply. A session already in remote mode but still initializing queues /compact exactly like a prompt and lets it settle into its real FIFO position once the launcher is ready. * fix(cli): don't let the remote-init /compact queuing fix reopen the teardown race A hostile-review whole-feature sweep found that gating /compact queuing on sessionWrapperRef.current?.mode alone (26344118) fixed the startup window but silently reopened the exact race OpencodeRemoteLauncher's onLeavingRemote() exists to close: mode stays 'remote' for the entire teardown window too (it only flips back to 'local' once runMainLoop() fully unwinds), so a /compact arriving after switch/exit was requested -- while compactSupported has already gone false -- queued anyway instead of getting rejected, and could still run once local mode bounced back to remote to drain a non-empty queue. Add compactTeardownInProgress, true from the moment onCompactAvailabilityChange(false) fires (which -- since the old reset-on-local-entry was removed -- only ever means "leaving remote", never "not ready yet") until the session next re-enters remote mode. Wrap the onModeChange callback opencodeLoop already receives to reset it back to false on that re-entry. The existing "stops queuing /compact once availability is reset to false" test didn't catch this because its mock never set mode to 'remote' during the simulated teardown, unlike real production timing -- fixed to match. * fix(cli): keep the dequeue loop blocked on a compact until it really finishes on plain Stop A 6th PR-review round rejected the quiet-drain mitigation from the previous round (bounded at ~1.2s) and asked for the originally proposed fix instead: quiet-drain cannot guarantee a multi-minute server-side compaction has actually finished, since session/update is a separate notification channel from the aborted HTTP request's lifecycle. Unconditionally aborting compactAbortController on any abort (the previous fix) let the dequeue loop move on to the next queued prompt while the agent could still be compacting the same OpenCode session server-side -- breaking the core invariant this feature's whole queue-based redesign depends on: compact and a prompt must never touch the same session concurrently. handleAbort() now takes a leavingRemote parameter. Plain Stop (leavingRemote=false, the default) only sets compactResultSuppressed -- the eventual result gets hidden, but compactAbortController is left 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 (leavingRemote=true) still abort the controller for real, since cleanup() disconnects the whole ACP subprocess right after regardless -- there's no shared session left to protect there, and the responsiveness fixed in an earlier round still matters for that path. The quiet-drain from the previous round (AcpSdkBackend.suppressUpdatesDuring) is left in place -- it still helps for a compaction that finishes quickly and for trailing session/update stragglers right after a real completion, it's just no longer the thing plain Stop relies on for correctness. * fix(cli): close 3 gaps a hostile-review pass expected the next bot round to flag Pre-emptively addresses findings a hostile-review final pass on 65729bb7 judged likely for the next external bot round, since a communication round-trip costs more than fixing them now: 1. No dedicated test existed for the Stop/switch-to-local RPC-overlap message-ordering fix (handleAbort() re-reading compactAbortController's abort state instead of a stale snapshot). The test harness has no way to observe MessageBuffer/Ink content, so the decision logic that picks the status message and whether to clear `thinking` is extracted into a pure, exported selectAbortStatusMessage() function and unit tested directly against all four (hasCompactInFlight, leavingRemote, compactAborted) combinations, including the exact overlap case. 2. No test verified compactResultSuppressed doesn't leak between two sequential compact operations. Added a regression test: a Stop-suppressed compact #1 finishing for real must not silence a normally-completed compact #2 queued after it. 3. The "Stop requested — waiting..." status message didn't tell the user how to actually leave the session (switch-to-local/exit) while a compaction is deliberately left running. Appended that guidance. * fix(cli): skip a compact cancelled before its request ever went out A plain Stop landing during the inline model/effort switch that precedes a compact batch only set compactResultSuppressed; it never stopped the dequeue loop from calling runCompactOperation() unconditionally once that switch resolved, so a cancelled-before-start compact would still fire a brand new REST request and block the loop for however long that call takes. Skip starting the operation when compactResultSuppressed is set and the controller was never actually aborted (plain Stop leaves the signal alone by design). Deliberately excludes the compactAbortController.signal.aborted case (switch/exit) and isLocalIdCancelled: both must keep falling through to runCompactOperation() as before, per Round 5's and the isLocalIdCancelled suite's existing coverage. * fix(cli): also skip a compact cancelled-before-start via isLocalIdCancelled Round 7's pre-start skip check only covered compactResultSuppressed (plain Stop), deliberately leaving isLocalIdCancelled out so as not to disturb the "Compaction started is never suppressed" tests that predated that check. But isLocalIdCancelled's backing Set can only ever be populated during the brief ack-vs-hub-DB-write race before a queued item's REST call is sent, never while it's actually running (see runOpencode.ts's cancelledBeforeEnqueue doc comment) — so a true result here unconditionally means the same "cancelled before it ever went out" situation Round 7 already handles for plain Stop, and deserves the same treatment: skip starting the operation instead of sending "Compaction started" for a request already known to be discarded. Updates the two tests that had encoded the old "started is never suppressed" behavior for this specific signal to their corrected expectation, and removes a no-longer-consumed mockImplementationOnce that would otherwise have leaked its failure response into the next test that actually calls the REST bridge. * fix(cli): don't resurrect /compact availability after a startup-time switch/exit onCompactAvailabilityChange(true) fired unconditionally right after newSession/loadSession resolved, with no way to know a terminal switch-to-local/exit had already run during that pending ACP round trip (RemoteLauncherBase.requestExit() sets shouldExit synchronously before awaiting its handler, so the flag is already accurate at that point). runOpencode.ts's compactSupported/compactTeardownInProgress gate treats compactSupported flipping true as reason enough to ignore compactTeardownInProgress entirely, so this belated true could let a /compact arriving right after slip into the queue mid-teardown. Guard the call with the same shouldExit flag requestExit() already set. The race can only be reached via the terminal UI's onExit/ onSwitchToLocal callbacks (wired up before runMainLoop starts, well before the RPC 'abort'/'switch' handlers exist), so the regression test forces isTTY and captures ink's render() props to invoke them directly. * fix(cli): clear the hub's queued-thinking grace when a compact is skipped pre-start The cancelledBeforeStart skip path never calls session.onThinkingChange(true) (the whole point of skipping), but also never told the hub the queued item was done. The hub's 15s queued-thinking grace (markMessageQueued, sessionCache.ts) keeps thinking pinned true regardless of keepalives until a messages-consumed ack with clearQueuedThinkingGrace arrives, so the web UI spinner could sit stuck for the full grace window. Applies the same pattern already used by runOpencode.ts's synchronous slash.kind === 'handled' path (e.g. /model): an emitMessagesConsumed ack with clearQueuedThinkingGrace, then an immediate thinking=false keepalive. This is additive to, not a replacement for, 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 protocol, and clearQueuedThinkingGrace is keyed by session, not localId, so both are idempotent. --- .../agent/backends/acp/AcpSdkBackend.test.ts | 131 ++ cli/src/agent/backends/acp/AcpSdkBackend.ts | 62 + .../common/remote/RemoteLauncherBase.ts | 34 + cli/src/opencode/loop.test.ts | 109 ++ cli/src/opencode/loop.ts | 23 +- .../opencode/opencodeRemoteLauncher.test.ts | 1239 ++++++++++++++++- cli/src/opencode/opencodeRemoteLauncher.ts | 498 ++++++- cli/src/opencode/runOpencode.test.ts | 281 +++- cli/src/opencode/runOpencode.ts | 207 ++- cli/src/opencode/types.ts | 8 + .../opencode/utils/opencodeBackend.test.ts | 55 + cli/src/opencode/utils/opencodeBackend.ts | 42 + .../utils/opencodeCompactBridge.test.ts | 418 ++++++ .../opencode/utils/opencodeCompactBridge.ts | 221 +++ cli/src/opencode/utils/slashCommands.test.ts | 16 +- cli/src/opencode/utils/slashCommands.ts | 15 +- 16 files changed, 3323 insertions(+), 36 deletions(-) create mode 100644 cli/src/opencode/loop.test.ts create mode 100644 cli/src/opencode/utils/opencodeBackend.test.ts create mode 100644 cli/src/opencode/utils/opencodeCompactBridge.test.ts create mode 100644 cli/src/opencode/utils/opencodeCompactBridge.ts 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')