Skip to content

feat(opencode): wire session fork via the server HTTP API - #1676

Open
junmo-kim wants to merge 23 commits into
tiann:mainfrom
junmo-kim:feat-opencode-fork
Open

feat(opencode): wire session fork via the server HTTP API#1676
junmo-kim wants to merge 23 commits into
tiann:mainfrom
junmo-kim:feat-opencode-fork

Conversation

@junmo-kim

Copy link
Copy Markdown
Contributor

Problem

Forking an opencode session from the Web UI currently fails with "Fork current is not supported". OpenCode itself supports forking natively (POST /session/:id/fork), but the HAPI opencode flavor doesn't advertise a conversation-history capability or handle the ForkConversation RPC, so the hub rejects the request before it ever reaches opencode.

Solution

Follow the same pattern the codex/grok flavors use, backed by the loopback HTTP API that the opencode acp subprocess already exposes (the compaction bridge already talks to it for /session/:id/summarize):

  • New OpencodeConversationHistory (cli/src/opencode/conversationHistory.ts): capability probe reads the server's published OpenAPI spec (/doc, falling back to /openapi.json) and looks for the fork route; any failure gracefully hides the affordance instead of showing a broken button.
  • Fork current posts to /session/:id/fork; fork-at-message resolves the boundary prompt via the history points already tracked per prompt and passes { messageID }, so the child gets everything before the selected turn.
  • Register the ForkConversation RPC handler and publish capability states + history points/indexes into session metadata (same shape as grok), which makes the existing web UI buttons appear with no web changes.

Usage

Fork on any opencode session from the web UI — "fork now" creates a full copy; selecting a past turn forks up to that point. The child session responds normally on its first message.

Tests

Unit tests for probe/fork/busy/mapping in cli/src/opencode/conversationHistory.test.ts (mock fetch). Manually verified end-to-end on an isolated hub+runner stack against a real opencode acp process: current and historical forks both return 200, children receive the correct prefix, and both answered their first message.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Fork children can silently lose native context — the new OpenCode fork RPC returns a native id, but fork-child resume still falls back to a fresh session when loading that id fails. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:285 and surrounding context cli/src/opencode/opencodeRemoteLauncher.ts:203.
  • [Major] History indexes restart at zero after resume or mode handoff — each new remote launcher resets the counter while restored/native history may already contain user turns, so historical forks can resolve an unrelated earlier message. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:140, cli/src/opencode/opencodeRemoteLauncher.ts:280, and cli/src/opencode/opencodeRemoteLauncher.ts:663.
  • [Major] The documented OpenAPI fallback stops on a missing /doc endpoint — a non-2xx response throws out of the loop before /openapi.json is tried. Evidence cli/src/opencode/conversationHistory.ts:105 and cli/src/opencode/conversationHistory.ts:126.

Questions

  • None.

Summary

Review mode: initial

Three major correctness issues affect fork availability or forked context integrity.

Testing

Not run (automation). Static review only; the PR adds class-level tests, but lacks coverage for a /doc 404 fallback, resumed/local-handoff prompt indexing, and fork-child native resume failure.

HAPI Bot

Comment thread cli/src/opencode/opencodeRemoteLauncher.ts
Comment thread cli/src/opencode/opencodeRemoteLauncher.ts Outdated
Comment thread cli/src/opencode/conversationHistory.ts Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Fork children can silently lose native context — the new OpenCode fork RPC returns a native id, but fork-child resume still falls back to a fresh session when loading that id fails. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:290 and related context cli/src/opencode/opencodeRemoteLauncher.ts:203.
  • [Major] Historical fork indexes restart after resume or local handoff — each launcher starts its counter at zero even when the native session already contains user turns, so a later fork can resolve the wrong native message. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:663.
  • [Major] The fork busy guard is never activated — queued rows are acknowledged before turn setup, but the launcher never calls setBusy, leaving a window for a concurrent fork during model/effort setup or prompt execution. Evidence cli/src/opencode/conversationHistory.ts:138.

Questions

  • None.

Summary

Review mode: follow-up after new commits

The new commit fixes the prior /doc 404 fallback. Three major correctness issues remain around forked-context integrity and concurrent history mutation.

