From a92391635e1856419de6b4631f93996b9ff08136 Mon Sep 17 00:00:00 2001 From: Vernon Stinebaker Date: Fri, 20 Feb 2026 19:23:27 +0800 Subject: [PATCH] fix(pushover): address Copilot review feedback - Register Pushover tool only when channel is enabled - Add 10s timeout to http.Client to prevent indefinite hangs - Fix UTF-8 truncation using runes instead of bytes - Include response body in API error messages for debugging - Add Err field to ToolResult on callback error - Add unit tests for PushoverTool covering all cases --- pkg/agent/loop.go | 20 +++--- pkg/channels/pushover.go | 25 +++++-- pkg/tools/pushover.go | 1 + pkg/tools/pushover_test.go | 130 +++++++++++++++++++++++++++++++++++++ 4 files changed, 162 insertions(+), 14 deletions(-) create mode 100644 pkg/tools/pushover_test.go diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 91a639f42..131e38fb1 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -117,16 +117,18 @@ func registerSharedTools(cfg *config.Config, msgBus *bus.MessageBus, registry *A }) agent.Tools.Register(messageTool) - // Pushover tool - send push notifications - pushoverTool := tools.NewPushoverTool() - pushoverTool.SetPushoverCallback(func(message string) error { - msgBus.PublishOutbound(bus.OutboundMessage{ - Channel: "pushover", - Content: message, + // Pushover tool - send push notifications (only if enabled) + if cfg.Channels.Pushover.Enabled { + pushoverTool := tools.NewPushoverTool() + pushoverTool.SetPushoverCallback(func(message string) error { + msgBus.PublishOutbound(bus.OutboundMessage{ + Channel: "pushover", + Content: message, + }) + return nil }) - return nil - }) - agent.Tools.Register(pushoverTool) + agent.Tools.Register(pushoverTool) + } // Spawn tool with allowlist checker subagentManager := tools.NewSubagentManager(provider, agent.Model, agent.Workspace, msgBus) diff --git a/pkg/channels/pushover.go b/pkg/channels/pushover.go index 231fd616f..b6fb2f2cd 100644 --- a/pkg/channels/pushover.go +++ b/pkg/channels/pushover.go @@ -3,15 +3,19 @@ package channels import ( "context" "fmt" + "io" "net/http" "net/url" "strings" + "time" "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/logger" ) +const pushoverTimeout = 10 * time.Second + type PushoverChannel struct { *BaseChannel config config.PushoverConfig @@ -24,7 +28,9 @@ func NewPushoverChannel(cfg config.PushoverConfig, bus *bus.MessageBus) (*Pushov return &PushoverChannel{ BaseChannel: base, config: cfg, - client: &http.Client{}, + client: &http.Client{ + Timeout: pushoverTimeout, + }, }, nil } @@ -57,10 +63,13 @@ func (c *PushoverChannel) Send(ctx context.Context, msg bus.OutboundMessage) err data.Set("token", c.config.AppToken) data.Set("user", c.config.UserKey) - // Truncate message if too long (Pushover limit is 1024 chars) + // Truncate message if too long (Pushover limit is 1024 characters) + // Use runes to properly handle multi-byte UTF-8 characters message := msg.Content - if len(message) > 1024 { - message = message[:1021] + "..." + runes := []rune(message) + if len(runes) > 1024 { + // Reserve space for "..." so total length does not exceed 1024 characters + message = string(runes[:1021]) + "..." } data.Set("message", message) @@ -78,7 +87,13 @@ func (c *PushoverChannel) Send(ctx context.Context, msg bus.OutboundMessage) err defer resp.Body.Close() if resp.StatusCode != 200 { - return fmt.Errorf("pushover API returned status %d", resp.StatusCode) + // Read response body for debugging info + body, _ := io.ReadAll(resp.Body) + bodyStr := string(body) + if len(bodyStr) > 200 { + bodyStr = bodyStr[:200] + "..." + } + return fmt.Errorf("pushover API returned status %d: %s", resp.StatusCode, bodyStr) } logger.DebugCF("pushover", "Notification sent", map[string]any{ diff --git a/pkg/tools/pushover.go b/pkg/tools/pushover.go index cc4e4bb7f..23916f191 100644 --- a/pkg/tools/pushover.go +++ b/pkg/tools/pushover.go @@ -52,6 +52,7 @@ func (t *PushoverTool) Execute(ctx context.Context, args map[string]interface{}) return &ToolResult{ ForLLM: fmt.Sprintf("failed to send pushover notification: %v", err), IsError: true, + Err: err, } } diff --git a/pkg/tools/pushover_test.go b/pkg/tools/pushover_test.go new file mode 100644 index 000000000..6afc612ac --- /dev/null +++ b/pkg/tools/pushover_test.go @@ -0,0 +1,130 @@ +package tools + +import ( + "context" + "errors" + "testing" +) + +func TestPushoverTool_Execute_Success(t *testing.T) { + tool := NewPushoverTool() + + var sentMessage string + tool.SetPushoverCallback(func(message string) error { + sentMessage = message + return nil + }) + + ctx := context.Background() + args := map[string]interface{}{ + "message": "Test notification", + } + + result := tool.Execute(ctx, args) + + if sentMessage != "Test notification" { + t.Errorf("Expected message 'Test notification', got '%s'", sentMessage) + } + + if result.IsError { + t.Error("Expected IsError=false for successful send") + } + + if result.ForLLM != "Push notification sent: Test notification" { + t.Errorf("Unexpected ForLLM: %s", result.ForLLM) + } +} + +func TestPushoverTool_Execute_MissingMessage(t *testing.T) { + tool := NewPushoverTool() + tool.SetPushoverCallback(func(message string) error { + return nil + }) + + ctx := context.Background() + args := map[string]interface{}{} + + result := tool.Execute(ctx, args) + + if !result.IsError { + t.Error("Expected IsError=true for missing message") + } + + if result.ForLLM != "message is required" { + t.Errorf("Expected 'message is required', got '%s'", result.ForLLM) + } +} + +func TestPushoverTool_Execute_NilCallback(t *testing.T) { + tool := NewPushoverTool() + + ctx := context.Background() + args := map[string]interface{}{ + "message": "Test notification", + } + + result := tool.Execute(ctx, args) + + if !result.IsError { + t.Error("Expected IsError=true for nil callback") + } + + if result.ForLLM != "Pushover not configured" { + t.Errorf("Expected 'Pushover not configured', got '%s'", result.ForLLM) + } +} + +func TestPushoverTool_Execute_CallbackError(t *testing.T) { + tool := NewPushoverTool() + expectedErr := errors.New("pushover API error") + tool.SetPushoverCallback(func(message string) error { + return expectedErr + }) + + ctx := context.Background() + args := map[string]interface{}{ + "message": "Test notification", + } + + result := tool.Execute(ctx, args) + + if !result.IsError { + t.Error("Expected IsError=true for callback error") + } + + if result.Err != expectedErr { + t.Errorf("Expected Err to be '%v', got '%v'", expectedErr, result.Err) + } +} + +func TestPushoverTool_Name(t *testing.T) { + tool := NewPushoverTool() + if tool.Name() != "pushover" { + t.Errorf("Expected name 'pushover', got '%s'", tool.Name()) + } +} + +func TestPushoverTool_Description(t *testing.T) { + tool := NewPushoverTool() + if tool.Description() == "" { + t.Error("Expected non-empty description") + } +} + +func TestPushoverTool_Parameters(t *testing.T) { + tool := NewPushoverTool() + params := tool.Parameters() + + if params["type"] != "object" { + t.Errorf("Expected type 'object', got '%v'", params["type"]) + } + + props, ok := params["properties"].(map[string]interface{}) + if !ok { + t.Fatal("Expected properties to be a map") + } + + if _, exists := props["message"]; !exists { + t.Error("Expected 'message' property to exist") + } +}