fix: implement code review suggestions

Address all feedback from PR review:
- Lock granularity
- Empty response handling
- Shutdown race condition
- Interface naming
This commit is contained in:
Lixeer 2026-02-24 22:33:04 +08:00
parent 3d605a4f53
commit d09c64fcee
4 changed files with 26 additions and 21 deletions

View file

@ -211,10 +211,10 @@ func gatewayCmd() {
<-sigChan <-sigChan
fmt.Println("\nShutting down...") fmt.Println("\nShutting down...")
cancel() if cp, ok := provider.(providers.StatefulProvider); ok {
if cp, ok := provider.(providers.SessionProvider); ok {
cp.Close() cp.Close()
} }
cancel()
healthServer.Stop(context.Background()) healthServer.Stop(context.Background())
deviceService.Stop() deviceService.Stop()
heartbeatService.Stop() heartbeatService.Stop()

View file

@ -94,19 +94,24 @@ func (p *GitHubCopilotProvider) Chat(
return nil, fmt.Errorf("marshal messages: %w", err) return nil, fmt.Errorf("marshal messages: %w", err)
} }
p.mu.Lock() 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), Prompt: string(fullcontent),
}) })
if err != nil {
return nil, err
}
var content string if resp == nil {
if resp != nil && resp.Data.Content != nil { return nil, fmt.Errorf("empty response from copilot")
content = *resp.Data.Content
} }
if resp.Data.Content == nil {
return nil, fmt.Errorf("no content in copilot response")
}
content := *resp.Data.Content
return &LLMResponse{ return &LLMResponse{
FinishReason: "stop", FinishReason: "stop",

View file

@ -30,7 +30,7 @@ type LLMProvider interface {
GetDefaultModel() string GetDefaultModel() string
} }
type SessionProvider interface { type StatefulProvider interface {
LLMProvider LLMProvider
Close() Close()
} }

View file

@ -14,14 +14,14 @@ import (
type mockRegistryTool struct { type mockRegistryTool struct {
name string name string
desc string desc string
params map[string]any params map[string]interface{}
result *ToolResult result *ToolResult
} }
func (m *mockRegistryTool) Name() string { return m.name } func (m *mockRegistryTool) Name() string { return m.name }
func (m *mockRegistryTool) Description() string { return m.desc } func (m *mockRegistryTool) Description() string { return m.desc }
func (m *mockRegistryTool) Parameters() map[string]any { return m.params } func (m *mockRegistryTool) Parameters() map[string]interface{} { return m.params }
func (m *mockRegistryTool) Execute(_ context.Context, _ map[string]any) *ToolResult { func (m *mockRegistryTool) Execute(_ context.Context, _ map[string]interface{}) *ToolResult {
return m.result return m.result
} }
@ -51,7 +51,7 @@ func newMockTool(name, desc string) *mockRegistryTool {
return &mockRegistryTool{ return &mockRegistryTool{
name: name, name: name,
desc: desc, desc: desc,
params: map[string]any{"type": "object"}, params: map[string]interface{}{"type": "object"},
result: SilentResult("ok"), result: SilentResult("ok"),
} }
} }
@ -109,7 +109,7 @@ func TestToolRegistry_Execute_Success(t *testing.T) {
r.Register(&mockRegistryTool{ r.Register(&mockRegistryTool{
name: "greet", name: "greet",
desc: "says hello", desc: "says hello",
params: map[string]any{}, params: map[string]interface{}{},
result: SilentResult("hello"), result: SilentResult("hello"),
}) })
@ -203,7 +203,7 @@ func TestToolRegistry_GetDefinitions(t *testing.T) {
if defs[0]["type"] != "function" { if defs[0]["type"] != "function" {
t.Errorf("expected type 'function', got %v", defs[0]["type"]) 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 { if !ok {
t.Fatal("expected 'function' key to be a map") 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) { func TestToolRegistry_ToProviderDefs(t *testing.T) {
r := NewToolRegistry() 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{ r.Register(&mockRegistryTool{
name: "beta", name: "beta",
desc: "tool B", desc: "tool B",
@ -310,7 +310,7 @@ func TestToolToSchema(t *testing.T) {
if schema["type"] != "function" { if schema["type"] != "function" {
t.Errorf("expected type 'function', got %v", schema["type"]) 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 { if !ok {
t.Fatal("expected 'function' to be a map") t.Fatal("expected 'function' to be a map")
} }