Testing

Not run (automation). Static review only; coverage is still missing for strict fork-child resume, resumed/local-handoff index derivation, and fork rejection across the full dequeued-turn lifecycle.

HAPI Bot

Comment thread cli/src/opencode/opencodeRemoteLauncher.ts
Comment thread cli/src/opencode/opencodeRemoteLauncher.ts Outdated
Comment thread cli/src/opencode/conversationHistory.ts

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] A failed native-history lookup still creates a durable wrong fork point - getNativeUserMessageCount() returns null on HTTP or parse failure, but the launcher falls back to a counter that starts at zero for every remote launcher. On a resumed or local-to-remote session, the next message can therefore be recorded against an earlier native user turn; a later historical fork can hydrate one transcript prefix while loading different model context. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:684 and related fallback behavior cli/src/opencode/conversationHistory.ts:207.

    Suggested fix:

    const promptIndex = await this.conversationHistory.getNativeUserMessageCount();
    if (promptIndex !== null) {
        this.conversationHistory.rememberPromptIndex(batch.items[0]?.localId, promptIndex);
        void this.conversationHistory.publish().catch(() => {});
    } else {
        logger.warn('[opencode-remote] Native history unavailable; skipping fork point');
    }

Questions

  • None.

Summary

Review mode: follow-up after new commits

One major correctness issue remains in the failure path for resumed/handoff history indexing. The fallback can publish a valid-looking but incorrect historical fork boundary.

Testing

Not run locally (automation/security constraint). GitHub test and integration checks pass. Missing regression coverage: a resumed or local-handoff session where GET /session/:id/message fails must not publish a fallback index.

HAPI Bot

Comment thread cli/src/opencode/opencodeRemoteLauncher.ts Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Fork success is returned before the strict OpenCode child binds the forked native session — the new fork-child load failure is raised only after runner startup has already been acknowledged, while the hub waits for exact native binding only for Grok and Pi. A failed OpenCode load can therefore leave the caller with a child ID whose process subsequently exits, without rolling back the HAPI child/native fork. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:215, related context cli/src/agent/sessionFactory.ts:376, hub/src/sync/syncEngine.ts:1493, and hub/src/sync/syncEngine.ts:1527.

    Suggested fix:

    // Extend waitForExactNativeForkBound's metadataKey union.
    if (flavor === 'opencode') {
        const bound = await this.waitForExactNativeForkBound(
            childId, rpcResult.nativeSessionId, 'opencodeSessionId', false
        )
        if (!bound) {
            throw new Error('OpenCode fork could not load the forked native session')
        }
    }

Questions

  • None.

Summary

Review mode: follow-up after new commits

One major integration gap remains: the fork route can report success before the OpenCode child proves it loaded the exact native fork.

Testing

Not run locally (automation/security constraint). GitHub integration and drift-gate pass; test is pending. Add a hub fork regression test where runner startup succeeds but the OpenCode child never publishes the expected opencodeSessionId, asserting an error and child cleanup.

HAPI Bot

Comment thread cli/src/opencode/opencodeRemoteLauncher.ts

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Disable conversation-history actions after resume falls back to a fresh native session — an ordinary loadSession failure starts a blank OpenCode session, but the new code then restores the previous numeric history locators. The HAPI transcript is retained, so an old local ID can later resolve to an unrelated new native user message at the same index; current forks can also hydrate the old transcript over a reset native context. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:287, related context cli/src/opencode/opencodeRemoteLauncher.ts:207.

    Suggested fix:

    const historyDiverged = currentMetadata?.conversationHistoryDiverged === true
        || startedFreshAfterResumeFailure
    
    if (historyDiverged) {
        session.client.updateMetadata((metadata) => {
            const capabilities = { ...metadata.capabilities }
            delete capabilities.conversationHistory
            return {
                ...metadata,
                capabilities,
                conversationHistoryPoints: undefined,
                conversationHistoryIndexes: undefined,
                conversationHistoryDiverged: true
            }
        })
    } else {
        this.conversationHistory.restorePromptIndexes(
            session.client.getMetadata()?.conversationHistoryIndexes
        )
        void this.conversationHistory.probeCapabilities().catch(() => {})
    }
  • [Major] Avoid downloading the complete native transcript before every prompt — getNativeUserMessageCount() calls GET /session/:id/message, parses every message/part, and is now awaited for each turn. Over a long tool-heavy session this produces quadratic transfer/parsing work and increasing prompt latency. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:682, related context cli/src/opencode/conversationHistory.ts:211.

    Suggested fix:

    // Seed once after load/newSession; refresh only after native history mutations.
    let nextPromptIndex = await this.conversationHistory.getNativeUserMessageCount()
    
    const promptIndex = nextPromptIndex
    await backend.prompt(acpSessionId, promptContent, onMessage)
    if (localId && promptIndex !== null) {
        this.conversationHistory.rememberPromptIndex(localId, promptIndex)
        nextPromptIndex = promptIndex + 1
        await this.conversationHistory.publish()
    }

