fix(opencode): resume the OpenCode session on follow-ups instead of starting an empty one#3617
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Needs human review This PR introduces substantial new runtime behavior: session resumption via durable cursors, session forking when directories change, and new external API calls (session.get/update/fork). While well-tested, these changes fundamentally alter how OpenCode sessions are managed and warrant human review. You can customize Macroscope's approvability policy. Learn more. |
…ass) Addresses review feedback on pingdotgg#3617 (macroscope + Cursor Bugbot + a deep multi-agent review) without widening scope beyond the adapter: - Re-apply permissions on resume. `session.create` is the only place the runtimeMode permission ruleset is set, so re-adopting a session skipped it; the reactor restarts with the persisted cursor on a runtime-mode change, which would leave a resumed OpenCode session on stale (e.g. full-access) permissions. Resume now calls session.update with buildOpenCodePermissionRules(runtimeMode). - Don't resume into the wrong directory. OpenCode routes a prompt to the session's own stored directory, so reusing a session created under a different cwd would silently run there. Resume now starts a fresh session when the re-adopted session's directory differs from the requested cwd. - Distinguish "not found" from transient failures. The SDK client uses throwOnError:true, so session.get rejects on any non-2xx. Only a confirmed 404 / NotFoundError now falls back to creating a fresh session; transport/auth/server errors propagate instead of silently resetting a live thread to an empty session. Tests model throwOnError:true (session.get rejects) and add coverage for the permission re-application, cwd-mismatch fallback, and transient-error propagation paths.
|
Thanks for the reviews — addressed in 7be6629. Three hardening changes, all kept within the adapter:
Tests updated to model |
|
UPDATE: |
|
Pushed a follow-up correcting the directory-mismatch handling that was added during hardening. Problem: the guard that started a fresh session when the resumed session's stored directory differed from the requested cwd was justified by "OpenCode routes a prompt to the session's own stored directory." That premise doesn't hold — OpenCode resolves tool execution, snapshots and file ops from the per-request Fix: when the persisted session exists but was created under a different directory, Verified against the OpenCode source that fork copies all messages/parts with fresh ids and stamps the new session's directory/path from the request context. The former "starts fresh on directory mismatch" test now asserts fork-with-history. |
…ork base Adopts the OpenCode session-resume hardening from upstream pingdotgg#3617 (the pingdotgg#3604 class of silent context loss) onto this fork's adapter. Hand-ported, not cherry-picked: the fork runs ahead of upstream on @opencode-ai/sdk (1.17 vs the PR's 1.3) and passes a per-call `directory` to every client.session.* call, and it already had an independent, strict resume implementation. The upstream diff does not apply cleanly, so the PR's robustness was ported onto the fork's directory-aware base, preserving the per-call `directory` and isOpenCodeMessageAborted. - Schema-versioned cursor (OPENCODE_RESUME_VERSION / parseOpenCodeResume): malformed or foreign cursors are ignored rather than throwing. - isOpenCodeNotFound classifies only structured 404 / NotFoundError signals. A confirmed miss falls back to session.create; transport/auth/5xx errors propagate instead of masquerading as a brand-new empty session. - Fork on cwd change: when the stored directory differs (e.g. the thread moved into a git worktree), session.fork carries history into the requested directory instead of dropping context. - session.update re-applies the current runtimeMode ruleset on reuse/fork, since session.create is skipped on resume. - Race cleanup aborts only a session this call created; sendTurn echoes resumeCursor so the persisted cursor stays fresh. This trades a loud failure for a silent recovery on cwd mismatch: the fork previously hard-errored, and the two tests asserting that are replaced. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s onto fork base" This reverts commit 0e5fb26. Its centerpiece does not work. The adopted code forks a session into the requested directory on a cwd change, on the stated premise that "OpenCode routes tool execution by the per-request `directory`, NOT the session's stored directory". Probed against a live opencode 1.17.20 server, both halves of that are false: create(directory=B) -> session.directory=B, `pwd` -> B (create binds) reuse + directory=B -> `pwd` -> A (session dir wins) fork(A, directory=B) -> session.directory=A, `pwd` -> A (fork ignores it) Tool execution follows the session's OWN directory; the per-request `directory` is a lookup/app-context scope, not a binding. session.fork inherits the parent's directory and cannot be pointed elsewhere, and session.update cannot rebind it (its body only accepts `title`). create({directory, parentID}) does bind the new directory but starts empty — parentID is a child link, not a history copy. So no opencode API moves a session's history into a new directory. The practical effect is worse than what it replaced: the fork's session stays bound to the old tree, so after a thread moves into a git worktree the agent would keep editing the ORIGINAL checkout while T3 reports the worktree — silent wrong-tree writes. The prior strict behaviour at least failed loudly. The PR's other parts are genuinely good (schema-versioned cursor, 404-only fallback so transport/5xx errors propagate, resumeCursor echo, abort-only-if- created) and are worth re-landing without the fork. Reverted wholesale rather than patched in place so the broken path cannot ship while that is sorted out. Note the PR's tests are entirely mocked, which is why this passed review twice: mocks cannot observe where a tool actually runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The single issue from Bugbot's Jul 19 review (raw string comparison of the resume directory against the session's stored directory) is fixed in 5741a3b and the inline thread is resolved: |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 061be07. Configure here.
…tarting an empty one The OpenCode adapter always called session.create and never read or emitted a resume cursor, so the upstream ses_… id lived only in memory. When that in-memory binding was lost — the ProviderSessionReaper stopping an idle session (~30 min) or an app/server restart — the next follow-up in the same visible thread was sent to a brand-new, empty OpenCode session. t3code kept rendering its own projection DB, so the user still saw the full history while the model had no context (issue pingdotgg#3604). Mirror the Grok/Cursor/Codex resume pattern, entirely within the adapter: - Emit resumeCursor { schemaVersion, sessionId } on the started ProviderSession (and echo it from sendTurn) so ProviderService persists it into provider_session_runtime.resume_cursor_json. - On startSession, when a cursor is present, validate the id with session.get and re-adopt that session instead of creating a new one. OpenCode scopes history by session id, so prompting the same id restores the full prior conversation. A missing/closed session (or any get failure) falls back to a fresh session so a stale cursor can't wedge the thread. - Only abort the upstream session in the start race-cleanup when we actually created it; never abort a session we merely resumed. The persistence/recovery plumbing is provider-agnostic and already feeds a persisted cursor back into startSession on a reaped/restarted follow-up (ProviderService falls back to the stored binding cursor when the reactor passes none), so no changes outside the adapter are needed. Adds regression tests: fresh-session cursor emission, resume re-adopting the persisted id (no create), follow-up turns targeting the resumed id, stale-cursor fallback to create, and malformed-cursor rejection. Fixes pingdotgg#3604 Co-authored-by: codex <codex@users.noreply.github.com>
…ass) Addresses review feedback on pingdotgg#3617 (macroscope + Cursor Bugbot + a deep multi-agent review) without widening scope beyond the adapter: - Re-apply permissions on resume. `session.create` is the only place the runtimeMode permission ruleset is set, so re-adopting a session skipped it; the reactor restarts with the persisted cursor on a runtime-mode change, which would leave a resumed OpenCode session on stale (e.g. full-access) permissions. Resume now calls session.update with buildOpenCodePermissionRules(runtimeMode). - Don't resume into the wrong directory. OpenCode routes a prompt to the session's own stored directory, so reusing a session created under a different cwd would silently run there. Resume now starts a fresh session when the re-adopted session's directory differs from the requested cwd. - Distinguish "not found" from transient failures. The SDK client uses throwOnError:true, so session.get rejects on any non-2xx. Only a confirmed 404 / NotFoundError now falls back to creating a fresh session; transport/auth/server errors propagate instead of silently resetting a live thread to an empty session. Tests model throwOnError:true (session.get rejects) and add coverage for the permission re-application, cwd-mismatch fallback, and transient-error propagation paths. Co-authored-by: codex <codex@users.noreply.github.com>
Address review (Cursor Bugbot, high): isOpenCodeNotFound only walked the `cause` chain checking a numeric `status`, so it relied on the 404 sitting at one specific nesting and ignored `response.status` and the OpenCodeRuntimeError `detail` string. Reworked it into a bounded BFS that also checks `statusCode`, nested `response.status`, the NotFoundError `name`/`body`, and `message`/`detail` text, descending cause/body/error/data. Export it and add direct unit tests across every shape (incl. the real wrapped-Error production shape and a response.status-only 404), plus the transient/auth/network cases that must still propagate. Co-authored-by: codex <codex@users.noreply.github.com>
…e text Address review (Cursor Bugbot): isOpenCodeNotFound matched the free-text message/detail, so a non-404 error whose text merely contains 'not found' (a 500 saying 'upstream X not found', an auth error, or a serialized body from openCodeRuntimeErrorDetail) was misclassified as a missing session and silently started a fresh one. Decide only on structured signals — a numeric 404 (status/statusCode/nested response.status) or an explicit NotFoundError name — which already cover the real throwOnError:true production shape (cause.status=404 + body.name). Update unit tests to assert free-text-only inputs (incl. a 500 whose message contains 'not found') now propagate. Co-authored-by: codex <codex@users.noreply.github.com>
…ontext The directory-mismatch guard added while hardening this PR started a fresh, EMPTY session whenever the resumed session's stored directory differed from the requested cwd. Its stated rationale -- "OpenCode routes a prompt to the session's own stored directory, so resuming under a different cwd would run in the wrong tree" -- does not hold: OpenCode resolves tool execution, snapshots and file ops from the per-request directory param (instance context), not from session.info.directory. So the guard solved a non-problem and introduced a real one: any time a thread's cwd changes (most commonly when it moves from the project root into a git worktree between turns) the whole conversation was stranded in the old session and the follow-up landed in an empty one -- the exact pingdotgg#3604 symptom this PR set out to fix. Fix: when the persisted session exists but was created under a different directory, fork it INTO the requested directory (client.session.fork({ sessionID, directory })) instead of creating an empty session. OpenCode clones the full message history into a new session bound to the requested worktree, so the follow-up keeps its context and tools still run on the correct tree. The fork id becomes the durable resume cursor. A genuinely missing (404) session still starts fresh, unchanged. Verified against the OpenCode source that fork copies all messages/parts with fresh ids and stamps the new session's directory/path from the request context. Test: the former "starts fresh on directory mismatch" case now asserts the session is forked with history (fork called, no session.create, cursor -> fork id) via a new fork mock; the not-found path still starts fresh. Co-authored-by: codex <codex@users.noreply.github.com>
…ts name A node carrying an explicit non-404 numeric HTTP status now seals its subtree in isOpenCodeNotFound: a 500 whose serialized body is named NotFoundError (or that is itself named UpstreamNotFoundError) propagates instead of silently falling back to session.create and dropping context. Co-authored-by: codex <codex@users.noreply.github.com>
A trailing slash, an unnormalized segment, or a symlinked cwd (macOS /tmp -> /private/tmp) made the resume path misread the same working tree as a cwd change, forking the session and repointing the durable cursor at the clone on every resume. Compare lexically resolved forms first, then realpath both sides, each degrading to its lexical form when resolution fails (deleted directory, external-server path). Co-authored-by: codex <codex@users.noreply.github.com>
…m/Path services Drops the nodeBuiltinImport pragmas: isSameOpenCodeDirectory now takes the FileSystem and Path services (resolved once in makeOpenCodeAdapter, already present in OpenCodeDriverEnv) instead of importing node:fs and node:path directly, and the symlink test fixture moves to makeTempDirectoryScoped/symlink on the FileSystem service. Co-authored-by: codex <codex@users.noreply.github.com>
Comment-only: the resume doc blocks had grown to 17-23 lines while sibling adapters cap around 11; keep the constraints (structured-404-only classification, subtree sealing, fork-preserves-history, race cleanup scope) and drop the narration. Co-authored-by: codex <codex@users.noreply.github.com>
A substring match let any status-less error named *NotFound* (UpstreamNotFoundError, ProviderNotFoundError) pass as a missing session and silently start an empty one. OpenCode's API generates exactly name "NotFoundError" for every 404, so match it exactly. Co-authored-by: codex <codex@users.noreply.github.com>
ea323c0 to
8bfeff6
Compare

