From a849e02917e6c3c6a52a0e2c27e19552cc91f88d Mon Sep 17 00:00:00 2001 From: Lixeer <1612655510@qq.com> Date: Sun, 22 Feb 2026 22:30:53 +0800 Subject: [PATCH 1/3] fix: better session management for `github_copilot_provider` --- cmd/picoclaw/cmd_gateway.go | 3 + pkg/providers/github_copilot_provider.go | 74 ++++++++++++++++-------- pkg/providers/types.go | 5 ++ 3 files changed, 59 insertions(+), 23 deletions(-) diff --git a/cmd/picoclaw/cmd_gateway.go b/cmd/picoclaw/cmd_gateway.go index 28ef76ad3..30d61aec3 100644 --- a/cmd/picoclaw/cmd_gateway.go +++ b/cmd/picoclaw/cmd_gateway.go @@ -212,6 +212,9 @@ func gatewayCmd() { fmt.Println("\nShutting down...") cancel() + if cp, ok := provider.(providers.SessionProvider); ok { + cp.Close() + } healthServer.Stop(context.Background()) deviceService.Stop() heartbeatService.Stop() diff --git a/pkg/providers/github_copilot_provider.go b/pkg/providers/github_copilot_provider.go index 6124881f7..8131b76fc 100644 --- a/pkg/providers/github_copilot_provider.go +++ b/pkg/providers/github_copilot_provider.go @@ -4,60 +4,75 @@ import ( "context" "encoding/json" "fmt" + "sync" copilot "github.com/github/copilot-sdk/go" ) type GitHubCopilotProvider struct { uri string - connectMode string // `stdio` or `grpc`` + connectMode string // "stdio" or "grpc" + client *copilot.Client session *copilot.Session + + mu sync.Mutex } func NewGitHubCopilotProvider(uri string, connectMode string, model string) (*GitHubCopilotProvider, error) { - var session *copilot.Session if connectMode == "" { connectMode = "grpc" } - switch connectMode { + switch connectMode { case "stdio": - // todo + // TODO: + return nil, fmt.Errorf("stdio mode not implemented") case "grpc": 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, https://github.com/github/copilot-sdk/blob/main/docs/getting-started.md#connecting-to-an-external-cli-server for details", - ) + 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) } - defer client.Stop() - session, _ = client.CreateSession(context.Background(), &copilot.SessionConfig{ + + 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) } - - return &GitHubCopilotProvider{ - uri: uri, - connectMode: connectMode, - session: session, - }, nil } -// Chat sends a chat request to GitHub Copilot -func (p *GitHubCopilotProvider) Chat( - ctx context.Context, messages []Message, tools []ToolDefinition, model string, options map[string]any, -) (*LLMResponse, error) { +func (p *GitHubCopilotProvider) Close() { + p.mu.Lock() + defer p.mu.Unlock() + if p.client != nil { + p.client.Stop() + p.client = nil + p.session = nil + } +} + +func (p *GitHubCopilotProvider) Chat(ctx context.Context, messages []Message, tools []ToolDefinition, model string, options map[string]interface{}) (*LLMResponse, error) { type tempMessage struct { Role string `json:"role"` Content string `json:"content"` } out := make([]tempMessage, 0, len(messages)) - for _, msg := range messages { out = append(out, tempMessage{ Role: msg.Role, @@ -65,18 +80,31 @@ func (p *GitHubCopilotProvider) Chat( }) } - fullcontent, _ := json.Marshal(out) + fullcontent, err := json.Marshal(out) + if err != nil { + return nil, fmt.Errorf("marshal messages: %w", err) + } + p.mu.Lock() + defer p.mu.Unlock() - content, _ := p.session.Send(ctx, copilot.MessageOptions{ + resp, err := p.session.SendAndWait(ctx, copilot.MessageOptions{ Prompt: string(fullcontent), }) + if err != nil { + return nil, err + } + + var content string + if resp != nil && resp.Data.Content != nil { + content = *resp.Data.Content + } return &LLMResponse{ FinishReason: "stop", Content: content, }, nil } - func (p *GitHubCopilotProvider) GetDefaultModel() string { + return "gpt-4.1" } diff --git a/pkg/providers/types.go b/pkg/providers/types.go index f711e7803..40ff6f7c8 100644 --- a/pkg/providers/types.go +++ b/pkg/providers/types.go @@ -30,6 +30,11 @@ type LLMProvider interface { GetDefaultModel() string } +type SessionProvider interface { + LLMProvider + Close() +} + // FailoverReason classifies why an LLM request failed for fallback decisions. type FailoverReason string From 3d605a4f537507713706aa626b74ad8506e6e12a Mon Sep 17 00:00:00 2001 From: Lixeer <1612655510@qq.com> Date: Sun, 22 Feb 2026 23:02:29 +0800 Subject: [PATCH 2/3] fix: run fmt and lint --- pkg/providers/github_copilot_provider.go | 15 ++++++++++++--- pkg/tools/registry_test.go | 20 ++++++++++---------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/pkg/providers/github_copilot_provider.go b/pkg/providers/github_copilot_provider.go index 8131b76fc..c69658b44 100644 --- a/pkg/providers/github_copilot_provider.go +++ b/pkg/providers/github_copilot_provider.go @@ -33,7 +33,10 @@ func NewGitHubCopilotProvider(uri string, connectMode string, model string) (*Gi 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) + 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{ @@ -67,7 +70,13 @@ func (p *GitHubCopilotProvider) Close() { } } -func (p *GitHubCopilotProvider) Chat(ctx context.Context, messages []Message, tools []ToolDefinition, model string, options map[string]interface{}) (*LLMResponse, error) { +func (p *GitHubCopilotProvider) Chat( + ctx context.Context, + messages []Message, + tools []ToolDefinition, + model string, + options map[string]any, +) (*LLMResponse, error) { type tempMessage struct { Role string `json:"role"` Content string `json:"content"` @@ -104,7 +113,7 @@ func (p *GitHubCopilotProvider) Chat(ctx context.Context, messages []Message, to Content: content, }, nil } -func (p *GitHubCopilotProvider) GetDefaultModel() string { +func (p *GitHubCopilotProvider) GetDefaultModel() string { return "gpt-4.1" } diff --git a/pkg/tools/registry_test.go b/pkg/tools/registry_test.go index 33978e543..8ae13b20c 100644 --- a/pkg/tools/registry_test.go +++ b/pkg/tools/registry_test.go @@ -14,14 +14,14 @@ import ( type mockRegistryTool struct { name string desc string - params map[string]interface{} + params map[string]any result *ToolResult } -func (m *mockRegistryTool) Name() string { return m.name } -func (m *mockRegistryTool) Description() string { return m.desc } -func (m *mockRegistryTool) Parameters() map[string]interface{} { return m.params } -func (m *mockRegistryTool) Execute(_ context.Context, _ map[string]interface{}) *ToolResult { +func (m *mockRegistryTool) Name() string { return m.name } +func (m *mockRegistryTool) Description() string { return m.desc } +func (m *mockRegistryTool) Parameters() map[string]any { return m.params } +func (m *mockRegistryTool) Execute(_ context.Context, _ map[string]any) *ToolResult { return m.result } @@ -51,7 +51,7 @@ func newMockTool(name, desc string) *mockRegistryTool { return &mockRegistryTool{ name: name, desc: desc, - params: map[string]interface{}{"type": "object"}, + params: map[string]any{"type": "object"}, result: SilentResult("ok"), } } @@ -109,7 +109,7 @@ func TestToolRegistry_Execute_Success(t *testing.T) { r.Register(&mockRegistryTool{ name: "greet", desc: "says hello", - params: map[string]interface{}{}, + params: map[string]any{}, result: SilentResult("hello"), }) @@ -203,7 +203,7 @@ func TestToolRegistry_GetDefinitions(t *testing.T) { if defs[0]["type"] != "function" { t.Errorf("expected type 'function', got %v", defs[0]["type"]) } - fn, ok := defs[0]["function"].(map[string]interface{}) + fn, ok := defs[0]["function"].(map[string]any) if !ok { t.Fatal("expected 'function' key to be a map") } @@ -217,7 +217,7 @@ func TestToolRegistry_GetDefinitions(t *testing.T) { func TestToolRegistry_ToProviderDefs(t *testing.T) { r := NewToolRegistry() - params := map[string]interface{}{"type": "object", "properties": map[string]interface{}{}} + params := map[string]any{"type": "object", "properties": map[string]any{}} r.Register(&mockRegistryTool{ name: "beta", desc: "tool B", @@ -310,7 +310,7 @@ func TestToolToSchema(t *testing.T) { if schema["type"] != "function" { t.Errorf("expected type 'function', got %v", schema["type"]) } - fn, ok := schema["function"].(map[string]interface{}) + fn, ok := schema["function"].(map[string]any) if !ok { t.Fatal("expected 'function' to be a map") } From d09c64fcee20e9eabb13ca9c8de483747aafa930 Mon Sep 17 00:00:00 2001 From: Lixeer <1612655510@qq.com> Date: Tue, 24 Feb 2026 22:33:04 +0800 Subject: [PATCH 3/3] fix: implement code review suggestions Address all feedback from PR review: - Lock granularity - Empty response handling - Shutdown race condition - Interface naming --- cmd/picoclaw/cmd_gateway.go | 4 ++-- pkg/providers/github_copilot_provider.go | 21 +++++++++++++-------- pkg/providers/types.go | 2 +- pkg/tools/registry_test.go | 20 ++++++++++---------- 4 files changed, 26 insertions(+), 21 deletions(-) diff --git a/cmd/picoclaw/cmd_gateway.go b/cmd/picoclaw/cmd_gateway.go index 30d61aec3..fea569381 100644 --- a/cmd/picoclaw/cmd_gateway.go +++ b/cmd/picoclaw/cmd_gateway.go @@ -211,10 +211,10 @@ func gatewayCmd() { <-sigChan fmt.Println("\nShutting down...") - cancel() - if cp, ok := provider.(providers.SessionProvider); ok { + if cp, ok := provider.(providers.StatefulProvider); ok { cp.Close() } + cancel() healthServer.Stop(context.Background()) deviceService.Stop() heartbeatService.Stop() diff --git a/pkg/providers/github_copilot_provider.go b/pkg/providers/github_copilot_provider.go index c69658b44..9210021e1 100644 --- a/pkg/providers/github_copilot_provider.go +++ b/pkg/providers/github_copilot_provider.go @@ -94,19 +94,24 @@ func (p *GitHubCopilotProvider) Chat( return nil, fmt.Errorf("marshal messages: %w", err) } p.mu.Lock() - defer p.mu.Unlock() + session := p.session + p.mu.Unlock() - resp, err := p.session.SendAndWait(ctx, copilot.MessageOptions{ + if session == nil { + return nil, fmt.Errorf("provider closed") + } + + resp, err := session.SendAndWait(ctx, copilot.MessageOptions{ Prompt: string(fullcontent), }) - if err != nil { - return nil, err - } - var content string - if resp != nil && resp.Data.Content != nil { - content = *resp.Data.Content + if resp == nil { + return nil, fmt.Errorf("empty response from copilot") } + if resp.Data.Content == nil { + return nil, fmt.Errorf("no content in copilot response") + } + content := *resp.Data.Content return &LLMResponse{ FinishReason: "stop", diff --git a/pkg/providers/types.go b/pkg/providers/types.go index 40ff6f7c8..b2dda04a5 100644 --- a/pkg/providers/types.go +++ b/pkg/providers/types.go @@ -30,7 +30,7 @@ type LLMProvider interface { GetDefaultModel() string } -type SessionProvider interface { +type StatefulProvider interface { LLMProvider Close() } diff --git a/pkg/tools/registry_test.go b/pkg/tools/registry_test.go index 8ae13b20c..33978e543 100644 --- a/pkg/tools/registry_test.go +++ b/pkg/tools/registry_test.go @@ -14,14 +14,14 @@ import ( type mockRegistryTool struct { name string desc string - params map[string]any + params map[string]interface{} result *ToolResult } -func (m *mockRegistryTool) Name() string { return m.name } -func (m *mockRegistryTool) Description() string { return m.desc } -func (m *mockRegistryTool) Parameters() map[string]any { return m.params } -func (m *mockRegistryTool) Execute(_ context.Context, _ map[string]any) *ToolResult { +func (m *mockRegistryTool) Name() string { return m.name } +func (m *mockRegistryTool) Description() string { return m.desc } +func (m *mockRegistryTool) Parameters() map[string]interface{} { return m.params } +func (m *mockRegistryTool) Execute(_ context.Context, _ map[string]interface{}) *ToolResult { return m.result } @@ -51,7 +51,7 @@ func newMockTool(name, desc string) *mockRegistryTool { return &mockRegistryTool{ name: name, desc: desc, - params: map[string]any{"type": "object"}, + params: map[string]interface{}{"type": "object"}, result: SilentResult("ok"), } } @@ -109,7 +109,7 @@ func TestToolRegistry_Execute_Success(t *testing.T) { r.Register(&mockRegistryTool{ name: "greet", desc: "says hello", - params: map[string]any{}, + params: map[string]interface{}{}, result: SilentResult("hello"), }) @@ -203,7 +203,7 @@ func TestToolRegistry_GetDefinitions(t *testing.T) { if defs[0]["type"] != "function" { t.Errorf("expected type 'function', got %v", defs[0]["type"]) } - fn, ok := defs[0]["function"].(map[string]any) + fn, ok := defs[0]["function"].(map[string]interface{}) if !ok { t.Fatal("expected 'function' key to be a map") } @@ -217,7 +217,7 @@ func TestToolRegistry_GetDefinitions(t *testing.T) { func TestToolRegistry_ToProviderDefs(t *testing.T) { r := NewToolRegistry() - params := map[string]any{"type": "object", "properties": map[string]any{}} + params := map[string]interface{}{"type": "object", "properties": map[string]interface{}{}} r.Register(&mockRegistryTool{ name: "beta", desc: "tool B", @@ -310,7 +310,7 @@ func TestToolToSchema(t *testing.T) { if schema["type"] != "function" { t.Errorf("expected type 'function', got %v", schema["type"]) } - fn, ok := schema["function"].(map[string]any) + fn, ok := schema["function"].(map[string]interface{}) if !ok { t.Fatal("expected 'function' to be a map") }