Questions

  • None.

Summary

Review mode: follow-up after new commits

Two major issues remain: fallback-to-new-session can expose forks over divergent native/HAPI history, and fork-point indexing adds quadratic full-history reads to normal prompting.

Testing

Not run locally (automation/security constraint). GitHub test, integration, and drift-gate pass. Missing focused coverage: resume fallback must clear/disable history locators; multiple prompts should not fetch the complete message list each turn; exact-native bind failure should assert child cleanup, not only the successful wait call.

HAPI Bot

Comment thread cli/src/opencode/opencodeRemoteLauncher.ts Outdated
Comment thread cli/src/opencode/opencodeRemoteLauncher.ts Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Commit fork indexes only after OpenCode accepts the prompt — the launcher increments the cursor and publishes a history point before backend.prompt() runs. Existing prompt-error paths can reject the ACP request; that leaves the failed HAPI message mapped to a native index that may not exist, and the next prompt continues from the shifted cursor. A later historical fork can therefore select the wrong native user message or fail lookup. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:721.

    Suggested fix:

    const promptIndex = this.nativeUserIndexCursor
        ?? await this.conversationHistory.getNativeUserMessageCount()
    try {
        await backend.prompt(acpSessionId, promptContent, onMessage)
        if (promptIndex !== null) {
            this.nativeUserIndexCursor = promptIndex + 1
            this.conversationHistory.rememberPromptIndex(localId, promptIndex)
            void this.conversationHistory.publish().catch(() => {})
        }
    } catch (error) {
        this.nativeUserIndexCursor = null
        throw error
    }
  • [Minor] Keep persisted divergent sessions from re-advertising fork controls — this gate only checks whether the current launch just fell back. On a later successful resume, conversationHistoryDiverged is still true and the hub rejects every history action, but the launcher probes again and republishes the fork capability, so the UI shows buttons that can only fail. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:326, related guard hub/src/sync/syncEngine.ts:1142.

    Suggested fix:

    const historyDiverged = currentMetadata?.conversationHistoryDiverged === true
        || startedFreshAfterResumeFailure
    
    if (historyDiverged) {
        // clear locators/capability; do not restore or probe
    } else {
        this.conversationHistory.restorePromptIndexes(
            session.client.getMetadata()?.conversationHistoryIndexes
        )
        void this.conversationHistory.probeCapabilities().catch(() => {})
    }

Questions

  • None.

Summary

Review mode: follow-up after new commits

One major fork-boundary correctness issue and one minor persisted-divergence affordance issue remain.

Testing

Not run locally (automation/security constraint). GitHub test, integration, and drift-gate pass. Missing focused coverage: a rejected prompt must not publish/advance a fork index, the next prompt must recount native history, and a session already marked conversationHistoryDiverged must not probe or re-advertise fork capabilities.

HAPI Bot