Fixes #3604.
What
The OpenCode adapter never read or emitted a resume cursor, so the upstream
ses_…id lived only in process memory. When that in-memory binding was lost —ProviderSessionReaperstopping an idle session (~30 min), or an app/server restart — the next follow-up in the same visible thread was sent to a brand-new, empty OpenCode session. t3code kept rendering its own projection DB, so the user still saw the full history while the model had no context.This makes the OpenCode adapter resumable, mirroring the existing Grok/Cursor/Codex pattern, entirely within
apps/server/src/provider/Layers/OpenCodeAdapter.ts:startSessionnow emitsresumeCursor: { schemaVersion, sessionId }on the returnedProviderSession(andsendTurnechoes it), soProviderServicepersists it intoprovider_session_runtime.resume_cursor_json.startSessionvalidates the id withsession.getand re-adopts that session instead of callingsession.create. OpenCode scopes history by session id, so prompting the same id restores the full prior conversation.session.getfailure) falls back to a fresh session, so a stale cursor can never wedge the thread.Why
This is the documented root cause in #3604 (with DB-level evidence of two OpenCode sessions per visible thread). The persistence/recovery plumbing is already provider-agnostic:
ProviderService.startSessionfalls back to the stored binding cursor when the reactor passes none, so no changes are needed outside the adapter — it just needed the adapter to start producing and consuming a cursor like every other provider already does.Scope
Intentionally small and focused — 2 files, no contract / persistence / orchestration changes:
apps/server/src/provider/Layers/OpenCodeAdapter.ts(+118/-20)apps/server/src/provider/Layers/OpenCodeAdapter.test.ts(+147) — regression tests for: fresh-session cursor emission, resume re-adopting the persisted id (nocreate), follow-up turns targeting the resumed id, stale-cursor fallback tocreate, and malformed/foreign-cursor rejection.Validation
tsgo --noEmitclean;OpenCodeAdapter.test.ts(21 tests) plus the fullsrc/provider+src/orchestrationsuites (531 tests) pass.Note
Medium Risk
Changes core session lifecycle for OpenCode threads; misclassified errors could still wedge or reset context, but behavior is guarded by structured 404 detection and extensive adapter tests.
Overview
Fixes #3604 by making the OpenCode adapter produce and consume a persisted
resumeCursor(schemaVersion+sessionId), like other providers, so follow-ups keep upstream conversation context after idle reaper or restart.startSessionnow probessession.getwhen a valid cursor is present: reuse the session when cwd matches (withsession.updatefor currentruntimeModepermissions), fork into the requested directory when cwd changed (preserving history), or create only on a confirmed 404. Transient/auth errors from the probe fail instead of silently starting fresh. Race cleanup aborts only sessions this call created, not resumed ones.sendTurnreturns the cursor so persistence stays fresh. HelpersisOpenCodeNotFoundandisSameOpenCodeDirectoryclassify SDK errors and path equivalence (symlinks, trailing slashes).Tests extend the OpenCode mock with
get/update/forkand cover resume, stale cursor, bad cursor, transient errors, cwd fork, and utility behavior.Reviewed by Cursor Bugbot for commit 8bfeff6. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Resume existing OpenCode sessions on follow-up turns instead of creating empty ones
startSessioninOpenCodeAdapter.tsnow parses a persistedresumeCursorand probessession.getto verify the session still exists before reusing it.cwd, the session is forked into the new directory; permissions are re-applied viasession.updateon resume.sendTurnresults now include the session'sresumeCursorso callers can persist it for future follow-ups.Macroscope summarized 8bfeff6.