Import external OpenCode sessions for T3 projects#14
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds installer-managed OpenCode runtime discovery to ChangesOpenCode Provider Integration
Test Framework Migration
Sequence DiagramssequenceDiagram
participant Server as Server Startup
participant SyncService as OpenCodeExternalSessionSync
participant Adapter as OpenCodeAdapter
participant SDK as OpenCode SDK
participant Engine as OrchestrationEngineService
Server->>SyncService: start() — forked scheduled worker
loop every syncIntervalMs
SyncService->>Engine: list active projects
Engine-->>SyncService: projects[]
SyncService->>Adapter: listExternalSessions(directories)
Adapter->>SDK: session.list(dir) per directory (reuseLocalServer: true)
SDK-->>Adapter: Session[]
Adapter-->>SyncService: OpenCodeExternalSessionSummary[]
loop per session
SyncService->>SyncService: matchOpenCodeSessionToProject
SyncService->>Engine: thread.create or thread.meta.update
SyncService->>Engine: thread.session.set (stopped, resume cursor)
end
SyncService-->>Server: SyncResult (imported/refreshed/skipped counts)
end
sequenceDiagram
participant Caller as startSession caller
participant Adapter as OpenCodeAdapter
participant SDK as OpenCode SDK
Caller->>Adapter: startSession(resumeCursor?)
alt resumeCursor present
Adapter->>SDK: session.get(sessionId)
SDK-->>Adapter: Session (createdOpenCodeSession=false)
else no cursor
Adapter->>SDK: session.create(...)
SDK-->>Adapter: Session (createdOpenCodeSession=true)
end
Adapter-->>Caller: ProviderSession with resumeCursor
Note over Adapter: Concurrent race-loser path
alt loser created fresh session
Adapter->>SDK: session.abort(sessionId)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
vouch:trusted
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/server/src/provider/Layers/OpenCodeExternalSessionSync.test.ts (1)
81-85: ⚡ Quick winAssert
limitpropagation in the discovery test harness.The mock currently records only
directories, so a regression that dropslimitfromlistExternalSessionscalls would pass unnoticed.Suggested test-hardening diff
-function makeFakeOpenCodeAdapter(input: { - readonly sessions: ReadonlyArray<OpenCodeExternalSessionSummary>; - readonly calls: Array<ReadonlyArray<string>>; -}): OpenCodeAdapterShape { +function makeFakeOpenCodeAdapter(input: { + readonly sessions: ReadonlyArray<OpenCodeExternalSessionSummary>; + readonly calls: Array<{ readonly directories: ReadonlyArray<string>; readonly limit?: number }>; +}): OpenCodeAdapterShape { return { @@ - listExternalSessions: ({ directories }) => + listExternalSessions: ({ directories, limit }) => Effect.sync(() => { - input.calls.push([...directories]); + input.calls.push({ directories: [...directories], limit }); return input.sessions; }), @@ -function makeHarnessLayer(sessions: ReadonlyArray<OpenCodeExternalSessionSummary>) { - const calls: Array<ReadonlyArray<string>> = []; +function makeHarnessLayer(sessions: ReadonlyArray<OpenCodeExternalSessionSummary>) { + const calls: Array<{ readonly directories: ReadonlyArray<string>; readonly limit?: number }> = []; @@ - expect(harness.calls).toEqual([[KNOWN_PROJECT_ROOT]]); + expect(harness.calls).toEqual([ + { directories: [KNOWN_PROJECT_ROOT], limit: 50 }, + ]);Also applies to: 110-112, 268-268
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/provider/Layers/OpenCodeExternalSessionSync.test.ts` around lines 81 - 85, The mock function for listExternalSessions currently only records the directories parameter in input.calls but omits the limit parameter. Update the listExternalSessions mock function at all three locations (lines around 81-85, 110-112, and 268) to also capture and record the limit parameter alongside directories in the input.calls array to ensure that any regression dropping the limit parameter would be detected by the test harness.apps/server/src/provider/Drivers/OpenCodeDriver.ts (1)
213-221: 💤 Low valueInconsistent sort order between
findInstallerManagedOpenCodeRuntimeandselectInstallerManagedOpenCodeVersionDirectory.The sorting logic here sorts in descending order (
right.version, left.version), whileselectInstallerManagedOpenCodeVersionDirectory(lines 126-128) sorts in ascending order and picks the last element. Both achieve "select newest" but via different approaches.This inconsistency could cause confusion for future maintainers. Consider standardizing on one approach.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/provider/Drivers/OpenCodeDriver.ts` around lines 213 - 221, The sorting logic in the current method sorts in descending order to get the newest version, while selectInstallerManagedOpenCodeVersionDirectory sorts in ascending order and picks the last element. Both achieve the same result but with inconsistent approaches. Standardize by making both methods use the same sorting strategy and element selection. Choose either to sort descending and pick the first element (like the current implementation) or sort ascending and pick the last element (like selectInstallerManagedOpenCodeVersionDirectory). Update whichever function is easier to modify to match the other's approach, ensuring consistent logic across the OpenCodeDriver class.apps/server/src/provider/Layers/OpenCodeAdapter.ts (2)
1128-1147: ⚡ Quick winMissing distinct error handling for resumed sessions that no longer exist.
When resuming a session via
session.get, if the session was deleted or doesn't exist, the SDK may return{ data: undefined }or throw. The current error message "OpenCode session.get returned no session payload" is generic.Consider adding a more specific check or error message to help users understand that the resumed session may have been deleted, allowing them to retry without a resume cursor.
💡 Suggested improvement
if (!openCodeSession.data) { return yield* new OpenCodeRuntimeError({ operation: resumeSessionId !== undefined ? "session.get" : "session.create", detail: resumeSessionId !== undefined - ? "OpenCode session.get returned no session payload." + ? `OpenCode session '${resumeSessionId}' not found or was deleted.` : "OpenCode session.create returned no session payload.", }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/provider/Layers/OpenCodeAdapter.ts` around lines 1128 - 1147, The error handling after the openCodeSession retrieval does not distinguish between a failed session.get for a resumed session versus a failed session.create. When resumeSessionId is defined and openCodeSession.data is falsy, add a more specific error message that indicates the resumed session may have been deleted or no longer exists, suggesting the user retry without the resume cursor. Consider adding a separate conditional check for the resumeSessionId !== undefined case before the generic error return in the openCodeSession.data validation block to provide clearer feedback to users attempting to resume sessions that no longer exist.
1450-1497: 💤 Low valueThe session limit is applied per-directory, potentially returning more results than expected.
The
sessionLimitis passed to eachclient.session.listcall per directory. If there are N directories, the total results could be up toN * sessionLimitbefore deduplication. This is likely intentional since sessions are deduplicated bysessionId, but the behavior differs from what a caller might expect if they passlimit: 10expecting at most 10 results.Consider documenting this behavior in the interface or applying a final cap after deduplication if strict limiting is desired.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/provider/Layers/OpenCodeAdapter.ts` around lines 1450 - 1497, The sessionLimit in the listExternalSessions function is applied per-directory when calling client.session.list inside the for loop, which means with N directories you could return up to N*sessionLimit results before deduplication. Either document this per-directory behavior in the interface definition for listExternalSessions to set caller expectations, or apply a final cap by limiting the results returned from the deduplication map (e.g., slice or limit the array returned from [...summaries.values()]) to ensure the total returned results never exceed sessionLimit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/provider/Layers/OpenCodeAdapter.test.ts`:
- Line 362: The test assertion for closeCalls in the concurrent startSession
test expects a single close call, but because each concurrent startSession
creates its own sessionScope with a finalizer registered via
connectToOpenCodeServer, when the race loser's scope is cleaned up at line 1177,
it triggers an additional close call. Update the assertion at line 362 to expect
two close calls ["http://127.0.0.1:9999", "http://127.0.0.1:9999"] to match the
actual behavior from both sessionScope finalizers executing, or alternatively
suppress the loser's scope cleanup if the intended behavior is to only have one
close call.
---
Nitpick comments:
In `@apps/server/src/provider/Drivers/OpenCodeDriver.ts`:
- Around line 213-221: The sorting logic in the current method sorts in
descending order to get the newest version, while
selectInstallerManagedOpenCodeVersionDirectory sorts in ascending order and
picks the last element. Both achieve the same result but with inconsistent
approaches. Standardize by making both methods use the same sorting strategy and
element selection. Choose either to sort descending and pick the first element
(like the current implementation) or sort ascending and pick the last element
(like selectInstallerManagedOpenCodeVersionDirectory). Update whichever function
is easier to modify to match the other's approach, ensuring consistent logic
across the OpenCodeDriver class.
In `@apps/server/src/provider/Layers/OpenCodeAdapter.ts`:
- Around line 1128-1147: The error handling after the openCodeSession retrieval
does not distinguish between a failed session.get for a resumed session versus a
failed session.create. When resumeSessionId is defined and openCodeSession.data
is falsy, add a more specific error message that indicates the resumed session
may have been deleted or no longer exists, suggesting the user retry without the
resume cursor. Consider adding a separate conditional check for the
resumeSessionId !== undefined case before the generic error return in the
openCodeSession.data validation block to provide clearer feedback to users
attempting to resume sessions that no longer exist.
- Around line 1450-1497: The sessionLimit in the listExternalSessions function
is applied per-directory when calling client.session.list inside the for loop,
which means with N directories you could return up to N*sessionLimit results
before deduplication. Either document this per-directory behavior in the
interface definition for listExternalSessions to set caller expectations, or
apply a final cap by limiting the results returned from the deduplication map
(e.g., slice or limit the array returned from [...summaries.values()]) to ensure
the total returned results never exceed sessionLimit.
In `@apps/server/src/provider/Layers/OpenCodeExternalSessionSync.test.ts`:
- Around line 81-85: The mock function for listExternalSessions currently only
records the directories parameter in input.calls but omits the limit parameter.
Update the listExternalSessions mock function at all three locations (lines
around 81-85, 110-112, and 268) to also capture and record the limit parameter
alongside directories in the input.calls array to ensure that any regression
dropping the limit parameter would be detected by the test harness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5efd253b-7ea1-43be-aa96-9f555929adec
📒 Files selected for processing (16)
apps/server/src/provider/Drivers/OpenCodeDriver.test.tsapps/server/src/provider/Drivers/OpenCodeDriver.tsapps/server/src/provider/Layers/OpenCodeAdapter.test.tsapps/server/src/provider/Layers/OpenCodeAdapter.tsapps/server/src/provider/Layers/OpenCodeExternalSessionSync.test.tsapps/server/src/provider/Layers/OpenCodeExternalSessionSync.tsapps/server/src/provider/Layers/ProviderAdapterRegistry.test.tsapps/server/src/provider/Services/OpenCodeAdapter.tsapps/server/src/provider/Services/OpenCodeExternalSessionSync.tsapps/server/src/provider/builtInDrivers.test.tsapps/server/src/provider/opencodeRuntime.test.tsapps/server/src/server.tsapps/server/src/serverRuntimeStartup.tsapps/web/src/firstRunOnboarding.test.tsapps/web/src/providerModels.test.tsscripts/tritongpt-sync-upstream.test.ts
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
dbalders
marked this pull request as ready for review
June 20, 2026 06:36
This was referenced Jun 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
T3CODE_DEV_INSTANCE=opencode-external-thread-sync npm run dev:desktop/Users/davidbalderston/Github/UCSD-Skills-Library; it appeared under the existing T3 project in the UI./node_modules/.bin/vp test run apps/server/src/provider/Layers/OpenCodeAdapter.test.ts apps/server/src/provider/Layers/OpenCodeExternalSessionSync.test.ts apps/server/src/provider/Drivers/OpenCodeDriver.test.ts./node_modules/.bin/vp run --filter t3 typecheck./node_modules/.bin/vp check(passes with the existing 13 lint warnings)/Users/davidbalderston/.agents/skills/autoreview/scripts/autoreview --mode local --parallel-tests './node_modules/.bin/vp test run apps/server/src/provider/Layers/OpenCodeAdapter.test.ts apps/server/src/provider/Layers/OpenCodeExternalSessionSync.test.ts apps/server/src/provider/Drivers/OpenCodeDriver.test.ts && ./node_modules/.bin/vp run --filter t3 typecheck && ./node_modules/.bin/vp check'-> cleanSummary by CodeRabbit
vite-plus/test.