Comment thread cli/src/opencode/opencodeRemoteLauncher.ts
Comment thread cli/src/opencode/opencodeRemoteLauncher.ts Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Publish fork indexes only after OpenCode accepts the prompt — the follow-up delays the cursor advance, but rememberPromptIndex() and publish() still run before backend.prompt(). A rejected prompt therefore leaves a durable locator for a native message that may not exist; if OpenCode persisted the user message before returning an error, the unchanged cursor also shifts the next locator. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:721.

    Suggested fix:

    const localId = batch.items[0]?.localId
    try {
        await backend.prompt(acpSessionId, promptContent, onMessage)
        if (nativeUserCount !== null) {
            this.nativeUserIndexCursor = nativeUserCount + 1
            this.conversationHistory.rememberPromptIndex(localId, nativeUserCount)
            void this.conversationHistory.publish().catch(() => {})
        }
    } catch (error) {
        this.nativeUserIndexCursor = null
        throw error
    }
  • [Major] Bound the optional history lookup before normal prompting — getNativeUserMessageCount() is awaited before every first/recounted prompt, while the default fetch wrapper explicitly uses timeout: false and supplies no abort signal. If the loopback HTTP endpoint accepts but stalls, an otherwise usable ACP session never reaches backend.prompt(). Evidence cli/src/opencode/conversationHistory.ts:211, call site cli/src/opencode/opencodeRemoteLauncher.ts:716.

    Suggested fix:

    const response = await this.fetchFn(
        `${baseUrl}/session/${encodeURIComponent(sessionId)}/message`,
        { method: 'GET', signal: AbortSignal.timeout(5_000) }
    )
  • [Minor] Keep persisted divergent sessions from re-advertising fork controls — the gate checks only the fallback performed during this launch. On a later successful resume, conversationHistoryDiverged remains true, the hub rejects every history action, but the probe republishes capabilities and exposes controls that can only fail. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:326, related guard hub/src/sync/syncEngine.ts:1142.

    Suggested fix:

    const historyDiverged = startedFreshAfterResumeFailure
        || currentMetadata?.conversationHistoryDiverged === true
    
    if (historyDiverged) {
        // clear history capabilities/locators; do not restore or probe
    } else {
        this.conversationHistory.restorePromptIndexes(
            session.client.getMetadata()?.conversationHistoryIndexes
        )
        void this.conversationHistory.probeCapabilities().catch(() => {})
    }

Questions

  • None.

Summary

Review mode: follow-up after new commits

Two fork-boundary/prompt-availability issues and one persisted-divergence affordance issue remain at the latest head.

Testing

Not run locally (automation/security constraint). GitHub test, integration, and drift-gate pass. Missing focused coverage: rejected prompts must not publish/advance locators and must force a recount; a stalled message-history endpoint must fail open within a deadline; previously divergent sessions must not probe or re-advertise history capabilities.

HAPI Bot

Comment thread cli/src/opencode/opencodeRemoteLauncher.ts Outdated
Comment thread cli/src/opencode/conversationHistory.ts
Comment thread cli/src/opencode/opencodeRemoteLauncher.ts Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Publish fork locators only after OpenCode accepts the prompt — the local ID is added to the in-memory map and queued for metadata publication before backend.prompt(). A rejected turn therefore remains forkable at an index that may belong to a later native prompt; the cursor is also not invalidated for a recount. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:726.

    Suggested fix:

    const localId = batch.items[0]?.localId;
    try {
        await backend.prompt(acpSessionId, promptContent, (message) => {
            this.handleAgentMessage(message);
        });
        if (nativeUserCount !== null) {
            this.nativeUserIndexCursor = nativeUserCount + 1;
            this.conversationHistory.rememberPromptIndex(localId, nativeUserCount);
            void this.conversationHistory.publish().catch(() => {});
        }
    } catch (error) {
        this.nativeUserIndexCursor = null;
        throw error;
    }
  • [Major] Bound the optional native-history lookup — the first/recounted prompt awaits this GET, while the default fetch wrapper disables timeouts. A loopback endpoint that accepts and stalls prevents the ACP prompt from starting, turning optional fork indexing into a session outage. Evidence cli/src/opencode/conversationHistory.ts:211, call site cli/src/opencode/opencodeRemoteLauncher.ts:721.

    Suggested fix:

    const response = await this.fetchFn(
        `${baseUrl}/session/${encodeURIComponent(sessionId)}/message`,
        { method: 'GET', signal: AbortSignal.timeout(5_000) }
    );

Questions

  • None.

Summary

Review mode: follow-up after new commits

The persisted-divergence follow-up is addressed, but two Major prompt-path defects remain at the latest head.

