diff --git a/pkg/providers/factory.go b/pkg/providers/factory.go index 5b3e42b9e..45361bd5f 100644 --- a/pkg/providers/factory.go +++ b/pkg/providers/factory.go @@ -192,12 +192,13 @@ func resolveProviderSelection(cfg *config.Config) (providerSelection, error) { } case "github_copilot", "copilot": sel.providerType = providerTypeGitHubCopilot - if cfg.Providers.GitHubCopilot.APIBase != "" { - sel.apiBase = cfg.Providers.GitHubCopilot.APIBase - } else { + sel.connectMode = cfg.Providers.GitHubCopilot.ConnectMode + sel.apiBase = cfg.Providers.GitHubCopilot.APIBase + // For grpc mode (or default), use localhost:4321 if no address is specified. + // For stdio mode, apiBase is the optional CLI binary path (empty = "copilot" from PATH). + if sel.apiBase == "" && sel.connectMode != "stdio" { sel.apiBase = "localhost:4321" } - sel.connectMode = cfg.Providers.GitHubCopilot.ConnectMode return sel, nil } } diff --git a/pkg/providers/factory_provider.go b/pkg/providers/factory_provider.go index 155317a3b..76967a73d 100644 --- a/pkg/providers/factory_provider.go +++ b/pkg/providers/factory_provider.go @@ -154,15 +154,17 @@ func CreateProviderFromConfig(cfg *config.ModelConfig) (LLMProvider, string, err return NewCodexCliProvider(workspace), modelID, nil case "github-copilot", "copilot": - apiBase := cfg.APIBase - if apiBase == "" { - apiBase = "localhost:4321" - } connectMode := cfg.ConnectMode if connectMode == "" { connectMode = "grpc" } - provider, err := NewGitHubCopilotProvider(apiBase, connectMode, modelID) + uri := cfg.APIBase + // For grpc mode, default to localhost:4321 if no address is specified. + // For stdio mode, uri is the optional CLI binary path (empty = "copilot" from PATH). + if uri == "" && connectMode == "grpc" { + uri = "localhost:4321" + } + provider, err := NewGitHubCopilotProvider(uri, connectMode, modelID) if err != nil { return nil, "", err } diff --git a/pkg/providers/factory_test.go b/pkg/providers/factory_test.go index f7a916d9e..90cdeac15 100644 --- a/pkg/providers/factory_test.go +++ b/pkg/providers/factory_test.go @@ -54,6 +54,34 @@ func TestResolveProviderSelection(t *testing.T) { wantType: providerTypeGitHubCopilot, wantAPIBase: "localhost:4321", }, + { + name: "copilot stdio mode does not set default apiBase", + setup: func(cfg *config.Config) { + cfg.Agents.Defaults.Provider = "copilot" + cfg.Providers.GitHubCopilot.ConnectMode = "stdio" + }, + wantType: providerTypeGitHubCopilot, + }, + { + name: "copilot stdio mode preserves custom cli path", + setup: func(cfg *config.Config) { + cfg.Agents.Defaults.Provider = "copilot" + cfg.Providers.GitHubCopilot.ConnectMode = "stdio" + cfg.Providers.GitHubCopilot.APIBase = "/usr/local/bin/copilot" + }, + wantType: providerTypeGitHubCopilot, + wantAPIBase: "/usr/local/bin/copilot", + }, + { + name: "copilot grpc mode with custom apiBase", + setup: func(cfg *config.Config) { + cfg.Agents.Defaults.Provider = "copilot" + cfg.Providers.GitHubCopilot.ConnectMode = "grpc" + cfg.Providers.GitHubCopilot.APIBase = "myhost:5000" + }, + wantType: providerTypeGitHubCopilot, + wantAPIBase: "myhost:5000", + }, { name: "explicit deepseek provider uses deepseek defaults", setup: func(cfg *config.Config) { diff --git a/pkg/providers/github_copilot_provider.go b/pkg/providers/github_copilot_provider.go index 6d642b2b5..4467545ae 100644 --- a/pkg/providers/github_copilot_provider.go +++ b/pkg/providers/github_copilot_provider.go @@ -9,6 +9,10 @@ import ( copilot "github.com/github/copilot-sdk/go" ) +// GitHubCopilotProvider provides LLM capabilities via the GitHub Copilot SDK. +// It supports two connection modes: +// - "stdio": spawns a local Copilot CLI process and communicates via stdin/stdout (default SDK behavior) +// - "grpc": connects to an external Copilot CLI server over TCP type GitHubCopilotProvider struct { uri string connectMode string // "stdio" or "grpc" @@ -19,45 +23,57 @@ type GitHubCopilotProvider struct { mu sync.Mutex } +// NewGitHubCopilotProvider creates a new GitHub Copilot provider. +// +// Parameters: +// - uri: for "grpc" mode, the address of an external CLI server (e.g. "localhost:4321"); +// for "stdio" mode, the path to the Copilot CLI binary (empty string uses the default "copilot" from PATH) +// - connectMode: "stdio" or "grpc" (defaults to "grpc" if empty) +// - model: the model identifier to use for the session func NewGitHubCopilotProvider(uri string, connectMode string, model string) (*GitHubCopilotProvider, error) { if connectMode == "" { connectMode = "grpc" } + var client *copilot.Client + switch connectMode { case "stdio": - // TODO: Implement stdio mode for GitHub Copilot provider - // See https://github.com/github/copilot-sdk/blob/main/docs/getting-started.md for details - return nil, fmt.Errorf("stdio mode not implemented for GitHub Copilot provider; please use 'grpc' mode instead") + opts := &copilot.ClientOptions{} + if uri != "" { + opts.CLIPath = uri + } + client = copilot.NewClient(opts) case "grpc": - client := copilot.NewClient(&copilot.ClientOptions{ + client = copilot.NewClient(&copilot.ClientOptions{ CLIUrl: uri, }) - if err := client.Start(context.Background()); err != nil { - return nil, fmt.Errorf( - "can't connect to Github Copilot: %w; `https://github.com/github/copilot-sdk/blob/main/docs/getting-started.md#connecting-to-an-external-cli-server` for details", - err, - ) - } - - session, err := client.CreateSession(context.Background(), &copilot.SessionConfig{ - Model: model, - Hooks: &copilot.SessionHooks{}, - }) - if err != nil { - client.Stop() - return nil, fmt.Errorf("create session failed: %w", err) - } - - return &GitHubCopilotProvider{ - uri: uri, - connectMode: connectMode, - client: client, - session: session, - }, nil default: return nil, fmt.Errorf("unknown connect mode: %s", connectMode) } + + if err := client.Start(context.Background()); err != nil { + return nil, fmt.Errorf( + "can't connect to GitHub Copilot (%s mode): %w", + connectMode, err, + ) + } + + session, err := client.CreateSession(context.Background(), &copilot.SessionConfig{ + Model: model, + Hooks: &copilot.SessionHooks{}, + }) + if err != nil { + client.Stop() + return nil, fmt.Errorf("create session failed: %w", err) + } + + return &GitHubCopilotProvider{ + uri: uri, + connectMode: connectMode, + client: client, + session: session, + }, nil } func (p *GitHubCopilotProvider) Close() { diff --git a/pkg/providers/github_copilot_provider_test.go b/pkg/providers/github_copilot_provider_test.go new file mode 100644 index 000000000..05a6228e8 --- /dev/null +++ b/pkg/providers/github_copilot_provider_test.go @@ -0,0 +1,120 @@ +package providers + +import ( + "testing" + + "github.com/sipeed/picoclaw/pkg/config" +) + +func TestNewGitHubCopilotProvider_UnknownConnectMode(t *testing.T) { + _, err := NewGitHubCopilotProvider("localhost:4321", "unknown", "gpt-4.1") + if err == nil { + t.Fatal("expected error for unknown connect mode, got nil") + } + if got := err.Error(); got != "unknown connect mode: unknown" { + t.Fatalf("error = %q, want %q", got, "unknown connect mode: unknown") + } +} + +func TestNewGitHubCopilotProvider_DefaultConnectMode(t *testing.T) { + // When connectMode is empty, it should default to "grpc" and attempt to connect. + // Since there's no server, we expect a connection error (not a "not implemented" error). + _, err := NewGitHubCopilotProvider("localhost:19999", "", "gpt-4.1") + if err == nil { + t.Fatal("expected connection error, got nil") + } + // Should NOT get "not implemented" error — that was the old behavior before stdio was implemented + if got := err.Error(); got == "stdio mode not implemented for GitHub Copilot provider; please use 'grpc' mode instead" { + t.Fatal("got old 'not implemented' error; stdio mode should be implemented now") + } +} + +func TestNewGitHubCopilotProvider_StdioModeAccepted(t *testing.T) { + // Stdio mode should no longer return "not implemented". + // It will fail to start because the copilot CLI binary is likely not installed, + // but the error should be about starting the CLI process, not about the mode + // being unimplemented. + _, err := NewGitHubCopilotProvider("", "stdio", "gpt-4.1") + if err == nil { + // If it succeeds, that's fine too (copilot CLI is installed). + return + } + got := err.Error() + if got == "stdio mode not implemented for GitHub Copilot provider; please use 'grpc' mode instead" { + t.Fatal("got old 'not implemented' error; stdio mode should be implemented now") + } + // Expect a startup/connection error, not an implementation error + t.Logf("expected startup error (copilot CLI likely not installed): %v", err) +} + +func TestNewGitHubCopilotProvider_StdioWithCustomCLIPath(t *testing.T) { + // When a custom CLI path is provided in stdio mode, the error should reference + // the startup failure, not "not implemented". + _, err := NewGitHubCopilotProvider("/nonexistent/copilot", "stdio", "gpt-4.1") + if err == nil { + t.Fatal("expected error for nonexistent CLI path, got nil") + } + got := err.Error() + if got == "stdio mode not implemented for GitHub Copilot provider; please use 'grpc' mode instead" { + t.Fatal("got old 'not implemented' error; stdio mode should be implemented now") + } + t.Logf("got expected startup error: %v", err) +} + +func TestGitHubCopilotProvider_GetDefaultModel(t *testing.T) { + p := &GitHubCopilotProvider{} + if got := p.GetDefaultModel(); got != "gpt-4.1" { + t.Fatalf("GetDefaultModel() = %q, want %q", got, "gpt-4.1") + } +} + +func TestGitHubCopilotProvider_CloseNilClient(t *testing.T) { + // Close on a provider with nil client should not panic + p := &GitHubCopilotProvider{} + p.Close() // should not panic +} + +func TestGitHubCopilotProvider_ChatNilSession(t *testing.T) { + p := &GitHubCopilotProvider{} + _, err := p.Chat(nil, nil, nil, "gpt-4.1", nil) + if err == nil { + t.Fatal("expected error for nil session, got nil") + } + if got := err.Error(); got != "provider closed" { + t.Fatalf("error = %q, want %q", got, "provider closed") + } +} + +func TestCreateProviderFromConfig_CopilotStdioMode(t *testing.T) { + cfg := &config.ModelConfig{ + Model: "copilot/gpt-4.1", + ConnectMode: "stdio", + } + + // This will try to start the copilot CLI which likely isn't installed, + // but the error should be a startup error, not "unknown protocol" or "not implemented". + _, _, err := CreateProviderFromConfig(cfg) + if err == nil { + return // copilot CLI is installed, all good + } + got := err.Error() + if got == "stdio mode not implemented for GitHub Copilot provider; please use 'grpc' mode instead" { + t.Fatal("got old 'not implemented' error; stdio should be supported now") + } + t.Logf("expected startup error: %v", err) +} + +func TestCreateProviderFromConfig_CopilotGrpcModeDefaultAPIBase(t *testing.T) { + cfg := &config.ModelConfig{ + Model: "github-copilot/gpt-4.1", + ConnectMode: "grpc", + } + + // Will fail to connect, but should attempt localhost:4321 + _, _, err := CreateProviderFromConfig(cfg) + if err == nil { + return + } + // The error should mention the connection failure, not "not implemented" + t.Logf("expected connection error: %v", err) +}