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

@@ -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)

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{

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,
 		}
 	}
 

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")
+	}
+}