Testing

Not run (automation/security constraint). Current GitHub state: integration and drift-gate pass; test is still in progress. Missing focused coverage: rejected prompts must not publish locators and must force a recount; a stalled native-history endpoint must fail open within a deadline.

HAPI Bot

Comment thread cli/src/opencode/opencodeRemoteLauncher.ts Outdated
Comment thread cli/src/opencode/conversationHistory.ts

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Cache an unavailable native-history lookup instead of delaying every prompt — when the bounded GET returns null, nativeUserIndexCursor remains null, so every later batch awaits the same 5-second lookup before backend.prompt(). A stalled optional history endpoint therefore adds recurring latency to the primary prompt path. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:725, lookup cli/src/opencode/conversationHistory.ts:225.

    Suggested fix:

    private nativeUserIndexInitialized = false;
    
    let nativeUserCount = this.nativeUserIndexCursor;
    if (nativeUserCount === null && !this.nativeUserIndexInitialized) {
        this.nativeUserIndexInitialized = true;
        nativeUserCount = await this.conversationHistory.getNativeUserMessageCount();
    }
    // Reset initialized=false together with cursor=null after compaction or prompt failure.
  • [Minor] Disable both fork modes when the shared POST route is missing — current and historical forks use the same /session/:id/fork route, but the catch only disables forkCurrent when there is no messageID. After a successful probe, a 404/405 leaves historical fork advertised; a historical request disables neither mode, so the UI keeps offering an action that cannot work. Evidence cli/src/opencode/conversationHistory.ts:188.

    Suggested fix:

    if (isRouteMissing(error)) {
        this.states = markUnsupported(
            markUnsupported(this.states, 'forkCurrent'),
            'forkAtMessage'
        );
        await this.publishCapabilities?.();
    }

Questions

  • None.

Summary

Review mode: follow-up after new commits

The two findings from the prior bot pass are fixed at this head. The latest full diff still has one prompt-path performance regression and one stale capability state.

Testing

Not run (automation/security constraint). Current checks: integration and drift-gate pass; test remains in progress. Missing focused coverage: two consecutive prompts after a stalled history lookup should not each wait for the timeout; a probed-supported route returning 404/405 should remove both fork capabilities.

HAPI Bot

Comment thread cli/src/opencode/opencodeRemoteLauncher.ts Outdated
Comment thread cli/src/opencode/conversationHistory.ts Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Wait for validated OpenCode load before reporting the fork child — the child writes the requested native id into metadata before the ACP session/load finishes, so the new requireSessionReady=false poll can return success even if load later fails. Evidence hub/src/sync/syncEngine.ts:1549, related context cli/src/opencode/loop.ts:59 and cli/src/opencode/opencodeRemoteLauncher.ts:217.

    Suggested fix:

    // After loadSession/newSession succeeds:
    session.onSessionFound(acpSessionId);
    session.client.emitSessionReady();
    
    const bound = await this.waitForExactNativeForkBound(
        childId, rpcResult.nativeSessionId, "opencodeSessionId", true
    );

Questions

  • None.

Summary

Review mode: follow-up after new commits

The two findings from the prior bot pass are fixed at this head. One native-binding race remains: the hub can return a successful OpenCode fork before the child has actually loaded the forked native session.

Testing

Not run (automation/security constraint). Current CI: test, integration, and drift-gate pass. Missing focused coverage: delay or reject OpenCode loadSession and verify the fork request cannot resolve successfully until a post-load readiness signal arrives.

HAPI Bot

