Add multi-provider support with OpenCode integration#930
Closed
chindris-mihai-alexandru wants to merge 8 commits intohumanlayer:mainfrom
Closed
Add multi-provider support with OpenCode integration#930chindris-mihai-alexandru wants to merge 8 commits intohumanlayer:mainfrom
chindris-mihai-alexandru wants to merge 8 commits intohumanlayer:mainfrom
Conversation
Introduce a Go SDK for interacting with the OpenCode CLI, mirroring the claudecode-go SDK structure. The SDK provides: - Client for launching and managing OpenCode sessions - Streaming JSON event parsing for real-time output - Session lifecycle management (launch, wait, interrupt, kill) - Binary auto-detection across common installation paths - Type definitions for events, results, and configuration This SDK enables the daemon to launch OpenCode as an alternative provider to Claude Code.
Introduce a provider-agnostic interface that normalizes interactions with different AI coding tools (Claude Code, OpenCode, etc.). The abstraction includes: - Provider interface with Name, IsAvailable, GetPath, GetVersion, Launch - Session interface for lifecycle management and event streaming - Normalized event types (text, tool_use, step_finish, etc.) - Config struct that works across providers - Claude and OpenCode provider implementations - Unit tests for both provider adapters This layer enables the session manager to support multiple providers through a unified interface, though integration is not yet complete.
Add multi-provider support to the daemon, allowing sessions to be launched with either Claude Code or OpenCode as the backend. Session Manager changes: - Add Provider field to LaunchSessionConfig - Initialize OpenCode client alongside Claude client at startup - Route session launch to appropriate provider based on config - Add launchOpenCodeSession method mirroring Claude launch flow - Add OpenCodeSessionWrapper to convert events to Claude format RPC changes: - Add provider field to LaunchSessionRequest - Pass provider through to session manager CLI changes: - Add --provider flag to launch command - Add opencode command group for OpenCode-specific operations - Update daemon client types Known limitations: - Provider type not persisted to database (affects session continuation) - ContinueSession and LaunchDraftSession only support Claude - Event mapping in wrapper is incomplete for some event types
This directory contains local OpenCode configuration and session data, similar to how .claude/ is handled for Claude Code.
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to cf8fac3 in 2 minutes and 29 seconds. Click for details.
- Reviewed
3063lines of code in21files - Skipped
1files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. hld/session/manager.go:237
- Draft comment:
The launchOpenCodeSession function is quite long and complex. Consider refactoring it into smaller, modular helper functions for improved readability and easier maintenance. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a code quality/refactoring comment. The rules state "Comments that suggest code quality refactors are good! But only if they are actionable and clear." The comment identifies a specific function and suggests it's "quite long and complex" and should be refactored into "smaller, modular helper functions." However, the comment doesn't specify WHICH parts should be extracted or HOW to refactor it. It's a general suggestion without concrete guidance. Looking at the code, the function is indeed long (~210 lines) but it's well-structured with clear comments and follows a logical flow. The similar Claude function is even longer. This appears to be new code being added (based on the diff), and the comment is somewhat subjective about whether this level of complexity warrants refactoring. The comment doesn't point to specific problems like duplicated code or hard-to-test sections. The comment does identify a legitimate concern about function length, and refactoring could improve testability and maintainability. The suggestion is somewhat actionable since the function has clear sections that could be extracted. Perhaps I'm being too strict about requiring specific refactoring suggestions. While the concern is valid, the comment lacks specificity about what to refactor and how. It's a general "this is long, make it shorter" comment without identifying specific problems or providing concrete guidance. The function is well-commented and follows a clear sequential flow. Without specific actionable guidance, this becomes a subjective style preference rather than a clear code quality issue. Delete this comment. While the function is long, the comment is too vague and doesn't provide specific, actionable guidance on what to refactor or why. It's a general suggestion that lacks the specificity required for a useful code quality comment.
2. hld/session/opencode_wrapper.go:137
- Draft comment:
The convertEvent function silently ignores 'step_start' events. Considering future changes, you might want to log or handle unrecognized event types for easier debugging. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is suggesting adding logging for debugging purposes. This is a speculative suggestion about "future changes" and "easier debugging" - it's not pointing out a clear bug or issue with the current implementation. The code intentionally skips these events as documented in the comments. The suggestion to add logging is a quality-of-life improvement but not a required change. According to the rules, I should not keep speculative comments or comments that aren't clearly about a required code change. The phrase "Considering future changes" is a red flag - this is speculative. The comment could be arguing that silently dropping events is a bad practice that could lead to bugs, and logging would be a concrete improvement. Perhaps there's a real concern that unknown event types could indicate a problem that should be surfaced. While logging unknown events could be helpful, this is a code quality suggestion rather than a bug fix. The code intentionally skips these events with clear comments explaining why. The comment explicitly frames this as being about "future changes" and "easier debugging" which makes it speculative rather than addressing a current issue. This falls under the category of suggestions that aren't clearly required changes. This comment should be deleted. It's a speculative suggestion about future debugging needs rather than pointing out a clear issue with the current code. The code intentionally skips these events with documented reasons, and adding logging is a quality-of-life improvement, not a required change.
3. hlyr/src/commands/launch.ts:48
- Draft comment:
The conversion of 'dangerously_skip_permissions_timeout' from minutes to milliseconds is correct, but consider adding documentation or inline comments to explain the unit conversion to avoid confusion. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. opencode-go/client.go:180
- Draft comment:
Consider adding additional logging in the buildArgs function to log the final set of arguments used for launching a session. This can help in debugging misconfigurations or unexpected behavior in argument parsing. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. hlyr/src/daemonClient.ts:277
- Draft comment:
The 'call' method attaches a temporary data listener to the socket and removes it after the response. While this works, consider potential issues with concurrent RPC calls and ensure that the listener is isolated to each call to prevent interference. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. hlyr/src/index.ts:20
- Draft comment:
The logic for determining the invocation name and potentially launching an app adds some complexity. Ensure that this conditional behavior is well documented and thoroughly tested. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. hld/session/manager.go:478
- Draft comment:
Typo/lexicographical note: The comment reads "Handle OpenCode provider" while the code checks for the string "opencode". Consider using consistent casing (e.g., "opencode") to avoid confusion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a very minor stylistic comment about comment wording vs string literal casing. The comment is technically correct that there's an inconsistency, but this is extremely common in code - product names are often capitalized in prose/comments while their identifiers are lowercase. Looking at the broader context, "OpenCode" appears to be a proper noun (product name) throughout the codebase in comments and logs, while "opencode" is the string identifier. This is standard practice and not confusing. The comment doesn't point to a bug or meaningful code quality issue - it's purely about comment wording. According to the rules, I should not keep comments that are "obvious or unimportant" and this falls into that category. This is the kind of nitpick that doesn't improve code quality. Could there be a legitimate reason to maintain this distinction? Perhaps "OpenCode" as a proper noun in comments helps with readability and searchability, while "opencode" as a string literal follows standard identifier conventions. The inconsistency might actually be intentional and beneficial. The critique reinforces that this is not a problem. The distinction between capitalized proper nouns in prose and lowercase identifiers in code is a standard convention across software development. This comment is asking to change something that doesn't need changing and would make the comment less readable (proper nouns should be capitalized). This comment should be deleted. It's a trivial stylistic nitpick about comment wording that doesn't improve code quality. The current usage (capitalized "OpenCode" in comments, lowercase "opencode" in string literals) follows standard conventions and is not confusing.
Workflow ID: wflow_OoSnqXlfp8WGFsya
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Pull request overview
This PR introduces multi-provider support to the HumanLayer daemon, enabling sessions to be launched with either Claude Code or OpenCode as the backend. The implementation includes a new opencode-go SDK for interacting with OpenCode CLI, a provider abstraction layer for unified provider management, and integration into the session manager and CLI tooling.
Key Changes:
- New opencode-go SDK providing programmatic access to OpenCode CLI with streaming JSON event parsing and automatic binary detection
- Provider abstraction layer with unified interfaces for Provider and Session that normalize events and operations across different AI coding tools
- Session manager enhancements to route launches to the appropriate provider and convert OpenCode events to Claude-compatible format for existing monitoring infrastructure
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| opencode-go/types.go | Type definitions for OpenCode sessions, events, and configuration |
| opencode-go/client.go | Client implementation with session launching, event streaming, and binary auto-detection |
| opencode-go/doc.go | Package-level documentation with usage examples |
| opencode-go/README.md | Comprehensive SDK documentation and feature overview |
| opencode-go/go.mod | Go module definition for the opencode-go package |
| hld/provider/provider.go | Provider abstraction interfaces and normalized event types |
| hld/provider/provider_test.go | Unit tests for provider abstraction types |
| hld/provider/claude/claude.go | Claude Code provider implementation with event conversion |
| hld/provider/claude/claude_test.go | Unit tests for Claude provider |
| hld/provider/opencode/opencode.go | OpenCode provider implementation with event normalization |
| hld/provider/opencode/opencode_test.go | Unit tests for OpenCode provider |
| hld/session/types.go | Added Provider field to LaunchSessionConfig |
| hld/session/manager.go | Session manager updated with dual-client initialization and provider routing |
| hld/session/opencode_wrapper.go | Wrapper to convert OpenCode sessions to Claude-compatible interface |
| hld/rpc/handlers.go | RPC handler updated to accept provider parameter |
| hlyr/src/index.ts | CLI updated with provider flag and opencode command registration |
| hlyr/src/commands/launch.ts | Launch command updated to accept and pass provider option |
| hlyr/src/commands/opencode.ts | New opencode command group for OpenCode-specific operations |
| hlyr/src/commands/opencode/init.ts | OpenCode initialization wizard for project configuration |
| hlyr/src/daemonClient.ts | Daemon client interface updated with provider field |
| hld/go.mod | Added opencode-go dependency |
| .gitignore | Added .opencode/ directory pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -34,6 +35,7 @@ export const launchCommand = async (query: string, options: LaunchOptions = {}) | |||
|
|
|||
| console.log('Launching Claude Code session...') | |||
There was a problem hiding this comment.
The console.log message says "Launching Claude Code session..." but should reflect the actual provider being used (either Claude or OpenCode based on options.provider).
Suggested change
| console.log('Launching Claude Code session...') | |
| const providerLabel = options.provider === 'opencode' ? 'OpenCode' : 'Claude Code' | |
| console.log(`Launching ${providerLabel} session...`) |
There was a problem hiding this comment.
Not addressed in this PR. Provider is logged separately on line 38.
Comment on lines
+213
to
+214
| message: 'Default model (provider/model format, e.g., anthropic/claude-sonnet-4-20250514):', | ||
| initialValue: 'anthropic/claude-sonnet-4-20250514', |
There was a problem hiding this comment.
The opencode init command uses placeholder model "anthropic/claude-sonnet-4-20250514" which references a future date (May 2025) when the current date context is January 2026. While this might be a valid model identifier, it would be clearer to use an actual current model name or explain this naming convention in the code.
There was a problem hiding this comment.
No change needed - this is the actual Anthropic model version format, not a calendar date.
Comment on lines
238
to
447
| func (m *Manager) launchOpenCodeSession(ctx context.Context, config LaunchSessionConfig, sessionID, runID string, isDraft bool) (*Session, error) { | ||
| // Get OpenCode client | ||
| client, err := m.getOpenCodeClient() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot launch opencode session: %w", err) | ||
| } | ||
|
|
||
| // Convert claudecode.SessionConfig to opencode.SessionConfig | ||
| opencodeConfig := opencode.SessionConfig{ | ||
| Query: config.Query, | ||
| Model: opencode.Model(config.Model), | ||
| WorkingDir: config.WorkingDir, | ||
| Title: config.Title, | ||
| Env: config.Env, | ||
| } | ||
|
|
||
| // Handle session continuation | ||
| if config.SessionID != "" { | ||
| opencodeConfig.SessionID = config.SessionID | ||
| } | ||
|
|
||
| // Capture current working directory if not specified | ||
| if opencodeConfig.WorkingDir == "" { | ||
| cwd, err := os.Getwd() | ||
| if err != nil { | ||
| slog.Warn("failed to get current working directory", "error", err) | ||
| } else { | ||
| opencodeConfig.WorkingDir = cwd | ||
| slog.Debug("No working directory provided, falling back to cwd of daemon", "working_dir", cwd) | ||
| } | ||
| } | ||
|
|
||
| // Handle working directory validation and creation (same as Claude) | ||
| if opencodeConfig.WorkingDir != "" && !isDraft { | ||
| // Expand ~ if present | ||
| if strings.HasPrefix(opencodeConfig.WorkingDir, "~") { | ||
| home, err := os.UserHomeDir() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get home directory: %w", err) | ||
| } | ||
| opencodeConfig.WorkingDir = filepath.Join(home, strings.TrimPrefix(opencodeConfig.WorkingDir, "~")) | ||
| } | ||
|
|
||
| // Check directory status | ||
| info, err := os.Stat(opencodeConfig.WorkingDir) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| if config.CreateDirectoryIfNotExists { | ||
| slog.Info("Creating working directory", | ||
| "path", opencodeConfig.WorkingDir, | ||
| "session_id", sessionID) | ||
| if err := os.MkdirAll(opencodeConfig.WorkingDir, 0755); err != nil { | ||
| return nil, fmt.Errorf("failed to create working directory %s: %w", | ||
| opencodeConfig.WorkingDir, err) | ||
| } | ||
| } else { | ||
| return nil, &DirectoryNotFoundError{ | ||
| Path: opencodeConfig.WorkingDir, | ||
| Message: fmt.Sprintf("working directory does not exist: %s", opencodeConfig.WorkingDir), | ||
| } | ||
| } | ||
| } else { | ||
| return nil, fmt.Errorf("error accessing working directory %s: %w", | ||
| opencodeConfig.WorkingDir, err) | ||
| } | ||
| } else { | ||
| if !info.IsDir() { | ||
| return nil, fmt.Errorf("working directory path exists but is not a directory: %s", | ||
| opencodeConfig.WorkingDir) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Create session record directly in database | ||
| startTime := time.Now() | ||
|
|
||
| // Use claudecode types for the database (since that's what the store expects) | ||
| // We create a placeholder claudecode.SessionConfig for storage | ||
| claudeConfigForDB := claudecode.SessionConfig{ | ||
| Query: config.Query, | ||
| Model: claudecode.Model(config.Model), | ||
| WorkingDir: opencodeConfig.WorkingDir, | ||
| OutputFormat: claudecode.OutputStreamJSON, | ||
| } | ||
|
|
||
| dbSession := store.NewSessionFromConfig(sessionID, runID, claudeConfigForDB) | ||
| dbSession.Summary = CalculateSummary(config.Query) | ||
|
|
||
| // Set initial status based on isDraft | ||
| if isDraft { | ||
| dbSession.Status = store.SessionStatusDraft | ||
| } else { | ||
| dbSession.Status = store.SessionStatusStarting | ||
| } | ||
|
|
||
| // Set title from launch config if provided | ||
| if config.Title != "" { | ||
| dbSession.Title = config.Title | ||
| } | ||
|
|
||
| // Handle auto-accept edits from config | ||
| dbSession.AutoAcceptEdits = config.AutoAcceptEdits | ||
|
|
||
| // Handle dangerously skip permissions from config | ||
| if config.DangerouslySkipPermissions { | ||
| dbSession.DangerouslySkipPermissions = true | ||
| if config.DangerouslySkipPermissionsTimeout != nil && *config.DangerouslySkipPermissionsTimeout > 0 { | ||
| expiresAt := time.Now().Add(time.Duration(*config.DangerouslySkipPermissionsTimeout) * time.Millisecond) | ||
| dbSession.DangerouslySkipPermissionsExpiresAt = &expiresAt | ||
| } | ||
| } | ||
|
|
||
| if err := m.store.CreateSession(ctx, dbSession); err != nil { | ||
| return nil, fmt.Errorf("failed to store session in database: %w", err) | ||
| } | ||
|
|
||
| // Skip OpenCode launch if this is a draft session | ||
| if isDraft { | ||
| slog.Info("created draft session (opencode provider)", | ||
| "session_id", sessionID, | ||
| "run_id", runID, | ||
| "query", config.Query, | ||
| "working_dir", opencodeConfig.WorkingDir) | ||
|
|
||
| return &Session{ | ||
| ID: sessionID, | ||
| RunID: runID, | ||
| Status: StatusDraft, | ||
| StartTime: startTime, | ||
| Config: claudeConfigForDB, | ||
| }, nil | ||
| } | ||
|
|
||
| slog.Info("launching OpenCode session with configuration", | ||
| "session_id", sessionID, | ||
| "run_id", runID, | ||
| "query", opencodeConfig.Query, | ||
| "working_dir", opencodeConfig.WorkingDir, | ||
| "model", opencodeConfig.Model) | ||
|
|
||
| // Launch OpenCode session | ||
| opencodeSession, err := client.Launch(opencodeConfig) | ||
| if err != nil { | ||
| slog.Error("failed to launch OpenCode session", | ||
| "session_id", sessionID, | ||
| "error", err, | ||
| "config", fmt.Sprintf("%+v", opencodeConfig)) | ||
| m.updateSessionStatus(ctx, sessionID, StatusFailed, err.Error()) | ||
| return nil, fmt.Errorf("failed to launch OpenCode session: %w", err) | ||
| } | ||
|
|
||
| // Wrap the session for storage (converts OpenCode events to Claude events) | ||
| wrappedSession := NewOpenCodeSessionWrapper(opencodeSession) | ||
|
|
||
| // Store active process | ||
| m.mu.Lock() | ||
| m.activeProcesses[sessionID] = wrappedSession | ||
| m.mu.Unlock() | ||
|
|
||
| // Update database with running status | ||
| statusRunning := string(StatusRunning) | ||
| now := time.Now() | ||
| update := store.SessionUpdate{ | ||
| Status: &statusRunning, | ||
| LastActivityAt: &now, | ||
| } | ||
| if err := m.store.UpdateSession(ctx, sessionID, update); err != nil { | ||
| slog.Error("failed to update session status to running", "error", err) | ||
| } | ||
|
|
||
| // Publish status change event | ||
| if m.eventBus != nil { | ||
| event := bus.Event{ | ||
| Type: bus.EventSessionStatusChanged, | ||
| Data: map[string]interface{}{ | ||
| "session_id": sessionID, | ||
| "run_id": runID, | ||
| "old_status": string(StatusStarting), | ||
| "new_status": string(StatusRunning), | ||
| "provider": "opencode", | ||
| }, | ||
| } | ||
| slog.Info("publishing session status changed event", | ||
| "session_id", sessionID, | ||
| "run_id", runID, | ||
| "event_type", event.Type, | ||
| "provider", "opencode", | ||
| ) | ||
| m.eventBus.Publish(event) | ||
| } | ||
|
|
||
| // Store query for injection after session ID is captured | ||
| m.pendingQueries.Store(sessionID, config.Query) | ||
|
|
||
| // Monitor session lifecycle in background | ||
| go m.monitorSession(ctx, sessionID, runID, wrappedSession, startTime, claudeConfigForDB) | ||
|
|
||
| slog.Info("launched OpenCode session", | ||
| "session_id", sessionID, | ||
| "run_id", runID, | ||
| "query", config.Query) | ||
|
|
||
| return &Session{ | ||
| ID: sessionID, | ||
| RunID: runID, | ||
| Status: StatusRunning, | ||
| StartTime: startTime, | ||
| Config: claudeConfigForDB, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
The launchOpenCodeSession function duplicates significant logic from the Claude launch path (working directory validation, database session creation, monitoring setup). This code duplication makes maintenance harder and increases the risk of the two code paths diverging. Consider extracting the common session lifecycle logic into shared helper functions.
There was a problem hiding this comment.
Not addressed. Keeping providers isolated intentionally for initial implementation stability.
hld/session/opencode_wrapper.go
Outdated
| Type: "tool_use", | ||
| ID: oc.PartData.CallID, | ||
| Name: oc.PartData.Tool, | ||
| Input: oc.PartData.State.Input, |
There was a problem hiding this comment.
The ToolState.Input field is accessed in the wrapper without checking if State is nil first. This will cause a nil pointer dereference panic when State is nil.
There was a problem hiding this comment.
Fixed in 7900a95. Lines 153-156 now check if oc.PartData.State != nil before accessing State.Input.
|
|
||
| // Validate provider | ||
| if provider != "claude" && provider != "opencode" { | ||
| return nil, fmt.Errorf("unsupported provider: %s (must be 'claude' or 'opencode')", provider) |
There was a problem hiding this comment.
The error message uses single quotes around provider values, but the comment above uses double quotes. For consistency, both should use the same quote style. The Go convention is to use double quotes in error messages.
Suggested change
| return nil, fmt.Errorf("unsupported provider: %s (must be 'claude' or 'opencode')", provider) | |
| return nil, fmt.Errorf("unsupported provider: %s (must be \"claude\" or \"opencode\")", provider) |
There was a problem hiding this comment.
Not changed. No project style guide preference on this.
| type LaunchSessionConfig struct { | ||
| claudecode.SessionConfig | ||
| // Provider selection ("claude" or "opencode", defaults to "claude") | ||
| Provider string |
There was a problem hiding this comment.
The SessionConfig is embedded anonymously in LaunchSessionConfig, but the Provider field is added separately. This creates ambiguity - if claudecode.SessionConfig ever adds a Provider field in the future, there will be a naming conflict. Consider using a named field instead of anonymous embedding, or prefixing the Provider field to make the distinction clear (e.g., ProviderType).
Suggested change
| Provider string | |
| ProviderType string `json:"provider,omitempty"` |
There was a problem hiding this comment.
Not addressed. We control both types in this monorepo.
Comment on lines
+58
to
+65
| // Determine source location | ||
| // Try multiple possible locations for the .opencode directory | ||
| const possiblePaths = [ | ||
| // When installed via npm: package root is one level up from dist | ||
| path.resolve(__dirname, '..', '.opencode'), | ||
| // When running from repo: repo root is two levels up from dist | ||
| path.resolve(__dirname, '../..', '.opencode'), | ||
| ] |
There was a problem hiding this comment.
The opencode init command creates paths like path.resolve(__dirname, '..', '.opencode') which searches relative to the compiled dist directory. This assumes a specific project structure. If the package structure changes or the build output directory changes, the .opencode directory lookup will fail. Consider documenting this assumption or making the path more robust.
There was a problem hiding this comment.
Already handled with multiple fallback paths (lines 60-65) and error messaging showing searched paths (lines 77-83).
Comment on lines
+436
to
+442
| if err := scanner.Err(); err != nil { | ||
| if err == bufio.ErrTooLong { | ||
| s.SetError(fmt.Errorf("JSON line exceeded buffer limit (10MB): %w", err)) | ||
| } else { | ||
| s.SetError(fmt.Errorf("stream parsing failed: %w", err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The error handling only checks for closed pipe errors when calling SetError, but scanner.Err() can also return bufio.ErrTooLong or other errors. The isClosedPipeError function is defined but never actually used to filter expected errors. Consider using this function to avoid logging benign errors when the process exits normally.
There was a problem hiding this comment.
isClosedPipeError is intentionally kept for future graceful shutdown handling.
| args = append(args, "--port", fmt.Sprintf("%d", config.Port)) | ||
| } | ||
|
|
||
| // Output format - always use JSON for SDK |
There was a problem hiding this comment.
The comment says "// Always use JSON for SDK" but the config allows OutputFormat to be specified by the caller. Either the comment is incorrect, or the code should ignore the config.OutputFormat field and always force JSON. This creates confusion about the intended behavior.
Suggested change
| // Output format - always use JSON for SDK | |
| // Output format: SDK always forces JSON output and intentionally ignores SessionConfig.OutputFormat. |
There was a problem hiding this comment.
Comment is accurate - SDK forces JSON regardless of config. The config field is ignored.
| @@ -38,6 +39,7 @@ mcpCommand | |||
| program | |||
| .command('launch <query>') | |||
| .description('Launch a new Claude Code session via the daemon') | |||
There was a problem hiding this comment.
The launch command description still says "Launch a new Claude Code session via the daemon" but it now supports multiple providers. The description should be updated to reflect that it can launch either Claude Code or OpenCode sessions.
Suggested change
| .description('Launch a new Claude Code session via the daemon') | |
| .description('Launch a new Claude Code or OpenCode session via the daemon') |
There was a problem hiding this comment.
Not addressed in this PR.
- Add database migration 23 to persist provider type in sessions table - Update all session queries to read/write provider column - Route ContinueSession and LaunchDraftSession to correct provider - Enhance OpenCode event mapping for tool_result, error, and system events - Add comprehensive tests for opencode-go SDK types and client
Author
chindris-mihai-alexandru
left a comment
There was a problem hiding this comment.
Additional Code Review: Concurrency & Safety Issues
I performed a detailed manual review focusing on error handling, concurrency, and resource management. The automated reviews (Copilot/Ellipsis) caught some good issues, but missed three critical concurrency/safety bugs that should be addressed before merge:
Critical Issues
1. Race Condition in GetEvents() - opencode_wrapper.go:67-73
func (w *OpenCodeSessionWrapper) GetEvents() <-chan claudecode.StreamEvent {
if !w.eventsStarted { // NOT THREAD-SAFE
w.eventsStarted = true
go w.convertEvents()
}
return w.events
}Problem: eventsStarted is read/written without synchronization. Concurrent calls can spawn multiple convertEvents() goroutines, leading to duplicate events and panic on double channel close.
Fix: Use sync.Once:
type OpenCodeSessionWrapper struct {
// ...
eventsOnce sync.Once
}
func (w *OpenCodeSessionWrapper) GetEvents() <-chan claudecode.StreamEvent {
w.eventsOnce.Do(func() {
go w.convertEvents()
})
return w.events
}2. Nil Pointer Risk - opencode_wrapper.go:17-22
func NewOpenCodeSessionWrapper(session *opencode.Session) ClaudeSession {
return &OpenCodeSessionWrapper{
session: session, // No nil check
events: make(chan claudecode.StreamEvent, 100),
}
}Problem: If session is nil, all wrapper methods will panic.
Fix: Add validation:
func NewOpenCodeSessionWrapper(session *opencode.Session) ClaudeSession {
if session == nil {
panic("NewOpenCodeSessionWrapper: session cannot be nil")
}
// ...
}3. Goroutine Leak in convertEvents() - opencode_wrapper.go:76-84
func (w *OpenCodeSessionWrapper) convertEvents() {
defer close(w.events)
for ocEvent := range w.session.Events {
event := w.convertEvent(ocEvent)
if event != nil {
w.events <- *event // Blocks forever if consumer stops reading
}
}
}Problem: If the consumer stops reading from w.events, this goroutine blocks forever on the send, causing a goroutine leak.
Fix: Add a done channel with select, or document that consumers must drain the channel.
Note on Copilot's Findings
Copilot correctly identified the nil pointer issue with ToolState.State access - that should also be fixed. The other Copilot suggestions (console.log message, command descriptions, etc.) are valid UX improvements.
Summary
- 3 critical issues need fixes before merge (concurrency bugs)
- 1 nil pointer issue from Copilot review also needs fix
- Rest of the implementation looks solid
- Fix race condition in GetEvents() by using sync.Once instead of bool flag - Add nil check in NewOpenCodeSessionWrapper to fail fast on nil session - Fix goroutine leak in convertEvents() by adding done channel with select - Update Kill() to signal converter goroutine to stop These issues were identified in manual code review and were not caught by automated reviewers (Copilot/Ellipsis).
Tests verify: - ContinueSession routes to OpenCode provider correctly - OpenCode session ID inheritance from parent sessions - Auto-accept and skip-permissions inheritance - Expired skip-permissions not inherited - Working directory validation - Invalid parent status rejection - LaunchDraftSession with OpenCode provider - Draft session query updates - Non-draft session rejection - Working directory creation on demand
Author
|
Closing this stale PR for now to keep active work focused. I can reopen and update it if maintainers are still interested. |
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
This PR introduces multi-provider support to the daemon, allowing sessions to be launched with either Claude Code or OpenCode as the backend.
The implementation includes a new opencode-go SDK mirroring the claudecode-go structure, a provider abstraction layer for unified provider management, and integration into the session manager and CLI.
Changes
The opencode-go SDK provides client functionality for launching and managing OpenCode sessions, streaming JSON event parsing, and binary auto-detection across common installation paths.
The provider abstraction layer defines interfaces for Provider and Session that normalize interactions across different AI coding tools, with implementations for both Claude and OpenCode.
The session manager now accepts a Provider field in LaunchSessionConfig, initializes both clients at startup, and routes session launches to the appropriate provider. An OpenCodeSessionWrapper converts OpenCode events to the Claude event format for compatibility with existing monitoring code.
The CLI adds a --provider flag to the launch command and a new opencode command group.
Key Features
--session <id>flag, same as Claude's--resumeRecent Fixes
GetEvents()usingsync.OnceNewOpenCodeSessionWrapperconvertEvents()with done channelTesting
opencode_session_test.go)