Comment thread hub/src/sync/syncEngine.ts

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Reject replacement native IDs before publishing readiness — session/load may return a sessionId different from the requested one, but the child is already pre-bound to the requested ID and this code emits session-ready before rebinding. The hub can therefore observe the expected stale ID plus readiness and report a successful fork backed by a replacement session. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:248, related context cli/src/agent/backends/acp/AcpSdkBackend.ts:370 and cli/src/opencode/loop.ts:59.

    Suggested fix:

    if (strictForkResume && acpSessionId !== resumeSessionId) {
        throw new Error('OpenCode fork loaded a different native session')
    }
    session.onSessionFound(acpSessionId)
    if (resumeSessionId) session.client.emitSessionReady()
  • [Major] Invalidate old absolute history indexes after compaction — the new code explicitly treats compaction as changing native user-message cardinality, but resets only the cursor used for future prompts. Existing promptIndexByLocalId entries and persisted points/indexes survive, so a later historical fork can resolve an old local ID against a different post-compaction message and fork the wrong boundary. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:698, related context cli/src/opencode/opencodeRemoteLauncher.ts:290 and cli/src/opencode/conversationHistory.ts:98.

    Suggested fix:

    this.conversationHistory.clearPromptIndexes()
    session.client.updateMetadata((metadata) => ({
        ...metadata,
        conversationHistoryPoints: undefined,
        conversationHistoryIndexes: undefined
    }))
    this.nativeUserIndexCursor = null
    this.nativeUserCountAttempted = false
  • [Major] Do not start a prompt after Stop/switch during the history lookup — the new bounded GET runs before backend.prompt, but it is not tied to the turn abort signal and the code does not re-check that signal afterward. handleAbort() can send session/cancel while no prompt exists, return "Turn aborted", and then this continuation starts the prompt once the GET finishes. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:741, related context cli/src/opencode/opencodeRemoteLauncher.ts:751 and cli/src/opencode/opencodeRemoteLauncher.ts:1235.

    Suggested fix:

    const lookupSignal = AbortSignal.any([waitSignal, AbortSignal.timeout(5_000)])
    nativeUserCount = await this.conversationHistory.getNativeUserMessageCount(lookupSignal)
    if (waitSignal.aborted || this.shouldExit) continue

Questions

  • None.

Summary

Review mode: follow-up after new commits

The prior thrown-load false-success path is fixed, but exact binding still accepts a replacement native ID. Two additional correctness regressions remain around compaction locator invalidation and aborting the new pre-prompt history lookup.

Testing

Not run (automation/security constraint). Current GitHub checks: test, integration, and drift-gate pass. Missing focused coverage: replacement session/load IDs, historical locators after compaction changes message count, and Stop/switch while getNativeUserMessageCount() is delayed.

HAPI Bot

Comment thread cli/src/opencode/opencodeRemoteLauncher.ts
Comment thread cli/src/opencode/opencodeRemoteLauncher.ts
Comment thread cli/src/opencode/opencodeRemoteLauncher.ts Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Reset the one-shot history lookup after an abort — the lookup guard is set before the abortable fetch, but the new abort branch continues without clearing it. A plain Stop during the first lookup therefore leaves nativeUserIndexCursor === null and nativeUserCountAttempted === true; every later prompt skips the lookup and never publishes a historical fork point. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:758.

    Suggested fix:

    if (waitSignal.aborted || this.shouldExit) {
        this.nativeUserIndexCursor = null;
        this.nativeUserCountAttempted = false;
        continue;
    }
  • [Minor] Preserve locators when compaction exits before issuing the summarize request — runCompactOperation() can return normally before any native mutation (for example cancellation before the marker snapshot, missing model metadata, or a pre-POST cancellation), but the caller now clears every persisted fork locator unconditionally. That permanently removes historical fork affordances for unchanged native history. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:704, related context cli/src/opencode/opencodeRemoteLauncher.ts:1035 and cli/src/opencode/opencodeRemoteLauncher.ts:1059.

    Suggested fix:

    let nativeHistoryMayHaveChanged = false;
    await this.runCompactOperation(
        acpSessionId,
        compactAbortController,
        compactLocalId,
        () => { nativeHistoryMayHaveChanged = true; }
    );
    if (nativeHistoryMayHaveChanged) {
        this.conversationHistory.clearPromptIndexes();
        // clear persisted points/indexes and reset the cursor
    }

    Invoke the callback immediately before triggerOpencodeCompact().

Questions

  • None.

Summary

Review mode: follow-up after new commits

Two correctness issues remain: Stop during the first history lookup disables future historical points for that launcher, and a non-mutating compaction exit erases valid historical locators.

Testing

Not run (automation/security constraint). Current GitHub checks: integration and drift-gate pass; test is still pending. Missing focused coverage: Stop during a delayed initial history lookup followed by a successful prompt; pre-POST compact cancellation preserving locators; actual summarize issuance clearing locators. The replacement-ID path also lacks a test where loadSession() returns a different ID.

HAPI Bot

Comment thread cli/src/opencode/opencodeRemoteLauncher.ts
Comment thread cli/src/opencode/opencodeRemoteLauncher.ts

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Minor] Clear fork locators only after a native compaction request is issued — runCompactOperation() has normal early returns before the summarize POST (missing runtime/model metadata or cancellation before/after the marker snapshot). The unconditional clear removes valid historical fork points even though native history did not change. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:704; related context cli/src/opencode/opencodeRemoteLauncher.ts:1041, cli/src/opencode/opencodeRemoteLauncher.ts:1053, cli/src/opencode/opencodeRemoteLauncher.ts:1063, and cli/src/opencode/opencodeRemoteLauncher.ts:1076.

    Suggested fix:

    let nativeHistoryMayHaveChanged = false;
    await this.runCompactOperation(
        acpSessionId,
        compactAbortController,
        compactLocalId,
        () => { nativeHistoryMayHaveChanged = true; }
    );
    if (nativeHistoryMayHaveChanged) {
        this.conversationHistory.clearPromptIndexes();
        // clear persisted points/indexes and reset the cursor
    }

    Invoke the callback immediately before triggerOpencodeCompact().

Questions

  • None.

Summary

Review mode: follow-up after new commits

The new head resolves the prior abort-state finding. One correctness issue remains: a compaction path that exits before native mutation still erases valid historical fork locators.

Testing

Not run (automation/security constraint). Missing focused coverage: abort during the delayed initial native-history lookup followed by a successful retry; pre-POST compact exits preserving locators; actual summarize issuance clearing locators.

HAPI Bot

await this.runCompactOperation(acpSessionId, compactAbortController, compactLocalId);
// Compaction can reindex native history: old absolute
// indexes and their locators are invalid afterwards.
this.conversationHistory.clearPromptIndexes();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Clear locators only after native mutation becomes possible

runCompactOperation() returns normally before triggerOpencodeCompact() when runtime/model metadata is unavailable or cancellation lands before/after the marker snapshot. This unconditional clear therefore removes every valid historical fork locator even though OpenCode history is unchanged; later prompts restore only newer points.

Suggested fix:

let nativeHistoryMayHaveChanged = false;
await this.runCompactOperation(
    acpSessionId,
    compactAbortController,
    compactLocalId,
    () => { nativeHistoryMayHaveChanged = true; }
);
if (nativeHistoryMayHaveChanged) {
    this.conversationHistory.clearPromptIndexes();
    // clear persisted points/indexes and reset the cursor
}

Call the callback immediately before triggerOpencodeCompact(), so request failures remain conservative while pre-request exits preserve existing indexes.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Minor] Withdraw fork support when the remote launcher starts leaving — the new handler and advertised capability outlive the remote launcher because onLeavingRemote() only disables /compact. During teardown, the hub can still observe a remote, idle session and route a fork to the closing loopback server; after the mode changes to local, the web continues to render fork actions even though the hub rejects local history actions. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:343; related context cli/src/opencode/opencodeRemoteLauncher.ts:831.

    Suggested fix:

    protected onLeavingRemote(): void {
        this.options.onCompactAvailabilityChange?.(false);
        this.conversationHistory.setBusy(true);
        void this.conversationHistory.disableFork().catch(() => {});
    }

Questions

  • None.

Summary

Review mode: follow-up after new commits

The new head resolves the prior compaction-locator invalidation issue. One lifecycle issue remains around remote teardown/local handoff.

Testing

Not run locally (automation/security constraint). Current GitHub test and integration checks passed. Missing focused coverage: remote-to-local handoff immediately fences fork-conversation and removes the advertised conversation-history capability.

HAPI Bot

: undefined
);
}
session.client.rpcHandlerManager.registerHandler(RPC_METHODS.ForkConversation, async (payload: unknown) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Withdraw fork support when leaving remote mode

This handler and the capability published above survive onLeavingRemote(), which currently only disables /compact. A switch/exit can therefore race a fork against the loopback server being torn down; once local mode is active, the stale metadata also keeps rendering fork controls even though the hub rejects history actions for user-controlled sessions.

Suggested fix:

protected onLeavingRemote(): void {
    this.options.onCompactAvailabilityChange?.(false);
    this.conversationHistory.setBusy(true);
    void this.conversationHistory.disableFork().catch(() => {});
}

The synchronous busy fence closes the teardown window; the metadata update hides the controls until the next remote launcher probes support again.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Minor] Fence detached capability probes after launcher exit — probeCapabilities() is started without awaiting or cancellation. If teardown begins while either documentation fetch is pending, the old instance is marked unsupported, but the detached probe can still finish later and publish that old state. A subsequent remote launcher shares the same session client, so the stale write can land after the new launcher advertises support and remove the fork controls from an active session. Evidence cli/src/opencode/opencodeRemoteLauncher.ts:351; related teardown cli/src/opencode/opencodeRemoteLauncher.ts:831.

    Suggested fix:

    private readonly historyProbeAbortController = new AbortController();
    
    void this.conversationHistory.probeCapabilities(
        this.historyProbeAbortController.signal
    ).catch(() => {});
    
    protected onLeavingRemote(): void {
        this.historyProbeAbortController.abort();
        this.options.onCompactAvailabilityChange?.(false);
        this.conversationHistory.setBusy(true);
        void this.conversationHistory.disableFork().catch(() => {});
    }
    
    // OpencodeConversationHistory
    async probeCapabilities(signal?: AbortSignal): Promise<void> {
        // combine signal with each request timeout
        // ...probe...
        if (signal?.aborted) return;
        await this.publishCapabilities?.();
    }

Questions

  • None.

Summary

Review mode: follow-up after new commits

The synchronous teardown fence from the prior review is present. One cross-launch metadata race remains because the capability probe can outlive its launcher and publish after a newer launcher.

Testing

Not run locally (security constraint; PR content was not executed). GitHub test, integration, and drift-gate checks passed. Add a focused test that delays launcher A's probe, exits A, starts launcher B, then resolves A's probe and verifies B's advertised capability remains intact.

HAPI Bot

return await this.conversationHistory.fork(messageLocalId);
});
if (!historyDiverged) {
void this.conversationHistory.probeCapabilities().catch(() => {});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] Fence detached capability probes after launcher exit

This detached probe can outlive its launcher. If teardown starts during either bounded documentation fetch, onLeavingRemote() marks this old instance unsupported, but probe completion still invokes its metadata publisher. After a quick local→remote handoff, launcher B may publish support first and launcher A can then overwrite it with its stale unsupported state, hiding fork controls for the active launcher.

Suggested fix:

private readonly historyProbeAbortController = new AbortController();

void this.conversationHistory.probeCapabilities(
    this.historyProbeAbortController.signal
).catch(() => {});

protected onLeavingRemote(): void {
    this.historyProbeAbortController.abort();
    this.options.onCompactAvailabilityChange?.(false);
    this.conversationHistory.setBusy(true);
    void this.conversationHistory.disableFork().catch(() => {});
}

// OpencodeConversationHistory
async probeCapabilities(signal?: AbortSignal): Promise<void> {
    // combine signal with each request timeout
    // ...probe...
    if (signal?.aborted) return;
    await this.publishCapabilities?.();
}

The final aborted check prevents an old launcher from publishing after its teardown; threading the signal into the fetches also avoids keeping the obsolete probe alive.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • None.

Questions

  • None.

Summary

Review mode: follow-up after new commits

No issues found in added/modified lines. The prior detached-probe race is fenced by the new abort signal and final aborted-state check. Residual risk: no regression test covers launcher A’s delayed probe resolving after teardown/launcher B startup; OpenCode’s fork endpoint boundary and response contract is Not found in repo/docs and remains runtime-dependent.

Testing

Not run locally (automation; PR code was not executed under the review security constraint). GitHub test, integration, and drift-gate checks passed. Suggested coverage: delay the capability probe, leave remote mode, start a successor launcher, resolve the old probe, and assert the successor’s capability metadata is unchanged.

HAPI Bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant